From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27718 invoked by alias); 7 Aug 2012 05:56:52 -0000 Received: (qmail 27706 invoked by uid 22791); 7 Aug 2012 05:56:50 -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:56:37 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 77A352F7800B for ; Tue, 7 Aug 2012 06:56:35 +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 nBtlrWLteZY8; Tue, 7 Aug 2012 06:56:16 +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" MIME-Version: 1.0 Date: Tue, 07 Aug 2012 05:56:00 -0000 Message-Id: <20120807055616.5B1B42F78004@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/msg00002.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 #16 from Jonathan Larmour 2012-08-07 06:56:01 BST --- Hi Ilija, I'm finally in a position to look at this again. Sorry it's taken a while - things kept cropping up. I won't quote everything otherwise this will just be too big! Thanks for the changes you made. I've had a good look at them and I'll go through some comments I have. I'll just tackle CDL first and make a separate comment for the rest. I'll number them this time so it makes it easier to refer to them rather than do all the quoting. 1) About the CDL and code generation, first off, I have looked into the configuration tool and have had a stab at fixing it so that it could correctly handle the conflict resolution needed for my version of the CDL to fix the issue you describe in comment #11, but I'm afraid I just got myself into a tangled mess. I will make a separate bug report about this issue as it ought to be fixed some time, but I'm going to have to give up. I've looked at your changes, and I'm afraid I'm not very keen on options called "Use Cortex-M4 code radio buttons" - content should not make assumptions about presentation (even though obviously it's something we like to do). This would be particularly odd to someone looking at the ecos.ecc file. I also had a go at trying to work around the issues with simpler CDL, and I think we could do something with a CYGBLD_ARCH_XFLAGS option which would be set by the architecture HAL, and would be explicitly placed in CYGBLD_GLOBAL_CFLAGS in each platform HAL instead of the platform HAL putting in its own -mcpu=... etc. flags. However taking a step back, I think we're going down a dead end route, creating lots of complicated CDL when really this is a temporary problem, if it's even a problem at all. It's true that anyone using Cortex-M4 would either need gcc 4.6.3 or need to use -mcpu=cortex-m3 with no FP. But it's not like we haven't got those tools. Anyone who is interested could try them - they may not be mature, but neither is the eCos floating point support at this point anyway, so anyone who is looking for solid stable code should probably wait longer for the dust to settle in any case. FP support is a new feature, so it's not like we have to be concerned about backward compatibility. Plus once we have all the new code generation related options in the CDL it's very hard to remove them. For example removing even one option causes problems for anyone with a .ecc file (the tools complain it contains options which do not exist). So I think I'm now increasingly of the opinion that we should just drop all the CYGOPT_HAL_CORTEXM4_CODE and assorted complexity entirely. And essentially assume the new tools are available. It's also true that it would be good for the CDL to be checking that the compile flags match the CDL (i.e. Cortex-M4 in the CDL means -mcpu=cortex-m4 and so on). So rather than having a temporary change which involves many extra CDL options, the part we are making temporary is the bit that says: requires { ("CYGHWR_HAL_CORTEXM == "M4") implies is_substr(!is_substr(CYGBLD_GLOBAL_CFLAGS, "-mcpu=cortex-m3") && is_substr(CYGBLD_GLOBAL_CFLAGS, " -mcpu=cortex-m4 ") } which instead we will comment out for now until we are happy with gcc 4.6.3. All the M3-based platform HALs will have their CFLAGS default to -mcpu=cortex-m3, and the M4 HALs will have -mcpu=cortex-m4 -mfloat-abi=hard -mfpu=fpv4-sp-d16. And until we are happy with the gcc 4.6 tools, anyone using cortex-m4 with older tools can just manually edit the value of CYGBLD_GLOBAL_CFLAGS/LDFLAGS to change the values, and we can put a note to that effect in the CDL description and the platform documentation. So the CDL may say it is an M4, but it won't complain if you compile with -mcpu=cortex-m3 for now. We can put these checks back in later instead. I think asking people to do this is better than forcing them to understand how all the many configuration options are related. So I really do think this is the way we should go. Other comments: 2) The declaration of "cdl_interface CYGINT_HAL_CORTEXM_FPU" can live in hal_cortexm_fpu.cdl instead just to keep it all together. 3) A very trivial change but under CYGARC_CORTEXM_FPU_EXC_AUTOSAVE: loating -> floating 4) You don't need the CYGHWR_HAL_FPU_ANTIFLAGS option. While I'm amused by the name :-), it probably isn't clear to anyone browsing the configuration what its purpose is really. As a little cheat, you can place these requirements inside the CYGINT_HAL_CORTEXM_FPU block (assuming it's useful to keep it in hal_cortexm_fpu.cdl rather than under CYGPKG_HAL_CORTEXM_FPU in hal_cortexm.cdl). The reason this works is because a CDL interface is still considered to be "active" and "enabled" even if nothing "implements" it. As long as its parent in the configuration tree is, anyway. This would get rid of the ANTIFLAGS option entirely which is a useful tidy. 5) I think CYGHWR_HAL_FPV4_SP_D16 can live under the CYGHWR_HAL_CORTEXM_FPU component. 6) A general comment as something which can be done last thing before check-in, and I know it's only a cosmetic thing, but contents of options are a little more readable if things are aligned. For example: cdl_option CYGHWR_HAL_CORTEXM_FPU_SWITCH { display "FPU context switch" flavor data legal_values { "ALL" "LAZY" "NONE" } default_value { "LAZY" } description " is clearer to read as: cdl_option CYGHWR_HAL_CORTEXM_FPU_SWITCH { display "FPU context switch" flavor data legal_values { "ALL" "LAZY" "NONE" } default_value { "LAZY" } description " and so on. Apologies if you were going to do this anyway :-). I'll make my other comments in the next bugzilla entry... -- 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.