On Wed, Dec 16, 2020 at 06:38:59PM +0100, Cornelia Huck wrote:
> On Wed, 25 Nov 2020 13:55:18 +0800
> Jie Deng <[email protected]> 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 <[email protected]>
Given the comments do we want to
> > ---
> > 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?
> 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
> > +
> > +\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 ...
> > +
> > +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.
> > +
> > +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
> > +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"?
--
MST
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
