Re: tidy STM32 SPI driver

Re: tidy STM32 SPI driver

[Chris Holgate]

Apologies for breaking the thread, but I don't actually subscribe to the
patches list so can't just hit reply!

Bart wrote:

> This SPI driver did not use linker garbage collection the way I
> intended for SPI drivers. It created a C++ object in
> src/spi_stm32_init.cxx which was places in libextras.a. Hence the C++
> object was always part of any application link, and because of the
> KEEP(*.ctors) in the linker script its constructor would never get
> garbage-collected. Net result: all applications would end up with the
> C++ object, the constructor, and the cyg_spi_cortexm_stm32_init()
> function, even if SPI was never used.

That bit of code was essentially a cut and paste from the AT91 SPI
driver, which sounds as though it may suffer from the same problem.

> I also looked at moving the three SPI buses into separate modules,
> built unconditionally. Again linker magic would ensure that only buses
> actually used by the application would end up in the application, with
> no need for developers to enable the relevant CDL components. This
> does not look entirely feasible right now, there are too many per-bus
> configuration options.

Getting rid of the 'bus enable' checkboxes would definitely be a good thing.

Generally, the configuration options are only used to statically
initialise the 3 bus data structures - so it should be fairly easy to
pull each bus data structure declaration into a separate compile unit
with per-bus constructors and function tables.  The common core
functions would then only be pulled in if at least one of the per-bus
compile units were actually referenced.

I'll have a look at it.

Chris.

Re: tidy STM32 SPI driver

[Bart Veer]

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

>>>>> "Chris" == Chris Holgate <chris@zynaptic.com> writes:

    >> This SPI driver did not use linker garbage collection the way I
    >> intended for SPI drivers. It created a C++ object in
    >> src/spi_stm32_init.cxx which was places in libextras.a. Hence
    >> the C++ object was always part of any application link, and
    >> because of the KEEP(*.ctors) in the linker script its
    >> constructor would never get garbage-collected. Net result: all
    >> applications would end up with the C++ object, the constructor,
    >> and the cyg_spi_cortexm_stm32_init() function, even if SPI was
    >> never used.

    Chris> That bit of code was essentially a cut and paste from the
    Chris> AT91 SPI driver, which sounds as though it may suffer from
    Chris> the same problem.

It does - and is next on my list of things to fix.

    >> I also looked at moving the three SPI buses into separate
    >> modules, built unconditionally. Again linker magic would ensure
    >> that only buses actually used by the application would end up
    >> in the application, with no need for developers to enable the
    >> relevant CDL components. This does not look entirely feasible
    >> right now, there are too many per-bus configuration options.

    Chris> Getting rid of the 'bus enable' checkboxes would definitely
    Chris> be a good thing.

    Chris> Generally, the configuration options are only used to
    Chris> statically initialise the 3 bus data structures - so it
    Chris> should be fairly easy to pull each bus data structure
    Chris> declaration into a separate compile unit with per-bus
    Chris> constructors and function tables. The common core functions
    Chris> would then only be pulled in if at least one of the per-bus
    Chris> compile units were actually referenced.

That is the general idea. Ideally things always "just work", with some
careful coding and linker garbage collection eliminating any
functionality that the application does not actually need.
Configuration options can be used when that is not possible, but do
require more effort from application developers - especially in the
absence of any driver documentation.

For the chip select handling, if I understand the code correctly then
currently the SPI driver is responsible for both setting up the GPIO
pins and for driving them. There are other ways of handling this. One
approach is for the platform HAL to set up all the pins early on
during initialization, before the static constructors, on the theory
that the platform HAL author knows what devices are attached to which
pins. That saves having to do pin setup in each driver, so saves a bit
more code.

A more flexible approach is used in the coldfire HALs, see the mcf5272
processor HAL's gpio.cdl and CYGHWR_HAL_M68K_MCF5272_BOARD_PINS in the
m5272c3 platform HAL. That exposes all the pin settings to the
configuration system, with the platform HAL providing default
settings. The approach makes it possible to support boards with
expansion connectors etc., where the platform HAL author does not
necessarily know exactly what hardware will be used. It offers a lot
of flexibility, and involves very little target-side code or data, but
at the cost of significant programming effort.

If pin setup can be left to the platform HAL then the SPI driver only
needs to worry about driving the chip select pins. That can be
handled by putting the pin id into the SPI device structure instead of
the bus structure. That should save more code and data.

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: tidy STM32 SPI driver

[Nick Garnett]

Bart Veer <bartv@ecoscentric.com> writes:

> For the chip select handling, if I understand the code correctly then
> currently the SPI driver is responsible for both setting up the GPIO
> pins and for driving them. There are other ways of handling this. One
> approach is for the platform HAL to set up all the pins early on
> during initialization, before the static constructors, on the theory
> that the platform HAL author knows what devices are attached to which
> pins. That saves having to do pin setup in each driver, so saves a bit
> more code.


However, I have come to almost the exact opposite conclusion. The HAL
should do the minimum necessary to get the board up and running.
Device drivers should then configure their own GPIO lines when they
start up. This is the approach I have tried to adopt in the Cortex-M
HAL, and have implemented macros and functions to make this easy to
do.

This simplifies the platform HAL since the author doesn't need to
worry about which devices are present, and avoids ending up with a
maze of ifdefs, or a lot of complicated CDL to define the pin
configuration. With multi-function pins, it ensures that they get set
to the correct mode for each of the loaded drivers. If a driver is
configured but never used, it's pins don't get changed. This also make
it easier to implement dynamic reconfiguration of the pins if they
need to be shared between different devices.

The cost of this is not high since the pin configuration work is
usually passed off to shared functions in the HAL. 


-- 
Nick Garnett                                        eCos Kernel Architect
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
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: tidy STM32 SPI driver

[Chris Holgate]

Bart Veer wrote:
> 
> For the chip select handling, if I understand the code correctly then
> currently the SPI driver is responsible for both setting up the GPIO
> pins and for driving them. There are other ways of handling this. One
> approach is for the platform HAL to set up all the pins early on
> during initialization, before the static constructors, on the theory
> that the platform HAL author knows what devices are attached to which
> pins. That saves having to do pin setup in each driver, so saves a bit
> more code.

Possibly, although the STM32E-EVAL board has a lot of flexibility in
terms of how it can be configured.  It would be quite difficult to
second-guess how a 'typical' developer would want to set it up.

> A more flexible approach is used in the coldfire HALs, see the mcf5272
> processor HAL's gpio.cdl and CYGHWR_HAL_M68K_MCF5272_BOARD_PINS in the
> m5272c3 platform HAL. That exposes all the pin settings to the
> configuration system, with the platform HAL providing default
> settings. The approach makes it possible to support boards with
> expansion connectors etc., where the platform HAL author does not
> necessarily know exactly what hardware will be used. It offers a lot
> of flexibility, and involves very little target-side code or data, but
> at the cost of significant programming effort.

If chip select setup is to be done in the HAL I suspect that something
like this would probably be required.  I'm not volunteering - I really
need to get cracking with my application!

> If pin setup can be left to the platform HAL then the SPI driver only
> needs to worry about driving the chip select pins. That can be
> handled by putting the pin id into the SPI device structure instead of
> the bus structure. That should save more code and data.

Now you come to mention it, the device structure is the most logical
place to put the chip select pin ID.  The only reason it's not there is
because of the requirement to set up the pin during initialisation.

Given that the CYG_DEVS_SPI_CORTEXM_STM32_DEVICE macro is already used
to hide all the bus attachment and type aliasing, it would also be
possible for that macro to automatically generate a static
initialisation function to set up the chip select pin.  That way there
is no need for the SPI bus data structure to hold any information about
chip selects and that configuration option can be discarded.

Chris.

tidy STM32 SPI driver

[Bart Veer]

This SPI driver did not use linker garbage collection the way I
intended for SPI drivers. It created a C++ object in
src/spi_stm32_init.cxx which was places in libextras.a. Hence the C++
object was always part of any application link, and because of the
KEEP(*.ctors) in the linker script its constructor would never get
garbage-collected. Net result: all applications would end up with the
C++ object, the constructor, and the cyg_spi_cortexm_stm32_init()
function, even if SPI was never used.

This patch should fix this. src/spi_stm32_init.cxx is eliminated
completely. cyg_spi_cortexm_stm32_init() is declared as a prioritized
constructor, and made static because it is no longer accessed from the
outside the module.

  If the application does not make use of any SPI functions then there
  will be no references to any of the SPI bus objects and the linker
  will not pull in spi_stm32.o from libtarget.a. So the linker never
  sees any of the driver code, including the constructor, and there
  are no overheads.

  If the application does make use of SPI then it will reference one
  of the SPI bus objects and the linker will pull in spi_stm32.o from
  the library. The module is still subject to linker garbage
  collection, although that won't have much effect because of the
  table of function pointers. The init function is a constructor so
  the linker will keep it because of the KEEP(*.ctors). And everything
  should work as intended.

I also looked at moving the three SPI buses into separate modules,
built unconditionally. Again linker magic would ensure that only buses
actually used by the application would end up in the application, with
no need for developers to enable the relevant CDL components. This
does not look entirely feasible right now, there are too many per-bus
configuration options.

Bart

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

	* src/spi_stm32.c (cyg_spi_cortexm_stm32_init): mark as
	prioritized constructor and rename.

	* cdl/spi_stm32.cdl: stop building spi_stm32_init.cxx

	* src/spi_stm32_init.cxx: removed, no longer needed.

Index: cdl/spi_stm32.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/spi/cortexm/stm32/current/cdl/spi_stm32.cdl,v
retrieving revision 1.2
diff -u -p -r1.2 spi_stm32.cdl
--- cdl/spi_stm32.cdl	10 Feb 2009 16:12:53 -0000	1.2
+++ cdl/spi_stm32.cdl	10 Feb 2009 22:29:42 -0000
@@ -59,7 +59,6 @@ cdl_package CYGPKG_DEVS_SPI_CORTEXM_STM3
     hardware
     include_dir   cyg/io
     compile       spi_stm32.c
-    compile       -library=libextras.a spi_stm32_init.cxx
 
 cdl_option CYGNUM_DEVS_SPI_CORTEXM_STM32_PIN_TOGGLE_RATE {
     display       "Pin toggle rate"
Index: src/spi_stm32.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/spi/cortexm/stm32/current/src/spi_stm32.c,v
retrieving revision 1.4
diff -u -p -r1.4 spi_stm32.c
--- src/spi_stm32.c	10 Feb 2009 16:12:54 -0000	1.4
+++ src/spi_stm32.c	10 Feb 2009 22:29:56 -0000
@@ -611,8 +611,8 @@ static void spi_transaction_dma 
 //-----------------------------------------------------------------------------
 // Initialise SPI interfaces on startup.
 
-void cyg_spi_cortexm_stm32_init 
-  (void)
+static void CYGBLD_ATTRIB_C_INIT_PRI(CYG_INIT_BUS_SPI)
+stm32_spi_init(void)
 {
 #if defined(CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS3) && \
     defined(CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS3_DISABLE_DEBUG_PORT)