fix AT91 SPI driver

fix AT91 SPI driver

[Bart Veer]

This fixes the AT91 SPI driver in the same way as the CortexM STM32
one.

Bart

2009-02-11  Bart Veer  <bartv@ecoscentric.com>

	* src/spi_at91.c (cyg_spi_at91_bus_init): turn into a prioritized
	constructor, make it a static and rename.

	* cdl/spi_at91.cdl: remove src/spi_at91_init.cxx
	
	* src/spi_at91_init.cxx: removed, no longer needed.

Index: cdl/spi_at91.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/spi/arm/at91/current/cdl/spi_at91.cdl,v
retrieving revision 1.3
diff -u -p -r1.3 spi_at91.cdl
--- cdl/spi_at91.cdl	29 Jan 2009 17:48:44 -0000	1.3
+++ cdl/spi_at91.cdl	11 Feb 2009 15:48:12 -0000
@@ -54,8 +54,6 @@ cdl_package CYGPKG_DEVS_SPI_ARM_AT91 {
     hardware
     include_dir   cyg/io
     compile       spi_at91.c
-    compile       -library=libextras.a spi_at91_init.cxx
-
 
     cdl_option CYGHWR_DEVS_SPI_ARM_AT91_BUS0 {
         display       "Enable support for SPI bus 0"
Index: src/spi_at91.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/spi/arm/at91/current/src/spi_at91.c,v
retrieving revision 1.8
diff -u -p -r1.8 spi_at91.c
--- src/spi_at91.c	29 Jan 2009 17:48:45 -0000	1.8
+++ src/spi_at91.c	11 Feb 2009 15:48:34 -0000
@@ -175,8 +175,8 @@ CYG_SPI_DEFINE_BUS_TABLE(cyg_spi_at91_de
 #endif
 // -------------------------------------------------------------------------
 
-void
-cyg_spi_at91_bus_init(void)
+static void CYGBLD_ATTRIB_C_INIT_PRI(CYG_INIT_BUS_SPI)
+spi_at91_bus_init(void)
 {
 
 #ifdef CYGHWR_DEVS_SPI_ARM_AT91_BUS0

Re: fix AT91 SPI driver

[Jonathan Larmour]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

Bart Veer wrote:
> This fixes the AT91 SPI driver in the same way as the CortexM STM32
> one.

I object to this change. It requires very recent tools (gcc 4.3.0+). It's 
acceptable for the Cortex HAL to do this as it is a new HAL, but not all 
the AT91 HALs.

Instead I am reinstating the build of spi_at91_init.cxx, but ifdeffing it 
on the presence of CYGBLD_ATTRIB_C_INIT_PRI, which is only defined if GCC 
is recent enough. Patch is attached and checked in.

If there are any other examples of this sort of thing, that I haven't seen 
go past, please fix similarly or let me know.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

[-- Attachment #2: at91.spi.init.patch --]
[-- Type: text/x-patch, Size: 6351 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/spi/arm/at91/current/ChangeLog,v
retrieving revision 1.10
diff -u -5 -p -r1.10 ChangeLog
--- ChangeLog	12 Feb 2009 14:50:07 -0000	1.10
+++ ChangeLog	16 Feb 2009 23:31:27 -0000
@@ -1,5 +1,15 @@
+2009-02-16  Jonathan Larmour  <jifl@eCosCentric.com>
+
+	* src/spi_at91.c (cyg_spi_at91_bus_init): Partially revert change of
+	2009-02-11. Define an empty CYGBLD_ATTRIB_C_INIT_PRI and rely on
+	spi_at91_init.cxx to call, in case the compiler does not supply
+	that macro (e.g. if too old).
+	* cdl/spi_at91.cdl: Revert change of 2009-02-11.
+	* src/spi_at91_init.cxx: Revive. Set priority to CYG_INIT_BUS_SPI.
+	Conditionalise on CYGBLD_ATTRIB_C_INIT_PRI.
+
 2009-02-12  Nick Garnett  <nickg@ecoscentric.com>
 
 	* cdl/spi_at91.cdl: Add a requires for CYGPKG_ERROR.
 
 2009-02-11  Bart Veer  <bartv@ecoscentric.com>
Index: cdl/spi_at91.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/spi/arm/at91/current/cdl/spi_at91.cdl,v
retrieving revision 1.5
diff -u -5 -p -r1.5 spi_at91.cdl
--- cdl/spi_at91.cdl	12 Feb 2009 14:50:07 -0000	1.5
+++ cdl/spi_at91.cdl	16 Feb 2009 23:31:28 -0000
@@ -53,10 +53,11 @@ cdl_package CYGPKG_DEVS_SPI_ARM_AT91 {
     requires      CYGPKG_HAL_ARM_AT91
     requires      CYGPKG_ERROR
     hardware
     include_dir   cyg/io
     compile       spi_at91.c
+    compile       -library=libextras.a spi_at91_init.cxx
 
     cdl_option CYGHWR_DEVS_SPI_ARM_AT91_BUS0 {
         display       "Enable support for SPI bus 0"
         flavor        bool
         default_value 1
Index: src/spi_at91.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/spi/arm/at91/current/src/spi_at91.c,v
retrieving revision 1.9
diff -u -5 -p -r1.9 spi_at91.c
--- src/spi_at91.c	11 Feb 2009 15:49:38 -0000	1.9
+++ src/spi_at91.c	16 Feb 2009 23:31:28 -0000
@@ -173,12 +173,18 @@ cyg_spi_at91_bus_t cyg_spi_at91_bus1 = {
 
 CYG_SPI_DEFINE_BUS_TABLE(cyg_spi_at91_device_t, 1);
 #endif
 // -------------------------------------------------------------------------
 
-static void CYGBLD_ATTRIB_C_INIT_PRI(CYG_INIT_BUS_SPI)
-spi_at91_bus_init(void)
+// If C constructor with init priority functionality is not in compiler,
+// rely on spi_at91_init.cxx to init us.
+#ifndef CYGBLD_ATTRIB_C_INIT_PRI
+# define CYGBLD_ATTRIB_C_INIT_PRI(x)
+#endif
+
+void CYGBLD_ATTRIB_C_INIT_PRI(CYG_INIT_BUS_SPI)
+cyg_spi_at91_bus_init(void)
 {
 
 #ifdef CYGHWR_DEVS_SPI_ARM_AT91_BUS0
    // NOTE: here we let the SPI controller control 
    //       the data in, out and clock signals, but 
Index: src/spi_at91_init.cxx
===================================================================
RCS file: src/spi_at91_init.cxx
diff -N src/spi_at91_init.cxx
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ src/spi_at91_init.cxx	16 Feb 2009 23:31:28 -0000
@@ -0,0 +1,73 @@
+//==========================================================================
+//
+//      spi_at91_init.cxx
+//
+//      Atmel AT91 (ARM) SPI bus init 
+//
+//==========================================================================
+// ####ECOSGPLCOPYRIGHTBEGIN####                                            
+// -------------------------------------------                              
+// This file is part of eCos, the Embedded Configurable Operating System.   
+// Copyright (C) 1998, 1999, 2000, 2001, 2002, 2009 Free Software Foundation, Inc.
+//
+// eCos is free software; you can redistribute it and/or modify it under    
+// the terms of the GNU General Public License as published by the Free     
+// Software Foundation; either version 2 or (at your option) any later      
+// version.                                                                 
+//
+// eCos is distributed in the hope that it will be useful, but WITHOUT      
+// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or    
+// FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License    
+// for more details.                                                        
+//
+// You should have received a copy of the GNU General Public License        
+// along with eCos; if not, write to the Free Software Foundation, Inc.,    
+// 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.            
+//
+// As a special exception, if other files instantiate templates or use      
+// macros or inline functions from this file, or you compile this file      
+// and link it with other works to produce a work based on this file,       
+// this file does not by itself cause the resulting work to be covered by   
+// the GNU General Public License. However the source code for this file    
+// must still be made available in accordance with section (3) of the GNU   
+// General Public License v2.                                               
+//
+// This exception does not invalidate any other reasons why a work based    
+// on this file might be covered by the GNU General Public License.         
+// -------------------------------------------                              
+// ####ECOSGPLCOPYRIGHTEND####                                              
+//==========================================================================
+//#####DESCRIPTIONBEGIN####
+//
+// Author(s):     Savin Zlobec <savin@elatec.si> 
+// Date:          2004-08-25
+//
+//####DESCRIPTIONEND####
+//
+//==========================================================================
+
+#include <cyg/infra/cyg_type.h>
+
+// This file is not needed if we support CYGBLD_ATTRIB_C_INIT_PRI, as the
+// init happens directly in spi_at91.c then.
+#ifndef CYGBLD_ATTRIB_C_INIT_PRI
+
+// -------------------------------------------------------------------------
+
+externC void cyg_spi_at91_bus_init(void);
+
+class cyg_spi_at91_bus_init_class {
+public:
+    cyg_spi_at91_bus_init_class(void) {
+        cyg_spi_at91_bus_init();
+    }
+};
+
+// -------------------------------------------------------------------------
+
+static cyg_spi_at91_bus_init_class spi_at91_bus_init CYGBLD_ATTRIB_INIT_PRI(CYG_INIT_BUS_SPI);
+
+#endif // ifndef CYGBLD_ATTRIB_C_INIT_PRI
+
+// -------------------------------------------------------------------------
+// EOF spi_at91_init.cxx

Re: fix AT91 SPI driver

[Bart Veer]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]

>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:

    Jifl> Bart Veer wrote:
    >> This fixes the AT91 SPI driver in the same way as the CortexM
    >> STM32 one.

    Jifl> I object to this change. It requires very recent tools (gcc
    Jifl> 4.3.0+). It's acceptable for the Cortex HAL to do this as it
    Jifl> is a new HAL, but not all the AT91 HALs.

    Jifl> Instead I am reinstating the build of spi_at91_init.cxx, but
    Jifl> ifdeffing it on the presence of CYGBLD_ATTRIB_C_INIT_PRI,
    Jifl> which is only defined if GCC is recent enough. Patch is
    Jifl> attached and checked in.

I did think about this. We have already changed the relevant platform
HALs to default to arm-eabi-gcc instead of arm-elf-gcc, so the default
behaviour is already to require an up-to-date compiler that provides
the necessary functionality. I expect some 3.0 testing of AT91 tools
with the new tools, but none with the old tools.

If anybody really does want to keep building AT91 targets with old
tools then they can continue to use old sources: either for all of
eCos, or just for the SPI driver. I really don't see this as a big
problem.

Partially reverting the change means there is a still an SPI driver in
the source tree which gets linker garbage collection wrong, albeit
only if built with an old compiler. I really wanted all SPI drivers to
get this right, since any of the existing drivers may get used as the
starting point for a new driver. There are other ways in which it
could have been fixed, e.g. turning spi_at91.c into spi_at91.cxx as
per the LPC driver, or something less pleasant like:

#ifdef CYGBLD_ATTRIB_C_INIT_PRI
    ...
#else
static void (*spi_at91_init_constructor)(void)  __attribute__((unused)) CYGBLD_ATTRIB_SECTION(".ctors.33535")   = &spi_at91_bus_init;
#endif

which makes the fairly safe assumption that any pre-4.3 ARM toolchains
use .ctors.

So I would have preferred it if you had not reinstated the _init.cxx
module. But if you really think it is important, I'll live with it.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

Re: fix AT91 SPI driver

[Jonathan Larmour]

Bart Veer wrote:
>>>>>>"Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:
> 
> 
>     Jifl> Bart Veer wrote:
>     >> This fixes the AT91 SPI driver in the same way as the CortexM
>     >> STM32 one.
> 
>     Jifl> I object to this change. It requires very recent tools (gcc
>     Jifl> 4.3.0+). It's acceptable for the Cortex HAL to do this as it
>     Jifl> is a new HAL, but not all the AT91 HALs.
> 
>     Jifl> Instead I am reinstating the build of spi_at91_init.cxx, but
>     Jifl> ifdeffing it on the presence of CYGBLD_ATTRIB_C_INIT_PRI,
>     Jifl> which is only defined if GCC is recent enough. Patch is
>     Jifl> attached and checked in.
> 
> I did think about this. We have already changed the relevant platform
> HALs to default to arm-eabi-gcc instead of arm-elf-gcc, so the default
> behaviour is already to require an up-to-date compiler that provides
> the necessary functionality. I expect some 3.0 testing of AT91 tools
> with the new tools, but none with the old tools.

Just because something isn't tested doesn't mean it should be avoidably 
broken. It's different from preventing use of old tools from working.

> If anybody really does want to keep building AT91 targets with old
> tools then they can continue to use old sources: either for all of
> eCos, or just for the SPI driver. I really don't see this as a big
> problem.

GCC 4.3.x is too recent to mandate. If it turns out there are problems 
with some people's sources (maybe not exposed in our own testing) they 
have no fallback of an earlier version. And they may not want to change to 
the bleeding edge. Or there may be all sorts of reasons why they can't.

> Partially reverting the change means there is a still an SPI driver in
> the source tree which gets linker garbage collection wrong, albeit
> only if built with an old compiler. I really wanted all SPI drivers to
> get this right, since any of the existing drivers may get used as the
> starting point for a new driver. There are other ways in which it
> could have been fixed, e.g. turning spi_at91.c into spi_at91.cxx as
> per the LPC driver, or something less pleasant like:
> 
> #ifdef CYGBLD_ATTRIB_C_INIT_PRI
>     ...
> #else
> static void (*spi_at91_init_constructor)(void)  __attribute__((unused)) CYGBLD_ATTRIB_SECTION(".ctors.33535")   = &spi_at91_bus_init;
> #endif
> 
> which makes the fairly safe assumption that any pre-4.3 ARM toolchains
> use .ctors.

That's incompatible with the eabi. And people may be using eabi tools 
older than 4.3.x. And keying off version wouldn't be a solution as we 
should still be allowing people to use arm-elf if they want. arm-elf isn't 
obsolete. Nor is it deprecated.

If you would like to change it in line with the LPC driver, please feel 
free - that seems a fine solution.

> So I would have preferred it if you had not reinstated the _init.cxx
> module. But if you really think it is important, I'll live with it.

I think allowing GCC prior to 4.3.x is essential. Some people are using 
earlier GCC 4. eCosCentric still don't officially ship 4.3.x yet. Some 
people are using tools provided by codesourcery (particularly true for 
eabi). It's a simple matter of changing a compiler prefix in comparison.

So for the record, any change which breaks older tools can be considered a 
bug which wants to be fixed (unless the fix would cause problems with 
newer tools and ifdeffing isn't practicable).

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["The best things in life aren't things."]------      Opinions==mine