[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v5] virtio-i2c: add the device specification
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 ïexibly communicate with the host I2C slave devices froms/ïexibly/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?
The virtio-i2c is a virtual I2C master device. One can attach a host physical--- 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?
Will fix it.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
Got it. I'm OK to remove read and written and use the size in the descriptor.+ +\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 ...
OK. Will fix it.+ +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.
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 thes/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 thes/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"?
It means if for example 5 requests are sent and received with
req3 failed.Since the driver must treat 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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]