[virtio-dev][PATCH 0/2] virtio-spi: add virtual SPI master

[virtio-dev][PATCH 0/2] virtio-spi: add virtual SPI master

[Haixu Cui]

virtio-spi is a virtual SPI(Serial Peripheral Interface) master and
it allows a guset to operate and use the physical SPI master controlled
by the host.

Patch summary:
patch 1 define the DEVICE ID for virtio-spi
patch 2 add the specification for virtio-spi

Haixu Cui (2):
  virtio-spi: define the DEVICE ID for virtio SPI master
  virtio-spi: add the device specification

 content.tex                             |   2 +
 device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 ++
 device-types/spi/driver-conformance.tex |   7 ++
 4 files changed, 171 insertions(+)
 create mode 100644 device-types/spi/description.tex
 create mode 100644 device-types/spi/device-conformance.tex
 create mode 100644 device-types/spi/driver-conformance.tex

-- 
2.17.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Haixu Cui]

Define the DEVICE ID of virtio SPI master device as 45.
---
 content.tex | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/content.tex b/content.tex
index cff548a..29f2859 100644
--- a/content.tex
+++ b/content.tex
@@ -682,6 +682,8 @@ \chapter{Device Types}\label{sec:Device Types}
 \hline
 44         &   ISM device \\
 \hline
+45         &   SPI master \\
+\hline
 \end{tabular}
 
 Some of the devices above are unspecified by this document,
-- 
2.17.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Michael S. Tsirkin]

On Mon, Apr 17, 2023 at 10:03:52PM +0800, Haixu Cui wrote:
> Define the DEVICE ID of virtio SPI master device as 45.

It does not looks like patch 2 will make it in time for 1.3
but should we vote on reserving device id maybe?
If yes pls create a github issue and request vote on list.

> ---
>  content.tex | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index cff548a..29f2859 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -682,6 +682,8 @@ \chapter{Device Types}\label{sec:Device Types}
>  \hline
>  44         &   ISM device \\
>  \hline
> +45         &   SPI master \\
> +\hline
>  \end{tabular}
>  
>  Some of the devices above are unspecified by this document,
> -- 
> 2.17.1
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] Re: [virtio-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Haixu Cui]

Hi Michael S. Tsirkin, Cornelia Huck,
     I have created an issue 
https://github.com/oasis-tcs/virtio-spec/issues/174 to request Device ID 
45 for SPI master. Please help to deal with it, thank you very much.

Best Regards
Haixu Cui

On 6/30/2023 10:57 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 17, 2023 at 10:03:52PM +0800, Haixu Cui wrote:
>> Define the DEVICE ID of virtio SPI master device as 45.
> 
> It does not looks like patch 2 will make it in time for 1.3
> but should we vote on reserving device id maybe?
> If yes pls create a github issue and request vote on list.
> 
>> ---
>>   content.tex | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index cff548a..29f2859 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -682,6 +682,8 @@ \chapter{Device Types}\label{sec:Device Types}
>>   \hline
>>   44         &   ISM device \\
>>   \hline
>> +45         &   SPI master \\
>> +\hline
>>   \end{tabular}
>>   
>>   Some of the devices above are unspecified by this document,
>> -- 
>> 2.17.1
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

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-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Haixu Cui]

Hi Michael S. Tsirkin, Cornelia Huck,
     I have created an issue 
https://github.com/oasis-tcs/virtio-spec/issues/174 to request Device ID 
45 for SPI master. Please help to deal with it, thank you very much.

Best Regards
Haixu Cui

On 6/30/2023 10:57 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 17, 2023 at 10:03:52PM +0800, Haixu Cui wrote:
>> Define the DEVICE ID of virtio SPI master device as 45.
> 
> It does not looks like patch 2 will make it in time for 1.3
> but should we vote on reserving device id maybe?
> If yes pls create a github issue and request vote on list.
> 
>> ---
>>   content.tex | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index cff548a..29f2859 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -682,6 +682,8 @@ \chapter{Device Types}\label{sec:Device Types}
>>   \hline
>>   44         &   ISM device \\
>>   \hline
>> +45         &   SPI master \\
>> +\hline
>>   \end{tabular}
>>   
>>   Some of the devices above are unspecified by this document,
>> -- 
>> 2.17.1
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] Re: [virtio-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Cornelia Huck]

On Fri, Jul 07 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Michael S. Tsirkin, Cornelia Huck,
>      I have created an issue 
> https://github.com/oasis-tcs/virtio-spec/issues/174 to request Device ID 
> 45 for SPI master. Please help to deal with it, thank you very much.

Technically, we're in feature freeze already, but practically, I don't
think we should block adding a simple device id reservation... Michael,
any objections to me creating a ballot later today?


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-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Cornelia Huck]

On Fri, Jul 07 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Michael S. Tsirkin, Cornelia Huck,
>      I have created an issue 
> https://github.com/oasis-tcs/virtio-spec/issues/174 to request Device ID 
> 45 for SPI master. Please help to deal with it, thank you very much.

Technically, we're in feature freeze already, but practically, I don't
think we should block adding a simple device id reservation... Michael,
any objections to me creating a ballot later today?


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] Re: [virtio-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Michael S. Tsirkin]

On Mon, Jul 10, 2023 at 10:47:35AM +0200, Cornelia Huck wrote:
> On Fri, Jul 07 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
> > Hi Michael S. Tsirkin, Cornelia Huck,
> >      I have created an issue 
> > https://github.com/oasis-tcs/virtio-spec/issues/174 to request Device ID 
> > 45 for SPI master. Please help to deal with it, thank you very much.
> 
> Technically, we're in feature freeze already, but practically, I don't
> think we should block adding a simple device id reservation... Michael,
> any objections to me creating a ballot later today?

I think it's fine. We can also put things on a next branch once we
freeze.

-- 
MST


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-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Michael S. Tsirkin]

On Mon, Jul 10, 2023 at 10:47:35AM +0200, Cornelia Huck wrote:
> On Fri, Jul 07 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
> > Hi Michael S. Tsirkin, Cornelia Huck,
> >      I have created an issue 
> > https://github.com/oasis-tcs/virtio-spec/issues/174 to request Device ID 
> > 45 for SPI master. Please help to deal with it, thank you very much.
> 
> Technically, we're in feature freeze already, but practically, I don't
> think we should block adding a simple device id reservation... Michael,
> any objections to me creating a ballot later today?

I think it's fine. We can also put things on a next branch once we
freeze.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Haixu Cui]

virtio-spi is a virtual SPI master and it allows a guset to operate and
use the physical SPI master controlled by the host.
---
 device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 ++
 device-types/spi/driver-conformance.tex |   7 ++
 3 files changed, 169 insertions(+)
 create mode 100644 device-types/spi/description.tex
 create mode 100644 device-types/spi/device-conformance.tex
 create mode 100644 device-types/spi/driver-conformance.tex

diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
new file mode 100644
index 0000000..a68e5f4
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,155 @@
+\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
+
+virtio-spi is a virtual SPI(Serial Peripheral Interface) master and it allows a
+guest to operate and use the physical SPI master devices controlled by the host.
+By attaching the host SPI controlled nodes to the virtual SPI master device, the
+guest can communicate with them without changing or adding extra drivers for these
+controlled SPI devices.
+
+In a typical host and guest architecture with Virtio SPI, Virtio SPI driver
+is the front-end and exists in the guest kernel, Virtio SPI device acts as
+the back-end and exists in the host. And VirtQueue is used for communication
+between the front-end and the back-end.
+
+\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
+45
+
+\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
+
+None.
+
+\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for the Virtio SPI driver.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+    u32 bus_num;
+    u32 chip_select_max_number;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{bus_num}] is the physical spi master assigned to guset, and this is SOC-specific.
+
+\item[\field{chip_select_max_number}] is the number of chipselect the physical spi master supported.
+\end{description}
+
+\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
+
+\begin{itemize}
+\item The Virtio SPI driver configures and initializes the virtqueue.
+
+\item The Virtio SPI driver allocates and registers the virtual SPI master.
+\end{itemize}
+
+\subsection{Device Operation}\label{sec:Device Types / SPI Master Device / Device Operation}
+
+\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI Master Device / Device Operation: Request Queue}
+
+The Virtio SPI driver queues requests to the virtqueue, and they are used by the
+Virtio SPI device. Each request represents one SPI transfer and it is of the form:
+
+\begin{lstlisting}
+struct virtio_spi_transfer_head {
+    u32 mode;
+    u32 freq;
+    u32 word_delay;
+    u8 slave_id;
+    u8 bits_per_word;
+    u8 cs_change;
+    u8 reserved;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_transfer_end {
+    u8 status;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_req {
+    struct virtio_spi_transfer_head head;
+    u8 *rx_buf;
+    u8 *tx_buf;
+    struct virtio_spi_transfer_end end;
+};
+\end{lstlisting}
+
+The \field{mode} defines the SPI transfer mode.
+
+The \field{freq} defines the SPI transfer speed in Hz.
+
+The \field{word_delay} defines how long to wait between words within one SPI transfer,
+in ns unit.
+
+The \field{slave_id} defines the chipselect index the SPI transfer used.
+
+The \field{bits_per_word} defines the number of bits in each SPI transfer word.
+
+The \field{cs_change} defines whether to deselect device before starting the next SPI transfer.
+
+The \field{rx_buf} is the receive buffer, used to hold the data received from the external device.
+
+The \field{tx_buf} is the transmit buffer, used to hold the data sent to the external device.
+
+The final \field{status} byte of the request is written by the Virtio SPI device: either
+VIRTIO_SPI_MSG_OK for success or VIRTIO_SPI_MSG_ERR for error.
+
+\begin{lstlisting}
+#define VIRTIO_SPI_MSG_OK     0
+#define VIRTIO_SPI_MSG_ERR    1
+\end{lstlisting}
+
+\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
+
+Members in \field{struct virtio_spi_transfer_head} are determined by the Virtio SPI driver, while \field{status}
+in \field{struct virtio_spi_transfer_end} is determined by the processing of the Virtio SPI device.
+
+Virtio SPI supports three transfer types: 1) half-duplex read, 2) half-duplex write, 3) full-duplex read and write.
+For half-duplex read transfer, \field{rx_buf} is filled by the Virtio SPI device and consumed by the Virtio SPI driver.
+For half-duplex write transfer, \field{tx_buf} is filled by the Virtio SPI driver and consumed by the Virtio SPI device.
+And for full-duplex read and write transfer, both \field{tx_buf} and \field{rx_buf} are used.
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
+
+The Virtio SPI driver MUST send messages on the requestq virtqueue.
+
+The \field{struct spi_transfer_head} MUST be filled by the Virtio SPI driver
+and MUST be readable for the Virtio SPI device.
+
+The \field{struct spi_transfer_end} MUST be filled by the Virtio SPI device
+and MUST be writable for the Virtio SPI device.
+
+For half-duplex read, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
+\field{rx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
+
+For half-duplex write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
+\field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
+
+For full-duplex read and write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
+\field{rx_buf}, \field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
+
+For half-duplex write or full-duplex read and write, the Virtio SPI driver MUST not use
+\field{rx_buf} if the final \field{status} returned from the Virtio SPI device is VIRTIO_SPI_MSG_ERR.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
+
+The Virtio SPI device MUST set all the fields of the \field{struct virtio_spi_config} on
+receiving a configuration request from the Virtio SPI driver.
+
+The Virtio SPI device MUST set the \field{struct spi_transfer_end} before sending it
+back to the Virtio SPI driver.
+
+The Virtio SPI device MUST be able to identify the transfer type according to the
+received VirtQueue descriptors.
+
+The Virtio SPI device MUST NOT change the value of the send buffer if transfer type
+is half-duplex write or full-duplex read and write.
diff --git a/device-types/spi/device-conformance.tex b/device-types/spi/device-conformance.tex
new file mode 100644
index 0000000..b9dea91
--- /dev/null
+++ b/device-types/spi/device-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Master Device Conformance}\label{sec:Conformance / Device Conformance / SPI Master Device Conformance}
+
+A SPI Master device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / SPI Master Device / Device Operation}
+\end{itemize}
diff --git a/device-types/spi/driver-conformance.tex b/device-types/spi/driver-conformance.tex
new file mode 100644
index 0000000..719b42e
--- /dev/null
+++ b/device-types/spi/driver-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Master Driver Conformance}\label{sec:Conformance / Driver Conformance / SPI Master Driver Conformance}
+
+A SPI Master driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
+\end{itemize}
-- 
2.17.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Cornelia Huck]

On Mon, Apr 17 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

Meta comment: I think you want to send to virtio-comment (feel free to
keep virtio-dev on cc:).

> virtio-spi is a virtual SPI master and it allows a guset to operate and
> use the physical SPI master controlled by the host.
> ---
>  device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
>  device-types/spi/device-conformance.tex |   7 ++
>  device-types/spi/driver-conformance.tex |   7 ++
>  3 files changed, 169 insertions(+)
>  create mode 100644 device-types/spi/description.tex
>  create mode 100644 device-types/spi/device-conformance.tex
>  create mode 100644 device-types/spi/driver-conformance.tex
>
> diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
> new file mode 100644
> index 0000000..a68e5f4
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,155 @@
> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
> +
> +virtio-spi is a virtual SPI(Serial Peripheral Interface) master and it allows a

Nit: missing blank after "SPI"

s/it//

> +guest to operate and use the physical SPI master devices controlled by the host.
> +By attaching the host SPI controlled nodes to the virtual SPI master device, the
> +guest can communicate with them without changing or adding extra drivers for these
> +controlled SPI devices.

Can we rewrite this paragraph without relying on the host/guest
terminology? Does

"allows a driver to operate and use the physical SPI master devices
controlled by the device"

capture the essence of what this is doing?

I don't quite understand the second sentence, maybe someone familiar
with SPI can come up with something.

> +
> +In a typical host and guest architecture with Virtio SPI, Virtio SPI driver
> +is the front-end and exists in the guest kernel, Virtio SPI device acts as

s/guest kernel/guest/ ?

> +the back-end and exists in the host. And VirtQueue is used for communication
> +between the front-end and the back-end.

"A virtqueue is used for communication between the front-end and the
back-end." ?

[I *think* "virtqueue" is the term we've agreed on?]

> +
> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
> +45
> +
> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
> +
> +None.
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
> +
> +All fields of this configuration are always available and read-only for the Virtio SPI driver.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> +    u32 bus_num;
> +    u32 chip_select_max_number;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{bus_num}] is the physical spi master assigned to guset, and this is SOC-specific.

Maybe better "the physical SPI master controled by the device"?

> +
> +\item[\field{chip_select_max_number}] is the number of chipselect the physical spi master supported.
> +\end{description}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
> +
> +\begin{itemize}
> +\item The Virtio SPI driver configures and initializes the virtqueue.
> +
> +\item The Virtio SPI driver allocates and registers the virtual SPI master.

What does this mean? Shouldn't we rather only require the driver to set
up the virtqueue and leave details on how to operate beyond the device
<-> driver interface to the implementation?

> +\end{itemize}
> +
> +\subsection{Device Operation}\label{sec:Device Types / SPI Master Device / Device Operation}
> +
> +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI Master Device / Device Operation: Request Queue}
> +
> +The Virtio SPI driver queues requests to the virtqueue, and they are used by the
> +Virtio SPI device. Each request represents one SPI transfer and it is of the form:
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +    u32 mode;
> +    u32 freq;
> +    u32 word_delay;
> +    u8 slave_id;
> +    u8 bits_per_word;
> +    u8 cs_change;
> +    u8 reserved;
> +};
> +\end{lstlisting}

[Side note: it seems the master/slave terminology is baked into SPI and
I assume we cannot avoid it?]

> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_end {
> +    u8 status;
> +};
> +\end{lstlisting}
> +
> +\begin{lstlisting}
> +struct virtio_spi_req {
> +    struct virtio_spi_transfer_head head;
> +    u8 *rx_buf;
> +    u8 *tx_buf;
> +    struct virtio_spi_transfer_end end;
> +};
> +\end{lstlisting}
> +
> +The \field{mode} defines the SPI transfer mode.
> +
> +The \field{freq} defines the SPI transfer speed in Hz.
> +
> +The \field{word_delay} defines how long to wait between words within one SPI transfer,
> +in ns unit.
> +
> +The \field{slave_id} defines the chipselect index the SPI transfer used.
> +
> +The \field{bits_per_word} defines the number of bits in each SPI transfer word.
> +
> +The \field{cs_change} defines whether to deselect device before starting the next SPI transfer.
> +
> +The \field{rx_buf} is the receive buffer, used to hold the data received from the external device.
> +
> +The \field{tx_buf} is the transmit buffer, used to hold the data sent to the external device.
> +
> +The final \field{status} byte of the request is written by the Virtio SPI device: either
> +VIRTIO_SPI_MSG_OK for success or VIRTIO_SPI_MSG_ERR for error.
> +
> +\begin{lstlisting}
> +#define VIRTIO_SPI_MSG_OK     0
> +#define VIRTIO_SPI_MSG_ERR    1
> +\end{lstlisting}
> +
> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
> +
> +Members in \field{struct virtio_spi_transfer_head} are determined by the Virtio SPI driver, while \field{status}
> +in \field{struct virtio_spi_transfer_end} is determined by the processing of the Virtio SPI device.
> +
> +Virtio SPI supports three transfer types: 1) half-duplex read, 2) half-duplex write, 3) full-duplex read and write.
> +For half-duplex read transfer, \field{rx_buf} is filled by the Virtio SPI device and consumed by the Virtio SPI driver.
> +For half-duplex write transfer, \field{tx_buf} is filled by the Virtio SPI driver and consumed by the Virtio SPI device.
> +And for full-duplex read and write transfer, both \field{tx_buf} and \field{rx_buf} are used.
> +
> +\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
> +
> +The Virtio SPI driver MUST send messages on the requestq virtqueue.
> +
> +The \field{struct spi_transfer_head} MUST be filled by the Virtio SPI driver
> +and MUST be readable for the Virtio SPI device.
> +
> +The \field{struct spi_transfer_end} MUST be filled by the Virtio SPI device
> +and MUST be writable for the Virtio SPI device.
> +
> +For half-duplex read, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
> +\field{rx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
> +
> +For half-duplex write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
> +\field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
> +
> +For full-duplex read and write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
> +\field{rx_buf}, \field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
> +
> +For half-duplex write or full-duplex read and write, the Virtio SPI driver MUST not use
> +\field{rx_buf} if the final \field{status} returned from the Virtio SPI device is VIRTIO_SPI_MSG_ERR.
> +
> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
> +
> +The Virtio SPI device MUST set all the fields of the \field{struct virtio_spi_config} on
> +receiving a configuration request from the Virtio SPI driver.
> +
> +The Virtio SPI device MUST set the \field{struct spi_transfer_end} before sending it
> +back to the Virtio SPI driver.
> +
> +The Virtio SPI device MUST be able to identify the transfer type according to the
> +received VirtQueue descriptors.
> +
> +The Virtio SPI device MUST NOT change the value of the send buffer if transfer type
> +is half-duplex write or full-duplex read and write.

I'm not familiar with how SPI works in general and would appreciate if
someone else could chime in here.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Haixu Cui]

Hello Cornelia Huck,
     Thank you so much for your comments. I respond these comments, 
could you please help check again. Really appreciate.
     I will raise next proposal to fix these comments.

Best Regards
Haixu Cui

On 4/18/2023 5:10 PM, Cornelia Huck wrote:
> On Mon, Apr 17 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
> Meta comment: I think you want to send to virtio-comment (feel free to
> keep virtio-dev on cc:).
Yes, I will send to virtio-comment and cc to virtio-dev in subsequent 
commits.

> 
>> virtio-spi is a virtual SPI master and it allows a guset to operate and
>> use the physical SPI master controlled by the host.
>> ---
>>   device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 ++
>>   device-types/spi/driver-conformance.tex |   7 ++
>>   3 files changed, 169 insertions(+)
>>   create mode 100644 device-types/spi/description.tex
>>   create mode 100644 device-types/spi/device-conformance.tex
>>   create mode 100644 device-types/spi/driver-conformance.tex
>>
>> diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
>> new file mode 100644
>> index 0000000..a68e5f4
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,155 @@
>> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
>> +
>> +virtio-spi is a virtual SPI(Serial Peripheral Interface) master and it allows a
> 
> Nit: missing blank after "SPI"
Got it. Will add blank after SPI.
> 
> s/it//
> 
>> +guest to operate and use the physical SPI master devices controlled by the host.
>> +By attaching the host SPI controlled nodes to the virtual SPI master device, the
>> +guest can communicate with them without changing or adding extra drivers for these
>> +controlled SPI devices.
> 
> Can we rewrite this paragraph without relying on the host/guest
> terminology? Does
> 
> "allows a driver to operate and use the physical SPI master devices
> controlled by the device"
> 
> capture the essence of what this is doing?
> 
> I don't quite understand the second sentence, maybe someone familiar
> with SPI can come up with something.
> 
This statement is referred to the Virtio-I2C Chapter, because Virtio-SPI 
is very similar to Virtio-SPI in architecture. The guest can communicate 
through the physical SPI master without operating the hardware directly, 
but calling the physical SPI master driver running on the host. So the 
physical SPI driver on the host doesn't need changes any more.

>> +
>> +In a typical host and guest architecture with Virtio SPI, Virtio SPI driver
>> +is the front-end and exists in the guest kernel, Virtio SPI device acts as
> 
> s/guest kernel/guest/ ?
Yes, I will update in next commit.
> 
>> +the back-end and exists in the host. And VirtQueue is used for communication
>> +between the front-end and the back-end.
> 
> "A virtqueue is used for communication between the front-end and the
> back-end." ?
> 
> [I *think* "virtqueue" is the term we've agreed on?]
Yes, I will remove this line.
> 
>> +
>> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
>> +45
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] requestq
>> +\end{description}
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
>> +
>> +None.
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
>> +
>> +All fields of this configuration are always available and read-only for the Virtio SPI driver.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +    u32 bus_num;
>> +    u32 chip_select_max_number;
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{description}
>> +\item[\field{bus_num}] is the physical spi master assigned to guset, and this is SOC-specific.
> 
> Maybe better "the physical SPI master controled by the device"?
Because this item is set by host and used by guest, so it seems that 
this is assigned to the guset by the host.
> 
>> +
>> +\item[\field{chip_select_max_number}] is the number of chipselect the physical spi master supported.
>> +\end{description}
>> +
>> +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
>> +
>> +\begin{itemize}
>> +\item The Virtio SPI driver configures and initializes the virtqueue.
>> +
>> +\item The Virtio SPI driver allocates and registers the virtual SPI master.
> 
> What does this mean? Shouldn't we rather only require the driver to set
> up the virtqueue and leave details on how to operate beyond the device
> <-> driver interface to the implementation?
Yes, I will remove this line in next commit.

>> +\end{itemize}
>> +
>> +\subsection{Device Operation}\label{sec:Device Types / SPI Master Device / Device Operation}
>> +
>> +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI Master Device / Device Operation: Request Queue}
>> +
>> +The Virtio SPI driver queues requests to the virtqueue, and they are used by the
>> +Virtio SPI device. Each request represents one SPI transfer and it is of the form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +    u32 mode;
>> +    u32 freq;
>> +    u32 word_delay;
>> +    u8 slave_id;
>> +    u8 bits_per_word;
>> +    u8 cs_change;
>> +    u8 reserved;
>> +};
>> +\end{lstlisting}
> 
> [Side note: it seems the master/slave terminology is baked into SPI and
> I assume we cannot avoid it?]
On the side of the whole system, both guest and host are master.
But on the side of guset, host just like a proxy, in a way, a slave 
driven by the host.
> 
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_end {
>> +    u8 status;
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_req {
>> +    struct virtio_spi_transfer_head head;
>> +    u8 *rx_buf;
>> +    u8 *tx_buf;
>> +    struct virtio_spi_transfer_end end;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{mode} defines the SPI transfer mode.
>> +
>> +The \field{freq} defines the SPI transfer speed in Hz.
>> +
>> +The \field{word_delay} defines how long to wait between words within one SPI transfer,
>> +in ns unit.
>> +
>> +The \field{slave_id} defines the chipselect index the SPI transfer used.
>> +
>> +The \field{bits_per_word} defines the number of bits in each SPI transfer word.
>> +
>> +The \field{cs_change} defines whether to deselect device before starting the next SPI transfer.
>> +
>> +The \field{rx_buf} is the receive buffer, used to hold the data received from the external device.
>> +
>> +The \field{tx_buf} is the transmit buffer, used to hold the data sent to the external device.
>> +
>> +The final \field{status} byte of the request is written by the Virtio SPI device: either
>> +VIRTIO_SPI_MSG_OK for success or VIRTIO_SPI_MSG_ERR for error.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_MSG_OK     0
>> +#define VIRTIO_SPI_MSG_ERR    1
>> +\end{lstlisting}
>> +
>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
>> +
>> +Members in \field{struct virtio_spi_transfer_head} are determined by the Virtio SPI driver, while \field{status}
>> +in \field{struct virtio_spi_transfer_end} is determined by the processing of the Virtio SPI device.
>> +
>> +Virtio SPI supports three transfer types: 1) half-duplex read, 2) half-duplex write, 3) full-duplex read and write.
>> +For half-duplex read transfer, \field{rx_buf} is filled by the Virtio SPI device and consumed by the Virtio SPI driver.
>> +For half-duplex write transfer, \field{tx_buf} is filled by the Virtio SPI driver and consumed by the Virtio SPI device.
>> +And for full-duplex read and write transfer, both \field{tx_buf} and \field{rx_buf} are used.
>> +
>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>> +
>> +The Virtio SPI driver MUST send messages on the requestq virtqueue.
>> +
>> +The \field{struct spi_transfer_head} MUST be filled by the Virtio SPI driver
>> +and MUST be readable for the Virtio SPI device.
>> +
>> +The \field{struct spi_transfer_end} MUST be filled by the Virtio SPI device
>> +and MUST be writable for the Virtio SPI device.
>> +
>> +For half-duplex read, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
>> +\field{rx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
>> +
>> +For half-duplex write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
>> +\field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
>> +
>> +For full-duplex read and write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
>> +\field{rx_buf}, \field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
>> +
>> +For half-duplex write or full-duplex read and write, the Virtio SPI driver MUST not use
>> +\field{rx_buf} if the final \field{status} returned from the Virtio SPI device is VIRTIO_SPI_MSG_ERR.
>> +
>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>> +
>> +The Virtio SPI device MUST set all the fields of the \field{struct virtio_spi_config} on
>> +receiving a configuration request from the Virtio SPI driver.
>> +
>> +The Virtio SPI device MUST set the \field{struct spi_transfer_end} before sending it
>> +back to the Virtio SPI driver.
>> +
>> +The Virtio SPI device MUST be able to identify the transfer type according to the
>> +received VirtQueue descriptors.
>> +
>> +The Virtio SPI device MUST NOT change the value of the send buffer if transfer type
>> +is half-duplex write or full-duplex read and write.
> 
> I'm not familiar with how SPI works in general and would appreciate if
> someone else could chime in here.
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Harald Mommer]

Hello,

I've some comments on the draft specification. Looks promising but 
pointers in structures used to exchange information between driver and 
device are a no go. And there are some things which need to be (better) 
defined and clarified to implement an SPI device or driver from this 
draft specification.

On 17.04.23 16:03, Haixu Cui wrote:
> virtio-spi is a virtual SPI master and it allows a guset to operate and
> use the physical SPI master controlled by the host.
> ---
>   device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
>   device-types/spi/device-conformance.tex |   7 ++
>   device-types/spi/driver-conformance.tex |   7 ++
>   3 files changed, 169 insertions(+)
>   create mode 100644 device-types/spi/description.tex
>   create mode 100644 device-types/spi/device-conformance.tex
>   create mode 100644 device-types/spi/driver-conformance.tex
>
> diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
> new file mode 100644
> index 0000000..a68e5f4
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,155 @@
> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
> +
> +virtio-spi is a virtual SPI(Serial Peripheral Interface) master and it allows a
> +guest to operate and use the physical SPI master devices controlled by the host.
> +By attaching the host SPI controlled nodes to the virtual SPI master device, the
> +guest can communicate with them without changing or adding extra drivers for these
> +controlled SPI devices.
> +
> +In a typical host and guest architecture with Virtio SPI, Virtio SPI driver
> +is the front-end and exists in the guest kernel, Virtio SPI device acts as
> +the back-end and exists in the host. And VirtQueue is used for communication
> +between the front-end and the back-end.
> +
> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
> +45
> +
> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
> +
> +None.
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
> +
> +All fields of this configuration are always available and read-only for the Virtio SPI driver.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> +    u32 bus_num;
> +    u32 chip_select_max_number;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{bus_num}] is the physical spi master assigned to guset, and this is SOC-specific.
> +
> +\item[\field{chip_select_max_number}] is the number of chipselect the physical spi master supported.
> +\end{description}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
> +
> +\begin{itemize}
> +\item The Virtio SPI driver configures and initializes the virtqueue.
> +
> +\item The Virtio SPI driver allocates and registers the virtual SPI master.
> +\end{itemize}
> +
> +\subsection{Device Operation}\label{sec:Device Types / SPI Master Device / Device Operation}
> +
> +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI Master Device / Device Operation: Request Queue}
> +
> +The Virtio SPI driver queues requests to the virtqueue, and they are used by the
> +Virtio SPI device. Each request represents one SPI transfer and it is of the form:
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +    u32 mode;
> +    u32 freq;
> +    u32 word_delay;
> +    u8 slave_id;
> +    u8 bits_per_word;
> +    u8 cs_change;
> +    u8 reserved;
> +};

Hunting for some length information of tx_buf and rx_buf.

The driver provides the device readable and the device writable buffers.

So for half duplex write the length of tx_buf is the total length of the 
device readable descriptors - transfer head.

So for half duplex read the length of rx_buf is the total length of the 
device writable descriptors - transfer end.

if (total size of device readable descriptors == transfer head) => half 
duplex read

if (total size of device writable descriptors == transfer end) => half 
duplex write

otherwise it's full duplex

The "mode" field seems to be foreseen to determine "half duplex read", 
"half duplex write" or "full duplex". So the "mode" field is redundant 
information but this is really no issue.

I think I got it now how to get the length information for all the 
possible transfers without having any buffer length information in the 
transfer head. Tricky. Should really be elaborated in the specification, 
took me hours to get it and I'm not convinced yet it's that. Initially I 
thought the length information in the transfer head was simply missing 
but most probably it is indeed not because the information can be 
obtained in a different way from the virtqueues.

What to send for half duplex read? Is it don't care or 0xFF, 0x00 or 
something else which needs to be determinable? In the past I had to use 
SPI always full duplex on the small controllers I worked with so the 
question did not arise.

> +\end{lstlisting}
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_end {
> +    u8 status; // Device=>Driver, device writable
> +};
> +\end{lstlisting}
> +
> +\begin{lstlisting}
> +struct virtio_spi_req {
> +    struct virtio_spi_transfer_head head; // Driver=>Device, device readable
> +    u8 *rx_buf; // Device=>Driver, device writable
> +    u8 *tx_buf; // Driver=>Device, device readable

Won't work 1:

"2.7.4.2 Driver Requirements: Message Framing
The driver MUST place any device­-writable descriptor elements after any 
device­-readable descriptor ele­ments."

=> First tx_buf and afterwards rx_buf. The elements need to be exchanged.

Won't work 2:

You cannot send pointers around in virtio. A pointer in the virtual 
address space of the device has no meaning in the virtual address space 
of the driver and vice versa. And what's a pointer? Is it 32 bit or 64 
bit? Casting to le64 won't help also. You need to have buffers of 
sufficient size instead of the pointers in the structure.

Compare this with struct virtio_i2c_req from the I2C chapter:

u8 buf[];

This is a buffer of sufficient size, not a pointer. As there is only 1 
device writable descriptor (struct virtio_i2c_in_hdr) we have there also 
no issue finding the status. Different here. We have tx_buf in front of 
the status and we don't know how long this is.

The buf[] length in I2C may be obtained by the device from the bytes 
written by the driver to the virtqueue (need to check again) so no issue 
for I2C as they seem not to have a dedicated length information in their 
message structures.

=>

u8 tx_buf[];

u8 rx_buf[];

> +    struct virtio_spi_transfer_end end;
> +};
> +\end{lstlisting}
> +
> +The \field{mode} defines the SPI transfer mode.

Difficult. Your enumeration below looks like a enumeration in the text 
and not like the definition of values. Better is to clearly define the 
values.

#define VIRTIO_SPI_MODE_XXX 1 /* or whatever */

#define VIRTIO_SPI_MODE_YYY 2 /* or whatever */

#define VIRTIO_SPI_MODE_ZZZ 3 /* or whatever */

> +
> +The \field{freq} defines the SPI transfer speed in Hz.
> +
> +The \field{word_delay} defines how long to wait between words within one SPI transfer,
> +in ns unit.
> +
> +The \field{slave_id} defines the chipselect index the SPI transfer used.
> +
> +The \field{bits_per_word} defines the number of bits in each SPI transfer word.
> +
> +The \field{cs_change} defines whether to deselect device before starting the next SPI transfer.
Most probably 0 is "don't deselect" and 1 is "deselect". Better define 
this clearly.
> +
> +The \field{rx_buf} is the receive buffer, used to hold the data received from the external device.
> +
> +The \field{tx_buf} is the transmit buffer, used to hold the data sent to the external device.
> +
> +The final \field{status} byte of the request is written by the Virtio SPI device: either
> +VIRTIO_SPI_MSG_OK for success or VIRTIO_SPI_MSG_ERR for error.
> +
> +\begin{lstlisting}
> +#define VIRTIO_SPI_MSG_OK     0
> +#define VIRTIO_SPI_MSG_ERR    1
> +\end{lstlisting}
> +
> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
> +
> +Members in \field{struct virtio_spi_transfer_head} are determined by the Virtio SPI driver, while \field{status}
> +in \field{struct virtio_spi_transfer_end} is determined by the processing of the Virtio SPI device.
"determined" = "written" or "filled"? Below you use "filled". Beware 
English is not my native language but at least in the translation to 
German "determined" becomes ambiguous so it may be indeed ambiguous.
> +
> +Virtio SPI supports three transfer types: 1) half-duplex read, 2) half-duplex write, 3) full-duplex read and write.
> +For half-duplex read transfer, \field{rx_buf} is filled by the Virtio SPI device and consumed by the Virtio SPI driver.
> +For half-duplex write transfer, \field{tx_buf} is filled by the Virtio SPI driver and consumed by the Virtio SPI device.
> +And for full-duplex read and write transfer, both \field{tx_buf} and \field{rx_buf} are used.
The "transfer mode" above has become a "transfer type" here.
> +
> +\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
> +
> +The Virtio SPI driver MUST send messages on the requestq virtqueue.
> +
> +The \field{struct spi_transfer_head} MUST be filled by the Virtio SPI driver
> +and MUST be readable for the Virtio SPI device.
> +
> +The \field{struct spi_transfer_end} MUST be filled by the Virtio SPI device
> +and MUST be writable for the Virtio SPI device.

Isn't it obvious that the parts which MUST be written by side X must be 
readable by side Y? Nothing wrong here.

However this here is under the headline of "drivernormative" and I see a 
"MUST be filled by the Virtio SPI device" which is a requirement for the 
device.

> +
> +For half-duplex read, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
> +\field{rx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
> +
> +For half-duplex write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
> +\field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
> +
> +For full-duplex read and write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
> +\field{rx_buf}, \field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
> +
> +For half-duplex write or full-duplex read and write, the Virtio SPI driver MUST not use
> +\field{rx_buf} if the final \field{status} returned from the Virtio SPI device is VIRTIO_SPI_MSG_ERR.
> +
> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
> +
> +The Virtio SPI device MUST set all the fields of the \field{struct virtio_spi_config} on
> +receiving a configuration request from the Virtio SPI driver.
> +
> +The Virtio SPI device MUST set the \field{struct spi_transfer_end} before sending it
> +back to the Virtio SPI driver.
> +
> +The Virtio SPI device MUST be able to identify the transfer type according to the
> +received VirtQueue descriptors.

Having difficulties. "Type" is "Mode"? By reading the "mode" field? This 
would be the trivial meaning of the sentence.

Or is there a more deep meaning of "according to the received VirtQueue 
descriptors"?

If the length information of tx_buf and rx_buf are indeed not missing in 
the transfer head structure it would be probably good to elaborate on 
obtaining the length information here.

> +
> +The Virtio SPI device MUST NOT change the value of the send buffer if transfer type
> +is half-duplex write or full-duplex read and write.
> diff --git a/device-types/spi/device-conformance.tex b/device-types/spi/device-conformance.tex
> new file mode 100644
> index 0000000..b9dea91
> --- /dev/null
> +++ b/device-types/spi/device-conformance.tex
> @@ -0,0 +1,7 @@
> +\conformance{\subsection}{SPI Master Device Conformance}\label{sec:Conformance / Device Conformance / SPI Master Device Conformance}
> +
> +A SPI Master device MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\item \ref{devicenormative:Device Types / SPI Master Device / Device Operation}
> +\end{itemize}
> diff --git a/device-types/spi/driver-conformance.tex b/device-types/spi/driver-conformance.tex
> new file mode 100644
> index 0000000..719b42e
> --- /dev/null
> +++ b/device-types/spi/driver-conformance.tex
> @@ -0,0 +1,7 @@
> +\conformance{\subsection}{SPI Master Driver Conformance}\label{sec:Conformance / Driver Conformance / SPI Master Driver Conformance}
> +
> +A SPI Master driver MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
> +\end{itemize}

Regards
Harald Mommer

Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Haixu Cui]

Hi Harald Mommer,

     Thank you so much for all your helpful advice and info!

     I have updated the transfer request as follows:

     struct spi_transfer_head {
           u8 slave_id;
           u8 bits_per_word;
           u8 cs_change;
           u8 tx_nbits;
           u8 rx_nbits;
           u8 reserved[3];
           u32 len;
           u32 mode;
           u32 freq;
           u32 word_delay_ns;
           u32 cs_setup_ns;
           u32 cs_delay_hold_ns;
           u32 cs_change_delay_inactive_ns;
     };

     struct spi_transfer_end {
           u8 result;
     };

     struct virtio_spi_transfer_req {
         struct spi_transfer_head head;
         u8 rx_buf[];
         u8 tx_buf[];
         struct spi_end end;
     };


     Also I add the member and field definition as follows:

     @slave_id: chipselect index the SPI transfer used.

     @bits_per_word: the number of bits in each SPI transfer word.

     @cs_change: whether to deselect device after finishing this transfer
         before starting the next transfer, 0 means cs keep asserted and
         1 means cs deasserted then asserted again.

     @tx_nbits: bus width for write transfer.
         0,1: bus width is 1, also known as SINGLE
         2  : bus width is 2, also known as DUAL
         4  : bus width is 4, also known as QUAD
         8  : bus width is 8, also known as OCTAL
         other values are invalid.

     @rx_nbits: bus width for read transfer.
         0,1: bus width is 1, also known as SINGLE
         2  : bus width is 2, also known as DUAL
         4  : bus width is 4, also known as QUAD
         8  : bus width is 8, also known as OCTAL
         other values are invalid.

     @reserved: for future use.

     @len: transfer length.

     @mode: SPI transfer mode.
         bit 0: CPHA, determines the timing (i.e. phase) of the data
             bits relative to the clock pulses.For CPHA=0, the
             "out" side changes the data on the trailing edge of the
             preceding clock cycle, while the "in" side captures the data
             on (or shortly after) the leading edge of the clock cycle.
             For CPHA=1, the "out" side changes the data on the leading
             edge of the current clock cycle, while the "in" side
             captures the data on (or shortly after) the trailing edge of
             the clock cycle.
         bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a
             clock which idles at 0, and each cycle consists of a pulse
             of 1. CPOL=1 is a clock which idles at 1, and each cycle
             consists of a pulse of 0.
         bit 2: CS_HIGH, if 1, chip select active high, else active low.
         bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
             first, else LSB first.
         bit 4: LOOP, loopback mode.

     @freq: the transfer speed in Hz.

     @word_delay_ns: delay to be inserted between consecutive words of a
         transfer, in ns unit.
     @cs_setup_ns: delay to be introduced after CS is asserted, in ns
         unit.
     @cs_delay_hold_ns: delay to be introduced before CS is deasserted
         for each transfer, in ns unit.
     @cs_change_delay_inactive_ns: delay to be introduced after CS is
         deasserted and before next asserted, in ns unit.


     Could you please help review the transfer request above again to 
check if the interface is clear and enough for back-end to perform SPI 
transfers. Thank you again for your comments.


Best Regards
Haixu Cui

On 7/19/2023 2:25 AM, Harald Mommer wrote:
> Hello,
> 
> I've some comments on the draft specification. Looks promising but
> pointers in structures used to exchange information between driver and
> device are a no go. And there are some things which need to be (better)
> defined and clarified to implement an SPI device or driver from this
> draft specification.
> 
> On 17.04.23 16:03, Haixu Cui wrote:
>> virtio-spi is a virtual SPI master and it allows a guset to operate and
>> use the physical SPI master controlled by the host.
>> ---
>>    device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
>>    device-types/spi/device-conformance.tex |   7 ++
>>    device-types/spi/driver-conformance.tex |   7 ++
>>    3 files changed, 169 insertions(+)
>>    create mode 100644 device-types/spi/description.tex
>>    create mode 100644 device-types/spi/device-conformance.tex
>>    create mode 100644 device-types/spi/driver-conformance.tex
>>
>> diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
>> new file mode 100644
>> index 0000000..a68e5f4
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,155 @@
>> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
>> +
>> +virtio-spi is a virtual SPI(Serial Peripheral Interface) master and it allows a
>> +guest to operate and use the physical SPI master devices controlled by the host.
>> +By attaching the host SPI controlled nodes to the virtual SPI master device, the
>> +guest can communicate with them without changing or adding extra drivers for these
>> +controlled SPI devices.
>> +
>> +In a typical host and guest architecture with Virtio SPI, Virtio SPI driver
>> +is the front-end and exists in the guest kernel, Virtio SPI device acts as
>> +the back-end and exists in the host. And VirtQueue is used for communication
>> +between the front-end and the back-end.
>> +
>> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
>> +45
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] requestq
>> +\end{description}
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
>> +
>> +None.
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
>> +
>> +All fields of this configuration are always available and read-only for the Virtio SPI driver.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +    u32 bus_num;
>> +    u32 chip_select_max_number;
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{description}
>> +\item[\field{bus_num}] is the physical spi master assigned to guset, and this is SOC-specific.
>> +
>> +\item[\field{chip_select_max_number}] is the number of chipselect the physical spi master supported.
>> +\end{description}
>> +
>> +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
>> +
>> +\begin{itemize}
>> +\item The Virtio SPI driver configures and initializes the virtqueue.
>> +
>> +\item The Virtio SPI driver allocates and registers the virtual SPI master.
>> +\end{itemize}
>> +
>> +\subsection{Device Operation}\label{sec:Device Types / SPI Master Device / Device Operation}
>> +
>> +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI Master Device / Device Operation: Request Queue}
>> +
>> +The Virtio SPI driver queues requests to the virtqueue, and they are used by the
>> +Virtio SPI device. Each request represents one SPI transfer and it is of the form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +    u32 mode;
>> +    u32 freq;
>> +    u32 word_delay;
>> +    u8 slave_id;
>> +    u8 bits_per_word;
>> +    u8 cs_change;
>> +    u8 reserved;
>> +};
>
> Hunting for some length information of tx_buf and rx_buf.
> 
> The driver provides the device readable and the device writable buffers.
> 
> So for half duplex write the length of tx_buf is the total length of the
> device readable descriptors - transfer head.
> 
> So for half duplex read the length of rx_buf is the total length of the
> device writable descriptors - transfer end.
> 
> if (total size of device readable descriptors == transfer head) => half
> duplex read
> 
> if (total size of device writable descriptors == transfer end) => half
> duplex write
> 
> otherwise it's full duplex
> 
> The "mode" field seems to be foreseen to determine "half duplex read",
> "half duplex write" or "full duplex". So the "mode" field is redundant
> information but this is really no issue.
> 
> I think I got it now how to get the length information for all the
> possible transfers without having any buffer length information in the
> transfer head. Tricky. Should really be elaborated in the specification,
> took me hours to get it and I'm not convinced yet it's that. Initially I
> thought the length information in the transfer head was simply missing
> but most probably it is indeed not because the information can be
> obtained in a different way from the virtqueues.

Yes, in our current design, back-end can parse the transfer length from 
the received virtqueue descriptor, without transfer length information 
involved in the transfer request. Even so, I think it is necessary to 
add the length information to make the transfer request more clear.
> 
> What to send for half duplex read? Is it don't care or 0xFF, 0x00 or
> something else which needs to be determinable? In the past I had to use
> SPI always full duplex on the small controllers I worked with so the
> question did not arise.

For half duplex read, we send virtio_spi_req->head, 
virtio_spi_req->rx_buf, virtio_spi_req->end in order. It doesn't need to 
send the tx_buf.

For half duplex write, send virtio_spi_req->head, 
virtio_spi_req->tx_buf, virtio_spi_req->end in order.

For full duplex, send virtio_spi_req->head, virtio_spi_req->rx_buf, 
virtio_spi_req->tx_buf, virtio_spi_req->end in order.

> 
>> +\end{lstlisting}
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_end {
>> +    u8 status; // Device=>Driver, device writable
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_req {
>> +    struct virtio_spi_transfer_head head; // Driver=>Device, device readable
>> +    u8 *rx_buf; // Device=>Driver, device writable
>> +    u8 *tx_buf; // Driver=>Device, device readable
> 
> Won't work 1:
> 
> "2.7.4.2 Driver Requirements: Message Framing
> The driver MUST place any device­-writable descriptor elements after any
> device­-readable descriptor ele­ments."
> 
> => First tx_buf and afterwards rx_buf. The elements need to be exchanged.
> 
> Won't work 2:
> 
> You cannot send pointers around in virtio. A pointer in the virtual
> address space of the device has no meaning in the virtual address space
> of the driver and vice versa. And what's a pointer? Is it 32 bit or 64
> bit? Casting to le64 won't help also. You need to have buffers of
> sufficient size instead of the pointers in the structure.
> 
> Compare this with struct virtio_i2c_req from the I2C chapter:
> 
> u8 buf[];
> 
> This is a buffer of sufficient size, not a pointer. As there is only 1
> device writable descriptor (struct virtio_i2c_in_hdr) we have there also
> no issue finding the status. Different here. We have tx_buf in front of
> the status and we don't know how long this is.
> 
> The buf[] length in I2C may be obtained by the device from the bytes
> written by the driver to the virtqueue (need to check again) so no issue
> for I2C as they seem not to have a dedicated length information in their
> message structures.
> 
> =>
> 
> u8 tx_buf[];
> 
> u8 rx_buf[];
> 
OK, I will update it.

>> +    struct virtio_spi_transfer_end end;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{mode} defines the SPI transfer mode.
> 
> Difficult. Your enumeration below looks like a enumeration in the text
> and not like the definition of values. Better is to clearly define the
> values.
> 
> #define VIRTIO_SPI_MODE_XXX 1 /* or whatever */
> 
> #define VIRTIO_SPI_MODE_YYY 2 /* or whatever */
> 
> #define VIRTIO_SPI_MODE_ZZZ 3 /* or whatever */
> 

mode here is the same as the mode filed in spi_device structure, in this 
file, bit0 (CPHA) and bit1 (CPOL) are the most important, indicate the 
clock phase and clock polarity respectively.

I will add the definition and meaning of each bit of mode in this 
specification.

>> +
>> +The \field{freq} defines the SPI transfer speed in Hz.
>> +
>> +The \field{word_delay} defines how long to wait between words within one SPI transfer,
>> +in ns unit.
>> +
>> +The \field{slave_id} defines the chipselect index the SPI transfer used.
>> +
>> +The \field{bits_per_word} defines the number of bits in each SPI transfer word.
>> +
>> +The \field{cs_change} defines whether to deselect device before starting the next SPI transfer.
> Most probably 0 is "don't deselect" and 1 is "deselect". Better define
> this clearly.

Exactly! I will add it.
>> +
>> +The \field{rx_buf} is the receive buffer, used to hold the data received from the external device.
>> +
>> +The \field{tx_buf} is the transmit buffer, used to hold the data sent to the external device.
>> +
>> +The final \field{status} byte of the request is written by the Virtio SPI device: either
>> +VIRTIO_SPI_MSG_OK for success or VIRTIO_SPI_MSG_ERR for error.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_MSG_OK     0
>> +#define VIRTIO_SPI_MSG_ERR    1
>> +\end{lstlisting}
>> +
>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
>> +
>> +Members in \field{struct virtio_spi_transfer_head} are determined by the Virtio SPI driver, while \field{status}
>> +in \field{struct virtio_spi_transfer_end} is determined by the processing of the Virtio SPI device.
> "determined" = "written" or "filled"? Below you use "filled". Beware
> English is not my native language but at least in the translation to
> German "determined" becomes ambiguous so it may be indeed ambiguous.

Yeah! "filled" seems better.
Well, "determined", a little strange

>> +
>> +Virtio SPI supports three transfer types: 1) half-duplex read, 2) half-duplex write, 3) full-duplex read and write.
>> +For half-duplex read transfer, \field{rx_buf} is filled by the Virtio SPI device and consumed by the Virtio SPI driver.
>> +For half-duplex write transfer, \field{tx_buf} is filled by the Virtio SPI driver and consumed by the Virtio SPI device.
>> +And for full-duplex read and write transfer, both \field{tx_buf} and \field{rx_buf} are used.
> The "transfer mode" above has become a "transfer type" here.

"transfer mode" reflects the mode filed in spi_device structure, which 
defines the clock phase, clock polarity and so on. This filed is not 
constant, for example it can be changed by calling ioctl with 
SPI_IOC_WR_MODE arguments, so it should be passed to the back-end to 
avoid using the expired mode.

I will add the definition of each bit of mode later.

"transfer type" here means the half duplex or full duplex.

>> +
>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>> +
>> +The Virtio SPI driver MUST send messages on the requestq virtqueue.
>> +
>> +The \field{struct spi_transfer_head} MUST be filled by the Virtio SPI driver
>> +and MUST be readable for the Virtio SPI device.
>> +
>> +The \field{struct spi_transfer_end} MUST be filled by the Virtio SPI device
>> +and MUST be writable for the Virtio SPI device.
> 
> Isn't it obvious that the parts which MUST be written by side X must be
> readable by side Y? Nothing wrong here.
> 
> However this here is under the headline of "drivernormative" and I see a
> "MUST be filled by the Virtio SPI device" which is a requirement for the
> device.

OK, I will remove the device operation to the "devicenormative".
> 
>> +
>> +For half-duplex read, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
>> +\field{rx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
>> +
>> +For half-duplex write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
>> +\field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
>> +
>> +For full-duplex read and write, the Virtio SPI driver MUST send \field{struct spi_transfer_head},
>> +\field{rx_buf}, \field{tx_buf} and \field{struct spi_transfer_end} to the SPI Virtio Device in order.
>> +
>> +For half-duplex write or full-duplex read and write, the Virtio SPI driver MUST not use
>> +\field{rx_buf} if the final \field{status} returned from the Virtio SPI device is VIRTIO_SPI_MSG_ERR.
>> +
>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>> +
>> +The Virtio SPI device MUST set all the fields of the \field{struct virtio_spi_config} on
>> +receiving a configuration request from the Virtio SPI driver.
>> +
>> +The Virtio SPI device MUST set the \field{struct spi_transfer_end} before sending it
>> +back to the Virtio SPI driver.
>> +
>> +The Virtio SPI device MUST be able to identify the transfer type according to the
>> +received VirtQueue descriptors.
> 
> Having difficulties. "Type" is "Mode"? By reading the "mode" field? This
> would be the trivial meaning of the sentence.

As mentioned before, type and mode are different in this specification.
> 
> Or is there a more deep meaning of "according to the received VirtQueue
> descriptors"?
> 
> If the length information of tx_buf and rx_buf are indeed not missing in
> the transfer head structure it would be probably good to elaborate on
> obtaining the length information here.
> 

Obtaining the length information relys on the back-end which will add 
extra requirement to the back-end. I think it is better to have length 
information involved.

>> +
>> +The Virtio SPI device MUST NOT change the value of the send buffer if transfer type
>> +is half-duplex write or full-duplex read and write.
>> diff --git a/device-types/spi/device-conformance.tex b/device-types/spi/device-conformance.tex
>> new file mode 100644
>> index 0000000..b9dea91
>> --- /dev/null
>> +++ b/device-types/spi/device-conformance.tex
>> @@ -0,0 +1,7 @@
>> +\conformance{\subsection}{SPI Master Device Conformance}\label{sec:Conformance / Device Conformance / SPI Master Device Conformance}
>> +
>> +A SPI Master device MUST conform to the following normative statements:
>> +
>> +\begin{itemize}
>> +\item \ref{devicenormative:Device Types / SPI Master Device / Device Operation}
>> +\end{itemize}
>> diff --git a/device-types/spi/driver-conformance.tex b/device-types/spi/driver-conformance.tex
>> new file mode 100644
>> index 0000000..719b42e
>> --- /dev/null
>> +++ b/device-types/spi/driver-conformance.tex
>> @@ -0,0 +1,7 @@
>> +\conformance{\subsection}{SPI Master Driver Conformance}\label{sec:Conformance / Driver Conformance / SPI Master Driver Conformance}
>> +
>> +A SPI Master driver MUST conform to the following normative statements:
>> +
>> +\begin{itemize}
>> +\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
>> +\end{itemize}
> 
> Regards
> Harald Mommer
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Harald Mommer]

Hello,

a quick incomplete on. I'm currently with both hands in an attempt to 
implement a virtio SPI device for our proprietary hypervisor based on 
the draft specification and your E-Mail. Means I see now some more 
things while implementing.

u32 len; /* @len: transfer length.*/

is this a byte or a word count?

The comment in Linux for the length field in struct spi_ioc_transfer is

/* @len: Length of tx and rx buffers, in bytes. */ so I assume this here 
is also a byte count.

As we have only one len I think it's still needed to look at the device 
readable and device writable descriptor sizes to decide on half duplex 
read, half duplex write or full duplex. Having still to do this the 
transfer length in the len field may be redundant (superfluous) 
information.

No problem with this, I'm now only at a point where I want to be sure 
whether this is meant as a byte length (vs. a word length).

Then there is no u32. There is le32. Only RPMB uses be32 but they have a 
special reason to do this. The byte order must be defined for 16 and 32 
bit values!

On 20.07.23 09:50, Haixu Cui wrote:
> Hi Harald Mommer,
>
>     Thank you so much for all your helpful advice and info!
>
>     I have updated the transfer request as follows:
>
>     struct spi_transfer_head {
>           u8 slave_id;
>           u8 bits_per_word;
>           u8 cs_change;
>           u8 tx_nbits;
>           u8 rx_nbits;
>           u8 reserved[3];
>           u32 len;
>           u32 mode;
>           u32 freq;
>           u32 word_delay_ns;
>           u32 cs_setup_ns;
>           u32 cs_delay_hold_ns;
>           u32 cs_change_delay_inactive_ns;
>     };
>
>     struct spi_transfer_end {
>           u8 result;
>     };
May become "struct spi_transfer_result result" for naming reasons with 
same content, see below. Just a proposal which may not be followed, see 
below.
>
>     struct virtio_spi_transfer_req {
>         struct spi_transfer_head head;
>         u8 rx_buf[];
>         u8 tx_buf[];
>         struct spi_end end;
Above this was "struct spi_transfer_end"
>
>     };
>
Device readable must go before device writable. So rx_buf[] and tx_buf[] 
still have to be swapped.

I need to separate struct virtio_spi_transfer_req in my C implementation to

struct virtio_spi_transfer_req_out { /* Device readable */
     struct spi_transfer_head head;
     u8 tx_buf[]; /* Variable array at the end of the structure, gcc is 
happy */
};

struct virtio_spi_transfer_req_in { /* Device writable */
     u8 rx_buf[];
     /* "struct spi_transfer_end result;" is behind rx_buf but variable 
array must be last element for gcc for reasons */
};

Means you have to calculate the address where the result code is to be 
written from some length information. Can be done. But then I should be 
sure about the address. And this I can only be after all the length 
information (head, device writable) are checked for consistency. Clumsy 
and asking for mistakes.

However what also could be done (but you may have to obey alignment 
requirement for rx_buf[] when words are transferred and may have to add 
some padding in "struct spi_transfer_result", I don't know this currently):

struct virtio_spi_transfer_req_in { /* Device writable */
     struct spi_transfer_result result; /* The result code is the 1st 
device writable byte */
     u8 rx_buf[];
};

With this definition there was no need to calculate the address of the 
result byte. Just a thought to make life easier.

> Also I add the member and field definition as follows:
>
>     @slave_id: chipselect index the SPI transfer used.
>
>     @bits_per_word: the number of bits in each SPI transfer word.
>
>     @cs_change: whether to deselect device after finishing this transfer
>         before starting the next transfer, 0 means cs keep asserted and
>         1 means cs deasserted then asserted again.
>
>     @tx_nbits: bus width for write transfer.
>         0,1: bus width is 1, also known as SINGLE
>         2  : bus width is 2, also known as DUAL
>         4  : bus width is 4, also known as QUAD
>         8  : bus width is 8, also known as OCTAL
>         other values are invalid.
>
>     @rx_nbits: bus width for read transfer.
>         0,1: bus width is 1, also known as SINGLE
>         2  : bus width is 2, also known as DUAL
>         4  : bus width is 4, also known as QUAD
>         8  : bus width is 8, also known as OCTAL
>         other values are invalid.
>
>     @reserved: for future use.
>
>     @len: transfer length.
Improve comment!
>
>     @mode: SPI transfer mode.
>         bit 0: CPHA, determines the timing (i.e. phase) of the data
>             bits relative to the clock pulses.For CPHA=0, the
>             "out" side changes the data on the trailing edge of the
>             preceding clock cycle, while the "in" side captures the data
>             on (or shortly after) the leading edge of the clock cycle.
>             For CPHA=1, the "out" side changes the data on the leading
>             edge of the current clock cycle, while the "in" side
>             captures the data on (or shortly after) the trailing edge of
>             the clock cycle.
>         bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a
>             clock which idles at 0, and each cycle consists of a pulse
>             of 1. CPOL=1 is a clock which idles at 1, and each cycle
>             consists of a pulse of 0.
>         bit 2: CS_HIGH, if 1, chip select active high, else active low.
>         bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
>             first, else LSB first.
>         bit 4: LOOP, loopback mode.
>
>     @freq: the transfer speed in Hz.
>
>     @word_delay_ns: delay to be inserted between consecutive words of a
>         transfer, in ns unit.
>     @cs_setup_ns: delay to be introduced after CS is asserted, in ns
>         unit.
>     @cs_delay_hold_ns: delay to be introduced before CS is deasserted
>         for each transfer, in ns unit.
>     @cs_change_delay_inactive_ns: delay to be introduced after CS is
>         deasserted and before next asserted, in ns unit.
>
>
>     Could you please help review the transfer request above again to 
> check if the interface is clear and enough for back-end to perform SPI 
> transfers. Thank you again for your comments. 
I'm working and I'll come back on this.
> Best Regards
> Haixu Cui 

Regards
Harald Mommer



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Haixu Cui]

Hi Harald Mommer,

     Thank you for your comments and please refer to my following replies.

On 8/10/2023 6:40 PM, Harald Mommer wrote:
> Hello,
> 
> a quick incomplete on. I'm currently with both hands in an attempt to 
> implement a virtio SPI device for our proprietary hypervisor based on 
> the draft specification and your E-Mail. Means I see now some more 
> things while implementing.
> 
> u32 len; /* @len: transfer length.*/
> 
> is this a byte or a word count?
> 
> The comment in Linux for the length field in struct spi_ioc_transfer is
> 
> /* @len: Length of tx and rx buffers, in bytes. */ so I assume this here 
> is also a byte count.
> 
> As we have only one len I think it's still needed to look at the device 
> readable and device writable descriptor sizes to decide on half duplex 
> read, half duplex write or full duplex. Having still to do this the 
> transfer length in the len field may be redundant (superfluous) 
> information.

"len" is byte count.

You are right, "len" parameter seems redundant. Virtqueue used to pass 
parameters between front-end and back-end and its descriptor is defined 
as follows(refer to: 
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/listings/virtio_queue.h): 


struct virtq_desc {
         /* Address (guest-physical). */
         le64 addr;
         /* Length. */
         le32 len;
         /* The flags as indicated above. */
         le16 flags;
         /* We chain unused descriptors via this, too */
         le16 next;
};

For the rx_buf or tx_buf descriptor, len member shows the transfer 
length, so it is not necessary to pass "len" in the head structure.
I will remove "len" in next version.


> 
> No problem with this, I'm now only at a point where I want to be sure 
> whether this is meant as a byte length (vs. a word length).
> 
> Then there is no u32. There is le32. Only RPMB uses be32 but they have a 
> special reason to do this. The byte order must be defined for 16 and 32 
> bit values!

Yes! Use "le32" instead of "u32" in next patch.

> 
> On 20.07.23 09:50, Haixu Cui wrote:
>> Hi Harald Mommer,
>>
>>     Thank you so much for all your helpful advice and info!
>>
>>     I have updated the transfer request as follows:
>>
>>     struct spi_transfer_head {
>>           u8 slave_id;
>>           u8 bits_per_word;
>>           u8 cs_change;
>>           u8 tx_nbits;
>>           u8 rx_nbits;
>>           u8 reserved[3];
>>           u32 len;
>>           u32 mode;
>>           u32 freq;
>>           u32 word_delay_ns;
>>           u32 cs_setup_ns;
>>           u32 cs_delay_hold_ns;
>>           u32 cs_change_delay_inactive_ns;
>>     };
>>
>>     struct spi_transfer_end {
>>           u8 result;
>>     };
> May become "struct spi_transfer_result result" for naming reasons with 
> same content, see below. Just a proposal which may not be followed, see 
> below.
>>
>>     struct virtio_spi_transfer_req {
>>         struct spi_transfer_head head;
>>         u8 rx_buf[];
>>         u8 tx_buf[];
>>         struct spi_end end;
> Above this was "struct spi_transfer_end"
>>
>>     };

Agreed! Will change to "struct spi_transfer_result".

>>
> Device readable must go before device writable. So rx_buf[] and tx_buf[] 
> still have to be swapped.

Yes, rx_buf and tx_buf should be swapped.

> 
> I need to separate struct virtio_spi_transfer_req in my C implementation to
> 
> struct virtio_spi_transfer_req_out { /* Device readable */
>      struct spi_transfer_head head;
>      u8 tx_buf[]; /* Variable array at the end of the structure, gcc is 
> happy */
> };
> 
> struct virtio_spi_transfer_req_in { /* Device writable */
>      u8 rx_buf[];
>      /* "struct spi_transfer_end result;" is behind rx_buf but variable 
> array must be last element for gcc for reasons */
> };
> 
> Means you have to calculate the address where the result code is to be 
> written from some length information. Can be done. But then I should be 
> sure about the address. And this I can only be after all the length 
> information (head, device writable) are checked for consistency. Clumsy 
> and asking for mistakes.
> 
> However what also could be done (but you may have to obey alignment 
> requirement for rx_buf[] when words are transferred and may have to add 
> some padding in "struct spi_transfer_result", I don't know this currently):
> 
> struct virtio_spi_transfer_req_in { /* Device writable */
>      struct spi_transfer_result result; /* The result code is the 1st 
> device writable byte */
>      u8 rx_buf[];
> };
> 
> With this definition there was no need to calculate the address of the 
> result byte. Just a thought to make life easier.

As I just mentioned, front-end sends the buffer base address and 
transfer length to the back-end using virtq_desc structure, rather than 
sending the data directly.
And the virtq_desc length is always 16 bytes, so there has nothing to do 
with the sending order.

> 
>> Also I add the member and field definition as follows:
>>
>>     @slave_id: chipselect index the SPI transfer used.
>>
>>     @bits_per_word: the number of bits in each SPI transfer word.
>>
>>     @cs_change: whether to deselect device after finishing this transfer
>>         before starting the next transfer, 0 means cs keep asserted and
>>         1 means cs deasserted then asserted again.
>>
>>     @tx_nbits: bus width for write transfer.
>>         0,1: bus width is 1, also known as SINGLE
>>         2  : bus width is 2, also known as DUAL
>>         4  : bus width is 4, also known as QUAD
>>         8  : bus width is 8, also known as OCTAL
>>         other values are invalid.
>>
>>     @rx_nbits: bus width for read transfer.
>>         0,1: bus width is 1, also known as SINGLE
>>         2  : bus width is 2, also known as DUAL
>>         4  : bus width is 4, also known as QUAD
>>         8  : bus width is 8, also known as OCTAL
>>         other values are invalid.
>>
>>     @reserved: for future use.
>>
>>     @len: transfer length.
> Improve comment!

Actually, it is byte count.
len will be removed.

>>
>>     @mode: SPI transfer mode.
>>         bit 0: CPHA, determines the timing (i.e. phase) of the data
>>             bits relative to the clock pulses.For CPHA=0, the
>>             "out" side changes the data on the trailing edge of the
>>             preceding clock cycle, while the "in" side captures the data
>>             on (or shortly after) the leading edge of the clock cycle.
>>             For CPHA=1, the "out" side changes the data on the leading
>>             edge of the current clock cycle, while the "in" side
>>             captures the data on (or shortly after) the trailing edge of
>>             the clock cycle.
>>         bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a
>>             clock which idles at 0, and each cycle consists of a pulse
>>             of 1. CPOL=1 is a clock which idles at 1, and each cycle
>>             consists of a pulse of 0.
>>         bit 2: CS_HIGH, if 1, chip select active high, else active low.
>>         bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
>>             first, else LSB first.
>>         bit 4: LOOP, loopback mode.
>>
>>     @freq: the transfer speed in Hz.
>>
>>     @word_delay_ns: delay to be inserted between consecutive words of a
>>         transfer, in ns unit.
>>     @cs_setup_ns: delay to be introduced after CS is asserted, in ns
>>         unit.
>>     @cs_delay_hold_ns: delay to be introduced before CS is deasserted
>>         for each transfer, in ns unit.
>>     @cs_change_delay_inactive_ns: delay to be introduced after CS is
>>         deasserted and before next asserted, in ns unit.
>>
>>
>>     Could you please help review the transfer request above again to 
>> check if the interface is clear and enough for back-end to perform SPI 
>> transfers. Thank you again for your comments. 
> I'm working and I'll come back on this.
>> Best Regards
>> Haixu Cui 
> 
> Regards
> Harald Mommer
> 
> 

According to your helpful comments and suggestion, I will submit another 
patch as soon as possible.

Best Regards & Thanks
Haixu Cui

> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Harald Mommer]

Hello,

is there already some virtio SPI Linux driver published to the public to 
have a chance to look at?

Same question for the device side. Is there a qemu device (or something 
for a different virtualization environment like kvmtool) already 
published to the public anywhere?

The spec made the impression that it's in an early stage so I expect 
there is nothing yet but I may be wrong with my assumption.

On 17.04.23 16:03, Haixu Cui wrote:
> virtio-spi is a virtual SPI master and it allows a guset to operate and
> use the physical SPI master controlled by the host.
> ---
>   device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
>   device-types/spi/device-conformance.tex |   7 ++
>   device-types/spi/driver-conformance.tex |   7 ++
>   3 files changed, 169 insertions(+)
>   create mode 100644 device-types/spi/description.tex
>   create mode 100644 device-types/spi/device-conformance.tex
>   create mode 100644 device-types/spi/driver-conformance.tex

Regards
Harald Mommer



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Haixu Cui]

Hello Harald Mommer,
     I've searched but didn't found opensource virtio SPI Linux driver 
or any document/spec about this.

     We implement the virtio SPI prototype, based on the third-party 
non-opensource hypervisor.

Thanks & Best Regards
Haixu Cui

On 7/21/2023 9:50 PM, Harald Mommer wrote:
> Hello,
> 
> is there already some virtio SPI Linux driver published to the public to 
> have a chance to look at?
> 
> Same question for the device side. Is there a qemu device (or something 
> for a different virtualization environment like kvmtool) already 
> published to the public anywhere?
> 
> The spec made the impression that it's in an early stage so I expect 
> there is nothing yet but I may be wrong with my assumption.
> 
> On 17.04.23 16:03, Haixu Cui wrote:
>> virtio-spi is a virtual SPI master and it allows a guset to operate and
>> use the physical SPI master controlled by the host.
>> ---
>>   device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 ++
>>   device-types/spi/driver-conformance.tex |   7 ++
>>   3 files changed, 169 insertions(+)
>>   create mode 100644 device-types/spi/description.tex
>>   create mode 100644 device-types/spi/device-conformance.tex
>>   create mode 100644 device-types/spi/driver-conformance.tex
> 
> Regards
> Harald Mommer
> 
> 

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-dev][PATCH 2/2] virtio-spi: add the device specification

[Haixu Cui]

Hello Harald Mommer,
     I've searched but didn't found opensource virtio SPI Linux driver 
or any document/spec about this.

     We implement the virtio SPI prototype, based on the third-party 
non-opensource hypervisor.

Thanks & Best Regards
Haixu Cui

On 7/21/2023 9:50 PM, Harald Mommer wrote:
> Hello,
> 
> is there already some virtio SPI Linux driver published to the public to 
> have a chance to look at?
> 
> Same question for the device side. Is there a qemu device (or something 
> for a different virtualization environment like kvmtool) already 
> published to the public anywhere?
> 
> The spec made the impression that it's in an early stage so I expect 
> there is nothing yet but I may be wrong with my assumption.
> 
> On 17.04.23 16:03, Haixu Cui wrote:
>> virtio-spi is a virtual SPI master and it allows a guset to operate and
>> use the physical SPI master controlled by the host.
>> ---
>>   device-types/spi/description.tex        | 155 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 ++
>>   device-types/spi/driver-conformance.tex |   7 ++
>>   3 files changed, 169 insertions(+)
>>   create mode 100644 device-types/spi/description.tex
>>   create mode 100644 device-types/spi/device-conformance.tex
>>   create mode 100644 device-types/spi/driver-conformance.tex
> 
> Regards
> Harald Mommer
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] [virtio-dev][PATCH 0/2] virtio-spi: add virtual SPI master

[Haixu Cui]

virtio-spi is a virtual SPI(Serial Peripheral Interface) master and
it allows a guset to operate and use the physical SPI master controlled
by the host.

Patch summary:
patch 1 define the DEVICE ID for virtio-spi
patch 2 add the specification for virtio-spi

Haixu Cui (2):
  virtio-spi: define the DEVICE ID for virtio SPI master
  virtio-spi: add the device specification

 content.tex                             |   2 +
 device-types/spi/description.tex        | 152 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 ++
 device-types/spi/driver-conformance.tex |   7 ++
 4 files changed, 168 insertions(+)
 create mode 100644 device-types/spi/description.tex
 create mode 100644 device-types/spi/device-conformance.tex
 create mode 100644 device-types/spi/driver-conformance.tex

-- 
2.17.1


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-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master

[Haixu Cui]

Define the DEVICE ID of virtio SPI master device as 45.
---
 content.tex | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/content.tex b/content.tex
index d2ab9eb..0a62dce 100644
--- a/content.tex
+++ b/content.tex
@@ -737,6 +737,8 @@ \chapter{Device Types}\label{sec:Device Types}
 \hline
 44         &   ISM device \\
 \hline
+45         &   SPI master \\
+\hline
 \end{tabular}
 
 Some of the devices above are unspecified by this document,
-- 
2.17.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org