lkml.org 
[lkml]   [2023]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).
From
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.

\
 
 \ /
  Last update: 2023-12-06 18:35    [W:0.142 / U:1.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site