From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25574 invoked by alias); 20 Feb 2013 03:21:48 -0000 Received: (qmail 25531 invoked by uid 22791); 20 Feb 2013 03:21:45 -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; Wed, 20 Feb 2013 03:21:36 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 8A10C4680013 for ; Wed, 20 Feb 2013 03:21:35 +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 I311nqiDwO9N; Wed, 20 Feb 2013 03:21:29 +0000 (GMT) From: bugzilla-daemon@bugs.ecos.sourceware.org To: ecos-patches@ecos.sourceware.org Subject: [Bug 1001607] Cortex-M4F architectural Floating Point Support Date: Wed, 20 Feb 2013 03:21:00 -0000 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: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Bugzilla-URL: http://bugs.ecos.sourceware.org/ Auto-Submitted: auto-generated MIME-Version: 1.0 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: 2013-02/txt/msg00041.txt.bz2 Please do not reply to this email, use the link below. http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607 --- Comment #57 from Jonathan Larmour --- Hi Ilija, (In reply to comment #53) > (In reply to comment #52) > > (In reply to comment #45) > > > - fpinttest.c renamed fpinttestf.c. > > > - Added fpinttestf1.c for testing of NONE context switching scheme, as > > > it normally fails fpinttestf.c > > > > In that case fpinttestf.c should use CYG_TEST_NA if it's using the NONE > > context scheme - tests should do the right thing whatever the configuration. > > > > So I suggest adding this to the tests in fpinttestf.c for when to be NA > > (including in the CYG_TEST_NA call itself at the bottom): > > (!defined(CYGHWR_HAL_CORTEXM_FPU) || !defined(CYGHWR_HAL_CORTEXM_FPU_SWITCH_NONE)) > > I don't think we need the CYGHWR_HAL_CORTEXM_FPU test. On the other hand > fprintestsf.c is included in fpinttestf1.c which overrides FP_THREADS_N. > Therefore for the last condition I put: > (!defined(CYGHWR_HAL_CORTEXM_FPU_SWITCH_NONE) || (FP_THREADS_N == 1)) > > FAOD: Since we want to move tests in kernel/tests, we don't have problem > with mentioning CORTEXM macros, do we? Well, we could abstract this out as a separate property using a test-specific option in the kernel CDL. In eCoscentric we do something similar for the except1/kexcept1 tests, although we've never got around to contributing that yet. So instead we could have: (!defined(CYGTST_KERNEL_SKIP_MULTI_THREAD_FP_TEST) || (FP_THREADS_N == 1)) Then in CYGHWR_HAL_CORTEXM_FPU_SWITCH, the cortex-m arch HAL can have: requires { CYGHWR_HAL_CORTEXM_FPU_SWITCH_NONE implies CYGTST_KERNEL_SKIP_MULTI_THREAD_FP_TEST } I have some other comments on the patch for the tests: - In the changelog, "floatn" -> "floating" - fpinttestf1.c says it is fpinttestf.c at the top, and the mention of the NONE switching scheme is a bit Cortex-M specific for something in the kernel. So perhaps some more generic comments could be used instead, or at least qualify it by saying it was written intended for the Cortex-M's NONE switching scheme, but is worth testing elsewhere. - fpinttestf2.c has the same issues, and also: - the RAM size test can be removed now that FP2_COUNT is fixed. - I think the stack sizes seem quite large. At the very least, they should probably use CYGNUM_HAL_STACK_SIZE_TYPICAL as a starting point to allow for variations between architectures. - The stacks should also be defined with CYGBLD_ATTRIB_ALIGN_MAX. - For generic, rather than cortex-m code, I think there needs to be a CYGBLD_ATTRIB_NO_INLINE - Most places I've worked have coding standards that recommend againts this sort of thing: while(ticks == alarm_ticks); Instead I'd recommend: while(ticks == alarm_ticks) CYG_EMPTY_STATEMENT; - In recalc(), you test sizeof(float) != sizeof(cyg_uint32), but the printed message has 2*sizeof(cyg_uint32) - Like the others, fpinttestf3.c also has the issue of mentioning the cortex-m specific NONE switching scheme. - Same issues with stack size+alignment, noinline, recalc. - For thread_switch_fpu.cxx: - the stack size should be CYGNUM_HAL_STACK_SIZE_TYPICAL not CYGNUM_HAL_STACK_SIZE_MINIMUM (I know, this is also a problem in tm_basic). - the stacks need aligned again - you can remove all the clock latency stuff. > > (In reply to comment #50) > > > > > > Here is a patch with CYGARC_CORTEXM_FPU_EXC_AUTOSAVE option removed. > > > CYGARC_CORTEXM_FPU_EXC_AUTOSAVE still lives as a macro defined in > > > fpv4_sp_d16.h. > > > > Hmm, so I've now started wondering about saving the context in lazy mode as > > well. You already set CYGARC_REG_FPU_FPCCR_ASPEN/LSPEN for lazy mode which > > means the CPU will already be doing lazy stacking in interrupt/exception > > handlers. > > > > So in fact, can we just change the define at the top of fpv4_sp_d16.h to: > > > > #if defined CYGHWR_HAL_CORTEXM_FPU_SWITCH_ALL || \ > > defined CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY > > #define CYGARC_CORTEXM_FPU_EXC_AUTOSAVE > > #endif > > > > and then change the CYGHWR_HAL_CORTEXM_FPU_SWITCH_ALL test on line 158 to > > also include LAZY, and then it might just work for lazy mode too? I can't > > see anything else in the way. > > Provided that that LAZY uses FPU enabled/disbled state in order to > distinguish between _FP_ and _INT_ threads, suppose that _INT_ thread is > interrupted by _FP_ ISR. Then Usage Fault VSR will enable FPU and FPU will > remain enabled after ISR returns [in thread context], effectively converting > the tread to _FP_. Gradually, this ISR "_FP_ missioner" ISR may convert all > threads to _FP_ so we're not lazy any more. Yes you're right of course. But I can't help feel it wouldn't be difficult to fix this in the exception and interrupt vsrs - e.g. set HAL_SAVEDREGISTERS_WITH_FPU as the saved register type if the FPU enabled bit is set in FPU_CPACR on entry to the exception or interrupt VSR, and then ensure the FPU is enabled/disabled accordingly before exit. I just get the feeling this should be able to be solved with little overhead. That said, I don't want to hold up this patch now because of any potential improvement here. > There are also other problems, related to variable ISR stack frame length, > that are described in that comment #42 and/or comment #47. I'm not sure those are big issues - or at least, there has to be an acceptance that lazy mode can have a bit of overhead. But again, let's leave for now. > [snip] > > > > > BTW don't forget at some point to either tidy up the indentations in the > > CDL, or let me know and I will do so after check-in - you've done enough > > after all! > > I take it that we are a step or less to check in :) Yes! In fact I have no problems with the FP patch 130210 and as far as I'm concerned that patch can be committed. However I do have an issue with your changes for the code build flag which you added in comment #56, which is that this will also prevent use of the DSP instructions by GCC I believe? Also of course as mentioned above, we still need to work out the last few niggles with those kernel tests. > I am trying to keep the code tidy as we apporach end of the pipeline, but > sometimes my editor tries to play smart and doesn't agree with me. I'll > check it before check-in, but if you are not happy you are welcome to edit. Yes that's fine, I'll get anything I see cleaned up, so don't worry about it too much. > Speaking of CDL cross check, I would ask you is to make language and clarity > check on larger descrioptions, especially CYGHWR_HAL_CORTEXM_FPU_SWITCH. In fact, I've got a willing lackey here (he'll hate me saying that ;-)) who will be able to help clean up that and few other Cortex-M and, for that matter, Kinetis related CDL. I'll put some patches in bugzilla soon for you to look at. Jifl -- You are receiving this mail because: You are on the CC list for the bug.