[virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
a guest to operate and use the host SPI controller.

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
 device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 +
 device-types/spi/driver-conformance.tex |   7 +
 3 files changed, 295 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..8ca8c0d
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,281 @@
+\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
+
+virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
+a guest to operate and use the host SPI controller. Host SPI controller may be a
+physical controller, or a virtual one(e.g. controller emulated by the software
+running in the host).
+
+virtio-spi has a single virtqueue. SPI transfer requests are placed into
+the virtqueue, and serviced by the host SPI controller.
+
+In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
+the front-end running in the guest kernel, and Virtio SPI device acts as the
+back-end in the host.
+
+\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
+45
+
+\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
+
+None
+
+\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for Virtio SPI driver.
+The config space shows the features and settings supported by the host SPI controller.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+	u8 chip_select_max_number;
+	u8 cs_change_supported;
+	u8 tx_nbits_supported;
+	u8 rx_nbits_supported;
+	le32 bits_per_word_mask;
+	le32 mode_func_supported;
+	le32 max_freq_hz;
+	le32 max_word_delay_ns;
+	le32 max_cs_setup_ns;
+	le32 max_cs_hold_ns;
+	le32 max_cs_inactive_ns;
+};
+\end{lstlisting}
+
+The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
+
+The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
+after each transfer in one message:
+        0: supported;
+        1: unsupported, means chipselect will keep active when executing the message transaction.
+Note: Message here contains a sequence of SPI transfer.
+
+The \field{tx_nbits_supported} indicates the supported number of bit for writing:
+        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
+        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
+        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
+        other bits are reserved as 0, 1-bit transfer is always supported.
+
+The \field{rx_nbits_supported} indicates the supported number of bit for reading:
+        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
+        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
+        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
+        other bits are reserved as 0, 1-bit transfer is always supported.
+
+The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
+are supported. If not set, no limitation for bits_per_word.
+Note: bits_per_word corresponds to \field{bits_per_word} field in
+structure \field{virtio_spi_transfer_head}, see below.
+
+The \field{mode_func_supported} indicates the following features are supported or not:
+        bit 0-1: CPHA feature,
+            0b00: supports CPHA=0 and CPHA=1;
+            0b01: supports CPHA=0 only;
+            0b10: supports CPHA=1 only;
+            0b11: invalid, should support as least one CPHA setting.
+
+        bit 2-3: CPOL feature,
+            0b00: supports CPOL=0 and CPOL=1;
+            0b01: supports CPOL=0 only;
+            0b10: supports CPOL=1 only;
+            0b11: invalid, should support as least one CPOL setting.
+
+        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
+            chipselect active low should always be supported.
+
+        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
+            supported.
+
+        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
+            should always be supported.
+
+The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
+limitation for transfer speed.
+
+The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
+word delay feature is unsupported.
+Note: Just as one message contains a sequence of transfers, one transfer may contain
+a sequence of word.
+
+The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
+0 means delay is not supported to introduce after chipselect is asserted.
+
+The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
+0 means delay is not supported to introduce before chipselect is deasserted.
+
+The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
+0 means delay is not supported to introduce after chipselect is deasserted.
+
+\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 chip_select_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{chip_select_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 number of bits used for writing:
+        0,1: 1-bit transfer, also known as SINGLE;
+        2  : 2-bit transfer, also known as DUAL;
+        4  : 4-bit transfer, also known as QUAD;
+        8  : 8-bit transfer, also known as OCTAL;
+        other values are invalid.
+
+The \field{rx_nbits} indicates number of bits used for reading:
+        0,1: 1-bit transfer, also known as SINGLE;
+        2  : 2-bit transfer, also known as DUAL;
+        4  : 4-bit transfer, also known as QUAD;
+        8  : 8-bit transfer, also known as OCTAL;
+        other values are invalid.
+
+The \field{reserved} is for alignement, also for further extension.
+
+The \field{mode} indicates some transfer settings. 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 from the device.
+
+The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
+VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
+VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
+valid but trasnfer process failed.
+
+\begin{lstlisting}
+#define VIRTIO_SPI_TRANS_OK     0
+#define VIRTIO_SPI_PARAM_ERR    1
+#define VIRTIO_SPI_TRANS_ERR    2
+\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 read 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 not VIRTIO_SPI_TRANS_OK.
+
+For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
+and MUST reject the request if any filed is invalid or enabling the features not supported by host.
+For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
+in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
+\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.
+
+\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.
+
+Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
+and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
+some host SPI controller unsupported features are set.
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


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
a guest to operate and use the host SPI controller.

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
 device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 +
 device-types/spi/driver-conformance.tex |   7 +
 3 files changed, 295 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..8ca8c0d
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,281 @@
+\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
+
+virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
+a guest to operate and use the host SPI controller. Host SPI controller may be a
+physical controller, or a virtual one(e.g. controller emulated by the software
+running in the host).
+
+virtio-spi has a single virtqueue. SPI transfer requests are placed into
+the virtqueue, and serviced by the host SPI controller.
+
+In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
+the front-end running in the guest kernel, and Virtio SPI device acts as the
+back-end in the host.
+
+\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
+45
+
+\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
+
+None
+
+\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for Virtio SPI driver.
+The config space shows the features and settings supported by the host SPI controller.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+	u8 chip_select_max_number;
+	u8 cs_change_supported;
+	u8 tx_nbits_supported;
+	u8 rx_nbits_supported;
+	le32 bits_per_word_mask;
+	le32 mode_func_supported;
+	le32 max_freq_hz;
+	le32 max_word_delay_ns;
+	le32 max_cs_setup_ns;
+	le32 max_cs_hold_ns;
+	le32 max_cs_inactive_ns;
+};
+\end{lstlisting}
+
+The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
+
+The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
+after each transfer in one message:
+        0: supported;
+        1: unsupported, means chipselect will keep active when executing the message transaction.
+Note: Message here contains a sequence of SPI transfer.
+
+The \field{tx_nbits_supported} indicates the supported number of bit for writing:
+        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
+        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
+        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
+        other bits are reserved as 0, 1-bit transfer is always supported.
+
+The \field{rx_nbits_supported} indicates the supported number of bit for reading:
+        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
+        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
+        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
+        other bits are reserved as 0, 1-bit transfer is always supported.
+
+The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
+are supported. If not set, no limitation for bits_per_word.
+Note: bits_per_word corresponds to \field{bits_per_word} field in
+structure \field{virtio_spi_transfer_head}, see below.
+
+The \field{mode_func_supported} indicates the following features are supported or not:
+        bit 0-1: CPHA feature,
+            0b00: supports CPHA=0 and CPHA=1;
+            0b01: supports CPHA=0 only;
+            0b10: supports CPHA=1 only;
+            0b11: invalid, should support as least one CPHA setting.
+
+        bit 2-3: CPOL feature,
+            0b00: supports CPOL=0 and CPOL=1;
+            0b01: supports CPOL=0 only;
+            0b10: supports CPOL=1 only;
+            0b11: invalid, should support as least one CPOL setting.
+
+        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
+            chipselect active low should always be supported.
+
+        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
+            supported.
+
+        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
+            should always be supported.
+
+The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
+limitation for transfer speed.
+
+The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
+word delay feature is unsupported.
+Note: Just as one message contains a sequence of transfers, one transfer may contain
+a sequence of word.
+
+The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
+0 means delay is not supported to introduce after chipselect is asserted.
+
+The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
+0 means delay is not supported to introduce before chipselect is deasserted.
+
+The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
+0 means delay is not supported to introduce after chipselect is deasserted.
+
+\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 chip_select_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{chip_select_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 number of bits used for writing:
+        0,1: 1-bit transfer, also known as SINGLE;
+        2  : 2-bit transfer, also known as DUAL;
+        4  : 4-bit transfer, also known as QUAD;
+        8  : 8-bit transfer, also known as OCTAL;
+        other values are invalid.
+
+The \field{rx_nbits} indicates number of bits used for reading:
+        0,1: 1-bit transfer, also known as SINGLE;
+        2  : 2-bit transfer, also known as DUAL;
+        4  : 4-bit transfer, also known as QUAD;
+        8  : 8-bit transfer, also known as OCTAL;
+        other values are invalid.
+
+The \field{reserved} is for alignement, also for further extension.
+
+The \field{mode} indicates some transfer settings. 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 from the device.
+
+The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
+VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
+VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
+valid but trasnfer process failed.
+
+\begin{lstlisting}
+#define VIRTIO_SPI_TRANS_OK     0
+#define VIRTIO_SPI_PARAM_ERR    1
+#define VIRTIO_SPI_TRANS_ERR    2
+\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 read 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 not VIRTIO_SPI_TRANS_OK.
+
+For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
+and MUST reject the request if any filed is invalid or enabling the features not supported by host.
+For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
+in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
+\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.
+
+\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.
+
+Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
+and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
+some host SPI controller unsupported features are set.
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

[virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification

[Cornelia Huck]

On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> a guest to operate and use the host SPI controller.
>
> This patch adds the specification for virtio-spi.

As Mark has already reviewed it from the SPI POV (thanks!), I'm now
looking at the virtio side.

>
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>  device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>  device-types/spi/device-conformance.tex |   7 +
>  device-types/spi/driver-conformance.tex |   7 +
>  3 files changed, 295 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..8ca8c0d
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,281 @@
> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
> +
> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> +a guest to operate and use the host SPI controller. Host SPI controller may be a
> +physical controller, or a virtual one(e.g. controller emulated by the software

Missing space before the opening bracket.

> +running in the host).
> +
> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
> +the virtqueue, and serviced by the host SPI controller.

Is there any way to express all of this without referring to "host" and
"guest" here? (The paragraph below giving it as an example is fine.)

Something like "The virtio-spi device is a virtual SPI controller. It
allows the driver to operate and use an SPI controller under the control
of the device, either a physical controller, or an emulated one."

> +
> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
> +the front-end running in the guest kernel, and Virtio SPI device acts as the
> +back-end in the host.
> +
> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
> +45
> +
> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
> +
> +None
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
> +
> +All fields of this configuration are always available and read-only for Virtio SPI driver.
> +The config space shows the features and settings supported by the host SPI controller.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> +	u8 chip_select_max_number;
> +	u8 cs_change_supported;
> +	u8 tx_nbits_supported;
> +	u8 rx_nbits_supported;
> +	le32 bits_per_word_mask;
> +	le32 mode_func_supported;
> +	le32 max_freq_hz;
> +	le32 max_word_delay_ns;
> +	le32 max_cs_setup_ns;
> +	le32 max_cs_hold_ns;
> +	le32 max_cs_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.

"chipselect" is probably a known term for people familiar with SPI -- is
there any definition of those terms that the spec can point to?

Also, it should be either "The field \field{whatever}" or
"\field{whatever}"; "The \field{whatever}" looks good in the LaTeX
source, but not in the end result.

> +
> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
> +after each transfer in one message:
> +        0: supported;
> +        1: unsupported, means chipselect will keep active when executing the message transaction.
> +Note: Message here contains a sequence of SPI transfer.
> +
> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> +        other bits are reserved as 0, 1-bit transfer is always supported.
> +
> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> +        other bits are reserved as 0, 1-bit transfer is always supported.
> +
> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
> +are supported. If not set, no limitation for bits_per_word.
> +Note: bits_per_word corresponds to \field{bits_per_word} field in
> +structure \field{virtio_spi_transfer_head}, see below.
> +
> +The \field{mode_func_supported} indicates the following features are supported or not:

s/indicates/indicates whether/

Can we point to some documentation that explains CPHA and CPOL?

> +        bit 0-1: CPHA feature,
> +            0b00: supports CPHA=0 and CPHA=1;
> +            0b01: supports CPHA=0 only;
> +            0b10: supports CPHA=1 only;
> +            0b11: invalid, should support as least one CPHA setting.
> +
> +        bit 2-3: CPOL feature,
> +            0b00: supports CPOL=0 and CPOL=1;
> +            0b01: supports CPOL=0 only;
> +            0b10: supports CPOL=1 only;
> +            0b11: invalid, should support as least one CPOL setting.
> +
> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
> +            chipselect active low should always be supported.
> +
> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
> +            supported.

Probably better to expand the LSB/MSB acronyms, just to avoid any
possible confusion. What data does this apply to?

> +
> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
> +            should always be supported.
> +
> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
> +limitation for transfer speed.
> +
> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
> +word delay feature is unsupported.
> +Note: Just as one message contains a sequence of transfers, one transfer may contain
> +a sequence of word.

s/word/words/

> +
> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is asserted.
> +
> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce before chipselect is deasserted.
> +
> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is deasserted.
> +
> +\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:

s/tranfer/transfer/

> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.
> +
> +The \field{rx_nbits} indicates number of bits used for reading:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.
> +
> +The \field{reserved} is for alignement, also for further extension.

Maybe "\field{reserved} is currently unused and might be used for
further extensions in the future." ?

> +
> +The \field{mode} indicates some transfer settings. 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 from the device.
> +
> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
> +valid but trasnfer process failed.

This is quite a complicated sentence, can we maybe rewrite it to

"\field{result} contains the transfer result. It may have the following
values:"

> +
> +\begin{lstlisting}
> +#define VIRTIO_SPI_TRANS_OK     0
> +#define VIRTIO_SPI_PARAM_ERR    1
> +#define VIRTIO_SPI_TRANS_ERR    2
> +\end{lstlisting}

"VIRTIO_SPI_TRANS_OK indicates successful completion of the transfer.

VIRTIO_SPI_PARAM_ERR indicates a parameter error: Some of the parameters
in \field{virtio_spi_transfer_head} are not valid, or some fields are
set as non-zero values, but the corresponding features are not supported
by the device.

VIRTIO_SPI_TRANS_ERR indicates a transfer error: The parameters are all
valid, but the transfer process failed."

> +
> +\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. 

Can you make this either an enumerate or an itemize?

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

I don't think this makes sense as a normative statement: the driver
sending transfer requests on the request queue is the whole point of
this setup.

> +
> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
> +and MUST be readable for Virtio SPI device.

Likewise, I think we can assume this from the general idea of how
virtio-spi is supposed to work.

> +
> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
> +and MUST be writable for Virtio SPI device.

Likewise.

> +
> +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 read 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 not VIRTIO_SPI_TRANS_OK.
> +
> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.

s/filed/field/
s/host/device/

Also, isn't the rejecting supposed to be done by the device, as the
driver is the party enqueueing the requests? Or do I have some kind of
fundamental misunderstanding?

> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.

An example like this should be in the non-normative section (probably
where you describe the potential errors indicated in result.) (And
again, I think it is the device that rejects requests?)

> +
> +\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.

This follows from basic virtio operation requirements and does not need
a normative statement.

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

I'm not sure what this statement means -- doesn't the driver need to set
up the request in a way that the device understands what to do? The
device actually looking at the driver's requests and processing them is
basic virtqueue operation, no need for a normative statement.

> +
> +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.
> +
> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
> +some host SPI controller unsupported features are set.
> 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:

This one is still using the "SPI Master" term.

> +
> +\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:

Likewise.

> +
> +\begin{itemize}
> +\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
> +\end{itemize}


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Cornelia Huck]

On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> a guest to operate and use the host SPI controller.
>
> This patch adds the specification for virtio-spi.

As Mark has already reviewed it from the SPI POV (thanks!), I'm now
looking at the virtio side.

>
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>  device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>  device-types/spi/device-conformance.tex |   7 +
>  device-types/spi/driver-conformance.tex |   7 +
>  3 files changed, 295 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..8ca8c0d
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,281 @@
> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
> +
> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> +a guest to operate and use the host SPI controller. Host SPI controller may be a
> +physical controller, or a virtual one(e.g. controller emulated by the software

Missing space before the opening bracket.

> +running in the host).
> +
> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
> +the virtqueue, and serviced by the host SPI controller.

Is there any way to express all of this without referring to "host" and
"guest" here? (The paragraph below giving it as an example is fine.)

Something like "The virtio-spi device is a virtual SPI controller. It
allows the driver to operate and use an SPI controller under the control
of the device, either a physical controller, or an emulated one."

> +
> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
> +the front-end running in the guest kernel, and Virtio SPI device acts as the
> +back-end in the host.
> +
> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
> +45
> +
> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
> +
> +None
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
> +
> +All fields of this configuration are always available and read-only for Virtio SPI driver.
> +The config space shows the features and settings supported by the host SPI controller.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> +	u8 chip_select_max_number;
> +	u8 cs_change_supported;
> +	u8 tx_nbits_supported;
> +	u8 rx_nbits_supported;
> +	le32 bits_per_word_mask;
> +	le32 mode_func_supported;
> +	le32 max_freq_hz;
> +	le32 max_word_delay_ns;
> +	le32 max_cs_setup_ns;
> +	le32 max_cs_hold_ns;
> +	le32 max_cs_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.

"chipselect" is probably a known term for people familiar with SPI -- is
there any definition of those terms that the spec can point to?

Also, it should be either "The field \field{whatever}" or
"\field{whatever}"; "The \field{whatever}" looks good in the LaTeX
source, but not in the end result.

> +
> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
> +after each transfer in one message:
> +        0: supported;
> +        1: unsupported, means chipselect will keep active when executing the message transaction.
> +Note: Message here contains a sequence of SPI transfer.
> +
> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> +        other bits are reserved as 0, 1-bit transfer is always supported.
> +
> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> +        other bits are reserved as 0, 1-bit transfer is always supported.
> +
> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
> +are supported. If not set, no limitation for bits_per_word.
> +Note: bits_per_word corresponds to \field{bits_per_word} field in
> +structure \field{virtio_spi_transfer_head}, see below.
> +
> +The \field{mode_func_supported} indicates the following features are supported or not:

s/indicates/indicates whether/

Can we point to some documentation that explains CPHA and CPOL?

> +        bit 0-1: CPHA feature,
> +            0b00: supports CPHA=0 and CPHA=1;
> +            0b01: supports CPHA=0 only;
> +            0b10: supports CPHA=1 only;
> +            0b11: invalid, should support as least one CPHA setting.
> +
> +        bit 2-3: CPOL feature,
> +            0b00: supports CPOL=0 and CPOL=1;
> +            0b01: supports CPOL=0 only;
> +            0b10: supports CPOL=1 only;
> +            0b11: invalid, should support as least one CPOL setting.
> +
> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
> +            chipselect active low should always be supported.
> +
> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
> +            supported.

Probably better to expand the LSB/MSB acronyms, just to avoid any
possible confusion. What data does this apply to?

> +
> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
> +            should always be supported.
> +
> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
> +limitation for transfer speed.
> +
> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
> +word delay feature is unsupported.
> +Note: Just as one message contains a sequence of transfers, one transfer may contain
> +a sequence of word.

s/word/words/

> +
> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is asserted.
> +
> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce before chipselect is deasserted.
> +
> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is deasserted.
> +
> +\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:

s/tranfer/transfer/

> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.
> +
> +The \field{rx_nbits} indicates number of bits used for reading:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.
> +
> +The \field{reserved} is for alignement, also for further extension.

Maybe "\field{reserved} is currently unused and might be used for
further extensions in the future." ?

> +
> +The \field{mode} indicates some transfer settings. 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 from the device.
> +
> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
> +valid but trasnfer process failed.

This is quite a complicated sentence, can we maybe rewrite it to

"\field{result} contains the transfer result. It may have the following
values:"

> +
> +\begin{lstlisting}
> +#define VIRTIO_SPI_TRANS_OK     0
> +#define VIRTIO_SPI_PARAM_ERR    1
> +#define VIRTIO_SPI_TRANS_ERR    2
> +\end{lstlisting}

"VIRTIO_SPI_TRANS_OK indicates successful completion of the transfer.

VIRTIO_SPI_PARAM_ERR indicates a parameter error: Some of the parameters
in \field{virtio_spi_transfer_head} are not valid, or some fields are
set as non-zero values, but the corresponding features are not supported
by the device.

VIRTIO_SPI_TRANS_ERR indicates a transfer error: The parameters are all
valid, but the transfer process failed."

> +
> +\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. 

Can you make this either an enumerate or an itemize?

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

I don't think this makes sense as a normative statement: the driver
sending transfer requests on the request queue is the whole point of
this setup.

> +
> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
> +and MUST be readable for Virtio SPI device.

Likewise, I think we can assume this from the general idea of how
virtio-spi is supposed to work.

> +
> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
> +and MUST be writable for Virtio SPI device.

Likewise.

> +
> +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 read 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 not VIRTIO_SPI_TRANS_OK.
> +
> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.

s/filed/field/
s/host/device/

Also, isn't the rejecting supposed to be done by the device, as the
driver is the party enqueueing the requests? Or do I have some kind of
fundamental misunderstanding?

> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.

An example like this should be in the non-normative section (probably
where you describe the potential errors indicated in result.) (And
again, I think it is the device that rejects requests?)

> +
> +\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.

This follows from basic virtio operation requirements and does not need
a normative statement.

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

I'm not sure what this statement means -- doesn't the driver need to set
up the request in a way that the device understands what to do? The
device actually looking at the driver's requests and processing them is
basic virtqueue operation, no need for a normative statement.

> +
> +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.
> +
> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
> +some host SPI controller unsupported features are set.
> 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:

This one is still using the "SPI Master" term.

> +
> +\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:

Likewise.

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Viresh Kumar]

Hi Haixu,

Cornelia has already covered most of the issues, I will try to add few more
things I noticed.

On 24-11-23, 15:20, Haixu Cui wrote:
> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> a guest to operate and use the host SPI controller.
> 
> This patch adds the specification for virtio-spi.
> 
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>  device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>  device-types/spi/device-conformance.tex |   7 +
>  device-types/spi/driver-conformance.tex |   7 +
>  3 files changed, 295 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..8ca8c0d
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,281 @@
> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}

Not sure if we should use "Master" anywhere..
Maybe: s/SPI Master Device/SPI Device/ ?

Applies elsewhere too..

> +
> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows

Maybe:

s/virtio-spi/The Virtio SPI device/ (this applies at multiple places)

s/and it allows/that allows/

?

> +a guest to operate and use the host SPI controller. Host SPI controller may be a

Maybe: The host SPI controller ... ?

> +physical controller, or a virtual one(e.g. controller emulated by the software
> +running in the host).
> +
> +virtio-spi has a single virtqueue. SPI transfer requests are placed into

The Virtio SPI device..

> +the virtqueue, and serviced by the host SPI controller.
> +
> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
> +the front-end running in the guest kernel, and Virtio SPI device acts as the
> +back-end in the host.
> +
> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
> +45
> +
> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
> +
> +None
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
> +
> +All fields of this configuration are always available and read-only for Virtio SPI driver.
> +The config space shows the features and settings supported by the host SPI controller.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> +	u8 chip_select_max_number;
> +	u8 cs_change_supported;
> +	u8 tx_nbits_supported;
> +	u8 rx_nbits_supported;
> +	le32 bits_per_word_mask;
> +	le32 mode_func_supported;
> +	le32 max_freq_hz;
> +	le32 max_word_delay_ns;
> +	le32 max_cs_setup_ns;
> +	le32 max_cs_hold_ns;
> +	le32 max_cs_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
> +
> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
> +after each transfer in one message:
> +        0: supported;
> +        1: unsupported, means chipselect will keep active when executing the message transaction.
> +Note: Message here contains a sequence of SPI transfer.

I would rather suggest using '1' for supported and '0' for unsupported, like you
have done for LSB first, loopback, etc. later..

> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;

Maybe you can mention first that, a set bit means feature is supported,
otherwise now. And then you can just write:

        bit 0: 2-bit transfer
        bit 1: 4-bit transfer
        bit 2: 8-bit transfer

> +        other bits are reserved as 0, 1-bit transfer is always supported.

Maybe write as: other bits are reserved and MUST be set to 0 by the device.

> +
> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> +        other bits are reserved as 0, 1-bit transfer is always supported.

Same here..

> +
> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
> +are supported. If not set, no limitation for bits_per_word.
> +Note: bits_per_word corresponds to \field{bits_per_word} field in
> +structure \field{virtio_spi_transfer_head}, see below.
> +
> +The \field{mode_func_supported} indicates the following features are supported or not:
> +        bit 0-1: CPHA feature,
> +            0b00: supports CPHA=0 and CPHA=1;
> +            0b01: supports CPHA=0 only;
> +            0b10: supports CPHA=1 only;
> +            0b11: invalid, should support as least one CPHA setting.
> +
> +        bit 2-3: CPOL feature,
> +            0b00: supports CPOL=0 and CPOL=1;
> +            0b01: supports CPOL=0 only;
> +            0b10: supports CPOL=1 only;
> +            0b11: invalid, should support as least one CPOL setting.

Maybe explain what CPHA and CPOL are ..

> +
> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
> +            chipselect active low should always be supported.
> +
> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
> +            supported.
> +
> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
> +            should always be supported.
> +
> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
> +limitation for transfer speed.
> +
> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
> +word delay feature is unsupported.
> +Note: Just as one message contains a sequence of transfers, one transfer may contain
> +a sequence of word.
> +
> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is asserted.
> +
> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce before chipselect is deasserted.
> +
> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is deasserted.
> +
> +\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.

The Virtio SPI driver..

> +Each request represents one SPI tranfer and is of the form:
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.
> +
> +The \field{rx_nbits} indicates number of bits used for reading:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.

The description of the above two fields look exactly same, can we define this
just once and mention it applies for both ?

> +The \field{reserved} is for alignement, also for further extension.
> +
> +The \field{mode} indicates some transfer settings. 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 from the device.
> +
> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
> +valid but trasnfer process failed.
> +
> +\begin{lstlisting}
> +#define VIRTIO_SPI_TRANS_OK     0
> +#define VIRTIO_SPI_PARAM_ERR    1
> +#define VIRTIO_SPI_TRANS_ERR    2
> +\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

See, you are already using "Virtio SPI device/driver" here. Just use the same
throughout instead of "virtio-spi".

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

Should the length of both the buffers in full-duplex mode be same ? If yes, then
this should be mentioned (in case it is not).

> +
> +\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.

s/structure \field{virtio_spi_transfer_head}/\field{struct virtio_spi_transfer_head}/ ?

That should automatically add struct before its name.

> +
> +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 read or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}

s/ or /, /

> +if the \field{result} returned from Virtio SPI device is not VIRTIO_SPI_TRANS_OK.
> +
> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}

s/driver/device/

> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.

s/filed/field/
s/features not supported by host/features is not supported by the host/

> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.

As I said earlier, maybe we should talk about length of the rx/tx buffers for
full-duplex transfers here.

> +
> +\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.

The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.

> +
> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
> +some host SPI controller unsupported features are set.
> 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:

The Virtio SPI device

> +
> +\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}

Thanks.

-- 
viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Viresh Kumar]

Hi Haixu,

Cornelia has already covered most of the issues, I will try to add few more
things I noticed.

On 24-11-23, 15:20, Haixu Cui wrote:
> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> a guest to operate and use the host SPI controller.
> 
> This patch adds the specification for virtio-spi.
> 
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>  device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>  device-types/spi/device-conformance.tex |   7 +
>  device-types/spi/driver-conformance.tex |   7 +
>  3 files changed, 295 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..8ca8c0d
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,281 @@
> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}

Not sure if we should use "Master" anywhere..
Maybe: s/SPI Master Device/SPI Device/ ?

Applies elsewhere too..

> +
> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows

Maybe:

s/virtio-spi/The Virtio SPI device/ (this applies at multiple places)

s/and it allows/that allows/

?

> +a guest to operate and use the host SPI controller. Host SPI controller may be a

Maybe: The host SPI controller ... ?

> +physical controller, or a virtual one(e.g. controller emulated by the software
> +running in the host).
> +
> +virtio-spi has a single virtqueue. SPI transfer requests are placed into

The Virtio SPI device..

> +the virtqueue, and serviced by the host SPI controller.
> +
> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
> +the front-end running in the guest kernel, and Virtio SPI device acts as the
> +back-end in the host.
> +
> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
> +45
> +
> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] requestq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
> +
> +None
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
> +
> +All fields of this configuration are always available and read-only for Virtio SPI driver.
> +The config space shows the features and settings supported by the host SPI controller.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> +	u8 chip_select_max_number;
> +	u8 cs_change_supported;
> +	u8 tx_nbits_supported;
> +	u8 rx_nbits_supported;
> +	le32 bits_per_word_mask;
> +	le32 mode_func_supported;
> +	le32 max_freq_hz;
> +	le32 max_word_delay_ns;
> +	le32 max_cs_setup_ns;
> +	le32 max_cs_hold_ns;
> +	le32 max_cs_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
> +
> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
> +after each transfer in one message:
> +        0: supported;
> +        1: unsupported, means chipselect will keep active when executing the message transaction.
> +Note: Message here contains a sequence of SPI transfer.

I would rather suggest using '1' for supported and '0' for unsupported, like you
have done for LSB first, loopback, etc. later..

> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;

Maybe you can mention first that, a set bit means feature is supported,
otherwise now. And then you can just write:

        bit 0: 2-bit transfer
        bit 1: 4-bit transfer
        bit 2: 8-bit transfer

> +        other bits are reserved as 0, 1-bit transfer is always supported.

Maybe write as: other bits are reserved and MUST be set to 0 by the device.

> +
> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> +        other bits are reserved as 0, 1-bit transfer is always supported.

Same here..

> +
> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
> +are supported. If not set, no limitation for bits_per_word.
> +Note: bits_per_word corresponds to \field{bits_per_word} field in
> +structure \field{virtio_spi_transfer_head}, see below.
> +
> +The \field{mode_func_supported} indicates the following features are supported or not:
> +        bit 0-1: CPHA feature,
> +            0b00: supports CPHA=0 and CPHA=1;
> +            0b01: supports CPHA=0 only;
> +            0b10: supports CPHA=1 only;
> +            0b11: invalid, should support as least one CPHA setting.
> +
> +        bit 2-3: CPOL feature,
> +            0b00: supports CPOL=0 and CPOL=1;
> +            0b01: supports CPOL=0 only;
> +            0b10: supports CPOL=1 only;
> +            0b11: invalid, should support as least one CPOL setting.

Maybe explain what CPHA and CPOL are ..

> +
> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
> +            chipselect active low should always be supported.
> +
> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
> +            supported.
> +
> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
> +            should always be supported.
> +
> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
> +limitation for transfer speed.
> +
> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
> +word delay feature is unsupported.
> +Note: Just as one message contains a sequence of transfers, one transfer may contain
> +a sequence of word.
> +
> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is asserted.
> +
> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce before chipselect is deasserted.
> +
> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
> +0 means delay is not supported to introduce after chipselect is deasserted.
> +
> +\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.

The Virtio SPI driver..

> +Each request represents one SPI tranfer and is of the form:
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.
> +
> +The \field{rx_nbits} indicates number of bits used for reading:
> +        0,1: 1-bit transfer, also known as SINGLE;
> +        2  : 2-bit transfer, also known as DUAL;
> +        4  : 4-bit transfer, also known as QUAD;
> +        8  : 8-bit transfer, also known as OCTAL;
> +        other values are invalid.

The description of the above two fields look exactly same, can we define this
just once and mention it applies for both ?

> +The \field{reserved} is for alignement, also for further extension.
> +
> +The \field{mode} indicates some transfer settings. 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 from the device.
> +
> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
> +valid but trasnfer process failed.
> +
> +\begin{lstlisting}
> +#define VIRTIO_SPI_TRANS_OK     0
> +#define VIRTIO_SPI_PARAM_ERR    1
> +#define VIRTIO_SPI_TRANS_ERR    2
> +\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

See, you are already using "Virtio SPI device/driver" here. Just use the same
throughout instead of "virtio-spi".

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

Should the length of both the buffers in full-duplex mode be same ? If yes, then
this should be mentioned (in case it is not).

> +
> +\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.

s/structure \field{virtio_spi_transfer_head}/\field{struct virtio_spi_transfer_head}/ ?

That should automatically add struct before its name.

> +
> +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 read or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}

s/ or /, /

> +if the \field{result} returned from Virtio SPI device is not VIRTIO_SPI_TRANS_OK.
> +
> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}

s/driver/device/

> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.

s/filed/field/
s/features not supported by host/features is not supported by the host/

> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.

As I said earlier, maybe we should talk about length of the rx/tx buffers for
full-duplex transfers here.

> +
> +\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.

The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.

> +
> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
> +some host SPI controller unsupported features are set.
> 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:

The Virtio SPI device

> +
> +\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}

Thanks.

-- 
viresh

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

[virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

Hi Mark,
     Thank you, really appreciate.

Best Regards
Haixu Cui

On 11/24/2023 8:48 PM, Mark Brown wrote:
> On Fri, Nov 24, 2023 at 03:20:15PM +0800, Haixu Cui wrote:
>> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> a guest to operate and use the host SPI controller.
>>
>> This patch adds the specification for virtio-spi.
> 
> This looks fine from a SPI point of view, I don't really have any idea
> about what makes sense or is idomatic at the virtio level so can't
> really review there:
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

Hi Mark,
     Thank you, really appreciate.

Best Regards
Haixu Cui

On 11/24/2023 8:48 PM, Mark Brown wrote:
> On Fri, Nov 24, 2023 at 03:20:15PM +0800, Haixu Cui wrote:
>> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> a guest to operate and use the host SPI controller.
>>
>> This patch adds the specification for virtio-spi.
> 
> This looks fine from a SPI point of view, I don't really have any idea
> about what makes sense or is idomatic at the virtio level so can't
> really review there:
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

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

[virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

Hi Huck,
     Really appreciate for your comments and please refer to my replies 
below each comment.

On 11/24/2023 11:46 PM, Cornelia Huck wrote:
> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> a guest to operate and use the host SPI controller.
>>
>> This patch adds the specification for virtio-spi.
> 
> As Mark has already reviewed it from the SPI POV (thanks!), I'm now
> looking at the virtio side.
> 
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> ---
>>   device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 +
>>   device-types/spi/driver-conformance.tex |   7 +
>>   3 files changed, 295 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..8ca8c0d
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,281 @@
>> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
>> +
>> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> +a guest to operate and use the host SPI controller. Host SPI controller may be a
>> +physical controller, or a virtual one(e.g. controller emulated by the software
> 
> Missing space before the opening bracket.
> 
>> +running in the host).
>> +
>> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
>> +the virtqueue, and serviced by the host SPI controller.
> 
> Is there any way to express all of this without referring to "host" and
> "guest" here? (The paragraph below giving it as an example is fine.)
> 
> Something like "The virtio-spi device is a virtual SPI controller. It
> allows the driver to operate and use an SPI controller under the control
> of the device, either a physical controller, or an emulated one."
> 

Okay, in next patch, guest&host will be replaced by driver&device.

>> +
>> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>> +the front-end running in the guest kernel, and Virtio SPI device acts as the
>> +back-end in the host.
>> +
>> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
>> +45
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] requestq
>> +\end{description}
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
>> +
>> +None
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
>> +
>> +All fields of this configuration are always available and read-only for Virtio SPI driver.
>> +The config space shows the features and settings supported by the host SPI controller.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +	u8 chip_select_max_number;
>> +	u8 cs_change_supported;
>> +	u8 tx_nbits_supported;
>> +	u8 rx_nbits_supported;
>> +	le32 bits_per_word_mask;
>> +	le32 mode_func_supported;
>> +	le32 max_freq_hz;
>> +	le32 max_word_delay_ns;
>> +	le32 max_cs_setup_ns;
>> +	le32 max_cs_hold_ns;
>> +	le32 max_cs_inactive_ns;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
> 
> "chipselect" is probably a known term for people familiar with SPI -- is
> there any definition of those terms that the spec can point to?

Just as Mark said, there is no formal spec for SPI, so no standard spec 
for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see 
below.

> 
> Also, it should be either "The field \field{whatever}" or
> "\field{whatever}"; "The \field{whatever}" looks good in the LaTeX
> source, but not in the end result.

Okay, use "\field{whatever}".
> 
>> +
>> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
>> +after each transfer in one message:
>> +        0: supported;
>> +        1: unsupported, means chipselect will keep active when executing the message transaction.
>> +Note: Message here contains a sequence of SPI transfer.
>> +
>> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
>> +
>> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
>> +
>> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
>> +are supported. If not set, no limitation for bits_per_word.
>> +Note: bits_per_word corresponds to \field{bits_per_word} field in
>> +structure \field{virtio_spi_transfer_head}, see below.
>> +
>> +The \field{mode_func_supported} indicates the following features are supported or not:
> 
> s/indicates/indicates whether/
Will update.

> 
> Can we point to some documentation that explains CPHA and CPOL?

Here. No standard SPI spec to point to. CPOL/CPHA have definitions in 
wikipedia(Clock polarity and phase chapter):

https://en.wikipedia.org/wiki/Serial_Peripheral_Interface

How about copying some concise information from wikipedia as Note? Or is 
referring to such webpage acceptable in this spec.

Looking forward to your suggestion.

> 
>> +        bit 0-1: CPHA feature,
>> +            0b00: supports CPHA=0 and CPHA=1;
>> +            0b01: supports CPHA=0 only;
>> +            0b10: supports CPHA=1 only;
>> +            0b11: invalid, should support as least one CPHA setting.
>> +
>> +        bit 2-3: CPOL feature,
>> +            0b00: supports CPOL=0 and CPOL=1;
>> +            0b01: supports CPOL=0 only;
>> +            0b10: supports CPOL=1 only;
>> +            0b11: invalid, should support as least one CPOL setting.
>> +
>> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
>> +            chipselect active low should always be supported.
>> +
>> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
>> +            supported.
> 
> Probably better to expand the LSB/MSB acronyms, just to avoid any
> possible confusion. What data does this apply to?

Add a Note:
LSB first indicates that data is transferred least significant bit 
first, and MSB first indicates that data is transferred most significant 
bit first.

> 
>> +
>> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
>> +            should always be supported.
>> +
>> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
>> +limitation for transfer speed.
>> +
>> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
>> +word delay feature is unsupported.
>> +Note: Just as one message contains a sequence of transfers, one transfer may contain
>> +a sequence of word.
> 
> s/word/words/
Will update.

> 
>> +
>> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is asserted.
>> +
>> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce before chipselect is deasserted.
>> +
>> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is deasserted.
>> +
>> +\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:
> 
> s/tranfer/transfer/
yes!
> 
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.
>> +
>> +The \field{rx_nbits} indicates number of bits used for reading:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.
>> +
>> +The \field{reserved} is for alignement, also for further extension.
> 
> Maybe "\field{reserved} is currently unused and might be used for
> further extensions in the future." ?
Will update.

> 
>> +
>> +The \field{mode} indicates some transfer settings. 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 from the device.
>> +
>> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
>> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
>> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
>> +valid but trasnfer process failed.
> 
> This is quite a complicated sentence, can we maybe rewrite it to
> 
> "\field{result} contains the transfer result. It may have the following
> values:"
> 
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_TRANS_OK     0
>> +#define VIRTIO_SPI_PARAM_ERR    1
>> +#define VIRTIO_SPI_TRANS_ERR    2
>> +\end{lstlisting}
> 
> "VIRTIO_SPI_TRANS_OK indicates successful completion of the transfer.
> 
> VIRTIO_SPI_PARAM_ERR indicates a parameter error: Some of the parameters
> in \field{virtio_spi_transfer_head} are not valid, or some fields are
> set as non-zero values, but the corresponding features are not supported
> by the device.
> 
> VIRTIO_SPI_TRANS_ERR indicates a transfer error: The parameters are all
> valid, but the transfer process failed."
> 

In next patch, I will rewrite the sentence as your advice.

>> +
>> +\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.
> 
> Can you make this either an enumerate or an itemize?

Use itemize.

> 
>> +
>> +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.
> 
> I don't think this makes sense as a normative statement: the driver
> sending transfer requests on the request queue is the whole point of
> this setup.
Will remove this item.

> 
>> +
>> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
>> +and MUST be readable for Virtio SPI device.
> 
> Likewise, I think we can assume this from the general idea of how
> virtio-spi is supposed to work.
Will remove this item.
> 
>> +
>> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
>> +and MUST be writable for Virtio SPI device.
> 
> Likewise
Will remove this item.
.
> 
>> +
>> +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 read 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 not VIRTIO_SPI_TRANS_OK.
>> +
>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
> 
> s/filed/field/
> s/host/device/
> 
> Also, isn't the rejecting supposed to be done by the device, as the
> driver is the party enqueueing the requests? Or do I have some kind of
> fundamental misunderstanding?

It may be better to filter some invalid requests by driver, as in the 
request header there are many parameters, and some of them are not 
supported by device, so it's quite possible that many requests invalid 
for the device. So if driver can do the first filter, such invalid 
requests will not be sent at all, this will conserve virtqueue and 
system overhead.

And this is why exposing device supported features in the config space, 
it ensures that almost all requests in virtqueue are nice to the backend.

device also will verify the requests again, as the following requirement:
Virtio SPI device MUST verify the parameters in 
\field{virtio_spi_transfer_head} after receiving the request,
and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR 
if not all parameters are valid or some device unsupported features are set.

Although checking the requests twice seems a little redundant, it is 
more efficient comparing with sending some invalid requests to the device.

What is your opinion? Do you think it is acceptable?

> 
>> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
>> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
>> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.
> 
> An example like this should be in the non-normative section (probably
> where you describe the potential errors indicated in result.) (And
> again, I think it is the device that rejects requests?)

Sure, I will move it to above part.

> 
>> +
>> +\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.
> 
> This follows from basic virtio operation requirements and does not need
> a normative statement.
Will remove this item.

> 
>> +
>> +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.
> 
> I'm not sure what this statement means -- doesn't the driver need to set
> up the request in a way that the device understands what to do? The
> device actually looking at the driver's requests and processing them is
> basic virtqueue operation, no need for a normative statement.
Will remove this item from normative section.

> 
>> +
>> +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.
>> +
>> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
>> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
>> +some host SPI controller unsupported features are set.
>> 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:
> 
> This one is still using the "SPI Master" term.

In next patch, will use "SPI Controller", and also in 
virtio-spec/contents.tex.
> 
>> +
>> +\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:
> 
> Likewise.
> 
>> +
>> +\begin{itemize}
>> +\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
>> +\end{itemize}
> 

Once again, thanks a lot for your support.

Best Regards
Haixu Cui


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

Hi Huck,
     Really appreciate for your comments and please refer to my replies 
below each comment.

On 11/24/2023 11:46 PM, Cornelia Huck wrote:
> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> a guest to operate and use the host SPI controller.
>>
>> This patch adds the specification for virtio-spi.
> 
> As Mark has already reviewed it from the SPI POV (thanks!), I'm now
> looking at the virtio side.
> 
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> ---
>>   device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 +
>>   device-types/spi/driver-conformance.tex |   7 +
>>   3 files changed, 295 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..8ca8c0d
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,281 @@
>> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
>> +
>> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> +a guest to operate and use the host SPI controller. Host SPI controller may be a
>> +physical controller, or a virtual one(e.g. controller emulated by the software
> 
> Missing space before the opening bracket.
> 
>> +running in the host).
>> +
>> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
>> +the virtqueue, and serviced by the host SPI controller.
> 
> Is there any way to express all of this without referring to "host" and
> "guest" here? (The paragraph below giving it as an example is fine.)
> 
> Something like "The virtio-spi device is a virtual SPI controller. It
> allows the driver to operate and use an SPI controller under the control
> of the device, either a physical controller, or an emulated one."
> 

Okay, in next patch, guest&host will be replaced by driver&device.

>> +
>> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>> +the front-end running in the guest kernel, and Virtio SPI device acts as the
>> +back-end in the host.
>> +
>> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
>> +45
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] requestq
>> +\end{description}
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
>> +
>> +None
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
>> +
>> +All fields of this configuration are always available and read-only for Virtio SPI driver.
>> +The config space shows the features and settings supported by the host SPI controller.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +	u8 chip_select_max_number;
>> +	u8 cs_change_supported;
>> +	u8 tx_nbits_supported;
>> +	u8 rx_nbits_supported;
>> +	le32 bits_per_word_mask;
>> +	le32 mode_func_supported;
>> +	le32 max_freq_hz;
>> +	le32 max_word_delay_ns;
>> +	le32 max_cs_setup_ns;
>> +	le32 max_cs_hold_ns;
>> +	le32 max_cs_inactive_ns;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
> 
> "chipselect" is probably a known term for people familiar with SPI -- is
> there any definition of those terms that the spec can point to?

Just as Mark said, there is no formal spec for SPI, so no standard spec 
for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see 
below.

> 
> Also, it should be either "The field \field{whatever}" or
> "\field{whatever}"; "The \field{whatever}" looks good in the LaTeX
> source, but not in the end result.

Okay, use "\field{whatever}".
> 
>> +
>> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
>> +after each transfer in one message:
>> +        0: supported;
>> +        1: unsupported, means chipselect will keep active when executing the message transaction.
>> +Note: Message here contains a sequence of SPI transfer.
>> +
>> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
>> +
>> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
>> +
>> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
>> +are supported. If not set, no limitation for bits_per_word.
>> +Note: bits_per_word corresponds to \field{bits_per_word} field in
>> +structure \field{virtio_spi_transfer_head}, see below.
>> +
>> +The \field{mode_func_supported} indicates the following features are supported or not:
> 
> s/indicates/indicates whether/
Will update.

> 
> Can we point to some documentation that explains CPHA and CPOL?

Here. No standard SPI spec to point to. CPOL/CPHA have definitions in 
wikipedia(Clock polarity and phase chapter):

https://en.wikipedia.org/wiki/Serial_Peripheral_Interface

How about copying some concise information from wikipedia as Note? Or is 
referring to such webpage acceptable in this spec.

Looking forward to your suggestion.

> 
>> +        bit 0-1: CPHA feature,
>> +            0b00: supports CPHA=0 and CPHA=1;
>> +            0b01: supports CPHA=0 only;
>> +            0b10: supports CPHA=1 only;
>> +            0b11: invalid, should support as least one CPHA setting.
>> +
>> +        bit 2-3: CPOL feature,
>> +            0b00: supports CPOL=0 and CPOL=1;
>> +            0b01: supports CPOL=0 only;
>> +            0b10: supports CPOL=1 only;
>> +            0b11: invalid, should support as least one CPOL setting.
>> +
>> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
>> +            chipselect active low should always be supported.
>> +
>> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
>> +            supported.
> 
> Probably better to expand the LSB/MSB acronyms, just to avoid any
> possible confusion. What data does this apply to?

Add a Note:
LSB first indicates that data is transferred least significant bit 
first, and MSB first indicates that data is transferred most significant 
bit first.

> 
>> +
>> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
>> +            should always be supported.
>> +
>> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
>> +limitation for transfer speed.
>> +
>> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
>> +word delay feature is unsupported.
>> +Note: Just as one message contains a sequence of transfers, one transfer may contain
>> +a sequence of word.
> 
> s/word/words/
Will update.

> 
>> +
>> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is asserted.
>> +
>> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce before chipselect is deasserted.
>> +
>> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is deasserted.
>> +
>> +\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:
> 
> s/tranfer/transfer/
yes!
> 
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.
>> +
>> +The \field{rx_nbits} indicates number of bits used for reading:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.
>> +
>> +The \field{reserved} is for alignement, also for further extension.
> 
> Maybe "\field{reserved} is currently unused and might be used for
> further extensions in the future." ?
Will update.

> 
>> +
>> +The \field{mode} indicates some transfer settings. 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 from the device.
>> +
>> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
>> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
>> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
>> +valid but trasnfer process failed.
> 
> This is quite a complicated sentence, can we maybe rewrite it to
> 
> "\field{result} contains the transfer result. It may have the following
> values:"
> 
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_TRANS_OK     0
>> +#define VIRTIO_SPI_PARAM_ERR    1
>> +#define VIRTIO_SPI_TRANS_ERR    2
>> +\end{lstlisting}
> 
> "VIRTIO_SPI_TRANS_OK indicates successful completion of the transfer.
> 
> VIRTIO_SPI_PARAM_ERR indicates a parameter error: Some of the parameters
> in \field{virtio_spi_transfer_head} are not valid, or some fields are
> set as non-zero values, but the corresponding features are not supported
> by the device.
> 
> VIRTIO_SPI_TRANS_ERR indicates a transfer error: The parameters are all
> valid, but the transfer process failed."
> 

In next patch, I will rewrite the sentence as your advice.

>> +
>> +\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.
> 
> Can you make this either an enumerate or an itemize?

Use itemize.

> 
>> +
>> +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.
> 
> I don't think this makes sense as a normative statement: the driver
> sending transfer requests on the request queue is the whole point of
> this setup.
Will remove this item.

> 
>> +
>> +Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
>> +and MUST be readable for Virtio SPI device.
> 
> Likewise, I think we can assume this from the general idea of how
> virtio-spi is supposed to work.
Will remove this item.
> 
>> +
>> +Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
>> +and MUST be writable for Virtio SPI device.
> 
> Likewise
Will remove this item.
.
> 
>> +
>> +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 read 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 not VIRTIO_SPI_TRANS_OK.
>> +
>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
> 
> s/filed/field/
> s/host/device/
> 
> Also, isn't the rejecting supposed to be done by the device, as the
> driver is the party enqueueing the requests? Or do I have some kind of
> fundamental misunderstanding?

It may be better to filter some invalid requests by driver, as in the 
request header there are many parameters, and some of them are not 
supported by device, so it's quite possible that many requests invalid 
for the device. So if driver can do the first filter, such invalid 
requests will not be sent at all, this will conserve virtqueue and 
system overhead.

And this is why exposing device supported features in the config space, 
it ensures that almost all requests in virtqueue are nice to the backend.

device also will verify the requests again, as the following requirement:
Virtio SPI device MUST verify the parameters in 
\field{virtio_spi_transfer_head} after receiving the request,
and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR 
if not all parameters are valid or some device unsupported features are set.

Although checking the requests twice seems a little redundant, it is 
more efficient comparing with sending some invalid requests to the device.

What is your opinion? Do you think it is acceptable?

> 
>> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
>> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
>> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.
> 
> An example like this should be in the non-normative section (probably
> where you describe the potential errors indicated in result.) (And
> again, I think it is the device that rejects requests?)

Sure, I will move it to above part.

> 
>> +
>> +\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.
> 
> This follows from basic virtio operation requirements and does not need
> a normative statement.
Will remove this item.

> 
>> +
>> +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.
> 
> I'm not sure what this statement means -- doesn't the driver need to set
> up the request in a way that the device understands what to do? The
> device actually looking at the driver's requests and processing them is
> basic virtqueue operation, no need for a normative statement.
Will remove this item from normative section.

> 
>> +
>> +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.
>> +
>> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
>> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
>> +some host SPI controller unsupported features are set.
>> 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:
> 
> This one is still using the "SPI Master" term.

In next patch, will use "SPI Controller", and also in 
virtio-spec/contents.tex.
> 
>> +
>> +\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:
> 
> Likewise.
> 
>> +
>> +\begin{itemize}
>> +\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
>> +\end{itemize}
> 

Once again, thanks a lot for your support.

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

[virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification

[Cornelia Huck]

On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>> 
>> "chipselect" is probably a known term for people familiar with SPI -- is
>> there any definition of those terms that the spec can point to?
>
> Just as Mark said, there is no formal spec for SPI, so no standard spec 
> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see 
> below.

If we have nothing to point to, it is probably best to simply
expand/explain the terms on their first usage.

>> Can we point to some documentation that explains CPHA and CPOL?
>
> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in 
> wikipedia(Clock polarity and phase chapter):
>
> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>
> How about copying some concise information from wikipedia as Note? Or is 
> referring to such webpage acceptable in this spec.

Not sure if we can do an outright copy (licence compatibility), but
paraphrasing should be fine. (I'd rather not directly reference the
site, because the content is not guaranteed to be stable, but we could
maybe add it as "further reading".)

>>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
>>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
>> 
>> s/filed/field/
>> s/host/device/
>> 
>> Also, isn't the rejecting supposed to be done by the device, as the
>> driver is the party enqueueing the requests? Or do I have some kind of
>> fundamental misunderstanding?
>
> It may be better to filter some invalid requests by driver, as in the 
> request header there are many parameters, and some of them are not 
> supported by device, so it's quite possible that many requests invalid 
> for the device. So if driver can do the first filter, such invalid 
> requests will not be sent at all, this will conserve virtqueue and 
> system overhead.
>
> And this is why exposing device supported features in the config space, 
> it ensures that almost all requests in virtqueue are nice to the backend.
>
> device also will verify the requests again, as the following requirement:
> Virtio SPI device MUST verify the parameters in 
> \field{virtio_spi_transfer_head} after receiving the request,
> and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR 
> if not all parameters are valid or some device unsupported features are set.
>
> Although checking the requests twice seems a little redundant, it is 
> more efficient comparing with sending some invalid requests to the device.
>
> What is your opinion? Do you think it is acceptable?

Thanks for your explanation, I think we simply have some terminology
issues. In the virtio spec, "driver" refers to one side of the
driver/device pair, and is used to describe how to communicate with the
device. In this case, "driver" would be the entity interacting with the
device, regardless of how it is implemented, and would be responsible
for sending the requests. Any filtering that would be done in a concrete
implementation (i.e. if you have one component generating the requests,
and then another component filtering them before actually putting them
into the queue) is out of scope for this spec -- we can maybe specify
that the driver should not send invalid requests, but I'm not sure that
this is actually needed.

> Once again, thanks a lot for your support.

You're welcome!


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Cornelia Huck]

On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>> 
>> "chipselect" is probably a known term for people familiar with SPI -- is
>> there any definition of those terms that the spec can point to?
>
> Just as Mark said, there is no formal spec for SPI, so no standard spec 
> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see 
> below.

If we have nothing to point to, it is probably best to simply
expand/explain the terms on their first usage.

>> Can we point to some documentation that explains CPHA and CPOL?
>
> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in 
> wikipedia(Clock polarity and phase chapter):
>
> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>
> How about copying some concise information from wikipedia as Note? Or is 
> referring to such webpage acceptable in this spec.

Not sure if we can do an outright copy (licence compatibility), but
paraphrasing should be fine. (I'd rather not directly reference the
site, because the content is not guaranteed to be stable, but we could
maybe add it as "further reading".)

>>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
>>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
>> 
>> s/filed/field/
>> s/host/device/
>> 
>> Also, isn't the rejecting supposed to be done by the device, as the
>> driver is the party enqueueing the requests? Or do I have some kind of
>> fundamental misunderstanding?
>
> It may be better to filter some invalid requests by driver, as in the 
> request header there are many parameters, and some of them are not 
> supported by device, so it's quite possible that many requests invalid 
> for the device. So if driver can do the first filter, such invalid 
> requests will not be sent at all, this will conserve virtqueue and 
> system overhead.
>
> And this is why exposing device supported features in the config space, 
> it ensures that almost all requests in virtqueue are nice to the backend.
>
> device also will verify the requests again, as the following requirement:
> Virtio SPI device MUST verify the parameters in 
> \field{virtio_spi_transfer_head} after receiving the request,
> and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR 
> if not all parameters are valid or some device unsupported features are set.
>
> Although checking the requests twice seems a little redundant, it is 
> more efficient comparing with sending some invalid requests to the device.
>
> What is your opinion? Do you think it is acceptable?

Thanks for your explanation, I think we simply have some terminology
issues. In the virtio spec, "driver" refers to one side of the
driver/device pair, and is used to describe how to communicate with the
device. In this case, "driver" would be the entity interacting with the
device, regardless of how it is implemented, and would be responsible
for sending the requests. Any filtering that would be done in a concrete
implementation (i.e. if you have one component generating the requests,
and then another component filtering them before actually putting them
into the queue) is out of scope for this spec -- we can maybe specify
that the driver should not send invalid requests, but I'm not sure that
this is actually needed.

> Once again, thanks a lot for your support.

You're welcome!


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

Re: [virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification

[Cornelia Huck]

On Mon, Nov 27 2023, Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>
>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>>> 
>>> "chipselect" is probably a known term for people familiar with SPI -- is
>>> there any definition of those terms that the spec can point to?
>>
>> Just as Mark said, there is no formal spec for SPI, so no standard spec 
>> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see 
>> below.
>
> If we have nothing to point to, it is probably best to simply
> expand/explain the terms on their first usage.
>
>>> Can we point to some documentation that explains CPHA and CPOL?
>>
>> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in 
>> wikipedia(Clock polarity and phase chapter):
>>
>> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>>
>> How about copying some concise information from wikipedia as Note? Or is 
>> referring to such webpage acceptable in this spec.
>
> Not sure if we can do an outright copy (licence compatibility), but
> paraphrasing should be fine. (I'd rather not directly reference the
> site, because the content is not guaranteed to be stable, but we could
> maybe add it as "further reading".)

Looking again, the kernel doc referenced by Mark
(https://www.kernel.org/doc/html/v6.6/driver-api/spi.html) might also be
a good candidate, especially as we can refer to a specific version.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Cornelia Huck]

On Mon, Nov 27 2023, Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>
>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>>> 
>>> "chipselect" is probably a known term for people familiar with SPI -- is
>>> there any definition of those terms that the spec can point to?
>>
>> Just as Mark said, there is no formal spec for SPI, so no standard spec 
>> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see 
>> below.
>
> If we have nothing to point to, it is probably best to simply
> expand/explain the terms on their first usage.
>
>>> Can we point to some documentation that explains CPHA and CPOL?
>>
>> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in 
>> wikipedia(Clock polarity and phase chapter):
>>
>> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>>
>> How about copying some concise information from wikipedia as Note? Or is 
>> referring to such webpage acceptable in this spec.
>
> Not sure if we can do an outright copy (licence compatibility), but
> paraphrasing should be fine. (I'd rather not directly reference the
> site, because the content is not guaranteed to be stable, but we could
> maybe add it as "further reading".)

Looking again, the kernel doc referenced by Mark
(https://www.kernel.org/doc/html/v6.6/driver-api/spi.html) might also be
a good candidate, especially as we can refer to a specific version.


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

Re: [virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

On 11/27/2023 10:33 PM, Cornelia Huck wrote:
> On Mon, Nov 27 2023, Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>
>>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>>>>
>>>> "chipselect" is probably a known term for people familiar with SPI -- is
>>>> there any definition of those terms that the spec can point to?
>>>
>>> Just as Mark said, there is no formal spec for SPI, so no standard spec
>>> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
>>> below.
>>
>> If we have nothing to point to, it is probably best to simply
>> expand/explain the terms on their first usage.
>>
>>>> Can we point to some documentation that explains CPHA and CPOL?
>>>
>>> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in
>>> wikipedia(Clock polarity and phase chapter):
>>>
>>> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>>>
>>> How about copying some concise information from wikipedia as Note? Or is
>>> referring to such webpage acceptable in this spec.
>>
>> Not sure if we can do an outright copy (licence compatibility), but
>> paraphrasing should be fine. (I'd rather not directly reference the
>> site, because the content is not guaranteed to be stable, but we could
>> maybe add it as "further reading".)
> 
> Looking again, the kernel doc referenced by Mark
> (https://www.kernel.org/doc/html/v6.6/driver-api/spi.html) might also be
> a good candidate, especially as we can refer to a specific version.
> 
This Linux doc is a good Normative References, but it doesn't have 
definitions of the terms, like chipselect, CPOA, LSB...

I will add some concise descriptions on their first usage.

Best Regards
Thanks

Haixu Cui

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

On 11/27/2023 10:33 PM, Cornelia Huck wrote:
> On Mon, Nov 27 2023, Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>
>>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>>>>
>>>> "chipselect" is probably a known term for people familiar with SPI -- is
>>>> there any definition of those terms that the spec can point to?
>>>
>>> Just as Mark said, there is no formal spec for SPI, so no standard spec
>>> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
>>> below.
>>
>> If we have nothing to point to, it is probably best to simply
>> expand/explain the terms on their first usage.
>>
>>>> Can we point to some documentation that explains CPHA and CPOL?
>>>
>>> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in
>>> wikipedia(Clock polarity and phase chapter):
>>>
>>> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>>>
>>> How about copying some concise information from wikipedia as Note? Or is
>>> referring to such webpage acceptable in this spec.
>>
>> Not sure if we can do an outright copy (licence compatibility), but
>> paraphrasing should be fine. (I'd rather not directly reference the
>> site, because the content is not guaranteed to be stable, but we could
>> maybe add it as "further reading".)
> 
> Looking again, the kernel doc referenced by Mark
> (https://www.kernel.org/doc/html/v6.6/driver-api/spi.html) might also be
> a good candidate, especially as we can refer to a specific version.
> 
This Linux doc is a good Normative References, but it doesn't have 
definitions of the terms, like chipselect, CPOA, LSB...

I will add some concise descriptions on their first usage.

Best Regards
Thanks

Haixu Cui

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

[virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

Hi Huck,

On 11/27/2023 10:26 PM, Cornelia Huck wrote:
> On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>>>
>>> "chipselect" is probably a known term for people familiar with SPI -- is
>>> there any definition of those terms that the spec can point to?
>>
>> Just as Mark said, there is no formal spec for SPI, so no standard spec
>> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
>> below.
> 
> If we have nothing to point to, it is probably best to simply
> expand/explain the terms on their first usage.

OK, I will add description at where they are firstly used.
> 
>>> Can we point to some documentation that explains CPHA and CPOL?
>>
>> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in
>> wikipedia(Clock polarity and phase chapter):
>>
>> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>>
>> How about copying some concise information from wikipedia as Note? Or is
>> referring to such webpage acceptable in this spec.
> 
> Not sure if we can do an outright copy (licence compatibility), but
> paraphrasing should be fine. (I'd rather not directly reference the
> site, because the content is not guaranteed to be stable, but we could
> maybe add it as "further reading".)
> 
>>>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
>>>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
>>>
>>> s/filed/field/
>>> s/host/device/
>>>
>>> Also, isn't the rejecting supposed to be done by the device, as the
>>> driver is the party enqueueing the requests? Or do I have some kind of
>>> fundamental misunderstanding?
>>
>> It may be better to filter some invalid requests by driver, as in the
>> request header there are many parameters, and some of them are not
>> supported by device, so it's quite possible that many requests invalid
>> for the device. So if driver can do the first filter, such invalid
>> requests will not be sent at all, this will conserve virtqueue and
>> system overhead.
>>
>> And this is why exposing device supported features in the config space,
>> it ensures that almost all requests in virtqueue are nice to the backend.
>>
>> device also will verify the requests again, as the following requirement:
>> Virtio SPI device MUST verify the parameters in
>> \field{virtio_spi_transfer_head} after receiving the request,
>> and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR
>> if not all parameters are valid or some device unsupported features are set.
>>
>> Although checking the requests twice seems a little redundant, it is
>> more efficient comparing with sending some invalid requests to the device.
>>
>> What is your opinion? Do you think it is acceptable?
> 
> Thanks for your explanation, I think we simply have some terminology
> issues. In the virtio spec, "driver" refers to one side of the
> driver/device pair, and is used to describe how to communicate with the
> device. In this case, "driver" would be the entity interacting with the
> device, regardless of how it is implemented, and would be responsible
> for sending the requests. Any filtering that would be done in a concrete
> implementation (i.e. if you have one component generating the requests,
> and then another component filtering them before actually putting them
> into the queue) is out of scope for this spec -- we can maybe specify
> that the driver should not send invalid requests, but I'm not sure that
> this is actually needed.

Got it. I agree with you, and I suggest that driver should do some 
filter jobs to reduce the system overhead, but this should not be 
compulsory for driver.

Thank you for your ideas.

Best Regards
Haixu Cui
> 
>> Once again, thanks a lot for your support.
> 
> You're welcome!
> 

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

Hi Huck,

On 11/27/2023 10:26 PM, Cornelia Huck wrote:
> On Mon, Nov 27 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>> On Fri, Nov 24 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>>>
>>> "chipselect" is probably a known term for people familiar with SPI -- is
>>> there any definition of those terms that the spec can point to?
>>
>> Just as Mark said, there is no formal spec for SPI, so no standard spec
>> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
>> below.
> 
> If we have nothing to point to, it is probably best to simply
> expand/explain the terms on their first usage.

OK, I will add description at where they are firstly used.
> 
>>> Can we point to some documentation that explains CPHA and CPOL?
>>
>> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in
>> wikipedia(Clock polarity and phase chapter):
>>
>> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>>
>> How about copying some concise information from wikipedia as Note? Or is
>> referring to such webpage acceptable in this spec.
> 
> Not sure if we can do an outright copy (licence compatibility), but
> paraphrasing should be fine. (I'd rather not directly reference the
> site, because the content is not guaranteed to be stable, but we could
> maybe add it as "further reading".)
> 
>>>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
>>>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
>>>
>>> s/filed/field/
>>> s/host/device/
>>>
>>> Also, isn't the rejecting supposed to be done by the device, as the
>>> driver is the party enqueueing the requests? Or do I have some kind of
>>> fundamental misunderstanding?
>>
>> It may be better to filter some invalid requests by driver, as in the
>> request header there are many parameters, and some of them are not
>> supported by device, so it's quite possible that many requests invalid
>> for the device. So if driver can do the first filter, such invalid
>> requests will not be sent at all, this will conserve virtqueue and
>> system overhead.
>>
>> And this is why exposing device supported features in the config space,
>> it ensures that almost all requests in virtqueue are nice to the backend.
>>
>> device also will verify the requests again, as the following requirement:
>> Virtio SPI device MUST verify the parameters in
>> \field{virtio_spi_transfer_head} after receiving the request,
>> and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR
>> if not all parameters are valid or some device unsupported features are set.
>>
>> Although checking the requests twice seems a little redundant, it is
>> more efficient comparing with sending some invalid requests to the device.
>>
>> What is your opinion? Do you think it is acceptable?
> 
> Thanks for your explanation, I think we simply have some terminology
> issues. In the virtio spec, "driver" refers to one side of the
> driver/device pair, and is used to describe how to communicate with the
> device. In this case, "driver" would be the entity interacting with the
> device, regardless of how it is implemented, and would be responsible
> for sending the requests. Any filtering that would be done in a concrete
> implementation (i.e. if you have one component generating the requests,
> and then another component filtering them before actually putting them
> into the queue) is out of scope for this spec -- we can maybe specify
> that the driver should not send invalid requests, but I'm not sure that
> this is actually needed.

Got it. I agree with you, and I suggest that driver should do some 
filter jobs to reduce the system overhead, but this should not be 
compulsory for driver.

Thank you for your ideas.

Best Regards
Haixu Cui
> 
>> Once again, thanks a lot for your support.
> 
> You're welcome!
> 

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

Hi Viresh,

     Thank you for your comments and please refer to my replies below 
each comment.

On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> Hi Haixu,
> 
> Cornelia has already covered most of the issues, I will try to add few more
> things I noticed.
> 
> On 24-11-23, 15:20, Haixu Cui wrote:
>> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> a guest to operate and use the host SPI controller.
>>
>> This patch adds the specification for virtio-spi.
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> ---
>>   device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 +
>>   device-types/spi/driver-conformance.tex |   7 +
>>   3 files changed, 295 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..8ca8c0d
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,281 @@
>> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
> 
> Not sure if we should use "Master" anywhere..
> Maybe: s/SPI Master Device/SPI Device/ ?
> 
> Applies elsewhere too..

Master here shows the spec is for SPI master rather than SPI slave.

But Master is outdated term and will be replaced by "SPI controller 
device" in next patch.

> 
>> +
>> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> 
> Maybe:
> 
> s/virtio-spi/The Virtio SPI device/ (this applies at multiple places)
> 
> s/and it allows/that allows/
> 
> ?

Okay, "The Virtio SPI device" is much better than virtio-spi.

> 
>> +a guest to operate and use the host SPI controller. Host SPI controller may be a
> 
> Maybe: The host SPI controller ... ?
> 
>> +physical controller, or a virtual one(e.g. controller emulated by the software
>> +running in the host).
>> +
>> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
> 
> The Virtio SPI device..

Likewise.

> 
>> +the virtqueue, and serviced by the host SPI controller.
>> +
>> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>> +the front-end running in the guest kernel, and Virtio SPI device acts as the
>> +back-end in the host.
>> +
>> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
>> +45
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] requestq
>> +\end{description}
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
>> +
>> +None
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
>> +
>> +All fields of this configuration are always available and read-only for Virtio SPI driver.
>> +The config space shows the features and settings supported by the host SPI controller.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +	u8 chip_select_max_number;
>> +	u8 cs_change_supported;
>> +	u8 tx_nbits_supported;
>> +	u8 rx_nbits_supported;
>> +	le32 bits_per_word_mask;
>> +	le32 mode_func_supported;
>> +	le32 max_freq_hz;
>> +	le32 max_word_delay_ns;
>> +	le32 max_cs_setup_ns;
>> +	le32 max_cs_hold_ns;
>> +	le32 max_cs_inactive_ns;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>> +
>> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
>> +after each transfer in one message:
>> +        0: supported;
>> +        1: unsupported, means chipselect will keep active when executing the message transaction.
>> +Note: Message here contains a sequence of SPI transfer >
> I would rather suggest using '1' for supported and '0' for unsupported, like you
> have done for LSB first, loopback, etc. later..

OK, will update in next patch.

> 
>> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> 
> Maybe you can mention first that, a set bit means feature is supported,
> otherwise now. And then you can just write:
> 
>          bit 0: 2-bit transfer
>          bit 1: 4-bit transfer
>          bit 2: 8-bit transfer
> 
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
> 
> Maybe write as: other bits are reserved and MUST be set to 0 by the device.
> 

Agree. Will update.

>> +
>> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
> 
> Same here..
> 
>> +
>> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
>> +are supported. If not set, no limitation for bits_per_word.
>> +Note: bits_per_word corresponds to \field{bits_per_word} field in
>> +structure \field{virtio_spi_transfer_head}, see below.
>> +
>> +The \field{mode_func_supported} indicates the following features are supported or not:
>> +        bit 0-1: CPHA feature,
>> +            0b00: supports CPHA=0 and CPHA=1;
>> +            0b01: supports CPHA=0 only;
>> +            0b10: supports CPHA=1 only;
>> +            0b11: invalid, should support as least one CPHA setting.
>> +
>> +        bit 2-3: CPOL feature,
>> +            0b00: supports CPOL=0 and CPOL=1;
>> +            0b01: supports CPOL=0 only;
>> +            0b10: supports CPOL=1 only;
>> +            0b11: invalid, should support as least one CPOL setting.
> 
> Maybe explain what CPHA and CPOL are ..

I will add Note to explain these terms here.
> 
>> +
>> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
>> +            chipselect active low should always be supported.
>> +
>> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
>> +            supported.
>> +
>> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
>> +            should always be supported.
>> +
>> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
>> +limitation for transfer speed.
>> +
>> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
>> +word delay feature is unsupported.
>> +Note: Just as one message contains a sequence of transfers, one transfer may contain
>> +a sequence of word.
>> +
>> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is asserted.
>> +
>> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce before chipselect is deasserted.
>> +
>> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is deasserted.
>> +
>> +\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.
> 
> The Virtio SPI driver..
> 
>> +Each request represents one SPI tranfer and is of the form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.
>> +
>> +The \field{rx_nbits} indicates number of bits used for reading:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.

Agree. Will update.

> 
> The description of the above two fields look exactly same, can we define this
> just once and mention it applies for both ?
> 
>> +The \field{reserved} is for alignement, also for further extension.
>> +
>> +The \field{mode} indicates some transfer settings. 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 from the device.
>> +
>> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
>> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
>> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
>> +valid but trasnfer process failed.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_TRANS_OK     0
>> +#define VIRTIO_SPI_PARAM_ERR    1
>> +#define VIRTIO_SPI_TRANS_ERR    2
>> +\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
> 
> See, you are already using "Virtio SPI device/driver" here. Just use the same
> throughout instead of "virtio-spi".
> 
>> +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.
> 
> Should the length of both the buffers in full-duplex mode be same ? If yes, then
> this should be mentioned (in case it is not).
> 

No, there is no such limitation. Write and read buffers may be different 
is size.

>> +
>> +\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.
> 
> s/structure \field{virtio_spi_transfer_head}/\field{struct virtio_spi_transfer_head}/ ?
> 
> That should automatically add struct before its name.
> 
Got it. I will update the statement.

>> +
>> +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 read or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
> 
> s/ or /, /
> 
>> +if the \field{result} returned from Virtio SPI device is not VIRTIO_SPI_TRANS_OK.
>> +
>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
> 
> s/driver/device/
> 
>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
> 
> s/filed/field/
> s/features not supported by host/features is not supported by the host/

As Cornelia has suggested, I will remove this requirement and reserve 
the following one:

Virtio SPI device MUST verify the parameters in 
\field{virtio_spi_transfer_head} after receiving the request,
and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR 
if not all parameters are valid or some host SPI controller unsupported 
features are set.

> 
>> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
>> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
>> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.
> 
> As I said earlier, maybe we should talk about length of the rx/tx buffers for
> full-duplex transfers here.
>  >> +
>> +\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.
> 
> The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
> there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.
> 

Sorry I don'y understand here. In this statement, the transfer type is 
either half-duplex write or full-duplex read and write. half-duplex read 
type has no tx_buf, so I didn't mention it.

Looking forware to your comments.

>> +
>> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
>> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
>> +some host SPI controller unsupported features are set.
>> 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:
> 
> The Virtio SPI device
> 
>> +
>> +\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}
> 
> Thanks.
> 
Thank you very much again for your supports.

Best Regards
Haixu Cui

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

Hi Viresh,

     Thank you for your comments and please refer to my replies below 
each comment.

On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> Hi Haixu,
> 
> Cornelia has already covered most of the issues, I will try to add few more
> things I noticed.
> 
> On 24-11-23, 15:20, Haixu Cui wrote:
>> virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
>> a guest to operate and use the host SPI controller.
>>
>> This patch adds the specification for virtio-spi.
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> ---
>>   device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>>   device-types/spi/device-conformance.tex |   7 +
>>   device-types/spi/driver-conformance.tex |   7 +
>>   3 files changed, 295 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..8ca8c0d
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,281 @@
>> +\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
> 
> Not sure if we should use "Master" anywhere..
> Maybe: s/SPI Master Device/SPI Device/ ?
> 
> Applies elsewhere too..

Master here shows the spec is for SPI master rather than SPI slave.

But Master is outdated term and will be replaced by "SPI controller 
device" in next patch.

> 
>> +
>> +virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it allows
> 
> Maybe:
> 
> s/virtio-spi/The Virtio SPI device/ (this applies at multiple places)
> 
> s/and it allows/that allows/
> 
> ?

Okay, "The Virtio SPI device" is much better than virtio-spi.

> 
>> +a guest to operate and use the host SPI controller. Host SPI controller may be a
> 
> Maybe: The host SPI controller ... ?
> 
>> +physical controller, or a virtual one(e.g. controller emulated by the software
>> +running in the host).
>> +
>> +virtio-spi has a single virtqueue. SPI transfer requests are placed into
> 
> The Virtio SPI device..

Likewise.

> 
>> +the virtqueue, and serviced by the host SPI controller.
>> +
>> +In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
>> +the front-end running in the guest kernel, and Virtio SPI device acts as the
>> +back-end in the host.
>> +
>> +\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
>> +45
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] requestq
>> +\end{description}
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature bits}
>> +
>> +None
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / SPI Master Device / Device configuration layout}
>> +
>> +All fields of this configuration are always available and read-only for Virtio SPI driver.
>> +The config space shows the features and settings supported by the host SPI controller.
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_config {
>> +	u8 chip_select_max_number;
>> +	u8 cs_change_supported;
>> +	u8 tx_nbits_supported;
>> +	u8 rx_nbits_supported;
>> +	le32 bits_per_word_mask;
>> +	le32 mode_func_supported;
>> +	le32 max_freq_hz;
>> +	le32 max_word_delay_ns;
>> +	le32 max_cs_setup_ns;
>> +	le32 max_cs_hold_ns;
>> +	le32 max_cs_inactive_ns;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{chip_select_max_number} is the maximum number of chipselect the host SPI controller supports.
>> +
>> +The \field{cs_change_supported} indicates if the host SPI controller supports to toggle chipselect
>> +after each transfer in one message:
>> +        0: supported;
>> +        1: unsupported, means chipselect will keep active when executing the message transaction.
>> +Note: Message here contains a sequence of SPI transfer >
> I would rather suggest using '1' for supported and '0' for unsupported, like you
> have done for LSB first, loopback, etc. later..

OK, will update in next patch.

> 
>> +The \field{tx_nbits_supported} indicates the supported number of bit for writing:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
> 
> Maybe you can mention first that, a set bit means feature is supported,
> otherwise now. And then you can just write:
> 
>          bit 0: 2-bit transfer
>          bit 1: 4-bit transfer
>          bit 2: 8-bit transfer
> 
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
> 
> Maybe write as: other bits are reserved and MUST be set to 0 by the device.
> 

Agree. Will update.

>> +
>> +The \field{rx_nbits_supported} indicates the supported number of bit for reading:
>> +        bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
>> +        bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
>> +        other bits are reserved as 0, 1-bit transfer is always supported.
> 
> Same here..
> 
>> +
>> +The \field{bits_per_word_mask} is a mask indicating which values of bits_per_word
>> +are supported. If not set, no limitation for bits_per_word.
>> +Note: bits_per_word corresponds to \field{bits_per_word} field in
>> +structure \field{virtio_spi_transfer_head}, see below.
>> +
>> +The \field{mode_func_supported} indicates the following features are supported or not:
>> +        bit 0-1: CPHA feature,
>> +            0b00: supports CPHA=0 and CPHA=1;
>> +            0b01: supports CPHA=0 only;
>> +            0b10: supports CPHA=1 only;
>> +            0b11: invalid, should support as least one CPHA setting.
>> +
>> +        bit 2-3: CPOL feature,
>> +            0b00: supports CPOL=0 and CPOL=1;
>> +            0b01: supports CPOL=0 only;
>> +            0b10: supports CPOL=1 only;
>> +            0b11: invalid, should support as least one CPOL setting.
> 
> Maybe explain what CPHA and CPOL are ..

I will add Note to explain these terms here.
> 
>> +
>> +        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
>> +            chipselect active low should always be supported.
>> +
>> +        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first should always be
>> +            supported.
>> +
>> +        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
>> +            should always be supported.
>> +
>> +The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no
>> +limitation for transfer speed.
>> +
>> +The \field{max_word_delay_ns} is the maximum word delay supported in ns unit, 0 means
>> +word delay feature is unsupported.
>> +Note: Just as one message contains a sequence of transfers, one transfer may contain
>> +a sequence of word.
>> +
>> +The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is asserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is asserted.
>> +
>> +The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce before chipselect is deasserted.
>> +
>> +The \field{max_cs_incative_ns} is the maximum delay supported after chipselect is deasserted, in ns unit,
>> +0 means delay is not supported to introduce after chipselect is deasserted.
>> +
>> +\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.
> 
> The Virtio SPI driver..
> 
>> +Each request represents one SPI tranfer and is of the form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_spi_transfer_head {
>> +        u8 chip_select_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{chip_select_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 number of bits used for writing:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.
>> +
>> +The \field{rx_nbits} indicates number of bits used for reading:
>> +        0,1: 1-bit transfer, also known as SINGLE;
>> +        2  : 2-bit transfer, also known as DUAL;
>> +        4  : 4-bit transfer, also known as QUAD;
>> +        8  : 8-bit transfer, also known as OCTAL;
>> +        other values are invalid.

Agree. Will update.

> 
> The description of the above two fields look exactly same, can we define this
> just once and mention it applies for both ?
> 
>> +The \field{reserved} is for alignement, also for further extension.
>> +
>> +The \field{mode} indicates some transfer settings. 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 from the device.
>> +
>> +The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for success,
>> +VIRTIO_SPI_PARAM_ERR for parameter error, which means 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, while
>> +VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
>> +valid but trasnfer process failed.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_SPI_TRANS_OK     0
>> +#define VIRTIO_SPI_PARAM_ERR    1
>> +#define VIRTIO_SPI_TRANS_ERR    2
>> +\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
> 
> See, you are already using "Virtio SPI device/driver" here. Just use the same
> throughout instead of "virtio-spi".
> 
>> +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.
> 
> Should the length of both the buffers in full-duplex mode be same ? If yes, then
> this should be mentioned (in case it is not).
> 

No, there is no such limitation. Write and read buffers may be different 
is size.

>> +
>> +\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.
> 
> s/structure \field{virtio_spi_transfer_head}/\field{struct virtio_spi_transfer_head}/ ?
> 
> That should automatically add struct before its name.
> 
Got it. I will update the statement.

>> +
>> +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 read or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
> 
> s/ or /, /
> 
>> +if the \field{result} returned from Virtio SPI device is not VIRTIO_SPI_TRANS_OK.
>> +
>> +For each transfer request, Virtio SPI driver MUST check the fields in structure \field{virtio_spi_transfer_head}
> 
> s/driver/device/
> 
>> +and MUST reject the request if any filed is invalid or enabling the features not supported by host.
> 
> s/filed/field/
> s/features not supported by host/features is not supported by the host/

As Cornelia has suggested, I will remove this requirement and reserve 
the following one:

Virtio SPI device MUST verify the parameters in 
\field{virtio_spi_transfer_head} after receiving the request,
and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR 
if not all parameters are valid or some host SPI controller unsupported 
features are set.

> 
>> +For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is greater than \field{max_freq_hz}
>> +in config space, or if \field{max_cs_setup_ns} in config space is 0 while \field{cs_setup_ns} in structure
>> +\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject such request.
> 
> As I said earlier, maybe we should talk about length of the rx/tx buffers for
> full-duplex transfers here.
>  >> +
>> +\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.
> 
> The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
> there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.
> 

Sorry I don'y understand here. In this statement, the transfer type is 
either half-duplex write or full-duplex read and write. half-duplex read 
type has no tx_buf, so I didn't mention it.

Looking forware to your comments.

>> +
>> +Virtio SPI device MUST verify the parameters in \field{virtio_spi_transfer_head} after receiving the request,
>> +and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all parameters are valid or
>> +some host SPI controller unsupported features are set.
>> 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:
> 
> The Virtio SPI device
> 
>> +
>> +\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}
> 
> Thanks.
> 
Thank you very much again for your supports.

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Viresh Kumar]

On 28-11-23, 20:58, Haixu Cui wrote:
> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> > On 24-11-23, 15:20, Haixu Cui wrote:
> > > +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.
> > 
> > Should the length of both the buffers in full-duplex mode be same ? If yes, then
> > this should be mentioned (in case it is not).
> > 
> 
> No, there is no such limitation. Write and read buffers may be different is
> size.

Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
something here I guess. But from whatever little I remember, with full-duplex
transfer, data flows on both MOSI and MISO lines as soon as clock signal is
applied.  And so amount of data sent is always be equal to amount of data
received by both sides.

Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
`tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
Which I guess is indicating that both buffers are supposed to be of same length.

What am I missing ?

> > > +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.
> > 
> > The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
> > there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.
> > 
> 
> Sorry I don'y understand here. In this statement, the transfer type is
> either half-duplex write or full-duplex read and write. half-duplex read
> type has no tx_buf, so I didn't mention it.

Sorry for not being clear earlier. I was suggesting you to just write this
instead:

The Virtio SPI device MUST NOT change the data in the \field{tx_buf} field.

You don't really need to specify the modes here, as we are talking about tx_buf,
which isn't available in the half-duplex read mode but all others.

--
Viresh

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/spi/spi.h#n1031

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Viresh Kumar]

On 28-11-23, 20:58, Haixu Cui wrote:
> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> > On 24-11-23, 15:20, Haixu Cui wrote:
> > > +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.
> > 
> > Should the length of both the buffers in full-duplex mode be same ? If yes, then
> > this should be mentioned (in case it is not).
> > 
> 
> No, there is no such limitation. Write and read buffers may be different is
> size.

Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
something here I guess. But from whatever little I remember, with full-duplex
transfer, data flows on both MOSI and MISO lines as soon as clock signal is
applied.  And so amount of data sent is always be equal to amount of data
received by both sides.

Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
`tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
Which I guess is indicating that both buffers are supposed to be of same length.

What am I missing ?

> > > +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.
> > 
> > The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
> > there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.
> > 
> 
> Sorry I don'y understand here. In this statement, the transfer type is
> either half-duplex write or full-duplex read and write. half-duplex read
> type has no tx_buf, so I didn't mention it.

Sorry for not being clear earlier. I was suggesting you to just write this
instead:

The Virtio SPI device MUST NOT change the data in the \field{tx_buf} field.

You don't really need to specify the modes here, as we are talking about tx_buf,
which isn't available in the half-duplex read mode but all others.

--
Viresh

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/spi/spi.h#n1031

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

Hi Viresh,

On 11/29/2023 3:30 PM, Viresh Kumar wrote:
> On 28-11-23, 20:58, Haixu Cui wrote:
>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>> On 24-11-23, 15:20, Haixu Cui wrote:
>>>> +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.
>>>
>>> Should the length of both the buffers in full-duplex mode be same ? If yes, then
>>> this should be mentioned (in case it is not).
>>>
>>
>> No, there is no such limitation. Write and read buffers may be different is
>> size.
> 
> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
> something here I guess. But from whatever little I remember, with full-duplex
> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
> applied.  And so amount of data sent is always be equal to amount of data
> received by both sides.
> 
> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
> Which I guess is indicating that both buffers are supposed to be of same length.
> 
> What am I missing ?

Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf 
have the same size, Linux has such restriction. Just as you mention, 
kernel level spi_transfer has single "len", the same for 
spi_ioc_transfer passed from the userland.

But I am not sure if this is in the scope of the spec. Because this is 
ensured by Linux, but Virtio SPI driver won't also can't verify this.
This is a prerequisite for virtio spi processing requests.

What is your suggestion? How about adding some descriptions here, like 
"for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed 
by the kernel"?

> 
>>>> +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.
>>>
>>> The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
>>> there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.
>>>
>>
>> Sorry I don'y understand here. In this statement, the transfer type is
>> either half-duplex write or full-duplex read and write. half-duplex read
>> type has no tx_buf, so I didn't mention it.
> 
> Sorry for not being clear earlier. I was suggesting you to just write this
> instead:
> 
> The Virtio SPI device MUST NOT change the data in the \field{tx_buf} field.
> 
> You don't really need to specify the modes here, as we are talking about tx_buf,
> which isn't available in the half-duplex read mode but all others.
> 

OK, thank you for your explanation. I will remove the "if" statement here.

> -- > Viresh
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/spi/spi.h#n1031

Thanks again.

Best Regards
Haixu Cui

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

Hi Viresh,

On 11/29/2023 3:30 PM, Viresh Kumar wrote:
> On 28-11-23, 20:58, Haixu Cui wrote:
>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>> On 24-11-23, 15:20, Haixu Cui wrote:
>>>> +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.
>>>
>>> Should the length of both the buffers in full-duplex mode be same ? If yes, then
>>> this should be mentioned (in case it is not).
>>>
>>
>> No, there is no such limitation. Write and read buffers may be different is
>> size.
> 
> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
> something here I guess. But from whatever little I remember, with full-duplex
> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
> applied.  And so amount of data sent is always be equal to amount of data
> received by both sides.
> 
> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
> Which I guess is indicating that both buffers are supposed to be of same length.
> 
> What am I missing ?

Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf 
have the same size, Linux has such restriction. Just as you mention, 
kernel level spi_transfer has single "len", the same for 
spi_ioc_transfer passed from the userland.

But I am not sure if this is in the scope of the spec. Because this is 
ensured by Linux, but Virtio SPI driver won't also can't verify this.
This is a prerequisite for virtio spi processing requests.

What is your suggestion? How about adding some descriptions here, like 
"for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed 
by the kernel"?

> 
>>>> +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.
>>>
>>> The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
>>> there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.
>>>
>>
>> Sorry I don'y understand here. In this statement, the transfer type is
>> either half-duplex write or full-duplex read and write. half-duplex read
>> type has no tx_buf, so I didn't mention it.
> 
> Sorry for not being clear earlier. I was suggesting you to just write this
> instead:
> 
> The Virtio SPI device MUST NOT change the data in the \field{tx_buf} field.
> 
> You don't really need to specify the modes here, as we are talking about tx_buf,
> which isn't available in the half-duplex read mode but all others.
> 

OK, thank you for your explanation. I will remove the "if" statement here.

> -- > Viresh
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/spi/spi.h#n1031

Thanks again.

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Cornelia Huck]

On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Viresh,
>
> On 11/29/2023 3:30 PM, Viresh Kumar wrote:
>> On 28-11-23, 20:58, Haixu Cui wrote:
>>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>>> On 24-11-23, 15:20, Haixu Cui wrote:
>>>>> +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.
>>>>
>>>> Should the length of both the buffers in full-duplex mode be same ? If yes, then
>>>> this should be mentioned (in case it is not).
>>>>
>>>
>>> No, there is no such limitation. Write and read buffers may be different is
>>> size.
>> 
>> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
>> something here I guess. But from whatever little I remember, with full-duplex
>> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
>> applied.  And so amount of data sent is always be equal to amount of data
>> received by both sides.
>> 
>> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
>> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
>> Which I guess is indicating that both buffers are supposed to be of same length.
>> 
>> What am I missing ?
>
> Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf 
> have the same size, Linux has such restriction. Just as you mention, 
> kernel level spi_transfer has single "len", the same for 
> spi_ioc_transfer passed from the userland.
>
> But I am not sure if this is in the scope of the spec. Because this is 
> ensured by Linux, but Virtio SPI driver won't also can't verify this.
> This is a prerequisite for virtio spi processing requests.
>
> What is your suggestion? How about adding some descriptions here, like 
> "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed 
> by the kernel"?

We must not really make any assumptions in the spec about concrete
implementations (here, the Linux kernel), as someone implementing it in
a different environment will need to make explicit choices.

So, if tx_buf and rx_buf are required to be of the same size, it needs
to be explicitly stated in the spec, or an implementation might choose
to do it differently.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Cornelia Huck]

On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Viresh,
>
> On 11/29/2023 3:30 PM, Viresh Kumar wrote:
>> On 28-11-23, 20:58, Haixu Cui wrote:
>>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>>> On 24-11-23, 15:20, Haixu Cui wrote:
>>>>> +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.
>>>>
>>>> Should the length of both the buffers in full-duplex mode be same ? If yes, then
>>>> this should be mentioned (in case it is not).
>>>>
>>>
>>> No, there is no such limitation. Write and read buffers may be different is
>>> size.
>> 
>> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
>> something here I guess. But from whatever little I remember, with full-duplex
>> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
>> applied.  And so amount of data sent is always be equal to amount of data
>> received by both sides.
>> 
>> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
>> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
>> Which I guess is indicating that both buffers are supposed to be of same length.
>> 
>> What am I missing ?
>
> Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf 
> have the same size, Linux has such restriction. Just as you mention, 
> kernel level spi_transfer has single "len", the same for 
> spi_ioc_transfer passed from the userland.
>
> But I am not sure if this is in the scope of the spec. Because this is 
> ensured by Linux, but Virtio SPI driver won't also can't verify this.
> This is a prerequisite for virtio spi processing requests.
>
> What is your suggestion? How about adding some descriptions here, like 
> "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed 
> by the kernel"?

We must not really make any assumptions in the spec about concrete
implementations (here, the Linux kernel), as someone implementing it in
a different environment will need to make explicit choices.

So, if tx_buf and rx_buf are required to be of the same size, it needs
to be explicitly stated in the spec, or an implementation might choose
to do it differently.


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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

On 11/29/2023 4:33 PM, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> Hi Viresh,
>>
>> On 11/29/2023 3:30 PM, Viresh Kumar wrote:
>>> On 28-11-23, 20:58, Haixu Cui wrote:
>>>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>>>> On 24-11-23, 15:20, Haixu Cui wrote:
>>>>>> +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.
>>>>>
>>>>> Should the length of both the buffers in full-duplex mode be same ? If yes, then
>>>>> this should be mentioned (in case it is not).
>>>>>
>>>>
>>>> No, there is no such limitation. Write and read buffers may be different is
>>>> size.
>>>
>>> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
>>> something here I guess. But from whatever little I remember, with full-duplex
>>> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
>>> applied.  And so amount of data sent is always be equal to amount of data
>>> received by both sides.
>>>
>>> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
>>> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
>>> Which I guess is indicating that both buffers are supposed to be of same length.
>>>
>>> What am I missing ?
>>
>> Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
>> have the same size, Linux has such restriction. Just as you mention,
>> kernel level spi_transfer has single "len", the same for
>> spi_ioc_transfer passed from the userland.
>>
>> But I am not sure if this is in the scope of the spec. Because this is
>> ensured by Linux, but Virtio SPI driver won't also can't verify this.
>> This is a prerequisite for virtio spi processing requests.
>>
>> What is your suggestion? How about adding some descriptions here, like
>> "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
>> by the kernel"?
> 
> We must not really make any assumptions in the spec about concrete
> implementations (here, the Linux kernel), as someone implementing it in
> a different environment will need to make explicit choices.
> 
> So, if tx_buf and rx_buf are required to be of the same size, it needs
> to be explicitly stated in the spec, or an implementation might choose
> to do it differently.

Okay! Thanks Huck for guidance and thanks Viresh for pointing it out. 
Much appreciated.

Best Regards
Haixu Cui

> 
> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
> 


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: 
https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

On 11/29/2023 4:33 PM, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> Hi Viresh,
>>
>> On 11/29/2023 3:30 PM, Viresh Kumar wrote:
>>> On 28-11-23, 20:58, Haixu Cui wrote:
>>>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>>>> On 24-11-23, 15:20, Haixu Cui wrote:
>>>>>> +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.
>>>>>
>>>>> Should the length of both the buffers in full-duplex mode be same ? If yes, then
>>>>> this should be mentioned (in case it is not).
>>>>>
>>>>
>>>> No, there is no such limitation. Write and read buffers may be different is
>>>> size.
>>>
>>> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
>>> something here I guess. But from whatever little I remember, with full-duplex
>>> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
>>> applied.  And so amount of data sent is always be equal to amount of data
>>> received by both sides.
>>>
>>> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
>>> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
>>> Which I guess is indicating that both buffers are supposed to be of same length.
>>>
>>> What am I missing ?
>>
>> Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
>> have the same size, Linux has such restriction. Just as you mention,
>> kernel level spi_transfer has single "len", the same for
>> spi_ioc_transfer passed from the userland.
>>
>> But I am not sure if this is in the scope of the spec. Because this is
>> ensured by Linux, but Virtio SPI driver won't also can't verify this.
>> This is a prerequisite for virtio spi processing requests.
>>
>> What is your suggestion? How about adding some descriptions here, like
>> "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
>> by the kernel"?
> 
> We must not really make any assumptions in the spec about concrete
> implementations (here, the Linux kernel), as someone implementing it in
> a different environment will need to make explicit choices.
> 
> So, if tx_buf and rx_buf are required to be of the same size, it needs
> to be explicitly stated in the spec, or an implementation might choose
> to do it differently.

Okay! Thanks Huck for guidance and thanks Viresh for pointing it out. 
Much appreciated.

Best Regards
Haixu Cui

> 
> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
> 


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: 
https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Viresh Kumar]

On 29-11-23, 16:54, Haixu Cui wrote:
> On 11/29/2023 4:33 PM, Cornelia Huck wrote:
> > On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> > > On 11/29/2023 3:30 PM, Viresh Kumar wrote:
> > > > On 28-11-23, 20:58, Haixu Cui wrote:
> > > > > On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> > > > Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
> > > > something here I guess. But from whatever little I remember, with full-duplex
> > > > transfer, data flows on both MOSI and MISO lines as soon as clock signal is
> > > > applied.  And so amount of data sent is always be equal to amount of data
> > > > received by both sides.
> > > > 
> > > > Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
> > > > `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
> > > > Which I guess is indicating that both buffers are supposed to be of same length.
> > > > 
> > > > What am I missing ?
> > > 
> > > Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
> > > have the same size, Linux has such restriction. Just as you mention,
> > > kernel level spi_transfer has single "len", the same for
> > > spi_ioc_transfer passed from the userland.
> > > 
> > > But I am not sure if this is in the scope of the spec. Because this is
> > > ensured by Linux, but Virtio SPI driver won't also can't verify this.
> > > This is a prerequisite for virtio spi processing requests.

I don't think this is a Linux only thing. This is how the SPI protocol is
designed to begin with. A clock is applied, data from FIFOs of both master and
slaves gets exchanged over the MOSI and MISO lines... And the length is _always_
the same. We are talking about the full-duplex mode of the hardware here and
this is how it works AFAICT.

So the spec should clearly point this out to avoid any mistakes. Or if you have
a usecase where you can have different lengths for tx and rx buffers, that I am
not aware of ?

> > > What is your suggestion? How about adding some descriptions here, like
> > > "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
> > > by the kernel"?
> > 
> > We must not really make any assumptions in the spec about concrete
> > implementations (here, the Linux kernel), as someone implementing it in
> > a different environment will need to make explicit choices.

I agree. I pointed this out since this is how the protocol works.

> > So, if tx_buf and rx_buf are required to be of the same size, it needs
> > to be explicitly stated in the spec, or an implementation might choose
> > to do it differently.

--
Viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Viresh Kumar]

On 29-11-23, 16:54, Haixu Cui wrote:
> On 11/29/2023 4:33 PM, Cornelia Huck wrote:
> > On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> > > On 11/29/2023 3:30 PM, Viresh Kumar wrote:
> > > > On 28-11-23, 20:58, Haixu Cui wrote:
> > > > > On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> > > > Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
> > > > something here I guess. But from whatever little I remember, with full-duplex
> > > > transfer, data flows on both MOSI and MISO lines as soon as clock signal is
> > > > applied.  And so amount of data sent is always be equal to amount of data
> > > > received by both sides.
> > > > 
> > > > Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
> > > > `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
> > > > Which I guess is indicating that both buffers are supposed to be of same length.
> > > > 
> > > > What am I missing ?
> > > 
> > > Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
> > > have the same size, Linux has such restriction. Just as you mention,
> > > kernel level spi_transfer has single "len", the same for
> > > spi_ioc_transfer passed from the userland.
> > > 
> > > But I am not sure if this is in the scope of the spec. Because this is
> > > ensured by Linux, but Virtio SPI driver won't also can't verify this.
> > > This is a prerequisite for virtio spi processing requests.

I don't think this is a Linux only thing. This is how the SPI protocol is
designed to begin with. A clock is applied, data from FIFOs of both master and
slaves gets exchanged over the MOSI and MISO lines... And the length is _always_
the same. We are talking about the full-duplex mode of the hardware here and
this is how it works AFAICT.

So the spec should clearly point this out to avoid any mistakes. Or if you have
a usecase where you can have different lengths for tx and rx buffers, that I am
not aware of ?

> > > What is your suggestion? How about adding some descriptions here, like
> > > "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
> > > by the kernel"?
> > 
> > We must not really make any assumptions in the spec about concrete
> > implementations (here, the Linux kernel), as someone implementing it in
> > a different environment will need to make explicit choices.

I agree. I pointed this out since this is how the protocol works.

> > So, if tx_buf and rx_buf are required to be of the same size, it needs
> > to be explicitly stated in the spec, or an implementation might choose
> > to do it differently.

--
Viresh

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

On 11/29/2023 6:00 PM, Viresh Kumar wrote:
> On 29-11-23, 16:54, Haixu Cui wrote:
>> On 11/29/2023 4:33 PM, Cornelia Huck wrote:
>>> On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>> On 11/29/2023 3:30 PM, Viresh Kumar wrote:
>>>>> On 28-11-23, 20:58, Haixu Cui wrote:
>>>>>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>>>> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
>>>>> something here I guess. But from whatever little I remember, with full-duplex
>>>>> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
>>>>> applied.  And so amount of data sent is always be equal to amount of data
>>>>> received by both sides.
>>>>>
>>>>> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
>>>>> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
>>>>> Which I guess is indicating that both buffers are supposed to be of same length.
>>>>>
>>>>> What am I missing ?
>>>>
>>>> Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
>>>> have the same size, Linux has such restriction. Just as you mention,
>>>> kernel level spi_transfer has single "len", the same for
>>>> spi_ioc_transfer passed from the userland.
>>>>
>>>> But I am not sure if this is in the scope of the spec. Because this is
>>>> ensured by Linux, but Virtio SPI driver won't also can't verify this.
>>>> This is a prerequisite for virtio spi processing requests.
> 
> I don't think this is a Linux only thing. This is how the SPI protocol is
> designed to begin with. A clock is applied, data from FIFOs of both master and
> slaves gets exchanged over the MOSI and MISO lines... And the length is _always_
> the same. We are talking about the full-duplex mode of the hardware here and
> this is how it works AFAICT.
> 
> So the spec should clearly point this out to avoid any mistakes. Or if you have
> a usecase where you can have different lengths for tx and rx buffers, that I am
> not aware of ?

Hi Viresh,

     Thank you for your helpful comments. In next patch, I will clearly 
point this out:

     "For full-duplex read and write transfer, both \field{tx_buf} and 
\field{rx_buf} are used and the buffer size of \field{tx_buf} must be 
same as \field{rx_buf}."

     And in drivernormative section, I will add a requirement:
     "For full-duplex transfer, Virtio SPI driver MUST guarantee the 
write transfer size is equal to the read transfer size"

     In fact, in my prototype, I only perform the transfers with same 
tx/rx length.

     Much appreciated.

Best Regards
Haixu Cui
> 
>>>> What is your suggestion? How about adding some descriptions here, like
>>>> "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
>>>> by the kernel"?
>>>
>>> We must not really make any assumptions in the spec about concrete
>>> implementations (here, the Linux kernel), as someone implementing it in
>>> a different environment will need to make explicit choices.
> 
> I agree. I pointed this out since this is how the protocol works.
> 
>>> So, if tx_buf and rx_buf are required to be of the same size, it needs
>>> to be explicitly stated in the spec, or an implementation might choose
>>> to do it differently.
> 
> --
> Viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

On 11/29/2023 6:00 PM, Viresh Kumar wrote:
> On 29-11-23, 16:54, Haixu Cui wrote:
>> On 11/29/2023 4:33 PM, Cornelia Huck wrote:
>>> On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>> On 11/29/2023 3:30 PM, Viresh Kumar wrote:
>>>>> On 28-11-23, 20:58, Haixu Cui wrote:
>>>>>> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
>>>>> Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
>>>>> something here I guess. But from whatever little I remember, with full-duplex
>>>>> transfer, data flows on both MOSI and MISO lines as soon as clock signal is
>>>>> applied.  And so amount of data sent is always be equal to amount of data
>>>>> received by both sides.
>>>>>
>>>>> Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
>>>>> `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
>>>>> Which I guess is indicating that both buffers are supposed to be of same length.
>>>>>
>>>>> What am I missing ?
>>>>
>>>> Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
>>>> have the same size, Linux has such restriction. Just as you mention,
>>>> kernel level spi_transfer has single "len", the same for
>>>> spi_ioc_transfer passed from the userland.
>>>>
>>>> But I am not sure if this is in the scope of the spec. Because this is
>>>> ensured by Linux, but Virtio SPI driver won't also can't verify this.
>>>> This is a prerequisite for virtio spi processing requests.
> 
> I don't think this is a Linux only thing. This is how the SPI protocol is
> designed to begin with. A clock is applied, data from FIFOs of both master and
> slaves gets exchanged over the MOSI and MISO lines... And the length is _always_
> the same. We are talking about the full-duplex mode of the hardware here and
> this is how it works AFAICT.
> 
> So the spec should clearly point this out to avoid any mistakes. Or if you have
> a usecase where you can have different lengths for tx and rx buffers, that I am
> not aware of ?

Hi Viresh,

     Thank you for your helpful comments. In next patch, I will clearly 
point this out:

     "For full-duplex read and write transfer, both \field{tx_buf} and 
\field{rx_buf} are used and the buffer size of \field{tx_buf} must be 
same as \field{rx_buf}."

     And in drivernormative section, I will add a requirement:
     "For full-duplex transfer, Virtio SPI driver MUST guarantee the 
write transfer size is equal to the read transfer size"

     In fact, in my prototype, I only perform the transfers with same 
tx/rx length.

     Much appreciated.

Best Regards
Haixu Cui
> 
>>>> What is your suggestion? How about adding some descriptions here, like
>>>> "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
>>>> by the kernel"?
>>>
>>> We must not really make any assumptions in the spec about concrete
>>> implementations (here, the Linux kernel), as someone implementing it in
>>> a different environment will need to make explicit choices.
> 
> I agree. I pointed this out since this is how the protocol works.
> 
>>> So, if tx_buf and rx_buf are required to be of the same size, it needs
>>> to be explicitly stated in the spec, or an implementation might choose
>>> to do it differently.
> 
> --
> Viresh

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Viresh Kumar]

On 29-11-23, 18:31, Haixu Cui wrote:
> Hi Viresh,
> 
>     Thank you for your helpful comments. In next patch, I will clearly point
> this out:

Great, finally we are on the same page. Thanks Haixu.

>     "For full-duplex read and write transfer, both \field{tx_buf} and
> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
> as \field{rx_buf}."

Suggest rewriting as:

In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
buffers MUST be same.

>     And in drivernormative section, I will add a requirement:
>     "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
> transfer size is equal to the read transfer size"

Maybe:

drivernormative:

In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
length of both \field{tx_buf} and \field{rx_buf} are same.

devicenormative:

In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the
device MUST fail the transfer and notify the driver.

>     In fact, in my prototype, I only perform the transfers with same tx/rx
> length.

Great.

Thanks.

-- 
viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Viresh Kumar]

On 29-11-23, 18:31, Haixu Cui wrote:
> Hi Viresh,
> 
>     Thank you for your helpful comments. In next patch, I will clearly point
> this out:

Great, finally we are on the same page. Thanks Haixu.

>     "For full-duplex read and write transfer, both \field{tx_buf} and
> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
> as \field{rx_buf}."

Suggest rewriting as:

In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
buffers MUST be same.

>     And in drivernormative section, I will add a requirement:
>     "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
> transfer size is equal to the read transfer size"

Maybe:

drivernormative:

In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
length of both \field{tx_buf} and \field{rx_buf} are same.

devicenormative:

In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the
device MUST fail the transfer and notify the driver.

>     In fact, in my prototype, I only perform the transfers with same tx/rx
> length.

Great.

Thanks.

-- 
viresh

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Cornelia Huck]

On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 29-11-23, 18:31, Haixu Cui wrote:
>> Hi Viresh,
>> 
>>     Thank you for your helpful comments. In next patch, I will clearly point
>> this out:
>
> Great, finally we are on the same page. Thanks Haixu.
>
>>     "For full-duplex read and write transfer, both \field{tx_buf} and
>> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
>> as \field{rx_buf}."
>
> Suggest rewriting as:
>
> In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
> the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
> buffers MUST be same.

Is that in a non-normative section? (Sorry, I've lost track here...) If
so, I would say:

"The length of both buffers has to be the same."

>
>>     And in drivernormative section, I will add a requirement:
>>     "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
>> transfer size is equal to the read transfer size"
>
> Maybe:
>
> drivernormative:
>
> In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
> length of both \field{tx_buf} and \field{rx_buf} are same.

s/are same/is the same/

>
> devicenormative:
>
> In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
> of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the

s/are same/is the same/

> device MUST fail the transfer and notify the driver.

"notify the driver" == "set an appropriate error value"?

Can this be covered by one of the existing error values?


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Cornelia Huck]

On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 29-11-23, 18:31, Haixu Cui wrote:
>> Hi Viresh,
>> 
>>     Thank you for your helpful comments. In next patch, I will clearly point
>> this out:
>
> Great, finally we are on the same page. Thanks Haixu.
>
>>     "For full-duplex read and write transfer, both \field{tx_buf} and
>> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
>> as \field{rx_buf}."
>
> Suggest rewriting as:
>
> In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
> the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
> buffers MUST be same.

Is that in a non-normative section? (Sorry, I've lost track here...) If
so, I would say:

"The length of both buffers has to be the same."

>
>>     And in drivernormative section, I will add a requirement:
>>     "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
>> transfer size is equal to the read transfer size"
>
> Maybe:
>
> drivernormative:
>
> In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
> length of both \field{tx_buf} and \field{rx_buf} are same.

s/are same/is the same/

>
> devicenormative:
>
> In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
> of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the

s/are same/is the same/

> device MUST fail the transfer and notify the driver.

"notify the driver" == "set an appropriate error value"?

Can this be covered by one of the existing error values?


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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Viresh Kumar]

On 29-11-23, 13:42, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > On 29-11-23, 18:31, Haixu Cui wrote:
> >> Hi Viresh,
> >> 
> >>     Thank you for your helpful comments. In next patch, I will clearly point
> >> this out:
> >
> > Great, finally we are on the same page. Thanks Haixu.
> >
> >>     "For full-duplex read and write transfer, both \field{tx_buf} and
> >> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
> >> as \field{rx_buf}."
> >
> > Suggest rewriting as:
> >
> > In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
> > the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
> > buffers MUST be same.
> 
> Is that in a non-normative section? (Sorry, I've lost track here...)

I think yes, though I am not sure where Haixu will put it eventually :)

> If
> so, I would say:
> 
> "The length of both buffers has to be the same."

That's better. Thanks.

> > device MUST fail the transfer and notify the driver.
> 
> "notify the driver" == "set an appropriate error value"?
> 
> Can this be covered by one of the existing error values?

I think yes, this can be handled by the Param error.

-- 
viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Viresh Kumar]

On 29-11-23, 13:42, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > On 29-11-23, 18:31, Haixu Cui wrote:
> >> Hi Viresh,
> >> 
> >>     Thank you for your helpful comments. In next patch, I will clearly point
> >> this out:
> >
> > Great, finally we are on the same page. Thanks Haixu.
> >
> >>     "For full-duplex read and write transfer, both \field{tx_buf} and
> >> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
> >> as \field{rx_buf}."
> >
> > Suggest rewriting as:
> >
> > In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
> > the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
> > buffers MUST be same.
> 
> Is that in a non-normative section? (Sorry, I've lost track here...)

I think yes, though I am not sure where Haixu will put it eventually :)

> If
> so, I would say:
> 
> "The length of both buffers has to be the same."

That's better. Thanks.

> > device MUST fail the transfer and notify the driver.
> 
> "notify the driver" == "set an appropriate error value"?
> 
> Can this be covered by one of the existing error values?

I think yes, this can be handled by the Param error.

-- 
viresh

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

Hi Huck,

On 11/29/2023 8:42 PM, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
>> On 29-11-23, 18:31, Haixu Cui wrote:
>>> Hi Viresh,
>>>
>>>      Thank you for your helpful comments. In next patch, I will clearly point
>>> this out:
>>
>> Great, finally we are on the same page. Thanks Haixu.
>>
>>>      "For full-duplex read and write transfer, both \field{tx_buf} and
>>> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
>>> as \field{rx_buf}."
>>
>> Suggest rewriting as:
>>
>> In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
>> the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
>> buffers MUST be same.
> 
> Is that in a non-normative section? (Sorry, I've lost track here...) If
> so, I would say:
> 
> "The length of both buffers has to be the same."

This is non-normative section.
> 
>>
>>>      And in drivernormative section, I will add a requirement:
>>>      "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
>>> transfer size is equal to the read transfer size"
>>
>> Maybe:
>>
>> drivernormative:
>>
>> In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
>> length of both \field{tx_buf} and \field{rx_buf} are same.
> 
> s/are same/is the same/
> 
>>
>> devicenormative:
>>
>> In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
>> of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the
> 
> s/are same/is the same/
> 
>> device MUST fail the transfer and notify the driver.
> 
> "notify the driver" == "set an appropriate error value"?
> 
> Can this be covered by one of the existing error values?

I'd like to use VIRTIO_SPI_PARAM_ERR to indicates the buffer size 
inequality issue here because this is in the scope of parameter checking.

Thanks & BR

Haixu Cui

> 

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

Hi Huck,

On 11/29/2023 8:42 PM, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
>> On 29-11-23, 18:31, Haixu Cui wrote:
>>> Hi Viresh,
>>>
>>>      Thank you for your helpful comments. In next patch, I will clearly point
>>> this out:
>>
>> Great, finally we are on the same page. Thanks Haixu.
>>
>>>      "For full-duplex read and write transfer, both \field{tx_buf} and
>>> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
>>> as \field{rx_buf}."
>>
>> Suggest rewriting as:
>>
>> In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
>> the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
>> buffers MUST be same.
> 
> Is that in a non-normative section? (Sorry, I've lost track here...) If
> so, I would say:
> 
> "The length of both buffers has to be the same."

This is non-normative section.
> 
>>
>>>      And in drivernormative section, I will add a requirement:
>>>      "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
>>> transfer size is equal to the read transfer size"
>>
>> Maybe:
>>
>> drivernormative:
>>
>> In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
>> length of both \field{tx_buf} and \field{rx_buf} are same.
> 
> s/are same/is the same/
> 
>>
>> devicenormative:
>>
>> In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
>> of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the
> 
> s/are same/is the same/
> 
>> device MUST fail the transfer and notify the driver.
> 
> "notify the driver" == "set an appropriate error value"?
> 
> Can this be covered by one of the existing error values?

I'd like to use VIRTIO_SPI_PARAM_ERR to indicates the buffer size 
inequality issue here because this is in the scope of parameter checking.

Thanks & BR

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

Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

[Haixu Cui]

On 11/30/2023 12:13 PM, Viresh Kumar wrote:
> On 29-11-23, 13:42, Cornelia Huck wrote:
>> On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>>> On 29-11-23, 18:31, Haixu Cui wrote:
>>>> Hi Viresh,
>>>>
>>>>      Thank you for your helpful comments. In next patch, I will clearly point
>>>> this out:
>>>
>>> Great, finally we are on the same page. Thanks Haixu.
>>>
>>>>      "For full-duplex read and write transfer, both \field{tx_buf} and
>>>> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
>>>> as \field{rx_buf}."
>>>
>>> Suggest rewriting as:
>>>
>>> In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
>>> the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
>>> buffers MUST be same.
>>
>> Is that in a non-normative section? (Sorry, I've lost track here...)

Hi Viresh，
> 
> I think yes, though I am not sure where Haixu will put it eventually :)

I would like to put it in normative section, both for driver and device.

> 
>> If
>> so, I would say:
>>
>> "The length of both buffers has to be the same."
> 
> That's better. Thanks.
> 
>>> device MUST fail the transfer and notify the driver.
>>
>> "notify the driver" == "set an appropriate error value"?
>>
>> Can this be covered by one of the existing error values?
> 
> I think yes, this can be handled by the Param error.

Yes, this seems still in scope of parameter checking.

> 

Thanks & Best Regards

Haixu Cui

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Haixu Cui]

On 11/30/2023 12:13 PM, Viresh Kumar wrote:
> On 29-11-23, 13:42, Cornelia Huck wrote:
>> On Wed, Nov 29 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>>> On 29-11-23, 18:31, Haixu Cui wrote:
>>>> Hi Viresh,
>>>>
>>>>      Thank you for your helpful comments. In next patch, I will clearly point
>>>> this out:
>>>
>>> Great, finally we are on the same page. Thanks Haixu.
>>>
>>>>      "For full-duplex read and write transfer, both \field{tx_buf} and
>>>> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
>>>> as \field{rx_buf}."
>>>
>>> Suggest rewriting as:
>>>
>>> In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
>>> the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
>>> buffers MUST be same.
>>
>> Is that in a non-normative section? (Sorry, I've lost track here...)

Hi Viresh，
> 
> I think yes, though I am not sure where Haixu will put it eventually :)

I would like to put it in normative section, both for driver and device.

> 
>> If
>> so, I would say:
>>
>> "The length of both buffers has to be the same."
> 
> That's better. Thanks.
> 
>>> device MUST fail the transfer and notify the driver.
>>
>> "notify the driver" == "set an appropriate error value"?
>>
>> Can this be covered by one of the existing error values?
> 
> I think yes, this can be handled by the Param error.

Yes, this seems still in scope of parameter checking.

> 

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

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

[Jonathon Reinhart]

On Wed, Nov 29, 2023 at 5:00 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 29-11-23, 16:54, Haixu Cui wrote:
> > On 11/29/2023 4:33 PM, Cornelia Huck wrote:
> > > On Wed, Nov 29 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> > > > On 11/29/2023 3:30 PM, Viresh Kumar wrote:
> > > > > On 28-11-23, 20:58, Haixu Cui wrote:
> > > > > > On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> > > > > Hmm, I worked with a SPI controller over a decade ago, and I must be forgetting
> > > > > something here I guess. But from whatever little I remember, with full-duplex
> > > > > transfer, data flows on both MOSI and MISO lines as soon as clock signal is
> > > > > applied.  And so amount of data sent is always be equal to amount of data
> > > > > received by both sides.
> > > > >
> > > > > Also if I see Linux's implementation of the `struct spi_transfer` [1], I see
> > > > > `tx_buf`, `rx_buf` and a single `len` field, which applies to both the buffers.
> > > > > Which I guess is indicating that both buffers are supposed to be of same length.
> > > > >
> > > > > What am I missing ?
> > > >
> > > > Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
> > > > have the same size, Linux has such restriction. Just as you mention,
> > > > kernel level spi_transfer has single "len", the same for
> > > > spi_ioc_transfer passed from the userland.
> > > >
> > > > But I am not sure if this is in the scope of the spec. Because this is
> > > > ensured by Linux, but Virtio SPI driver won't also can't verify this.
> > > > This is a prerequisite for virtio spi processing requests.
>
> I don't think this is a Linux only thing. This is how the SPI protocol is
> designed to begin with. A clock is applied, data from FIFOs of both master and
> slaves gets exchanged over the MOSI and MISO lines... And the length is _always_
> the same. We are talking about the full-duplex mode of the hardware here and
> this is how it works AFAICT.
>

IMO the length doesn't _need_ to be the same. Sure, the clock always
runs, and the shift registers of the master and slave are always
clocking in data from their respective input lines. But that doesn't
mean that (all of) the received data actually *means* anything -- that
is determined by the application protocol.

For example, the initiator (master) might want to send 100 bytes but
only cares about the first 8 bytes received. Why should it need to
provide a 100 byte buffer for that received data? Certainly the stack
should be capable of discarding the other 92 bytes?

For a given SPI transfer, the clock needs to run for
(max(num_tx_bytes, num_rx_bytes) * word_size) cycles.

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