[Bug 1001607] New: Cortex-M4F architectural Floating Point Support

[Bug 1001607] New: Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

           Summary: Cortex-M4F architectural Floating Point Support
           Product: eCos
           Version: CVS
          Platform: stm3240g_eval (ST STM3240G-EVAL board)
        OS/Version: Cortex-M
            Status: NEW
          Severity: enhancement
          Priority: low
         Component: Patches and contributions
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: ilijak@siva.com.mk
                CC: ecos-patches@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1784)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1784)
Cortex-M4F Floating Point Support 120603

Here I submit architectural floating point support for Cortex-M4F.

Floating point operation:
Three context saving schemes are available as configurable options:

   LAZY: Save context only for threads that actually use FP. This scheme
provides lower delay penalty for threads that don't use FP but is more complex
than the other two as it involves Usage Fault Exception. It is suitable for
applications where few threads use FP.

   ALL : Save FPU context for all threads. This is suitable for applications
where all or most threads use hardware FP. Then average context switching delay
may be lower than for LAZY. Also worst case delay is smaller than for LAZY.

   NONE: Do not save FPU context. This is applicable only if maximum one thread
uses hardware FP and adds no additional delay compared to standard no FPU
operation.

The proposed code only supports FPU usage within thread context. FPU in
exceptions and interrupts is possible but then the handler must save/restore
the FP registers. Such example is the modified _default exception VSR_. This
choice is deliberate since saving FPU context during interrupts would add
considerable delay penalty (ALL) and/or complexity+penalty (LAZY).

GDB STUB: GDB STUB is extended with floating point registers d0..d15 aka
s0..s31 as per http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524 .

Testing:
Software was tested on Kinetis K70. Testing programs, provided as separate
attachments are taken from standard Kernel tests and may be included into if
desirable:

thread_switch_fpu.cxx - (context switching test stripped from tm_basic.cxx then
modified to present different context switching cases)

fptestf.c - fptest.c, only with float instead of double.

fpinttest.c - fptestf.c with added some "integer" threads.

Everybody is welcome to comment and ask.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #1 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 13:57:54 BST ---
Created an attachment (id=1785)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1785)
Kinetis variant Floating Point Support

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #2 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 13:59:25 BST ---
Created an attachment (id=1786)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1786)
TWR-K70F120M platform Floating Pouint support

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #3 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 14:03:56 BST ---
Created an attachment (id=1787)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1787)
thread_switch_fpu.cxx

Thread switching delay measurement. /Float/ and /integer/ threads are permuted
in 4 test cases.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #4 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 14:05:12 BST ---
Created an attachment (id=1788)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1788)
fptestf.c

fptest.c with floats instead of doubles.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #5 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 14:06:38 BST ---
Created an attachment (id=1789)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1789)
fpinttest.c

Like fptestf.c but with added /integer/ threads.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #6 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 15:21:42 BST ---
Since I expect this code to evolve, I haven't included the ChangeoLogs. For the
time being here is the difstat instead.

 cdl/hal_cortexm.cdl     |   66 ++++++++++++++++++---
 cdl/hal_cortexm_fpu.cdl |  138 +++++++++++++++++++++++++++++++++++++++++++++
 include/cortexm_fpu.h   |  143 ++++++++++++++++++++++++++++++++++++++++++++++
 include/cortexm_regs.h  |   40 +++++++++++++
 include/cortexm_stub.h  |   76 ++++++++++++++++++------
 include/hal_arch.h      |  118 ++++++++++++++++++++++++++++++++++----
 include/hal_io.h        |   10 +++
 src/context.S           |  145 ++++++++++++++++++++++++++++++++++++++++++++++-
 src/cortexm_fpu.c       |  131 ++++++++++++++++++++++++++++++++++++++++++
 src/cortexm_stub.c      |   39 ++++++++++++
 src/hal_misc.c          |  147
++++++++++++++++++++++++++++++++++++++++++++++--
 src/vectors.S           |   95 ++++++++++++++++++++++++++++++-
 12 files changed, 1094 insertions(+), 54 deletions(-)

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1789|1                           |0
           is patch|                            |

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #7 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-06 07:32:40 BST ---
Created an attachment (id=1791)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1791)
STM32F4 FPU patch

For those having STM32F4 handy, here is a patch to implement the hardware
Floating Point.

Note: The patch is provided for testing of Cortex-M floating point support and
assumes that every STM32F4 has Floating Point. Production version may need
additional checks.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #8 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-10 21:56:21 BST ---
Here is a useful ARM application note on Cortex-M4F lazy FPU context saving.
http://infocenter.arm.com/help/topic/com.arm.doc.dai0298a/index.html

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |jifl@ecoscentric.com
         AssignedTo|unassigned@bugs.ecos.source |jifl@ecoscentric.com
                   |ware.org                    |
               Flag|                            |Patch_or_Contribution+

--- Comment #9 from Jonathan Larmour <jifl@ecoscentric.com> 2012-06-22 05:55:09 BST ---
Hi Ilija,

Thanks for all this work. It looks really good! I've gone through it all now,
and unsurprisingly have some observations. It's quite a big patch so it ends up
being quite long...

Architecture HAL:

- The CDL seems to be getting much more complex than it really needs to be.
There is now CDL for: CYGHWR_HAL_CORTEXM (== M3 or M4),
CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM3_CODE,
CYGINT_HAL_CORTEXM_FPU, CYGINT_HAL_CORTEXM_FPV4_SP_D16, CYGHWR_HAL_CORTEXM_FPU,
and to an extent CYGHWR_HAL_CORTEXM_FPU_SWITCH=="NONE", which all have
overlapping behaviour with respect to each other and effects on each other.
There is unnecessary redundancy here.

I think we can simply remove CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE
and CYGOPT_HAL_CORTEXM3_CODE because even if building with -mcpu=cortex-m4, gcc
defaults to soft floating-point. So that means if CYGHWR_HAL_CORTEXM == "M4"
then it's safe to use -mcpu=cortex-m4. So CYGBLD_GLOBAL_XFLAGS_ARCH can go too.

This line can just go away:
+        requires { !is_active(CYGOPT_HAL_CORTEXM4_CODE) implies
CYGHWR_HAL_CORTEXM_FPU == 0 }

- CYGHWR_HAL_CORTEXM_FPU should be just:
            active_if       CYGINT_HAL_CORTEXM_FPU
            default_value   1

You don't need to set the default value specially when its inactive anyway.

- In CYGHWR_HAL_CORTEXM_FPU's description there's a "\n". Also I wouldn't call
it "ordinary" behaviour. Perhaps "simplest".

- Here's a possible outline of a simplification. What do you think?
  cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAGS {
     display    "FPU build flags"
     calculated { (CYGHWR_HAL_CORTEXM_FPU ? " -mfloat-abi=hard" : "" ) .
(CYGINT_HAL_CORTEXM_FPV4_SP_D16 ? " -mfpu=fpv4-sp-d16" : "" ) }
   }
   requires is_substr(CYGBLD_GLOBAL_CFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
   requires is_substr(CYGBLD_GLOBAL_LDFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
  requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_CFLAGS,
"-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfloat-abi=hard") }
  requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_LDFLAGS,
"-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mfloat-abi=hard") }

- There is a quite important design issue. A lot of things assume the only
processor with an FPU is the Cortex-M4 and that FPU has the fpv4-sp-d16
arrangement. This is unlikely to remain the case, and it's easy to anticipate.
The M4 FPU is a bit cut-down after all, so it's easy to see a full VFP4 unit
being added in a later Cortex-M.  So we should be wrapping up much more
functionality in macros that can be supplied according to the FPU design, the
most obvious alternative possibility being a full VFP4 unit, to give an idea of
what to abstract.

With that in mind, I think there needs to be a fpv4-sp-d16.h header file,
probably included from cortexm_fpu.h (which could in future then instead
include alternatives e.g. a vfp4.h header once that became relevant). That
FPU-specific header will define a number of FPU-model-specific properties and
macros used for the implementation, such as from cortexm_stub.h and context.S. 

So for example, cortexm_stub.h would just have:
#ifndef NUMREGS
# define NUMREGS (16+8+2)  // 16 GPR, 8 FPR (unused), 2 PS
#endif

and that definition would have been overriden by fpv4-sp-d16.h (or in future by
vfp4.h).

For consistency with the practice of other HALs, something like 'enum regnames'
would become:
#ifndef ENUM_REGNAMES_DEFINED
enum regnames {
    R0, R1, R2, R3, R4, R5, R6, R7,
    R8, R9, R10, FP, IP, SP, LR, PC,
    F0, F1, F2, F3, F4, F5, F6, F7,
    FPS, PS
};
#endif

Of course we only need to override where necessary - things which are the same
in the FPv4 implementation, such as PS_N, PS_Z, PS_C, PS_V, target_register_t,
CYGARC_STUB_REGISTER_ACCESS_DEFINED, TARGET_HAS_LARGE_REGISTERS,
HAL_STUB_REGISTERS_SIZE, etc. are just set once in cortexm_stub.h.

- I wondered whether the current contents of cortexm_fpu.h would be shared with
any future potential alternative FPU unit in future Cortex-M's. I think the
current cortexm_fpu.h probably would be common between them, but do you agree?
So again with the most obvious example, would a VFP4 implementation still use
the same cortexm_fpu.h contents (after including a future vfp4.h header).

- In cortexm_fpu.h, you define hal_cortexm_fpu_enable/disable as essentially
"extern inline". You have to be careful with this, because using extern inline
means no non-inline implementation is provided. So if the compiler cannot
inline (or chooses not to, due to side-effects of optimisation), they would
fail to link. You could either play some tricks using the C preprocessor so you
can have this both as an inline and also define it in a linkable way from a C
file, but I think it may be better to just replace these inlines with
HAL_CORTEXM_FPU_ENABLE/DISABLE macros really. I do prefer inlines myself, but
using them easily from C can be a pain.

- In cortexm_regs.h, there has been the addition of
CYGARC_DSB()/CYGARC_CORTEXM_DATA_SYNC_BARRIER() and
CYGARC_ISB()/CYGARC_CORTEXM_INSTR_SYNC_BARRIER(). I think you should choose one
name for each and stick with it.

Also for consistency with some other architectural ports, there should be a
HAL_MEMORY_BARRIER define:

#define HAL_MEMORY_BARRIER() \
  CYGARC_CORTEXM_DATA_SYNC_BARRIER(); CYGARC_CORTEXM_INSTR_SYNC_BARRIER()

( This could then be used in cortexm_fpu.h, hal_fpu.c etc, to replace existing
calls which do exactly that, although that isn't vital.)

OOI strictly that cortexm_regs.h doesn't need #ifndef __ASSEMBLER__ as its all
macro definitions, but you can keep it like that if you want.

- Looking at the top of hal_arch.h, I think you should always be able to
include <cyg/hal/cortexm_fpu.h>, and we then have cortexm_fpu.h handling the
FPU config settings in a single place (including the check for which sort of
FPU unit), rather than each of the users of the header.

- Again in hal_arch.h, there are defines here which are specific to
fpv4-sp-d16. e.g. HAL_SAVEDREGISTERS_FPU_CONTEXT_N. Or the HAL saved context
members. These should instead be defined by fpv4-sp-d16.h

Although things like HAL_THREAD_INIT_FPU_REGS could be left in hal_arch.h
because once HAL_SAVEDREGISTERS_FPU_CONTEXT_N is abstracted, it will probably
work for everything. Although, that said, there's no harm putting an #ifndef
HAL_THREAD_INIT_FPU_REGS/#endif block around it for future-proofing to be on
the safe side. Similarly HAL_THREAD_INIT_FPU_CONTEXT().

- You don't save the floating point context for interrupts, but do for
exceptions. There's an argument that since both interrupt and exception
handlers could call arbitrary code which therefore may use FP, the context
should be saved. There's also an argument that unless this is a ROM monitor and
including stubs, you don't need to save the exception info either. I think
there may be merit in having two config options to govern whether to save FP
state for exceptions and interrupts. The default value for the exceptions one
would be CYGSEM_HAL_ROM_MONITOR || CYGDBG_HAL_DEBUG_GDB_INCLUDE_STUBS, the
default for the interrupt one would be 0.

- Just as a general coding standards thing, remember to put brackets round uses
of __regs_p in the new HAL_THREAD_INIT_FPU_* macros.

- FYI you can use CYG_EMPTY_STATEMENT instead of " CYG_MACRO_START
CYG_MACRO_END"

- I wouldn't bother with CYGARC_CORTEXM_GDB_REG_FPA, and instead, similar to
things above, have something like:
#ifndef HAL_CORTEXM_GDB_REGISTERS_DEFINED
typedef struct
{
    cyg_uint32  gpr[16];
[etc.]
} HAL_CORTEXM_GDB_Registers;
#endif

which is overridden from fpv4-sp-d16.h.

and CYGNUM_HAL_STACK_CONTEXT_SIZE can be:
#ifndef CYGNUM_HAL_STACK_FPU_CONTEXT_SIZE
# define CYGNUM_HAL_STACK_FPU_CONTEXT_SIZE 0
#endif
#define CYGNUM_HAL_STACK_CONTEXT_SIZE \
   ((4 * 20) + CYGNUM_HAL_STACK_FPU_CONTEXT_SIZE)

I think you're probably getting the idea about these macro overrides by now
:-).

- hal_io.h defines UFSR. I think you should give the comment as "Usage fault
status reg" rather than just "Misc".

- In context.S, CYGARC_FPU_CPACR_ENABLE could usefully be put in cortexm_fpu.h
where it could also be used by hal_cortexm_fpu_enable/disable (or their
replacement macros, as above).

- The title at the top of hal_fpu.c should be something like "HAL FPU support".

- Why does it include <pkgconf/kernel.h> ?

- Typo's: autometic -> automatic and ececute -> execute

- hal_usagefault_check_fpu() should only enable the FPU if
CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY. Otherwise it implies something has gone
wrong and should return false for an error.

- cortexm_stub.c: There's a diag_printf left over in reg_offset().

- Again with reg_offset(), more abstraction of FPU model needed.

- hal_misc.c: You should ignore the startup type for the call to hal_init_fpu.
We can't assume for definite a previous ROM monitor was built with FPU support
enabled, so we can't rely on it (and equally hal_init_fpu() should not just
enable the FPU if needed, but also explicitly disable it if this is lazy
switching mode and it's RAM startup).

- hal_misc.c: hal_get/set_gdb_registers. This duplication can definitely be
avoided with the use of macros (including a default one for the pretend FPA
regs). And then the details would be provided via cortexm_fpu.h by my proposed
fpv4-sp-d16.h.

- In vectors.S: This change is accidental:
  -// NOTE: At present this implementation does not permit an exception
  +// NOTE:p At present this implementation does not permit an exception

- Again we should definitely aim to replace FPU-specific parts with macros,
rather than duplicating entire functions such as hal_default_exception_vsr.
Either with the C preprocessor, or GAS .macro. If the latter you could have an
include/fpu.inc file, which will #include fpv4-sp-d16.h again (which would then
need to be asm-include safe) to configure itself - up to you. You can again
look at the i386 arch HAL to see what it did in the same situation -
specifically arch.inc, although it is more complex than we would need to be in
cortex-m.

But I think something needs to be done otherwise it becomes much harder to
follow the asm.

- Given what I suggested above about being able to save FP context from ISRs,
something would be needed with the hal_default_interrupt_vsr here, although
with some care, I would expect the same macros should be able to be reused for
both exceptions and interrupts.

- hal_usagefault_exception_vsr is almost identical to
hal_default_exception_vsr. I don't see why hal_default_exception_vsr can't be
used, and instead treated specially in hal_deliver_exception() instead, which
would be less code overall.

I had been also going to say about only setting the usagefault VSR (in
hal_misc.c) if CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY, but indeed if we use
hal_deafult_exception_vsr then we could now just call
hal_usagefault_check_fpu() from hal_deliver_exception(), still only if
CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY. And if hal_usagefault_check_fpu() returns
an error, then that allows us to enter the stub which seems like a good idea so
the user can debug the cause.

In other words hal_deliver_exception() would be like this (with some
comments+whitespace stripped for brevity):
void hal_deliver_exception( HAL_SavedRegisters *regs )
{
#ifdef CYGDBG_HAL_DEBUG_GDB_INCLUDE_STUBS
    if (__mem_fault_handler )
    {
        regs->u.exception.pc = (unsigned long)__mem_fault_handler;
        return; // Caught an exception inside stubs
    }
#endif
#ifdef CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY
    if ( hal_usagefault_check_fpu() )
    return; // Successfully handled, so return
#endif

#if defined(CYGDBG_HAL_DEBUG_GDB_INCLUDE_STUBS)
    _hal_registers = regs;
    __handle_exception();
#elif defined(CYGPKG_KERNEL_EXCEPTIONS)
    cyg_hal_deliver_exception( regs->u.exception.vector, (CYG_ADDRWORD)regs );
#else
    CYG_FAIL("Exception!!!");
#endif
}

- Alternatively, if there's a reason to keep hal_usagefault_exception_vsr
separate from hal_default_exception_vsr, then it's still the case it only needs
to be included if CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY I think.

Tests:

- For fptestf.c and fpinttest.c: The test for whether the test is NA should
also include <pkgconf/isoinfra.h> and check CYGINT_ISO_C_CLOCK_FUNCS is
defined, and also CYGPKG_LIBM for fabsf() (which should then not be
prototyped). Note this uses a libm implementation which hasn't been checked in
yet. Overall it might be easier to just do our own fabsf() since we can do a
simple one since we're not concerned with NaNs or infinity. I think this:
float my_fabsf(float f)
{
    if ( f < 0.0 )
        return -f;
    if ( f == -0.0 )
        return 0.0;
    return f;
}

 I think stdio.h isn't needed? And the include of math.h should be within the
#if block for the test requirements otherwise the test won't build because
math.h doesn't exist. You should include <string.h> for strlen and check for
CYGINT_ISO_STRING_STRFUNCS.

- In thread_switch_fpu.cxx you have a FIXME in thread_fp() about gcc maybe
optimising things away. I think it could. A simple solution is to make op1 and
op2 be global arrays, indexed by the thread index (the indx variable). That
should make gcc treat them more carefully. At the very end,
thread_swotch_fpu.cxx -> thread_switch_fpu.cxx :-).

Don't let the length of this make you think that there are big problems
here.... overall this is great work!

Jifl

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #10 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-24 16:00:34 BST ---
Hi Jifl

Thank you for your review.

(In reply to comment #9)
> Hi Ilija,
> 
> Thanks for all this work. It looks really good! I've gone through it all now,
> and unsurprisingly have some observations. It's quite a big patch so it ends up
> being quite long...
> 
> Architecture HAL:
> 
> - The CDL seems to be getting much more complex than it really needs to be.
> There is now CDL for: CYGHWR_HAL_CORTEXM (== M3 or M4),
> CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM3_CODE,
> CYGINT_HAL_CORTEXM_FPU, CYGINT_HAL_CORTEXM_FPV4_SP_D16, CYGHWR_HAL_CORTEXM_FPU,
> and to an extent CYGHWR_HAL_CORTEXM_FPU_SWITCH=="NONE", which all have
> overlapping behaviour with respect to each other and effects on each other.
> There is unnecessary redundancy here.
> 
> I think we can simply remove CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE
> and CYGOPT_HAL_CORTEXM3_CODE because even if building with -mcpu=cortex-m4, gcc
> defaults to soft floating-point. So that means if CYGHWR_HAL_CORTEXM == "M4"
> then it's safe to use -mcpu=cortex-m4. So CYGBLD_GLOBAL_XFLAGS_ARCH can go too.

I am aware of this excessive complexity, as it gave me some headache but I'm
afraid that for the time being it is necesary. The reason for keeping
-mcpu=cortex-m3 option even for Cortex-M4 cores, is compatibility with the
actual eCos compiler, gcc-4.3.2, that doesn't support/recognize
-mcpu=cortex-m4. We can reconsider once we switch to gcc-4.6.3 or newer. 

Provided that we keep selection I would be happier with radio buttons. I have
seen some of them (such as schedulers) but haven't figured out how it's done.
What's the trick?

FAOD, I allow that it could be simpler. I have tried a number of code variants
and stayed at the one that gives complete conflict resolution in configtool for
most of user interactions.
To my understanding is_substr() is kind of tough task for configtool and I
would avoid it (see discussion on FPU CFLAGS below).

> 
> This line can just go away:
> +        requires { !is_active(CYGOPT_HAL_CORTEXM4_CODE) implies
> CYGHWR_HAL_CORTEXM_FPU == 0 }
> 
> - CYGHWR_HAL_CORTEXM_FPU should be just:
>             active_if       CYGINT_HAL_CORTEXM_FPU
>             default_value   1
> 
> You don't need to set the default value specially when its inactive anyway.

I know, only it doesn't look beautiful (in my eyes) if some checkbox is checked
and inactive. But probably I could get used to.

> 
> - In CYGHWR_HAL_CORTEXM_FPU's description there's a "\n". Also I wouldn't call
> it "ordinary" behaviour. Perhaps "simplest".

I took the description from 386, but I agree "simplest" is better, I'll change.

> 
> - Here's a possible outline of a simplification. What do you think?
>   cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAGS {
>      display    "FPU build flags"
>      calculated { (CYGHWR_HAL_CORTEXM_FPU ? " -mfloat-abi=hard" : "" ) .
> (CYGINT_HAL_CORTEXM_FPV4_SP_D16 ? " -mfpu=fpv4-sp-d16" : "" ) }
>    }
>    requires is_substr(CYGBLD_GLOBAL_CFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
>    requires is_substr(CYGBLD_GLOBAL_LDFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
>   requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_CFLAGS,
> "-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfloat-abi=hard") }
>   requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_LDFLAGS,
> "-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mfloat-abi=hard") }

Since you ask me, I would get rid of is_substr(). Something like this:

cdl_option CYGHWR_HAL_CORTEXM_FLOAT_ABI_FLAG {
      flavor data
      no_define
      display    "Float ABI flag"
      calculated { CYGHWR_HAL_CORTEXM_FPU ? " -mfloat-abi=hard" : "" }
}

cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAG {
      flavor data
      no_define
      display    "FPU build flags"
      calculated { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_FPV4_SP_D16) ?
                 " -mfpu=fpv4-sp-d16" : "" }
}

Ditto for -mcpu=cortex-m3/m4 (CYGHWR_HAL_CORTEXM_CPU_FLAG).

Then we concatenate all these in CYGBLD_GLOBAL_CFLAGS.

cdl_option CYGBLD_GLOBAL_CFLAGS {
    display "Global compiler flags"
    flavor  data
    no_define
    default_value { CYGBLD_GLOBAL_WARNFLAGS . CYGHWR_HAL_CORTEXM_CPU_FLAG .
       " -mthumb -g -O2 ffunction-sections -fdata-sections -fno-rtti
-fno-exceptions" .
       CYGHWR_HAL_CORTEXM_FLOAT_ABI_FLAG . CYGHWR_HAL_CORTEXM_FPU_FLAG }
}

I think that in long term this is better solution since it is easy to expand
with new FPUs in future.

> 
> - There is a quite important design issue. A lot of things assume the only
> processor with an FPU is the Cortex-M4 and that FPU has the fpv4-sp-d16
> arrangement. This is unlikely to remain the case, and it's easy to anticipate.
> The M4 FPU is a bit cut-down after all, so it's easy to see a full VFP4 unit
> being added in a later Cortex-M.  So we should be wrapping up much more
> functionality in macros that can be supplied according to the FPU design, the
> most obvious alternative possibility being a full VFP4 unit, to give an idea of
> what to abstract.
> 
> With that in mind, I think there needs to be a fpv4-sp-d16.h header file,
> probably included from cortexm_fpu.h (which could in future then instead
> include alternatives e.g. a vfp4.h header once that became relevant). That
> FPU-specific header will define a number of FPU-model-specific properties and
> macros used for the implementation, such as from cortexm_stub.h and context.S. 
> 

Funny, usually I am warned for over-generalization, now it'oposite :-).
I will consuder this and try to construct something. I wonder if we will need
cortexm_fpu.h (yet another level of indirection) or we can simply rename
cortexm_fpu.h into fpv4-sp-d16.h. Then we have cortexm_fpu.c and
hal_cortexm_fpu.cdl...

In the text below i'll simply snip related text segments. Don't worry, nothing
is discarded.

[snip]

> - In cortexm_regs.h, there has been the addition of
> CYGARC_DSB()/CYGARC_CORTEXM_DATA_SYNC_BARRIER() and
> CYGARC_ISB()/CYGARC_CORTEXM_INSTR_SYNC_BARRIER(). I think you should choose one
> name for each and stick with it.

I'll keep the short one.

> 
> Also for consistency with some other architectural ports, there should be a
> HAL_MEMORY_BARRIER define:
> 
> #define HAL_MEMORY_BARRIER() \
>   CYGARC_CORTEXM_DATA_SYNC_BARRIER(); CYGARC_CORTEXM_INSTR_SYNC_BARRIER()

Or:
#define HAL_MEMORY_BARRIER() CYGARC_DSB(); CYGARC(ISB)

> 
> ( This could then be used in cortexm_fpu.h, hal_fpu.c etc, to replace existing
> calls which do exactly that, although that isn't vital.)
> 
> OOI strictly that cortexm_regs.h doesn't need #ifndef __ASSEMBLER__ as its all
> macro definitions, but you can keep it like that if you want.

What's OOI?

[snip]

> 
> - You don't save the floating point context for interrupts, but do for
> exceptions. There's an argument that since both interrupt and exception
> handlers could call arbitrary code which therefore may use FP, the context
> should be saved. There's also an argument that unless this is a ROM monitor and
> including stubs, you don't need to save the exception info either. I think
> there may be merit in having two config options to govern whether to save FP
> state for exceptions and interrupts. The default value for the exceptions one
> would be CYGSEM_HAL_ROM_MONITOR || CYGDBG_HAL_DEBUG_GDB_INCLUDE_STUBS, the
> default for the interrupt one would be 0.
> 

I expected some arguments here... I can't imagine myself using FPU in ISR or
DSR, but if it is an option, disabled by default here are some considerations:
  - ALL: Implementation is easy, we just skip disabling of automatic stack
saving.
  - LAZY: Lazy saving introduces variable frame length that requires handling
that will produce worst case latency even worse than for ALL. It requires suble
intervention in ISR and may unnecessarily delay the project.

Therefore I would consider immediate implementation for ALL, but for LAZY i
would prefer to put "Not implemented in current release" for the time being.

> - Just as a general coding standards thing, remember to put brackets round uses
> of __regs_p in the new HAL_THREAD_INIT_FPU_* macros.
> 
> - FYI you can use CYG_EMPTY_STATEMENT instead of " CYG_MACRO_START
> CYG_MACRO_END"

Useful information, thanks.


[snip]

> 
> - hal_io.h defines UFSR. I think you should give the comment as "Usage fault
> status reg" rather than just "Misc".

OK.

> 
> - In context.S, CYGARC_FPU_CPACR_ENABLE could usefully be put in cortexm_fpu.h
> where it could also be used by hal_cortexm_fpu_enable/disable (or their
> replacement macros, as above).
> 
> - The title at the top of hal_fpu.c should be something like "HAL FPU support".
> 
> - Why does it include <pkgconf/kernel.h> ?

It is a remaining form older file that I used as template. There might be other
similar cases. Also I don't have complete overview what to include. I am going
to clean-up once the packages are close to check in.

[snip]

> - hal_misc.c: You should ignore the startup type for the call to hal_init_fpu.
> We can't assume for definite a previous ROM monitor was built with FPU support
> enabled, so we can't rely on it (and equally hal_init_fpu() should not just
> enable the FPU if needed, but also explicitly disable it if this is lazy
> switching mode and it's RAM startup).

OK.

> 
> - hal_misc.c: hal_get/set_gdb_registers. This duplication can definitely be
> avoided with the use of macros (including a default one for the pretend FPA
> regs). And then the details would be provided via cortexm_fpu.h by my proposed
> fpv4-sp-d16.h.
> 
> - In vectors.S: This change is accidental:
>   -// NOTE: At present this implementation does not permit an exception
>   +// NOTE:p At present this implementation does not permit an exception
> 
> - Again we should definitely aim to replace FPU-specific parts with macros,
> rather than duplicating entire functions such as hal_default_exception_vsr.
> Either with the C preprocessor, or GAS .macro. If the latter you could have an
> include/fpu.inc file, which will #include fpv4-sp-d16.h again (which would then
> need to be asm-include safe) to configure itself - up to you. You can again
> look at the i386 arch HAL to see what it did in the same situation -
> specifically arch.inc, although it is more complex than we would need to be in
> cortex-m.
> 
> But I think something needs to be done otherwise it becomes much harder to
> follow the asm.

My intention was to leave as much of present code, especially ASM verbatim.
Hence the code duplication.
But now we can go on with macros.

> 
> - Given what I suggested above about being able to save FP context from ISRs,
> something would be needed with the hal_default_interrupt_vsr here, although
> with some care, I would expect the same macros should be able to be reused for
> both exceptions and interrupts.
> 

The automatic stack frame is different than the one used in
default_exception_vsr

> - hal_usagefault_exception_vsr is almost identical to
> hal_default_exception_vsr. I don't see why hal_default_exception_vsr can't be
> used, and instead treated specially in hal_deliver_exception() instead, which
> would be less code overall.
> 

Cortex-M architecture, unlike pure ARM, provides us with individual
exception/interrupt vectors and it would be waste not to use them. We get both
simpler, cleaner and faster code. I would rather extend this approach to other
other exception handlers.

However, we can put the common code sequences in macros.

[snip] I really stand for individual vectors.

> 
> - Alternatively, if there's a reason to keep hal_usagefault_exception_vsr
> separate from hal_default_exception_vsr, then it's still the case it only needs
> to be included if CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY I think.

OK. 

> 
> Tests:
> 
> - For fptestf.c and fpinttest.c: The test for whether the test is NA should
> also include <pkgconf/isoinfra.h> and check CYGINT_ISO_C_CLOCK_FUNCS is
> defined, and also CYGPKG_LIBM for fabsf() (which should then not be
> prototyped). Note this uses a libm implementation which hasn't been checked in
> yet. 

FYI fabsf() is a GCC built-in function, no libm involved, hence declaration. It
is of course temporary until float libm gets committed (thanks for the
reminder).

[snip]

> 
>  I think stdio.h isn't needed? And the include of math.h should be within the
> #if block for the test requirements otherwise the test won't build because
> math.h doesn't exist. You should include <string.h> for strlen and check for
> CYGINT_ISO_STRING_STRFUNCS.

I am using printf() with serial device driver occasionally in order to enforce
some interrupt traffic.

> 
> - In thread_switch_fpu.cxx you have a FIXME in thread_fp() about gcc maybe
> optimising things away. I think it could. A simple solution is to make op1 and
> op2 be global arrays, indexed by the thread index (the indx variable). That
> should make gcc treat them more carefully. At the very end,
> thread_swotch_fpu.cxx -> thread_switch_fpu.cxx :-).

I'll check.

> 
> Don't let the length of this make you think that there are big problems
> here.... overall this is great work!

Your review is great work too. Thanks.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #11 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-28 08:40:46 BST ---
Hi Jifl

Here I revisit the CFLAGS

(In reply to comment #10)
> > - Here's a possible outline of a simplification. What do you think?
> >   cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAGS {
> >      display    "FPU build flags"
> >      calculated { (CYGHWR_HAL_CORTEXM_FPU ? " -mfloat-abi=hard" : "" ) .
> > (CYGINT_HAL_CORTEXM_FPV4_SP_D16 ? " -mfpu=fpv4-sp-d16" : "" ) }
> >    }
> >    requires is_substr(CYGBLD_GLOBAL_CFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
> >    requires is_substr(CYGBLD_GLOBAL_LDFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
> >   requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_CFLAGS,
> > "-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfloat-abi=hard") }
> >   requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_LDFLAGS,
> > "-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mfloat-abi=hard") }
> 
> Since you ask me, I would get rid of is_substr(). Something like this:
> 
> cdl_option CYGHWR_HAL_CORTEXM_FLOAT_ABI_FLAG {
>       flavor data
>       no_define
>       display    "Float ABI flag"
>       calculated { CYGHWR_HAL_CORTEXM_FPU ? " -mfloat-abi=hard" : "" }
> }
> 
> cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAG {
>       flavor data
>       no_define
>       display    "FPU build flags"
>       calculated { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_FPV4_SP_D16) ?
>                  " -mfpu=fpv4-sp-d16" : "" }
> }
> 
> Ditto for -mcpu=cortex-m3/m4 (CYGHWR_HAL_CORTEXM_CPU_FLAG).
> 
> Then we concatenate all these in CYGBLD_GLOBAL_CFLAGS.
> 
> cdl_option CYGBLD_GLOBAL_CFLAGS {
>     display "Global compiler flags"
>     flavor  data
>     no_define
>     default_value { CYGBLD_GLOBAL_WARNFLAGS . CYGHWR_HAL_CORTEXM_CPU_FLAG .
>        " -mthumb -g -O2 ffunction-sections -fdata-sections -fno-rtti
> -fno-exceptions" .
>        CYGHWR_HAL_CORTEXM_FLOAT_ABI_FLAG . CYGHWR_HAL_CORTEXM_FPU_FLAG }
> }
> 

I found out that this requires CYGBLD_GLOBAL_CFLAGS to be *calculated* instead
of *default_value*. I assume that it is not flexible enough so I revoke it.

Regarding your outline it looks nicer than the one I posted in the patch.
However it leaves some unresolved conflicts when I try to change code
generation by clicking the subject controls in configtool.
Actually I came to the construction from the submitted patch after pretty much
experimenting and my criterion was: least clicks that produce unresolved
conflicts. To explain what I mean, here's one example:

1. Create a configuration with "Freescale Kinetis TWR-K70F120M" default
template.
2. Deselect Cortex-M4 code.

Configtool shall report unresolved conflicts. (You can make it easier to
configtool if you deselect Hardware FPU prior to deselecting Cortex-M4). This
is AFAICT the only case that produces unresolved conflict for present CDL
construction. Other combinations that I have tried have more cases. I'm not
much familiar with CDL engine, but from my experience I would say that it is
easier to corner configtool to resolve the conflicts if expressions are
constants (rather than variables).

Cheers
Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEEDINFO

--- Comment #12 from Jonathan Larmour <jifl@ecoscentric.com> 2012-07-01 05:23:09 BST ---
Hi Ilija,

(In reply to comment #10)
> (In reply to comment #9)
> > Architecture HAL:
> > 
> > - The CDL seems to be getting much more complex than it really needs to be.
> > There is now CDL for: CYGHWR_HAL_CORTEXM (== M3 or M4),
> > CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM3_CODE,
> > CYGINT_HAL_CORTEXM_FPU, CYGINT_HAL_CORTEXM_FPV4_SP_D16, CYGHWR_HAL_CORTEXM_FPU,
> > and to an extent CYGHWR_HAL_CORTEXM_FPU_SWITCH=="NONE", which all have
> > overlapping behaviour with respect to each other and effects on each other.
> > There is unnecessary redundancy here.
> > 
> > I think we can simply remove CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE
> > and CYGOPT_HAL_CORTEXM3_CODE because even if building with -mcpu=cortex-m4, gcc
> > defaults to soft floating-point. So that means if CYGHWR_HAL_CORTEXM == "M4"
> > then it's safe to use -mcpu=cortex-m4. So CYGBLD_GLOBAL_XFLAGS_ARCH can go too.
> 
> I am aware of this excessive complexity, as it gave me some headache but I'm
> afraid that for the time being it is necesary. The reason for keeping
> -mcpu=cortex-m3 option even for Cortex-M4 cores, is compatibility with the
> actual eCos compiler, gcc-4.3.2, that doesn't support/recognize
> -mcpu=cortex-m4. We can reconsider once we switch to gcc-4.6.3 or newer. 

Okay. But can't we just let the user do this by changing the value of
CYGHWR_HAL_CORTEXM? We can just (temporarily) relax the CYGHWR_HAL_CORTEXM ==
"M4" in the kinetis variant HAL. Since we can't use M4 features without the
compiler support, in practice it's treating as an M3 anyway. Then we ensure
CFLAGS/LDFLAGS get set according to CYGHWR_HAL_CORTEXM.

> Provided that we keep selection I would be happier with radio buttons. I have
> seen some of them (such as schedulers) but haven't figured out how it's done.
> What's the trick?

The trick is for the two options to implement a CDL interface, and that CDL
interface has a requires statement on it which says it must be == 1.

> > - CYGHWR_HAL_CORTEXM_FPU should be just:
> >             active_if       CYGINT_HAL_CORTEXM_FPU
> >             default_value   1
> > 
> > You don't need to set the default value specially when its inactive anyway.
> 
> I know, only it doesn't look beautiful (in my eyes) if some checkbox is checked
> and inactive. But probably I could get used to.

I guess it's only my own preference, so if you prefer it that way, keep it like
that.

> > - Here's a possible outline of a simplification. What do you think?
> >   cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAGS {
> >      display    "FPU build flags"
> >      calculated { (CYGHWR_HAL_CORTEXM_FPU ? " -mfloat-abi=hard" : "" ) .
> > (CYGINT_HAL_CORTEXM_FPV4_SP_D16 ? " -mfpu=fpv4-sp-d16" : "" ) }
> >    }
> >    requires is_substr(CYGBLD_GLOBAL_CFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
> >    requires is_substr(CYGBLD_GLOBAL_LDFLAGS, CYGHWR_HAL_CORTEXM_FPU_FLAGS )
> >   requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_CFLAGS,
> > "-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfloat-abi=hard") }
> >   requires { !CYGHWR_HAL_CORTEXM_FPU implies !is_substr(CYGBLD_GLOBAL_LDFLAGS,
> > "-mfpu=fpv4-sp-d16") && !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mfloat-abi=hard") }
> 
> Since you ask me, I would get rid of is_substr(). Something like this:
[snip]
>       calculated { CYGHWR_HAL_CORTEXM_FPU ? " -mfloat-abi=hard" : "" }
[snip similar]
> Then we concatenate all these in CYGBLD_GLOBAL_CFLAGS.

Sure you could do it like that, I was just trying to avoid the extra options,
but I see your argument about it being easier to deal with other FPUs later.
But you need to have appropriate "requires" lines still, otherwise there are
problems if the user has set a value, or one has been inferred, which means the
default_value is no longer being used. Imagine the user changes -O2 to -O0
because they're debugging (so CYGBLD_GLOBAL_CFLAGS no longer comes from the
default_value), and then disables the hard FPU support. The flags used wouldn't
change and there is no conflict, but it is a problem. Ditto the other way
round.

> > - There is a quite important design issue. A lot of things assume the only
> > processor with an FPU is the Cortex-M4 and that FPU has the fpv4-sp-d16
> > arrangement. This is unlikely to remain the case, and it's easy to anticipate.
> > The M4 FPU is a bit cut-down after all, so it's easy to see a full VFP4 unit
> > being added in a later Cortex-M.  So we should be wrapping up much more
> > functionality in macros that can be supplied according to the FPU design, the
> > most obvious alternative possibility being a full VFP4 unit, to give an idea of
> > what to abstract.
> > 
> > With that in mind, I think there needs to be a fpv4-sp-d16.h header file,
> > probably included from cortexm_fpu.h (which could in future then instead
> > include alternatives e.g. a vfp4.h header once that became relevant). That
> > FPU-specific header will define a number of FPU-model-specific properties and
> > macros used for the implementation, such as from cortexm_stub.h and context.S. 
> > 
> 
> Funny, usually I am warned for over-generalization, now it'oposite :-).

You can't win. :-)

> I will consuder this and try to construct something. I wonder if we will need
> cortexm_fpu.h (yet another level of indirection) or we can simply rename
> cortexm_fpu.h into fpv4-sp-d16.h.

I think it is better to have one single header to include which provides the
necessary definitions, even if other files need to be included. Otherwise we
get things like this at the top of multiple files:
#if defined(CYGINT_HAL_CORTEXM_FPV4_SP_D16)
# include <cyg/hal/fpv4-sp-d16.h>
#elif defined(CYGINT_HAL_CORTEXM_VFP4)
# include <cyg/hal/vfp4.h>
#elif defined(CYGINT_HAL_CORTEXM_VFP4_D16)
etc.etc.

It also keeps anything common in one place, and maybe certain defaults which
are only overriden by the FPU-implementation specific header if needed, which
may simplify those headers.

> Then we have cortexm_fpu.c and hal_cortexm_fpu.cdl...

We don't need separate versions of those files. The CDL file has very little
specific to the FPU implementation other than the build flags. At this point,
cortexm_fpu.c doesn't need to change, but if it turned out there were things in
future which depended on the FPU implementation, then the way to deal with it
is to provide definitions in fpv4-sp-d16.h (included via cortexm_fpu.h) which
allow cortexm_fpu.c to do the right thing.

> > - In cortexm_regs.h, there has been the addition of
> > CYGARC_DSB()/CYGARC_CORTEXM_DATA_SYNC_BARRIER() and
> > CYGARC_ISB()/CYGARC_CORTEXM_INSTR_SYNC_BARRIER(). I think you should choose one
> > name for each and stick with it.
> 
> I'll keep the short one.

Fine.

> > Also for consistency with some other architectural ports, there should be a
> > HAL_MEMORY_BARRIER define:
> > 
> > #define HAL_MEMORY_BARRIER() \
> >   CYGARC_CORTEXM_DATA_SYNC_BARRIER(); CYGARC_CORTEXM_INSTR_SYNC_BARRIER()
> 
> Or:
> #define HAL_MEMORY_BARRIER() CYGARC_DSB(); CYGARC(ISB)

Indeed :).

Although thinking about it, it probably does want a CYG_MACRO_START/END
wrapper, otherwise (although very unlikely) someone could theoretically do:
if (something)
  HAL_MEMORY_BARRIER();
and things might not behave as intended.

> > ( This could then be used in cortexm_fpu.h, hal_fpu.c etc, to replace existing
> > calls which do exactly that, although that isn't vital.)
> > 
> > OOI strictly that cortexm_regs.h doesn't need #ifndef __ASSEMBLER__ as its all
> > macro definitions, but you can keep it like that if you want.
> 
> What's OOI?

Out of interest. http://www.internetslang.com/OOI-meaning-definition.asp
English has too many idioms and sayings, so lazy people like me abbreviate
them. :-)

> > - You don't save the floating point context for interrupts, but do for
> > exceptions. There's an argument that since both interrupt and exception
> > handlers could call arbitrary code which therefore may use FP, the context
> > should be saved. There's also an argument that unless this is a ROM monitor and
> > including stubs, you don't need to save the exception info either. I think
> > there may be merit in having two config options to govern whether to save FP
> > state for exceptions and interrupts. The default value for the exceptions one
> > would be CYGSEM_HAL_ROM_MONITOR || CYGDBG_HAL_DEBUG_GDB_INCLUDE_STUBS, the
> > default for the interrupt one would be 0.
> 
> I expected some arguments here... I can't imagine myself using FPU in ISR or
> DSR, but if it is an option, disabled by default here are some considerations:
>   - ALL: Implementation is easy, we just skip disabling of automatic stack
> saving.
>   - LAZY: Lazy saving introduces variable frame length that requires handling
> that will produce worst case latency even worse than for ALL. It requires suble
> intervention in ISR and may unnecessarily delay the project.
> 
> Therefore I would consider immediate implementation for ALL, but for LAZY i
> would prefer to put "Not implemented in current release" for the time being.

I think that's ok. So, to be clear, there would be options for whether to save
FP state for interrupts and exceptions, and for the interrupt one, it would
require !LAZY?

[snip] 
> > - Again we should definitely aim to replace FPU-specific parts with macros,
> > rather than duplicating entire functions such as hal_default_exception_vsr.
> > Either with the C preprocessor, or GAS .macro. If the latter you could have an
> > include/fpu.inc file, which will #include fpv4-sp-d16.h again (which would then
> > need to be asm-include safe) to configure itself - up to you. You can again
> > look at the i386 arch HAL to see what it did in the same situation -
> > specifically arch.inc, although it is more complex than we would need to be in
> > cortex-m.
> > 
> > But I think something needs to be done otherwise it becomes much harder to
> > follow the asm.
> 
> My intention was to leave as much of present code, especially ASM verbatim.
> Hence the code duplication.
> But now we can go on with macros.

Nick has mentioned to me in passing that he thinks that although it doesn't
implement lazy saves, the MIPS HAL is a better example to look at for examples
of such macros - the I386 has other complex bits you wouldn't have to worry
about, like SSE, which just make things look harder than they actually are. So
have a look at the MIPS architecture HAL's arch.inc.

> > - Given what I suggested above about being able to save FP context from ISRs,
> > something would be needed with the hal_default_interrupt_vsr here, although
> > with some care, I would expect the same macros should be able to be reused for
> > both exceptions and interrupts.
> 
> The automatic stack frame is different than the one used in
> default_exception_vsr

Of course, yes, that slipped my mind.

> > - hal_usagefault_exception_vsr is almost identical to
> > hal_default_exception_vsr. I don't see why hal_default_exception_vsr can't be
> > used, and instead treated specially in hal_deliver_exception() instead, which
> > would be less code overall.
> > 
> 
> Cortex-M architecture, unlike pure ARM, provides us with individual
> exception/interrupt vectors and it would be waste not to use them. We get both
> simpler, cleaner and faster code. I would rather extend this approach to other
> other exception handlers.

For interrupts I might agree, but exceptions are as defined in the name -
exceptional, i.e. unusual. They aren't common so runtime performance is not the
same concern. Duplicating each one is just a waste of code space. The
efficiency saving is probably only about 3 instructions (a mov, comparison and
branch). And this for something which only ever really gets used in lazy FPU
mode, and then only if a thread uses FP, and then only once ever per thread.

Also as I mentioned, it also breaks the ability to properly debug an unhandled
usage fault with the stub.

> > Tests:
> > 
> > - For fptestf.c and fpinttest.c: The test for whether the test is NA should
> > also include <pkgconf/isoinfra.h> and check CYGINT_ISO_C_CLOCK_FUNCS is
> > defined, and also CYGPKG_LIBM for fabsf() (which should then not be
> > prototyped). Note this uses a libm implementation which hasn't been checked in
> > yet. 
> 
> FYI fabsf() is a GCC built-in function, no libm involved, hence declaration.

Okay. Unless someone uses -fno-builtin, which I've had cause to do in the past
when the compiler has misoptimised some things! Please use __builtin_fabsf()
instead (with a comment to say it comes from GCC).

> It
> is of course temporary until float libm gets committed (thanks for the
> reminder).

In which case at that point we'll be able to use:
#ifndef CYGPKG_LIBM
# define fabsf(_f) __builtin_fabsf(_f)
#endif

[snip stuff presumably you agree with :)] 

Regarding comment #11, can you send me an updated version of the CDL which uses
my approach to the FPU flags (which you say results in an unresolved conflict
when the user changes options)? It would save me having to duplicate by hand
something you've already done. Then I can hopefully see if there's a better
way, but still simpler than your original. I might be wrong :-). You can email
me direct rather than attaching it to the bug if you prefer, whichever is
easiest.

Cheers,

Jifl

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1784|0                           |1
        is obsolete|                            |

--- Comment #13 from Ilija Kocho <ilijak@siva.com.mk> 2012-07-08 17:50:29 BST ---
Created an attachment (id=1816)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1816)
Cortex-M4F Floating Point Support 120708

Hi Jifl

Here I post updated FPU patch.
Below are replies to some of your comments.

(In reply to comment #12)
> Hi Ilija,
> 
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Architecture HAL:
> > > 
> > > - The CDL seems to be getting much more complex than it really needs to be.
> > > There is now CDL for: CYGHWR_HAL_CORTEXM (== M3 or M4),
> > > CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM3_CODE,
> > > CYGINT_HAL_CORTEXM_FPU, CYGINT_HAL_CORTEXM_FPV4_SP_D16, CYGHWR_HAL_CORTEXM_FPU,
> > > and to an extent CYGHWR_HAL_CORTEXM_FPU_SWITCH=="NONE", which all have
> > > overlapping behaviour with respect to each other and effects on each other.
> > > There is unnecessary redundancy here.
> > > 
> > > I think we can simply remove CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE
> > > and CYGOPT_HAL_CORTEXM3_CODE because even if building with -mcpu=cortex-m4, gcc
> > > defaults to soft floating-point. So that means if CYGHWR_HAL_CORTEXM == "M4"
> > > then it's safe to use -mcpu=cortex-m4. So CYGBLD_GLOBAL_XFLAGS_ARCH can go too.
> > 
> > I am aware of this excessive complexity, as it gave me some headache but I'm
> > afraid that for the time being it is necesary. The reason for keeping
> > -mcpu=cortex-m3 option even for Cortex-M4 cores, is compatibility with the
> > actual eCos compiler, gcc-4.3.2, that doesn't support/recognize
> > -mcpu=cortex-m4. We can reconsider once we switch to gcc-4.6.3 or newer. 
> 
> Okay. But can't we just let the user do this by changing the value of
> CYGHWR_HAL_CORTEXM? We can just (temporarily) relax the CYGHWR_HAL_CORTEXM ==
> "M4" in the kinetis variant HAL. Since we can't use M4 features without the
> compiler support, in practice it's treating as an M3 anyway. Then we ensure
> CFLAGS/LDFLAGS get set according to CYGHWR_HAL_CORTEXM.
> 

I think that this work-arround would just move the problem from one place to
other. Now I have placed radio buttons and it (at least) looks nicer.


> > > - Here's a possible outline of a simplification. What do you think?
> > >   cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAGS {

I made some changes to this CDL. I think we're bit closer.

> > 
> > > - There is a quite important design issue. A lot of things assume the only
> > > processor with an FPU is the Cortex-M4 and that FPU has the fpv4-sp-d16
> > > arrangement. This is unlikely to remain the case, and it's easy to anticipate.
> > > The M4 FPU is a bit cut-down after all, so it's easy to see a full VFP4 unit
> > > being added in a later Cortex-M.  So we should be wrapping up much more
> > > functionality in macros that can be supplied according to the FPU design, the
> > > most obvious alternative possibility being a full VFP4 unit, to give an idea of
> > > what to abstract.
> > > 
> > > With that in mind, I think there needs to be a fpv4-sp-d16.h header file,
> > > probably included from cortexm_fpu.h (which could in future then instead
> > > include alternatives e.g. a vfp4.h header once that became relevant). That
> > > FPU-specific header will define a number of FPU-model-specific properties and
> > > macros used for the implementation, such as from cortexm_stub.h and context.S. 
> > > 
> > 

I wrote macros for vectors.S but left context.S without macros. The reason
vectors.S functions are easily represented as sequences of macros while for
context.S it would either give less optimal code.

Also introduced fpv4_sp_d16.[h|c]

> 
> 
> Although thinking about it, it probably does want a CYG_MACRO_START/END
> wrapper, otherwise (although very unlikely) someone could theoretically do:
> if (something)
>   HAL_MEMORY_BARRIER();
> and things might not behave as intended.

I forgot this, sorry. Now I'm posting as is but, I will add the wrapper in next
iteration.

> > > - You don't save the floating point context for interrupts, but do for
> > > exceptions. There's an argument that since both interrupt and exception
> > > handlers could call arbitrary code which therefore may use FP, the context
> > > should be saved. There's also an argument that unless this is a ROM monitor and
> > > including stubs, you don't need to save the exception info either. I think
> > > there may be merit in having two config options to govern whether to save FP
> > > state for exceptions and interrupts. The default value for the exceptions one
> > > would be CYGSEM_HAL_ROM_MONITOR || CYGDBG_HAL_DEBUG_GDB_INCLUDE_STUBS, the
> > > default for the interrupt one would be 0.
> > 
> > I expected some arguments here... I can't imagine myself using FPU in ISR or
> > DSR, but if it is an option, disabled by default here are some considerations:
> >   - ALL: Implementation is easy, we just skip disabling of automatic stack
> > saving.
> >   - LAZY: Lazy saving introduces variable frame length that requires handling
> > that will produce worst case latency even worse than for ALL. It requires suble
> > intervention in ISR and may unnecessarily delay the project.
> > 
> > Therefore I would consider immediate implementation for ALL, but for LAZY i
> > would prefer to put "Not implemented in current release" for the time being.
> 
> I think that's ok. So, to be clear, there would be options for whether to save
> FP state for interrupts and exceptions, and for the interrupt one, it would
> require !LAZY?

Added automatic context saving option for ALL. Need's test programs.

> > 
> > The automatic stack frame is different than the one used in
> > default_exception_vsr
> 
> Of course, yes, that slipped my mind.
> 
> > > - hal_usagefault_exception_vsr is almost identical to
> > > hal_default_exception_vsr. I don't see why hal_default_exception_vsr can't be
> > > used, and instead treated specially in hal_deliver_exception() instead, which
> > > would be less code overall.
> > > 
> > 
> > Cortex-M architecture, unlike pure ARM, provides us with individual
> > exception/interrupt vectors and it would be waste not to use them. We get both
> > simpler, cleaner and faster code. I would rather extend this approach to other
> > other exception handlers.
> 
> For interrupts I might agree, but exceptions are as defined in the name -
> exceptional, i.e. unusual. They aren't common so runtime performance is not the
> same concern. Duplicating each one is just a waste of code space. The
> efficiency saving is probably only about 3 instructions (a mov, comparison and
> branch). And this for something which only ever really gets used in lazy FPU
> mode, and then only if a thread uses FP, and then only once ever per thread.
> 
> Also as I mentioned, it also breaks the ability to properly debug an unhandled
> usage fault with the stub.
> 

Please note that when using FPU the hal_usagefault_exception_vsr and
hal_default_exception_vsr have different prologues/epilogues.


Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1785|0                           |1
        is obsolete|                            |

--- Comment #14 from Ilija Kocho <ilijak@siva.com.mk> 2012-07-08 17:51:50 BST ---
Created an attachment (id=1817)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1817)
Kinetis variant Floating Point Support 120708

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1786|0                           |1
        is obsolete|                            |

--- Comment #15 from Ilija Kocho <ilijak@siva.com.mk> 2012-07-08 17:53:37 BST ---
Created an attachment (id=1818)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1818)
TWR-K70F120M platform Floating Point support 120708

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #17 from Jonathan Larmour <jifl@ecoscentric.com> 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_d16.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. //=========================== 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 specifically
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) and
the description of cortexm_fpu.c could be better - I think that was just taken
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 switching
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=0,offset=0; i<NUMREGS; i++)
    {
        if ( i == reg )
            break;
        offset += REGSIZE(i);
    }
    if ( NUMREGS == i || REGSIZE(i) == 0 )
        return -1;
    return offset;
}
since this is only used by the stub, a tiny amount of runtime inefficiency is
fine.

20) vectors.S still has a "NOTE:p" accidental change on line 158.

21) It looks like we still may not agree about whether usage faults should have
their own exception vsr or not. As I said they are only used with lazy FPU
handling and at most once per thread, so this is not performance critical code.
Code size might be critical though. But the biggest problem is that, as you
say, at present the hal_usagefault_exception_vsr has a different
prologue/epilogue to hal_default_exception_vsr. That means the user can't debug
any usage faults such as illegal instructions or unaligned accesses because the
stub won't be entered. Whether with a separate vsr or not, I think it is
important we have a solution that still allows users to debug usage faults. I
guess that if we did do this through hal_default_exception_vsr then it would
mean having to test whether the FPU was enabled or not before attempting to
save the context. On the other hand, exceptions are of course rare and usually
not something programs rely on for normal operation (only fatal errors
usually), so performance probably isn't critical either. Can you think of any
other way to allow usage faults to be debuggable when using the usage fault
exception and lazy FPU context switching?


I'm prepared to make many of these changes as it is unfair of me to ask you to
do them all when you have already done so much, but given how much time has
passed since this patch, I'm not sure if you have made other changes in the
meantime, so let me know if you want me to make changes, so we can avoid
clashes and having to merge.

Благодарам !

Jifl

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #19 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-08 18:14:13 BST ---
(In reply to comment #17)
> More comments....
> 
> 7) There's a few more things from cortexm_stub.h which you put in fpv4_sp_d16.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.

OK.

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

I haven't experienced problems so far, but I haven't tried other optimization
than default. But we can trade them for macros, though I am reluctant.

> 
> 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. //=========================== etc.).

I'll try to improve.

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

My English is less than perfect but here it is not a lost memory. The meaning
is (intended to be), as stated in the comment just below the line
_undo_last_enable_. It is not equivalent to disable because the FPU might have
been enabled already.

> 13) In fpv4_sp_d16.h the uses of __regs_p in HAL_THREAD_INIT_FPU_REGS and
> _CONTEXT still need brackets for safety :).
> 

One reason why I prefer functions to macros.

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

OK. Then it (or override) shall be defined in fpv4_sp_d16.h


> 15) In hal_arch.inc, hal_fpu_enable and _disable should use
> CYGARC_REG_FPU_CPACR_ENABLE to be consistent

OK. There are some other places as well.

> 
> 16) The banner at the top of hal_arch.inc isn't right (it says vectors.S) and
> the description of cortexm_fpu.c could be better - I think that was just taken
> from vectors.S as well.

Yes, it is.

> 
> 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 switching
> is enabled. Otherwise let it default to the hal_default_exception_vsr. That's
> the only time we need it.

OK.

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

It is only possible when automatic FPU context saving is disabled. With enabled
automatic FPU context saving we have no choice. 

> 
> 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=0,offset=0; i<NUMREGS; i++)
>     {
>         if ( i == reg )
>             break;
>         offset += REGSIZE(i);
>     }
>     if ( NUMREGS == i || REGSIZE(i) == 0 )
>         return -1;
>     return offset;
> }
> since this is only used by the stub, a tiny amount of runtime inefficiency is
> fine.
> 

I need some time to check this.

> 
> 21) It looks like we still may not agree about whether usage faults should have
> their own exception vsr or not. As I said they are only used with lazy FPU
> handling and at most once per thread, so this is not performance critical code.
> Code size might be critical though. But the biggest problem is that, as you
> say, at present the hal_usagefault_exception_vsr has a different
> prologue/epilogue to hal_default_exception_vsr. That means the user can't debug
> any usage faults such as illegal instructions or unaligned accesses because the
> stub won't be entered. Whether with a separate vsr or not, I think it is
> important we have a solution that still allows users to debug usage faults. I
> guess that if we did do this through hal_default_exception_vsr then it would
> mean having to test whether the FPU was enabled or not before attempting to
> save the context. On the other hand, exceptions are of course rare and usually
> not something programs rely on for normal operation (only fatal errors
> usually), so performance probably isn't critical either. Can you think of any
> other way to allow usage faults to be debuggable when using the usage fault
> exception and lazy FPU context switching?
> 

It is possible to change hal_deliver_usagefault_exception to return a value
that will be !0 if there are still exceptions to process. Then
hal_usagefault_exception_vsr can be tweaked to jump into
hal_default_exception_vsr.  I am testing this and am going to post a patch
later, here is a snippet:

=== Code snippet =================
hal_usagefault_exception_vsr:

        mrs     r0,psp                  // Get process stack
        sub     r1,r0,#(12*4)           // Make space for saved state
        msr     psp,r1                  // Ensure PSP is up to date
        mrs     r3,basepri              // R3 = basepri
        stmfd   r0!,{r2-r11,lr}         // save registers
        sub     r0,#4
        mov     r4,r0                   // R4 = saved state pointer

        bl      hal_deliver_usagefault_exception
        mov     r1,r0                   // return code

        mov     r0,r4                   // R0 = state saved across call
        add     r0,#4
        ldmfd   r0!,{r2-r11,lr}         // Restore registers
        msr     psp,r0                  // Restore PSP
        msr     basepri,r3              // Restore basepri

        cmp     r1,#0                   // Exc. other than FPU?
        bne     hal_default_exception_vsr // Yes - process it

        bx      lr                      // Not: Return

====== Snippet end ====================

> 
> I'm prepared to make many of these changes as it is unfair of me to ask you to
> do them all when you have already done so much, but given how much time has
> passed since this patch, I'm not sure if you have made other changes in the
> meantime, so let me know if you want me to make changes, so we can avoid
> clashes and having to merge.

If you have time that would be helpful, especially with CDL, but before we jump
to coding let's discuss it. I haven't changed anything since I posted the last
patch as I expected your comments. Now I'm testing the usagefault tweak and I
plan to post a small incremental patch later today or tomorrow.

> 
> Благодарам !

So you do some Macedonian after all :-)

Cheers

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #20 from Jonathan Larmour <jifl@ecoscentric.com> 2012-08-08 19:01:08 BST ---
Hi Ilija,

(In reply to comment #18)
> (In reply to comment #16)
> >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.

True enough, then we simply leave that 'requires' out forever rather than
temporarily :-).

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

Yes, better to do it now than later.

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

Sure, I just meant for our current TWR-K70 and STM3240G platforms.

> We can apply following changes to CYGHWR_HAL_FPV4_SP_D16 [snip]
> +    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") }

Along with what was in the old "anti-flags" proposal to cover when it's
disabled, that looks good.

[snip] 
> > 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?

Yes since they will all be active_if CYGHWR_HAL_CORTEXM_FPU anyway, they may as
well just live under.

> > 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: [snip]
> 
> There are other places like this. My intention is to check them when we came
> close to commit.

Great, I'm just being paranoid then :-).

Jifl

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #21 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-08 20:27:50 BST ---
Created an attachment (id=1881)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1881)
Enter default VSR from Usagefault VSR if not FPU exception. 120808

This patch modifies Usagefault VSR behavior so it will enter Default VSR if
exception other than NOCP (FPU) occurs (ref: comment #20).

The patch is icremental to attachment 1816.

Ilija

P.S. TODO: conditinal application only for Lazy mode.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #22 from Jonathan Larmour <jifl@ecoscentric.com> 2012-08-09 05:03:23 BST ---
(In reply to comment #19)
> (In reply to comment #17)
> > 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.
> 
> I haven't experienced problems so far, but I haven't tried other optimization
> than default. But we can trade them for macros, though I am reluctant.

The alternative is to do some fiddling so that there's a non-inline version
available. I mentioned this in comment #9, and it's something we've done
elsewhere in eCos. Here's what you do in the header:

#ifndef CYGPRI_HAL_CORTEXM_FPU_INLINE
# define CYGPRI_HAL_CORTEXM_FPU_INLINE __externC inline
#endif

CYGPRI_HAL_CORTEXM_FPU_INLINE void hal_cortexm_fpu_enable(void)
{
  ...

and then in *one* real source file, you put the following before any #includes:
#define CYGPRI_HAL_CORTEXM_FPU_INLINE __externC

The only complication is that that file should not be cortexm_fpu.c or
fpv4_sp_d16.c since otherwise their own uses of those inline functions will not
be inlined after all, so it would have to go somewhere else like hal_misc.c.
Which is why I was thinking of just using preprocessor macros which might be
easier overall.

> > 9) CYGARC_CORTEXM_FPU_EXC_AUTOSAVE defaults to off.

Oops, I was going to write stuff here that I ended up writing as point 21. Just
ignore this.

> > 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 specifically
> > 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!.
> 
> My English is less than perfect but here it is not a lost memory. The meaning
> is (intended to be), as stated in the comment just below the line
> _undo_last_enable_. It is not equivalent to disable because the FPU might have
> been enabled already.

I'm happy with having it as disable. For example, we use the word the same way
with the hal cache and interrupt macros. In English this meaning is ok. Whereas
"unenable" isn't a word :).

> > 13) In fpv4_sp_d16.h the uses of __regs_p in HAL_THREAD_INIT_FPU_REGS and
> > _CONTEXT still need brackets for safety :).
> 
> One reason why I prefer functions to macros.

:)

[snip] 
> > 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.
> 
> It is only possible when automatic FPU context saving is disabled. With enabled
> automatic FPU context saving we have no choice. 

That's fine. If they've enabled CYGARC_CORTEXM_FPU_EXC_AUTOSAVE we can force
this new CDL option on/off (whichever way round it is implemented).

> > 21) [usage fault vsr]
[snip] 
> It is possible to change hal_deliver_usagefault_exception to return a value
> that will be !0 if there are still exceptions to process. Then
> hal_usagefault_exception_vsr can be tweaked to jump into
> hal_default_exception_vsr.  I am testing this and am going to post a patch
> later,

Looking at the patch you posted in comment #21, do you need to save r2/r3 at
all? In fact since hal_deliver_usagefault_exception doesn't use its 'regs'
argument, should we drop that and only be saving, I guess, r12 and lr? Although
we have to stash r0 into r4 as before, so that too. All the other registers
should be unaffected by hal_deliver_usagefault_exception, since unlike other
user exception handling by default_exception_vsr, we know what this handler
does. And basepri should also be unaffected I would have thought.

So something like this instead:
        mrs     r0,psp                  // Get process stack
        sub     r1,r0,#(3*4)            // Make space for saved state
        msr     psp,r1                  // Ensure PSP is up to date
        stmfd   r0!,{r4,r12,lr}         // save registers

        mov     r4,r0                   // R4 = saved state pointer

        bl      hal_deliver_usagefault_exception

        mov     r1,r4                   // R1 = state saved across call
        ldmfd   r1!,{r4,r12,lr}         // Restore registers
        msr     psp,r1                  // Restore PSP

        cmp     r0,#0                   // Exception other than FPU?
        bne     hal_default_exception_vsr // Y: - process it
        bx      lr                      // N: Return

This is fewer instructions and fewer memory accesses.

I think with this route then hal_usagefault_exception_vsr and
hal_deliver_usagefault_exception should be renamed to
hal_fpu_usagefault_exception_vsr and hal_deliver_fpu_usagefault_exception
instead. 

> > Благодарам !
> 
> So you do some Macedonian after all :-)

навистина не

Jifl

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1881|0                           |1
        is obsolete|                            |

--- Comment #23 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-09 11:11:57 BST ---
Created an attachment (id=1882)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1882)
Enter default VSR from Usagefault VSR if not FPU exception. 120809

(In reply to comment #22)
> (In reply to comment #19)
> > (In reply to comment #17)
> 
> Looking at the patch you posted in comment #21, do you need to save r2/r3 at
> all? In fact since hal_deliver_usagefault_exception doesn't use its 'regs'
> argument, should we drop that and only be saving, I guess, r12 and lr? 


Although
> we have to stash r0 into r4 as before, so that too. All the other registers
> should be unaffected by hal_deliver_usagefault_exception, since unlike other
> user exception handling by default_exception_vsr, we know what this handler
> does. And basepri should also be unaffected I would have thought.

You are right, now that exception's job is predetermined, it can be optimized.
We don't have to save all registers, indeed, and r12 can be omitted (auto-saved
by exception), also maybe lr - but we need to keep stack aligned to 8 bytes
anyway.

[snip]

> I think with this route then hal_usagefault_exception_vsr and
> hal_deliver_usagefault_exception should be renamed to
> hal_fpu_usagefault_exception_vsr and hal_deliver_fpu_usagefault_exception
> instead. 
> 

I renamed hal_deliver_usagefault..., but technically
hal_usagefault_exception_vsr still handles all usagefault exceptions, though
some of them with help of hal_default_exception_vsr

I am going to implement other changes for which which I think that are clear,
and for others I will reply in another post.

Поздрав

Илија

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1882|0                           |1
        is obsolete|                            |

--- Comment #24 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-10 19:09:21 BST ---
Created an attachment (id=1893)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1893)
Cortex-M4F Floating Point Support 120810 incremental to 1816

Hi Jifl

Here is a patch that applies fixes that we discussed past couple of days. It is
incremental to Attachment 1816.

Here are some comments. In the text below I am referencing with your numbers
from Comment #16 and Comment #17.

1) Cortex-M4/M3 selector is removed, and CYGBLD_ARCH_XFLAGS added, with comment
regarding GCC issue. Anti-flags (4) moved to CYGINT_HAL_CORTEXM_FPU and
CYGINT_HAL_FPV4_SP_D16 (respecting FPU abstraction). There is, however a
problem with unresolved conflicts when I click on CYGHWR_HAL_CORTEXM_FPU.
Funny, but it resolves when I right-click RESOLVE in conflict window.

19) Your reg_offset() code works. It would be inappropriate for me to sign it,
so I would be happy to see you in the list of contributors.
Also hal_cortex_fpu.cdl is at least 40% your ideas so we would share the
authorship.

18) I added condition to suppress FPU context saving for exceptions when not
selected ROM monitor or STUB.

I did some clean-up, formatting and banner update. Also added ChangeLog.

I did some regressions and it seems to work correctly. I will do more following
days, especially with non FPU devices (K60 and Cortex-M3).

Cheers

Ilija.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #25 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-10 19:14:09 BST ---
Created an attachment (id=1894)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1894)
TWR-K70F120M platform update incremental to 1818

The attached patch updates TWR-K70F120M for compatibility with Attachment 1893.
It is incremental to Attachment 1818.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #26 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-12 12:05:05 BST ---
(In reply to comment #24)

[snip]

> 
> 1) Cortex-M4/M3 selector is removed, and CYGBLD_ARCH_XFLAGS added, with comment
> regarding GCC issue. Anti-flags (4) moved to CYGINT_HAL_CORTEXM_FPU and
> CYGINT_HAL_FPV4_SP_D16 (respecting FPU abstraction). There is, however a
> problem with unresolved conflicts when I click on CYGHWR_HAL_CORTEXM_FPU.
> Funny, but it resolves when I right-click RESOLVE in conflict window.

It seems that is_substr() expressions are executed concurrently rather than
sequentially. Following snippets, although less elegant, produce correct
behavior.

        cdl_interface CYGINT_HAL_FPV4_SP_D16 {
            flavor     bool
            display    "FPU is FPv4-SP-D16"
            implements CYGINT_HAL_CORTEXM_FPU

            requires { !CYGHWR_HAL_CORTEXM_FPU implies
                !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfpu=fpv4-sp-d16") &&
                !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfloat-abi=hard")
            }
            requires { !CYGHWR_HAL_CORTEXM_FPU implies
                !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mfpu=fpv4-sp-d16") &&
                !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mfloat-abi=hard")

            }

            requires { (CYGHWR_HAL_CORTEXM_FPU && !CYGINT_HAL_FPV4_SP_D16) 
                implies
                !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfpu=fpv4-sp-d16")
            }
            requires { (CYGHWR_HAL_CORTEXM_FPU && !CYGINT_HAL_FPV4_SP_D16)
                implies
                !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mfpu=fpv4-sp-d16")
            }

        }

        cdl_component CYGHWR_HAL_CORTEXM_FPU {
            display        "Use hardware FPU"
            flavor         bool
            active_if      (CYGINT_HAL_CORTEXM_FPU )
            default_value  0; #{ CYGINT_HAL_CORTEXM_FPU }
            compile        cortexm_fpu.c

           }
      }

        cdl_component CYGHWR_HAL_FPV4_SP_D16 {
            flavor none
            active_if CYGINT_HAL_FPV4_SP_D16 && CYGHWR_HAL_CORTEXM_FPU
            display     "FPv4-SP-D16"
            no_define
            compile     fpv4_sp_d16.c

                requires {
                    is_substr(CYGBLD_GLOBAL_CFLAGS, " -mcpu=cortex-m4") &&
                    is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfloat-abi=hard") &&
                    is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") &&
                    !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mcpu=cortex-m3")
                }

                requires {
                    is_substr(CYGBLD_GLOBAL_LDFLAGS, " -mcpu=cortex-m4") &&
                    is_substr(CYGBLD_GLOBAL_LDFLAGS," -mfloat-abi=hard") &&
                    is_substr(CYGBLD_GLOBAL_LDFLAGS, " -mfpu=fpv4-sp-d16") &&
                    !is_substr(CYGBLD_GLOBAL_LDFLAGS, "-mcpu=cortex-m3")
                }
            }
       }

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1816|0                           |1
        is obsolete|                            |
   Attachment #1893|0                           |1
        is obsolete|                            |

--- Comment #27 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-29 20:34:27 BST ---
Created an attachment (id=1915)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1915)
Cortex-M4F Floating Point Support 120829

Hi Jifl and others.

Here is the integral patch for Cortex-M4F architectural floating point support.
I have passed through the code several times, cleaned up, fixed formatting and
typos...

There's however one essential addition: option for softfp ABI. Being able to
link softfp code with -mfloat-abi=soft, the -mfloat-abi=softfp flag (when
selected) is applied only to context.S and vectors.S. All other code compiles
with -mfloat-abi=soft thus avoiding potential GCC's "optimizations" - see Note
below.

Note (RFC): according to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFIFGGE.html
if hardware floating point option is selected, GCC may choose to use FPU
registers even if there isn't floating point source. Please comment.

Having this said I have set following defaults:
   - Hardware floating point is disabled by default.
   - When hardware floating point is enabled the default ABI is:
     - softfp for LAZY and NONE
     - hard   for ALL.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1817|0                           |1
        is obsolete|                            |

--- Comment #28 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-29 20:35:51 BST ---
Created an attachment (id=1916)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1916)
Kinetis variant Floating Point Support 120829

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1818|0                           |1
        is obsolete|                            |
   Attachment #1894|0                           |1
        is obsolete|                            |

--- Comment #29 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-29 20:37:15 BST ---
Created an attachment (id=1917)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1917)
TWR-K70F120M platform Floating Point support 120829

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1915|0                           |1
        is obsolete|                            |

--- Comment #30 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-31 19:39:57 BST ---
Created an attachment (id=1921)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1921)
Cortex-M4F Floating Point Support 120831

Trying to upgrade STM32F4 port I found out that upon change of processor family
(CYGHWR_HAL_CORTEXM_STM32_FAMILY) it can dynamically change CYGHWR_HAL_CORTEXM
from "M4" to "M3" with which the code in Attachment 1915 couldn't cope. Here is
an upgrade that fixes that.

Some notes:
It comes with a lot of "requires" in CYGBLD_ARCH_XFLAGS which that may question
its role.

Switching CYGHWR_HAL_CORTEXM_STM32_FAMILY from "F4" to "F3"("F2") when hard ABI
is selected, leaves some unresolved conflicts. They automatically resolve after
a couple interactions from user (right click -> resolve) I guess that
configtool should offer this automatically.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1791|0                           |1
        is obsolete|                            |

--- Comment #31 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-31 19:41:56 BST ---
Created an attachment (id=1922)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1922)
STM32F4 FPU patch 120931

Here is STM32 patch. Tested only for configuration and building.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #32 from Ilija Kocho <ilijak@siva.com.mk> 2012-10-09 20:32:59 BST ---
(In reply to comment #27)

Hi Jifl

Past weeks I did some searching and found no references to this unsolicited
usage of VFP registers. I even posted a question to GCC Help - no response...
Finally I asked at ARM and got following answer:
http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html

Now I am going to undo all /soft-float/ stuff introduced in attachment 1915 and
revert to original plan. It may take a while, and it would make it little-bit
easier if we resolve Bug 1001606 and avoid bug merging.

How does it look from your prospective Jifl?

Ilija.

> Created an attachment (id=1915)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1915) [details]
> Cortex-M4F Floating Point Support 120829
> 
[snip]

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFIFGGE.html

[snip]

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1921|0                           |1
        is obsolete|                            |

--- Comment #33 from Ilija Kocho <ilijak@siva.com.mk> 2012-11-06 20:54:04 GMT ---
Created an attachment (id=1979)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1979)
Cortex-M4F Floating Point Support 121106

I think that I got a solution for CFLAGS/LDFLAGS management.

The problem was that, as it occurs to me, if several CDL components manipulate
a string [by means of !is_substr()/is_substr()] they do it in a concurrent
manner. I am not familiar with configtool internal structure but it looked like
race conditions...

Therefore I decided to move all operations in a single CDL that is
CYGBLD_ARCH_CPUFLAGS. Besides working smoothly it also solves the following
problems:
   - Home place for FLAGS related "requires" commands,
   - Extensibility: New flags can be added in future,
   - Maintenance: All FLAGS operation on architectural level is consolidated in
a single CDL.
   - Compatibility: No tackling with FLAGS on platform level. Platform need
only to implement an interface. Look for example in STM 32 patch (untested in
runtime).
   - Simplicity: I hope so :-) 

My ability for testing is limited during this and following week, but from what
I could see it operates as it should.

As I have a feeling that we are approaching our goal I would like to do some
shaping work. One question is where is best place to put the tests. They stem
from kernel tests, should I place them there or under Cortex-M architecture?

So far I only run tests on Kinetis. I would appreciate if somebody tries
STM32F4.

Regards
Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1916|0                           |1
        is obsolete|                            |
   Attachment #1917|0                           |1
        is obsolete|                            |

--- Comment #34 from Ilija Kocho <ilijak@siva.com.mk> 2012-11-06 20:56:17 GMT ---
Created an attachment (id=1980)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1980)
Kinetis Support 121106

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1922|0                           |1
        is obsolete|                            |

--- Comment #35 from Ilija Kocho <ilijak@siva.com.mk> 2012-11-06 20:57:23 GMT ---
Created an attachment (id=1981)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1981)
STM32F4 FPU patch 121106

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1979|0                           |1
        is obsolete|                            |

--- Comment #36 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-02 20:15:41 GMT ---
Created an attachment (id=1991)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1991)
 Cortex-M4F Floating Point Support 121202

Hi Jifl

After fine tuning and extensive testing, I think that with attached patches we
are close to check-in. I hope we can aim to commit by end of 2012.

FPU support, including GDB stub has been tested on Kinetis K70 and STM32F4
targets.

Changes: Default setting now is SOFT FPU. Rationale is that more often people
will make integer applications even on systems with FPU. I may be completely
wrong so comments are appreciated.

Another dilemma: Where to put the tests (thread_switch_fpu.cxx and fpinttest.c
only): kernel, hal or cortexm?

Regards
Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1980|0                           |1
        is obsolete|                            |

--- Comment #37 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-02 20:17:57 GMT ---
Created an attachment (id=1992)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1992)
Kinetis Support 121106

Kinetis now has ECM file for RedBoot with FPU support.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1981|0                           |1
        is obsolete|                            |

--- Comment #38 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-02 20:20:08 GMT ---
Created an attachment (id=1993)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1993)
 STM32F4 FPU patch 121202

STM32 now tested and with ECM for RedBoot with FPU support.

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1992|Kinetis Support 121106      |Kinetis Support 121202
        description|                            |

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #39 from Jonathan Larmour <jifl@ecoscentric.com> 2012-12-13 05:37:23 GMT ---
Hi Ilija,

Excuse the length of this, since I have a few older things I never commented
on. On the plus side, we're nearly there! Everything below is easy to sort out
I think.

(In reply to comment #24)
> Here are some comments. In the text below I am referencing with your numbers
> from Comment #16 and Comment #17.
> 
> 1) Cortex-M4/M3 selector is removed, and CYGBLD_ARCH_XFLAGS added, with comment
> regarding GCC issue. Anti-flags (4) moved to CYGINT_HAL_CORTEXM_FPU and
> CYGINT_HAL_FPV4_SP_D16 (respecting FPU abstraction). There is, however a
> problem with unresolved conflicts when I click on CYGHWR_HAL_CORTEXM_FPU.
> Funny, but it resolves when I right-click RESOLVE in conflict window.

This seems to be okay in your latest version of the patch.

> 19) Your reg_offset() code works. It would be inappropriate for me to sign it,
> so I would be happy to see you in the list of contributors.
>
> Also hal_cortex_fpu.cdl is at least 40% your ideas so we would share the
> authorship.

Thanks. Just be sure to put your name first in the changelog though because
you've definitely done the hard work!

> 18) I added condition to suppress FPU context saving for exceptions when not
> selected ROM monitor or STUB.

Ah, but in cortexm_fpu.h you define CYGSEM_HAL_DEBUG_FPU if
CYGSEM_HAL_USE_ROM_MONITOR is set. But I don't think that one should cause it
to be set. CYGSEM_HAL_ROM_MONITOR yes, but not USE_ROM_MONITOR.

(In reply to comment #19)
>>> 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.
>>
>> It is only possible when automatic FPU context saving is disabled. With enabled
>> automatic FPU context saving we have no choice. 
> That's fine. If they've enabled CYGARC_CORTEXM_FPU_EXC_AUTOSAVE we can force
> this new CDL option on/off (whichever way round it is implemented).

I don't see where this happens or did it slip through the net? I would have
expected the conditional which enables CYGSEM_HAL_DEBUG_FPU to also have
checked CYGARC_CORTEXM_FPU_EXC_AUTOSAVE.

Incidentally, I think CYGARC_CORTEXM_FPU_EXC_AUTOSAVE should default to
enabled, partly because of that link you posted in comment #32:
http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html which means once the FPU
support is enabled, GCC may start generating code which uses the FPU registers;
but also simply because it's more important to be safe by default. People will
always be able to disable it.


(In reply to comment #27)
> There's however one essential addition: option for softfp ABI. Being able to
> link softfp code with -mfloat-abi=soft, the -mfloat-abi=softfp flag (when
> selected) is applied only to context.S and vectors.S. All other code compiles
> with -mfloat-abi=soft thus avoiding potential GCC's "optimizations" - see Note
> below.
>
> Note (RFC): according to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFIFGGE.html
> if hardware floating point option is selected, GCC may choose to use FPU
> registers even if there isn't floating point source. Please comment.
[and also the gcc-help link]

That's inconvenient. I think because of this it might be wise to always
explicitly use either -mfloat-abi=soft or -mfloat-abi=hard. So as well as
removing -mfloat-abi=hard in the current set of requires at the bottom of
hal_cortexm.cdl, we would also force in -mfloat-abi=soft (or -msoft-float). I
don't see a need for softfp - I think that's mostly aimed at people requiring
to interwork with prebuilt binaries.

Joey Ye's reply did also say that whatever he would do would be able to be
disabled (or maybe wouldn't be enabled by default), e.g. with
-fno-fpreg-for-nonfpdata. So I don't think we need to worry.

(In reply to comment #33)
> Therefore I decided to move all operations in a single CDL that is
> CYGBLD_ARCH_CPUFLAGS. Besides working smoothly it also solves the following
> problems: [snip]

I like this. Sneaky but effective!

> As I have a feeling that we are approaching our goal I would like to do some
> shaping work. One question is where is best place to put the tests. They stem
> from kernel tests, should I place them there or under Cortex-M architecture?

Definitely Cortex-M. Then the CDL which provides the list of tests
(CYGPKG_HAL_CORTEXM_TESTS) can also only build them if CYGPKG_KERNEL is present
and CYGHWR_HAL_CORTEXM_FPU on (just in case, you can see examples of how to
create the list of test names around the place in eCos, e.g. libc stdio).

The tests themselves may also need fine-tuning of more detailed configuration
options and can use CYG_TEST_NA for that if they are not applicable for the
configuration after all.

(In reply to comment #36)
> Changes: Default setting now is SOFT FPU. Rationale is that more often people
> will make integer applications even on systems with FPU. I may be completely
> wrong so comments are appreciated.

No I think you're right. There are other reasons to use the STM32F4 than the
FPU, for example.

Just a couple more little comments:

CYGHWR_HAL_FPV4_SP_D16 should be an option, not a component, and should
probably be a "calculated 1" bool rather than "flavor none".

In case you want to change your mind about HAL_CORTEXM_FPU_ENABLE/DISABLE being
macros instead of inline functions, I have belatedly realised it's possible to
use a GCC attribute: __attribute__((always_inline)). That would allow you to
keep them as inline functions if you like. I'll leave it up to you if you want
to go back and change it, given that what's there now works. I'm happy either
way.

Jifl

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #40 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-16 15:51:37 GMT ---
Hi Jifl

I'm sorry for little delay, but this time you caught me in France :).

(In reply to comment #39)
> Hi Ilija,
> 
> Excuse the length of this, since I have a few older things I never commented
> on. On the plus side, we're nearly there! Everything below is easy to sort out
> I think.

Thank you for extended elaboration. It seems that for many issues we have
consensus so they have been snipped out.

> > 18) I added condition to suppress FPU context saving for exceptions when not
> > selected ROM monitor or STUB.
> 
> Ah, but in cortexm_fpu.h you define CYGSEM_HAL_DEBUG_FPU if
> CYGSEM_HAL_USE_ROM_MONITOR is set. But I don't think that one should cause it
> to be set. CYGSEM_HAL_ROM_MONITOR yes, but not USE_ROM_MONITOR.
> 

OK. Now corrected.

> (In reply to comment #19)
> >>> 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.
> >>
> >> It is only possible when automatic FPU context saving is disabled. With enabled
> >> automatic FPU context saving we have no choice. 
> > That's fine. If they've enabled CYGARC_CORTEXM_FPU_EXC_AUTOSAVE we can force
> > this new CDL option on/off (whichever way round it is implemented).
> 
> I don't see where this happens or did it slip through the net? I would have
> expected the conditional which enables CYGSEM_HAL_DEBUG_FPU to also have
> checked CYGARC_CORTEXM_FPU_EXC_AUTOSAVE.
> 
> Incidentally, I think CYGARC_CORTEXM_FPU_EXC_AUTOSAVE should default to
> enabled, partly because of that link you posted in comment #32:
> http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html which means once the FPU
> support is enabled, GCC may start generating code which uses the FPU registers;

Fortunately this has been resolved in non issue after comment 32. And as you
have commented just below (in reply to comment 27).

> but also simply because it's more important to be safe by default. People will
> always be able to disable it.

Provided that no floating point is used in ISR/VSR, there is no danger. I find
it very hard [for me] to imagine application so badly needing floating point in
ISR/DSR. On the other hand enabling the auto saving _does_ use (waste) CPU time
and increase IRQ response time.

I would rather keep this option disabled by default, and those few that may
desperately need it, can easily enable it.

> 
> 
> (In reply to comment #27)

[snip]

> Joey Ye's reply did also say that whatever he would do would be able to be
> disabled (or maybe wouldn't be enabled by default), e.g. with
> -fno-fpreg-for-nonfpdata. So I don't think we need to worry.
> 
> (In reply to comment #33)

[snip]

> > As I have a feeling that we are approaching our goal I would like to do some
> > shaping work. One question is where is best place to put the tests. They stem
> > from kernel tests, should I place them there or under Cortex-M architecture?
> 
> Definitely Cortex-M. Then the CDL which provides the list of tests
> (CYGPKG_HAL_CORTEXM_TESTS) can also only build them if CYGPKG_KERNEL is present
> and CYGHWR_HAL_CORTEXM_FPU on (just in case, you can see examples of how to
> create the list of test names around the place in eCos, e.g. libc stdio).
> 
> The tests themselves may also need fine-tuning of more detailed configuration
> options and can use CYG_TEST_NA for that if they are not applicable for the
> configuration after all.

I think they should be applicable to all configurations that have enough
memory. They execute without problem on systems without FPU, and at least
thread_switch_fpu. is (IMO) gives interesting comparisons when run with and
without FPU.

> 
> (In reply to comment #36)
> > Changes: Default setting now is SOFT FPU. Rationale is that more often people
> > will make integer applications even on systems with FPU. I may be completely
> > wrong so comments are appreciated.
> 
> No I think you're right. There are other reasons to use the STM32F4 than the
> FPU, for example.

OK. Then we keep it as is.

> 
> Just a couple more little comments:
> 
> CYGHWR_HAL_FPV4_SP_D16 should be an option, not a component, and should
> probably be a "calculated 1" bool rather than "flavor none".
> 

Yes, it's nicer that way. I tried to make it look little bit more informative:

   cdl_option CYGHWR_HAL_FPV4_SP_D16 {
       flavor bool
       active_if { CYGINT_HAL_FPV4_SP_D16 }
       calculated { CYGINT_HAL_FPV4_SP_D16 && CYGHWR_HAL_CORTEXM_FPU }
       ...
  }

Out of topic: One question, just out of curiosity: is there a need in CDL for
cdl_option keyword? Isn't it just a terminal cdl_component?


> In case you want to change your mind about HAL_CORTEXM_FPU_ENABLE/DISABLE being
> macros instead of inline functions, I have belatedly realised it's possible to
> use a GCC attribute: __attribute__((always_inline)). That would allow you to
> keep them as inline functions if you like. I'll leave it up to you if you want
> to go back and change it, given that what's there now works. I'm happy either
> way.

I would not fix something that works, but I think there are some other inline
functions that I could treat.

What about adding 

#define CYGBLD_INLINE __attribute__((always_inline))

in infra/current/include/cyg_type.h?
Note: Proposal of the name above instead of CYGBLD_ATTRIB_ALWAYS_INLINE is
intentional.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #41 from Jonathan Larmour <jifl@ecoscentric.com> 2012-12-17 04:02:10 GMT ---
(In reply to comment #40)
> Hi Jifl
> 
> I'm sorry for little delay, but this time you caught me in France :).

I've been overseas again recently as well, and I'm sure you don't need to worry
about delays!

[snip about option for saving fpu regs with hal_fpu_exc_push/pop in
hal_default_exception_vsr)
> > >>
> > >> It is only possible when automatic FPU context saving is disabled. With enabled
> > >> automatic FPU context saving we have no choice. 
> > > That's fine. If they've enabled CYGARC_CORTEXM_FPU_EXC_AUTOSAVE we can force
> > > this new CDL option on/off (whichever way round it is implemented).
> > 
> > I don't see where this happens or did it slip through the net? I would have
> > expected the conditional which enables CYGSEM_HAL_DEBUG_FPU to also have
> > checked CYGARC_CORTEXM_FPU_EXC_AUTOSAVE.

Just to be clear, the above is a separate point to the below.

> > Incidentally, I think CYGARC_CORTEXM_FPU_EXC_AUTOSAVE should default to
> > enabled, partly because of that link you posted in comment #32:
> > http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html which means once the FPU
> > support is enabled, GCC may start generating code which uses the FPU registers;
> 
> Fortunately this has been resolved in non issue after comment 32. And as you
> have commented just below (in reply to comment 27).

Are you referring to my comment about it not being a problem that GCC may
generate code using the FPU regs for non-FP code? Being able to disable with a
compiler flag is fine for people definitely not using FP at all. However in the
above issue, my concern would be in applications which more generally _are_
using the FPU (so have enabled CYGHWR_HAL_CORTEXM_FPU), and so they should be
able to still use the compiler optimisation in general, which means the
compiler may use FPU registers in code which has nothing to do with FP.

> > but also simply because it's more important to be safe by default. People will
> > always be able to disable it.
> 
> Provided that no floating point is used in ISR/VSR, there is no danger. I find
> it very hard [for me] to imagine application so badly needing floating point in
> ISR/DSR. On the other hand enabling the auto saving _does_ use (waste) CPU time
> and increase IRQ response time.

In the applications I refer to above, it may mean that ISR/VSR code may not use
the FPU regs, but GCC may sneakily cause them to be used after all because of
that optimisation.

But I wonder if we've been missing something here. According to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFGGBJD.html
the Cortex-M can itself support lazy stacking, and you do enable the relevant
FPCCR bits in hal_init_fpu(). This means that the processor will lazily save
s0-s15 only if needed. We also don't need to worry about s16-s31 as the
procedure call standard means that they will automatically be saved/restored by
the compiler if it generates code which uses them. This should mean that there
is no penalty (except memory) of allowing FPU access in interrupt/exception
handlers.

I believe the only time we do need to save s16-31 is if this is a ROM monitor
or includes a GDB stub, i.e. if CYGSEM_HAL_DEBUG_FPU is defined (after the fix
we already talked about).

If you agree with my assessment (I may have missed something!), then I would
even go as far as saying that we could get rid of the _EXC_AUTOSAVE option,
because there probably isn't a reason to do anything else - i.e. we can
effectively have it permanently on.

> > > As I have a feeling that we are approaching our goal I would like to do some
> > > shaping work. One question is where is best place to put the tests. They stem
> > > from kernel tests, should I place them there or under Cortex-M architecture?
> > 
> > Definitely Cortex-M. Then the CDL which provides the list of tests
> > (CYGPKG_HAL_CORTEXM_TESTS) can also only build them if CYGPKG_KERNEL is present
> > and CYGHWR_HAL_CORTEXM_FPU on (just in case, you can see examples of how to
> > create the list of test names around the place in eCos, e.g. libc stdio).
> > 
> > The tests themselves may also need fine-tuning of more detailed configuration
> > options and can use CYG_TEST_NA for that if they are not applicable for the
> > configuration after all.
> 
> I think they should be applicable to all configurations that have enough
> memory.

Well... they all unconditionally include CYGPKG_KERNEL specific headers, so
must only be built if CYGPKG_KERNEL is enabled (which was my point).

But there are other issues with the tests, especially if placed outside of the
cortex-M HAL:-
 - fpinttest.c includes a call to CYGARC_MRS().

 - thread_switch_fpu.cxx's HAL_CLOCK_READ_NS is cortex-m specific

 - There are dependencies on strlen(), strcat(), fabs(), strcpy() at least - if
you want to use the compiler builtins, use e.g. __builtin_strlen instead).
Otherwise the functions may not be declared, or for fabs() <math.h> won't
exist.

 - alarm_fn calls time(), which obviously has no compiler builtin equivalent.

 - In thread_fp() in thread_switch_fpu.cxx: yes, GCC can optimise that out. It
is even allowed to do constant folding like that with no optimisation flag
(-O*). It didn't use to be the case for floating point constant, but now is (I
can't remember whether it's before or after 4.3, but it's true now).

 - I'm not sure I see the value in fpinttest.c in addition to fptestf.c?


> They execute without problem on systems without FPU, and at least
> thread_switch_fpu. is (IMO) gives interesting comparisons when run with and
> without FPU.

Hooray :).

> > Just a couple more little comments:
> > 
> > CYGHWR_HAL_FPV4_SP_D16 should be an option, not a component, and should
> > probably be a "calculated 1" bool rather than "flavor none".
> > 
> 
> Yes, it's nicer that way. I tried to make it look little bit more informative:
> 
>    cdl_option CYGHWR_HAL_FPV4_SP_D16 {
>        flavor bool
>        active_if { CYGINT_HAL_FPV4_SP_D16 }
>        calculated { CYGINT_HAL_FPV4_SP_D16 && CYGHWR_HAL_CORTEXM_FPU }
>        ...
>   }

Come to think of it, do we need CYGHWR_HAL_FPV4_SP_D16 at all? Can we not just
use CYGINT_HAL_FPV4_SP_D16 directly? And then make
CYGBLD_ARCH_CPUFLAG_FPV4SPD16 use (CYGHWR_HAL_CORTEXM_FPU &&
CYGINT_HAL_FPV4_SP_D16) instead? As far as I can tell that component is the
only user of that setting.

> Out of topic: One question, just out of curiosity: is there a need in CDL for
> cdl_option keyword? Isn't it just a terminal cdl_component?

There sort-of is, because it helps the UI. Options can have explicit parents,
including in other packages (the serial drivers have the most common example of
this). This means it's worth knowing whether something is really a component if
it is allowed to contain options underneath it, even if it currently doesn't.
Theoretically the UI could work out that it has to represent an option as what
we currently call a component if it has any children; however if we did that,
a) it would make some things intended as components look like options even
though they shouldn't; and b) allowing you to choose any option as a parent
would make it easier to make mistakes. So instead it's explicit which options
(i.e. components) are allowed to contain other options.

It's true there's no technical reason, only design reasons.

> What about adding 
> 
> #define CYGBLD_INLINE __attribute__((always_inline))
> 
> in infra/current/include/cyg_type.h?
> Note: Proposal of the name above instead of CYGBLD_ATTRIB_ALWAYS_INLINE is
> intentional.

I don't think that name is wise because "inline" can already mean lots of
different things. It has different meanings in old GCC, C99, C++, and if
prefaced with 'static' or 'extern'. Calling something just 'inline' isn't
enough.

What's your objection to CYGBLD_ATTRIB_ALWAYS_INLINE? Would
CYGBLD_ATTRIB_FORCE_INLINE be better? Or is it the length?

Jifl

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #42 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-17 14:04:59 GMT ---
Created an attachment (id=1999)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1999)
Alternative Cortex-M4F FPU support 121217 (probably outdated).

Hi Jifl

I will address other issues later. I think this one deserves special
explanation - once we have get to it.

(In reply to comment #41)
> (In reply to comment #40)
> But I wonder if we've been missing something here. According to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFGGBJD.html
> the Cortex-M can itself support lazy stacking, and you do enable the relevant
> FPCCR bits in hal_init_fpu(). This means that the processor will lazily save
> s0-s15 only if needed. We also don't need to worry about s16-s31 as the
> procedure call standard means that they will automatically be saved/restored by
> the compiler if it generates code which uses them. This should mean that there
> is no penalty (except memory) of allowing FPU access in interrupt/exception
> handlers.
> 
> I believe the only time we do need to save s16-31 is if this is a ROM monitor
> or includes a GDB stub, i.e. if CYGSEM_HAL_DEBUG_FPU is defined (after the fix
> we already talked about).
> 
> If you agree with my assessment (I may have missed something!), then I would
> even go as far as saying that we could get rid of the _EXC_AUTOSAVE option,
> because there probably isn't a reason to do anything else - i.e. we can
> effectively have it permanently on.
> 

I have tried this approach, indeed it was my first option, but I have abandoned
it. Here I attach what I believe to be the latest iteration. Note: this code
saves FPU state during interrupts for LAZY but does not for ALL.

Main reasons that I dropped this approach are:

1. Context switching takes more time, compared to submitted one, as now instead
of 1 we have 2 (or more - see point 3.) checks:
    a) Context switch initiated by interrupt/exception (such as tick) - then we
need to check FPCCR LSPACT flag;
    b) Context switch initiated by kernel without interrupt involved (such as
yield, priority change, semaphore, etc. Then we check CONTROL register FPCA
flag

Since we don't know the reason for context switch, or testing for that would
take even more time, we must check both flags compared to only one for
submitted version.

2. There are also some complexities coming from from variable CPU context
length. That requires some additional checks in, for instance pendable VSR,
interrupt end VSR, etc. that also takes some (ISR) time.

3. Although this code passes fpinttestf.c (or fptestf.c) there is at least one
case unattended:
   1. Thread foo uses FPU -> Context switch
   2. other threads ....
   3. foo gets cpu time but does not use fpu till next context switch. It's
context shall be marked FPU less.
   4. other threads
   5. foo scheduled again, now with lost FPU context!
This means that one more check is necessary in hal_thread_switch_context which
will add further delay.

To sum up, I opted for performance and simplicity. It's true I gave up the
possibility of floating point during ISR/VSR in LAZY scheme, but whoever needs
floating point within ISR/DSR still has the ALL scheme.

Last but not least, my knowledge of eCos kernel context switching is limited to
what I have learned during development of FPU support by code inspection and
tracing. I can imagine that you have a better insight.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #43 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-17 15:30:36 GMT ---
(In reply to comment #41)
> (In reply to comment #40)

[snip]

> 
> > > Incidentally, I think CYGARC_CORTEXM_FPU_EXC_AUTOSAVE should default to
> > > enabled, partly because of that link you posted in comment #32:
> > > http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html which means once the FPU
> > > support is enabled, GCC may start generating code which uses the FPU registers;
> > 
> > Fortunately this has been resolved in non issue after comment 32. And as you
> > have commented just below (in reply to comment 27).
> 
> Are you referring to my comment about it not being a problem that GCC may
> generate code using the FPU regs for non-FP code? Being able to disable with a
> compiler flag is fine for people definitely not using FP at all. However in the
> above issue, my concern would be in applications which more generally _are_
> using the FPU (so have enabled CYGHWR_HAL_CORTEXM_FPU), and so they should be
> able to still use the compiler optimisation in general, which means the
> compiler may use FPU registers in code which has nothing to do with FP.

I am referring to Joey Ye's statement that GCC does not use VFP registers for
non-FP code: http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html
and that once added, such optimization shall be accompanied with flag that
disables it. In such future we shall add (-fno-fpreg-for-nonfpdata) to CFLAGS.

[snip, replied in comment 42]

> > > > As I have a feeling that we are approaching our goal I would like to do some
> > > > shaping work. One question is where is best place to put the tests. They stem
> > > > from kernel tests, should I place them there or under Cortex-M architecture?
> > > 
> > > Definitely Cortex-M. Then the CDL which provides the list of tests
> > > (CYGPKG_HAL_CORTEXM_TESTS) can also only build them if CYGPKG_KERNEL is present
> > > and CYGHWR_HAL_CORTEXM_FPU on (just in case, you can see examples of how to
> > > create the list of test names around the place in eCos, e.g. libc stdio).
> > > 
> > > The tests themselves may also need fine-tuning of more detailed configuration
> > > options and can use CYG_TEST_NA for that if they are not applicable for the
> > > configuration after all.
> > 
> > I think they should be applicable to all configurations that have enough
> > memory.
> 
> Well... they all unconditionally include CYGPKG_KERNEL specific headers, so
> must only be built if CYGPKG_KERNEL is enabled (which was my point).
> 
> But there are other issues with the tests, especially if placed outside of the
> cortex-M HAL:-
>  - fpinttest.c includes a call to CYGARC_MRS().
> 
>  - thread_switch_fpu.cxx's HAL_CLOCK_READ_NS is cortex-m specific

I was not clear enough, sorry. I haven't stated that I have agreed for tests to
live in Cortex-M domain. My statement "applicable to all configurations ..." is
in that context.

> 
>  - There are dependencies on strlen(), strcat(), fabs(), strcpy() at least - if
> you want to use the compiler builtins, use e.g. __builtin_strlen instead).
> Otherwise the functions may not be declared, or for fabs() <math.h> won't
> exist.

I'll look at it.

> 
>  - alarm_fn calls time(), which obviously has no compiler builtin equivalent.
> 
>  - In thread_fp() in thread_switch_fpu.cxx: yes, GCC can optimise that out. It
> is even allowed to do constant folding like that with no optimisation flag
> (-O*). It didn't use to be the case for floating point constant, but now is (I
> can't remember whether it's before or after 4.3, but it's true now).

For this test it's only important for code to touch a VFP register. It does so
for my builds (GCC 4.6.3 and 4.7.2 with default eCos flags), but yes we should
be careful.

> 
>  - I'm not sure I see the value in fpinttest.c in addition to fptestf.c?

A replacement rather than addition. fpinttest.c runs a mix of pure integer and
float threads that enforces LAZY switches. It detected bugs (in LAZY context
switching scheme) that have passed fptestf.c

[snip]

> 
> > > Just a couple more little comments:
> > > 
> > > CYGHWR_HAL_FPV4_SP_D16 should be an option, not a component, and should
> > > probably be a "calculated 1" bool rather than "flavor none".
> > > 
> > 
> > Yes, it's nicer that way. I tried to make it look little bit more informative:
> > 
> >    cdl_option CYGHWR_HAL_FPV4_SP_D16 {
> >        flavor bool
> >        active_if { CYGINT_HAL_FPV4_SP_D16 }
> >        calculated { CYGINT_HAL_FPV4_SP_D16 && CYGHWR_HAL_CORTEXM_FPU }
> >        ...
> >   }
> 
> Come to think of it, do we need CYGHWR_HAL_FPV4_SP_D16 at all? Can we not just
> use CYGINT_HAL_FPV4_SP_D16 directly? And then make
> CYGBLD_ARCH_CPUFLAG_FPV4SPD16 use (CYGHWR_HAL_CORTEXM_FPU &&
> CYGINT_HAL_FPV4_SP_D16) instead? As far as I can tell that component is the
> only user of that setting.

CYGHWR_HAL_FPV4_SP_D16 also controls compilation of fpv4_sp_d16.c.

> 
> > Out of topic: 
[snip, thank you for explanation]

> > What about adding 
> > 
> > #define CYGBLD_INLINE __attribute__((always_inline))
> > 
> > in infra/current/include/cyg_type.h?
> > Note: Proposal of the name above instead of CYGBLD_ATTRIB_ALWAYS_INLINE is
> > intentional.
> 
> I don't think that name is wise because "inline" can already mean lots of
> different things. It has different meanings in old GCC, C99, C++, and if
> prefaced with 'static' or 'extern'. Calling something just 'inline' isn't
> enough.
> 
> What's your objection to CYGBLD_ATTRIB_ALWAYS_INLINE? Would
> CYGBLD_ATTRIB_FORCE_INLINE be better? Or is it the length?

Mainly length i guess. But I accept your arguments, in that case it could be
either CYGBLD_ATTRIB_ALWAYS_INLINE (ATTRIB should go with ALWAYS to reflect
original attribute name) or (shorter) CYGBLD_FORCE_INLINE 

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

--- Comment #44 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-21 13:42:02 GMT ---
Created an attachment (id=2002)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=2002)
Cortex-M4F Floating Point Support 121221 incremental to Att.1991

Here I submit incremental patch with accepted changes . The patch also contains
CDL for tests (tests body in a separate patch).

The patch is icremental to attachment 1991.

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1787|0                           |1
        is obsolete|                            |
   Attachment #1788|0                           |1
        is obsolete|                            |
   Attachment #1789|0                           |1
        is obsolete|                            |

--- Comment #45 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-21 13:53:10 GMT ---
Created an attachment (id=2003)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=2003)
Single precission floating point tests.

Tests added to Cortex-M architecture with following changes:
    - Dependencies on libraries have been removed by using built in functions.
    - Dependencies on Cortex-M arch have been removed, except
HAL_CLOCK_READ_NS. 
    - fpinttest.c renamed fpinttestf.c.
    - Added fpinttestf1.c for testing of NONE context switching scheme, as it
normally fails fpinttestf.c

Ilija

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

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2003|0                           |1
        is obsolete|                            |

--- Comment #46 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2074
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2074&action=edit
Single precission floating point tests. 130206

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1999|0                           |1
        is obsolete|                            |

--- Comment #47 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2075
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2075&action=edit
Alternative Cortex-M4F FPU support with Lazy Stacking 130206

Jifl

I did a backport of the code described in comment 42. The attached code should
patch onto current CVS, but it is for illustration only so many improvements in
CDL and GDB stub refined during our discussion are not implemented.

To start with, there are two aspects of Lazy state saving:
  - Context saving during exceptions. As you have noted Cortex-M features a
lazy FPU state saving. I shall call it Lazy Stacking - LS
  - Thread Context Switching that is implemented in context.S. I shall call it
Lazy Thread Switching - LTS

The attached code does both LS and LTS, unlike my "official proposal"
(currently attachment 1991) that does LTS exclusively.

First some comparative measurements of thread switching delay:

1. The (proposed) FPU disabling scheme - LTS only (LS-less)

Testing parameters:
   Thread switches:              128
   Time unit:         nanoseconds [ns]
                                 Confidence
     Ave     Min     Max Max-Min  Ave  Min  Samp  Function
  ======  ======  ======  ====== ==== ===== ===== ========
    2016    2016    2016       0  100% 100%   127 Thread switch: int-int
    2392    2392    2392       0  100% 100%   127 Thread switch: int-fpu
    2312    2312    2312       0  100% 100%   127 Thread switch: fpu-int
    2688    2688    2688       0  100% 100%   127 Thread switch: fpu-fpu
PASS:<Thread switching OK>
EXIT:<done>

2. Scheme with LS in addition to LTS

Testing parameters:
   Thread switches:              128
   Time unit:         nanoseconds [ns]
                                 Confidence
     Ave     Min     Max Max-Min  Ave  Min  Samp  Function
  ======  ======  ======  ====== ==== ===== ===== ======== 
    2088    2088    2088       0  100% 100%   127 Thread switch: int-int
    2432    2432    2432       0  100% 100%   127 Thread switch: int-fpu   
    2408    2408    2408       0  100% 100%   127 Thread switch: fpu-int
    2752    2752    2752       0  100% 100%   127 Thread switch: fpu-fpu
PASS:<Thread switching OK>
EXIT:<done>                                                            

As I mentioned in comment 42, itshows that LS-less code provides faster thread
switching. These measurements don't show interrupt processing times, but if you
compare the respective vectors.S files you can deduce that processing of
pendable SVC takes more time for LS code.

We can of course consider trading of processor time for having LS feature, but
I really find FPU usage in ISR academic. I understand and respect that ARM
fellows spent a great effort to provide us with LS but other than that I see no
practical reason to burn CPU cycles for something that very few, if any, would
want to use.

The attached code, also has a flaw that is described in comment 42 point 3. I
have developed a test that I will attach later. In order to fix this issue we
have to put more tests, at least 1 that will add more delay to context
switching.

Considering this, back then I did decide to omit FPU context saving for
exceptions. If there is a demand for FPU arithmetic we can consider it in
future _as option_ but for the time being I would say that we go with my
original proposal (LS-less).

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #48 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2076
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2076&action=edit
fpinttestf2 test that catches case described in comment 42 point 3

The attached text exposes issue with lazy thread switching for case described
in comment 42 point 3. Example of code having this issue is attachment 2075.

This test is general and could be placed in Kernel or common HAL tests

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|stm3240g_eval (ST           |All
                   |STM3240G-EVAL board)        |

--- Comment #49 from Ilija Kocho <ilijak@siva.com.mk> ---
(In reply to comment #41)
> But I wonder if we've been missing something here. According to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/
> DAFGGBJD.html the Cortex-M can itself support lazy stacking, and you do
> enable the relevant FPCCR bits in hal_init_fpu(). This means that the
> processor will lazily save s0-s15 only if needed. We also don't need to
> worry about s16-s31 as the procedure call standard means that they will
> automatically be saved/restored by the compiler if it generates code which
> uses them. This should mean that there is no penalty (except memory) of
> allowing FPU access in interrupt/exception handlers.
> 
> I believe the only time we do need to save s16-31 is if this is a ROM
> monitor or includes a GDB stub, i.e. if CYGSEM_HAL_DEBUG_FPU is defined
> (after the fix we already talked about).
> 
> If you agree with my assessment (I may have missed something!), then I would
> even go as far as saying that we could get rid of the _EXC_AUTOSAVE option,
> because there probably isn't a reason to do anything else - i.e. we can
> effectively have it permanently on.

Jifl
I missundersood you, lazy thinking :(. You are right, "ALL", as is, engages
Lazy Stacking indeed so we can drop the CYGARC_CORTEXM_FPU_EXC_AUTOSAVE option.
I am going to submit updated patch later today.

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1991|0                           |1
        is obsolete|                            |
   Attachment #2074|0                           |1
        is obsolete|                            |
   Attachment #2076|0                           |1
        is obsolete|                            |

--- Comment #50 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2078
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2078&action=edit
Cortex-M4F Floating Point Support 130207

Jifl

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.

Some notes:

Tests, included in this patch are general. The only Cortex-M depndent code is
HAL_CLOCK_READ_NS() that can be conditionaly replaced with HAL_CLOCK_READ() for
other architectures. Then we can consider placing these tests in kernel or
hal/common.

Speaking of HAL_CLOCK_READ_NS(), is it desirable for inclusion in cortex-m
headers?

And to continue with our off topic: I would go with CYGBLD_FORCE_INLINE if you
don't object, what is your opinion?

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1992|0                           |1
        is obsolete|                            |

--- Comment #51 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2079
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2079&action=edit
Kinetis Support 130207

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2002|0                           |1
        is obsolete|                            |

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2075|0                           |1
        is obsolete|                            |

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #52 from Jonathan Larmour <jifl@ecoscentric.com> ---
Hi Ilija,

(In reply to comment #49)
That's great that the lazy stacking will work after all.

In light of this, I am going to assume that comment #42 and comment #47 are now
irrelevant and can be ignored entirely. Let me know if not. Some other
responses:

(In reply to comment #43)
> > Are you referring to my comment about it not being a problem that GCC may
> > generate code using the FPU regs for non-FP code? Being able to disable with a
> > compiler flag is fine for people definitely not using FP at all. However in the
> > above issue, my concern would be in applications which more generally _are_
> > using the FPU (so have enabled CYGHWR_HAL_CORTEXM_FPU), and so they should be
> > able to still use the compiler optimisation in general, which means the
> > compiler may use FPU registers in code which has nothing to do with FP.
>
> I am referring to Joey Ye's statement that GCC does not use VFP registers
> for non-FP code: http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html
> and that once added, such optimization shall be accompanied with flag
> that disables it. In such future we shall add (-fno-fpreg-for-nonfpdata)
> to CFLAGS.

But then we would lose that optimisation, which is a shame. However I think
with the new code using CPU lazy stacking for exceptions and the removal of the
EXC_AUTOSAVE CDL option, all we would need in future if/when that gcc
optimisation happens, is to add -fno-fpreg-for-nonfpdata to CFLAGS if
(CYGINT_HAL_CORTEXM_FPU && !CYGHWR_HAL_CORTEXM_FPU). This also assumes by that
point we are also using -mcpu=cortex-m4 always on the M4 (uncommenting the
relevant line in the CDL). But for now we do nothing.

> >  - In thread_fp() in thread_switch_fpu.cxx: yes, GCC can optimise that out.
> > It is even allowed to do constant folding like that with no optimisation
> > flag (-O*). It didn't use to be the case for floating point constant, but
> > now is (I can't remember whether it's before or after 4.3, but it's true
> > now).
>
> For this test it's only important for code to touch a VFP register. It
> does so for my builds (GCC 4.6.3 and 4.7.2 with default eCos flags), but
> yes we should be careful.

I had a look and I think anything from GCC 4.5 may at some point optimise this
(even if you haven't had it happen yet):
http://gcc.gnu.org/gcc-4.5/changes.html talks about the use of the MPC library
under "General Optimizer Improvements", third bullet point. But yes, I now see
you use rand() which will nicely solve the problem :).

> > Come to think of it, do we need CYGHWR_HAL_FPV4_SP_D16 at all? Can we not
> > just use CYGINT_HAL_FPV4_SP_D16 directly? And then make
> > CYGBLD_ARCH_CPUFLAG_FPV4SPD16 use (CYGHWR_HAL_CORTEXM_FPU &&
> > CYGINT_HAL_FPV4_SP_D16) instead? As far as I can tell that component is the
> > only user of that setting.
>
> CYGHWR_HAL_FPV4_SP_D16 also controls compilation of fpv4_sp_d16.c.

Yes of course you're right.

(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))


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

> Tests, included in this patch are general. The only Cortex-M depndent code
> is HAL_CLOCK_READ_NS() that can be conditionaly replaced with
> HAL_CLOCK_READ() for other architectures. Then we can consider placing
> these tests in kernel or hal/common.

In principle, fpinttestf1 could run from cyg_start with no dependency on the
kernel, however that would require a few changes. But if you did change that,
then all the fp* tests may as well go in hal/common. Otherwise, if you don't
have time to adjust the tests, then since they all use kernel threads at
present then they should all just go in the kernel.

Hmm, out of interest, one of the contributions I intend to make when I have
time(!) is a complete revamp of the HAL_CLOCK* API with a lot more control and
the ability to convert to/from nsecs. Not today though...

> Speaking of HAL_CLOCK_READ_NS(), is it desirable for inclusion in cortex-m
> headers?

Despite my upcoming contribution, you can go ahead and add this to Cortex-M -
it's harmless even if it isn't the future :-). Obviously the tests still need
some changes to fall back on HAL_CLOCK_READ() still.

> And to continue with our off topic: I would go with CYGBLD_FORCE_INLINE
> if you don't object, what is your opinion?

CYGBLD_FORCE_INLINE or CYGBLD_ALWAYS_INLINE, either is fine with me.

A couple of other little things:

For the ChangeLog, to comply with the GNU standards, the format should really
be this:
2012-04-23  Ilija Kocho  <ilijak@siva.com.mk>
2012-04-23  Jonathan Larmour  <jifl@eCosCentric.com>

i.e. two consecutive lines. Still your name first obviously.

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!

Thanks for all this,

Jifl

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #53 from Ilija Kocho <ilijak@siva.com.mk> ---
Hi Jifl

(In reply to comment #52)
> Hi Ilija,
> 

[snip]

> (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?

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

There are also other problems, related to variable ISR stack frame length, that
are described in that comment #42 and/or comment #47.

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

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.

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.

Thanks for the comments.

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2078|0                           |1
        is obsolete|                            |

--- Comment #54 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2082
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2082&action=edit
Cortex-M4F Floating Point Support 130210 (tests moved to kernel)

Cortex-M architecure patch with tests removed (and placed under kernel/tests).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #55 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2083
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2083&action=edit
Add single precision floating point tests to kernel/tests

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #56 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2092
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2092&action=edit
Improve code build flag control - incremental to attachment 2082

Jifl

Here is a small patch (increment to attachment 2082) that eliminates the need
for compiler related changes (that were intended for switch to GCC 4.6).
Consequently the big comment together with commented pieces of code are gone.

Briefly, now if CYGHWR_HAL_CORTEXM == "M4"
 - FPU disable is permissible to both cortex-m3 and cortex-m4 so user can  
manually select either, just like in present CVS.
 - FPU enable enforces exclusion of cortex-m3 and inclusion of cortex-m4.
Subsequent FPU disable will not revert cortex-m3. The side effect is that 2
consecutive clicks will set build flags for Cortex-M4.

If CYGHWR_HAL_CORTEXM == "M4"
(manual) placement of -mcpu=cortex-m4 causes conflict

Now we can keep current default flags provided by platforms as are, even when
we switch to new compiler, though nothing can stop us to change them if we see
fit. 

Jifl,
With this I think we are go for commit. If you don't have objections I would
commit, some night during second half of next week (including the weekend) -
before Embedded World.

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

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 <jifl@ecoscentric.com> ---
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.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2082|0                           |1
        is obsolete|                            |
   Attachment #2092|0                           |1
        is obsolete|                            |

--- Comment #58 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2118
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2118&action=edit
Cortex-M4F Floating Point Support 130307

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2079|0                           |1
        is obsolete|                            |

--- Comment #59 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2119
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2119&action=edit
Kinetis FPU support 130307

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1993|0                           |1
        is obsolete|                            |

--- Comment #60 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2120
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2120&action=edit
STM32 FPU support 130307

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2083|0                           |1
        is obsolete|                            |

--- Comment #61 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2121
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2121&action=edit
Single precision FPU kernel tests 130307

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #62 from Ilija Kocho <ilijak@siva.com.mk> ---
Hi Jifl

(In reply to comment #57)

[snip]

I applied CYGTST_KERNEL_SKIP_MULTI_THREAD_FP_TEST and performed fixes to the
tests. In addition I renamed thread_switch_fpu.cxx to fpint_thread_switch.cxx -
sounds better to me and now all tests have same prefix.

> > > (In reply to comment #50)
[snip]

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

I think that for interrupts it would be a waste.
However this led me to idea to add check ASPEN bit of FPCCR to the
GDB_STUB_SAVEDREG_FPU_EXCEPTION_SET() so now RedBoot can determine if autosave
is enabled or disabled in runtime. Single RedBoot image to debug LAZY, ALL and
NONE. Implemented.

[snip]

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

There's no problem with using DSP instructions. CDL will allow you to set
-mcpu=cortex-m4 either manually or provide as default setting by platform (we
can do it when we adopt new compiler). As a side effect you can set cortex-m4
alone by sequence of enable/disable FPU.

> 
> Also of course as mentioned above, we still need to work out the last few
> niggles with those kernel tests.

I hope the tests are clean now. Some of the "features" were inherited from
original files. My next window for check-in is the upcoming weekend.

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #63 from Jonathan Larmour <jifl@ecoscentric.com> ---
The only things I've seen is:
- you need to align the thread stacks in fpint_thread_switch.cxx with
CYGBLD_ATTRIB_ALIGN_MAX
- The changelogs for the twr_k70f120m and kinetis variant HALs don't quite
cover all the changes made

But that's it!

(In reply to comment #62)
> 
> I think that for interrupts it would be a waste.
> However this led me to idea to add check ASPEN bit of FPCCR to the
> GDB_STUB_SAVEDREG_FPU_EXCEPTION_SET() so now RedBoot can determine if
> autosave is enabled or disabled in runtime. Single RedBoot image to debug
> LAZY, ALL and NONE. Implemented.

Good catch, I'd missed that that would have been a problem, so fixing it is
even better :).

> There's no problem with using DSP instructions. CDL will allow you to set
> -mcpu=cortex-m4 either manually or provide as default setting by platform

Ah, I'd missed that subtlety in the CDL. The M3 flag is excluded on the basis
of FPU support, the M4 flag is excluded on the basis of CYGHWR_HAL_CORTEXM.
Yes, that's definitely fine then.

> > Also of course as mentioned above, we still need to work out the last few
> > niggles with those kernel tests.
> 
> I hope the tests are clean now. Some of the "features" were inherited from
> original files. My next window for check-in is the upcoming weekend.

I think with the above few changes (stack alignment and changelogs), which are
too minor to re-post here, you can put it all in.

Thanks for all this really good work! It's taken quite a while, but I think
what we now have is a lot better as a result.

Jifl

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #64 from Ilija Kocho <ilijak@siva.com.mk> ---
Created attachment 2124
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2124&action=edit
Last fixes (comment 63) Incremental

Checked in. Tested after checkout, seems OK.
Here I attach last changes (ref comment 63) for completness.


Jifl, Thank you for good advices and for your time. Indeed we got not only
better, but also more beautiful product.

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |RESOLVED
         Resolution|---                         |CURRENTRELEASE

--- Comment #65 from Ilija Kocho <ilijak@siva.com.mk> ---
No complains so far. Closing.

-- 
You are receiving this mail because:
You are on the CC list for the bug.