From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28118 invoked by alias); 7 Aug 2012 05:57:28 -0000 Received: (qmail 28102 invoked by uid 22791); 7 Aug 2012 05:57:27 -0000 X-SWARE-Spam-Status: No, hits=-2.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED 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; Tue, 07 Aug 2012 05:57:14 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 2561E2F7800B for ; Tue, 7 Aug 2012 06:57:13 +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 XQ0rGyq7wXmX; Tue, 7 Aug 2012 06:57:11 +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: Tue, 07 Aug 2012 05:57:00 -0000 Message-Id: <20120807055711.818FD2F78004@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/msg00003.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 #17 from Jonathan Larmour 2012-08-07 06:= 57:07 BST --- More comments.... 7) There's a few more things from cortexm_stub.h which you put in fpv4_sp_d= 16.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. 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. 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. //=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.). 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 specifica= lly 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!. 13) In fpv4_sp_d16.h the uses of __regs_p in HAL_THREAD_INIT_FPU_REGS and _CONTEXT still need brackets for safety :). 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) 15) In hal_arch.inc, hal_fpu_enable and _disable should use CYGARC_REG_FPU_CPACR_ENABLE to be consistent 16) The banner at the top of hal_arch.inc isn't right (it says vectors.S) a= nd the description of cortexm_fpu.c could be better - I think that was just ta= ken from vectors.S as well. 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 switchi= ng is enabled. Otherwise let it default to the hal_default_exception_vsr. That= 's the only time we need it. 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. 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=3D0,offset=3D0; i