Please do not reply to this email. Use the web interface provided at: http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607
--- Comment #19 from Ilija Kocho <[email protected]> 2012-08-08 18:14:13 BST --- (In reply to comment #17) > More comments.... > > 7) There's a few more things from cortexm_stub.h which you put in > fpv4_sp_d16.h > which I don't think need to be: HAL_STUB_REGISTERS_SIZE, PS_N/Z/C/V, the > definition of target_register_t and TARGET_HAS_LARGE_REGISTERS. These > definitions will be the same everywhere in Cortex-M so can just live once in > cortexm_stub.h. OK. > > 8) In fpv4_sp_d16.h, hal_cortexm_fpu_enable/disable are still extern inline, > which as I mentioned in comment #9 can be problematic. You are slightly at > risk > of link errors depending on the compiler optimisations. So these should be > HAL_CORTEXM_FPU_ENABLE/DISABLE preprocessor macros instead. I haven't experienced problems so far, but I haven't tried other optimization than default. But we can trade them for macros, though I am reluctant. > > 9) CYGARC_CORTEXM_FPU_EXC_AUTOSAVE defaults to off. > > 10) A bit like my previous comment about lining up things in CDL, lining up > some things here can be clearer too. For example: > # define HAL_SAVEDREG_MAN_EXCEPTION_FPU_N 16 > # define HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N 16 > # define HAL_SAVEDREG_AUTO_FRAME_SIZE (8*4 + 16*4 + 4 + 4) > > // HAL_SavedRegisters entries for floating point registers > // see hal_arch.h for HAL_SavedRegisters definition. > > # define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S \ > cyg_uint32 s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]; \ > cyg_uint32 fpscr; \ > cyg_uint32 align; > > is a little better as: > > # define HAL_SAVEDREG_MAN_EXCEPTION_FPU_N 16 > # define HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N 16 > # define HAL_SAVEDREG_AUTO_FRAME_SIZE (8*4 + 16*4 + 4 + 4) > > // HAL_SavedRegisters entries for floating point registers > // see hal_arch.h for HAL_SavedRegisters definition. > > # define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S \ > cyg_uint32 s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]; \ > cyg_uint32 fpscr; \ > cyg_uint32 align; > > Well, personally I find it easier to read anyway. > > 11) hal_arch.inc: The line indentation isn't very consistent through this, > including of comments. It may also be helpful to put a comment separator > between the two halves (i.e. //=========================== etc.). I'll try to improve. > > 12) hal_fpu_unenable surely ought to be named just hal_fpu_disable ? Ditto all > the other mentions of unenable. Just wondering if you're using it specifically > or just temporarily forgot the word :-). Bear in mind I don't know a single > word of Macedonian so I'm grateful your English is so good!. > My English is less than perfect but here it is not a lost memory. The meaning is (intended to be), as stated in the comment just below the line _undo_last_enable_. It is not equivalent to disable because the FPU might have been enabled already. > 13) In fpv4_sp_d16.h the uses of __regs_p in HAL_THREAD_INIT_FPU_REGS and > _CONTEXT still need brackets for safety :). > One reason why I prefer functions to macros. > 14) In hal_arch.h, CYGNUM_HAL_STACK_CONTEXT_SIZE still needs abstracting so > that different FPUs can have different sizes (as per comment #9) > OK. Then it (or override) shall be defined in fpv4_sp_d16.h > 15) In hal_arch.inc, hal_fpu_enable and _disable should use > CYGARC_REG_FPU_CPACR_ENABLE to be consistent OK. There are some other places as well. > > 16) The banner at the top of hal_arch.inc isn't right (it says vectors.S) and > the description of cortexm_fpu.c could be better - I think that was just taken > from vectors.S as well. Yes, it is. > > 17) As mentioned in comment #9, I think all the usage fault processing in > vectors.S and hal_misc.c should only be present if lazy FPU context switching > is enabled. Otherwise let it default to the hal_default_exception_vsr. That's > the only time we need it. OK. > > 18) Before, I had mentioned about saving FP context with interrupts and > exceptions. I was making two observations there, one was about interrupts > (thanks for sorting that out with LSPEN/ASPEN) but the other is that, unless > the user explicitly requests it, we should only save the FP state for > exceptions if we're a ROM monitor or have GDB stubs. At the moment they're > always saved, which I think is unnecessary for most people's production > applications. So I think we need an option just to cover the > hal_fpu_exc_push/pop calls in hal_default_exception_vsr. It is only possible when automatic FPU context saving is disabled. With enabled automatic FPU context saving we have no choice. > > 19) reg_offset() in cortexm_stub.c still has a diag_printf. It also would be > improved with some abstraction. I _think_ the following untested code would do > it and then we wouldn't need two different versions at all: > static int > reg_offset(regnames_t reg) > { > int i, offset; > for (i=0,offset=0; i<NUMREGS; i++) > { > if ( i == reg ) > break; > offset += REGSIZE(i); > } > if ( NUMREGS == i || REGSIZE(i) == 0 ) > return -1; > return offset; > } > since this is only used by the stub, a tiny amount of runtime inefficiency is > fine. > I need some time to check this. > > 21) It looks like we still may not agree about whether usage faults should > have > their own exception vsr or not. As I said they are only used with lazy FPU > handling and at most once per thread, so this is not performance critical > code. > Code size might be critical though. But the biggest problem is that, as you > say, at present the hal_usagefault_exception_vsr has a different > prologue/epilogue to hal_default_exception_vsr. That means the user can't > debug > any usage faults such as illegal instructions or unaligned accesses because > the > stub won't be entered. Whether with a separate vsr or not, I think it is > important we have a solution that still allows users to debug usage faults. I > guess that if we did do this through hal_default_exception_vsr then it would > mean having to test whether the FPU was enabled or not before attempting to > save the context. On the other hand, exceptions are of course rare and usually > not something programs rely on for normal operation (only fatal errors > usually), so performance probably isn't critical either. Can you think of any > other way to allow usage faults to be debuggable when using the usage fault > exception and lazy FPU context switching? > It is possible to change hal_deliver_usagefault_exception to return a value that will be !0 if there are still exceptions to process. Then hal_usagefault_exception_vsr can be tweaked to jump into hal_default_exception_vsr. I am testing this and am going to post a patch later, here is a snippet: === Code snippet ================= hal_usagefault_exception_vsr: mrs r0,psp // Get process stack sub r1,r0,#(12*4) // Make space for saved state msr psp,r1 // Ensure PSP is up to date mrs r3,basepri // R3 = basepri stmfd r0!,{r2-r11,lr} // save registers sub r0,#4 mov r4,r0 // R4 = saved state pointer bl hal_deliver_usagefault_exception mov r1,r0 // return code mov r0,r4 // R0 = state saved across call add r0,#4 ldmfd r0!,{r2-r11,lr} // Restore registers msr psp,r0 // Restore PSP msr basepri,r3 // Restore basepri cmp r1,#0 // Exc. other than FPU? bne hal_default_exception_vsr // Yes - process it bx lr // Not: Return ====== Snippet end ==================== > > I'm prepared to make many of these changes as it is unfair of me to ask you to > do them all when you have already done so much, but given how much time has > passed since this patch, I'm not sure if you have made other changes in the > meantime, so let me know if you want me to make changes, so we can avoid > clashes and having to merge. If you have time that would be helpful, especially with CDL, but before we jump to coding let's discuss it. I haven't changed anything since I posted the last patch as I expected your comments. Now I'm testing the usagefault tweak and I plan to post a small incremental patch later today or tomorrow. > > Благодарам ! So you do some Macedonian after all :-) Cheers Ilija -- Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug.
