Discussion of the implementations of VIRTIO specification
 help / color / mirror / Atom feed
* [virtio-dev] [PATCH v4] virtio-spi: add the device specification
@ 2023-10-24 12:53 Haixu Cui
  2023-10-24 13:08 ` [virtio-dev] " Haixu Cui
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Haixu Cui @ 2023-10-24 12:53 UTC (permalink / raw)
  To: virtio-dev, virtio-comment, harald.mommer, cohuck; +Cc: quic_ztu, Haixu Cui

virtio-spi is a virtual SPI master and it allows a guest to operate and
use the physical SPI master controlled by the host.

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
 device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 +
 device-types/spi/driver-conformance.tex |   7 +
 3 files changed, 220 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..5dbceaf
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,206 @@
+\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 controlled by the host.
+
+virtio-spi has a single virtqueue. SPI transfer requests are placed into
+the virtqueue, and serviced by the physical SPI master.
+
+In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
+the front-end existing in the guest kernel, and Virtio SPI device acts as the
+back-end in the host platform.
+
+\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 Virtio SPI driver.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+	le16 bus_num;
+	le16 chip_select_max_number;
+	le8 cs_timing_setting_enable;
+	le8 reserved[3];
+};
+\end{lstlisting}
+
+The \field{bus_num} indicates the physical SPI master assigned to guest.
+
+The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
+
+The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
+	0: physical SPI master doesn't support cs timing setting;
+	1: physical SPI master supports cs timing setting.
+
+The \field{reserved} is for alignment purpose, also for future extension.
+
+\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
+
+\begin{enumerate}
+\item The Virtio SPI driver configures and initializes the virtqueue.
+\end{enumerate}
+
+\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}
+
+Virtio SPI driver enqueues requests to the virtqueue, and they are used by
+Virtio SPI device. Each request represents one SPI tranfer and is of the form: 
+
+\begin{lstlisting}
+struct virtio_spi_transfer_head {
+        u8 slave_id;
+        u8 bits_per_word;
+        u8 cs_change;
+        u8 tx_nbits;
+        u8 rx_nbits;
+	 u8 reserved[3];
+        le32 mode;
+        le32 freq;
+        le32 word_delay_ns;
+        le32 cs_setup_ns;
+        le32 cs_delay_hold_ns;
+        le32 cs_change_delay_inactive_ns;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_transfer_result {
+        u8 result;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_transfer_req {
+        struct virtio_spi_transfer_head head;
+        u8 tx_buf[];
+        u8 rx_buf[];
+        struct virtio_spi_transfer_result result;
+};
+\end{lstlisting}
+
+The \field{slave_id} indicates the chipselect index the SPI transfer used.
+
+The \field{bits_per_word} indicates the number of bits in each SPI transfer word.
+
+The \field{cs_change} indicates whether to deselect device before starting the
+next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
+then asserted again.
+
+The \field{tx_nbits} indicates 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.
+
+The \field{rx_nbits} indicates 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.
+
+The \field{reserved} is for alignement, also for further extension.
+
+The \field{mode} indicates how data is clocked out and in. Bit definitions as follows:
+        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
+	       relative to the clock pulses.
+        bit 1: CPOL, determines the polarity of the clock.
+        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
+
+The \field{freq} indicates the SPI transfer speed in Hz.
+
+The \field{word_delay_ns} indicates delay to be inserted between consecutive
+words of a transfer, in ns unit.
+
+The \field{cs_setup_ns} indicates delay to be introduced after chipselect
+is asserted, in ns unit.
+
+The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
+is deasserted, in ns unit.
+
+The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
+chipselect is deasserted and before next asserted, in ns unit.
+
+The \field{tx_buf} is the buffer for data sent to the device.
+
+The \field{rx_buf} is the buffer for data received to the device.
+
+The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
+or VIRTIO_SPI_TRANS_ERR for error.
+
+\begin{lstlisting}
+#define VIRTIO_SPI_TRANS_OK     0
+#define VIRTIO_SPI_TRANS_ERR    1
+\end{lstlisting}
+
+\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
+
+Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
+\field{result} in structure \field{virtio_spi_transfer_result} is written by 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 Virtio SPI device and consumed
+by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio 
+SPI driver and consumed by 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 transfer requests on the requestq virtqueue.
+
+Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
+and MUST be readable for Virtio SPI device.
+
+Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
+and MUST be writable for Virtio SPI device.
+
+For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
+\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
+\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head}, 
+\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
+if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
+
+If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
+\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
+SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
+
+Virtio SPI device MUST set all the fields of the structure \field{virtio_spi_config} before
+they are read by Virtio SPI driver.
+
+Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result} before sending
+it back to Virtio SPI driver.
+
+Virtio SPI device MUST be able to identify the transfer type according to the received
+virtqueue descriptors.
+
+Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
--- /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}
+
+An 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..3c965ef
--- /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}
+
+An 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [virtio-dev] Re: [PATCH v4] virtio-spi: add the device specification
  2023-10-24 12:53 [virtio-dev] [PATCH v4] virtio-spi: add the device specification Haixu Cui
@ 2023-10-24 13:08 ` Haixu Cui
  2023-11-07  7:40 ` [virtio-dev] " Qiang Zhang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Haixu Cui @ 2023-10-24 13:08 UTC (permalink / raw)
  To: virtio-dev, virtio-comment, harald.mommer, cohuck; +Cc: quic_ztu

Hi Harald, Cornelia,

     I submit this patch v4 with some updates according to Harald's 
comments.

     Can you please help review this patch. If there are no major 
problems, I think this patch can be merged as the initial version. And I 
will upstream kernel code after this patch gets merged.

     Thank you very much for your advice and support.

Best Regards
Haixu Cui

On 10/24/2023 8:53 PM, Haixu Cui wrote:
> virtio-spi is a virtual SPI master and it allows a guest to operate and
> use the physical SPI master controlled by the host.
> 
> This patch adds the specification for virtio-spi.
> 
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>   device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
>   device-types/spi/device-conformance.tex |   7 +
>   device-types/spi/driver-conformance.tex |   7 +
>   3 files changed, 220 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..5dbceaf
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,206 @@
> +\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 controlled by the host.
> +
> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
> +the virtqueue, and serviced by the physical SPI master.
> +
> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
> +the front-end existing in the guest kernel, and Virtio SPI device acts as the
> +back-end in the host platform.
> +
> +\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 Virtio SPI driver.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> +	le16 bus_num;
> +	le16 chip_select_max_number;
> +	le8 cs_timing_setting_enable;
> +	le8 reserved[3];
> +};
> +\end{lstlisting}
> +
> +The \field{bus_num} indicates the physical SPI master assigned to guest.
> +
> +The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
> +
> +The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
> +	0: physical SPI master doesn't support cs timing setting;
> +	1: physical SPI master supports cs timing setting.
> +
> +The \field{reserved} is for alignment purpose, also for future extension.
> +
> +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
> +
> +\begin{enumerate}
> +\item The Virtio SPI driver configures and initializes the virtqueue.
> +\end{enumerate}
> +
> +\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}
> +
> +Virtio SPI driver enqueues requests to the virtqueue, and they are used by
> +Virtio SPI device. Each request represents one SPI tranfer and is of the form:
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +        u8 slave_id;
> +        u8 bits_per_word;
> +        u8 cs_change;
> +        u8 tx_nbits;
> +        u8 rx_nbits;
> +	 u8 reserved[3];
> +        le32 mode;
> +        le32 freq;
> +        le32 word_delay_ns;
> +        le32 cs_setup_ns;
> +        le32 cs_delay_hold_ns;
> +        le32 cs_change_delay_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_result {
> +        u8 result;
> +};
> +\end{lstlisting}
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_req {
> +        struct virtio_spi_transfer_head head;
> +        u8 tx_buf[];
> +        u8 rx_buf[];
> +        struct virtio_spi_transfer_result result;
> +};
> +\end{lstlisting}
> +
> +The \field{slave_id} indicates the chipselect index the SPI transfer used.
> +
> +The \field{bits_per_word} indicates the number of bits in each SPI transfer word.
> +
> +The \field{cs_change} indicates whether to deselect device before starting the
> +next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
> +then asserted again.
> +
> +The \field{tx_nbits} indicates 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.
> +
> +The \field{rx_nbits} indicates 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.
> +
> +The \field{reserved} is for alignement, also for further extension.
> +
> +The \field{mode} indicates how data is clocked out and in. Bit definitions as follows:
> +        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
> +	       relative to the clock pulses.
> +        bit 1: CPOL, determines the polarity of the clock.
> +        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
> +
> +The \field{freq} indicates the SPI transfer speed in Hz.
> +
> +The \field{word_delay_ns} indicates delay to be inserted between consecutive
> +words of a transfer, in ns unit.
> +
> +The \field{cs_setup_ns} indicates delay to be introduced after chipselect
> +is asserted, in ns unit.
> +
> +The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
> +is deasserted, in ns unit.
> +
> +The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
> +chipselect is deasserted and before next asserted, in ns unit.
> +
> +The \field{tx_buf} is the buffer for data sent to the device.
> +
> +The \field{rx_buf} is the buffer for data received to the device.
> +
> +The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
> +or VIRTIO_SPI_TRANS_ERR for error.
> +
> +\begin{lstlisting}
> +#define VIRTIO_SPI_TRANS_OK     0
> +#define VIRTIO_SPI_TRANS_ERR    1
> +\end{lstlisting}
> +
> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
> +
> +Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
> +\field{result} in structure \field{virtio_spi_transfer_result} is written by 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 Virtio SPI device and consumed
> +by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio
> +SPI driver and consumed by 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 transfer requests on the requestq virtqueue.
> +
> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
> +and MUST be readable for Virtio SPI device.
> +
> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
> +and MUST be writable for Virtio SPI device.
> +
> +For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
> +\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
> +
> +For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
> +\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
> +
> +For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
> +\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
> +
> +For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
> +if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
> +
> +If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
> +\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
> +SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.
> +
> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
> +
> +Virtio SPI device MUST set all the fields of the structure \field{virtio_spi_config} before
> +they are read by Virtio SPI driver.
> +
> +Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result} before sending
> +it back to Virtio SPI driver.
> +
> +Virtio SPI device MUST be able to identify the transfer type according to the received
> +virtqueue descriptors.
> +
> +Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
> --- /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}
> +
> +An 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..3c965ef
> --- /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}
> +
> +An SPI Master driver MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
> +\end{itemize}

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
  2023-10-24 12:53 [virtio-dev] [PATCH v4] virtio-spi: add the device specification Haixu Cui
  2023-10-24 13:08 ` [virtio-dev] " Haixu Cui
@ 2023-11-07  7:40 ` Qiang Zhang
  2023-11-09  2:52   ` Haixu Cui
       [not found] ` <56fbe6cc-d7a5-438f-1dd2-939f4197a970@opensynergy.com>
       [not found] ` <ZUjrDjHpFVReDSgk@finisterre.sirena.org.uk>
  3 siblings, 1 reply; 12+ messages in thread
From: Qiang Zhang @ 2023-11-07  7:40 UTC (permalink / raw)
  To: Haixu Cui; +Cc: virtio-dev, virtio-comment, harald.mommer, cohuck, quic_ztu

On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
>virtio-spi is a virtual SPI master and it allows a guest to operate and
>use the physical SPI master controlled by the host.
>
>This patch adds the specification for virtio-spi.
>
>Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>---
> device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
> device-types/spi/device-conformance.tex |   7 +
> device-types/spi/driver-conformance.tex |   7 +
> 3 files changed, 220 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..5dbceaf
>--- /dev/null
>+++ b/device-types/spi/description.tex
>@@ -0,0 +1,206 @@
>+\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 controlled by the host.
>+
>+virtio-spi has a single virtqueue. SPI transfer requests are placed into
>+the virtqueue, and serviced by the physical SPI master.
>+
>+In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>+the front-end existing in the guest kernel, and Virtio SPI device acts as the
>+back-end in the host platform.
>+
>+\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 Virtio SPI driver.
>+
>+\begin{lstlisting}
>+struct virtio_spi_config {
>+	le16 bus_num;
>+	le16 chip_select_max_number;
>+	le8 cs_timing_setting_enable;
>+	le8 reserved[3];
>+};
>+\end{lstlisting}
>+
>+The \field{bus_num} indicates the physical SPI master assigned to guest.

Does this assume that a Virtio SPI controller always corresponds to a physical
SPI controller?
What if the backend SPI controller and SPI devices are fully emulated or the
SPI devices under the Virtio SPI controller come from multiple physical SPI
controllers?

Regards,
Qiang

>+
>+The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
>+
>+The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
>+	0: physical SPI master doesn't support cs timing setting;
>+	1: physical SPI master supports cs timing setting.
>+
>+The \field{reserved} is for alignment purpose, also for future extension.
>+
>+\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
>+
>+\begin{enumerate}
>+\item The Virtio SPI driver configures and initializes the virtqueue.
>+\end{enumerate}
>+
>+\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}
>+
>+Virtio SPI driver enqueues requests to the virtqueue, and they are used by
>+Virtio SPI device. Each request represents one SPI tranfer and is of the form: 
>+
>+\begin{lstlisting}
>+struct virtio_spi_transfer_head {
>+        u8 slave_id;
>+        u8 bits_per_word;
>+        u8 cs_change;
>+        u8 tx_nbits;
>+        u8 rx_nbits;
>+	 u8 reserved[3];
>+        le32 mode;
>+        le32 freq;
>+        le32 word_delay_ns;
>+        le32 cs_setup_ns;
>+        le32 cs_delay_hold_ns;
>+        le32 cs_change_delay_inactive_ns;
>+};
>+\end{lstlisting}
>+
>+\begin{lstlisting}
>+struct virtio_spi_transfer_result {
>+        u8 result;
>+};
>+\end{lstlisting}
>+
>+\begin{lstlisting}
>+struct virtio_spi_transfer_req {
>+        struct virtio_spi_transfer_head head;
>+        u8 tx_buf[];
>+        u8 rx_buf[];
>+        struct virtio_spi_transfer_result result;
>+};
>+\end{lstlisting}
>+
>+The \field{slave_id} indicates the chipselect index the SPI transfer used.
>+
>+The \field{bits_per_word} indicates the number of bits in each SPI transfer word.
>+
>+The \field{cs_change} indicates whether to deselect device before starting the
>+next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
>+then asserted again.
>+
>+The \field{tx_nbits} indicates 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.
>+
>+The \field{rx_nbits} indicates 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.
>+
>+The \field{reserved} is for alignement, also for further extension.
>+
>+The \field{mode} indicates how data is clocked out and in. Bit definitions as follows:
>+        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
>+	       relative to the clock pulses.
>+        bit 1: CPOL, determines the polarity of the clock.
>+        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
>+
>+The \field{freq} indicates the SPI transfer speed in Hz.
>+
>+The \field{word_delay_ns} indicates delay to be inserted between consecutive
>+words of a transfer, in ns unit.
>+
>+The \field{cs_setup_ns} indicates delay to be introduced after chipselect
>+is asserted, in ns unit.
>+
>+The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
>+is deasserted, in ns unit.
>+
>+The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
>+chipselect is deasserted and before next asserted, in ns unit.
>+
>+The \field{tx_buf} is the buffer for data sent to the device.
>+
>+The \field{rx_buf} is the buffer for data received to the device.
>+
>+The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
>+or VIRTIO_SPI_TRANS_ERR for error.
>+
>+\begin{lstlisting}
>+#define VIRTIO_SPI_TRANS_OK     0
>+#define VIRTIO_SPI_TRANS_ERR    1
>+\end{lstlisting}
>+
>+\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
>+
>+Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
>+\field{result} in structure \field{virtio_spi_transfer_result} is written by 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 Virtio SPI device and consumed
>+by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio 
>+SPI driver and consumed by 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 transfer requests on the requestq virtqueue.
>+
>+Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
>+and MUST be readable for Virtio SPI device.
>+
>+Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
>+and MUST be writable for Virtio SPI device.
>+
>+For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>+\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>+
>+For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>+\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>+
>+For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head}, 
>+\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>+
>+For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
>+if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
>+
>+If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
>+\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
>+SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.
>+
>+\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>+
>+Virtio SPI device MUST set all the fields of the structure \field{virtio_spi_config} before
>+they are read by Virtio SPI driver.
>+
>+Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result} before sending
>+it back to Virtio SPI driver.
>+
>+Virtio SPI device MUST be able to identify the transfer type according to the received
>+virtqueue descriptors.
>+
>+Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
>--- /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}
>+
>+An 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..3c965ef
>--- /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}
>+
>+An 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
>

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [virtio-dev] Re: [PATCH v4] virtio-spi: add the device specification
       [not found] ` <56fbe6cc-d7a5-438f-1dd2-939f4197a970@opensynergy.com>
@ 2023-11-07 17:10   ` Haixu Cui
  0 siblings, 0 replies; 12+ messages in thread
From: Haixu Cui @ 2023-11-07 17:10 UTC (permalink / raw)
  To: Harald Mommer, virtio-dev, virtio-comment, cohuck
  Cc: quic_ztu, Matti Moell, Mikhail Golubev

Hi Harald,
     Please refer to my following replies to your comments. Thank you 
very much.

On 10/27/2023 9:02 PM, Harald Mommer wrote:
> Hello Haixu,
> 
> this delay stuff causes some headache.
> 
> On 24.10.23 14:53, Haixu Cui wrote:
>> virtio-spi is a virtual SPI master and it allows a guest to operate and
>> use the physical SPI master controlled by the host.
>>
>> This patch adds the specification for virtio-spi.
>>
>> Signed-off-by: Haixu Cui<quic_haixcui@quicinc.com>
>> ---
>>   device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 +
>>   device-types/spi/driver-conformance.tex |   7 +
>>   3 files changed, 220 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..5dbceaf
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,206 @@
>> +\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 controlled by the 
>> host.
>> +
>> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
>> +the virtqueue, and serviced by the physical SPI master.
>> +
>> +In a typical host and guest architecture with virtio-spi, Virtio SPI 
>> driver is
>> +the front-end existing in the guest kernel, and Virtio SPI device 
>> acts as the
>> +back-end in the host platform.
>> +
>> +\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 Virtio SPI driver.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +    le16 bus_num;
>> +    le16 chip_select_max_number;
>> +    le8 cs_timing_setting_enable;
> The naming "cs_timing_setting_enable" may be sub-optimal as also timings 
> which have nothing to do with CS (chip select) are affected. See below 
> where it's discussed in more detail.

For delay parameters(including cs delay setting), see below, a big topic.
>> +    le8 reserved[3];
> 
> There is no "le8". This is "u8".
yes! u8, i made a mistake

> 
> BTW, I currently in my Linux driver code announce support for 8 and 16 
> bit SPI word length out of the blue, 125000000 bps transfer speed out of 
> the blue not knowing what my back end device really supports. In struct 
> spi_master the mode member is set to all kinds of bits hoping for the 
> best without really knowing whether the virtio SPI device supports all 
> of this.
> 
> For a first specification version this may be acceptable that there is 
> announced something based on nothing but I already see that the 
> information provided in the config space will not be the last word for 
> all times looking at my code.
> 
I also consider more fields in config space, or introduce another 
virtqueue to exchange some information between front and back, this will 
let front-end knows more about the virtio SPI device. And this needs 
more investigation.

>> +};
>> +\end{lstlisting}
>> +
>> +The \field{bus_num} indicates the physical SPI master assigned to guest.
>> +
>> +The \field{chip_select_max_number} is the maximum number of 
>> chipselect the physical SPI master supports.
>> +
>> +The \field{cs_timing_setting_enable} indicates if the physical SPI 
>> master supporting cs timing setting:
>> +    0: physical SPI master doesn't support cs timing setting;
>> +    1: physical SPI master supports cs timing setting.
>> +
>> +The \field{reserved} is for alignment purpose, also for future 
>> extension.
>> +
>> +\subsection{Device Initialization}\label{sec:Device Types / SPI 
>> Master Device / Device Initialization}
>> +
>> +\begin{enumerate}
>> +\item The Virtio SPI driver configures and initializes the virtqueue.
>> +\end{enumerate}
>> +
>> +\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}
>> +
>> +Virtio SPI driver enqueues requests to the virtqueue, and they are 
>> used by
>> +Virtio SPI device. Each request represents one SPI tranfer and is of 
>> the form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 slave_id;
>> +        u8 bits_per_word;
>> +        u8 cs_change;
>> +        u8 tx_nbits;
>> +        u8 rx_nbits;
>> +     u8 reserved[3];
> You may not see it in your E-Mail client, but there is a tab in front of 
> "u8 reserved[3];" instead of spaces. This destroys the formatting of the 
> generated PDF.

Will avoid alignment case in next patch.

>> +        le32 mode;
>> +        le32 freq;
>> +        le32 word_delay_ns;
>> +        le32 cs_setup_ns;
>> +        le32 cs_delay_hold_ns;
>> +        le32 cs_change_delay_inactive_ns;
>> +};
>> +\end{lstlisting}
> 
> I see a virtqueue as a communication channel over which all kinds of 
> messages can be transferred. The existing message may have to be 
> enhanced in the future in some way nobody today thinks about with the 
> outcome that we may have in the future the need to distinguish between 
> different kinds of messages.
> 
> Adding a flags field or a message type at the top of struct 
> virtio_spi_transfer_head to become more future proof? It may never be 
> needed, so I'm also fine without this small addition but it is worth to 
> think about this for some minutes.
> 

I have considered this topic before, introducing another config 
virtqueue, used to exchange config information. Just some tips:
1) all parameters in config virtqueue can be held in spi transfer header.
2) two virtqueues may have conflicts?
3) will make the driver code more complicated.
4) hard to define which parameters passed via config queue and which 
parameters passed via transfer queue.

>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_result {
>> +        u8 result;
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_req {
>> +        struct virtio_spi_transfer_head head;
>> +        u8 tx_buf[];
>> +        u8 rx_buf[];
>> +        struct virtio_spi_transfer_result result;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{slave_id} indicates the chipselect index the SPI transfer 
>> used.
>> +
>> +The \field{bits_per_word} indicates the number of bits in each SPI 
>> transfer word.
>> +
>> +The \field{cs_change} indicates whether to deselect device before 
>> starting the
>> +next SPI transfer, 0 means chipselect keep asserted and 1 means 
>> chipselect deasserted
>> +then asserted again.
>> +
>> +The \field{tx_nbits} indicates 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.
>> +
>> +The \field{rx_nbits} indicates 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.
>> +
>> +The \field{reserved} is for alignement, also for further extension.
> 
> alignment
yes.
> 
>> +
>> +The \field{mode} indicates how data is clocked out and in. Bit 
>> definitions as follows:
>> +        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
>> +           relative to the clock pulses.
>> +        bit 1: CPOL, determines the polarity of the clock.
>> +        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
>> +
>> +The \field{freq} indicates the SPI transfer speed in Hz.
>> +
>> +The \field{word_delay_ns} indicates delay to be inserted between 
>> consecutive
>> +words of a transfer, in ns unit.
>> +
>> +The \field{cs_setup_ns} indicates delay to be introduced after 
>> chipselect
>> +is asserted, in ns unit.
>> +
>> +The \field{cs_delay_hold_ns} indicates delay to be introduced before 
>> chipselect
>> +is deasserted, in ns unit.
>> +
>> +The \field{cs_change_delay_inactive_ns} indicates delay to be 
>> introduced after
>> +chipselect is deasserted and before next asserted, in ns unit.
>> +
>> +The \field{tx_buf} is the buffer for data sent to the device.
>> +
>> +The \field{rx_buf} is the buffer for data received to the device.
>> +
>> +The final \field{result} is the transfer result, either 
>> VIRTIO_SPI_TRANS_OK for success
>> +or VIRTIO_SPI_TRANS_ERR for error.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_TRANS_OK     0
>> +#define VIRTIO_SPI_TRANS_ERR    1
>> +\end{lstlisting}
>> +
>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device 
>> Types / SPI Master Device / Device Operation: Operation Status}
>> +
>> +Fields in structure \field{virtio_spi_transfer_head} are written by 
>> Virtio SPI driver, while
>> +\field{result} in structure \field{virtio_spi_transfer_result} is 
>> written by 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 Virtio SPI 
>> device and consumed
>> +by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} 
>> is filled by Virtio
>> +SPI driver and consumed by 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 transfer requests on the requestq 
>> virtqueue.
>> +
>> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled 
>> by Virtio SPI driver
>> +and MUST be readable for Virtio SPI device.
>> +
>> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio 
>> SPI device
>> +and MUST be writable for Virtio SPI device.
>> +
>> +For half-duplex read, Virtio SPI driver MUST send structure 
>> \field{virtio_spi_transfer_head},
>> +\field{rx_buf} and structure \field{virtio_spi_transfer_result} to 
>> SPI Virtio Device in order.
>> +
>> +For half-duplex write, Virtio SPI driver MUST send structure 
>> \field{virtio_spi_transfer_head},
>> +\field{tx_buf} and structure \field{virtio_spi_transfer_result} to 
>> SPI Virtio Device in order.
>> +
>> +For full-duplex read and write, Virtio SPI driver MUST send structure 
>> \field{virtio_spi_transfer_head},
>> +\field{tx_buf}, \field{rx_buf} and structure 
>> \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> +
>> +For half-duplex write or full-duplex read and write transfer, Virtio 
>> SPI driver MUST not use \field{rx_buf}
>> +if the \field{result} returned from Virtio SPI device is 
>> VIRTIO_SPI_TRANS_ERR.
>> +
>> +If \field{cs_timing_setting_enable} in structure 
>> \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
>> +\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the 
>> transfer are not all zero, Virtio
>> +SPI driver MUST print a warning log to alert that the cs timing won't 
>> be set as expected.
> 
> We are talking about
> 
>      __le32 word_delay_ns;
>      __le32 cs_setup_ns;
>      __le32 cs_delay_hold_ns;
>      __le32 cs_change_delay_inactive_ns;
> 
> 1.) cs_setup_ns is a duplicate. cs_delay_hold_ns is missing. 
> word_delay_ns is also missing!
yes!
> 
> 2.) There is on Linux no problem with cs_change_delay_inactive_ns. For 
> e.g. 4.14 this is in struct spi_transfer xfer the member delay_usecs, 
> for latest kernels this is delay (or cs_change_delay, currently unsure). 
> There should also be no problem to have this because on every platform 
> after a SPI transaction has been completed such a delay could be 
> introduced simply by calling some Delay() function. This one probably 
> can be supported everywhere.
> 
> 2.) The most important field, le32 word_delay_ns is also missing.
> 
>        - This one has nothing to do with CS, so the config space name 
> prefixed by cs_ is misnamed
>        - I have no idea how to support this word_delay_ns on older Linux 
> versions and on some hardware platforms
> 

I want separate these 4 values into 2 groups:

     group 1: word_delay_ns;
     group 2: cs_setup_ns, cs_delay_hold_ns, cs_change_delay_inactive_ns

group 1 is unrelated to CS, cause when this group takes effect, CS keeps 
as active.

group 2 always takes effect when CS changes, asserted or deasserted.

Considering this rules, cs_timing_setting_enable in config space only 
apply to group 2. group 1 can be easily supported by the virtio SPI 
device, by software Delay() and so on. But for group 2, not all SPI 
controllers support CS timing delay setting, software cannot do this even.

Maybe I should explain clearly in cs_timing_setting_enable field?

> 3.) Logging a warning. This is an unusual requirement. When the driver 
> can know from the config space it should handle according to the 
> information it got from there. I propose to get somewhat harder here.
> 
> Device requirement: The device MUST reject a message by some tbd error 
> if tbd delay timing is not supported but the requested value is not zero .
> 
> Driver requirement: If the device did not announce support of tbd delay 
> timings in the config space the driver SHOULD not sent a delay timing 
> not equal to zero but should immediately reject the message.
> 

Yes! I will update in next patch, stricter and harder seem much better.

> And I think best is not to define 0 and 1 for cs_timing_setting_enable but
> 
> #define SPI_HAVE_WORD_DELAY 0x01
> #define SPI_HAVE_CS_SETUP 0x02
> #define SPI_HAVE_CS_HOLD 0x04
> #define SPI_HAVE_CS_CHANGE_INACTIVE 0x08 /* Only if not required to be 
> supported always by the device */
> 
> so the driver knows the delays supported by the device exactly.
> 

good idea, each bit represents one cs timing parameter.

> BTW, in my driver code I have currently still a problem with the 
> mappings of
> 
>      __le32 word_delay_ns;
>      __le32 cs_setup_ns;
>      __le32 cs_delay_hold_ns;
>      __le32 cs_change_delay_inactive_ns;
> 
> to Linux struct spi_transfer word_delay, delay, cs_change_delay.
> 
> 4 values to be filled in struct spi_transfer_head from 3 values in 
> struct spi_transfer of latest Linux. Maybe I just did not get from where 
> to get cleanly the 4th value even from the newest Linux driver environment.
> 
> We have between Linux 4.14 and latest some intermediate states in struct 
> spi_transfer and someone (could be me) may have to cope with this.

I look into the latest Linux spi driver, and find these delay values:
     struct spi_device -> word_delay
     struct spi_device -> cs_setup
     struct spi_device -> cs_hold
     struct spi_device -> cs_inactive
     struct spi_transfer -> delay
     struct spi_transfer -> cs_change_delay
     struct spi_transfer -> word_delay

I take spi_transfer_one_message function in drivers/spi/spi.c to analyze 
these values take effect at which stage. Consider a condition, one 
spi_message contains two spi_transfer, cs is controlled as GPIO, and for 
the first transfer, cs_change is true which for the second cs_change 
false. Just ingoring some strange setting not used commonly, such as cs_off.

       .   .      .    .    .   .   .   .   .   .
Delay + A +      + B  +    + C + D + E + F + A +
       .   .      .    .    .   .   .   .   .   .
    ___.   .      .    .    .   .   .___.___.   .
CS#   |___.______.____.____.___.___|   .   |___._____________
       .   .      .    .    .   .   .   .   .   .
       .   .      .    .    .   .   .   .   .   .
SCLK__.___.___NNN_____NNN__.___.___.___.___.___.___NNN_______


NOTE: 1st transfer has two words, the delay betweent these two words are 
'B' in the diagram.

A => struct spi_device -> cs_setup
B => max{struct spi_transfer -> word_delay,
          struct spi_device -> word_delay}
     Note: spi_device and spi_transfer both have word_delay, Linux
          choose the bigger one, refer to _spi_xfer_word_delay_update 

          function
C => struct spi_transfer -> delay
D => struct spi_device -> cs_hold
E => struct spi_device -> cs_inactive
F => struct spi_transfer -> cs_change_delay

So the corresponding relationship:
A <===> cs_setup_ns (after CS asserted)
B <===> word_delay_ns (no matter with CS)
C+D <===> cs_delay_hold_ns (before CS deasserted)
E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two 
values also recommend in Linux driver to be added up)

It's quite possible that the host is not Linux and doesn't recognize 
these separate paramters, I add the values at the same stages(C and D, E 
and F), this makes sense I think.

> 
>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI 
>> Master Device / Device Operation}
>> +
>> +Virtio SPI device MUST set all the fields of the structure 
>> \field{virtio_spi_config} before
>> +they are read by Virtio SPI driver.
>> +
>> +Virtio SPI device MUST set the structure 
>> \field{virtio_spi_transfer_result} before sending
>> +it back to Virtio SPI driver.
>> +
>> +Virtio SPI device MUST be able to identify the transfer type 
>> according to the received
>> +virtqueue descriptors.
>> +
>> +Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
>> --- /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}
>> +
>> +An 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..3c965ef
>> --- /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}
>> +
>> +An SPI Master driver MUST conform to the following normative statements:
>> +
>> +\begin{itemize}
>> +\item \ref{drivernormative:Device Types / SPI Master Device / Device 
>> Operation}
>> +\end{itemize}
> 
> We may discuss further based on code, internal paperwork done so the 
> Linux driver which was done internally at OpenSynergy can go out now as 
> RFC as well.

Sure. Looking forward to discussing futher based on code and spec.

Thanks again for your comments and ideas.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
       [not found] ` <ZUjrDjHpFVReDSgk@finisterre.sirena.org.uk>
@ 2023-11-08 15:42   ` Haixu Cui
       [not found]     ` <ZUuxfbkSboAFQIQF@finisterre.sirena.org.uk>
  0 siblings, 1 reply; 12+ messages in thread
From: Haixu Cui @ 2023-11-08 15:42 UTC (permalink / raw)
  To: Mark Brown; +Cc: virtio-dev, virtio-comment, harald.mommer, cohuck, quic_ztu

Hi Mark,
     Thank you for your comments, my response is below each of your 
comments.

On 11/6/2023 9:33 PM, Mark Brown wrote:
> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
> 
>> virtio-spi is a virtual SPI master and it allows a guest to operate and
>> use the physical SPI master controlled by the host.
> 
> It would be good to avoid introducing new uses of this outdated
> terminology, terms like controller are better.  This is especially true
> for specifications.

Yes! Will use "controller" instead of "master" in next patch.
> 
> It would be good to be copied on future versions of this.
> 
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +	le16 bus_num;
>> +	le16 chip_select_max_number;
> 
> That's a *lot* of potential buses and chip selects...

Here you mean the types of bus_num and chip_select_max_number are "le16"?

Here I use 16 bits because bus_num and num_chipselect in spi_controller 
structure are both 16 bits.

> 
>> +The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
>> +	0: physical SPI master doesn't support cs timing setting;
>> +	1: physical SPI master supports cs timing setting.
> 
> This is very vauge - what exactly is meant by "cs timing setting"?  If
> we're specifying anything about timing it should probably be specific
> about which paramters might be controlled and the constraints on how
> they can be set.  It's also a bit surprising that the only parameter
> here is around chip select, there's generally a lot more sensitivity
> around the main clock.

Here I would like to say the support of 3 types of cs delay, that is, 
delay after cs asserted, delay before cs deasserted, delay after cs 
deasserted.

I will update as follows:

The \field{cs_timing_setting_enable} indicates if the host SPI 
controller supports cs delay setting when performing transfer:
         bit 0: delay after cs asserted, 0 means doesn't support and 1
                means support;
         bit 1: delay before cs deasserted, 0 means doesn't support and 1
                means support;
         bit 2: delay after cs deasserted, 0 means doesn't support and 1
                means support;
         other bits are reserved and always set as 0.


> 
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 slave_id;
> 
> More outdated terminology.
> 
>> +        u8 bits_per_word;
>> +        u8 cs_change;
>> +        u8 tx_nbits;
>> +        u8 rx_nbits;
>> +	 u8 reserved[3];
> 
> Indentation issue?

Yes, will fix in next patch.
> 
>> +        le32 mode;
>> +        le32 freq;
>> +        le32 word_delay_ns;
>> +        le32 cs_setup_ns;
>> +        le32 cs_delay_hold_ns;
>> +        le32 cs_change_delay_inactive_ns;
> 
> All these parameters depend on controller support but weren't included
> in the feature enumeration above and nothing in the spec seems to cover
> what happens when they can't be supported exactly.

Add another return value:
     #define VIRTIO_SPI_PARAM_ERR    1

If the parameters in \field{virtio_spi_transfer_head} are not all valid, 
or some fields are set as non-zero values but the corresponding features 
are not supported by host, then the backend return VIRTIO_SPI_PARAM_ERR 
to the frontend.

> 
>> +The \field{cs_change} indicates whether to deselect device before starting the
>> +next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
>> +then asserted again.
> 
> Note that this is *not* how Linux implements things, and it doesn't
> include any specification of timing constraints.
> 
>> +The \field{tx_nbits} indicates 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.
> 
> Width here being in terms of bits.
> 
Updated as:
0,1: 1-bit transfer, also known as SINGLE;
...

>> +The \field{tx_buf} is the buffer for data sent to the device.
>> +
>> +The \field{rx_buf} is the buffer for data received to the device.
> 
> In what format are these buffers presented?  I note that the spec
> supports more than one byte per word (and non-integer number of bytes).

In spi_transfer struct:
     @len: size of rx and tx buffers (in bytes)

Although the bits_per_word can be any value in 0..32, in-memory 
wordsizes are powers of two bytes (e.g. 20 bit samples use 32 bits).
So:
     0 < bits_per_word <=8: one word occupies 1 byte in memory;
     8 < bits_per_word <=16: one word occupies 2 bytes in memory;
     16 < bits_per_word <=32: one word occupies 4 bytes in memory.

In the spec, the format of buffers are u8 array,
     u8 tx_buf[];
     u8 rx_buf[];


Thanks again for your comments and ideas.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
       [not found]     ` <ZUuxfbkSboAFQIQF@finisterre.sirena.org.uk>
@ 2023-11-08 18:37       ` Harald Mommer
  2023-11-09 11:21       ` Haixu Cui
  1 sibling, 0 replies; 12+ messages in thread
From: Harald Mommer @ 2023-11-08 18:37 UTC (permalink / raw)
  To: Mark Brown, Haixu Cui; +Cc: virtio-dev, virtio-comment, cohuck, quic_ztu

On 08.11.23 17:04, Mark Brown wrote:
> On Wed, Nov 08, 2023 at 11:42:40PM +0800, Haixu Cui wrote:
>> On 11/6/2023 9:33 PM, Mark Brown wrote:
>>> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
>>>> +	le16 bus_num;
>>>> +	le16 chip_select_max_number;
>>> That's a *lot* of potential buses and chip selects...
>> Here you mean the types of bus_num and chip_select_max_number are "le16"?
>> Here I use 16 bits because bus_num and num_chipselect in spi_controller
>> structure are both 16 bits.
> Yeah, I think that's more for padding/alignment reasons than anything
> else.  TBH I wondered if just going for a standard int might be better,
> it's the combination of picking a smaller type but one that's relatively
> large for the domain.

We cannot use int at interfaces between device and driver in the virtio 
world:

- "int" is machine dependent, sizeof may be 2, 4 or 8.

- It is signed which would be new for the virtio specification. Not done 
for other devices.

- How are signs represented? 2s complement? Strange machines do not use 
2s complement. Good is that most of those strange machines have still 
tubes and are by now obsolete.

- The endianess is machine dependent and device and driver may use 
internally a different native byte ordering. Strange but this is imaginable.

le16 is a well defined unsigned type with sizeof 2 bytes and endianess 
defined as "little endian".

> What byte ordering are the multi-byte words in - CPU native or a
> particular endianness (eg, wire native)?

Virtio uses little endian. (Some data structures in the RPMB device use 
big endian. But this is an exception and there this was done for a good 
reason.)


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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
  2023-11-07  7:40 ` [virtio-dev] " Qiang Zhang
@ 2023-11-09  2:52   ` Haixu Cui
  2023-11-09  3:39     ` Qiang Zhang
  0 siblings, 1 reply; 12+ messages in thread
From: Haixu Cui @ 2023-11-09  2:52 UTC (permalink / raw)
  To: Qiang Zhang; +Cc: virtio-dev, virtio-comment, harald.mommer, cohuck, quic_ztu



On 11/7/2023 3:40 PM, Qiang Zhang wrote:
> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
>> virtio-spi is a virtual SPI master and it allows a guest to operate and
>> use the physical SPI master controlled by the host.
>>
>> This patch adds the specification for virtio-spi.
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> ---
>> device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
>> device-types/spi/device-conformance.tex |   7 +
>> device-types/spi/driver-conformance.tex |   7 +
>> 3 files changed, 220 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..5dbceaf
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,206 @@
>> +\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 controlled by the host.
>> +
>> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
>> +the virtqueue, and serviced by the physical SPI master.
>> +
>> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>> +the front-end existing in the guest kernel, and Virtio SPI device acts as the
>> +back-end in the host platform.
>> +
>> +\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 Virtio SPI driver.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +	le16 bus_num;
>> +	le16 chip_select_max_number;
>> +	le8 cs_timing_setting_enable;
>> +	le8 reserved[3];
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{bus_num} indicates the physical SPI master assigned to guest.
> 
> Does this assume that a Virtio SPI controller always corresponds to a physical
> SPI controller?
> What if the backend SPI controller and SPI devices are fully emulated or the
> SPI devices under the Virtio SPI controller come from multiple physical SPI
> controllers?
> 
> Regards,
> Qiang
> 

Hi Qiang,

     It's quite possible the host SPI controller is not physical, but 
virtualized controller. In next patch, I will use "host SPI controller" 
to replace "physical SPI controller", "host SPI controller" represents 
all kinds of controller controlled by the host, which is more appropriate.

     A Virtio SPI controller corresponds with a host SPI controller, and 
the guest can only access the devices under this host SPI controller, 
with the assigned cs. For devices under other host controllers, the 
guest cannot access.

     Thank you for your findings and comments.

Best Regards
Haixu Cui


>> +
>> +The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
>> +
>> +The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
>> +	0: physical SPI master doesn't support cs timing setting;
>> +	1: physical SPI master supports cs timing setting.
>> +
>> +The \field{reserved} is for alignment purpose, also for future extension.
>> +
>> +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
>> +
>> +\begin{enumerate}
>> +\item The Virtio SPI driver configures and initializes the virtqueue.
>> +\end{enumerate}
>> +
>> +\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}
>> +
>> +Virtio SPI driver enqueues requests to the virtqueue, and they are used by
>> +Virtio SPI device. Each request represents one SPI tranfer and is of the form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 slave_id;
>> +        u8 bits_per_word;
>> +        u8 cs_change;
>> +        u8 tx_nbits;
>> +        u8 rx_nbits;
>> +	 u8 reserved[3];
>> +        le32 mode;
>> +        le32 freq;
>> +        le32 word_delay_ns;
>> +        le32 cs_setup_ns;
>> +        le32 cs_delay_hold_ns;
>> +        le32 cs_change_delay_inactive_ns;
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_result {
>> +        u8 result;
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_req {
>> +        struct virtio_spi_transfer_head head;
>> +        u8 tx_buf[];
>> +        u8 rx_buf[];
>> +        struct virtio_spi_transfer_result result;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{slave_id} indicates the chipselect index the SPI transfer used.
>> +
>> +The \field{bits_per_word} indicates the number of bits in each SPI transfer word.
>> +
>> +The \field{cs_change} indicates whether to deselect device before starting the
>> +next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
>> +then asserted again.
>> +
>> +The \field{tx_nbits} indicates 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.
>> +
>> +The \field{rx_nbits} indicates 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.
>> +
>> +The \field{reserved} is for alignement, also for further extension.
>> +
>> +The \field{mode} indicates how data is clocked out and in. Bit definitions as follows:
>> +        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
>> +	       relative to the clock pulses.
>> +        bit 1: CPOL, determines the polarity of the clock.
>> +        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
>> +
>> +The \field{freq} indicates the SPI transfer speed in Hz.
>> +
>> +The \field{word_delay_ns} indicates delay to be inserted between consecutive
>> +words of a transfer, in ns unit.
>> +
>> +The \field{cs_setup_ns} indicates delay to be introduced after chipselect
>> +is asserted, in ns unit.
>> +
>> +The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
>> +is deasserted, in ns unit.
>> +
>> +The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
>> +chipselect is deasserted and before next asserted, in ns unit.
>> +
>> +The \field{tx_buf} is the buffer for data sent to the device.
>> +
>> +The \field{rx_buf} is the buffer for data received to the device.
>> +
>> +The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
>> +or VIRTIO_SPI_TRANS_ERR for error.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_TRANS_OK     0
>> +#define VIRTIO_SPI_TRANS_ERR    1
>> +\end{lstlisting}
>> +
>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
>> +
>> +Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
>> +\field{result} in structure \field{virtio_spi_transfer_result} is written by 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 Virtio SPI device and consumed
>> +by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio
>> +SPI driver and consumed by 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 transfer requests on the requestq virtqueue.
>> +
>> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
>> +and MUST be readable for Virtio SPI device.
>> +
>> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
>> +and MUST be writable for Virtio SPI device.
>> +
>> +For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> +\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> +
>> +For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> +\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> +
>> +For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> +\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> +
>> +For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
>> +if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
>> +
>> +If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
>> +\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
>> +SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.
>> +
>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>> +
>> +Virtio SPI device MUST set all the fields of the structure \field{virtio_spi_config} before
>> +they are read by Virtio SPI driver.
>> +
>> +Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result} before sending
>> +it back to Virtio SPI driver.
>> +
>> +Virtio SPI device MUST be able to identify the transfer type according to the received
>> +virtqueue descriptors.
>> +
>> +Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
>> --- /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}
>> +
>> +An 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..3c965ef
>> --- /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}
>> +
>> +An 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
>>

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
  2023-11-09  2:52   ` Haixu Cui
@ 2023-11-09  3:39     ` Qiang Zhang
  2023-11-09 10:21       ` Haixu Cui
  0 siblings, 1 reply; 12+ messages in thread
From: Qiang Zhang @ 2023-11-09  3:39 UTC (permalink / raw)
  To: Haixu Cui; +Cc: virtio-dev, virtio-comment, harald.mommer, cohuck, quic_ztu

On Thu, Nov 09, 2023 at 10:52:06AM +0800, Haixu Cui wrote:
>
>
>On 11/7/2023 3:40 PM, Qiang Zhang wrote:
>> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
>> > virtio-spi is a virtual SPI master and it allows a guest to operate and
>> > use the physical SPI master controlled by the host.
>> > 
>> > This patch adds the specification for virtio-spi.
>> > 
>> > Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> > ---
>> > device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
>> > device-types/spi/device-conformance.tex |   7 +
>> > device-types/spi/driver-conformance.tex |   7 +
>> > 3 files changed, 220 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..5dbceaf
>> > --- /dev/null
>> > +++ b/device-types/spi/description.tex
>> > @@ -0,0 +1,206 @@
>> > +\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 controlled by the host.
>> > +
>> > +virtio-spi has a single virtqueue. SPI transfer requests are placed into
>> > +the virtqueue, and serviced by the physical SPI master.
>> > +
>> > +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>> > +the front-end existing in the guest kernel, and Virtio SPI device acts as the
>> > +back-end in the host platform.
>> > +
>> > +\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 Virtio SPI driver.
>> > +
>> > +\begin{lstlisting}
>> > +struct virtio_spi_config {
>> > +	le16 bus_num;
>> > +	le16 chip_select_max_number;
>> > +	le8 cs_timing_setting_enable;
>> > +	le8 reserved[3];
>> > +};
>> > +\end{lstlisting}
>> > +
>> > +The \field{bus_num} indicates the physical SPI master assigned to guest.
>> 
>> Does this assume that a Virtio SPI controller always corresponds to a physical
>> SPI controller?
>> What if the backend SPI controller and SPI devices are fully emulated or the
>> SPI devices under the Virtio SPI controller come from multiple physical SPI
>> controllers?
>> 
>> Regards,
>> Qiang
>> 
>
>Hi Qiang,
>
>    It's quite possible the host SPI controller is not physical, but
>virtualized controller. In next patch, I will use "host SPI controller" to
>replace "physical SPI controller", "host SPI controller" represents all kinds
>of controller controlled by the host, which is more appropriate.

I think the meaning of "Host Controller" is a little bit opaque and is not
really necessary.

Hypervisor doesn't need to give guest an "id" of the Virtio SPI controller,
since this can be achieved by ACPI/DT in guest.

If guest needs to aware the topology of the backend, it's better to use
other mechanisms becuase it is board or configuration specific.

>
>    A Virtio SPI controller corresponds with a host SPI controller, and the
>guest can only access the devices under this host SPI controller, with the
>assigned cs. For devices under other host controllers, the guest cannot
>access.

We should allow the backend to emulate a SPI controller attached with physical
SPI devices from multiple physical SPI controllers. Anyway, this depends on
the backend implementation. We just give enough flexibility.


Regards,
Qiang

>
>    Thank you for your findings and comments.
>
>Best Regards
>Haixu Cui
>
>
>> > +
>> > +The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
>> > +
>> > +The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
>> > +	0: physical SPI master doesn't support cs timing setting;
>> > +	1: physical SPI master supports cs timing setting.
>> > +
>> > +The \field{reserved} is for alignment purpose, also for future extension.
>> > +
>> > +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
>> > +
>> > +\begin{enumerate}
>> > +\item The Virtio SPI driver configures and initializes the virtqueue.
>> > +\end{enumerate}
>> > +
>> > +\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}
>> > +
>> > +Virtio SPI driver enqueues requests to the virtqueue, and they are used by
>> > +Virtio SPI device. Each request represents one SPI tranfer and is of the form:
>> > +
>> > +\begin{lstlisting}
>> > +struct virtio_spi_transfer_head {
>> > +        u8 slave_id;
>> > +        u8 bits_per_word;
>> > +        u8 cs_change;
>> > +        u8 tx_nbits;
>> > +        u8 rx_nbits;
>> > +	 u8 reserved[3];
>> > +        le32 mode;
>> > +        le32 freq;
>> > +        le32 word_delay_ns;
>> > +        le32 cs_setup_ns;
>> > +        le32 cs_delay_hold_ns;
>> > +        le32 cs_change_delay_inactive_ns;
>> > +};
>> > +\end{lstlisting}
>> > +
>> > +\begin{lstlisting}
>> > +struct virtio_spi_transfer_result {
>> > +        u8 result;
>> > +};
>> > +\end{lstlisting}
>> > +
>> > +\begin{lstlisting}
>> > +struct virtio_spi_transfer_req {
>> > +        struct virtio_spi_transfer_head head;
>> > +        u8 tx_buf[];
>> > +        u8 rx_buf[];
>> > +        struct virtio_spi_transfer_result result;
>> > +};
>> > +\end{lstlisting}
>> > +
>> > +The \field{slave_id} indicates the chipselect index the SPI transfer used.
>> > +
>> > +The \field{bits_per_word} indicates the number of bits in each SPI transfer word.
>> > +
>> > +The \field{cs_change} indicates whether to deselect device before starting the
>> > +next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
>> > +then asserted again.
>> > +
>> > +The \field{tx_nbits} indicates 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.
>> > +
>> > +The \field{rx_nbits} indicates 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.
>> > +
>> > +The \field{reserved} is for alignement, also for further extension.
>> > +
>> > +The \field{mode} indicates how data is clocked out and in. Bit definitions as follows:
>> > +        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
>> > +	       relative to the clock pulses.
>> > +        bit 1: CPOL, determines the polarity of the clock.
>> > +        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
>> > +
>> > +The \field{freq} indicates the SPI transfer speed in Hz.
>> > +
>> > +The \field{word_delay_ns} indicates delay to be inserted between consecutive
>> > +words of a transfer, in ns unit.
>> > +
>> > +The \field{cs_setup_ns} indicates delay to be introduced after chipselect
>> > +is asserted, in ns unit.
>> > +
>> > +The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
>> > +is deasserted, in ns unit.
>> > +
>> > +The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
>> > +chipselect is deasserted and before next asserted, in ns unit.
>> > +
>> > +The \field{tx_buf} is the buffer for data sent to the device.
>> > +
>> > +The \field{rx_buf} is the buffer for data received to the device.
>> > +
>> > +The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
>> > +or VIRTIO_SPI_TRANS_ERR for error.
>> > +
>> > +\begin{lstlisting}
>> > +#define VIRTIO_SPI_TRANS_OK     0
>> > +#define VIRTIO_SPI_TRANS_ERR    1
>> > +\end{lstlisting}
>> > +
>> > +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
>> > +
>> > +Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
>> > +\field{result} in structure \field{virtio_spi_transfer_result} is written by 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 Virtio SPI device and consumed
>> > +by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio
>> > +SPI driver and consumed by 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 transfer requests on the requestq virtqueue.
>> > +
>> > +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
>> > +and MUST be readable for Virtio SPI device.
>> > +
>> > +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
>> > +and MUST be writable for Virtio SPI device.
>> > +
>> > +For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> > +\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> > +
>> > +For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> > +\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> > +
>> > +For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> > +\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> > +
>> > +For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
>> > +if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
>> > +
>> > +If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
>> > +\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
>> > +SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.
>> > +
>> > +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>> > +
>> > +Virtio SPI device MUST set all the fields of the structure \field{virtio_spi_config} before
>> > +they are read by Virtio SPI driver.
>> > +
>> > +Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result} before sending
>> > +it back to Virtio SPI driver.
>> > +
>> > +Virtio SPI device MUST be able to identify the transfer type according to the received
>> > +virtqueue descriptors.
>> > +
>> > +Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
>> > --- /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}
>> > +
>> > +An 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..3c965ef
>> > --- /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}
>> > +
>> > +An 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
>> > 

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
  2023-11-09  3:39     ` Qiang Zhang
@ 2023-11-09 10:21       ` Haixu Cui
  2023-11-13  1:37         ` Qiang Zhang
  0 siblings, 1 reply; 12+ messages in thread
From: Haixu Cui @ 2023-11-09 10:21 UTC (permalink / raw)
  To: Qiang Zhang; +Cc: virtio-dev, virtio-comment, harald.mommer, cohuck, quic_ztu



On 11/9/2023 11:39 AM, Qiang Zhang wrote:
> On Thu, Nov 09, 2023 at 10:52:06AM +0800, Haixu Cui wrote:
>>
>>
>> On 11/7/2023 3:40 PM, Qiang Zhang wrote:
>>> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
>>>> virtio-spi is a virtual SPI master and it allows a guest to operate and
>>>> use the physical SPI master controlled by the host.
>>>>
>>>> This patch adds the specification for virtio-spi.
>>>>
>>>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>>>> ---
>>>> device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
>>>> device-types/spi/device-conformance.tex |   7 +
>>>> device-types/spi/driver-conformance.tex |   7 +
>>>> 3 files changed, 220 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..5dbceaf
>>>> --- /dev/null
>>>> +++ b/device-types/spi/description.tex
>>>> @@ -0,0 +1,206 @@
>>>> +\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 controlled by the host.
>>>> +
>>>> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
>>>> +the virtqueue, and serviced by the physical SPI master.
>>>> +
>>>> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>>>> +the front-end existing in the guest kernel, and Virtio SPI device acts as the
>>>> +back-end in the host platform.
>>>> +
>>>> +\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 Virtio SPI driver.
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_spi_config {
>>>> +	le16 bus_num;
>>>> +	le16 chip_select_max_number;
>>>> +	le8 cs_timing_setting_enable;
>>>> +	le8 reserved[3];
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +The \field{bus_num} indicates the physical SPI master assigned to guest.
>>>
>>> Does this assume that a Virtio SPI controller always corresponds to a physical
>>> SPI controller?
>>> What if the backend SPI controller and SPI devices are fully emulated or the
>>> SPI devices under the Virtio SPI controller come from multiple physical SPI
>>> controllers?
>>>
>>> Regards,
>>> Qiang
>>>
>>
>> Hi Qiang,
>>
>>     It's quite possible the host SPI controller is not physical, but
>> virtualized controller. In next patch, I will use "host SPI controller" to
>> replace "physical SPI controller", "host SPI controller" represents all kinds
>> of controller controlled by the host, which is more appropriate.
> 
> I think the meaning of "Host Controller" is a little bit opaque and is not
> really necessary.
> 
> Hypervisor doesn't need to give guest an "id" of the Virtio SPI controller,
> since this can be achieved by ACPI/DT in guest.
> 
> If guest needs to aware the topology of the backend, it's better to use
> other mechanisms becuase it is board or configuration specific.

Hi Qiang,

Yes, bus_num is not necessary, hypervisor can maintain the mapping 
relationship between the frontend and backend without bus_num parameter. 
I will remove it in next patch.
> 
>>
>>     A Virtio SPI controller corresponds with a host SPI controller, and the
>> guest can only access the devices under this host SPI controller, with the
>> assigned cs. For devices under other host controllers, the guest cannot
>> access.
> 
> We should allow the backend to emulate a SPI controller attached with physical
> SPI devices from multiple physical SPI controllers. Anyway, this depends on
> the backend implementation. We just give enough flexibility.
> 

Here I'd like to use term "host SPI controller" to indicates the SPI 
controller controlled by the host and binded with the virtio SPI 
controller meanwhile. It may be a actual physical controller or may be 
an emulated one.

Do you think this term is appropriate, with the explanation above added 
in the spec? Or the statement "host SPI controlled devices" just like 
virtio-i2c used is more acceptable? Looking forware to your advice.

Thank you so much.

Best Regards
Haixu Cui

> 
> Regards,
> Qiang
> 
>>
>>     Thank you for your findings and comments.
>>
>> Best Regards
>> Haixu Cui
>>
>>
>>>> +
>>>> +The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
>>>> +
>>>> +The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
>>>> +	0: physical SPI master doesn't support cs timing setting;
>>>> +	1: physical SPI master supports cs timing setting.
>>>> +
>>>> +The \field{reserved} is for alignment purpose, also for future extension.
>>>> +
>>>> +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
>>>> +
>>>> +\begin{enumerate}
>>>> +\item The Virtio SPI driver configures and initializes the virtqueue.
>>>> +\end{enumerate}
>>>> +
>>>> +\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}
>>>> +
>>>> +Virtio SPI driver enqueues requests to the virtqueue, and they are used by
>>>> +Virtio SPI device. Each request represents one SPI tranfer and is of the form:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_spi_transfer_head {
>>>> +        u8 slave_id;
>>>> +        u8 bits_per_word;
>>>> +        u8 cs_change;
>>>> +        u8 tx_nbits;
>>>> +        u8 rx_nbits;
>>>> +	 u8 reserved[3];
>>>> +        le32 mode;
>>>> +        le32 freq;
>>>> +        le32 word_delay_ns;
>>>> +        le32 cs_setup_ns;
>>>> +        le32 cs_delay_hold_ns;
>>>> +        le32 cs_change_delay_inactive_ns;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_spi_transfer_result {
>>>> +        u8 result;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_spi_transfer_req {
>>>> +        struct virtio_spi_transfer_head head;
>>>> +        u8 tx_buf[];
>>>> +        u8 rx_buf[];
>>>> +        struct virtio_spi_transfer_result result;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +The \field{slave_id} indicates the chipselect index the SPI transfer used.
>>>> +
>>>> +The \field{bits_per_word} indicates the number of bits in each SPI transfer word.
>>>> +
>>>> +The \field{cs_change} indicates whether to deselect device before starting the
>>>> +next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
>>>> +then asserted again.
>>>> +
>>>> +The \field{tx_nbits} indicates 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.
>>>> +
>>>> +The \field{rx_nbits} indicates 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.
>>>> +
>>>> +The \field{reserved} is for alignement, also for further extension.
>>>> +
>>>> +The \field{mode} indicates how data is clocked out and in. Bit definitions as follows:
>>>> +        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
>>>> +	       relative to the clock pulses.
>>>> +        bit 1: CPOL, determines the polarity of the clock.
>>>> +        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
>>>> +
>>>> +The \field{freq} indicates the SPI transfer speed in Hz.
>>>> +
>>>> +The \field{word_delay_ns} indicates delay to be inserted between consecutive
>>>> +words of a transfer, in ns unit.
>>>> +
>>>> +The \field{cs_setup_ns} indicates delay to be introduced after chipselect
>>>> +is asserted, in ns unit.
>>>> +
>>>> +The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
>>>> +is deasserted, in ns unit.
>>>> +
>>>> +The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
>>>> +chipselect is deasserted and before next asserted, in ns unit.
>>>> +
>>>> +The \field{tx_buf} is the buffer for data sent to the device.
>>>> +
>>>> +The \field{rx_buf} is the buffer for data received to the device.
>>>> +
>>>> +The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
>>>> +or VIRTIO_SPI_TRANS_ERR for error.
>>>> +
>>>> +\begin{lstlisting}
>>>> +#define VIRTIO_SPI_TRANS_OK     0
>>>> +#define VIRTIO_SPI_TRANS_ERR    1
>>>> +\end{lstlisting}
>>>> +
>>>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
>>>> +
>>>> +Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
>>>> +\field{result} in structure \field{virtio_spi_transfer_result} is written by 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 Virtio SPI device and consumed
>>>> +by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio
>>>> +SPI driver and consumed by 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 transfer requests on the requestq virtqueue.
>>>> +
>>>> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
>>>> +and MUST be readable for Virtio SPI device.
>>>> +
>>>> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
>>>> +and MUST be writable for Virtio SPI device.
>>>> +
>>>> +For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>>>> +\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>>>> +
>>>> +For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>>>> +\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>>>> +
>>>> +For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>>>> +\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>>>> +
>>>> +For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
>>>> +if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
>>>> +
>>>> +If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
>>>> +\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
>>>> +SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.
>>>> +
>>>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>>>> +
>>>> +Virtio SPI device MUST set all the fields of the structure \field{virtio_spi_config} before
>>>> +they are read by Virtio SPI driver.
>>>> +
>>>> +Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result} before sending
>>>> +it back to Virtio SPI driver.
>>>> +
>>>> +Virtio SPI device MUST be able to identify the transfer type according to the received
>>>> +virtqueue descriptors.
>>>> +
>>>> +Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
>>>> --- /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}
>>>> +
>>>> +An 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..3c965ef
>>>> --- /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}
>>>> +
>>>> +An 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
>>>>

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
       [not found]     ` <ZUuxfbkSboAFQIQF@finisterre.sirena.org.uk>
  2023-11-08 18:37       ` Harald Mommer
@ 2023-11-09 11:21       ` Haixu Cui
       [not found]         ` <ZUzaZt4flHQgsBzu@finisterre.sirena.org.uk>
  1 sibling, 1 reply; 12+ messages in thread
From: Haixu Cui @ 2023-11-09 11:21 UTC (permalink / raw)
  To: Mark Brown, Harald Mommer
  Cc: virtio-dev, virtio-comment, harald.mommer, cohuck, quic_ztu

Hi Mark,

     For the "le16" type and byte ordering issue, Harald replies in 
another email, let's mainly discuss about checking parameters and 
exposing the backend supported features.


     struct virtio_spi_transfer_head {
         u8 chip_select;
         u8 bits_per_word;
         u8 cs_change;
         u8 tx_nbits;
         u8 rx_nbits;
         u8 reserved[3];
         le32 mode;
         le32 freq;
         le32 word_delay_ns;
         le32 cs_setup_ns;
         le32 cs_delay_hold_ns;
         le32 cs_change_delay_inactive_ns;
    };

    For checking the parameters above in virtio_spi_transfer_head, I'd 
like to add these field in config space:

     1. bits_per_word_mask, in Linux spi driver, 
__spi_validate_bits_per_word will use this field to validate 
bits_per_word parameters for each transfer

     2. cs_change_supported, 0 for unsupported and 1 for supported

     3. tx_nbits and rx_nbits:
        bit 0: 2-bit transfer supported;
        bit 1: 4-bit transfer supported;
        bit 2: 8-bit transfer supported;
        other bits reserved as 0, and 1-bit transfer should always 
supported.

     4. mode_feature_supported:
        bit 0-1: CPHA supported,
                 0b00: supports CPHA=0 and CPHA=1
                 0b01: only supports CPHA=0
                 0b10: only supports CPHA=1
                 0b11: invalid, should support as least one CPHA setting
        bit 2-3: CPOL supported
                 0b00: supports CPOL=0 and CPOL=1
                 0b01: only supports CPOL=0
                 0b10: only supports CPOL=1
                 0b11: invalid, should support as least one CPOL setting
        bit 4: CS_HIGH, if 1, supports chipselect active high, else
                 only supports chepselect active low.
        bit 3: LSB_FIRST, if 0, only supports MSB first transfer, if 1,
                 supports LSB first transfer
        bit 4: LOOP, if 1, support loopback mode, if 0, only supports
                 normal mode.

     5. freq_max_speed: if non-zero, maximum clock rate support, in Hz
                 unit. if 0, no limitation.

     6. max_word_delay_ns: if non-zero, the maximum word delay supported.
                 if 0, word delay is not supported to introduce.

     7. max_delay_after_cs_asserted: if non-zero, the maximum cs setup
                 delay supported.if 0, cs setup delay is not supported to
                 introduce

     8. max_delay_before_cs_deassert: if non-zero, the maximum
                 cs_delay_hold_ns delay supported.if 0, cs setup delay is
                 not supported to introduce

     9. max_delay_after_cs_deassert: if non-zero, the maximum
                 cs_change_delay_inactive_ns delay supported.if 0, cs
                 setup delay is not supported to introduce


Driver Requirement:

If Virtio SPI device announce it doesn't support any feature, Virtio SPI 
driver MUST reject the request if the corresponding feature field in 
request header is not zero.

Virtio SPI driver MUST reject the request if the freq exceeds the 
non-zero value of freq_max_speed filed in config space.

If any delay field in config space is set as non-zero, Virtio SPI driver 
MUST reject the request if the corresponding delay in request header 
exceeds the value in config space.


Do you think this mechanism, backend exposing its supported features and 
abilities through config space so Virtio SPI driver can filter and check 
before sending requests, is reasonable? And seems much stronger. A rule 
to validate seems necessary because the parameters in request header may 
differ in each transfer.

Looking forward to your advice and suggestion, if you agree that this 
mechanism is better, I will add it in next patch. Thank you so much.

Best Regard
Haixu Cui


On 11/9/2023 12:04 AM, Mark Brown wrote:
> On Wed, Nov 08, 2023 at 11:42:40PM +0800, Haixu Cui wrote:
>> On 11/6/2023 9:33 PM, Mark Brown wrote:
>>> On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
> 
>>>> +	le16 bus_num;
>>>> +	le16 chip_select_max_number;
> 
>>> That's a *lot* of potential buses and chip selects...
> 
>> Here you mean the types of bus_num and chip_select_max_number are "le16"?
> 
>> Here I use 16 bits because bus_num and num_chipselect in spi_controller
>> structure are both 16 bits.
> 
> Yeah, I think that's more for padding/alignment reasons than anything
> else.  TBH I wondered if just going for a standard int might be better,
> it's the combination of picking a smaller type but one that's relatively
> large for the domain.
> 
>> The \field{cs_timing_setting_enable} indicates if the host SPI controller
>> supports cs delay setting when performing transfer:
>>          bit 0: delay after cs asserted, 0 means doesn't support and 1
>>                 means support;
>>          bit 1: delay before cs deasserted, 0 means doesn't support and 1
>>                 means support;
>>          bit 2: delay after cs deasserted, 0 means doesn't support and 1
>>                 means support;
>>          other bits are reserved and always set as 0.
> 
> That looks a lot clearer.  There's still a potential issue with the fact
> that there might be limited configurability (eg, only a few values
> supported).  Perhaps we need a discovery mechanism, or perhaps
> specifying handling of inexact support might be good enough (see below)?
> 
>>>> +        le32 mode;
>>>> +        le32 freq;
>>>> +        le32 word_delay_ns;
>>>> +        le32 cs_setup_ns;
>>>> +        le32 cs_delay_hold_ns;
>>>> +        le32 cs_change_delay_inactive_ns;
> 
>>> All these parameters depend on controller support but weren't included
>>> in the feature enumeration above and nothing in the spec seems to cover
>>> what happens when they can't be supported exactly.
> 
>> Add another return value:
>>      #define VIRTIO_SPI_PARAM_ERR    1
> 
>> If the parameters in \field{virtio_spi_transfer_head} are not all valid, or
>> some fields are set as non-zero values but the corresponding features are
>> not supported by host, then the backend return VIRTIO_SPI_PARAM_ERR to the
>> frontend.
> 
> That looks good for things like the mode, for the frequencies, delays
> and so on it seems like we need either some specified handling of values
> that can't be supported exactly (eg, freq is a maximum, delays are a
> minimum) or an enumeration mechanism so the guest knows what values will
> be accepted.  The enumeration might be a bit unweildy since you can get
> huge numbers of values - Linux currently uses the latter approach.
> 
>>>> +The \field{tx_buf} is the buffer for data sent to the device.
>>>> +
>>>> +The \field{rx_buf} is the buffer for data received to the device.
>>>
>>> In what format are these buffers presented?  I note that the spec
>>> supports more than one byte per word (and non-integer number of bytes).
>>
>> In spi_transfer struct:
>>      @len: size of rx and tx buffers (in bytes)
>>
>> Although the bits_per_word can be any value in 0..32, in-memory wordsizes
>> are powers of two bytes (e.g. 20 bit samples use 32 bits).
>> So:
>>      0 < bits_per_word <=8: one word occupies 1 byte in memory;
>>      8 < bits_per_word <=16: one word occupies 2 bytes in memory;
>>      16 < bits_per_word <=32: one word occupies 4 bytes in memory.
>>
>> In the spec, the format of buffers are u8 array,
>>      u8 tx_buf[];
>>      u8 rx_buf[];
> 
> What byte ordering are the multi-byte words in - CPU native or a
> particular endianness (eg, wire native)?

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
       [not found]         ` <ZUzaZt4flHQgsBzu@finisterre.sirena.org.uk>
@ 2023-11-10  2:37           ` Haixu Cui
  0 siblings, 0 replies; 12+ messages in thread
From: Haixu Cui @ 2023-11-10  2:37 UTC (permalink / raw)
  To: Mark Brown; +Cc: Harald Mommer, virtio-dev, virtio-comment, cohuck, quic_ztu



On 11/9/2023 9:11 PM, Mark Brown wrote:
> On Thu, Nov 09, 2023 at 07:21:57PM +0800, Haixu Cui wrote:
> 
>>     For checking the parameters above in virtio_spi_transfer_head, I'd like
>> to add these field in config space:
> 
>>      5. freq_max_speed: if non-zero, maximum clock rate support, in Hz
>>                  unit. if 0, no limitation.
>>
>>      6. max_word_delay_ns: if non-zero, the maximum word delay supported.
>>                  if 0, word delay is not supported to introduce.
>>
>>      7. max_delay_after_cs_asserted: if non-zero, the maximum cs setup
>>                  delay supported.if 0, cs setup delay is not supported to
>>                  introduce
>>
>>      8. max_delay_before_cs_deassert: if non-zero, the maximum
>>                  cs_delay_hold_ns delay supported.if 0, cs setup delay is
>>                  not supported to introduce
>>
>>      9. max_delay_after_cs_deassert: if non-zero, the maximum
>>                  cs_change_delay_inactive_ns delay supported.if 0, cs
>>                  setup delay is not supported to introduce
> 
> I think this all makes sense - for the above we probably also need to
> say that if the value requested by the client can't be met exactly we
> either treat it as a minimum or maximum depending on the delays and
> choose the next value (probably a maximum for the clock and minimum for
> the rest).  So for example if the client requests 21MHz and the
> controller can only select 20MHz or 25MHz then it should pick 20MHz.

Sure, I will update to make it more flexible.

Thanks a lot.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-dev] [PATCH v4] virtio-spi: add the device specification
  2023-11-09 10:21       ` Haixu Cui
@ 2023-11-13  1:37         ` Qiang Zhang
  0 siblings, 0 replies; 12+ messages in thread
From: Qiang Zhang @ 2023-11-13  1:37 UTC (permalink / raw)
  To: Haixu Cui; +Cc: virtio-dev, virtio-comment, harald.mommer, cohuck, quic_ztu

On Thu, Nov 09, 2023 at 06:21:49PM +0800, Haixu Cui wrote:
>
>
>On 11/9/2023 11:39 AM, Qiang Zhang wrote:
>> On Thu, Nov 09, 2023 at 10:52:06AM +0800, Haixu Cui wrote:
>> > 
>> > 
>> > On 11/7/2023 3:40 PM, Qiang Zhang wrote:
>> > > On Tue, Oct 24, 2023 at 08:53:46PM +0800, Haixu Cui wrote:
>> > > > virtio-spi is a virtual SPI master and it allows a guest to operate and
>> > > > use the physical SPI master controlled by the host.
>> > > > 
>> > > > This patch adds the specification for virtio-spi.
>> > > > 
>> > > > Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> > > > ---
>> > > > device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
>> > > > device-types/spi/device-conformance.tex |   7 +
>> > > > device-types/spi/driver-conformance.tex |   7 +
>> > > > 3 files changed, 220 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..5dbceaf
>> > > > --- /dev/null
>> > > > +++ b/device-types/spi/description.tex
>> > > > @@ -0,0 +1,206 @@
>> > > > +\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 controlled by the host.
>> > > > +
>> > > > +virtio-spi has a single virtqueue. SPI transfer requests are placed into
>> > > > +the virtqueue, and serviced by the physical SPI master.
>> > > > +
>> > > > +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>> > > > +the front-end existing in the guest kernel, and Virtio SPI device acts as the
>> > > > +back-end in the host platform.
>> > > > +
>> > > > +\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 Virtio SPI driver.
>> > > > +
>> > > > +\begin{lstlisting}
>> > > > +struct virtio_spi_config {
>> > > > +	le16 bus_num;
>> > > > +	le16 chip_select_max_number;
>> > > > +	le8 cs_timing_setting_enable;
>> > > > +	le8 reserved[3];
>> > > > +};
>> > > > +\end{lstlisting}
>> > > > +
>> > > > +The \field{bus_num} indicates the physical SPI master assigned to guest.
>> > > 
>> > > Does this assume that a Virtio SPI controller always corresponds to a physical
>> > > SPI controller?
>> > > What if the backend SPI controller and SPI devices are fully emulated or the
>> > > SPI devices under the Virtio SPI controller come from multiple physical SPI
>> > > controllers?
>> > > 
>> > > Regards,
>> > > Qiang
>> > > 
>> > 
>> > Hi Qiang,
>> > 
>> >     It's quite possible the host SPI controller is not physical, but
>> > virtualized controller. In next patch, I will use "host SPI controller" to
>> > replace "physical SPI controller", "host SPI controller" represents all kinds
>> > of controller controlled by the host, which is more appropriate.
>> 
>> I think the meaning of "Host Controller" is a little bit opaque and is not
>> really necessary.
>> 
>> Hypervisor doesn't need to give guest an "id" of the Virtio SPI controller,
>> since this can be achieved by ACPI/DT in guest.
>> 
>> If guest needs to aware the topology of the backend, it's better to use
>> other mechanisms becuase it is board or configuration specific.
>
>Hi Qiang,
>
>Yes, bus_num is not necessary, hypervisor can maintain the mapping
>relationship between the frontend and backend without bus_num parameter. I
>will remove it in next patch.
>> 
>> > 
>> >     A Virtio SPI controller corresponds with a host SPI controller, and the
>> > guest can only access the devices under this host SPI controller, with the
>> > assigned cs. For devices under other host controllers, the guest cannot
>> > access.
>> 
>> We should allow the backend to emulate a SPI controller attached with physical
>> SPI devices from multiple physical SPI controllers. Anyway, this depends on
>> the backend implementation. We just give enough flexibility.
>> 
>
>Here I'd like to use term "host SPI controller" to indicates the SPI
>controller controlled by the host and binded with the virtio SPI controller
>meanwhile. It may be a actual physical controller or may be an emulated one.
>
>Do you think this term is appropriate, with the explanation above added in
>the spec? Or the statement "host SPI controlled devices" just like virtio-i2c
>used is more acceptable? Looking forware to your advice.

Fine with me.

Regards,
Qiang

>
>Thank you so much.
>
>Best Regards
>Haixu Cui
>
>> 
>> Regards,
>> Qiang
>> 
>> > 
>> >     Thank you for your findings and comments.
>> > 
>> > Best Regards
>> > Haixu Cui
>> > 
>> > 
>> > > > +
>> > > > +The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
>> > > > +
>> > > > +The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
>> > > > +	0: physical SPI master doesn't support cs timing setting;
>> > > > +	1: physical SPI master supports cs timing setting.
>> > > > +
>> > > > +The \field{reserved} is for alignment purpose, also for future extension.
>> > > > +
>> > > > +\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
>> > > > +
>> > > > +\begin{enumerate}
>> > > > +\item The Virtio SPI driver configures and initializes the virtqueue.
>> > > > +\end{enumerate}
>> > > > +
>> > > > +\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}
>> > > > +
>> > > > +Virtio SPI driver enqueues requests to the virtqueue, and they are used by
>> > > > +Virtio SPI device. Each request represents one SPI tranfer and is of the form:
>> > > > +
>> > > > +\begin{lstlisting}
>> > > > +struct virtio_spi_transfer_head {
>> > > > +        u8 slave_id;
>> > > > +        u8 bits_per_word;
>> > > > +        u8 cs_change;
>> > > > +        u8 tx_nbits;
>> > > > +        u8 rx_nbits;
>> > > > +	 u8 reserved[3];
>> > > > +        le32 mode;
>> > > > +        le32 freq;
>> > > > +        le32 word_delay_ns;
>> > > > +        le32 cs_setup_ns;
>> > > > +        le32 cs_delay_hold_ns;
>> > > > +        le32 cs_change_delay_inactive_ns;
>> > > > +};
>> > > > +\end{lstlisting}
>> > > > +
>> > > > +\begin{lstlisting}
>> > > > +struct virtio_spi_transfer_result {
>> > > > +        u8 result;
>> > > > +};
>> > > > +\end{lstlisting}
>> > > > +
>> > > > +\begin{lstlisting}
>> > > > +struct virtio_spi_transfer_req {
>> > > > +        struct virtio_spi_transfer_head head;
>> > > > +        u8 tx_buf[];
>> > > > +        u8 rx_buf[];
>> > > > +        struct virtio_spi_transfer_result result;
>> > > > +};
>> > > > +\end{lstlisting}
>> > > > +
>> > > > +The \field{slave_id} indicates the chipselect index the SPI transfer used.
>> > > > +
>> > > > +The \field{bits_per_word} indicates the number of bits in each SPI transfer word.
>> > > > +
>> > > > +The \field{cs_change} indicates whether to deselect device before starting the
>> > > > +next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect deasserted
>> > > > +then asserted again.
>> > > > +
>> > > > +The \field{tx_nbits} indicates 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.
>> > > > +
>> > > > +The \field{rx_nbits} indicates 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.
>> > > > +
>> > > > +The \field{reserved} is for alignement, also for further extension.
>> > > > +
>> > > > +The \field{mode} indicates how data is clocked out and in. Bit definitions as follows:
>> > > > +        bit 0: CPHA, determines the timing (i.e. phase) of the data bits
>> > > > +	       relative to the clock pulses.
>> > > > +        bit 1: CPOL, determines the polarity of the clock.
>> > > > +        bit 2: CS_HIGH, if 1, chipselect 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, if 1, device is in loopback mode, else normal mode.
>> > > > +
>> > > > +The \field{freq} indicates the SPI transfer speed in Hz.
>> > > > +
>> > > > +The \field{word_delay_ns} indicates delay to be inserted between consecutive
>> > > > +words of a transfer, in ns unit.
>> > > > +
>> > > > +The \field{cs_setup_ns} indicates delay to be introduced after chipselect
>> > > > +is asserted, in ns unit.
>> > > > +
>> > > > +The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
>> > > > +is deasserted, in ns unit.
>> > > > +
>> > > > +The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
>> > > > +chipselect is deasserted and before next asserted, in ns unit.
>> > > > +
>> > > > +The \field{tx_buf} is the buffer for data sent to the device.
>> > > > +
>> > > > +The \field{rx_buf} is the buffer for data received to the device.
>> > > > +
>> > > > +The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
>> > > > +or VIRTIO_SPI_TRANS_ERR for error.
>> > > > +
>> > > > +\begin{lstlisting}
>> > > > +#define VIRTIO_SPI_TRANS_OK     0
>> > > > +#define VIRTIO_SPI_TRANS_ERR    1
>> > > > +\end{lstlisting}
>> > > > +
>> > > > +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
>> > > > +
>> > > > +Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
>> > > > +\field{result} in structure \field{virtio_spi_transfer_result} is written by 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 Virtio SPI device and consumed
>> > > > +by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio
>> > > > +SPI driver and consumed by 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 transfer requests on the requestq virtqueue.
>> > > > +
>> > > > +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
>> > > > +and MUST be readable for Virtio SPI device.
>> > > > +
>> > > > +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
>> > > > +and MUST be writable for Virtio SPI device.
>> > > > +
>> > > > +For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> > > > +\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> > > > +
>> > > > +For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> > > > +\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> > > > +
>> > > > +For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
>> > > > +\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
>> > > > +
>> > > > +For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
>> > > > +if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
>> > > > +
>> > > > +If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
>> > > > +\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
>> > > > +SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.
>> > > > +
>> > > > +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
>> > > > +
>> > > > +Virtio SPI device MUST set all the fields of the structure \field{virtio_spi_config} before
>> > > > +they are read by Virtio SPI driver.
>> > > > +
>> > > > +Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result} before sending
>> > > > +it back to Virtio SPI driver.
>> > > > +
>> > > > +Virtio SPI device MUST be able to identify the transfer type according to the received
>> > > > +virtqueue descriptors.
>> > > > +
>> > > > +Virtio SPI device MUST NOT change the data in \field{tx_buf} 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..3e771bc
>> > > > --- /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}
>> > > > +
>> > > > +An 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..3c965ef
>> > > > --- /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}
>> > > > +
>> > > > +An 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
>> > > > 
>
>---------------------------------------------------------------------
>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


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-11-13  1:30 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-24 12:53 [virtio-dev] [PATCH v4] virtio-spi: add the device specification Haixu Cui
2023-10-24 13:08 ` [virtio-dev] " Haixu Cui
2023-11-07  7:40 ` [virtio-dev] " Qiang Zhang
2023-11-09  2:52   ` Haixu Cui
2023-11-09  3:39     ` Qiang Zhang
2023-11-09 10:21       ` Haixu Cui
2023-11-13  1:37         ` Qiang Zhang
     [not found] ` <56fbe6cc-d7a5-438f-1dd2-939f4197a970@opensynergy.com>
2023-11-07 17:10   ` [virtio-dev] " Haixu Cui
     [not found] ` <ZUjrDjHpFVReDSgk@finisterre.sirena.org.uk>
2023-11-08 15:42   ` [virtio-dev] " Haixu Cui
     [not found]     ` <ZUuxfbkSboAFQIQF@finisterre.sirena.org.uk>
2023-11-08 18:37       ` Harald Mommer
2023-11-09 11:21       ` Haixu Cui
     [not found]         ` <ZUzaZt4flHQgsBzu@finisterre.sirena.org.uk>
2023-11-10  2:37           ` Haixu Cui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox