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

[Haixu Cui <quic_haixcui@quicinc.com>]

Hi Mark,

     For the "le16" type and byte ordering issue, Harald replies in 
another email, let's mainly discuss about checking parameters and 
exposing the backend supported features.


     struct virtio_spi_transfer_head {
         u8 chip_select;
         u8 bits_per_word;
         u8 cs_change;
         u8 tx_nbits;
         u8 rx_nbits;
         u8 reserved[3];
         le32 mode;
         le32 freq;
         le32 word_delay_ns;
         le32 cs_setup_ns;
         le32 cs_delay_hold_ns;
         le32 cs_change_delay_inactive_ns;
    };

    For checking the parameters above in virtio_spi_transfer_head, I'd 
like to add these field in config space:

     1. bits_per_word_mask, in Linux spi driver, 
__spi_validate_bits_per_word will use this field to validate 
bits_per_word parameters for each transfer

     2. cs_change_supported, 0 for unsupported and 1 for supported

     3. tx_nbits and rx_nbits:
        bit 0: 2-bit transfer supported;
        bit 1: 4-bit transfer supported;
        bit 2: 8-bit transfer supported;
        other bits reserved as 0, and 1-bit transfer should always 
supported.

     4. mode_feature_supported:
        bit 0-1: CPHA supported,
                 0b00: supports CPHA=0 and CPHA=1
                 0b01: only supports CPHA=0
                 0b10: only supports CPHA=1
                 0b11: invalid, should support as least one CPHA setting
        bit 2-3: CPOL supported
                 0b00: supports CPOL=0 and CPOL=1
                 0b01: only supports CPOL=0
                 0b10: only supports CPOL=1
                 0b11: invalid, should support as least one CPOL setting
        bit 4: CS_HIGH, if 1, supports chipselect active high, else
                 only supports chepselect active low.
        bit 3: LSB_FIRST, if 0, only supports MSB first transfer, if 1,
                 supports LSB first transfer
        bit 4: LOOP, if 1, support loopback mode, if 0, only supports
                 normal mode.

     5. freq_max_speed: if non-zero, maximum clock rate support, in Hz
                 unit. if 0, no limitation.

     6. max_word_delay_ns: if non-zero, the maximum word delay supported.
                 if 0, word delay is not supported to introduce.

     7. max_delay_after_cs_asserted: if non-zero, the maximum cs setup
                 delay supported.if 0, cs setup delay is not supported to
                 introduce

     8. max_delay_before_cs_deassert: if non-zero, the maximum
                 cs_delay_hold_ns delay supported.if 0, cs setup delay is
                 not supported to introduce

     9. max_delay_after_cs_deassert: if non-zero, the maximum
                 cs_change_delay_inactive_ns delay supported.if 0, cs
                 setup delay is not supported to introduce


Driver Requirement：

If Virtio SPI device announce it doesn't support any feature, Virtio SPI 
driver MUST reject the request if the corresponding feature field in 
request header is not zero.

Virtio SPI driver MUST reject the request if the freq exceeds the 
non-zero value of freq_max_speed filed in config space.

If any delay field in config space is set as non-zero, Virtio SPI driver 
MUST reject the request if the corresponding delay in request header 
exceeds the value in config space.


Do you think this mechanism, backend exposing its supported features and 
abilities through config space so Virtio SPI driver can filter and check 
before sending requests, is reasonable? And seems much stronger. A rule 
to validate seems necessary because the parameters in request header may 
differ in each transfer.

Looking forward to your advice and suggestion, if you agree that this 
mechanism is better, I will add it in next patch. Thank you so much.

Best Regard
Haixu Cui


On 11/9/2023 12:04 AM, Mark Brown wrote:
> On Wed, Nov 08, 2023 at 11:42:40PM +0800, Haixu Cui wrote:
>> On 11/6/2023 9:33 PM, Mark Brown wrote:
>>> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
> 
>>>> +	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.
> 
> Yeah, I think that's more for padding/alignment reasons than anything
> else.  TBH I wondered if just going for a standard int might be better,
> it's the combination of picking a smaller type but one that's relatively
> large for the domain.
> 
>> 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.
> 
> That looks a lot clearer.  There's still a potential issue with the fact
> that there might be limited configurability (eg, only a few values
> supported).  Perhaps we need a discovery mechanism, or perhaps
> specifying handling of inexact support might be good enough (see below)?
> 
>>>> +        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.
> 
> That looks good for things like the mode, for the frequencies, delays
> and so on it seems like we need either some specified handling of values
> that can't be supported exactly (eg, freq is a maximum, delays are a
> minimum) or an enumeration mechanism so the guest knows what values will
> be accepted.  The enumeration might be a bit unweildy since you can get
> huge numbers of values - Linux currently uses the latter approach.
> 
>>>> +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[];
> 
> What byte ordering are the multi-byte words in - CPU native or a
> particular endianness (eg, wire native)?

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