Please do not reply to this email, use the link below. http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607
--- Comment #57 from Jonathan Larmour <[email protected]> --- Hi Ilija, (In reply to comment #53) > (In reply to comment #52) > > (In reply to comment #45) > > > - fpinttest.c renamed fpinttestf.c. > > > - Added fpinttestf1.c for testing of NONE context switching scheme, as > > > it normally fails fpinttestf.c > > > > In that case fpinttestf.c should use CYG_TEST_NA if it's using the NONE > > context scheme - tests should do the right thing whatever the configuration. > > > > So I suggest adding this to the tests in fpinttestf.c for when to be NA > > (including in the CYG_TEST_NA call itself at the bottom): > > (!defined(CYGHWR_HAL_CORTEXM_FPU) || > > !defined(CYGHWR_HAL_CORTEXM_FPU_SWITCH_NONE)) > > I don't think we need the CYGHWR_HAL_CORTEXM_FPU test. On the other hand > fprintestsf.c is included in fpinttestf1.c which overrides FP_THREADS_N. > Therefore for the last condition I put: > (!defined(CYGHWR_HAL_CORTEXM_FPU_SWITCH_NONE) || (FP_THREADS_N == 1)) > > FAOD: Since we want to move tests in kernel/tests, we don't have problem > with mentioning CORTEXM macros, do we? Well, we could abstract this out as a separate property using a test-specific option in the kernel CDL. In eCoscentric we do something similar for the except1/kexcept1 tests, although we've never got around to contributing that yet. So instead we could have: (!defined(CYGTST_KERNEL_SKIP_MULTI_THREAD_FP_TEST) || (FP_THREADS_N == 1)) Then in CYGHWR_HAL_CORTEXM_FPU_SWITCH, the cortex-m arch HAL can have: requires { CYGHWR_HAL_CORTEXM_FPU_SWITCH_NONE implies CYGTST_KERNEL_SKIP_MULTI_THREAD_FP_TEST } I have some other comments on the patch for the tests: - In the changelog, "floatn" -> "floating" - fpinttestf1.c says it is fpinttestf.c at the top, and the mention of the NONE switching scheme is a bit Cortex-M specific for something in the kernel. So perhaps some more generic comments could be used instead, or at least qualify it by saying it was written intended for the Cortex-M's NONE switching scheme, but is worth testing elsewhere. - fpinttestf2.c has the same issues, and also: - the RAM size test can be removed now that FP2_COUNT is fixed. - I think the stack sizes seem quite large. At the very least, they should probably use CYGNUM_HAL_STACK_SIZE_TYPICAL as a starting point to allow for variations between architectures. - The stacks should also be defined with CYGBLD_ATTRIB_ALIGN_MAX. - For generic, rather than cortex-m code, I think there needs to be a CYGBLD_ATTRIB_NO_INLINE - Most places I've worked have coding standards that recommend againts this sort of thing: while(ticks == alarm_ticks); Instead I'd recommend: while(ticks == alarm_ticks) CYG_EMPTY_STATEMENT; - In recalc(), you test sizeof(float) != sizeof(cyg_uint32), but the printed message has 2*sizeof(cyg_uint32) - Like the others, fpinttestf3.c also has the issue of mentioning the cortex-m specific NONE switching scheme. - Same issues with stack size+alignment, noinline, recalc. - For thread_switch_fpu.cxx: - the stack size should be CYGNUM_HAL_STACK_SIZE_TYPICAL not CYGNUM_HAL_STACK_SIZE_MINIMUM (I know, this is also a problem in tm_basic). - the stacks need aligned again - you can remove all the clock latency stuff. > > (In reply to comment #50) > > > > > > Here is a patch with CYGARC_CORTEXM_FPU_EXC_AUTOSAVE option removed. > > > CYGARC_CORTEXM_FPU_EXC_AUTOSAVE still lives as a macro defined in > > > fpv4_sp_d16.h. > > > > Hmm, so I've now started wondering about saving the context in lazy mode as > > well. You already set CYGARC_REG_FPU_FPCCR_ASPEN/LSPEN for lazy mode which > > means the CPU will already be doing lazy stacking in interrupt/exception > > handlers. > > > > So in fact, can we just change the define at the top of fpv4_sp_d16.h to: > > > > #if defined CYGHWR_HAL_CORTEXM_FPU_SWITCH_ALL || \ > > defined CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY > > #define CYGARC_CORTEXM_FPU_EXC_AUTOSAVE > > #endif > > > > and then change the CYGHWR_HAL_CORTEXM_FPU_SWITCH_ALL test on line 158 to > > also include LAZY, and then it might just work for lazy mode too? I can't > > see anything else in the way. > > Provided that that LAZY uses FPU enabled/disbled state in order to > distinguish between _FP_ and _INT_ threads, suppose that _INT_ thread is > interrupted by _FP_ ISR. Then Usage Fault VSR will enable FPU and FPU will > remain enabled after ISR returns [in thread context], effectively converting > the tread to _FP_. Gradually, this ISR "_FP_ missioner" ISR may convert all > threads to _FP_ so we're not lazy any more. Yes you're right of course. But I can't help feel it wouldn't be difficult to fix this in the exception and interrupt vsrs - e.g. set HAL_SAVEDREGISTERS_WITH_FPU as the saved register type if the FPU enabled bit is set in FPU_CPACR on entry to the exception or interrupt VSR, and then ensure the FPU is enabled/disabled accordingly before exit. I just get the feeling this should be able to be solved with little overhead. That said, I don't want to hold up this patch now because of any potential improvement here. > There are also other problems, related to variable ISR stack frame length, > that are described in that comment #42 and/or comment #47. I'm not sure those are big issues - or at least, there has to be an acceptance that lazy mode can have a bit of overhead. But again, let's leave for now. > [snip] > > > > > BTW don't forget at some point to either tidy up the indentations in the > > CDL, or let me know and I will do so after check-in - you've done enough > > after all! > > I take it that we are a step or less to check in :) Yes! In fact I have no problems with the FP patch 130210 and as far as I'm concerned that patch can be committed. However I do have an issue with your changes for the code build flag which you added in comment #56, which is that this will also prevent use of the DSP instructions by GCC I believe? Also of course as mentioned above, we still need to work out the last few niggles with those kernel tests. > I am trying to keep the code tidy as we apporach end of the pipeline, but > sometimes my editor tries to play smart and doesn't agree with me. I'll > check it before check-in, but if you are not happy you are welcome to edit. Yes that's fine, I'll get anything I see cleaned up, so don't worry about it too much. > Speaking of CDL cross check, I would ask you is to make language and clarity > check on larger descrioptions, especially CYGHWR_HAL_CORTEXM_FPU_SWITCH. In fact, I've got a willing lackey here (he'll hate me saying that ;-)) who will be able to help clean up that and few other Cortex-M and, for that matter, Kinetis related CDL. I'll put some patches in bugzilla soon for you to look at. Jifl -- You are receiving this mail because: You are on the CC list for the bug.
