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

Hi Huck,

On 11/27/2023 10:26 PM, Cornelia Huck wrote:
> On Mon, Nov 27 2023, Haixu Cui <[email protected]> wrote:
> 
>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>> On Fri, Nov 24 2023, Haixu Cui <[email protected]> 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!
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]