drivers/spi: reworked SPI driver interface

[haukepetersen]

This PR replaces #3216

Finally here my new approach to optimizing our SPI interface. Major design changes:

• chip select is now handled internally by the SPI driver implementations
• both SW and HW chip select are possible with this interface
• Multiple devices with different SPI configurations can share a single SPI device (so this solves Bus initialization procedure #2528)
• calling acquire/release is now a MUST
• removed slave mode for now -> I think this should go into it's own interface at some point

I implemented the changes for the stm32f4discovery (stm32f4 CPU) to demonstrate how this can be done. On the stm32f4, the new implementations uses roughly HALF the ROM (~400 instead of ~800 byte), while being more flexible...

[jnohlgard]

Are there any drawbacks to using a pointer as the dev parameter instead of an integer/enum?
It would make it less hacky to add hardware specific features like the CTAS timing parameter choice on Kinetis. The alternative solution would be to do what is done with GPIO and make SPI_DEV(DEVNUMBER, CTAS) for Kinetis (which is a perfectly fine solution as well IMHO).

[haukepetersen]

The advantage of a plain int/enum type for dev is, that you can specify the device from anywhere as static, possible const value.

What is the CTAS parameter doing exactly, and do you have this different for different devices on the same bus? And what are the possible values?

I think SPI_DEV(DEVNUMBER, CTAS) would not really work, as this would lead to redundant use of memory (as the pin + clock config are identical for all DEVNUMBER).

[jnohlgard]

@haukepetersen the CTAS parameter allows us to switch between two simultaneous timing configurations in hardware. On mulle we use it to allow using both the rf212b and lis3dh devices on the same bus without reconfiguring/reinitializing the hardware. The rf212b requires mode 0 and <7.5 MHz SCK (+ some other timing settings) while the lis3dh requires mode 3. This was more of a minor oversight when we chose the components for the board in the initial design phase but it's a bigger hassle to change the board design than to adapt the software to use two different SPI settings for the devices when the MCU supports it.

edit, Added excerpt from Kinetis reference manual:

In Master mode, the CTARs define combinations of transfer attributes such as frame size,
clock phase and polarity, data bit ordering, baud rate, and various delays. In Slave mode,
a subset of the fields in CTAR0 are used to set the slave transfer attributes.
When the module is configured as an SPI master, the CTAS field in the command portion
of the TX FIFO entry selects which of the CTAR register is used. When the module is
configured as an SPI bus slave, the CTAR0 is used.

edit2:

Other boards could potentially use it to support two different speeds on the same bus, for example a high speed setting for a flash chip and a low speed setting for a sensor. Many flash chips support >50 MHz speeds.

[haukepetersen]

I see, sounds very Freescale specific, I have not seen something like this for any other MCU.

Running slaves with the need of different configurations is possible with the interface changes as I proposed them, you even have an unlimited number of configurations :-) For implementations this is intended, so that they re-configure the bus every time the acquire function is called. Of course this has a slight overhead, but this overhead is very small (like writing 1-2 particular register(s) with mostly constant values, so I guess ~10-20 CPU cycles at the most).

Now for the Kinetis MCUs: I would actually tend to only use only a single CTAR. Of course this is (very little) more inefficient, but I think we gain more through simplicity here.

[jnohlgard]

With the proposed API design for the acquire function there would have to be some switches, a little math and some bit fiddling to arrive at a suitable configuration register value from the enum value given (I'm guessing for most platforms, but at least for Kinetis). If we on the other hand use either a bus context struct or a cached register value (both would be opaque to the applications and slave device drivers), it would only be necessary to copy the register value in software from the cached value to the actual hardware register. The values could be dynamically filled by the init function, or statically in e.g. periph_conf.h.

Actually, I would like even more fine grained control over timing parameters. This is already somewhat implemented in the Kinetis SPI driver, but it would be a good addition to at least be able to set t_delay, t_quiet and t_cs,dis in the below diagram, but also baud rate other than the fixed enum values might come in handy.

(figure taken from a random Google result for SPI timing diagram, used without permission)

[haukepetersen]

some switches, a little math and some bit fiddling

mostly only little fiddling and assignment of 1-3 registers, so as said, very minimal overhead

If we on the other hand use either a bus context struct or a cached register value (both would be opaque to the applications and slave device drivers),

I might see the benefit, but I fail to see how to implement this opaque, easy to use and as memory efficient. Mind to show an example?

Regarding timings: in my opinion they should not be part of the interface: (i) such settings are only supported by < 25% of the MCUs and (ii) they have practically only a minor relevance. We have not encountered a single device (yet), which needed special treatment and so in my opinion it a 1 in 1000 problem where these settings might be important is not worth the extra code.

[haukepetersen]

rebased

[OlegHahm]

What's the state here?

[jnohlgard]

I don't have a code example for the SPI context, but one piece of hardware that I had some issues with is the AT86RF212B which if running at the maximum baud rate described in the data sheet (7.5 MHz) will need extra delays for t_delay and t_quiet (at Eistec, we had some hard-to-trace problems with settings not registering etc on the transceiver at first, until we found that the delay timings were too fast compared to the data sheet spec even though the baud rate was within spec).

[haukepetersen]

I think we never saw that behavior, since as long as we were treating the CS lines manually via the GPIO module, the timings were always relaxed enough I guess. But I would still not make timings as part of the interface, I think it should be enough to make them configurable on a board-basis via the periph_conf.h if needed.

[haukepetersen]

So let's recap where we are with this PR:

The proposed changes solve some open issues:

• enable use of HW chip select
• allow multiple devices on the same bus with different parameters
• allow for more efficient implementation (by overriding parameters on a per CPU base)

Open points:

• configuration of timing parameters
• support for things like the Freescale CTAR stuff

IMHO, the timing can be done tuned on a per-board base. Most CPUs don't have support for influencing these anyway, and for those that can be influenced, we can put default parameters into the board's periph_conf.h and let the designer of the board adjust these if really needed.

So the biggest question that we have to find a solution to is if there is a better approach on defining devices, e.g. if we should introduce something like a driver specific context structs, that contain all the peripheral configuration for a device driver and which needs to be CPU and driver specific. I personally think this would complicate matters and increase code size quite a bit, while only giving minor benefits for certain use cases. Further nobody has come up with a concrete example how this could be done. So I would tend to stay with the current concept as proposed here.

@gebart, @kaspar030, @jfischer-phytec-iot, @thomaseichinger: let's try to find an agreement ASAP and get this over with, so we can focus on more fun stuff the the peripheral driver remodeling...

[PeterKietzmann]

Hi. Sorry for hopping in so late. Just wanted to state that I also faced t_delay timing problems on the nrf24l01p driver. Because of that I needed to introduce delays between CS_LOW and SPI_START. Would be helpful to have this configurable in an easy way with internally handled CS-lines. I agree that any adaptions in that direction would increase size and complexity. Currently I have not allround-concept in mind but I can test the nrf24l01p driver once a proposal is here.

[haukepetersen]

hm, can you elaborate? By looking into the nrf24l01+ datasheet (p. 52-55), I would say the problem with the timings is rather the other way around -> we have to be careful not to be too slow. The minimum time for 't_DEALY' (as in the diagram above) is 2ns, which should not cause any trouble with default SPI parameter?!

[PeterKietzmann]

Whoops, you're right. Seems I confused something. So, the delay I talked about was definitively needed on some platforms but as it seems it was unrelated to the device timing requirements but rather to anything else (periph driver implementation?). We should consider that once the nrf24l01p driver is reworked. Please remind me then ;-)

[haukepetersen]

Ok, then I got it right. @gebart: looking at the AT86RF233 datasheet, I see similar numbers in the range of ns, so you had trouble in being faster than that for t_DELAY?

But anyway, this solves not the problem in general. So to have some kind of control over the timings (which is now more of an issue once the CS line is handled internally), first I would like to make the following simplification: Can we for sake of simplicity boil down all timings to a single number number, being the t_DEALY and set it equal to t_QUIET? In my understanding this should be sufficient for 99% of the devices?

To then specify this delay, I see again two possibilities: add it as parameter to acquire or make it configurable through the periph_conf.h.

[jnohlgard]

@haukepetersen for the rf2xx, the data sheet says "t_5: LSB last byte to MSB next byte, 250 ns" and "t_8: SPI idle time: SEL rising to falling edge, 250 ns"

There's even a 500 ns timing if you are using SRAM in a certain way. The 250 ns timing equates to the same timing as a 4 MHz bus clock, which is how we managed to get the device to ignore some bytes when running at default timings on a 7.5 MHz bus.

That t_5 timing is missing from the diagram above, and t_8 is equivalent to t_cs,dis. Here's the diagram from the RF233 data sheet:

[haukepetersen]

rebased for the 500th time

[kaspar030]

Does the new SPI increase RAM usage? Or why do you need to blacklist them now?

[haukepetersen]

I wouldn't think so, but might be. Some of the errors seem completely unrelated, though

[haukepetersen]

Finally -> green!

[PeterKietzmann]

@haukepetersen thanks for your all the work you put into this! I will hit the button in about 4 hours, if not already done by someone else then. We know that there are still some open topics with the SPI rework, which we collect in #6437 and handle in follow-up PRs.

[PeterKietzmann]

All right. Let's do this now. ACK and go :-)

[OlegHahm]

Hooray! And congrats!

[emmanuelsearch]

Yay! Congrats!

[kaspar030]

Yeah!

[jnohlgard]

Thank you for the huge effort put into getting this PR finished. It was finally merged! Congrats!

[haukepetersen]

Thanks to everyone for their help!

[miri64]

Congrats 🎉

[kYc0o]

Yeeeaaah!!! Cheers! 🍻 🎉