Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 6 Dec 2023 18:23:08 +0100 | Subject | Re: [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification). | From | Harald Mommer <> |
| |
On 06.11.23 14:48, Mark Brown wrote: > On Fri, Oct 27, 2023 at 06:10:16PM +0200, Harald Mommer wrote: > >> +config SPI_VIRTIO >> + tristate "Virtio SPI SPI Controller" >> + depends on VIRTIO >> + help >> + This enables the Virtio SPI driver. >> + >> + Virtio SPI is an SPI driver for virtual machines using Virtio. >> + >> + If your Linux is a virtual machine using Virtio, say Y here. >> + > This advice is going to be inappropriate for the majortiy of guests. Reminder for me: Need still to address this, but this is not code I'm currently working on, so comes later. >> + // clang-format off >> + struct spi_transfer_head transfer_head ____cacheline_aligned; >> + const uint8_t *tx_buf ____cacheline_aligned; >> + uint8_t *rx_buf ____cacheline_aligned; >> + struct spi_transfer_result result ____cacheline_aligned; >> + // clang-format on > Remove this clang-format stuff. Not needed any more, will be removed. Maybe I should remove this ____cacheline_aligned. It's only there because I looked too deeply into struct virtio_i2c_req, not because I think this ____cacheline_aligned is decisive here. >> +static struct spi_board_info board_info = { >> + .modalias = "spi-virtio", >> + .max_speed_hz = 125000000, /* Arbitrary very high limit */ >> + .bus_num = 0, /* Patched later during initialization */ >> + .chip_select = 0, /* Patched later during initialization */ >> + .mode = SPI_MODE_0, >> +}; >> +/* Compare with file i2c_virtio.c structure virtio_i2c_msg_done */ > In what way is one supposed to compare with the i2c driver? What > happens if the I2C driver changes? It would be better to ensure that > the code can be read and understood as a standalone thing. It was a reminder for me from where I got some inspiration and to reveal that not everything was invented by me. Served it's purpose, all such comment references to foreign code is being removed. >> + /* Fill struct spi_transfer_head */ >> + th->slave_id = spi->chip_select; > If the spec just copied the Linux terminology it'd have few issues :( Spec changed this in the meantime so I can do also. >> + th->bits_per_word = spi->bits_per_word; >> + th->cs_change = xfer->cs_change; > The virtio spec for cs_change is *not* what the Linux cs_change field > does, this will not do the right thing.
This is a hard one. Linux says (spi.h):
"@cs_change: affects chipselect after this transfer completes
...
All SPI transfers start with the relevant chipselect active. Normally it stays selected until after the last transfer in a message. Drivers can affect the chipselect signal using cs_change."
V8 of the draft spec says:
"cs_change indicates whether to deselect device before starting the next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted then asserted again."
What I understand here (unfortunately in both texts) is
- at the start of a transfer CS is made active
- it is kept active during the transfer
- when cs_change is 0 after the transfer CS is kept active (not changed)
- when cs_change is 1 after the transfer CS is de-asserted
So if cs_change is 0 keep CS asserted after transfer, otherwise de-assert after transfer.
I fear there is some subtle thing I haven't gotten yet but I don't understand / see it.What is it?
>> + th->tx_nbits = xfer->tx_nbits; >> + th->rx_nbits = xfer->rx_nbits; >> + th->reserved[0] = 0; >> + th->reserved[1] = 0; >> + th->reserved[2] = 0; >> + >> +#if (VIRTIO_SPI_CPHA != SPI_CPHA) >> +#error VIRTIO_SPI_CPHA != SPI_CPHA >> +#endif > BUILD_BUG_ON()
Thanks for this one, didn't know yet.
| ![\](/images/icornerr.gif) |