Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification

[Haixu Cui <quic_haixcui@quicinc.com>]

Hi Mark,
     Thank you for your comments, my response is below each of your 
comments.

On 11/6/2023 9:33 PM, Mark Brown wrote:
> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
> 
>> virtio-spi is a virtual SPI master and it allows a guest to operate and
>> use the physical SPI master controlled by the host.
> 
> It would be good to avoid introducing new uses of this outdated
> terminology, terms like controller are better.  This is especially true
> for specifications.

Yes! Will use "controller" instead of "master" in next patch.
> 
> It would be good to be copied on future versions of this.
> 
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +	le16 bus_num;
>> +	le16 chip_select_max_number;
> 
> That's a *lot* of potential buses and chip selects...

Here you mean the types of bus_num and chip_select_max_number are "le16"?

Here I use 16 bits because bus_num and num_chipselect in spi_controller 
structure are both 16 bits.

> 
>> +The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
>> +	0: physical SPI master doesn't support cs timing setting;
>> +	1: physical SPI master supports cs timing setting.
> 
> This is very vauge - what exactly is meant by "cs timing setting"?  If
> we're specifying anything about timing it should probably be specific
> about which paramters might be controlled and the constraints on how
> they can be set.  It's also a bit surprising that the only parameter
> here is around chip select, there's generally a lot more sensitivity
> around the main clock.

Here I would like to say the support of 3 types of cs delay, that is, 
delay after cs asserted, delay before cs deasserted, delay after cs 
deasserted.

I will update as follows:

The \field{cs_timing_setting_enable} indicates if the host SPI 
controller supports cs delay setting when performing transfer:
         bit 0: delay after cs asserted, 0 means doesn't support and 1
                means support;
         bit 1: delay before cs deasserted, 0 means doesn't support and 1
                means support;
         bit 2: delay after cs deasserted, 0 means doesn't support and 1
                means support;
         other bits are reserved and always set as 0.


> 
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 slave_id;
> 
> More outdated terminology.
> 
>> +        u8 bits_per_word;
>> +        u8 cs_change;
>> +        u8 tx_nbits;
>> +        u8 rx_nbits;
>> +	 u8 reserved[3];
> 
> Indentation issue?

Yes, will fix in next patch.
> 
>> +        le32 mode;
>> +        le32 freq;
>> +        le32 word_delay_ns;
>> +        le32 cs_setup_ns;
>> +        le32 cs_delay_hold_ns;
>> +        le32 cs_change_delay_inactive_ns;
> 
> All these parameters depend on controller support but weren't included
> in the feature enumeration above and nothing in the spec seems to cover
> what happens when they can't be supported exactly.

Add another return value:
     #define VIRTIO_SPI_PARAM_ERR    1

If the parameters in \field{virtio_spi_transfer_head} are not all valid, 
or some fields are set as non-zero values but the corresponding features 
are not supported by host, then the backend return VIRTIO_SPI_PARAM_ERR 
to the frontend.

> 
>> +The \field{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.
> 
> Note that this is *not* how Linux implements things, and it doesn't
> include any specification of timing constraints.
> 
>> +The \field{tx_nbits} indicates bus width for write transfer:
>> +        0,1: bus width is 1, also known as SINGLE;
>> +	2  : bus width is 2, also known as DUAL;
>> +	4  : bus width is 4, also known as QUAD;
>> +	8  : bus width is 8, also known as OCTAL;
>> +	other values are invalid.
> 
> Width here being in terms of bits.
> 
Updated as:
0,1: 1-bit transfer, also known as SINGLE;
...

>> +The \field{tx_buf} is the buffer for data sent to the device.
>> +
>> +The \field{rx_buf} is the buffer for data received to the device.
> 
> In what format are these buffers presented?  I note that the spec
> supports more than one byte per word (and non-integer number of bytes).

In spi_transfer struct:
     @len: size of rx and tx buffers (in bytes)

Although the bits_per_word can be any value in 0..32, in-memory 
wordsizes are powers of two bytes (e.g. 20 bit samples use 32 bits).
So:
     0 < bits_per_word <=8: one word occupies 1 byte in memory;
     8 < bits_per_word <=16: one word occupies 2 bytes in memory;
     16 < bits_per_word <=32: one word occupies 4 bytes in memory.

In the spec, the format of buffers are u8 array,
     u8 tx_buf[];
     u8 rx_buf[];


Thanks again for your comments and ideas.

Best Regards
Haixu Cui

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org