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

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

[Jie Deng]

virtio-i2c is a virtual I2C adapter device. It provides a way
to flexibly communicate with the I2C slave devices from the guest.

This patch adds the specification for this device.

Signed-off-by: Jie Deng <jie.deng@intel.com>
---
 conformance.tex | 28 ++++++++++++++---
 content.tex     |  1 +
 virtio-i2c.tex  | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+), 4 deletions(-)
 create mode 100644 virtio-i2c.tex

diff --git a/conformance.tex b/conformance.tex
index f1f23a8..12bce3a 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -27,8 +27,10 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Driver Conformance / Socket Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / RPMB Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / IOMMU Driver Conformance},
-\ref{sec:Conformance / Driver Conformance / Sound Driver Conformance} or
-\ref{sec:Conformance / Driver Conformance / Memory Driver Conformance}.
+\ref{sec:Conformance / Driver Conformance / Sound Driver Conformance}
+\ref{sec:Conformance / Driver Conformance / Memory Driver Conformance} or
+\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}.
+
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
 \item[Device] A device MUST conform to four conformance clauses:
@@ -47,8 +49,10 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Device Conformance / Socket Device Conformance}, 
 \ref{sec:Conformance / Device Conformance / RPMB Device Conformance},
 \ref{sec:Conformance / Device Conformance / IOMMU Device Conformance},
-\ref{sec:Conformance / Device Conformance / Sound Device Conformance} or
-\ref{sec:Conformance / Device Conformance / Memory Device Conformance}.
+\ref{sec:Conformance / Device Conformance / Sound Device Conformance}
+\ref{sec:Conformance / Device Conformance / Memory Device Conformance} or
+\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance}.
+
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
 \end{description}
@@ -261,6 +265,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Device Types / Memory Device / Device Operation / STATE request}
 \end{itemize}
 
+\conformance{\subsection}{I2C Adapter Driver Conformance}\label{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}
+
+An I2C Adapter driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / I2C Adapter Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -477,6 +489,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / Memory Device / Device Operation / STATE request}
 \end{itemize}
 
+\conformance{\subsection}{I2C Adapter Device Conformance}\label{sec:Conformance / Device Conformance / I2C Adapter Device Conformance}
+
+An I2C Adapter device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / I2C Adapter Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
 A conformant implementation MUST be either transitional or
 non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index 91855b4..95e2ed8 100644
--- a/content.tex
+++ b/content.tex
@@ -6200,6 +6200,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-iommu.tex}
 \input{virtio-sound.tex}
 \input{virtio-mem.tex}
+\input{virtio-i2c.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/virtio-i2c.tex b/virtio-i2c.tex
new file mode 100644
index 0000000..96c96bf
--- /dev/null
+++ b/virtio-i2c.tex
@@ -0,0 +1,94 @@
+\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 manage I2C slave devices from the guest. By attaching 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. 
+
+\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}
+
+The driver queues requests to the virtqueue, and they are used by the
+device. The request is the representation of one segment of an I2C
+transaction. Each request is of form:
+
+\begin{lstlisting}
+struct virtio_i2c_req {
+        le16 addr;
+        le16 flags;
+        le16 len;
+        u8 buf[];
+        u8 status;
+};
+\end{lstlisting}
+
+The \field{addr} is the address of the I2C slave device.
+
+The first bit of \field{flags} indicates whether it is a read or write request.
+It means a read request if the first bit of \field{flags} is set, otherwise
+it is a write request. The rest bits of \field{flags} are reserved.
+
+The \field{len} is the number of data bytes in the \field{buf} being read from or
+written to the I2C slave address.
+
+The \field{buf} of the request contains one segment of an I2C transaction.
+If the first bit of \field{flags} is '1', the \field{buf} is written by the
+device and it contains one segment of an I2C transaction being read from the
+device. If the first bit of \field{flags} is '0', the \field{buf} is written
+by the driver and it contains one segment of an I2C transaction being written
+to the the device.
+
+The final \field{status} byte 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}
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
+
+A driver MUST set \field{addr}, \field{flags} and \field{len} before sending
+the request.
+
+A driver MUST place one segment of an I2C transaction into \field{buf} if the
+first bit of \field{flags} is '0'.
+
+A driver MUST NOT use \field{addr}, \field{flags}, \field{len} and \field{buf}
+if the final \field{status} returned from the device is VIRTIO_I2C_MSG_ERR.
+
+A driver MUST queue the requests in order if multiple requests are going to
+be sent at a time.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
+
+A device MUST NOT change the value of \field{addr}, \field{flags} and \field{len}.
+
+A device MUST place one segment of an I2C transaction into \field{buf} if the first
+bit of \field{flags} is '1'.
+
+A device MUST guarantee the requests being handled in order if multiple requests
+are received.
-- 
2.7.4


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/

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

[Michael S. Tsirkin]

On Tue, Oct 27, 2020 at 02:00:24PM +0800, Jie Deng wrote:
> virtio-i2c is a virtual I2C adapter device. It provides a way
> to flexibly communicate with the I2C slave devices from the guest.
> 
> This patch adds the specification for this device.
> 
> Signed-off-by: Jie Deng <jie.deng@intel.com>
> ---
>  conformance.tex | 28 ++++++++++++++---
>  content.tex     |  1 +
>  virtio-i2c.tex  | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 119 insertions(+), 4 deletions(-)
>  create mode 100644 virtio-i2c.tex
> 
> diff --git a/conformance.tex b/conformance.tex
> index f1f23a8..12bce3a 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -27,8 +27,10 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \ref{sec:Conformance / Driver Conformance / Socket Driver Conformance},
>  \ref{sec:Conformance / Driver Conformance / RPMB Driver Conformance},
>  \ref{sec:Conformance / Driver Conformance / IOMMU Driver Conformance},
> -\ref{sec:Conformance / Driver Conformance / Sound Driver Conformance} or
> -\ref{sec:Conformance / Driver Conformance / Memory Driver Conformance}.
> +\ref{sec:Conformance / Driver Conformance / Sound Driver Conformance}
> +\ref{sec:Conformance / Driver Conformance / Memory Driver Conformance} or
> +\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}.
> +
>      \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
>    \end{itemize}
>  \item[Device] A device MUST conform to four conformance clauses:
> @@ -47,8 +49,10 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \ref{sec:Conformance / Device Conformance / Socket Device Conformance}, 
>  \ref{sec:Conformance / Device Conformance / RPMB Device Conformance},
>  \ref{sec:Conformance / Device Conformance / IOMMU Device Conformance},
> -\ref{sec:Conformance / Device Conformance / Sound Device Conformance} or
> -\ref{sec:Conformance / Device Conformance / Memory Device Conformance}.
> +\ref{sec:Conformance / Device Conformance / Sound Device Conformance}
> +\ref{sec:Conformance / Device Conformance / Memory Device Conformance} or
> +\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance}.
> +
>      \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
>    \end{itemize}
>  \end{description}
> @@ -261,6 +265,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{drivernormative:Device Types / Memory Device / Device Operation / STATE request}
>  \end{itemize}
>  
> +\conformance{\subsection}{I2C Adapter Driver Conformance}\label{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}
> +
> +An I2C Adapter driver MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\item \ref{drivernormative:Device Types / I2C Adapter Device / Device Operation}
> +\end{itemize}
> +
>  \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
>  
>  A device MUST conform to the following normative statements:
> @@ -477,6 +489,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Device Types / Memory Device / Device Operation / STATE request}
>  \end{itemize}
>  
> +\conformance{\subsection}{I2C Adapter Device Conformance}\label{sec:Conformance / Device Conformance / I2C Adapter Device Conformance}
> +
> +An I2C Adapter device MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\item \ref{devicenormative:Device Types / I2C Adapter Device / Device Operation}
> +\end{itemize}
> +
>  \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
>  A conformant implementation MUST be either transitional or
>  non-transitional, see \ref{intro:Legacy
> diff --git a/content.tex b/content.tex
> index 91855b4..95e2ed8 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -6200,6 +6200,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  \input{virtio-iommu.tex}
>  \input{virtio-sound.tex}
>  \input{virtio-mem.tex}
> +\input{virtio-i2c.tex}
>  
>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>  
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> new file mode 100644
> index 0000000..96c96bf
> --- /dev/null
> +++ b/virtio-i2c.tex
> @@ -0,0 +1,94 @@
> +\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 manage I2C slave devices from the guest. By attaching 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. 
> +
> +\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}
> +
> +The driver queues requests to the virtqueue, and they are used by the
> +device. The request is the representation of one segment of an I2C
> +transaction. Each request is of form:
> +
> +\begin{lstlisting}
> +struct virtio_i2c_req {
> +        le16 addr;
> +        le16 flags;
> +        le16 len;
> +        u8 buf[];
> +        u8 status;
> +};
> +\end{lstlisting}
> +
> +The \field{addr} is the address of the I2C slave device.
> +
> +The first bit of \field{flags} indicates whether it is a read or write request.
> +It means a read request if the first bit of \field{flags} is set, otherwise
> +it is a write request. The rest bits of \field{flags} are reserved.
> +
> +The \field{len} is the number of data bytes in the \field{buf} being read from or
> +written to the I2C slave address.
> +
> +The \field{buf} of the request contains one segment of an I2C transaction.
> +If the first bit of \field{flags} is '1', the \field{buf} is written by the
> +device and it contains one segment of an I2C transaction being read from the
> +device.

Let's give a name to the flag then? READ I guess?
I guess this means it's an exact reverse of the write-only/read-only
flag in the descriptor?
I still think it's both a potential source of errors and a waste
to have this bit in the device struct when we have a generic one.

How about adding some motivation to explain why it's done
like this?

> If the first bit of \field{flags} is '0', the \field{buf} is written
> +by the driver and it contains one segment of an I2C transaction being written
> +to the the device.

one the?


more detail on how are multi-segment transactions formed?
don't you need flags to start/stop?



> +The final \field{status} byte 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}

what if one segment in a transaction fails?

> +\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> +
> +A driver MUST set \field{addr}, \field{flags} and \field{len} before sending
> +the request.
> +
> +A driver MUST place one segment of an I2C transaction into \field{buf} if the
> +first bit of \field{flags} is '0'.
> +
> +A driver MUST NOT use \field{addr}, \field{flags}, \field{len} and \field{buf}
> +if the final \field{status} returned from the device is VIRTIO_I2C_MSG_ERR.
> +
> +A driver MUST queue the requests in order if multiple requests are going to
> +be sent at a time.
> +
> +\devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> +
> +A device MUST NOT change the value of \field{addr}, \field{flags} and \field{len}.
> +
> +A device MUST place one segment of an I2C transaction into \field{buf} if the first
> +bit of \field{flags} is '1'.
> +
> +A device MUST guarantee the requests being handled in order if multiple requests
> +are received.
> -- 
> 2.7.4


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/

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

[Jie Deng]

On 2020/10/27 20:20, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2020 at 02:00:24PM +0800, Jie Deng wrote:
>> virtio-i2c is a virtual I2C adapter device. It provides a way
>> to flexibly communicate with the I2C slave devices from the guest.
>>
>> This patch adds the specification for this device.
>>
>> Signed-off-by: Jie Deng<jie.deng@intel.com>
>> ---
>>   conformance.tex | 28 ++++++++++++++---
>>   content.tex     |  1 +
>>   virtio-i2c.tex  | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 119 insertions(+), 4 deletions(-)
>>   create mode 100644 virtio-i2c.tex
>>
>> +The driver queues requests to the virtqueue, and they are used by the
>> +device. The request is the representation of one segment of an I2C
>> +transaction. Each request is of form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_i2c_req {
>> +        le16 addr;
>> +        le16 flags;
>> +        le16 len;
>> +        u8 buf[];
>> +        u8 status;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{addr} is the address of the I2C slave device.
>> +
>> +The first bit of \field{flags} indicates whether it is a read or write request.
>> +It means a read request if the first bit of \field{flags} is set, otherwise
>> +it is a write request. The rest bits of \field{flags} are reserved.
>> +
>> +The \field{len} is the number of data bytes in the \field{buf} being read from or
>> +written to the I2C slave address.
>> +
>> +The \field{buf} of the request contains one segment of an I2C transaction.
>> +If the first bit of \field{flags} is '1', the \field{buf} is written by the
>> +device and it contains one segment of an I2C transaction being read from the
>> +device.
> Let's give a name to the flag then? READ I guess?
> I guess this means it's an exact reverse of the write-only/read-only
> flag in the descriptor?
> I still think it's both a potential source of errors and a waste
> to have this bit in the device struct when we have a generic one.
>
> How about adding some motivation to explain why it's done
> like this?
Hmm... It seems the description here is a bit unsatisfactory. I don't 
mean to use this flag
to play the role of that flag of the descriptor. I just want to 
encapsulate all the i2c_msg fields
into the request for I2C use. The flag in the descriptor is defined from 
virtio perspective
while the flag in this request is defined from I2C perspective.

It may be necessary to reverse the cause and effect:

It seems better to say "If it is a write request (write descriptor), 
then the first bit of the flag in the request should be set to 0."
than "the first bit of the flag in the request is 0, then it is a write 
request (write descriptor)".

I will try to add more description to make it looks better.

Thanks.


>> If the first bit of \field{flags} is '0', the \field{buf} is written
>> +by the driver and it contains one segment of an I2C transaction being written
>> +to the the device.
> one the?
>
Right. Thanks for your careful review.


> more detail on how are multi-segment transactions formed?
> don't you need flags to start/stop?
>
Currently, it is designed to simply transparently transmit
the "i2c_msg messages" from the frontend OS kernel to the backend.
No need to tag the start/stop segment.

Thanks.

>
>> +The final \field{status} byte 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}
> what if one segment in a transaction fails?

The driver shall return the number of segments successfully processed.

Thanks.

>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>> +
>> +A driver MUST set \field{addr}, \field{flags} and \field{len} before sending
>> +the request.
>> +
>> +A driver MUST place one segment of an I2C transaction into \field{buf} if the
>> +first bit of \field{flags} is '0'.
>> +
>> +A driver MUST NOT use \field{addr}, \field{flags}, \field{len} and \field{buf}
>> +if the final \field{status} returned from the device is VIRTIO_I2C_MSG_ERR.
>> +
>> +A driver MUST queue the requests in order if multiple requests are going to
>> +be sent at a time.
>> +
>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>> +
>> +A device MUST NOT change the value of \field{addr}, \field{flags} and \field{len}.
>> +
>> +A device MUST place one segment of an I2C transaction into \field{buf} if the first
>> +bit of \field{flags} is '1'.
>> +
>> +A device MUST guarantee the requests being handled in order if multiple requests
>> +are received.
>> -- 
>> 2.7.4

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/

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

[Michael S. Tsirkin]

On Wed, Oct 28, 2020 at 03:54:39PM +0800, Jie Deng wrote:
> 
> On 2020/10/27 20:20, Michael S. Tsirkin wrote:
> > On Tue, Oct 27, 2020 at 02:00:24PM +0800, Jie Deng wrote:
> > > virtio-i2c is a virtual I2C adapter device. It provides a way
> > > to flexibly communicate with the I2C slave devices from the guest.
> > > 
> > > This patch adds the specification for this device.
> > > 
> > > Signed-off-by: Jie Deng<jie.deng@intel.com>
> > > ---
> > >   conformance.tex | 28 ++++++++++++++---
> > >   content.tex     |  1 +
> > >   virtio-i2c.tex  | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 119 insertions(+), 4 deletions(-)
> > >   create mode 100644 virtio-i2c.tex
> > > 
> > > +The driver queues requests to the virtqueue, and they are used by the
> > > +device. The request is the representation of one segment of an I2C
> > > +transaction. Each request is of form:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_i2c_req {
> > > +        le16 addr;
> > > +        le16 flags;
> > > +        le16 len;
> > > +        u8 buf[];
> > > +        u8 status;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The \field{addr} is the address of the I2C slave device.
> > > +
> > > +The first bit of \field{flags} indicates whether it is a read or write request.
> > > +It means a read request if the first bit of \field{flags} is set, otherwise
> > > +it is a write request. The rest bits of \field{flags} are reserved.
> > > +
> > > +The \field{len} is the number of data bytes in the \field{buf} being read from or
> > > +written to the I2C slave address.
> > > +
> > > +The \field{buf} of the request contains one segment of an I2C transaction.
> > > +If the first bit of \field{flags} is '1', the \field{buf} is written by the
> > > +device and it contains one segment of an I2C transaction being read from the
> > > +device.
> > Let's give a name to the flag then? READ I guess?
> > I guess this means it's an exact reverse of the write-only/read-only
> > flag in the descriptor?
> > I still think it's both a potential source of errors and a waste
> > to have this bit in the device struct when we have a generic one.
> > 
> > How about adding some motivation to explain why it's done
> > like this?
> Hmm... It seems the description here is a bit unsatisfactory. I don't mean
> to use this flag
> to play the role of that flag of the descriptor. I just want to encapsulate
> all the i2c_msg fields
> into the request for I2C use. The flag in the descriptor is defined from
> virtio perspective
> while the flag in this request is defined from I2C perspective.
> It may be necessary to reverse the cause and effect:
> 
> It seems better to say "If it is a write request (write descriptor), then
> the first bit of the flag in the request should be set to 0."
> than "the first bit of the flag in the request is 0, then it is a write
> request (write descriptor)".
> 
> I will try to add more description to make it looks better.
> 
> Thanks.


I think it's better to have the device take it from the virtio descriptor though.
Duplicating info just causes bugs ...

> 
> 
> > > If the first bit of \field{flags} is '0', the \field{buf} is written
> > > +by the driver and it contains one segment of an I2C transaction being written
> > > +to the the device.
> > one the?
> > 
> Right. Thanks for your careful review.
> 
> 
> > more detail on how are multi-segment transactions formed?
> > don't you need flags to start/stop?
> > 
> Currently, it is designed to simply transparently transmit
> the "i2c_msg messages" from the frontend OS kernel to the backend.

From spec perspective we don't really care. We also don't
assume driver and device are using linux.

> No need to tag the start/stop segment.
> 
> Thanks.



i2c_msg has flags to signal start/stop of multi-segment transactions.


> > 
> > > +The final \field{status} byte 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}
> > what if one segment in a transaction fails?
> 
> The driver shall return the number of segments successfully processed.
> 
> Thanks.

where would it return it?

> > > +\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> > > +
> > > +A driver MUST set \field{addr}, \field{flags} and \field{len} before sending
> > > +the request.
> > > +
> > > +A driver MUST place one segment of an I2C transaction into \field{buf} if the
> > > +first bit of \field{flags} is '0'.
> > > +
> > > +A driver MUST NOT use \field{addr}, \field{flags}, \field{len} and \field{buf}
> > > +if the final \field{status} returned from the device is VIRTIO_I2C_MSG_ERR.
> > > +
> > > +A driver MUST queue the requests in order if multiple requests are going to
> > > +be sent at a time.
> > > +
> > > +\devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> > > +
> > > +A device MUST NOT change the value of \field{addr}, \field{flags} and \field{len}.
> > > +
> > > +A device MUST place one segment of an I2C transaction into \field{buf} if the first
> > > +bit of \field{flags} is '1'.
> > > +
> > > +A device MUST guarantee the requests being handled in order if multiple requests
> > > +are received.
> > > -- 
> > > 2.7.4


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/

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

[Paolo Bonzini]

On 27/10/20 07:00, Jie Deng wrote:
> +The driver queues requests to the virtqueue, and they are used by the
> +device. The request is the representation of one segment of an I2C
> +transaction. Each request is of form:
> +
> +\begin{lstlisting}
> +struct virtio_i2c_req {
> +        le16 addr;
> +        le16 flags;
> +        le16 len;
> +        u8 buf[];
> +        u8 status;
> +};
> +\end{lstlisting}

Hi,

one extremely common two-segment I2C transaction is not reproducible
with virtio-i2c.  This is when a write is followed by a read: the master
starts off the transmission with a write, then sends a second START,
then continues with a read from the same address.

Would it make sense to define the request like

struct virtio_i2c_req {
	le16 addr;
	le16 written;
	le16 read;
	u8 bufwrite[];
	u8 status;
	u8 bufread[];
};

instead?

Thanks,

Paolo


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/

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

[Jie Deng]

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

On 2020/10/28 19:27, Michael S. Tsirkin wrote:

> On Wed, Oct 28, 2020 at 03:54:39PM +0800, Jie Deng wrote:
>> On 2020/10/27 20:20, Michael S. Tsirkin wrote:
>>> On Tue, Oct 27, 2020 at 02:00:24PM +0800, Jie Deng wrote:
>>>> virtio-i2c is a virtual I2C adapter device. It provides a way
>>>> to flexibly communicate with the I2C slave devices from the guest.
>>>>
>>>> This patch adds the specification for this device.
>>>>
>>>> Signed-off-by: Jie Deng<jie.deng@intel.com>
>>>> ---
>>>>    conformance.tex | 28 ++++++++++++++---
>>>>    content.tex     |  1 +
>>>>    virtio-i2c.tex  | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 119 insertions(+), 4 deletions(-)
>>>>    create mode 100644 virtio-i2c.tex
>>>>
>>>> +The driver queues requests to the virtqueue, and they are used by the
>>>> +device. The request is the representation of one segment of an I2C
>>>> +transaction. Each request is of form:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_i2c_req {
>>>> +        le16 addr;
>>>> +        le16 flags;
>>>> +        le16 len;
>>>> +        u8 buf[];
>>>> +        u8 status;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +The \field{addr} is the address of the I2C slave device.
>>>> +
>>>> +The first bit of \field{flags} indicates whether it is a read or write request.
>>>> +It means a read request if the first bit of \field{flags} is set, otherwise
>>>> +it is a write request. The rest bits of \field{flags} are reserved.
>>>> +
>>>> +The \field{len} is the number of data bytes in the \field{buf} being read from or
>>>> +written to the I2C slave address.
>>>> +
>>>> +The \field{buf} of the request contains one segment of an I2C transaction.
>>>> +If the first bit of \field{flags} is '1', the \field{buf} is written by the
>>>> +device and it contains one segment of an I2C transaction being read from the
>>>> +device.
>>> Let's give a name to the flag then? READ I guess?
>>> I guess this means it's an exact reverse of the write-only/read-only
>>> flag in the descriptor?
>>> I still think it's both a potential source of errors and a waste
>>> to have this bit in the device struct when we have a generic one.
>>>
>>> How about adding some motivation to explain why it's done
>>> like this?
>> Hmm... It seems the description here is a bit unsatisfactory. I don't mean
>> to use this flag
>> to play the role of that flag of the descriptor. I just want to encapsulate
>> all the i2c_msg fields
>> into the request for I2C use. The flag in the descriptor is defined from
>> virtio perspective
>> while the flag in this request is defined from I2C perspective.
>> It may be necessary to reverse the cause and effect:
>>
>> It seems better to say "If it is a write request (write descriptor), then
>> the first bit of the flag in the request should be set to 0."
>> than "the first bit of the flag in the request is 0, then it is a write
>> request (write descriptor)".
>>
>> I will try to add more description to make it looks better.
>>
>> Thanks.
>
> I think it's better to have the device take it from the virtio descriptor though.
> Duplicating info just causes bugs ...
So do you tend to delete the flags from virtio_i2c_req ?
>>
>>>> If the first bit of \field{flags} is '0', the \field{buf} is written
>>>> +by the driver and it contains one segment of an I2C transaction being written
>>>> +to the the device.
>>> one the?
>>>
>> Right. Thanks for your careful review.
>>
>>
>>> more detail on how are multi-segment transactions formed?
>>> don't you need flags to start/stop?
>>>
>> Currently, it is designed to simply transparently transmit
>> the "i2c_msg messages" from the frontend OS kernel to the backend.
>  From spec perspective we don't really care. We also don't
> assume driver and device are using linux.
I agree.
>> No need to tag the start/stop segment.
>>
>> Thanks.
>
>
> i2c_msg has flags to signal start/stop of multi-segment transactions.
>
Which flags ?

This adapter only declares following I2C functionality for this moment.

I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

So I think we only need the flag "I2C_M_RD" for transaction.

And if you think "I2C_M_RD" is redundant, I can remove the two byte 
flags from

virtio_i2c_req.

Thanks.

>>>> +The final \field{status} byte 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}
>>> what if one segment in a transaction fails?
>> The driver shall return the number of segments successfully processed.
>>
>> Thanks.
> where would it return it?
The requests are being handled in order, so we can return the number of
the last segment being successfully processed.

Thanks.

>>>> 2.7.4

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

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

[Jie Deng]

On 2020/10/29 4:10, Paolo Bonzini wrote:
> On 27/10/20 07:00, Jie Deng wrote:
>> +The driver queues requests to the virtqueue, and they are used by the
>> +device. The request is the representation of one segment of an I2C
>> +transaction. Each request is of form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_i2c_req {
>> +        le16 addr;
>> +        le16 flags;
>> +        le16 len;
>> +        u8 buf[];
>> +        u8 status;
>> +};
>> +\end{lstlisting}
> Hi,
>
> one extremely common two-segment I2C transaction is not reproducible
> with virtio-i2c.  This is when a write is followed by a read: the master
> starts off the transmission with a write, then sends a second START,
> then continues with a read from the same address.
>
> Would it make sense to define the request like
>
> struct virtio_i2c_req {
> 	le16 addr;
> 	le16 written;
> 	le16 read;
> 	u8 bufwrite[];
> 	u8 status;
> 	u8 bufread[];
> };
>
> instead?
>
> Thanks,
>
> Paolo
>
Hi Paolo,

I'm not sure why this two-segment I2C transaction is not reproducible ?

my understanding is that we don't need to care about the 
START/STOP/RESTART conditions
between virtio-i2c frontend and backend. These conditions only make 
sense when data
is exchanged between master and slave devices, rather than virtio 
frontend and backend.

Only the backend may need to consider these conditions when doing device 
emulation and
exchanging data with its slave devices. It has nothing to do with the 
frontend.

Is my understanding correct ?

Thanks


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/

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

[Jie Deng]

On 2020/10/29 4:10, Paolo Bonzini wrote:
> On 27/10/20 07:00, Jie Deng wrote:
>> +The driver queues requests to the virtqueue, and they are used by the
>> +device. The request is the representation of one segment of an I2C
>> +transaction. Each request is of form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_i2c_req {
>> +        le16 addr;
>> +        le16 flags;
>> +        le16 len;
>> +        u8 buf[];
>> +        u8 status;
>> +};
>> +\end{lstlisting}
> Hi,
>
> one extremely common two-segment I2C transaction is not reproducible
> with virtio-i2c.  This is when a write is followed by a read: the master
> starts off the transmission with a write, then sends a second START,
> then continues with a read from the same address.
>
> Would it make sense to define the request like
>
> struct virtio_i2c_req {
> 	le16 addr;
> 	le16 written;
> 	le16 read;
> 	u8 bufwrite[];
> 	u8 status;
> 	u8 bufread[];
> };
>
> instead?
>
> Thanks,
>
> Paolo

Hi,

Do you have any comments on Paolo's proposal ?

Thanks.


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/