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

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

[Haixu Cui]

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,
either a physical SPI controller, or an emulated one.

Patch summary:
patch 1 rename virtual SPI device name
patch 2 add the specification for virtio-spi

Haixu Cui (2):
  content: Rename SPI master to SPI controller
  virtio-spi: add the device specification

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

-- 
2.17.1


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

[virtio-dev] [PATCH V6 1/2] content: Rename SPI master to SPI controller

[Haixu Cui]

SPI master is an outdated term and should use SPI controller.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
 content.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index 0a62dce..1c608eb 100644
--- a/content.tex
+++ b/content.tex
@@ -737,7 +737,7 @@ \chapter{Device Types}\label{sec:Device Types}
 \hline
 44         &   ISM device \\
 \hline
-45         &   SPI master \\
+45         &   SPI controller \\
 \hline
 \end{tabular}
 
-- 
2.17.1


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

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

[Haixu Cui]

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,
+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.
+
+\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.
+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.
+
+\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;
+        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.
+
+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.
+
+\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{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.
+
+\field{reserved} is currently unused and might be used for further extensions in the future.
+
+\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.
+
+\field{freq} indicates the SPI transfer speed in Hz.
+
+\field{word_delay_ns} indicates delay to be inserted between consecutive words of a transfer,
+in ns unit.
+
+\field{cs_setup_ns} indicates delay to be introduced after chipselect is asserted, in ns unit.
+
+\field{cs_delay_hold_ns} indicates delay to be introduced before chipselect is deasserted,
+in ns unit.
+
+\field{cs_change_delay_inactive_ns} indicates delay to be introduced after chipselect is
+deasserted and before next asserted, in ns unit.
+
+\field{tx_buf} is the buffer for data sent to the device.
+
+\field{rx_buf} is the buffer for data received from the device.
+
+\field{result} is the transfer result, it may be one of 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, which means the parameters in 
+\field{struct 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 device.
+Especially, for full-duplex transfer, VIRTIO_SPI_PARAM_ERR also used to indicate the
+scenario that the buffer size of \field{tx_buf} is not equal to that of \field{rx_buf}.
+
+VIRTIO_SPI_TRANS_ERR indicates a transfer error, means that the parameters are all
+valid but trasnfer process failed.
+
+\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Controller Device / Device Operation: Operation Status}
+
+Fields in \field{struct virtio_spi_transfer_head} are written by Virtio SPI driver, while
+\field{result} in \field{struct virtio_spi_transfer_result} is written by Virtio SPI device.
+
+virtio-spi supports three transfer types:
+\begin{itemize}
+\item half-duplex read;
+\item half-duplex write;
+\item full-duplex transfer. 
+\end{itemize}
+
+For half-duplex read and full-duplex transfer, \field{rx_buf} is filled by Virtio SPI device
+and consumed by Virtio SPI driver. For half-duplex write and full-duplex transfer, \field{tx_buf}
+is filled by Virtio SPI driver and consumed by Virtio SPI device.
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
+
+For half-duplex read, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
+\field{rx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For half-duplex write, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
+\field{tx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For full-duplex transfer, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head}, 
+\field{tx_buf}, \field{rx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For full-duplex transfer, Virtio SPI driver MUST guarantee that the buffer size of \field{tx_buf}
+and \field{rx_buf} is the same.
+
+Virtio SPI driver MUST not use \field{rx_buf} if the \field{result} returned from Virtio SPI device is
+not VIRTIO_SPI_TRANS_OK.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
+
+Virtio SPI device MUST set all the fields of \field{struct virtio_spi_config} before they are
+read by Virtio SPI driver.
+
+Virtio SPI device MUST NOT change the data in \field{tx_buf}.
+
+Virtio SPI device MUST verify the parameters in \field{struct virtio_spi_transfer_head} after receiving
+the request, and MUST set \field{struct virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all
+parameters are valid or some device unsupported features are set.
+
+For full-duplex transfer, Virtio SPI device MUST verify if the buffer size of \field{tx_buf} is equal to
+that of \field{rx_buf}. If not, Virtio SPI device MUST set \field{struct virtio_spi_transfer_result}
+as VIRTIO_SPI_PARAM_ERR.
diff --git a/device-types/spi/device-conformance.tex b/device-types/spi/device-conformance.tex
new file mode 100644
index 0000000..4509156
--- /dev/null
+++ b/device-types/spi/device-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Controller Device Conformance}\label{sec:Conformance / Device Conformance / SPI Controller Device Conformance}
+
+An SPI Controller device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / SPI Controller 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..bbd1967
--- /dev/null
+++ b/device-types/spi/driver-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Controller Driver Conformance}\label{sec:Conformance / Driver Conformance / SPI Controller Driver Conformance}
+
+An SPI Controller driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / SPI Controller Device / Device Operation}
+\end{itemize}
-- 
2.17.1


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

Re: [virtio-dev] [PATCH V6 1/2] content: Rename SPI master to SPI controller

[Viresh Kumar]

On 30-11-23, 17:22, Haixu Cui wrote:
> SPI master is an outdated term and should use SPI controller.
> 
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>  content.tex | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 0a62dce..1c608eb 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -737,7 +737,7 @@ \chapter{Device Types}\label{sec:Device Types}
>  \hline
>  44         &   ISM device \\
>  \hline
> -45         &   SPI master \\
> +45         &   SPI controller \\
>  \hline
>  \end{tabular}

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

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

[Viresh Kumar]

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.

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

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

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

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

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

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

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

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

Looks much better now. 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-dev] [PATCH V6 2/2] virtio-spi: add the device specification

[Cornelia Huck]

On Thu, Nov 30 2023, Haixu Cui <quic_haixcui@quicinc.com> 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,
> +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.

I think it would still make sense to keep the host/guest example you
included in the last version; while we have to talk about device/driver
in the specification, giving host/guest as an example is completely
fine.

> +
> +\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/for Virtio SPI driver/for the driver/

(matches what is said for other device types)

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

I wonder whether another term is now more commonly used... the Wikipedia
article uses main/sub, is there a term that is usually used together
with "controller"?

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

s/no limitation/there is no limitation/

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

s/clock idles/the clock idles/

> +low voltage, otherwise idles at the logical high voltage. CPHA determines how data is outputted

s/idles/it idles/

> +and sampled.
> +
> +Note: LSB first indicates that data is transferred least significant bit first,and MSB first

missing space after the comma

> +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{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.
> +
> +\field{reserved} is currently unused and might be used for further extensions in the future.
> +
> +\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.
> +
> +\field{freq} indicates the SPI transfer speed in Hz.
> +
> +\field{word_delay_ns} indicates delay to be inserted between consecutive words of a transfer,
> +in ns unit.
> +
> +\field{cs_setup_ns} indicates delay to be introduced after chipselect is asserted, in ns unit.
> +
> +\field{cs_delay_hold_ns} indicates delay to be introduced before chipselect is deasserted,
> +in ns unit.
> +
> +\field{cs_change_delay_inactive_ns} indicates delay to be introduced after chipselect is
> +deasserted and before next asserted, in ns unit.
> +
> +\field{tx_buf} is the buffer for data sent to the device.
> +
> +\field{rx_buf} is the buffer for data received from the device.
> +
> +\field{result} is the transfer result, it may be one of 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, which means the parameters in 
> +\field{struct 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 device.
> +Especially, for full-duplex transfer, VIRTIO_SPI_PARAM_ERR also used to indicate the

s/Especially/In particular/

> +scenario that the buffer size of \field{tx_buf} is not equal to that of \field{rx_buf}.

Maybe

"VIRTIO_SPI_PARAM_ERR can also indicate that \field{tx_buf} and
\field{rx_buf} are not of the same length."

?

> +
> +VIRTIO_SPI_TRANS_ERR indicates a transfer error, means that the parameters are all

s/means/which means/

> +valid but trasnfer process failed.

s/trasnfer/the transfer/

> +
> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Controller Device / Device Operation: Operation Status}
> +
> +Fields in \field{struct virtio_spi_transfer_head} are written by Virtio SPI driver, while
> +\field{result} in \field{struct virtio_spi_transfer_result} is written by Virtio SPI device.
> +
> +virtio-spi supports three transfer types:
> +\begin{itemize}
> +\item half-duplex read;
> +\item half-duplex write;
> +\item full-duplex transfer. 
> +\end{itemize}
> +
> +For half-duplex read and full-duplex transfer, \field{rx_buf} is filled by Virtio SPI device
> +and consumed by Virtio SPI driver. For half-duplex write and full-duplex transfer, \field{tx_buf}
> +is filled by Virtio SPI driver and consumed by Virtio SPI device.
> +
> +\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
> +
> +For half-duplex read, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
> +\field{rx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.

Maybe "in that order" instead of "in order" (also below)?

> +
> +For half-duplex write, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
> +\field{tx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
> +
> +For full-duplex transfer, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head}, 
> +\field{tx_buf}, \field{rx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
> +
> +For full-duplex transfer, Virtio SPI driver MUST guarantee that the buffer size of \field{tx_buf}
> +and \field{rx_buf} is the same.
> +
> +Virtio SPI driver MUST not use \field{rx_buf} if the \field{result} returned from Virtio SPI device is
> +not VIRTIO_SPI_TRANS_OK.
> +
> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
> +
> +Virtio SPI device MUST set all the fields of \field{struct virtio_spi_config} before they are
> +read by Virtio SPI driver.
> +
> +Virtio SPI device MUST NOT change the data in \field{tx_buf}.
> +
> +Virtio SPI device MUST verify the parameters in \field{struct virtio_spi_transfer_head} after receiving
> +the request, and MUST set \field{struct virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all
> +parameters are valid or some device unsupported features are set.
> +
> +For full-duplex transfer, Virtio SPI device MUST verify if the buffer size of \field{tx_buf} is equal to

s/if/that/

> +that of \field{rx_buf}. If not, Virtio SPI device MUST set \field{struct virtio_spi_transfer_result}
> +as VIRTIO_SPI_PARAM_ERR.


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

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

[Haixu Cui]

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 virtio 
architecture, device here can also be considered as the proxy of the 
host to manage the real SPI controller.

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

"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 the 
Virtio 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 by 
controller, 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 the 
different n-bit transfer modes supported by the device, for writing and 
reading respectively. SINGLE is always supported. A set bit here 
indicates 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 for 
writing 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


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

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

[Haixu Cui]

Hi Huck,

On 11/30/2023 7:38 PM, Cornelia Huck wrote:
> On Thu, Nov 30 2023, Haixu Cui <quic_haixcui@quicinc.com> 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,
>> +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.
> 
> I think it would still make sense to keep the host/guest example you
> included in the last version; while we have to talk about device/driver
> in the specification, giving host/guest as an example is completely
> fine.

Okay, I will add it back to illustrate the host&guest scenario.
> 
>> +
>> +\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/for Virtio SPI driver/for the driver/

Okay.

> 
> (matches what is said for other device types)
> 
>> +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.
> 
> I wonder whether another term is now more commonly used... the Wikipedia
> article uses main/sub, is there a term that is usually used together
> with "controller"?

In Wikipedia,

"Historically, this arrangement has been called master/slave. But to 
avoid referencing slavery, alternative names discussed in § Alternative 
namings have been used."

I am not quite sure if main/sub is widely used, but in latest Linux, 
still use master/slave but master is gradually replaced by "controller".

So I think "controller" here does make sense. What is your suggestion?
> 

>> +
>> +\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;
>> +        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.
>> +
>> +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.
>> +
>> +\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.
> 
> s/no limitation/there is no limitation/

Okay.
> 
>> +
>> +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
> 
> s/clock idles/the clock idles/
> 
>> +low voltage, otherwise idles at the logical high voltage. CPHA determines how data is outputted
> 
> s/idles/it idles/
> 
Okay.

>> +and sampled.
>> +
>> +Note: LSB first indicates that data is transferred least significant bit first,and MSB first
> 
> missing space after the comma
Okay.
> 
>> +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{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.
>> +
>> +\field{reserved} is currently unused and might be used for further extensions in the future.
>> +
>> +\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.
>> +
>> +\field{freq} indicates the SPI transfer speed in Hz.
>> +
>> +\field{word_delay_ns} indicates delay to be inserted between consecutive words of a transfer,
>> +in ns unit.
>> +
>> +\field{cs_setup_ns} indicates delay to be introduced after chipselect is asserted, in ns unit.
>> +
>> +\field{cs_delay_hold_ns} indicates delay to be introduced before chipselect is deasserted,
>> +in ns unit.
>> +
>> +\field{cs_change_delay_inactive_ns} indicates delay to be introduced after chipselect is
>> +deasserted and before next asserted, in ns unit.
>> +
>> +\field{tx_buf} is the buffer for data sent to the device.
>> +
>> +\field{rx_buf} is the buffer for data received from the device.
>> +
>> +\field{result} is the transfer result, it may be one of 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, which means the parameters in
>> +\field{struct 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 device.
>> +Especially, for full-duplex transfer, VIRTIO_SPI_PARAM_ERR also used to indicate the
> 
> s/Especially/In particular/
> 
>> +scenario that the buffer size of \field{tx_buf} is not equal to that of \field{rx_buf}.
> 
> Maybe
> 
> "VIRTIO_SPI_PARAM_ERR can also indicate that \field{tx_buf} and
> \field{rx_buf} are not of the same length."
> 
> ?
> 
Yes, looks much better, I will update the statement.

>> +
>> +VIRTIO_SPI_TRANS_ERR indicates a transfer error, means that the parameters are all
> 
> s/means/which means/
> 
>> +valid but trasnfer process failed.
> 
> s/trasnfer/the transfer/
> 
>> +
>> +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Controller Device / Device Operation: Operation Status}
>> +
>> +Fields in \field{struct virtio_spi_transfer_head} are written by Virtio SPI driver, while
>> +\field{result} in \field{struct virtio_spi_transfer_result} is written by Virtio SPI device.
>> +
>> +virtio-spi supports three transfer types:
>> +\begin{itemize}
>> +\item half-duplex read;
>> +\item half-duplex write;
>> +\item full-duplex transfer.
>> +\end{itemize}
>> +
>> +For half-duplex read and full-duplex transfer, \field{rx_buf} is filled by Virtio SPI device
>> +and consumed by Virtio SPI driver. For half-duplex write and full-duplex transfer, \field{tx_buf}
>> +is filled by Virtio SPI driver and consumed by Virtio SPI device.
>> +
>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
>> +
>> +For half-duplex read, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
>> +\field{rx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
> 
> Maybe "in that order" instead of "in order" (also below)?

Sure, will use "in that order".
> 
>> +
>> +For half-duplex write, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
>> +\field{tx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
>> +
>> +For full-duplex transfer, Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
>> +\field{tx_buf}, \field{rx_buf} and \field{struct virtio_spi_transfer_result} to SPI Virtio Device in order.
>> +
>> +For full-duplex transfer, Virtio SPI driver MUST guarantee that the buffer size of \field{tx_buf}
>> +and \field{rx_buf} is the same.
>> +
>> +Virtio SPI driver MUST not use \field{rx_buf} if the \field{result} returned from Virtio SPI device is
>> +not VIRTIO_SPI_TRANS_OK.
>> +
>> +\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
>> +
>> +Virtio SPI device MUST set all the fields of \field{struct virtio_spi_config} before they are
>> +read by Virtio SPI driver.
>> +
>> +Virtio SPI device MUST NOT change the data in \field{tx_buf}.
>> +
>> +Virtio SPI device MUST verify the parameters in \field{struct virtio_spi_transfer_head} after receiving
>> +the request, and MUST set \field{struct virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not all
>> +parameters are valid or some device unsupported features are set.
>> +
>> +For full-duplex transfer, Virtio SPI device MUST verify if the buffer size of \field{tx_buf} is equal to
> 
> s/if/that/
> 
Okay.
>> +that of \field{rx_buf}. If not, Virtio SPI device MUST set \field{struct virtio_spi_transfer_result}
>> +as VIRTIO_SPI_PARAM_ERR.
> 
Thank you very much.

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

[Viresh Kumar]

On 01-12-23, 12:05, Haixu Cui wrote:
> Looks good. Will update as follows:
> 
> \field{tx_nbits_supported} and \field{rx_nbits_supported} indicate the
> different n-bit transfer modes supported by the device, for writing and
> reading respectively. SINGLE is always supported. A set bit here indicates
> 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:

Maybe:

Note: The commonly used SPI transfer modes are:

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

[Haixu Cui]

On 12/1/2023 12:32 PM, Viresh Kumar wrote:
> On 01-12-23, 12:05, Haixu Cui wrote:
>> Looks good. Will update as follows:
>>
>> \field{tx_nbits_supported} and \field{rx_nbits_supported} indicate the
>> different n-bit transfer modes supported by the device, for writing and
>> reading respectively. SINGLE is always supported. A set bit here indicates
>> 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:
> 
> Maybe:
> 
> Note: The commonly used SPI transfer modes are:
> 
Hi Viresh,

     SPI transfer modes here may cause misunderstanding, in Wikipedia, 
SPI mode is the combinations of clock polarity and clock phases:

 
https://en.wikipedia.org/w/index.php?title=Serial_Peripheral_Interface&action=edit&section=4

     What about updating as: "Note: The commonly used SPI n-bit transfer 
options are:"

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

[Viresh Kumar]

On 01-12-23, 13:35, Haixu Cui wrote:
> Hi Viresh,
> 
>     SPI transfer modes here may cause misunderstanding, in Wikipedia, SPI
> mode is the combinations of clock polarity and clock phases:
> 
> 
> https://en.wikipedia.org/w/index.php?title=Serial_Peripheral_Interface&action=edit&section=4

The word 'mode' is indeed confusing. It is used for combinations of PHA/POL,
half/full duplex, n-bit transfers, etc..

>     What about updating as: "Note: The commonly used SPI n-bit transfer
> options are:"

That's fine too.

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

[Haixu Cui]

Hi Viresh,

On 12/1/2023 1:44 PM, Viresh Kumar wrote:
> On 01-12-23, 13:35, Haixu Cui wrote:
>> Hi Viresh,
>>
>>      SPI transfer modes here may cause misunderstanding, in Wikipedia, SPI
>> mode is the combinations of clock polarity and clock phases:
>>
>>
>> https://en.wikipedia.org/w/index.php?title=Serial_Peripheral_Interface&action=edit&section=4
> 
> The word 'mode' is indeed confusing. It is used for combinations of PHA/POL,
> half/full duplex, n-bit transfers, etc..

Yes, it is. In this spec, for half/full duplex, I use "transfer type", 
and for line number, just use "n-bit transfer", to avoid ambiguity.
> 
>>      What about updating as: "Note: The commonly used SPI n-bit transfer
>> options are:"
> 
> That's fine too.
> 
OK, thank you.

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

[Cornelia Huck]

On Fri, Dec 01 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

>>> +Note: chipselect is an electrical signal controlled by the SPI controller, used to select the
>>> +SPI slaves connected to the controller.
>> 
>> I wonder whether another term is now more commonly used... the Wikipedia
>> article uses main/sub, is there a term that is usually used together
>> with "controller"?
>
> In Wikipedia,
>
> "Historically, this arrangement has been called master/slave. But to 
> avoid referencing slavery, alternative names discussed in § Alternative 
> namings have been used."
>
> I am not quite sure if main/sub is widely used, but in latest Linux, 
> still use master/slave but master is gradually replaced by "controller".
>
> So I think "controller" here does make sense. What is your suggestion?

"controller" is fine, I'm just wondering if "slave" has started to be
replaced with something else as well... if it hasn't, we probably need
to stick with it to avoid confusing readers.


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

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

[Haixu Cui]

Hi Huck,

On 12/1/2023 7:01 PM, Cornelia Huck wrote:
> On Fri, Dec 01 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>>>> +Note: chipselect is an electrical signal controlled by the SPI controller, used to select the
>>>> +SPI slaves connected to the controller.
>>>
>>> I wonder whether another term is now more commonly used... the Wikipedia
>>> article uses main/sub, is there a term that is usually used together
>>> with "controller"?
>>
>> In Wikipedia,
>>
>> "Historically, this arrangement has been called master/slave. But to
>> avoid referencing slavery, alternative names discussed in § Alternative
>> namings have been used."
>>
>> I am not quite sure if main/sub is widely used, but in latest Linux,
>> still use master/slave but master is gradually replaced by "controller".
>>
>> So I think "controller" here does make sense. What is your suggestion?
> 
> "controller" is fine, I'm just wondering if "slave" has started to be
> replaced with something else as well... if it hasn't, we probably need
> to stick with it to avoid confusing readers.
> 

slave is still being used, but it's better to be used along with "master".

Here I prefer using "SPI peripherals" instead of "SPI slaves". Looking 
forward to your advice. Thank you so much.

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

[Cornelia Huck]

On Mon, Dec 04 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Huck,
>
> On 12/1/2023 7:01 PM, Cornelia Huck wrote:
>> On Fri, Dec 01 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>> 
>>>>> +Note: chipselect is an electrical signal controlled by the SPI controller, used to select the
>>>>> +SPI slaves connected to the controller.
>>>>
>>>> I wonder whether another term is now more commonly used... the Wikipedia
>>>> article uses main/sub, is there a term that is usually used together
>>>> with "controller"?
>>>
>>> In Wikipedia,
>>>
>>> "Historically, this arrangement has been called master/slave. But to
>>> avoid referencing slavery, alternative names discussed in § Alternative
>>> namings have been used."
>>>
>>> I am not quite sure if main/sub is widely used, but in latest Linux,
>>> still use master/slave but master is gradually replaced by "controller".
>>>
>>> So I think "controller" here does make sense. What is your suggestion?
>> 
>> "controller" is fine, I'm just wondering if "slave" has started to be
>> replaced with something else as well... if it hasn't, we probably need
>> to stick with it to avoid confusing readers.
>> 
>
> slave is still being used, but it's better to be used along with "master".
>
> Here I prefer using "SPI peripherals" instead of "SPI slaves". Looking 
> forward to your advice. Thank you so much.

"SPI peripherals" sounds good to me.


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