From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17782 invoked by alias); 8 Aug 2012 17:14:40 -0000 Received: (qmail 17772 invoked by uid 22791); 8 Aug 2012 17:14:37 -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; Wed, 08 Aug 2012 17:14:23 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 4F04B2FB082B for ; Wed, 8 Aug 2012 18:14:22 +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 fRy2r0S8KCM4; Wed, 8 Aug 2012 18:14:17 +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: 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" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Date: Wed, 08 Aug 2012 17:14:00 -0000 Message-Id: <20120808171417.91BA42F78005@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/msg00019.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 #19 from Ilija Kocho 2012-08-08 18:14:13 B= ST --- (In reply to comment #17) > More comments.... >=20 > 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. >=20 > 8) In fpv4_sp_d16.h, hal_cortexm_fpu_enable/disable are still extern inli= ne, > which as I mentioned in comment #9 can be problematic. You are slightly a= t 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 optimizati= on than default. But we can trade them for macros, though I am reluctant. >=20 > 9) CYGARC_CORTEXM_FPU_EXC_AUTOSAVE defaults to off. >=20 > 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) >=20 > // HAL_SavedRegisters entries for floating point registers > // see hal_arch.h for HAL_SavedRegisters definition. >=20 > # define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S \ > cyg_uint32 s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]= ; \ > cyg_uint32 fpscr; \ > cyg_uint32 align; >=20 > is a little better as: >=20 > # 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) >=20 > // HAL_SavedRegisters entries for floating point registers > // see hal_arch.h for HAL_SavedRegisters definition. >=20 > # define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S = \ > cyg_uint32 s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]= ; \ > cyg_uint32 fpscr; = \ > cyg_uint32 align; >=20 > Well, personally I find it easier to read anyway. >=20 > 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. //=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D etc.). I'll try to improve. >=20 > 12) hal_fpu_unenable surely ought to be named just hal_fpu_disable ? Ditt= o all > the other mentions of unenable. Just wondering if you're using it specifi= cally > or just temporarily forgot the word :-). Bear in mind I don't know a sing= le > 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 meani= ng 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 h= ave 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 :). >=20 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) >=20 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. >=20 > 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. >=20 > 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 switc= hing > is enabled. Otherwise let it default to the hal_default_exception_vsr. Th= at's > the only time we need it. OK. >=20 > 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, unl= ess > 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 ena= bled automatic FPU context saving we have no choice.=20 >=20 > 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 wou= ld 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=3D0,offset=3D0; i { > if ( i =3D=3D reg ) > break; > offset +=3D REGSIZE(i); > } > if ( NUMREGS =3D=3D i || REGSIZE(i) =3D=3D 0 ) > return -1; > return offset; > } > since this is only used by the stub, a tiny amount of runtime inefficienc= y is > fine. >=20 I need some time to check this. >=20 > 21) It looks like we still may not agree about whether usage faults shoul= d 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 y= ou > 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 becau= se 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 fault= s. I > guess that if we did do this through hal_default_exception_vsr then it wo= uld > 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 us= ually > 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 fau= lt > exception and lazy FPU context switching? >=20 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: =3D=3D=3D Code snippet =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D 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 =3D basepri stmfd r0!,{r2-r11,lr} // save registers sub r0,#4 mov r4,r0 // R4 =3D saved state pointer bl hal_deliver_usagefault_exception mov r1,r0 // return code mov r0,r4 // R0 =3D 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 =3D=3D=3D=3D=3D=3D Snippet end =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D >=20 > I'm prepared to make many of these changes as it is unfair of me to ask y= ou to > do them all when you have already done so much, but given how much time h= as > passed since this patch, I'm not sure if you have made other changes in t= he > 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 l= ast 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. >=20 > =D0=91=D0=BB=D0=B0=D0=B3=D0=BE=D0=B4=D0=B0=D1=80=D0=B0=D0=BC ! So you do some Macedonian after all :-) Cheers Ilija --=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.