[RFC] SPI API update


Tomasz Bursztyka
 

Hello everyone,

I would like to introduce a major update on SPI API

Known problems
--------------

- Concurrency of multiple devices on the same SPI bus:

* User code has to (re)configure the driver before each spi API call,
to be sure its configuration is in use (frequency and so on).
* User code has to call slave select before each spi API call to be
sure the driver is using the right one.

This is a user experience improvement.

- Asymmetric buffers (aka: smart "dummy" bytes handling). This was
already handled partially in the current API. However, for the Quark
SE users, QMSI has brought a regression on that feature.

SPI is a symmetrical bus, for each written byte, one byte will be
received. (there is no way you will receive n bytes without
writing n bytes first, or being able to write 2+ bytes before
receiving 1 byte, controller's buffer and logic put aside)

SPI devices works (always I guess?) this way:
TX <n bytes of specified commands> + <0+ bytes of data>
RX <as many bytes as tx>

When you are interested only by reading bytes, you need to write a
0 per-each byte you want to read. This 0 is called a dummy byte.

For instance, let's take again cc2520 device. When you want to read
the received packet, you send one byte of command, and write as many
0s you need to be able to receive the requested content. For a
content of 100 bytes, we would then need to write 1 byte of command
+ 100 dummy bytes. Either we let the user handle the dummy bytes: up
to him to provide the buffers of dummy bytes (and thus ask him to
provide both TX and RX buffers of same length), or we have a driver
which is smart enough to guess when it's up to him to generate such
dummy bytes, and then in our example TX can be made of 1 byte, and
RX made of 1+100 bytes. (the 0-copy solution could solve that RX
would be made of only 100 bytes)

Note that such asymmetric buffer support can easily enable the use
of specific modes of the SPI controller itself (tx only, eeprom
read...)

This is a performance improvement and can be - if TX and RX have to
be of different origins - a memory improvement too.

- Enabling true zero-copy from user's buffers throughout the driver:
Unless the user has the possibility to think about the SPI command
bytes he will require, there is no way you can write n bytes without
copying these into a new buffer of a size such as: k bytes of SPI
commands + n bytes of user data. This problem occurs easily when an
SPI device is hidden under some API. Same example: cc2520 below the
network stack. Net stack wants to send a "packet", hold in a buffer,
and has no clue about how cc2520 works. And thus in cc2520 driver,
we copy the packet data into a buffer that holds the first SPI
command byte as well.

This is a memory and performance improvement.

- (Optional?) It's currently impossible to manage n transaction on the
same call without releasing the CS unless all is put in the same
buffer.

This is a user experience improvement.

- (Optional?) SPI daisy chaining:

Shall we do something about SPI daisy chaining on the API level?

Providing helpers, or even configuration bits that would let the
driver to be smart about such wiring setup?



Solutions
---------

1) The SPI configuration pointer should be provided to every API
calls. For instance:

int spi_write(struct device *dev, struct spi_config *conf,
const void *buf, uint32_t len)

From driver's implementation point of view, it will be then just a
matter of storing a struct spi_config pointer in its private data
holder, and then at every call:
if (conf != private->conf) {
... call configure ...

private->conf = conf;
}

Public function spi_configure() would thus disappear.

2) Instead of calling the slave every-time, we could give it as a
parameter to every call as well. The logic behind would be more or
less the same as for the configuration.

int spi_write(struct device *dev, struct spi_config *conf,
uint32_t slave, const void *buf, uint32_t len)

Public function spi_slave_select() would thus disappear as well.

3) Optional:

In order to reduce the number of parameters which has grown with
the addition of struct spi_config *conf parameter, we could put
struct device *dev into the struct spi_config. That's meant to
let the compiler to use as much as possible registers instead.

I am not proposing to include slave into the configuration because
the same device's configuration could be used against a different
slave.

4) About zero-copy, we would provide a "scatter-gather" type of buffer.

struct spi_buf {
void *buf;
uint32_t len;
};

And then spi_transceive would be:

int spi_transceive(struct spi_config *conf, uint32_t slave,
const struct spi_buf *tx, uint32_t tx_nb_buf,
struct spi_buf *rx, uint32_t rx_nb_buf);

tx and rx would be array of struct spi_buf, and the function would
get the number of items in each array.

Let's use again cc2520 as an example, on function
write_txfifo_content():

{
uint8_t ins = CC2520_INS_TXBUF;
struct spi_buf cmd[2] = {
{ .buf = &ins, .len = 1 },
{ .buf = net_nbuf_ll(buf),
.len = net_nbuf_ll_reserve(buf) +
net_buf_frags_len(buf)
},
};

return (spi_write(spi_conf, spi_slave, cmd, 2) == 0);
}

Now, when reading, on function cc2520's read_rxfifo_content():

{
uint8_t ins = CC2520_INS_RXBUF;
struct spi_buf cmd = { .buf = &ins, .len = 1 };
struct spi_buf data[2] = {
{ .buf = NULL, .len = 0 },
{ .buf = buf->data, len },
}

if (spi_transceive(spi_conf, spi_slave,
&cmd, 1, data, 2) != 0) {
return false;
}

( ... )
}

Which could be optimized for stack usage this way:

{
uint8_t ins = CC2520_INS_RXBUF;
struct spi_buf data[2] = {
{ .buf = &ins, .len = 1 },
{ .buf = buf->data, len },
}

if (spi_transceive(spi_conf, spi_slave,
data, 1, data, 2) != 0) {
return false;
}

( ... )
}

5) Multiple transaction without releasing the CS

I would like to get use cases on that one to define something
generic. Because, it could replace spi_transceive on driver's API
level, and public function spi_transceive could be an inline
function wrapping it then.

6) About daisy chaining, we could have a configuration bit telling the
wiring is following such setup. Then, according to the slave
parameter, the API implementation could be smart, and generate the
necessary train of dummy bytes to shift the user's buffer to
the right slave. If the user would like to handle it himself,
either he would not set such config bit.

That said, I am not sure it's relevant to chain different chips
this way. The common use case seems to be many identical chips,
like memory chips etc... thus all driven by a central code, so
buffers are directly managed, taking care of the actual chaining.


API transition
--------------

There would not be any proper API update without a transition period,
which would keep the legacy API around for some time, until the right
moment would come to remove it.

As stated above these 2 functions would be doomed to disappear:

- spi_slave_select
- spi_configure

In the mean time, it would however be necessary to support it. I don't
see another way but to keep the 2 related device api functions. The
implementation below would be slightly different, as both would just
set a data into driver's private structure. Which info, would be in
turn used the same way as the new API will.

spi_configure(spi_dev, spi_conf) in the driver would end up:

{
if (spi_conf != private->conf) {
... configure ...
}

private->conf = spi_conf;
private->conf->dev = spi_dev;
}

Legacy spi_transceive() for instance would be, inside the driver:

{
_select_slave(private->slave);

(...)
}

The only big issue is function naming. We obviously cannot have 2
different definitions for spi_read or spi_write, though their names
are quite self-documented. If anybody has an idea for new function
names, I will be glad to hear it. Here is what I have in mind:

- spi_transmit()
- spi_receive()

Unfortunately, both are longer names and spi_transceive() is already
in use... I am not super found about the idea of replacing current
names. Ok, maybe for write/read to transmit/receive, it's more
SPI-like. But I still fail to find a better name than spi_transceive.

The other solution, more like a bold move, would be to keep the
existing names, and make legacy and new API Kconfig based. Which means
only one API version would be available at built time. It's much
simpler from API modification point of view, less for all existing
devices/app/etc... using the current API.


Finally
-------

Once the transition period would be over, from legacy API to new one.


Buffer structure:

struct spi_buffer {
void *buf;
uint32_t len; /* Or should we use size_t ? */
};

Configuration structure:

struct spi_config {
struct device *dev;
uint32_t config; /* One of current reserved bits
could be used for daisy chaining */
uint32_t frequency; /* Previous name was not great at all */
};


Device's API structure:

typedef int (*spi_api_io)(struct spi_config *conf, uint32_t slave,
struct spi_buffer *tx, uint32_t nb_tx_buf,
struct spi_buffer *rx, uint32_t nb_rx_buf);
struct spi_driver_api {
spi_api_io transceive;
};


Function signatures:

Disappearance of spi_configure() and spi_slave_select()

inline int spi_read(struct spi_config *conf, uint32_t slave,
struct spi_buffer *rx, uint32_t nb_rx_buf);

inline int spi_write(struct spi_config *conf, uint32_t slave,
struct spi_buffer *tx, uint32_t nb_tx_buf);

(I did not change the name, but they could become respectively
spi_receive and spi_transmit)

inline int spi_transceive(struct spi_config *conf, uint32_t slave,
struct spi_buffer *tx, uint32_t nb_tx_buf,
struct spi_buffer *rx, uint32_t nb_rx_buf);



Please comment. If you have any use case that would not fit, or any
other issue about current API you know of and which is not addressed by
this RFC: please tell.


Br,

Tomasz

Join devel@lists.zephyrproject.org to automatically receive all group messages.