Re: [virtio-dev][PATCH V9 2/2] virtio-spi: add the device specification

[Cornelia Huck <cohuck@redhat.com>]

On Thu, Dec 21 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Cornelia,
>      Much appreciated for your comments and please refer to my response.
>
> On 12/18/2023 7:04 PM, Cornelia Huck wrote:
>> On Tue, Dec 12 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>> 
>> [BTW: A changelog would be useful, either in the patch or in the cover
>> letter.]
>
> Sure, I summarize the major changes between these versions and will add 
> in next patch:
>
> changelog:
> v8->v9:
> - add explanation of bits_per_word_mask in config space
> v7->v8:
> - change device to host
> v6->v7:
> - fix the format problem, syntax problem
> v5->v6:
> - use driver/device instead guest/host
> - add the definition of some terminologies
> - use controller instead of master throughout the spec
> - add buffer length validation for full-duplex transfer
> v4->v5:
> - use controller instead of master
> - fix indentation issue
> - extend the config space to expose the backend supported features
> - add another result value to indicate parameter error
> - add device and driver requirement about parameter checkig
> v3->v4:
> - fix the spell error
> - bus_num is not SOC-specific, remove it
> - add driver requirement to deal with the situation that the cs delay
> parameters are not 0 but the backend doesn't support cs timing setting
> v2->v3
> - remove unnecessary statements and driver implimentation details
> - add the parameters about cs timing delay and transfer delay
> - use "le32" instead of "u32"
> - swap the rx_buf and tx_buf in the request format
> - add the parameters about transfer bit width
> v1->v2:
> - explain SPI when it is firstly used
> - update the ambiguous expression of virtqueue
> v0->v1:
> - add definition of abbreviation SPI
> - remove the ID

Thanks. Might be best to add this in the cover letter.

> ---
>
>> 
>>> The Virtio SPI (Serial Peripheral Interface) device is a virtual
>>> SPI controller that allows the driver to operate and use the SPI
>>> controller under the control of the host.
>>>
>>> This patch adds the specification for virtio-spi.
>>>
>>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>   device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>>>   device-types/spi/device-conformance.tex |   7 +
>>>   device-types/spi/driver-conformance.tex |   7 +
>>>   3 files changed, 295 insertions(+)
>>>   create mode 100644 device-types/spi/description.tex
>>>   create mode 100644 device-types/spi/device-conformance.tex
>>>   create mode 100644 device-types/spi/driver-conformance.tex
>>>
>>> diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
>> 
>> Please move inclusion of this new file into content.tex here, instead of
>> including a not-yet-existing file in the previous patch.
>> 
>> (...)
>
>
> Sorry, I don't quite understand here. Maybe I should not add the 
> following line in content.tex, is that so?
>
> \input{device-types/spi/description.tex}

No, please add this line; but do so in this patch instead of the
previous one that only changes the wording.

>
>> 
>>> +\field{mode_func_supported} indicates whether the following features are supported or not:
>>> +        bit 0-1: CPHA feature,
>>> +            0b00: invalid, must support as least one CPHA setting.
>>> +            0b01: supports CPHA=0 only;
>>> +            0b10: supports CPHA=1 only;
>>> +            0b11: supports CPHA=0 and CPHA=1;
>>> +
>>> +        bit 2-3: CPOL feature,
>>> +            0b00: invalid, must support as least one CPOL setting.
>>> +            0b01: supports CPOL=0 only;
>>> +            0b10: supports CPOL=1 only;
>>> +            0b11: supports CPOL=0 and CPOL=1;
>>> +
>>> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
>>> +            chipselect active low must always be supported.
>>> +
>>> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first must always be
>>> +            supported.
>>> +
>>> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
>>> +            must always be supported.
>>> +
>>> +Note: CPOL is clock polarity and CPHA is clock phase. If CPOL is 0, the clock idles at the logical
>>> +low voltage, otherwise it idles at the logical high voltage. CPHA determines how data is outputted
>>> +and sampled.
>> 
>> Shouldn't you also specify what CPHA==0 and CPHA==1 mean?
>> 
>
> Yes, I will add the following explanation:
>
> If CPHA is 0, the first bit is outputted immediately when chipselect is 
> active and subsequent bits are outputted on the clock edges when clock 

s/when clock/when the clock/

> transitions from active level to idle level. Data is sampled on the 
> clock edges when clock transitions from idle level to active level.

same

>
> If CPHA is 1, the first bit is outputted on the fist clock edge after 
> chipselect is active, subsequent bits are outputted on the clock edges 
> when clock transitions from idle level to active level. Data is sampled 

same

> on the clock edges when clock transitions from active level to idle level.

same