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: Sun, 16 Dec 2012 15:51:40 +0000
- 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 #40 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-16 15:51:37 GMT ---
Hi Jifl
I'm sorry for little delay, but this time you caught me in France :).
(In reply to comment #39)
> Hi Ilija,
>
> Excuse the length of this, since I have a few older things I never commented
> on. On the plus side, we're nearly there! Everything below is easy to sort out
> I think.
Thank you for extended elaboration. It seems that for many issues we have
consensus so they have been snipped out.
> > 18) I added condition to suppress FPU context saving for exceptions when not
> > selected ROM monitor or STUB.
>
> Ah, but in cortexm_fpu.h you define CYGSEM_HAL_DEBUG_FPU if
> CYGSEM_HAL_USE_ROM_MONITOR is set. But I don't think that one should cause it
> to be set. CYGSEM_HAL_ROM_MONITOR yes, but not USE_ROM_MONITOR.
>
OK. Now corrected.
> (In reply to comment #19)
> >>> 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).
>
> I don't see where this happens or did it slip through the net? I would have
> expected the conditional which enables CYGSEM_HAL_DEBUG_FPU to also have
> checked CYGARC_CORTEXM_FPU_EXC_AUTOSAVE.
>
> Incidentally, I think CYGARC_CORTEXM_FPU_EXC_AUTOSAVE should default to
> enabled, partly because of that link you posted in comment #32:
> http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html which means once the FPU
> support is enabled, GCC may start generating code which uses the FPU registers;
Fortunately this has been resolved in non issue after comment 32. And as you
have commented just below (in reply to comment 27).
> but also simply because it's more important to be safe by default. People will
> always be able to disable it.
Provided that no floating point is used in ISR/VSR, there is no danger. I find
it very hard [for me] to imagine application so badly needing floating point in
ISR/DSR. On the other hand enabling the auto saving _does_ use (waste) CPU time
and increase IRQ response time.
I would rather keep this option disabled by default, and those few that may
desperately need it, can easily enable it.
>
>
> (In reply to comment #27)
[snip]
> Joey Ye's reply did also say that whatever he would do would be able to be
> disabled (or maybe wouldn't be enabled by default), e.g. with
> -fno-fpreg-for-nonfpdata. So I don't think we need to worry.
>
> (In reply to comment #33)
[snip]
> > As I have a feeling that we are approaching our goal I would like to do some
> > shaping work. One question is where is best place to put the tests. They stem
> > from kernel tests, should I place them there or under Cortex-M architecture?
>
> Definitely Cortex-M. Then the CDL which provides the list of tests
> (CYGPKG_HAL_CORTEXM_TESTS) can also only build them if CYGPKG_KERNEL is present
> and CYGHWR_HAL_CORTEXM_FPU on (just in case, you can see examples of how to
> create the list of test names around the place in eCos, e.g. libc stdio).
>
> The tests themselves may also need fine-tuning of more detailed configuration
> options and can use CYG_TEST_NA for that if they are not applicable for the
> configuration after all.
I think they should be applicable to all configurations that have enough
memory. They execute without problem on systems without FPU, and at least
thread_switch_fpu. is (IMO) gives interesting comparisons when run with and
without FPU.
>
> (In reply to comment #36)
> > Changes: Default setting now is SOFT FPU. Rationale is that more often people
> > will make integer applications even on systems with FPU. I may be completely
> > wrong so comments are appreciated.
>
> No I think you're right. There are other reasons to use the STM32F4 than the
> FPU, for example.
OK. Then we keep it as is.
>
> Just a couple more little comments:
>
> CYGHWR_HAL_FPV4_SP_D16 should be an option, not a component, and should
> probably be a "calculated 1" bool rather than "flavor none".
>
Yes, it's nicer that way. I tried to make it look little bit more informative:
cdl_option CYGHWR_HAL_FPV4_SP_D16 {
flavor bool
active_if { CYGINT_HAL_FPV4_SP_D16 }
calculated { CYGINT_HAL_FPV4_SP_D16 && CYGHWR_HAL_CORTEXM_FPU }
...
}
Out of topic: One question, just out of curiosity: is there a need in CDL for
cdl_option keyword? Isn't it just a terminal cdl_component?
> In case you want to change your mind about HAL_CORTEXM_FPU_ENABLE/DISABLE being
> macros instead of inline functions, I have belatedly realised it's possible to
> use a GCC attribute: __attribute__((always_inline)). That would allow you to
> keep them as inline functions if you like. I'll leave it up to you if you want
> to go back and change it, given that what's there now works. I'm happy either
> way.
I would not fix something that works, but I think there are some other inline
functions that I could treat.
What about adding
#define CYGBLD_INLINE __attribute__((always_inline))
in infra/current/include/cyg_type.h?
Note: Proposal of the name above instead of CYGBLD_ATTRIB_ALWAYS_INLINE is
intentional.
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.