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.
