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

[Cornelia Huck <cohuck@redhat.com>]

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>
> ---
>  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. 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:
> +
> +\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.
> +
> +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.

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


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/