public inbox for ecos-patches@sourceware.org
 help / color / mirror / Atom feed
From: bugzilla-daemon@bugs.ecos.sourceware.org
To: ecos-patches@ecos.sourceware.org
Subject: [Bug 1001607] Cortex-M4F architectural Floating Point Support
Date: Wed, 08 Aug 2012 16:49:00 -0000	[thread overview]
Message-ID: <20120808164904.E50F12F78005@mail.ecoscentric.com> (raw)
In-Reply-To: <bug-1001607-104@http.bugs.ecos.sourceware.org/>

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #18 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-08 17:48:56 BST ---
Hi Jifl

(In reply to comment #16)
> Hi Ilija,
> 
[snip]

> 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.
> 

[snip]

> 
> 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.
> 

I agree about CYGOPT_HAL_CORTEXM4_CODE removal, but I think that making
-mcpu=cortex-m4 mandatory for M4 is too inflexible. With present CDL it is
possible to edit CYGBLD_GLOBAL_CFLAGS and change -mcpu. And somebody will want
to try M4 with cortex-m3 code.

Therefore I think that the idea about CYGBLD_ARCH_XFLAGS is good. It will
provide -mcpu=  default value which can later be edited. This will require us
to touch some platform CDL files but we only have to deal with M4 devices with
FPU and they are few (though we can see if we want to "canonize" other
platforms).


> 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.

FAOD, there are cortex-m4 devices without FPU as well.

> 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.
> 

We can apply following changes to CYGHWR_HAL_FPV4_SP_D16 and enforce correct
-mcpu for -mfloat:
-    requires { is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfloat-abi=hard
-mfpu=fpv4-sp-d16") }
-    requires { is_substr(CYGBLD_GLOBAL_LDFLAGS, " -mfloat-abi=hard
-mfpu=fpv4-sp-d16") }
+    requires { is_substr(CYGBLD_GLOBAL_CFLAGS, " -mcpu=cortexm4
-mfloat-abi=hard -mfpu=fpv4-sp-d16") }
+    requires { !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mcpu=cortexm3") }
+    requires { is_substr(CYGBLD_GLOBAL_LDFLAGS, " -mcpu=cortexm4
-mfloat-abi=hard -mfpu=fpv4-sp-d16") }
+    requires { !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mcpu=cortexm3") }

> 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.
> 

OK.


> 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.

I just learnt something about cdl_interface, thanks.

> 
> 5) I think CYGHWR_HAL_FPV4_SP_D16 can live under the CYGHWR_HAL_CORTEXM_FPU
> component.

CYGHWR_HAL_FPV4_SP_D16 is a FPU instance. As you say in future there may be
others. Do we want them under CYGHWR_HAL_CORTEXM_FPU?

> 
> 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 :-).
> 

There are other places like this. My intention is to check them wehen we came
close to commit.


> I'll make my other comments in the next bugzilla entry...
Let's carry on...

-- 
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.

  parent reply	other threads:[~2012-08-08 16:49 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-06-03 12:57 [Bug 1001607] New: " bugzilla-daemon
2012-06-03 12:58 ` [Bug 1001607] " bugzilla-daemon
2012-06-03 12:59 ` bugzilla-daemon
2012-06-03 13:04 ` bugzilla-daemon
2012-06-03 13:05 ` bugzilla-daemon
2012-06-03 13:06 ` bugzilla-daemon
2012-06-03 14:22 ` bugzilla-daemon
2012-06-03 16:22 ` bugzilla-daemon
2012-06-06  6:33 ` bugzilla-daemon
2012-06-10 20:56 ` bugzilla-daemon
2012-06-22  4:56 ` bugzilla-daemon
2012-06-24 15:01 ` bugzilla-daemon
2012-06-28  7:41 ` bugzilla-daemon
2012-07-01  4:23 ` bugzilla-daemon
2012-07-08 16:51 ` bugzilla-daemon
2012-07-08 16:52 ` bugzilla-daemon
2012-07-08 16:53 ` bugzilla-daemon
2012-08-07  5:56 ` bugzilla-daemon
2012-08-07  5:57 ` bugzilla-daemon
2012-08-08 16:49 ` bugzilla-daemon [this message]
2012-08-08 17:14 ` bugzilla-daemon
2012-08-08 18:01 ` bugzilla-daemon
2012-08-08 19:28 ` bugzilla-daemon
2012-08-09  4:03 ` bugzilla-daemon
2012-08-09 10:12 ` bugzilla-daemon
2012-08-10 18:09 ` bugzilla-daemon
2012-08-10 18:14 ` bugzilla-daemon
2012-08-12 11:05 ` bugzilla-daemon
2012-08-29 19:34 ` bugzilla-daemon
2012-08-29 19:36 ` bugzilla-daemon
2012-08-29 19:37 ` bugzilla-daemon
2012-08-31 18:40 ` bugzilla-daemon
2012-08-31 18:42 ` bugzilla-daemon
2012-10-09 19:33 ` bugzilla-daemon
2012-11-06 20:54 ` bugzilla-daemon
2012-11-06 20:56 ` bugzilla-daemon
2012-11-06 20:57 ` bugzilla-daemon
2012-12-02 20:15 ` bugzilla-daemon
2012-12-02 20:18 ` bugzilla-daemon
2012-12-02 20:20 ` bugzilla-daemon
2012-12-02 20:21 ` bugzilla-daemon
2012-12-13  5:37 ` bugzilla-daemon
2012-12-16 15:51 ` bugzilla-daemon
2012-12-17  4:02 ` bugzilla-daemon
2012-12-17 14:05 ` bugzilla-daemon
2012-12-17 15:30 ` bugzilla-daemon
2012-12-21 13:42 ` bugzilla-daemon
2012-12-21 13:53 ` bugzilla-daemon
2013-02-06 21:29 ` bugzilla-daemon
2013-02-06 23:02 ` bugzilla-daemon
2013-02-06 23:08 ` bugzilla-daemon
2013-02-07  9:47 ` bugzilla-daemon
2013-02-07 21:52 ` bugzilla-daemon
2013-02-07 21:57 ` bugzilla-daemon
2013-02-07 21:59 ` bugzilla-daemon
2013-02-07 22:00 ` bugzilla-daemon
2013-02-09  2:08 ` bugzilla-daemon
2013-02-10 12:33 ` bugzilla-daemon
2013-02-10 17:30 ` bugzilla-daemon
2013-02-10 17:34 ` bugzilla-daemon
2013-02-16 14:34 ` bugzilla-daemon
2013-02-20  3:21 ` bugzilla-daemon
2013-03-07  0:16 ` bugzilla-daemon
2013-03-07  0:17 ` bugzilla-daemon
2013-03-07  0:18 ` bugzilla-daemon
2013-03-07  0:19 ` bugzilla-daemon
2013-03-07  1:46 ` bugzilla-daemon
2013-03-08 15:09 ` bugzilla-daemon
2013-03-09 22:49 ` bugzilla-daemon
2013-03-12 22:42 ` bugzilla-daemon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120808164904.E50F12F78005@mail.ecoscentric.com \
    --to=bugzilla-daemon@bugs.ecos.sourceware.org \
    --cc=ecos-patches@ecos.sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).