This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: fix AT91 SPI driver


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]