This is the mail archive of the
ecos-patches@sourceware.org
mailing list for the eCos project.
[Bug 1001607] Cortex-M4F architectural Floating Point Support
- From: bugzilla-daemon at bugs dot ecos dot sourceware dot org
- To: ecos-patches at ecos dot sourceware dot org
- Date: Wed, 8 Aug 2012 18:14:17 +0100
- Subject: [Bug 1001607] Cortex-M4F architectural Floating Point Support
- Auto-submitted: auto-generated
- References: <bug-1001607-104@http.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 #19 from Ilija Kocho <ilijak@siva.com.mk> 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.