From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2355 invoked by alias); 9 Aug 2012 04:03:57 -0000 Received: (qmail 2346 invoked by uid 22791); 9 Aug 2012 04:03:56 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,TW_DM 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; Thu, 09 Aug 2012 04:03:42 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id CA28F2F78009 for ; Thu, 9 Aug 2012 05:03:40 +0100 (BST) 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 O4DQ7daJ1E2P; Thu, 9 Aug 2012 05:03:38 +0100 (BST) 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: jifl@ecoscentric.com 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" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Date: Thu, 09 Aug 2012 04:03:00 -0000 Message-Id: <20120809040338.8AA982F78005@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-08/txt/msg00022.txt.bz2 Please do not reply to this email. Use the web interface provided at: http://bugs.ecos.sourceware.org/show_bug.cgi?id=3D1001607 --- Comment #22 from Jonathan Larmour 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 in= line, > > 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. >=20 > I haven't experienced problems so far, but I haven't tried other optimiza= tion > 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 #inclu= des: #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 ? Di= tto all > > the other mentions of unenable. Just wondering if you're using it speci= fically > > or just temporarily forgot the word :-). Bear in mind I don't know a si= ngle > > word of Macedonian so I'm grateful your English is so good!. >=20 > My English is less than perfect but here it is not a lost memory. The mea= ning > 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. Whe= reas "unenable" isn't a word :). > > 13) In fpv4_sp_d16.h the uses of __regs_p in HAL_THREAD_INIT_FPU_REGS a= nd > > _CONTEXT still need brackets for safety :). >=20 > One reason why I prefer functions to macros. :) [snip]=20 > > 18) Before, I had mentioned about saving FP context with interrupts and > > exceptions. I was making two observations there, one was about interrup= ts > > (thanks for sorting that out with LSPEN/ASPEN) but the other is that, u= nless > > 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. >=20 > It is only possible when automatic FPU context saving is disabled. With e= nabled > automatic FPU context saving we have no choice.=20 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]=20 > It is possible to change hal_deliver_usagefault_exception to return a val= ue > 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? Alth= ough 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 =3D saved state pointer bl hal_deliver_usagefault_exception mov r1,r4 // R1 =3D 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.=20 > > =D0=91=D0=BB=D0=B0=D0=B3=D0=BE=D0=B4=D0=B0=D1=80=D0=B0=D0=BC ! >=20 > So you do some Macedonian after all :-) =D0=BD=D0=B0=D0=B2=D0=B8=D1=81=D1=82=D0=B8=D0=BD=D0=B0 =D0=BD=D0=B5 Jifl --=20 Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=3Demail ------- You are receiving this mail because: ------- You are on the CC list for the bug.