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

[Haixu Cui <quic_haixcui@quicinc.com>]

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
---

> > 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}

> > +\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 
transitions from active level to idle level. Data is sampled on the 
clock edges when clock transitions from idle level to active level.

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 
on the clock edges when clock transitions from active level to idle level.

> (...)
>
> > +VIRTIO_SPI_TRANS_ERR indicates a transfer error, which means that the parameters are all
> > +valid but the trasnfer process failed.
>
> s/trasnfer/transfer/

OK will update the spell mistake
> LGTM otherwise. Does anybody else have comments?

Best Regards
Haixu Cui