virtio-comment message

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

From: Haixu Cui <quic_haixcui@quicinc.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 1 Dec 2023 12:05:54 +0800

Hi Viresh,

On 11/30/2023 7:19 PM, Viresh Kumar wrote:

On 30-11-23, 17:22, Haixu Cui wrote:

The Virtio SPI (Serial Peripheral Interface) device is a virtual
SPI controller that allows the driver to operate and use the SPI
controller under the control of the device.

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
  device-types/spi/description.tex        | 288 ++++++++++++++++++++++++
  device-types/spi/device-conformance.tex |   7 +
  device-types/spi/driver-conformance.tex |   7 +
  3 files changed, 302 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..c4816a6
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,288 @@
+\section{SPI Controller Device}\label{sec:Device Types / SPI Controller Device}
+
+The Virtio SPI (Serial Peripheral Interface) device is a virtual SPI controller that
+allows the driver to operate and use the SPI controller under the control of the device,


s/control of the device/control of the host/

The host emulates and provides the guest with a device. 'device' is not really a
replacement for the word 'host`, but should be used to refer to the entity
emulated by the host.

Here I want to emphasize the driver&device pair in the virtioarchitecture, device here can also be considered as the proxy of thehost to manage the real SPI controller.

I will add back the following statement to illustrate the host&guestcondition:

"In a typical host and guest architecture with the Virtio SPI device,the Virtio SPI driver is the front-end running in the guest, and theVirtio SPI device is the back-end in the host."

+either a physical SPI controller, or an emulated one.
+
+The Virtio SPI device has a single virtqueue. SPI transfer requests are placed into
+the virtqueue, and serviced by the device.


s/the virtqueue, and /the virtqueue by the driver, and are /


Okay.

+
+\subsection{Device ID}\label{sec:Device Types / SPI Controller Device / Device ID}
+45
+
+\subsection{Virtqueues}\label{sec:Device Types / SPI Controller Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / SPI Controller Device / Feature bits}
+
+None
+
+\subsection{Device configuration layout}\label{sec:Device Types / SPI Controller Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for Virtio SPI driver.


s/always available/mandatory/


Okay.


and "the Virtio SPI driver", everywhere else too..


Will update the whole spec.

+The config space shows the features and settings supported by the device.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+	u8 cs_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}
+
+\field{cs_max_number} is the maximum number of chipselect the device supports.
+
+Note: chipselect is an electrical signal controlled by the SPI controller, used to select the
+SPI slaves connected to the controller.


It isn't mandatory for the CS to be controlled by the controller. It can also be
a GPIO pin too which is controlled by the host driver.

Suggest rewriting as:

Note: chipselect is an electrical signal, which is used to select the SPI slaves
connected to the controller.

Got it. If GPIO, it's not appropriate to say cs is controlled bycontroller, maybe the SPI driver just invokes the GPIO interface.

+\field{cs_change_supported} indicates if the device supports to toggle chipselect
+after each transfer in one message:
+        0: unsupported, means chipselect will keep active when executing the message transaction;


         0: unsupported, chipselect will be kept in active state throughout the transaction;


Okay.

+        1: supported.
+
+Note: Message here contains a sequence of SPI transfers.
+
+\field{tx_nbits_supported} indicates the supported number of bit for writing, SINGLE is always
+supported. In the bit definition below, a set bit means the transfer is supported, otherwise not:
+        bit 0: DUAL;
+        bit 1: QUAD;
+        bit 2: OCTAL;
+        other bits are reserved and must be set to 0 by the device.
+


Not sure if the below Note and the item list is required at all.

+Note: Transfer bit options are commonly used in SPI:
+\begin{itemize}
+\item SINGLE: 1-bit transfer
+\item DUAL: 2-bit transfer
+\item QUAD: 4-bit transfer
+\item OCTAL: 8-bit transfer
+\end{itemize}
+
+\field{rx_nbits_supported} indicates the supported number of bit for reading, SINGLE is always
+supported. In the bit definition below, a set bit means the transfer is supported, otherwise not:
+        bit 0: DUAL;
+        bit 1: QUAD;
+        bit 2: OCTAL;
+        other bits are reserved and must be set to 0 by the device.
+


There is still some duplication left here in tx and rx bits supported fields.
Maybe rewrite the whole thing as:


Looks good. Will update as follows:

\field{tx_nbits_supported} and \field{rx_nbits_supported} indicate thedifferent n-bit transfer modes supported by the device, for writing andreading respectively. SINGLE is always supported. A set bit hereindicates that the corresponding n-bit transfer is supported, otherwise not:

        bit 0: DUAL;
        bit 1: QUAD;
        bit 2: OCTAL;
        other bits are reserved and must be set as 0 by the device.

Note: Transfer bit options are commonly used in SPI:
\begin{itemize}
\item SINGLE: 1-bit transfer
\item DUAL: 2-bit transfer
\item QUAD: 4-bit transfer
\item OCTAL: 8-bit transfer
\end{itemize}

I'd like to reserve the itemize here, to clarify what is SINGLE/DUAL...


"
\field{tx_nbits_supported} and \field{rx_nbits_supported} indicate the different
n-bit transfer modes supported by the device. The 1-bit transfer mode is always
supported. A set bit here indicates that the corresponding mode is supported,
otherwise not:

         bit 0: DUAL;
         bit 1: QUAD;
         bit 2: OCTAL;
         other bits are reserved and must be set to 0 by the device.
"

+\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} in \field{struct virtio_spi_transfer_head}.
+
+\field{mode_func_supported} indicates whether 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, must 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, must support as least one CPOL setting.
+
+        bit 4: chipselect active high feature, 0 for unsupported and 1 for supported,
+            chipselect active low must always be supported.
+
+        bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB first must always be
+            supported.
+
+        bit 6: loopback mode feature, 0 for unsupported and 1 for supported, normal mode
+            must always be supported.
+
+Note: CPOL is clock polarity and CPHA is clock phase. If CPOL is 0, clock idles at the logical
+low voltage, otherwise idles at the logical high voltage. CPHA determines how data is outputted
+and sampled.
+
+Note: LSB first indicates that data is transferred least significant bit first,and MSB first
+indicates that data is transferred most significant bit first.
+
+\field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0 means no limitation
+for transfer speed.
+
+\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 words.
+
+\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.
+
+\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.
+
+\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 Controller 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 Controller Device / Device Operation}
+
+\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI Controller Device / Device Operation: Request Queue}
+
+The Virtio SPI driver enqueues requests to the virtqueue, and they are used by Virtio SPI device.
+Each request represents one SPI transfer 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}
+
+\field{chip_select_id} indicates the chipselect index the SPI transfer used.


\field{chip_select_id} indicates the chipselect index to use for the SPI transfer.


Okay.

+\field{bits_per_word} indicates the number of bits in each SPI transfer word.
+
+\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.
+
+\field{tx_nbits} indicates number of bits used for writing:
+        0,1: SINGLE;
+        2  : DUAL;
+        4  : QUAD;
+        8  : OCTAL;
+        other values are invalid.
+
+\field{rx_nbits} indicates number of bits used for reading:
+        0,1: SINGLE;
+        2  : DUAL;
+        4  : QUAD;
+        8  : OCTAL;
+        other values are invalid.


Duplication here can be removed too, like in the earlier example I gave.


I will combine rx_nbits and tx_nbits, like:

\field{tx_nbits} and \field{rx_nbits} indicate n-bit transfer mode forwriting and reading:

        0,1: SINGLE;
        2  : DUAL;
        4  : QUAD;
        8  : OCTAL;
        other values are invalid.

Looks much better now. Thanks. >

Great. Thank you so much for your help.

Best Regards
Haixu Cui

Follow-Ups:
- Re: [virtio-dev] [PATCH V6 2/2] virtio-spi: add the device specification
  - From: Viresh Kumar <viresh.kumar@linaro.org>