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

[Jie Deng <jie.deng@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 10597 bytes --]


On 2020/12/17 3:55, Michael S. Tsirkin wrote:
> On Wed, Dec 16, 2020 at 06:38:59PM +0100, Cornelia Huck wrote:
>> On Wed, 25 Nov 2020 13:55:18 +0800
>> Jie Deng <jie.deng@intel.com> wrote:
>>
>> <some mostly editorial comments; sorry about bringing this up now>
>>
>>> virtio-i2c is a virtual I2C adapter device. It provides a way
>>> to flexibly communicate with the host I2C slave devices from
>> s/flexibly/flexibly/
>>
>>> the guest.
>>>
>>> This patch adds the specification for this device.
>>>
>>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/88
>>> Signed-off-by: Jie Deng <jie.deng@intel.com>
>
> Given the comments do we want to

I didn't get it. Can you explain more?

>
>>> ---
>>>   conformance.tex  |  28 +++++++++--
>>>   content.tex      |   1 +
>>>   introduction.tex |   3 ++
>>>   virtio-i2c.tex   | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 167 insertions(+), 4 deletions(-)
>>>   create mode 100644 virtio-i2c.tex
>>>
>> (...)
>>
>>
>>> diff --git a/introduction.tex b/introduction.tex
>>> index cc38e29..9f016d5 100644
>>> --- a/introduction.tex
>>> +++ b/introduction.tex
>>> @@ -73,6 +73,9 @@ \section{Normative References}\label{sec:Normative References}
>>>   	\phantomsection\label{intro:HDA}\textbf{[HDA]} &
>>>   	High Definition Audio Specification,
>>>   	\newline\url{https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/high-definition-audio-specification.pdf}\\
>>> +	\phantomsection\label{intro:I2C}\textbf{[I2C]} &
>>> +	I2C-bus specification and user manual,
>>> +	\newline\url{https://www.nxp.com.cn/docs/en/user-guide/UM10204.pdf}\\
>> I think this url should use www.nxp.com instead of www.nxp.com.cn (even
>> though both seem to lead to the same document).
>>
>>>   
>>>   \end{longtable}
>>>   
>>> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
>>> new file mode 100644
>>> index 0000000..fdb0050
>>> --- /dev/null
>>> +++ b/virtio-i2c.tex
>>> @@ -0,0 +1,139 @@
>>> +\section{I2C Adapter Device}\label{sec:Device Types / I2C Adapter Device}
>>> +
>>> +virtio-i2c is a virtual I2C adapter device.
> BTW is this neessarily a virtual device? One can build a physical
> one to this spec, no?
The virtio-i2c is a virtual I2C master device. One can attach a host 
physical
slave I2C device to this virtual device.
>
>> It provides a way to flexibly
>>> +organize and use the host I2C slave devices from the guest. By attaching
>>> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
>>> +communicate with them without changing or adding extra drivers for these
>>> +slave I2C devices.
>> I know that the i2c spec talks about 'slave' devices, but can we find a
>> better terminology for the virtio standard? E.g. prefix this with
>>
>> "Note: This standard uses the term 'controlled I2C device' to refer to
>> what the I2C standard calls 'slave I2C device'."
>>
>> (Or whatever better term might exist.)
>>
>>> +
>>> +\subsection{Device ID}\label{sec:Device Types / I2C Adapter Device / Device ID}
>>> +34
>>> +
>>> +\subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues}
>>> +
>>> +\begin{description}
>>> +\item[0] requestq
>>> +\end{description}
>>> +
>>> +\subsection{Feature bits}\label{sec:Device Types / I2C Adapter Device / Feature bits}
>>> +
>>> +None currently defined.
>>> +
>>> +\subsection{Device configuration layout}\label{sec:Device Types / I2C Adapter Device / Device configuration layout}
>>> +
>>> +None currently defined.
>>> +
>>> +\subsection{Device Initialization}\label{sec:Device Types / I2C Adapter Device / Device Initialization}
>>> +
>>> +\begin{enumerate}
>>> +\item The virtqueue is initialized.
>>> +\end{enumerate}
>>> +
>>> +\subsection{Device Operation}\label{sec:Device Types / I2C Adapter Device / Device Operation}
>>> +
>>> +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Adapter Device / Device Operation: Request Queue}
>>> +
>>> +The driver queues requests to the virtqueue, and they are used by the
>>> +device. The request is the representation of segments of an I2C
>>> +transaction. Each request is of form:
> of the form
Will fix it.
>>> +
>>> +\begin{lstlisting}
>>> +struct virtio_i2c_req {
>>> +        le16 addr;
>>> +        le32 flags;
>>> +        le16 written;
>>> +        le16 read;
>>> +        u8 write_buf[];
>>> +        u8 read_buf[];
>>> +        u8 status;
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +The \field{addr} of the request is the address of the I2C slave device.
>>> +
>>> +The \field{flags} of the request is currently reserved as zero for future
>>> +feature extensibility.
>>> +
>>> +The \field{written} of the request is the number of data bytes in the \field{write_buf}
>>> +being written to the I2C slave address.
>>> +
>>> +The \field{read} of the request is the number of data bytes in the \field{read_buf}
>>> +being read from the I2C slave address.
> I note that read and written actually duplicate buf size
> available in the descriptor.
> Given we no longer mirror i2c_msg 1:1 do we still want to do this?
> It will be trivial for the host device to populate these fields
> correctly for linux.
> Duplication of information iten leads to errors ...
Got it. I'm OK to remove read and written and use the size in the 
descriptor.
>
>>> +
>>> +The \field{write_buf} of the request contains one segment of an I2C transaction
>>> +being written to the device.
>>> +
>>> +The \field{read_buf} of the request contains one segment of an I2C transaction
>>> +being read from the device.
>>> +
>>> +The final \field{status} byte of the request is written by the device: either
>>> +VIRTIO_I2C_MSG_OK for success or VIRTIO_I2C_MSG_ERR for error.
>>> +
>>> +\begin{lstlisting}
>>> +#define VIRTIO_I2C_MSG_OK     0
>>> +#define VIRTIO_I2C_MSG_ERR    1
>>> +\end{lstlisting}
>>> +
>>> +If the \field{read}=0 and the \field{written}>0, the request is called write request.
>>> +
>>> +If the \field{read}>0 and the \field{written}=0, the request is called read request.
>>> +
>>> +If the \field{read}>0 and the \field{written}>0, the request is called write-read request.
>>> +It means an I2C write segment followed by a read segment. Usually, the write segment
>>> +provides the number of an I2C slave device register to be read.
>>> +
>>> +The \field{read}=0 and at the same time the \field{written}=0 doesn't make any sense.
>>> +The driver SHOULD never send such request.
>> 'SHOULD' makes this a normative statement.
>>
>>> +
>>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
>>> +
>>> +A driver may send one request or multiple requests to the device at a time.
>>> +The requests in the virtqueue MUST be queued and processed in order.
>> Same here, 'MUST' makes this a normative statement. I think lowercasing
>> these terms would make it correct.
>>
>
> It won't, the relevant rfc includes MUST and must etc.
>
> If this is normative move it to a normative statement.
> Or preferably, both change this to e.g.
>
> The requests in the virtqueue are both queued and processed in order.
>
> and add a normative statement in the normative section.
>
OK. Will fix it.
>>> +
>>> +If a driver sends multiple requests at a time and a device fails to process
>>> +some of them, then the first failed request MUST have its \field{status}
>>> +being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
>>> +failed one MUST also be treated as VIRTIO_I2C_MSG_ERR by the driver,
>>> +no matter what \field{status} of them. In this case, the number of successfully
>>> +sent requests this time is the number of the last request being successfully
>>> +processed.
>>> +
>>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>>> +
>>> +A driver MUST set \field{addr}, \field{flags}(currently reserved as zero),
>> I'd drop "(currently reserved as zero)" here and add
>>
>> "A driver MUST set \field{flags} to zero."
>>
>>
>>> +\field{written} and \field{read} before sending the request.
>>> +
>>> +A driver MUST place one segment of an I2C transaction into \field{write_buf} if the
>> s/if the/if/
>>
>>> +\field{written}>0.
>>> +
>>> +A driver MUST NOT use \field{read_buf} if the final \field{status} returned
>>> +from the device is VIRTIO_I2C_MSG_ERR.
>>> +
>>> +A driver MAY queue one request or multiple requests at a time.
>>> +
>>> +A driver MUST queue the requests in order if multiple requests are going to
>>> +be sent at a time.
>>> +
>>> +If multiple requests are sent at a time and some of them returned from the
>>> +device have the \field{status} being VIRTIO_I2C_MSG_ERR, a driver MUST treat
>>> +the first failed request and the requests after the first failed one as
>>> +VIRTIO_I2C_MSG_ERR.
>>> +
>>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>>> +
>>> +A device SHOULD keep consistent behaviors with the hardware as described in
>>> +\hyperref[intro:I2C]{I2C}.
>>> +
>>> +A device MUST NOT change the value of \field{addr}, \field{flags}, \field{written},
>>> +\field{read} and \field{write_buf}.
>>> +
>>> +A device MUST place one segment of an I2C transaction into \field{read_buf} if the
>> s/if/if the/
>>
>>> +\field{read}>0.
>>> +
>>> +A device MUST guarantee the requests in the virtqueue being processed in order
>>> +if multiple requests are received at a time.
>>> +
>>> +If multiple requests are received at a time and some of them being processed failed,
> and processing of some of the requests fails
Will fix it.
>>> +a device MUST set the \field{status} of the first failed request to be
>>> +VIRTIO_I2C_MSG_ERR and MAY set the \field{status} of the requests after
>>> +the first failed one to be VIRTIO_I2C_MSG_ERR.
> So what is it saying? That VIRTIO_I2C_MSG_ERR can also mean
> "previous request failed"?

It means if for example 5 requests are sent and received with
req3 failed.Since the driver musttreat the first failed request (req3) and the requests after the first 
failed one (req4 and req5) as VIRTIO_I2C_MSG_ERR.

So we don't need to care about the status of the req4 and req5 if req3 failed.They will
be treated as failed no matter what status.
  
req1(OK)  req2(OK)  req3(FAIL)  req4  req5


[-- Attachment #2: Type: text/html, Size: 15056 bytes --]