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