[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon at bugs dot ecos dot sourceware dot 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 #16 from Jonathan Larmour <jifl@ecoscentric.com> 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.