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

[Haixu Cui <quic_haixcui@quicinc.com>]

Hi Huck,

On 11/27/2023 10:26 PM, Cornelia Huck wrote:
> On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>
> > On 11/24/2023 11:46 PM, Cornelia Huck wrote:
> > > On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> > > > +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
> > >
> > > "chipselect" is probably a known term for people familiar with SPI -- is
> > > there any definition of those terms that the spec can point to?
> >
> > Just as Mark said, there is no formal spec for SPI, so no standard spec
> > for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
> > below.
>
> If we have nothing to point to, it is probably best to simply
> expand/explain the terms on their first usage.

OK, I will add description at where they are firstly used.
> > > Can we point to some documentation that explains CPHA and CPOL?
> >
> > Here. No standard SPI spec to point to. CPOL/CPHA have definitions in
> > wikipedia(Clock polarity and phase chapter):
> >
> > https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
> >
> > How about copying some concise information from wikipedia as Note? Or is
> > referring to such webpage acceptable in this spec.
>
> Not sure if we can do an outright copy (licence compatibility), but
> paraphrasing should be fine. (I'd rather not directly reference the
> site, because the content is not guaranteed to be stable, but we could
> maybe add it as "further reading".)
>
> > > > +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
> > > > +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
> > >
> > > s/filed/field/
> > > s/host/device/
> > >
> > > Also, isn't the rejecting supposed to be done by the device, as the
> > > driver is the party enqueueing the requests? Or do I have some kind of
> > > fundamental misunderstanding?
> >
> > It may be better to filter some invalid requests by driver, as in the
> > request header there are many parameters, and some of them are not
> > supported by device, so it's quite possible that many requests invalid
> > for the device. So if driver can do the first filter, such invalid
> > requests will not be sent at all, this will conserve virtqueue and
> > system overhead.
> >
> > And this is why exposing device supported features in the config space,
> > it ensures that almost all requests in virtqueue are nice to the backend.
> >
> > device also will verify the requests again, as the following requirement:
> > Virtio SPI device MUST verify the parameters in
> > \field{virtio_spi_transfer_head} after receiving the request,
> > and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR
> > if not all parameters are valid or some device unsupported features are set.
> >
> > Although checking the requests twice seems a little redundant, it is
> > more efficient comparing with sending some invalid requests to the device.
> >
> > What is your opinion? Do you think it is acceptable?
>
> Thanks for your explanation, I think we simply have some terminology
> issues. In the virtio spec, "driver" refers to one side of the
> driver/device pair, and is used to describe how to communicate with the
> device. In this case, "driver" would be the entity interacting with the
> device, regardless of how it is implemented, and would be responsible
> for sending the requests. Any filtering that would be done in a concrete
> implementation (i.e. if you have one component generating the requests,
> and then another component filtering them before actually putting them
> into the queue) is out of scope for this spec -- we can maybe specify
> that the driver should not send invalid requests, but I'm not sure that
> this is actually needed.

Got it. I agree with you, and I suggest that driver should do some 
filter jobs to reduce the system overhead, but this should not be 
compulsory for driver.

Thank you for your ideas.

Best Regards
Haixu Cui
> > Once again, thanks a lot for your support.
>
> You're welcome!