[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon@bugs.ecos.sourceware.org]

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 <jifl@ecoscentric.com> ---
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.