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

[Cornelia Huck <cohuck@redhat.com>]

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.

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

> Once again, thanks a lot for your support.

You're welcome!