From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23969 invoked by alias); 16 Dec 2012 15:51:54 -0000 Received: (qmail 23952 invoked by uid 22791); 16 Dec 2012 15:51:53 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from hagrid.ecoscentric.com (HELO mail.ecoscentric.com) (212.13.207.197) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 16 Dec 2012 15:51:48 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 16B124680007 for ; Sun, 16 Dec 2012 15:51:46 +0000 (GMT) Received: from mail.ecoscentric.com ([127.0.0.1]) by localhost (hagrid.ecoscentric.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Hx0p4RglQtii; Sun, 16 Dec 2012 15:51:40 +0000 (GMT) From: bugzilla-daemon@bugs.ecos.sourceware.org To: ecos-patches@ecos.sourceware.org Subject: [Bug 1001607] Cortex-M4F architectural Floating Point Support X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: eCos X-Bugzilla-Component: Patches and contributions X-Bugzilla-Keywords: X-Bugzilla-Severity: enhancement X-Bugzilla-Who: ilijak@siva.com.mk X-Bugzilla-Status: NEEDINFO X-Bugzilla-Priority: low X-Bugzilla-Assigned-To: jifl@ecoscentric.com X-Bugzilla-Target-Milestone: --- X-Bugzilla-Changed-Fields: In-Reply-To: References: X-Bugzilla-URL: http://bugs.ecos.sourceware.org/ Auto-Submitted: auto-generated Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Date: Sun, 16 Dec 2012 15:51:00 -0000 Message-Id: <20121216155140.D50FD4680003@mail.ecoscentric.com> Mailing-List: contact ecos-patches-help@ecos.sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: ecos-patches-owner@ecos.sourceware.org X-SW-Source: 2012-12/txt/msg00011.txt.bz2 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 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.