[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

[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

[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        | 152 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 ++
 device-types/spi/driver-conformance.tex |   7 ++
 3 files changed, 166 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..62fcb63
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,152 @@
+\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, Virtio SPI device acts as
+the back-end and exists in the host.
+
+\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.
+\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

[Haixu Cui]

Hi @jrreinhart@google.com,
     Thank you very much for your helpful comments.

     I missed the delay and cs_change_delay parameters. I will add both 
of them, although cs_change_delay can not be set from userspace, but can 
be set in kernel space.


     For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are 
defined in structure spi_device:

     @cs_setup: delay to be introduced by the controller after CS is 
asserted.

     @cs_hold: delay to be introduced by the controller before CS is 
deasserted.

     @cs_inactive: delay to be introduced by the controller after CS is
deasserted. If @cs_change_delay is used from @spi_transfer, then the
two delays will be added up.

     They show the SPI controller ability to control the CS 
assertion/deassertion timing and should not be changed for each transfer 
(because thay can be updated by setting structure spi_transfer or 
structure spi_ioc_transfer). I think it better to define these parameter 
in host OS rather than in guest OS since it's the host OS to operate the 
hardware SPI controller directly. Besides, it can also avoid passing the 
same values from guest to host time after time.

     What's your opinion on this topic? Again, thank you very much.

Best Regards
Haixu Cui

On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
> Hi Haixu,
> 
>> +The \field{word_delay} defines how long to wait between words within
>> one SPI transfer,
>> +in ns unit.
> 
> I'm a little surprised to see a word_delay but no frame_delay or
> transfer_delay.
> 
> For example, many SPI peripherals require a delay after CS is asserted,
> but before the first SCLK edge, allowing them to prepare to send data.
> (E.g. an ADC might begin taking a sample.)
> 
> 
> The linux struct spi_transfer has three delay fields:
> 
> * @cs_change_delay: delay between cs deassert and assert when
> *      @cs_change is set and @spi_transfer is not the last in
> *      @spi_message
> * @delay: delay to be introduced after this transfer before
> *    (optionally) changing the chipselect status, then starting the
> *    next transfer or completing this @spi_message.
> * @word_delay: inter word delay to be introduced after each word size
> *    (set by bits_per_word) transmission.
> 
> The userspace spidev.h API has only two:
> 
> * @delay_usecs: If nonzero, how long to delay after the last bit
> *       transfer before optionally deselecting the device before the
> *       next transfer.
> * @word_delay_usecs: If nonzero, how long to wait between words within
> *       one transfer. This property needs explicit support in the SPI
> *       controller, otherwise it is silently ignored.
> 
> The NXP i.MX RT500 SPI (master) controller, for example, has four
> configurable delays:
> 
> - PRE_DELAY: After CS assertion, before first SCLK edge.
> - POST_DELAY: After a transfer, before CS deassertion.
> - FRAME_DELAY: Between frames (which are arbitrarily defined by
>    software).
> - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
> 
> The NVIDIA Tegra114 SPI controller has:
> 
> - nvidia,cs-setup-clk-count: CS setup timing parameter.
> - nvidia,cs-hold-clk-count: CS hold timing parameter.
> 
> 
> I understand that accurately representing all possible delays might be
> difficult or futile, but I'm curious to understand why, of all the
> possible delays, inter-word delay was chosen for inclusion.
> 
> In a microcontroller, delays around CS (de)assertion can be customized
> by using a GPIO -- rather than the hardware SPI block -- to drive the CS
> signal. This way, delays can be inserted where needed. To do so with a
> virtualized SPI controller might prove difficult however, as the virtual
> channel carrying a CS GPIO signal might not be synchronized to the
> channel carrying the SPI data.
> 
> Curious to hear your thoughts.
> 
> Thanks,
> Jonathon Reinhart

---------------------------------------------------------------------
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

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

[Jonathon Reinhart]

> Hi @jrreinhart@google.com,
>      Thank you very much for your helpful comments.
>
>      I missed the delay and cs_change_delay parameters. I will add both
> of them, although cs_change_delay can not be set from userspace, but can
> be set in kernel space.
>
>
>      For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are
> defined in structure spi_device:
>
>      @cs_setup: delay to be introduced by the controller after CS is
> asserted.
>
>      @cs_hold: delay to be introduced by the controller before CS is
> deasserted.
>
>      @cs_inactive: delay to be introduced by the controller after CS is
> deasserted. If @cs_change_delay is used from @spi_transfer, then the
> two delays will be added up.
>
>      They show the SPI controller ability to control the CS
> assertion/deassertion timing and should not be changed for each transfer
> (because thay can be updated by setting structure spi_transfer or
> structure spi_ioc_transfer). I think it better to define these parameter
> in host OS rather than in guest OS since it's the host OS to operate the
> hardware SPI controller directly. Besides, it can also avoid passing the
> same values from guest to host time after time.
>
>      What's your opinion on this topic? Again, thank you very much.

Hi Haixu,

I took another look at the Linux structures and attempted to map up the
delay parameters to the various points in a SPI sequence. Please correct
me if I'm wrong about any of this.

Consider this diagram where CS# is an active-low chip select, and SCLK
is the serial clock:

      ___.                                                   ._____.
  CS#    |___________________________________________________|     |___
         .                                                   .     .
         .                                                   .     .
 SCLK ________NNNN___NNNN___NNNN_______NNNN___NNNN___NNNN______________
         .   .    . .           .     .                  .   .     .
Delays:  +-A-+    +B+           +--C--+                  +-D-+--E--+
         .   .    . .           .     .                  .   .     .
         0   1    2 3           4     5                  6   7     8
         .   .    .             .     .                  .   .
Terms:   .   +Word+             .     .                  .   .
         .   .                  .     .                  .   .
         .   +-------Frame------+     +-------Frame------+   .
         .                                                   .
         +-----------------Transfer--------------------------+

Using NXP terminology:

  From 1 to 2 is a "word"     (with e.g. 8 bits).
  From 1 to 4 is a "frame"    (with 3 words).
  From 1 to 6 is a "transfer" (with 3 frames).

Linux does not have the concept of a frame, only a series of transfers
(a spi_message), each of which can specify whether CS changes or not.

I've identified these various timing points and attempted to match them
to the Linux spi_device and spi_transfer parameters:

A - CS Setup: Delay after CS asserted before clock starts.
    spi_device.cs_setup

B - Word Delay: Delay between words.
    spi_transfer.word_delay

C - Frame Delay: Delay between frames.
    (Linux does not have the concept of a SPI "frame".)

D - CS Hold: Delay after clock stops, and before CS deasserted.
    spi_device.cs_hold (+ spi_transfer.delay ??)

E - CS Inactive: Delay after CS deasserted, before asserting again.
    spi_device.cs_inactive + spi_transfer.cs_change_delay


While I agree with you that some of these timings are unlikely to change
from transfer-to-transfer, the subset of which should be specified
at the device level vs. per-transfer seems somewhat arbitrary. As you
can see there is some overlap between them.

If I understand correctly, it appears that the CS hold time *can* be
controlled on a per-transfer bases, using spi_transfer.delay
("after this transfer before changing the chipselect status").

That leaves CS setup as the only parameter that cannot be influenced on
a per-transfer basis.

Theoretically, one might require different CS setup/hold times,
depending on which slave_id they are talking to (on the same bus).
In that case, one must set those spi_device parameters to the worst-case
(maximum) values. However, this is already a Linux limitation, so I'm
not sure it needs to be improved here.

I think you've achieved parity with what the Linux kernel API allows,
and that's probably good enough. As you said, anything else can be
adjusted in the host OS -- I'm not sure how the guest would go about
achieving that, though. You're not proposing the use of configuration
space for virtio-spi, are you?

Regards,
Jonathon Reinhart


>
> Best Regards
> Haixu Cui
>
> On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
> > Hi Haixu,
> >
> >> +The \field{word_delay} defines how long to wait between words within
> >> one SPI transfer,
> >> +in ns unit.
> >
> > I'm a little surprised to see a word_delay but no frame_delay or
> > transfer_delay.
> >
> > For example, many SPI peripherals require a delay after CS is asserted,
> > but before the first SCLK edge, allowing them to prepare to send data.
> > (E.g. an ADC might begin taking a sample.)
> >
> >
> > The linux struct spi_transfer has three delay fields:
> >
> > * @cs_change_delay: delay between cs deassert and assert when
> > *      @cs_change is set and @spi_transfer is not the last in
> > *      @spi_message
> > * @delay: delay to be introduced after this transfer before
> > *    (optionally) changing the chipselect status, then starting the
> > *    next transfer or completing this @spi_message.
> > * @word_delay: inter word delay to be introduced after each word size
> > *    (set by bits_per_word) transmission.
> >
> > The userspace spidev.h API has only two:
> >
> > * @delay_usecs: If nonzero, how long to delay after the last bit
> > *       transfer before optionally deselecting the device before the
> > *       next transfer.
> > * @word_delay_usecs: If nonzero, how long to wait between words within
> > *       one transfer. This property needs explicit support in the SPI
> > *       controller, otherwise it is silently ignored.
> >
> > The NXP i.MX RT500 SPI (master) controller, for example, has four
> > configurable delays:
> >
> > - PRE_DELAY: After CS assertion, before first SCLK edge.
> > - POST_DELAY: After a transfer, before CS deassertion.
> > - FRAME_DELAY: Between frames (which are arbitrarily defined by
> >    software).
> > - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
> >
> > The NVIDIA Tegra114 SPI controller has:
> >
> > - nvidia,cs-setup-clk-count: CS setup timing parameter.
> > - nvidia,cs-hold-clk-count: CS hold timing parameter.
> >
> >
> > I understand that accurately representing all possible delays might be
> > difficult or futile, but I'm curious to understand why, of all the
> > possible delays, inter-word delay was chosen for inclusion.
> >
> > In a microcontroller, delays around CS (de)assertion can be customized
> > by using a GPIO -- rather than the hardware SPI block -- to drive the CS
> > signal. This way, delays can be inserted where needed. To do so with a
> > virtualized SPI controller might prove difficult however, as the virtual
> > channel carrying a CS GPIO signal might not be synchronized to the
> > channel carrying the SPI data.
> >
> > Curious to hear your thoughts.
> >
> > Thanks,
> > Jonathon Reinhart

---------------------------------------------------------------------
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 Jonathon Reinhart,

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

     I took a look at latest Linux SPI driver and found, for 
cs_setup/cs_hold/cs_inactive set in device-tree, there are following two 
conditions:
     1) if SPI controller supports CS timing configuration and CS is not 
using a GPIO line, then the SPI driver set the 
cs_setup/cs_hold/ca_inactive values into the corresponding SPI CS registers;
     2) if CS is using a GPIO line, or SPI controller doesn't support CS 
timing configuration, it is the software to perform the 
cs_setup/cs_hold/cs_inactive delays, which is implemented in function 
spi_set_cs in driver/spi/spi.c.

     So I agree it is necessary to pass cs_setup/cs_hold/cs_inactive to 
the host for each transfer.

     For condition 1, host should set these values into the CS timing 
registers. And as you mentioned "one might require different CS 
setup/hold times, depending on which slave_id they are talking to (on 
the same bus)", if so, host need to overwrite the CS timing registers 
between the two transfers talking to different salve_id.

     For condition 2, SPI driver running in the host performing the 
cs_setup/cs_hold/cs_inactive delays seems more accurate, compared with 
performing delays in guest.


     And the latest virtio spi transfer structure is defined 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 mode;
         u32 freq;
         u32 word_delay_ns;
         u32 delay_ns;
         u32 cs_change_delay_ns;
         u32 cs_setup_ns;
         u32 cs_hold_ns;
         u32 cs_inactive_ns;
      };

      @slave_id: chipselect index the SPI transfer used
      @bits_per_word: the number of bits in each SPI transfer word
      @cs_change: True to deselect device before starting the next transfer
      @tx_nbits: number of bits used for writing
      @rx_nbits: number of bits used for reading
      @reserved: reserved for future use
      @mode: the spi mode defines how data is clocked out and in
      @freq: transfer speed
      @word_delay_ns: delay to be inserted between consecutive words of 
a transfer, in ns unit
      @delay_ns: delay to be introduced after this transfer before 
(optionally) changing the chipselect status, in ns unit
      @cs_change_delay_ns: delay between cs deassert and assert when 
cs_change is set, in ns unit
      @cs_setup_ns: delay to be introduced by the controller after CS is 
asserted, in ns unit
      @cs_hold_ns: delay to be introduced by the controller before CS is 
deasserted, in ns unit
      @cs_inactive_ns: delay to be introduced by the controller after CS 
is deasserted, in ns unit


     Could you please help check if this new structure contains enough 
information. Really appreciate for kind help.

Best Regards
Haixu Cui

On 7/1/2023 5:59 AM, Jonathon Reinhart wrote:
>> Hi @jrreinhart@google.com,
>>       Thank you very much for your helpful comments.
>>
>>       I missed the delay and cs_change_delay parameters. I will add both
>> of them, although cs_change_delay can not be set from userspace, but can
>> be set in kernel space.
>>
>>
>>       For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are
>> defined in structure spi_device:
>>
>>       @cs_setup: delay to be introduced by the controller after CS is
>> asserted.
>>
>>       @cs_hold: delay to be introduced by the controller before CS is
>> deasserted.
>>
>>       @cs_inactive: delay to be introduced by the controller after CS is
>> deasserted. If @cs_change_delay is used from @spi_transfer, then the
>> two delays will be added up.
>>
>>       They show the SPI controller ability to control the CS
>> assertion/deassertion timing and should not be changed for each transfer
>> (because thay can be updated by setting structure spi_transfer or
>> structure spi_ioc_transfer). I think it better to define these parameter
>> in host OS rather than in guest OS since it's the host OS to operate the
>> hardware SPI controller directly. Besides, it can also avoid passing the
>> same values from guest to host time after time.
>>
>>       What's your opinion on this topic? Again, thank you very much.
> 
> Hi Haixu,
> 
> I took another look at the Linux structures and attempted to map up the
> delay parameters to the various points in a SPI sequence. Please correct
> me if I'm wrong about any of this.
> 
> Consider this diagram where CS# is an active-low chip select, and SCLK
> is the serial clock:
> 
>        ___.                                                   ._____.
>    CS#    |___________________________________________________|     |___
>           .                                                   .     .
>           .                                                   .     .
>   SCLK ________NNNN___NNNN___NNNN_______NNNN___NNNN___NNNN______________
>           .   .    . .           .     .                  .   .     .
> Delays:  +-A-+    +B+           +--C--+                  +-D-+--E--+
>           .   .    . .           .     .                  .   .     .
>           0   1    2 3           4     5                  6   7     8
>           .   .    .             .     .                  .   .
> Terms:   .   +Word+             .     .                  .   .
>           .   .                  .     .                  .   .
>           .   +-------Frame------+     +-------Frame------+   .
>           .                                                   .
>           +-----------------Transfer--------------------------+
> 
> Using NXP terminology:
> 
>    From 1 to 2 is a "word"     (with e.g. 8 bits).
>    From 1 to 4 is a "frame"    (with 3 words).
>    From 1 to 6 is a "transfer" (with 3 frames).
> 
> Linux does not have the concept of a frame, only a series of transfers
> (a spi_message), each of which can specify whether CS changes or not.
> 
> I've identified these various timing points and attempted to match them
> to the Linux spi_device and spi_transfer parameters:
> 
> A - CS Setup: Delay after CS asserted before clock starts.
>      spi_device.cs_setup
> 
> B - Word Delay: Delay between words.
>      spi_transfer.word_delay
> 
> C - Frame Delay: Delay between frames.
>      (Linux does not have the concept of a SPI "frame".)
> 
> D - CS Hold: Delay after clock stops, and before CS deasserted.
>      spi_device.cs_hold (+ spi_transfer.delay ??)
> 
> E - CS Inactive: Delay after CS deasserted, before asserting again.
>      spi_device.cs_inactive + spi_transfer.cs_change_delay
> 
> 
> While I agree with you that some of these timings are unlikely to change
> from transfer-to-transfer, the subset of which should be specified
> at the device level vs. per-transfer seems somewhat arbitrary. As you
> can see there is some overlap between them.
> 
> If I understand correctly, it appears that the CS hold time *can* be
> controlled on a per-transfer bases, using spi_transfer.delay
> ("after this transfer before changing the chipselect status").
> 
> That leaves CS setup as the only parameter that cannot be influenced on
> a per-transfer basis.
> 
> Theoretically, one might require different CS setup/hold times,
> depending on which slave_id they are talking to (on the same bus).
> In that case, one must set those spi_device parameters to the worst-case
> (maximum) values. However, this is already a Linux limitation, so I'm
> not sure it needs to be improved here.
> 
> I think you've achieved parity with what the Linux kernel API allows,
> and that's probably good enough. As you said, anything else can be
> adjusted in the host OS -- I'm not sure how the guest would go about
> achieving that, though. You're not proposing the use of configuration
> space for virtio-spi, are you?
> 
> Regards,
> Jonathon Reinhart
> 
> 
>>
>> Best Regards
>> Haixu Cui
>>
>> On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
>>> Hi Haixu,
>>>
>>>> +The \field{word_delay} defines how long to wait between words within
>>>> one SPI transfer,
>>>> +in ns unit.
>>>
>>> I'm a little surprised to see a word_delay but no frame_delay or
>>> transfer_delay.
>>>
>>> For example, many SPI peripherals require a delay after CS is asserted,
>>> but before the first SCLK edge, allowing them to prepare to send data.
>>> (E.g. an ADC might begin taking a sample.)
>>>
>>>
>>> The linux struct spi_transfer has three delay fields:
>>>
>>> * @cs_change_delay: delay between cs deassert and assert when
>>> *      @cs_change is set and @spi_transfer is not the last in
>>> *      @spi_message
>>> * @delay: delay to be introduced after this transfer before
>>> *    (optionally) changing the chipselect status, then starting the
>>> *    next transfer or completing this @spi_message.
>>> * @word_delay: inter word delay to be introduced after each word size
>>> *    (set by bits_per_word) transmission.
>>>
>>> The userspace spidev.h API has only two:
>>>
>>> * @delay_usecs: If nonzero, how long to delay after the last bit
>>> *       transfer before optionally deselecting the device before the
>>> *       next transfer.
>>> * @word_delay_usecs: If nonzero, how long to wait between words within
>>> *       one transfer. This property needs explicit support in the SPI
>>> *       controller, otherwise it is silently ignored.
>>>
>>> The NXP i.MX RT500 SPI (master) controller, for example, has four
>>> configurable delays:
>>>
>>> - PRE_DELAY: After CS assertion, before first SCLK edge.
>>> - POST_DELAY: After a transfer, before CS deassertion.
>>> - FRAME_DELAY: Between frames (which are arbitrarily defined by
>>>     software).
>>> - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
>>>
>>> The NVIDIA Tegra114 SPI controller has:
>>>
>>> - nvidia,cs-setup-clk-count: CS setup timing parameter.
>>> - nvidia,cs-hold-clk-count: CS hold timing parameter.
>>>
>>>
>>> I understand that accurately representing all possible delays might be
>>> difficult or futile, but I'm curious to understand why, of all the
>>> possible delays, inter-word delay was chosen for inclusion.
>>>
>>> In a microcontroller, delays around CS (de)assertion can be customized
>>> by using a GPIO -- rather than the hardware SPI block -- to drive the CS
>>> signal. This way, delays can be inserted where needed. To do so with a
>>> virtualized SPI controller might prove difficult however, as the virtual
>>> channel carrying a CS GPIO signal might not be synchronized to the
>>> channel carrying the SPI data.
>>>
>>> Curious to hear your thoughts.
>>>
>>> Thanks,
>>> Jonathon Reinhart

---------------------------------------------------------------------
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

[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

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

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

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

[Jonathon Reinhart]

On Fri, Jul 7, 2023 at 3:21 AM Haixu Cui <quic_haixcui@quicinc.com> wrote:
>
> Hi Jonathon Reinhart,
>
>      Thank you so much for all your helpful advice and info!
>
>      I took a look at latest Linux SPI driver and found, for
> cs_setup/cs_hold/cs_inactive set in device-tree, there are following two
> conditions:
>      1) if SPI controller supports CS timing configuration and CS is not
> using a GPIO line, then the SPI driver set the
> cs_setup/cs_hold/ca_inactive values into the corresponding SPI CS registers;
>      2) if CS is using a GPIO line, or SPI controller doesn't support CS
> timing configuration, it is the software to perform the
> cs_setup/cs_hold/cs_inactive delays, which is implemented in function
> spi_set_cs in driver/spi/spi.c.
>
>      So I agree it is necessary to pass cs_setup/cs_hold/cs_inactive to
> the host for each transfer.
>
>      For condition 1, host should set these values into the CS timing
> registers. And as you mentioned "one might require different CS
> setup/hold times, depending on which slave_id they are talking to (on
> the same bus)", if so, host need to overwrite the CS timing registers
> between the two transfers talking to different salve_id.
>
>      For condition 2, SPI driver running in the host performing the
> cs_setup/cs_hold/cs_inactive delays seems more accurate, compared with
> performing delays in guest.
>
>
>      And the latest virtio spi transfer structure is defined 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 mode;
>          u32 freq;
>          u32 word_delay_ns;
>          u32 delay_ns;
>          u32 cs_change_delay_ns;
>          u32 cs_setup_ns;
>          u32 cs_hold_ns;
>          u32 cs_inactive_ns;
>       };
>

Hello Haixu Cui,

I think there may be some unnecessary redundancy in these fields. See below.

>       @slave_id: chipselect index the SPI transfer used
>       @bits_per_word: the number of bits in each SPI transfer word
>       @cs_change: True to deselect device before starting the next transfer
>       @tx_nbits: number of bits used for writing
>       @rx_nbits: number of bits used for reading
>       @reserved: reserved for future use
>       @mode: the spi mode defines how data is clocked out and in
>       @freq: transfer speed
>       @word_delay_ns: delay to be inserted between consecutive words of
> a transfer, in ns unit
>       @cs_setup_ns: delay to be introduced by the controller after CS is
> asserted, in ns unit

(I am reordering these in my quoted text to be grouped appropriately.)

>       @delay_ns: delay to be introduced after this transfer before
> (optionally) changing the chipselect status, in ns unit
>       @cs_hold_ns: delay to be introduced by the controller before CS is
> deasserted, in ns unit

Aren't @delay_ns and @cs_hold_ns specifying the same thing?

Linux spi_transfer defines @delay as:
    delay to be introduced after this transfer before
    (optionally) changing the chipselect status, then starting the
    next transfer or completing this spi_message.

...and spi_device @cs_hold as: delay to be introduced by the
    controller before CS is deasserted.

(This is the period labeled "D" in my diagram.)

I guess the only difference is if you have a series of transfers, where
only the last transfer has @cs_change=true, and all preceding messages
have @cs_change=false. In this case, @delay_ns would apply between each
transfer, and after the last transfer @delay_ns + @cs_hold would apply
before CS is deasserted:

    Transfer[0] (cs_change=false)
    delay(@delay_ns)
    Transfer[1] (cs_change=false)
    delay(@delay_ns)
    Transfer[3] (cs_change=true)
    delay(@delay_ns) + delay(@cs_hold_ns)
    CS deasserted


>       @cs_change_delay_ns: delay between cs deassert and assert when
> cs_change is set, in ns unit
>       @cs_inactive_ns: delay to be introduced by the controller after CS
> is deasserted, in ns unit

Similarly, aren't @cs_change_delay_ns and @cs_inactive_ns applied at the
same point in the cycle?

(This is the period labeled "E" in my diagram.)

Linux spi_device defines @cs_inactive as: delay to be introduced by the
    controller after CS is deasserted. If @cs_change_delay is used from
    @spi_transfer, then the two delays will be added up.

...and spi_transfer defines @cs_change_delay as: delay between cs
    deassert and assert when @cs_change is set and @spi_transfer is not
    the last in @spi_message.

It even says they will be added up.

The only difference seems to be that @cs_change_delay applies only when
the transfer is not the last in a list (spi_message, which we don't have
here.)

    CS asserted
    Transfer[0] (cs_change=true)
    CS deasserted
    delay(@cs_inactive) + delay(@cs_change_delay)
    CS asserted
    Transfer[1] (cs_change=true)
    CS deasserted
    delay(@cs_inactive)


In both cases of redundancy above, the difference in semantics between
the spi_device and spi_transfer only seem to apply when a series of
transfers (spi_message or SPI_IOC_MESSAGE) are involved. Since (AFAICT)
you aren't proposing that construct here, maybe the separate fields are
not necessary? IOW, maybe @delay_ns and @cs_change_delay can be
eliminated?

Please let me know if I've misunderstood anything.

Jonathon

>
>      Could you please help check if this new structure contains enough
> information. Really appreciate for kind help.
>
> Best Regards
> Haixu Cui
>
> On 7/1/2023 5:59 AM, Jonathon Reinhart wrote:
> >> Hi @jrreinhart@google.com,
> >>       Thank you very much for your helpful comments.
> >>
> >>       I missed the delay and cs_change_delay parameters. I will add both
> >> of them, although cs_change_delay can not be set from userspace, but can
> >> be set in kernel space.
> >>
> >>
> >>       For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are
> >> defined in structure spi_device:
> >>
> >>       @cs_setup: delay to be introduced by the controller after CS is
> >> asserted.
> >>
> >>       @cs_hold: delay to be introduced by the controller before CS is
> >> deasserted.
> >>
> >>       @cs_inactive: delay to be introduced by the controller after CS is
> >> deasserted. If @cs_change_delay is used from @spi_transfer, then the
> >> two delays will be added up.
> >>
> >>       They show the SPI controller ability to control the CS
> >> assertion/deassertion timing and should not be changed for each transfer
> >> (because thay can be updated by setting structure spi_transfer or
> >> structure spi_ioc_transfer). I think it better to define these parameter
> >> in host OS rather than in guest OS since it's the host OS to operate the
> >> hardware SPI controller directly. Besides, it can also avoid passing the
> >> same values from guest to host time after time.
> >>
> >>       What's your opinion on this topic? Again, thank you very much.
> >
> > Hi Haixu,
> >
> > I took another look at the Linux structures and attempted to map up the
> > delay parameters to the various points in a SPI sequence. Please correct
> > me if I'm wrong about any of this.
> >
> > Consider this diagram where CS# is an active-low chip select, and SCLK
> > is the serial clock:
> >
> >        ___.                                                   ._____.
> >    CS#    |___________________________________________________|     |___
> >           .                                                   .     .
> >           .                                                   .     .
> >   SCLK ________NNNN___NNNN___NNNN_______NNNN___NNNN___NNNN______________
> >           .   .    . .           .     .                  .   .     .
> > Delays:  +-A-+    +B+           +--C--+                  +-D-+--E--+
> >           .   .    . .           .     .                  .   .     .
> >           0   1    2 3           4     5                  6   7     8
> >           .   .    .             .     .                  .   .
> > Terms:   .   +Word+             .     .                  .   .
> >           .   .                  .     .                  .   .
> >           .   +-------Frame------+     +-------Frame------+   .
> >           .                                                   .
> >           +-----------------Transfer--------------------------+
> >
> > Using NXP terminology:
> >
> >    From 1 to 2 is a "word"     (with e.g. 8 bits).
> >    From 1 to 4 is a "frame"    (with 3 words).
> >    From 1 to 6 is a "transfer" (with 3 frames).
> >
> > Linux does not have the concept of a frame, only a series of transfers
> > (a spi_message), each of which can specify whether CS changes or not.
> >
> > I've identified these various timing points and attempted to match them
> > to the Linux spi_device and spi_transfer parameters:
> >
> > A - CS Setup: Delay after CS asserted before clock starts.
> >      spi_device.cs_setup
> >
> > B - Word Delay: Delay between words.
> >      spi_transfer.word_delay
> >
> > C - Frame Delay: Delay between frames.
> >      (Linux does not have the concept of a SPI "frame".)
> >
> > D - CS Hold: Delay after clock stops, and before CS deasserted.
> >      spi_device.cs_hold (+ spi_transfer.delay ??)
> >
> > E - CS Inactive: Delay after CS deasserted, before asserting again.
> >      spi_device.cs_inactive + spi_transfer.cs_change_delay
> >
> >
> > While I agree with you that some of these timings are unlikely to change
> > from transfer-to-transfer, the subset of which should be specified
> > at the device level vs. per-transfer seems somewhat arbitrary. As you
> > can see there is some overlap between them.
> >
> > If I understand correctly, it appears that the CS hold time *can* be
> > controlled on a per-transfer bases, using spi_transfer.delay
> > ("after this transfer before changing the chipselect status").
> >
> > That leaves CS setup as the only parameter that cannot be influenced on
> > a per-transfer basis.
> >
> > Theoretically, one might require different CS setup/hold times,
> > depending on which slave_id they are talking to (on the same bus).
> > In that case, one must set those spi_device parameters to the worst-case
> > (maximum) values. However, this is already a Linux limitation, so I'm
> > not sure it needs to be improved here.
> >
> > I think you've achieved parity with what the Linux kernel API allows,
> > and that's probably good enough. As you said, anything else can be
> > adjusted in the host OS -- I'm not sure how the guest would go about
> > achieving that, though. You're not proposing the use of configuration
> > space for virtio-spi, are you?
> >
> > Regards,
> > Jonathon Reinhart
> >
> >
> >>
> >> Best Regards
> >> Haixu Cui
> >>
> >> On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
> >>> Hi Haixu,
> >>>
> >>>> +The \field{word_delay} defines how long to wait between words within
> >>>> one SPI transfer,
> >>>> +in ns unit.
> >>>
> >>> I'm a little surprised to see a word_delay but no frame_delay or
> >>> transfer_delay.
> >>>
> >>> For example, many SPI peripherals require a delay after CS is asserted,
> >>> but before the first SCLK edge, allowing them to prepare to send data.
> >>> (E.g. an ADC might begin taking a sample.)
> >>>
> >>>
> >>> The linux struct spi_transfer has three delay fields:
> >>>
> >>> * @cs_change_delay: delay between cs deassert and assert when
> >>> *      @cs_change is set and @spi_transfer is not the last in
> >>> *      @spi_message
> >>> * @delay: delay to be introduced after this transfer before
> >>> *    (optionally) changing the chipselect status, then starting the
> >>> *    next transfer or completing this @spi_message.
> >>> * @word_delay: inter word delay to be introduced after each word size
> >>> *    (set by bits_per_word) transmission.
> >>>
> >>> The userspace spidev.h API has only two:
> >>>
> >>> * @delay_usecs: If nonzero, how long to delay after the last bit
> >>> *       transfer before optionally deselecting the device before the
> >>> *       next transfer.
> >>> * @word_delay_usecs: If nonzero, how long to wait between words within
> >>> *       one transfer. This property needs explicit support in the SPI
> >>> *       controller, otherwise it is silently ignored.
> >>>
> >>> The NXP i.MX RT500 SPI (master) controller, for example, has four
> >>> configurable delays:
> >>>
> >>> - PRE_DELAY: After CS assertion, before first SCLK edge.
> >>> - POST_DELAY: After a transfer, before CS deassertion.
> >>> - FRAME_DELAY: Between frames (which are arbitrarily defined by
> >>>     software).
> >>> - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
> >>>
> >>> The NVIDIA Tegra114 SPI controller has:
> >>>
> >>> - nvidia,cs-setup-clk-count: CS setup timing parameter.
> >>> - nvidia,cs-hold-clk-count: CS hold timing parameter.
> >>>
> >>>
> >>> I understand that accurately representing all possible delays might be
> >>> difficult or futile, but I'm curious to understand why, of all the
> >>> possible delays, inter-word delay was chosen for inclusion.
> >>>
> >>> In a microcontroller, delays around CS (de)assertion can be customized
> >>> by using a GPIO -- rather than the hardware SPI block -- to drive the CS
> >>> signal. This way, delays can be inserted where needed. To do so with a
> >>> virtualized SPI controller might prove difficult however, as the virtual
> >>> channel carrying a CS GPIO signal might not be synchronized to the
> >>> channel carrying the SPI data.
> >>>
> >>> Curious to hear your thoughts.
> >>>
> >>> Thanks,
> >>> Jonathon Reinhart

---------------------------------------------------------------------
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 Jonathon Reinhart,
     Thank you very much for your comments.

On 7/11/2023 12:42 AM, Jonathon Reinhart wrote:
> On Fri, Jul 7, 2023 at 3:21 AM Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>
>> Hi Jonathon Reinhart,
>>
>>       Thank you so much for all your helpful advice and info!
>>
>>       I took a look at latest Linux SPI driver and found, for
>> cs_setup/cs_hold/cs_inactive set in device-tree, there are following two
>> conditions:
>>       1) if SPI controller supports CS timing configuration and CS is not
>> using a GPIO line, then the SPI driver set the
>> cs_setup/cs_hold/ca_inactive values into the corresponding SPI CS registers;
>>       2) if CS is using a GPIO line, or SPI controller doesn't support CS
>> timing configuration, it is the software to perform the
>> cs_setup/cs_hold/cs_inactive delays, which is implemented in function
>> spi_set_cs in driver/spi/spi.c.
>>
>>       So I agree it is necessary to pass cs_setup/cs_hold/cs_inactive to
>> the host for each transfer.
>>
>>       For condition 1, host should set these values into the CS timing
>> registers. And as you mentioned "one might require different CS
>> setup/hold times, depending on which slave_id they are talking to (on
>> the same bus)", if so, host need to overwrite the CS timing registers
>> between the two transfers talking to different salve_id.
>>
>>       For condition 2, SPI driver running in the host performing the
>> cs_setup/cs_hold/cs_inactive delays seems more accurate, compared with
>> performing delays in guest.
>>
>>
>>       And the latest virtio spi transfer structure is defined 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 mode;
>>           u32 freq;
>>           u32 word_delay_ns;
>>           u32 delay_ns;
>>           u32 cs_change_delay_ns;
>>           u32 cs_setup_ns;
>>           u32 cs_hold_ns;
>>           u32 cs_inactive_ns;
>>        };
>>
> 
> Hello Haixu Cui,
> 
> I think there may be some unnecessary redundancy in these fields. See below.
> 
>>        @slave_id: chipselect index the SPI transfer used
>>        @bits_per_word: the number of bits in each SPI transfer word
>>        @cs_change: True to deselect device before starting the next transfer
>>        @tx_nbits: number of bits used for writing
>>        @rx_nbits: number of bits used for reading
>>        @reserved: reserved for future use
>>        @mode: the spi mode defines how data is clocked out and in
>>        @freq: transfer speed
>>        @word_delay_ns: delay to be inserted between consecutive words of
>> a transfer, in ns unit
>>        @cs_setup_ns: delay to be introduced by the controller after CS is
>> asserted, in ns unit
> 
> (I am reordering these in my quoted text to be grouped appropriately.)
> 
>>        @delay_ns: delay to be introduced after this transfer before
>> (optionally) changing the chipselect status, in ns unit
>>        @cs_hold_ns: delay to be introduced by the controller before CS is
>> deasserted, in ns unit
> 
> Aren't @delay_ns and @cs_hold_ns specifying the same thing?

I think they are not the same thing, delay_ns is the spi transfer 
property while cs_hold_ns is the spi device property， although they take 
effect at the same stage.

I list 4 conditions, here transfer cs delay including 
spi_transfer->delay and spi_transfer->cs_change_delay, and device cs 
delay including spi_device->cs_hold, spi_device->cs_setup and 
spi_device->cs_inactive.

condition 1:
     virtio-spi controller only has transfer_one interface but no 
transfer_one_message interface, and spi_transfer_one_message is the 
default transfer_one_message interface of virtio-spi controller.
     Hardware SPI controller doesn't support CS timing configuration.

     In this case, both transfer cs delay and device cs delay are 
executed by software in spi_transfer_one_message function in guest Linux 
OS. So guest doesn't need pass these cs timing parameters to host.

condition 2:
     virtio-spi controller only has transfer_one interface but no 
transfer_one_message interface, and spi_transfer_one_message is the 
default transfer_one_message interface of virtio-spi controller.
     Hardware SPI controller supports device CS timing configuration, 
which means it has registers to hold these configuration values.

     In this case, transfer cs delay is executed in 
spi_transfer_one_message function, and guest need pass device cs delay 
to host. Because one SPI number may have more than one slave, these 
slaves also may have different device cs delay values, so for each 
transfer, guest should pass device cs delay along with the slave_id to 
host to overwrite the corresponding registers.

condition 3:
     virtio-spi controller has transfer_one_message interface but no 
transfer_one interface.
     Hardware SPI controller doesn't support CS timing configuration.

     In this case, host SPI driver should implement all cs delay 
operations with the transfer cs delay and device cs delay received from 
the guest.

condition 4:
     virtio-spi controller has transfer_one_message interface but no 
transfer_one interface.
     Hardware SPI controller supports CS timing configuration.

     In this case, guest pass transfer cs delay and device cs delay to 
guest, and guest SPI driver should implement logic to execute transfer 
cs delay then overwrite the corresponding register with the device cs 
delay values.

Except for condition 1, in other three conditions, guest should pass 
both transfer cs delay and device cs delay to host.

Best Regards
Haixu Cui

> 
> Linux spi_transfer defines @delay as:
>      delay to be introduced after this transfer before
>      (optionally) changing the chipselect status, then starting the
>      next transfer or completing this spi_message.
> 
> ...and spi_device @cs_hold as: delay to be introduced by the
>      controller before CS is deasserted.
> 
> (This is the period labeled "D" in my diagram.)
> 
> I guess the only difference is if you have a series of transfers, where
> only the last transfer has @cs_change=true, and all preceding messages
> have @cs_change=false. In this case, @delay_ns would apply between each
> transfer, and after the last transfer @delay_ns + @cs_hold would apply
> before CS is deasserted:
> 
>      Transfer[0] (cs_change=false)
>      delay(@delay_ns)
>      Transfer[1] (cs_change=false)
>      delay(@delay_ns)
>      Transfer[3] (cs_change=true)
>      delay(@delay_ns) + delay(@cs_hold_ns)
>      CS deasserted
> 
> 
>>        @cs_change_delay_ns: delay between cs deassert and assert when
>> cs_change is set, in ns unit
>>        @cs_inactive_ns: delay to be introduced by the controller after CS
>> is deasserted, in ns unit
> 
> Similarly, aren't @cs_change_delay_ns and @cs_inactive_ns applied at the
> same point in the cycle?
> 
> (This is the period labeled "E" in my diagram.)
> 
> Linux spi_device defines @cs_inactive as: delay to be introduced by the
>      controller after CS is deasserted. If @cs_change_delay is used from
>      @spi_transfer, then the two delays will be added up.
> 
> ...and spi_transfer defines @cs_change_delay as: delay between cs
>      deassert and assert when @cs_change is set and @spi_transfer is not
>      the last in @spi_message.
> 
> It even says they will be added up.
> 
> The only difference seems to be that @cs_change_delay applies only when
> the transfer is not the last in a list (spi_message, which we don't have
> here.)
> 
>      CS asserted
>      Transfer[0] (cs_change=true)
>      CS deasserted
>      delay(@cs_inactive) + delay(@cs_change_delay)
>      CS asserted
>      Transfer[1] (cs_change=true)
>      CS deasserted
>      delay(@cs_inactive)
> 
> 
> In both cases of redundancy above, the difference in semantics between
> the spi_device and spi_transfer only seem to apply when a series of
> transfers (spi_message or SPI_IOC_MESSAGE) are involved. Since (AFAICT)
> you aren't proposing that construct here, maybe the separate fields are
> not necessary? IOW, maybe @delay_ns and @cs_change_delay can be
> eliminated?
> 
> Please let me know if I've misunderstood anything.
> 
> Jonathon
> 
>>
>>       Could you please help check if this new structure contains enough
>> information. Really appreciate for kind help.
>>
>> Best Regards
>> Haixu Cui
>>
>> On 7/1/2023 5:59 AM, Jonathon Reinhart wrote:
>>>> Hi @jrreinhart@google.com,
>>>>        Thank you very much for your helpful comments.
>>>>
>>>>        I missed the delay and cs_change_delay parameters. I will add both
>>>> of them, although cs_change_delay can not be set from userspace, but can
>>>> be set in kernel space.
>>>>
>>>>
>>>>        For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are
>>>> defined in structure spi_device:
>>>>
>>>>        @cs_setup: delay to be introduced by the controller after CS is
>>>> asserted.
>>>>
>>>>        @cs_hold: delay to be introduced by the controller before CS is
>>>> deasserted.
>>>>
>>>>        @cs_inactive: delay to be introduced by the controller after CS is
>>>> deasserted. If @cs_change_delay is used from @spi_transfer, then the
>>>> two delays will be added up.
>>>>
>>>>        They show the SPI controller ability to control the CS
>>>> assertion/deassertion timing and should not be changed for each transfer
>>>> (because thay can be updated by setting structure spi_transfer or
>>>> structure spi_ioc_transfer). I think it better to define these parameter
>>>> in host OS rather than in guest OS since it's the host OS to operate the
>>>> hardware SPI controller directly. Besides, it can also avoid passing the
>>>> same values from guest to host time after time.
>>>>
>>>>        What's your opinion on this topic? Again, thank you very much.
>>>
>>> Hi Haixu,
>>>
>>> I took another look at the Linux structures and attempted to map up the
>>> delay parameters to the various points in a SPI sequence. Please correct
>>> me if I'm wrong about any of this.
>>>
>>> Consider this diagram where CS# is an active-low chip select, and SCLK
>>> is the serial clock:
>>>
>>>         ___.                                                   ._____.
>>>     CS#    |___________________________________________________|     |___
>>>            .                                                   .     .
>>>            .                                                   .     .
>>>    SCLK ________NNNN___NNNN___NNNN_______NNNN___NNNN___NNNN______________
>>>            .   .    . .           .     .                  .   .     .
>>> Delays:  +-A-+    +B+           +--C--+                  +-D-+--E--+
>>>            .   .    . .           .     .                  .   .     .
>>>            0   1    2 3           4     5                  6   7     8
>>>            .   .    .             .     .                  .   .
>>> Terms:   .   +Word+             .     .                  .   .
>>>            .   .                  .     .                  .   .
>>>            .   +-------Frame------+     +-------Frame------+   .
>>>            .                                                   .
>>>            +-----------------Transfer--------------------------+
>>>
>>> Using NXP terminology:
>>>
>>>     From 1 to 2 is a "word"     (with e.g. 8 bits).
>>>     From 1 to 4 is a "frame"    (with 3 words).
>>>     From 1 to 6 is a "transfer" (with 3 frames).
>>>
>>> Linux does not have the concept of a frame, only a series of transfers
>>> (a spi_message), each of which can specify whether CS changes or not.
>>>
>>> I've identified these various timing points and attempted to match them
>>> to the Linux spi_device and spi_transfer parameters:
>>>
>>> A - CS Setup: Delay after CS asserted before clock starts.
>>>       spi_device.cs_setup
>>>
>>> B - Word Delay: Delay between words.
>>>       spi_transfer.word_delay
>>>
>>> C - Frame Delay: Delay between frames.
>>>       (Linux does not have the concept of a SPI "frame".)
>>>
>>> D - CS Hold: Delay after clock stops, and before CS deasserted.
>>>       spi_device.cs_hold (+ spi_transfer.delay ??)
>>>
>>> E - CS Inactive: Delay after CS deasserted, before asserting again.
>>>       spi_device.cs_inactive + spi_transfer.cs_change_delay
>>>
>>>
>>> While I agree with you that some of these timings are unlikely to change
>>> from transfer-to-transfer, the subset of which should be specified
>>> at the device level vs. per-transfer seems somewhat arbitrary. As you
>>> can see there is some overlap between them.
>>>
>>> If I understand correctly, it appears that the CS hold time *can* be
>>> controlled on a per-transfer bases, using spi_transfer.delay
>>> ("after this transfer before changing the chipselect status").
>>>
>>> That leaves CS setup as the only parameter that cannot be influenced on
>>> a per-transfer basis.
>>>
>>> Theoretically, one might require different CS setup/hold times,
>>> depending on which slave_id they are talking to (on the same bus).
>>> In that case, one must set those spi_device parameters to the worst-case
>>> (maximum) values. However, this is already a Linux limitation, so I'm
>>> not sure it needs to be improved here.
>>>
>>> I think you've achieved parity with what the Linux kernel API allows,
>>> and that's probably good enough. As you said, anything else can be
>>> adjusted in the host OS -- I'm not sure how the guest would go about
>>> achieving that, though. You're not proposing the use of configuration
>>> space for virtio-spi, are you?
>>>
>>> Regards,
>>> Jonathon Reinhart
>>>
>>>
>>>>
>>>> Best Regards
>>>> Haixu Cui
>>>>
>>>> On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
>>>>> Hi Haixu,
>>>>>
>>>>>> +The \field{word_delay} defines how long to wait between words within
>>>>>> one SPI transfer,
>>>>>> +in ns unit.
>>>>>
>>>>> I'm a little surprised to see a word_delay but no frame_delay or
>>>>> transfer_delay.
>>>>>
>>>>> For example, many SPI peripherals require a delay after CS is asserted,
>>>>> but before the first SCLK edge, allowing them to prepare to send data.
>>>>> (E.g. an ADC might begin taking a sample.)
>>>>>
>>>>>
>>>>> The linux struct spi_transfer has three delay fields:
>>>>>
>>>>> * @cs_change_delay: delay between cs deassert and assert when
>>>>> *      @cs_change is set and @spi_transfer is not the last in
>>>>> *      @spi_message
>>>>> * @delay: delay to be introduced after this transfer before
>>>>> *    (optionally) changing the chipselect status, then starting the
>>>>> *    next transfer or completing this @spi_message.
>>>>> * @word_delay: inter word delay to be introduced after each word size
>>>>> *    (set by bits_per_word) transmission.
>>>>>
>>>>> The userspace spidev.h API has only two:
>>>>>
>>>>> * @delay_usecs: If nonzero, how long to delay after the last bit
>>>>> *       transfer before optionally deselecting the device before the
>>>>> *       next transfer.
>>>>> * @word_delay_usecs: If nonzero, how long to wait between words within
>>>>> *       one transfer. This property needs explicit support in the SPI
>>>>> *       controller, otherwise it is silently ignored.
>>>>>
>>>>> The NXP i.MX RT500 SPI (master) controller, for example, has four
>>>>> configurable delays:
>>>>>
>>>>> - PRE_DELAY: After CS assertion, before first SCLK edge.
>>>>> - POST_DELAY: After a transfer, before CS deassertion.
>>>>> - FRAME_DELAY: Between frames (which are arbitrarily defined by
>>>>>      software).
>>>>> - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
>>>>>
>>>>> The NVIDIA Tegra114 SPI controller has:
>>>>>
>>>>> - nvidia,cs-setup-clk-count: CS setup timing parameter.
>>>>> - nvidia,cs-hold-clk-count: CS hold timing parameter.
>>>>>
>>>>>
>>>>> I understand that accurately representing all possible delays might be
>>>>> difficult or futile, but I'm curious to understand why, of all the
>>>>> possible delays, inter-word delay was chosen for inclusion.
>>>>>
>>>>> In a microcontroller, delays around CS (de)assertion can be customized
>>>>> by using a GPIO -- rather than the hardware SPI block -- to drive the CS
>>>>> signal. This way, delays can be inserted where needed. To do so with a
>>>>> virtualized SPI controller might prove difficult however, as the virtual
>>>>> channel carrying a CS GPIO signal might not be synchronized to the
>>>>> channel carrying the SPI data.
>>>>>
>>>>> Curious to hear your thoughts.
>>>>>
>>>>> Thanks,
>>>>> Jonathon Reinhart

---------------------------------------------------------------------
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

[Jonathon Reinhart]

On Mon, Jul 17, 2023 at 10:53 AM Haixu Cui <quic_haixcui@quicinc.com> wrote:
>
> Hi Jonathon Reinhart,
>      Thank you very much for your comments.
>
> On 7/11/2023 12:42 AM, Jonathon Reinhart wrote:
> > On Fri, Jul 7, 2023 at 3:21 AM Haixu Cui <quic_haixcui@quicinc.com> wrote:
> >>
> >> Hi Jonathon Reinhart,
> >>
> >>       Thank you so much for all your helpful advice and info!
> >>
> >>       I took a look at latest Linux SPI driver and found, for
> >> cs_setup/cs_hold/cs_inactive set in device-tree, there are following two
> >> conditions:
> >>       1) if SPI controller supports CS timing configuration and CS is not
> >> using a GPIO line, then the SPI driver set the
> >> cs_setup/cs_hold/ca_inactive values into the corresponding SPI CS registers;
> >>       2) if CS is using a GPIO line, or SPI controller doesn't support CS
> >> timing configuration, it is the software to perform the
> >> cs_setup/cs_hold/cs_inactive delays, which is implemented in function
> >> spi_set_cs in driver/spi/spi.c.
> >>
> >>       So I agree it is necessary to pass cs_setup/cs_hold/cs_inactive to
> >> the host for each transfer.
> >>
> >>       For condition 1, host should set these values into the CS timing
> >> registers. And as you mentioned "one might require different CS
> >> setup/hold times, depending on which slave_id they are talking to (on
> >> the same bus)", if so, host need to overwrite the CS timing registers
> >> between the two transfers talking to different salve_id.
> >>
> >>       For condition 2, SPI driver running in the host performing the
> >> cs_setup/cs_hold/cs_inactive delays seems more accurate, compared with
> >> performing delays in guest.
> >>
> >>
> >>       And the latest virtio spi transfer structure is defined 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 mode;
> >>           u32 freq;
> >>           u32 word_delay_ns;
> >>           u32 delay_ns;
> >>           u32 cs_change_delay_ns;
> >>           u32 cs_setup_ns;
> >>           u32 cs_hold_ns;
> >>           u32 cs_inactive_ns;
> >>        };
> >>
> >
> > Hello Haixu Cui,
> >
> > I think there may be some unnecessary redundancy in these fields. See below.
> >
> >>        @slave_id: chipselect index the SPI transfer used
> >>        @bits_per_word: the number of bits in each SPI transfer word
> >>        @cs_change: True to deselect device before starting the next transfer
> >>        @tx_nbits: number of bits used for writing
> >>        @rx_nbits: number of bits used for reading
> >>        @reserved: reserved for future use
> >>        @mode: the spi mode defines how data is clocked out and in
> >>        @freq: transfer speed
> >>        @word_delay_ns: delay to be inserted between consecutive words of
> >> a transfer, in ns unit
> >>        @cs_setup_ns: delay to be introduced by the controller after CS is
> >> asserted, in ns unit
> >
> > (I am reordering these in my quoted text to be grouped appropriately.)
> >
> >>        @delay_ns: delay to be introduced after this transfer before
> >> (optionally) changing the chipselect status, in ns unit
> >>        @cs_hold_ns: delay to be introduced by the controller before CS is
> >> deasserted, in ns unit
> >
> > Aren't @delay_ns and @cs_hold_ns specifying the same thing?
>
> I think they are not the same thing, delay_ns is the spi transfer
> property while cs_hold_ns is the spi device property， although they take
> effect at the same stage.

I see. I think we have differing perspectives here.

I assume that you are looking at this from the perspective of a Linux
guest on a Linux host, where virtio-spi is connecting a virtual spi
driver in the guest to a hardware spi driver on the host. In this
case, it makes sense to have parity between the fields in Linux
spi_device / spi_transfer and the fields in the virtio
spi_transfer_head. It makes the implementation easier if there is a
simple 1:1 mapping between them, even if you have to copy some
spi_transfer_head fields to spi_device (like the cs_ fields).

My interest in virtio-spi is actually a bit different. We are looking
to connect a virtual spi driver in a guest running Linux to another
instance of QEMU emulating a baremetal firmware image. For a use-case
like mine, the redundancy in the fields complicates things slightly.
Because there is no physical SPI controller represented by the
spi_device, all fields on spi_transfer_head have to be implemented in
software -- including adding together the delays when appropriate
(@delay_ns + @cs_hold_ns, and @cs_change_delay_ns + @cs_inactive_ns).

It's not a deal-breaker, but I wanted you to consider the non-Linux
perspective. From a purely protocol-centric view, I think the fields
can still be seen as redundant.

> I list 4 conditions, here transfer cs delay including
> spi_transfer->delay and spi_transfer->cs_change_delay, and device cs
> delay including spi_device->cs_hold, spi_device->cs_setup and
> spi_device->cs_inactive.
>
> condition 1:
>      virtio-spi controller only has transfer_one interface but no
> transfer_one_message interface, and spi_transfer_one_message is the
> default transfer_one_message interface of virtio-spi controller.
>      Hardware SPI controller doesn't support CS timing configuration.
>
>      In this case, both transfer cs delay and device cs delay are
> executed by software in spi_transfer_one_message function in guest Linux
> OS. So guest doesn't need pass these cs timing parameters to host.
>
> condition 2:
>      virtio-spi controller only has transfer_one interface but no
> transfer_one_message interface, and spi_transfer_one_message is the
> default transfer_one_message interface of virtio-spi controller.
>      Hardware SPI controller supports device CS timing configuration,
> which means it has registers to hold these configuration values.
>
>      In this case, transfer cs delay is executed in
> spi_transfer_one_message function, and guest need pass device cs delay
> to host. Because one SPI number may have more than one slave, these
> slaves also may have different device cs delay values, so for each
> transfer, guest should pass device cs delay along with the slave_id to
> host to overwrite the corresponding registers.
>
> condition 3:
>      virtio-spi controller has transfer_one_message interface but no
> transfer_one interface.
>      Hardware SPI controller doesn't support CS timing configuration.
>
>      In this case, host SPI driver should implement all cs delay
> operations with the transfer cs delay and device cs delay received from
> the guest.
>
> condition 4:
>      virtio-spi controller has transfer_one_message interface but no
> transfer_one interface.
>      Hardware SPI controller supports CS timing configuration.
>
>      In this case, guest pass transfer cs delay and device cs delay to
> guest, and guest SPI driver should implement logic to execute transfer
> cs delay then overwrite the corresponding register with the device cs
> delay values.
>
> Except for condition 1, in other three conditions, guest should pass
> both transfer cs delay and device cs delay to host.
>
>
> Best Regards
> Haixu Cui
>
> >
> > Linux spi_transfer defines @delay as:
> >      delay to be introduced after this transfer before
> >      (optionally) changing the chipselect status, then starting the
> >      next transfer or completing this spi_message.
> >
> > ...and spi_device @cs_hold as: delay to be introduced by the
> >      controller before CS is deasserted.
> >
> > (This is the period labeled "D" in my diagram.)
> >
> > I guess the only difference is if you have a series of transfers, where
> > only the last transfer has @cs_change=true, and all preceding messages
> > have @cs_change=false. In this case, @delay_ns would apply between each
> > transfer, and after the last transfer @delay_ns + @cs_hold would apply
> > before CS is deasserted:
> >
> >      Transfer[0] (cs_change=false)
> >      delay(@delay_ns)
> >      Transfer[1] (cs_change=false)
> >      delay(@delay_ns)
> >      Transfer[3] (cs_change=true)
> >      delay(@delay_ns) + delay(@cs_hold_ns)
> >      CS deasserted
> >
> >
> >>        @cs_change_delay_ns: delay between cs deassert and assert when
> >> cs_change is set, in ns unit
> >>        @cs_inactive_ns: delay to be introduced by the controller after CS
> >> is deasserted, in ns unit
> >
> > Similarly, aren't @cs_change_delay_ns and @cs_inactive_ns applied at the
> > same point in the cycle?
> >
> > (This is the period labeled "E" in my diagram.)
> >
> > Linux spi_device defines @cs_inactive as: delay to be introduced by the
> >      controller after CS is deasserted. If @cs_change_delay is used from
> >      @spi_transfer, then the two delays will be added up.
> >
> > ...and spi_transfer defines @cs_change_delay as: delay between cs
> >      deassert and assert when @cs_change is set and @spi_transfer is not
> >      the last in @spi_message.
> >
> > It even says they will be added up.
> >
> > The only difference seems to be that @cs_change_delay applies only when
> > the transfer is not the last in a list (spi_message, which we don't have
> > here.)
> >
> >      CS asserted
> >      Transfer[0] (cs_change=true)
> >      CS deasserted
> >      delay(@cs_inactive) + delay(@cs_change_delay)
> >      CS asserted
> >      Transfer[1] (cs_change=true)
> >      CS deasserted
> >      delay(@cs_inactive)
> >
> >
> > In both cases of redundancy above, the difference in semantics between
> > the spi_device and spi_transfer only seem to apply when a series of
> > transfers (spi_message or SPI_IOC_MESSAGE) are involved. Since (AFAICT)
> > you aren't proposing that construct here, maybe the separate fields are
> > not necessary? IOW, maybe @delay_ns and @cs_change_delay can be
> > eliminated?
> >
> > Please let me know if I've misunderstood anything.
> >
> > Jonathon
> >
> >>
> >>       Could you please help check if this new structure contains enough
> >> information. Really appreciate for kind help.
> >>
> >> Best Regards
> >> Haixu Cui
> >>
> >> On 7/1/2023 5:59 AM, Jonathon Reinhart wrote:
> >>>> Hi @jrreinhart@google.com,
> >>>>        Thank you very much for your helpful comments.
> >>>>
> >>>>        I missed the delay and cs_change_delay parameters. I will add both
> >>>> of them, although cs_change_delay can not be set from userspace, but can
> >>>> be set in kernel space.
> >>>>
> >>>>
> >>>>        For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are
> >>>> defined in structure spi_device:
> >>>>
> >>>>        @cs_setup: delay to be introduced by the controller after CS is
> >>>> asserted.
> >>>>
> >>>>        @cs_hold: delay to be introduced by the controller before CS is
> >>>> deasserted.
> >>>>
> >>>>        @cs_inactive: delay to be introduced by the controller after CS is
> >>>> deasserted. If @cs_change_delay is used from @spi_transfer, then the
> >>>> two delays will be added up.
> >>>>
> >>>>        They show the SPI controller ability to control the CS
> >>>> assertion/deassertion timing and should not be changed for each transfer
> >>>> (because thay can be updated by setting structure spi_transfer or
> >>>> structure spi_ioc_transfer). I think it better to define these parameter
> >>>> in host OS rather than in guest OS since it's the host OS to operate the
> >>>> hardware SPI controller directly. Besides, it can also avoid passing the
> >>>> same values from guest to host time after time.
> >>>>
> >>>>        What's your opinion on this topic? Again, thank you very much.
> >>>
> >>> Hi Haixu,
> >>>
> >>> I took another look at the Linux structures and attempted to map up the
> >>> delay parameters to the various points in a SPI sequence. Please correct
> >>> me if I'm wrong about any of this.
> >>>
> >>> Consider this diagram where CS# is an active-low chip select, and SCLK
> >>> is the serial clock:
> >>>
> >>>         ___.                                                   ._____.
> >>>     CS#    |___________________________________________________|     |___
> >>>            .                                                   .     .
> >>>            .                                                   .     .
> >>>    SCLK ________NNNN___NNNN___NNNN_______NNNN___NNNN___NNNN______________
> >>>            .   .    . .           .     .                  .   .     .
> >>> Delays:  +-A-+    +B+           +--C--+                  +-D-+--E--+
> >>>            .   .    . .           .     .                  .   .     .
> >>>            0   1    2 3           4     5                  6   7     8
> >>>            .   .    .             .     .                  .   .
> >>> Terms:   .   +Word+             .     .                  .   .
> >>>            .   .                  .     .                  .   .
> >>>            .   +-------Frame------+     +-------Frame------+   .
> >>>            .                                                   .
> >>>            +-----------------Transfer--------------------------+
> >>>
> >>> Using NXP terminology:
> >>>
> >>>     From 1 to 2 is a "word"     (with e.g. 8 bits).
> >>>     From 1 to 4 is a "frame"    (with 3 words).
> >>>     From 1 to 6 is a "transfer" (with 3 frames).
> >>>
> >>> Linux does not have the concept of a frame, only a series of transfers
> >>> (a spi_message), each of which can specify whether CS changes or not.
> >>>
> >>> I've identified these various timing points and attempted to match them
> >>> to the Linux spi_device and spi_transfer parameters:
> >>>
> >>> A - CS Setup: Delay after CS asserted before clock starts.
> >>>       spi_device.cs_setup
> >>>
> >>> B - Word Delay: Delay between words.
> >>>       spi_transfer.word_delay
> >>>
> >>> C - Frame Delay: Delay between frames.
> >>>       (Linux does not have the concept of a SPI "frame".)
> >>>
> >>> D - CS Hold: Delay after clock stops, and before CS deasserted.
> >>>       spi_device.cs_hold (+ spi_transfer.delay ??)
> >>>
> >>> E - CS Inactive: Delay after CS deasserted, before asserting again.
> >>>       spi_device.cs_inactive + spi_transfer.cs_change_delay
> >>>
> >>>
> >>> While I agree with you that some of these timings are unlikely to change
> >>> from transfer-to-transfer, the subset of which should be specified
> >>> at the device level vs. per-transfer seems somewhat arbitrary. As you
> >>> can see there is some overlap between them.
> >>>
> >>> If I understand correctly, it appears that the CS hold time *can* be
> >>> controlled on a per-transfer bases, using spi_transfer.delay
> >>> ("after this transfer before changing the chipselect status").
> >>>
> >>> That leaves CS setup as the only parameter that cannot be influenced on
> >>> a per-transfer basis.
> >>>
> >>> Theoretically, one might require different CS setup/hold times,
> >>> depending on which slave_id they are talking to (on the same bus).
> >>> In that case, one must set those spi_device parameters to the worst-case
> >>> (maximum) values. However, this is already a Linux limitation, so I'm
> >>> not sure it needs to be improved here.
> >>>
> >>> I think you've achieved parity with what the Linux kernel API allows,
> >>> and that's probably good enough. As you said, anything else can be
> >>> adjusted in the host OS -- I'm not sure how the guest would go about
> >>> achieving that, though. You're not proposing the use of configuration
> >>> space for virtio-spi, are you?
> >>>
> >>> Regards,
> >>> Jonathon Reinhart
> >>>
> >>>
> >>>>
> >>>> Best Regards
> >>>> Haixu Cui
> >>>>
> >>>> On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
> >>>>> Hi Haixu,
> >>>>>
> >>>>>> +The \field{word_delay} defines how long to wait between words within
> >>>>>> one SPI transfer,
> >>>>>> +in ns unit.
> >>>>>
> >>>>> I'm a little surprised to see a word_delay but no frame_delay or
> >>>>> transfer_delay.
> >>>>>
> >>>>> For example, many SPI peripherals require a delay after CS is asserted,
> >>>>> but before the first SCLK edge, allowing them to prepare to send data.
> >>>>> (E.g. an ADC might begin taking a sample.)
> >>>>>
> >>>>>
> >>>>> The linux struct spi_transfer has three delay fields:
> >>>>>
> >>>>> * @cs_change_delay: delay between cs deassert and assert when
> >>>>> *      @cs_change is set and @spi_transfer is not the last in
> >>>>> *      @spi_message
> >>>>> * @delay: delay to be introduced after this transfer before
> >>>>> *    (optionally) changing the chipselect status, then starting the
> >>>>> *    next transfer or completing this @spi_message.
> >>>>> * @word_delay: inter word delay to be introduced after each word size
> >>>>> *    (set by bits_per_word) transmission.
> >>>>>
> >>>>> The userspace spidev.h API has only two:
> >>>>>
> >>>>> * @delay_usecs: If nonzero, how long to delay after the last bit
> >>>>> *       transfer before optionally deselecting the device before the
> >>>>> *       next transfer.
> >>>>> * @word_delay_usecs: If nonzero, how long to wait between words within
> >>>>> *       one transfer. This property needs explicit support in the SPI
> >>>>> *       controller, otherwise it is silently ignored.
> >>>>>
> >>>>> The NXP i.MX RT500 SPI (master) controller, for example, has four
> >>>>> configurable delays:
> >>>>>
> >>>>> - PRE_DELAY: After CS assertion, before first SCLK edge.
> >>>>> - POST_DELAY: After a transfer, before CS deassertion.
> >>>>> - FRAME_DELAY: Between frames (which are arbitrarily defined by
> >>>>>      software).
> >>>>> - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
> >>>>>
> >>>>> The NVIDIA Tegra114 SPI controller has:
> >>>>>
> >>>>> - nvidia,cs-setup-clk-count: CS setup timing parameter.
> >>>>> - nvidia,cs-hold-clk-count: CS hold timing parameter.
> >>>>>
> >>>>>
> >>>>> I understand that accurately representing all possible delays might be
> >>>>> difficult or futile, but I'm curious to understand why, of all the
> >>>>> possible delays, inter-word delay was chosen for inclusion.
> >>>>>
> >>>>> In a microcontroller, delays around CS (de)assertion can be customized
> >>>>> by using a GPIO -- rather than the hardware SPI block -- to drive the CS
> >>>>> signal. This way, delays can be inserted where needed. To do so with a
> >>>>> virtualized SPI controller might prove difficult however, as the virtual
> >>>>> channel carrying a CS GPIO signal might not be synchronized to the
> >>>>> channel carrying the SPI data.
> >>>>>
> >>>>> Curious to hear your thoughts.
> >>>>>
> >>>>> Thanks,
> >>>>> Jonathon Reinhart

---------------------------------------------------------------------
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 Jonathon Reinhart,
     Thank you very much for your guidance. It is indeed an excellent 
idea to combine the parameters which take effect at the same phase.

     So the front-end & back-end interface updated 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;
     };

     @cs_delay_hold_ns: delay to be introduced before CS is deasserted 
for each transfer, in ns unit. (spi_device->cs_hold and 
spi_transfer->delay added up)

     @cs_change_delay_inactive_ns: delay to be introduced after CS is 
deasserted and before next asserted, in ns unit. It will be executed 
only if the cs_change is set and the transfer is not the last one. 
(spi_device->cs_inactive and spi_transfer->cs_change_delay added up)


Best Regards & Thanks
Haixu Cui



On 7/18/2023 12:19 AM, Jonathon Reinhart wrote:
> On Mon, Jul 17, 2023 at 10:53 AM Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>
>> Hi Jonathon Reinhart,
>>       Thank you very much for your comments.
>>
>> On 7/11/2023 12:42 AM, Jonathon Reinhart wrote:
>>> On Fri, Jul 7, 2023 at 3:21 AM Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>>
>>>> Hi Jonathon Reinhart,
>>>>
>>>>        Thank you so much for all your helpful advice and info!
>>>>
>>>>        I took a look at latest Linux SPI driver and found, for
>>>> cs_setup/cs_hold/cs_inactive set in device-tree, there are following two
>>>> conditions:
>>>>        1) if SPI controller supports CS timing configuration and CS is not
>>>> using a GPIO line, then the SPI driver set the
>>>> cs_setup/cs_hold/ca_inactive values into the corresponding SPI CS registers;
>>>>        2) if CS is using a GPIO line, or SPI controller doesn't support CS
>>>> timing configuration, it is the software to perform the
>>>> cs_setup/cs_hold/cs_inactive delays, which is implemented in function
>>>> spi_set_cs in driver/spi/spi.c.
>>>>
>>>>        So I agree it is necessary to pass cs_setup/cs_hold/cs_inactive to
>>>> the host for each transfer.
>>>>
>>>>        For condition 1, host should set these values into the CS timing
>>>> registers. And as you mentioned "one might require different CS
>>>> setup/hold times, depending on which slave_id they are talking to (on
>>>> the same bus)", if so, host need to overwrite the CS timing registers
>>>> between the two transfers talking to different salve_id.
>>>>
>>>>        For condition 2, SPI driver running in the host performing the
>>>> cs_setup/cs_hold/cs_inactive delays seems more accurate, compared with
>>>> performing delays in guest.
>>>>
>>>>
>>>>        And the latest virtio spi transfer structure is defined 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 mode;
>>>>            u32 freq;
>>>>            u32 word_delay_ns;
>>>>            u32 delay_ns;
>>>>            u32 cs_change_delay_ns;
>>>>            u32 cs_setup_ns;
>>>>            u32 cs_hold_ns;
>>>>            u32 cs_inactive_ns;
>>>>         };
>>>>
>>>
>>> Hello Haixu Cui,
>>>
>>> I think there may be some unnecessary redundancy in these fields. See below.
>>>
>>>>         @slave_id: chipselect index the SPI transfer used
>>>>         @bits_per_word: the number of bits in each SPI transfer word
>>>>         @cs_change: True to deselect device before starting the next transfer
>>>>         @tx_nbits: number of bits used for writing
>>>>         @rx_nbits: number of bits used for reading
>>>>         @reserved: reserved for future use
>>>>         @mode: the spi mode defines how data is clocked out and in
>>>>         @freq: transfer speed
>>>>         @word_delay_ns: delay to be inserted between consecutive words of
>>>> a transfer, in ns unit
>>>>         @cs_setup_ns: delay to be introduced by the controller after CS is
>>>> asserted, in ns unit
>>>
>>> (I am reordering these in my quoted text to be grouped appropriately.)
>>>
>>>>         @delay_ns: delay to be introduced after this transfer before
>>>> (optionally) changing the chipselect status, in ns unit
>>>>         @cs_hold_ns: delay to be introduced by the controller before CS is
>>>> deasserted, in ns unit
>>>
>>> Aren't @delay_ns and @cs_hold_ns specifying the same thing?
>>
>> I think they are not the same thing, delay_ns is the spi transfer
>> property while cs_hold_ns is the spi device property， although they take
>> effect at the same stage.
> 
> I see. I think we have differing perspectives here.
> 
> I assume that you are looking at this from the perspective of a Linux
> guest on a Linux host, where virtio-spi is connecting a virtual spi
> driver in the guest to a hardware spi driver on the host. In this
> case, it makes sense to have parity between the fields in Linux
> spi_device / spi_transfer and the fields in the virtio
> spi_transfer_head. It makes the implementation easier if there is a
> simple 1:1 mapping between them, even if you have to copy some
> spi_transfer_head fields to spi_device (like the cs_ fields).
> 
> My interest in virtio-spi is actually a bit different. We are looking
> to connect a virtual spi driver in a guest running Linux to another
> instance of QEMU emulating a baremetal firmware image. For a use-case
> like mine, the redundancy in the fields complicates things slightly.
> Because there is no physical SPI controller represented by the
> spi_device, all fields on spi_transfer_head have to be implemented in
> software -- including adding together the delays when appropriate
> (@delay_ns + @cs_hold_ns, and @cs_change_delay_ns + @cs_inactive_ns).
> 
> It's not a deal-breaker, but I wanted you to consider the non-Linux
> perspective. From a purely protocol-centric view, I think the fields
> can still be seen as redundant.
> 
>> I list 4 conditions, here transfer cs delay including
>> spi_transfer->delay and spi_transfer->cs_change_delay, and device cs
>> delay including spi_device->cs_hold, spi_device->cs_setup and
>> spi_device->cs_inactive.
>>
>> condition 1:
>>       virtio-spi controller only has transfer_one interface but no
>> transfer_one_message interface, and spi_transfer_one_message is the
>> default transfer_one_message interface of virtio-spi controller.
>>       Hardware SPI controller doesn't support CS timing configuration.
>>
>>       In this case, both transfer cs delay and device cs delay are
>> executed by software in spi_transfer_one_message function in guest Linux
>> OS. So guest doesn't need pass these cs timing parameters to host.
>>
>> condition 2:
>>       virtio-spi controller only has transfer_one interface but no
>> transfer_one_message interface, and spi_transfer_one_message is the
>> default transfer_one_message interface of virtio-spi controller.
>>       Hardware SPI controller supports device CS timing configuration,
>> which means it has registers to hold these configuration values.
>>
>>       In this case, transfer cs delay is executed in
>> spi_transfer_one_message function, and guest need pass device cs delay
>> to host. Because one SPI number may have more than one slave, these
>> slaves also may have different device cs delay values, so for each
>> transfer, guest should pass device cs delay along with the slave_id to
>> host to overwrite the corresponding registers.
>>
>> condition 3:
>>       virtio-spi controller has transfer_one_message interface but no
>> transfer_one interface.
>>       Hardware SPI controller doesn't support CS timing configuration.
>>
>>       In this case, host SPI driver should implement all cs delay
>> operations with the transfer cs delay and device cs delay received from
>> the guest.
>>
>> condition 4:
>>       virtio-spi controller has transfer_one_message interface but no
>> transfer_one interface.
>>       Hardware SPI controller supports CS timing configuration.
>>
>>       In this case, guest pass transfer cs delay and device cs delay to
>> guest, and guest SPI driver should implement logic to execute transfer
>> cs delay then overwrite the corresponding register with the device cs
>> delay values.
>>
>> Except for condition 1, in other three conditions, guest should pass
>> both transfer cs delay and device cs delay to host.
>>
>>
>> Best Regards
>> Haixu Cui
>>
>>>
>>> Linux spi_transfer defines @delay as:
>>>       delay to be introduced after this transfer before
>>>       (optionally) changing the chipselect status, then starting the
>>>       next transfer or completing this spi_message.
>>>
>>> ...and spi_device @cs_hold as: delay to be introduced by the
>>>       controller before CS is deasserted.
>>>
>>> (This is the period labeled "D" in my diagram.)
>>>
>>> I guess the only difference is if you have a series of transfers, where
>>> only the last transfer has @cs_change=true, and all preceding messages
>>> have @cs_change=false. In this case, @delay_ns would apply between each
>>> transfer, and after the last transfer @delay_ns + @cs_hold would apply
>>> before CS is deasserted:
>>>
>>>       Transfer[0] (cs_change=false)
>>>       delay(@delay_ns)
>>>       Transfer[1] (cs_change=false)
>>>       delay(@delay_ns)
>>>       Transfer[3] (cs_change=true)
>>>       delay(@delay_ns) + delay(@cs_hold_ns)
>>>       CS deasserted
>>>
>>>
>>>>         @cs_change_delay_ns: delay between cs deassert and assert when
>>>> cs_change is set, in ns unit
>>>>         @cs_inactive_ns: delay to be introduced by the controller after CS
>>>> is deasserted, in ns unit
>>>
>>> Similarly, aren't @cs_change_delay_ns and @cs_inactive_ns applied at the
>>> same point in the cycle?
>>>
>>> (This is the period labeled "E" in my diagram.)
>>>
>>> Linux spi_device defines @cs_inactive as: delay to be introduced by the
>>>       controller after CS is deasserted. If @cs_change_delay is used from
>>>       @spi_transfer, then the two delays will be added up.
>>>
>>> ...and spi_transfer defines @cs_change_delay as: delay between cs
>>>       deassert and assert when @cs_change is set and @spi_transfer is not
>>>       the last in @spi_message.
>>>
>>> It even says they will be added up.
>>>
>>> The only difference seems to be that @cs_change_delay applies only when
>>> the transfer is not the last in a list (spi_message, which we don't have
>>> here.)
>>>
>>>       CS asserted
>>>       Transfer[0] (cs_change=true)
>>>       CS deasserted
>>>       delay(@cs_inactive) + delay(@cs_change_delay)
>>>       CS asserted
>>>       Transfer[1] (cs_change=true)
>>>       CS deasserted
>>>       delay(@cs_inactive)
>>>
>>>
>>> In both cases of redundancy above, the difference in semantics between
>>> the spi_device and spi_transfer only seem to apply when a series of
>>> transfers (spi_message or SPI_IOC_MESSAGE) are involved. Since (AFAICT)
>>> you aren't proposing that construct here, maybe the separate fields are
>>> not necessary? IOW, maybe @delay_ns and @cs_change_delay can be
>>> eliminated?
>>>
>>> Please let me know if I've misunderstood anything.
>>>
>>> Jonathon
>>>
>>>>
>>>>        Could you please help check if this new structure contains enough
>>>> information. Really appreciate for kind help.
>>>>
>>>> Best Regards
>>>> Haixu Cui
>>>>
>>>> On 7/1/2023 5:59 AM, Jonathon Reinhart wrote:
>>>>>> Hi @jrreinhart@google.com,
>>>>>>         Thank you very much for your helpful comments.
>>>>>>
>>>>>>         I missed the delay and cs_change_delay parameters. I will add both
>>>>>> of them, although cs_change_delay can not be set from userspace, but can
>>>>>> be set in kernel space.
>>>>>>
>>>>>>
>>>>>>         For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are
>>>>>> defined in structure spi_device:
>>>>>>
>>>>>>         @cs_setup: delay to be introduced by the controller after CS is
>>>>>> asserted.
>>>>>>
>>>>>>         @cs_hold: delay to be introduced by the controller before CS is
>>>>>> deasserted.
>>>>>>
>>>>>>         @cs_inactive: delay to be introduced by the controller after CS is
>>>>>> deasserted. If @cs_change_delay is used from @spi_transfer, then the
>>>>>> two delays will be added up.
>>>>>>
>>>>>>         They show the SPI controller ability to control the CS
>>>>>> assertion/deassertion timing and should not be changed for each transfer
>>>>>> (because thay can be updated by setting structure spi_transfer or
>>>>>> structure spi_ioc_transfer). I think it better to define these parameter
>>>>>> in host OS rather than in guest OS since it's the host OS to operate the
>>>>>> hardware SPI controller directly. Besides, it can also avoid passing the
>>>>>> same values from guest to host time after time.
>>>>>>
>>>>>>         What's your opinion on this topic? Again, thank you very much.
>>>>>
>>>>> Hi Haixu,
>>>>>
>>>>> I took another look at the Linux structures and attempted to map up the
>>>>> delay parameters to the various points in a SPI sequence. Please correct
>>>>> me if I'm wrong about any of this.
>>>>>
>>>>> Consider this diagram where CS# is an active-low chip select, and SCLK
>>>>> is the serial clock:
>>>>>
>>>>>          ___.                                                   ._____.
>>>>>      CS#    |___________________________________________________|     |___
>>>>>             .                                                   .     .
>>>>>             .                                                   .     .
>>>>>     SCLK ________NNNN___NNNN___NNNN_______NNNN___NNNN___NNNN______________
>>>>>             .   .    . .           .     .                  .   .     .
>>>>> Delays:  +-A-+    +B+           +--C--+                  +-D-+--E--+
>>>>>             .   .    . .           .     .                  .   .     .
>>>>>             0   1    2 3           4     5                  6   7     8
>>>>>             .   .    .             .     .                  .   .
>>>>> Terms:   .   +Word+             .     .                  .   .
>>>>>             .   .                  .     .                  .   .
>>>>>             .   +-------Frame------+     +-------Frame------+   .
>>>>>             .                                                   .
>>>>>             +-----------------Transfer--------------------------+
>>>>>
>>>>> Using NXP terminology:
>>>>>
>>>>>      From 1 to 2 is a "word"     (with e.g. 8 bits).
>>>>>      From 1 to 4 is a "frame"    (with 3 words).
>>>>>      From 1 to 6 is a "transfer" (with 3 frames).
>>>>>
>>>>> Linux does not have the concept of a frame, only a series of transfers
>>>>> (a spi_message), each of which can specify whether CS changes or not.
>>>>>
>>>>> I've identified these various timing points and attempted to match them
>>>>> to the Linux spi_device and spi_transfer parameters:
>>>>>
>>>>> A - CS Setup: Delay after CS asserted before clock starts.
>>>>>        spi_device.cs_setup
>>>>>
>>>>> B - Word Delay: Delay between words.
>>>>>        spi_transfer.word_delay
>>>>>
>>>>> C - Frame Delay: Delay between frames.
>>>>>        (Linux does not have the concept of a SPI "frame".)
>>>>>
>>>>> D - CS Hold: Delay after clock stops, and before CS deasserted.
>>>>>        spi_device.cs_hold (+ spi_transfer.delay ??)
>>>>>
>>>>> E - CS Inactive: Delay after CS deasserted, before asserting again.
>>>>>        spi_device.cs_inactive + spi_transfer.cs_change_delay
>>>>>
>>>>>
>>>>> While I agree with you that some of these timings are unlikely to change
>>>>> from transfer-to-transfer, the subset of which should be specified
>>>>> at the device level vs. per-transfer seems somewhat arbitrary. As you
>>>>> can see there is some overlap between them.
>>>>>
>>>>> If I understand correctly, it appears that the CS hold time *can* be
>>>>> controlled on a per-transfer bases, using spi_transfer.delay
>>>>> ("after this transfer before changing the chipselect status").
>>>>>
>>>>> That leaves CS setup as the only parameter that cannot be influenced on
>>>>> a per-transfer basis.
>>>>>
>>>>> Theoretically, one might require different CS setup/hold times,
>>>>> depending on which slave_id they are talking to (on the same bus).
>>>>> In that case, one must set those spi_device parameters to the worst-case
>>>>> (maximum) values. However, this is already a Linux limitation, so I'm
>>>>> not sure it needs to be improved here.
>>>>>
>>>>> I think you've achieved parity with what the Linux kernel API allows,
>>>>> and that's probably good enough. As you said, anything else can be
>>>>> adjusted in the host OS -- I'm not sure how the guest would go about
>>>>> achieving that, though. You're not proposing the use of configuration
>>>>> space for virtio-spi, are you?
>>>>>
>>>>> Regards,
>>>>> Jonathon Reinhart
>>>>>
>>>>>
>>>>>>
>>>>>> Best Regards
>>>>>> Haixu Cui
>>>>>>
>>>>>> On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
>>>>>>> Hi Haixu,
>>>>>>>
>>>>>>>> +The \field{word_delay} defines how long to wait between words within
>>>>>>>> one SPI transfer,
>>>>>>>> +in ns unit.
>>>>>>>
>>>>>>> I'm a little surprised to see a word_delay but no frame_delay or
>>>>>>> transfer_delay.
>>>>>>>
>>>>>>> For example, many SPI peripherals require a delay after CS is asserted,
>>>>>>> but before the first SCLK edge, allowing them to prepare to send data.
>>>>>>> (E.g. an ADC might begin taking a sample.)
>>>>>>>
>>>>>>>
>>>>>>> The linux struct spi_transfer has three delay fields:
>>>>>>>
>>>>>>> * @cs_change_delay: delay between cs deassert and assert when
>>>>>>> *      @cs_change is set and @spi_transfer is not the last in
>>>>>>> *      @spi_message
>>>>>>> * @delay: delay to be introduced after this transfer before
>>>>>>> *    (optionally) changing the chipselect status, then starting the
>>>>>>> *    next transfer or completing this @spi_message.
>>>>>>> * @word_delay: inter word delay to be introduced after each word size
>>>>>>> *    (set by bits_per_word) transmission.
>>>>>>>
>>>>>>> The userspace spidev.h API has only two:
>>>>>>>
>>>>>>> * @delay_usecs: If nonzero, how long to delay after the last bit
>>>>>>> *       transfer before optionally deselecting the device before the
>>>>>>> *       next transfer.
>>>>>>> * @word_delay_usecs: If nonzero, how long to wait between words within
>>>>>>> *       one transfer. This property needs explicit support in the SPI
>>>>>>> *       controller, otherwise it is silently ignored.
>>>>>>>
>>>>>>> The NXP i.MX RT500 SPI (master) controller, for example, has four
>>>>>>> configurable delays:
>>>>>>>
>>>>>>> - PRE_DELAY: After CS assertion, before first SCLK edge.
>>>>>>> - POST_DELAY: After a transfer, before CS deassertion.
>>>>>>> - FRAME_DELAY: Between frames (which are arbitrarily defined by
>>>>>>>       software).
>>>>>>> - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
>>>>>>>
>>>>>>> The NVIDIA Tegra114 SPI controller has:
>>>>>>>
>>>>>>> - nvidia,cs-setup-clk-count: CS setup timing parameter.
>>>>>>> - nvidia,cs-hold-clk-count: CS hold timing parameter.
>>>>>>>
>>>>>>>
>>>>>>> I understand that accurately representing all possible delays might be
>>>>>>> difficult or futile, but I'm curious to understand why, of all the
>>>>>>> possible delays, inter-word delay was chosen for inclusion.
>>>>>>>
>>>>>>> In a microcontroller, delays around CS (de)assertion can be customized
>>>>>>> by using a GPIO -- rather than the hardware SPI block -- to drive the CS
>>>>>>> signal. This way, delays can be inserted where needed. To do so with a
>>>>>>> virtualized SPI controller might prove difficult however, as the virtual
>>>>>>> channel carrying a CS GPIO signal might not be synchronized to the
>>>>>>> channel carrying the SPI data.
>>>>>>>
>>>>>>> Curious to hear your thoughts.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jonathon Reinhart

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

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

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

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