[Bug 1001607] Cortex-M4F architectural Floating Point Support

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

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #22 from Jonathan Larmour <jifl@ecoscentric.com> 2012-08-09 05:03:23 BST ---
(In reply to comment #19)
> (In reply to comment #17)
> > 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.

The alternative is to do some fiddling so that there's a non-inline version
available. I mentioned this in comment #9, and it's something we've done
elsewhere in eCos. Here's what you do in the header:

#ifndef CYGPRI_HAL_CORTEXM_FPU_INLINE
# define CYGPRI_HAL_CORTEXM_FPU_INLINE __externC inline
#endif

CYGPRI_HAL_CORTEXM_FPU_INLINE void hal_cortexm_fpu_enable(void)
{
  ...

and then in *one* real source file, you put the following before any #includes:
#define CYGPRI_HAL_CORTEXM_FPU_INLINE __externC

The only complication is that that file should not be cortexm_fpu.c or
fpv4_sp_d16.c since otherwise their own uses of those inline functions will not
be inlined after all, so it would have to go somewhere else like hal_misc.c.
Which is why I was thinking of just using preprocessor macros which might be
easier overall.

> > 9) CYGARC_CORTEXM_FPU_EXC_AUTOSAVE defaults to off.

Oops, I was going to write stuff here that I ended up writing as point 21. Just
ignore this.

> > 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.

I'm happy with having it as disable. For example, we use the word the same way
with the hal cache and interrupt macros. In English this meaning is ok. Whereas
"unenable" isn't a word :).

> > 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.

:)

[snip] 
> > 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. 

That's fine. If they've enabled CYGARC_CORTEXM_FPU_EXC_AUTOSAVE we can force
this new CDL option on/off (whichever way round it is implemented).

> > 21) [usage fault vsr]
[snip] 
> 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,

Looking at the patch you posted in comment #21, do you need to save r2/r3 at
all? In fact since hal_deliver_usagefault_exception doesn't use its 'regs'
argument, should we drop that and only be saving, I guess, r12 and lr? Although
we have to stash r0 into r4 as before, so that too. All the other registers
should be unaffected by hal_deliver_usagefault_exception, since unlike other
user exception handling by default_exception_vsr, we know what this handler
does. And basepri should also be unaffected I would have thought.

So something like this instead:
        mrs     r0,psp                  // Get process stack
        sub     r1,r0,#(3*4)            // Make space for saved state
        msr     psp,r1                  // Ensure PSP is up to date
        stmfd   r0!,{r4,r12,lr}         // save registers

        mov     r4,r0                   // R4 = saved state pointer

        bl      hal_deliver_usagefault_exception

        mov     r1,r4                   // R1 = state saved across call
        ldmfd   r1!,{r4,r12,lr}         // Restore registers
        msr     psp,r1                  // Restore PSP

        cmp     r0,#0                   // Exception other than FPU?
        bne     hal_default_exception_vsr // Y: - process it
        bx      lr                      // N: Return

This is fewer instructions and fewer memory accesses.

I think with this route then hal_usagefault_exception_vsr and
hal_deliver_usagefault_exception should be renamed to
hal_fpu_usagefault_exception_vsr and hal_deliver_fpu_usagefault_exception
instead. 

> > Благодарам !
> 
> So you do some Macedonian after all :-)

навистина не

Jifl

-- 
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.