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

[virtio-dev][PATCH V9 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 host,
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                             |   3 +-
 device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
 device-types/spi/device-conformance.tex |   7 +
 device-types/spi/driver-conformance.tex |   7 +
 4 files changed, 297 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 V9 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>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 content.tex | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index 0a62dce..84bc68f 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}
 
@@ -767,6 +767,7 @@ \chapter{Device Types}\label{sec:Device Types}
 \input{device-types/scmi/description.tex}
 \input{device-types/gpio/description.tex}
 \input{device-types/pmem/description.tex}
+\input{device-types/spi/description.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
-- 
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 V9 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 host.

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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..b76258c
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,281 @@
+\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 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 by the driver, and are serviced by the device.
+
+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.
+
+\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 mandatory and read-only for the 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, which is used to select the SPI peripherals connected
+to the controller.
+
+\field{cs_change_supported} indicates if the device supports to toggle chipselect
+after each transfer in one message:
+        0: unsupported, chipselect will be kept in active state throughout the message transaction;
+        1: supported.
+
+Note: Message here contains a sequence of SPI transfers.
+
+\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: The commonly used SPI n-bit transfer options are:
+\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{bits_per_word_mask} is a mask indicating which values of bits_per_word are supported.
+If bit n of \field{bits_per_word_mask} is set, the bits_per_word with value (n+1) is supported.
+If all bits are not set, bits_per_word can be any value from 1 to 32.
+
+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: invalid, must support as least one CPHA setting.
+            0b01: supports CPHA=0 only;
+            0b10: supports CPHA=1 only;
+            0b11: supports CPHA=0 and CPHA=1;
+
+        bit 2-3: CPOL feature,
+            0b00: invalid, must support as least one CPOL setting.
+            0b01: supports CPOL=0 only;
+            0b10: supports CPOL=1 only;
+            0b11: supports CPOL=0 and CPOL=1;
+
+        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, the clock idles at the logical
+low voltage, otherwise it 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 the 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 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} 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.
+
+\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.
+In particular, for full-duplex transfer, 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, which means that the parameters are all
+valid but the 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 the Virtio SPI driver, while
+\field{result} in \field{struct virtio_spi_transfer_result} is written by the 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 the Virtio SPI device
+and consumed by the Virtio SPI driver. For half-duplex write and full-duplex transfer, \field{tx_buf}
+is filled by the Virtio SPI driver and consumed by the Virtio SPI device.
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
+
+For half-duplex read, the Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
+\field{rx_buf} and \field{struct virtio_spi_transfer_result} to the SPI Virtio Device in that order.
+
+For half-duplex write, the Virtio SPI driver MUST send \field{struct virtio_spi_transfer_head},
+\field{tx_buf} and \field{struct virtio_spi_transfer_result} to the SPI Virtio Device in that order.
+
+For full-duplex transfer, the 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 the SPI Virtio Device
+in that order.
+
+For full-duplex transfer, the Virtio SPI driver MUST guarantee that the buffer size of \field{tx_buf}
+and \field{rx_buf} is the same.
+
+The Virtio SPI driver MUST not use \field{rx_buf} if the \field{result} returned from the Virtio SPI device is
+not VIRTIO_SPI_TRANS_OK.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Controller Device / Device Operation}
+
+The Virtio SPI device MUST set all the fields of \field{struct virtio_spi_config} before they are
+read by the Virtio SPI driver.
+
+The Virtio SPI device MUST NOT change the data in \field{tx_buf}.
+
+The 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, the Virtio SPI device MUST verify that the buffer size of \field{tx_buf} is equal to
+that of \field{rx_buf}. If not, the 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 V9 2/2] virtio-spi: add the device specification

[Cornelia Huck]

On Tue, Dec 12 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

[BTW: A changelog would be useful, either in the patch or in the cover
letter.]

> 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 host.
>
> This patch adds the specification for virtio-spi.
>
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  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

Please move inclusion of this new file into content.tex here, instead of
including a not-yet-existing file in the previous patch.

(...)

> +\field{mode_func_supported} indicates whether the following features are supported or not:
> +        bit 0-1: CPHA feature,
> +            0b00: invalid, must support as least one CPHA setting.
> +            0b01: supports CPHA=0 only;
> +            0b10: supports CPHA=1 only;
> +            0b11: supports CPHA=0 and CPHA=1;
> +
> +        bit 2-3: CPOL feature,
> +            0b00: invalid, must support as least one CPOL setting.
> +            0b01: supports CPOL=0 only;
> +            0b10: supports CPOL=1 only;
> +            0b11: supports CPOL=0 and CPOL=1;
> +
> +        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, the clock idles at the logical
> +low voltage, otherwise it idles at the logical high voltage. CPHA determines how data is outputted
> +and sampled.

Shouldn't you also specify what CPHA==0 and CPHA==1 mean?


(...)

> +VIRTIO_SPI_TRANS_ERR indicates a transfer error, which means that the parameters are all
> +valid but the trasnfer process failed.

s/trasnfer/transfer/

LGTM otherwise. Does anybody else have comments?


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

[Haixu Cui]

Hi Cornelia,
     Much appreciated for your comments and please refer to my response.

On 12/18/2023 7:04 PM, Cornelia Huck wrote:
> On Tue, Dec 12 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
> [BTW: A changelog would be useful, either in the patch or in the cover
> letter.]

Sure, I summarize the major changes between these versions and will add 
in next patch:

changelog:
v8->v9:
- add explanation of bits_per_word_mask in config space
v7->v8:
- change device to host
v6->v7:
- fix the format problem, syntax problem
v5->v6:
- use driver/device instead guest/host
- add the definition of some terminologies
- use controller instead of master throughout the spec
- add buffer length validation for full-duplex transfer
v4->v5:
- use controller instead of master
- fix indentation issue
- extend the config space to expose the backend supported features
- add another result value to indicate parameter error
- add device and driver requirement about parameter checkig
v3->v4:
- fix the spell error
- bus_num is not SOC-specific, remove it
- add driver requirement to deal with the situation that the cs delay
parameters are not 0 but the backend doesn't support cs timing setting
v2->v3
- remove unnecessary statements and driver implimentation details
- add the parameters about cs timing delay and transfer delay
- use "le32" instead of "u32"
- swap the rx_buf and tx_buf in the request format
- add the parameters about transfer bit width
v1->v2:
- explain SPI when it is firstly used
- update the ambiguous expression of virtqueue
v0->v1:
- add definition of abbreviation SPI
- remove the ID
---

> 
>> 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 host.
>>
>> This patch adds the specification for virtio-spi.
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   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
> 
> Please move inclusion of this new file into content.tex here, instead of
> including a not-yet-existing file in the previous patch.
> 
> (...)


Sorry, I don't quite understand here. Maybe I should not add the 
following line in content.tex, is that so?

\input{device-types/spi/description.tex}

> 
>> +\field{mode_func_supported} indicates whether the following features are supported or not:
>> +        bit 0-1: CPHA feature,
>> +            0b00: invalid, must support as least one CPHA setting.
>> +            0b01: supports CPHA=0 only;
>> +            0b10: supports CPHA=1 only;
>> +            0b11: supports CPHA=0 and CPHA=1;
>> +
>> +        bit 2-3: CPOL feature,
>> +            0b00: invalid, must support as least one CPOL setting.
>> +            0b01: supports CPOL=0 only;
>> +            0b10: supports CPOL=1 only;
>> +            0b11: supports CPOL=0 and CPOL=1;
>> +
>> +        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, the clock idles at the logical
>> +low voltage, otherwise it idles at the logical high voltage. CPHA determines how data is outputted
>> +and sampled.
> 
> Shouldn't you also specify what CPHA==0 and CPHA==1 mean?
> 

Yes, I will add the following explanation:

If CPHA is 0, the first bit is outputted immediately when chipselect is 
active and subsequent bits are outputted on the clock edges when clock 
transitions from active level to idle level. Data is sampled on the 
clock edges when clock transitions from idle level to active level.

If CPHA is 1, the first bit is outputted on the fist clock edge after 
chipselect is active, subsequent bits are outputted on the clock edges 
when clock transitions from idle level to active level. Data is sampled 
on the clock edges when clock transitions from active level to idle level.

> 
> (...)
> 
>> +VIRTIO_SPI_TRANS_ERR indicates a transfer error, which means that the parameters are all
>> +valid but the trasnfer process failed.
> 
> s/trasnfer/transfer/

OK will update the spell mistake
> 
> LGTM otherwise. Does anybody else have comments?
> 

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

[Cornelia Huck]

On Thu, Dec 21 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Cornelia,
>      Much appreciated for your comments and please refer to my response.
>
> On 12/18/2023 7:04 PM, Cornelia Huck wrote:
>> On Tue, Dec 12 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>> 
>> [BTW: A changelog would be useful, either in the patch or in the cover
>> letter.]
>
> Sure, I summarize the major changes between these versions and will add 
> in next patch:
>
> changelog:
> v8->v9:
> - add explanation of bits_per_word_mask in config space
> v7->v8:
> - change device to host
> v6->v7:
> - fix the format problem, syntax problem
> v5->v6:
> - use driver/device instead guest/host
> - add the definition of some terminologies
> - use controller instead of master throughout the spec
> - add buffer length validation for full-duplex transfer
> v4->v5:
> - use controller instead of master
> - fix indentation issue
> - extend the config space to expose the backend supported features
> - add another result value to indicate parameter error
> - add device and driver requirement about parameter checkig
> v3->v4:
> - fix the spell error
> - bus_num is not SOC-specific, remove it
> - add driver requirement to deal with the situation that the cs delay
> parameters are not 0 but the backend doesn't support cs timing setting
> v2->v3
> - remove unnecessary statements and driver implimentation details
> - add the parameters about cs timing delay and transfer delay
> - use "le32" instead of "u32"
> - swap the rx_buf and tx_buf in the request format
> - add the parameters about transfer bit width
> v1->v2:
> - explain SPI when it is firstly used
> - update the ambiguous expression of virtqueue
> v0->v1:
> - add definition of abbreviation SPI
> - remove the ID

Thanks. Might be best to add this in the cover letter.

> ---
>
>> 
>>> 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 host.
>>>
>>> This patch adds the specification for virtio-spi.
>>>
>>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>   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
>> 
>> Please move inclusion of this new file into content.tex here, instead of
>> including a not-yet-existing file in the previous patch.
>> 
>> (...)
>
>
> Sorry, I don't quite understand here. Maybe I should not add the 
> following line in content.tex, is that so?
>
> \input{device-types/spi/description.tex}

No, please add this line; but do so in this patch instead of the
previous one that only changes the wording.

>
>> 
>>> +\field{mode_func_supported} indicates whether the following features are supported or not:
>>> +        bit 0-1: CPHA feature,
>>> +            0b00: invalid, must support as least one CPHA setting.
>>> +            0b01: supports CPHA=0 only;
>>> +            0b10: supports CPHA=1 only;
>>> +            0b11: supports CPHA=0 and CPHA=1;
>>> +
>>> +        bit 2-3: CPOL feature,
>>> +            0b00: invalid, must support as least one CPOL setting.
>>> +            0b01: supports CPOL=0 only;
>>> +            0b10: supports CPOL=1 only;
>>> +            0b11: supports CPOL=0 and CPOL=1;
>>> +
>>> +        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, the clock idles at the logical
>>> +low voltage, otherwise it idles at the logical high voltage. CPHA determines how data is outputted
>>> +and sampled.
>> 
>> Shouldn't you also specify what CPHA==0 and CPHA==1 mean?
>> 
>
> Yes, I will add the following explanation:
>
> If CPHA is 0, the first bit is outputted immediately when chipselect is 
> active and subsequent bits are outputted on the clock edges when clock 

s/when clock/when the clock/

> transitions from active level to idle level. Data is sampled on the 
> clock edges when clock transitions from idle level to active level.

same

>
> If CPHA is 1, the first bit is outputted on the fist clock edge after 
> chipselect is active, subsequent bits are outputted on the clock edges 
> when clock transitions from idle level to active level. Data is sampled 

same

> on the clock edges when clock transitions from active level to idle level.

same


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

[Haixu Cui]

Hi Cornelia,

On 12/21/2023 4:44 PM, Cornelia Huck wrote:
> On Thu, Dec 21 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> Hi Cornelia,
>>       Much appreciated for your comments and please refer to my response.
>>
>> On 12/18/2023 7:04 PM, Cornelia Huck wrote:
>>> On Tue, Dec 12 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>
>>> [BTW: A changelog would be useful, either in the patch or in the cover
>>> letter.]
>>
>> Sure, I summarize the major changes between these versions and will add
>> in next patch:
>>
>> changelog:
>> v8->v9:
>> - add explanation of bits_per_word_mask in config space
>> v7->v8:
>> - change device to host
>> v6->v7:
>> - fix the format problem, syntax problem
>> v5->v6:
>> - use driver/device instead guest/host
>> - add the definition of some terminologies
>> - use controller instead of master throughout the spec
>> - add buffer length validation for full-duplex transfer
>> v4->v5:
>> - use controller instead of master
>> - fix indentation issue
>> - extend the config space to expose the backend supported features
>> - add another result value to indicate parameter error
>> - add device and driver requirement about parameter checkig
>> v3->v4:
>> - fix the spell error
>> - bus_num is not SOC-specific, remove it
>> - add driver requirement to deal with the situation that the cs delay
>> parameters are not 0 but the backend doesn't support cs timing setting
>> v2->v3
>> - remove unnecessary statements and driver implimentation details
>> - add the parameters about cs timing delay and transfer delay
>> - use "le32" instead of "u32"
>> - swap the rx_buf and tx_buf in the request format
>> - add the parameters about transfer bit width
>> v1->v2:
>> - explain SPI when it is firstly used
>> - update the ambiguous expression of virtqueue
>> v0->v1:
>> - add definition of abbreviation SPI
>> - remove the ID
> 
> Thanks. Might be best to add this in the cover letter.
> 

In next patch I will add it in cover letter.

>> ---
>>
>>>
>>>> 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 host.
>>>>
>>>> This patch adds the specification for virtio-spi.
>>>>
>>>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>>>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> ---
>>>>    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
>>>
>>> Please move inclusion of this new file into content.tex here, instead of
>>> including a not-yet-existing file in the previous patch.
>>>
>>> (...)
>>
>>
>> Sorry, I don't quite understand here. Maybe I should not add the
>> following line in content.tex, is that so?
>>
>> \input{device-types/spi/description.tex}
> 
> No, please add this line; but do so in this patch instead of the
> previous one that only changes the wording.

I am not clear where to put this line? In the commit message or in the 
main text, e.g. at the start of the description.tex? As follows:

diff --git a/device-types/spi/description.tex 
b/device-types/spi/description.tex
new file mode 100644
index 0000000..b76258c
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,281 @@
+\input{device-types/spi/description.tex}
+\section{SPI Controller Device}\label{sec:Device Types / SPI Controller 
Device}
+
...

> 
>>
>>>
>>>> +\field{mode_func_supported} indicates whether the following features are supported or not:
>>>> +        bit 0-1: CPHA feature,
>>>> +            0b00: invalid, must support as least one CPHA setting.
>>>> +            0b01: supports CPHA=0 only;
>>>> +            0b10: supports CPHA=1 only;
>>>> +            0b11: supports CPHA=0 and CPHA=1;
>>>> +
>>>> +        bit 2-3: CPOL feature,
>>>> +            0b00: invalid, must support as least one CPOL setting.
>>>> +            0b01: supports CPOL=0 only;
>>>> +            0b10: supports CPOL=1 only;
>>>> +            0b11: supports CPOL=0 and CPOL=1;
>>>> +
>>>> +        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, the clock idles at the logical
>>>> +low voltage, otherwise it idles at the logical high voltage. CPHA determines how data is outputted
>>>> +and sampled.
>>>
>>> Shouldn't you also specify what CPHA==0 and CPHA==1 mean?
>>>
>>
>> Yes, I will add the following explanation:
>>
>> If CPHA is 0, the first bit is outputted immediately when chipselect is
>> active and subsequent bits are outputted on the clock edges when clock
> 
> s/when clock/when the clock/
> 
>> transitions from active level to idle level. Data is sampled on the
>> clock edges when clock transitions from idle level to active level.
> 
> same
> 
>>
>> If CPHA is 1, the first bit is outputted on the fist clock edge after
>> chipselect is active, subsequent bits are outputted on the clock edges
>> when clock transitions from idle level to active level. Data is sampled
> 
> same
> 
>> on the clock edges when clock transitions from active level to idle level.
> 
> same
> 
Ok, will update.

Thank you so much.

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

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

[Cornelia Huck]

On Thu, Dec 21 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:

> Hi Cornelia,
>
> On 12/21/2023 4:44 PM, Cornelia Huck wrote:
>> On Thu, Dec 21 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>> Please move inclusion of this new file into content.tex here, instead of
>>>> including a not-yet-existing file in the previous patch.
>>>>
>>>> (...)
>>>
>>>
>>> Sorry, I don't quite understand here. Maybe I should not add the
>>> following line in content.tex, is that so?
>>>
>>> \input{device-types/spi/description.tex}
>> 
>> No, please add this line; but do so in this patch instead of the
>> previous one that only changes the wording.
>
> I am not clear where to put this line? In the commit message or in the 
> main text, e.g. at the start of the description.tex? As follows:
>
> diff --git a/device-types/spi/description.tex 
> b/device-types/spi/description.tex
> new file mode 100644
> index 0000000..b76258c
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,281 @@
> +\input{device-types/spi/description.tex}
> +\section{SPI Controller Device}\label{sec:Device Types / SPI Controller 
> Device}
> +
> ...

Ah no, the inclusion is of course in the right place where you put it in
context.tex; the *patch hunk* should be moved from patch 1 to patch 2,
that's all. Hope that's more clear now :)


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

[Haixu Cui]

On 12/21/2023 5:54 PM, Cornelia Huck wrote:
> On Thu, Dec 21 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
> 
>> Hi Cornelia,
>>
>> On 12/21/2023 4:44 PM, Cornelia Huck wrote:
>>> On Thu, Dec 21 2023, Haixu Cui <quic_haixcui@quicinc.com> wrote:
>>>>> Please move inclusion of this new file into content.tex here, instead of
>>>>> including a not-yet-existing file in the previous patch.
>>>>>
>>>>> (...)
>>>>
>>>>
>>>> Sorry, I don't quite understand here. Maybe I should not add the
>>>> following line in content.tex, is that so?
>>>>
>>>> \input{device-types/spi/description.tex}
>>>
>>> No, please add this line; but do so in this patch instead of the
>>> previous one that only changes the wording.
>>
>> I am not clear where to put this line? In the commit message or in the
>> main text, e.g. at the start of the description.tex? As follows:
>>
>> diff --git a/device-types/spi/description.tex
>> b/device-types/spi/description.tex
>> new file mode 100644
>> index 0000000..b76258c
>> --- /dev/null
>> +++ b/device-types/spi/description.tex
>> @@ -0,0 +1,281 @@
>> +\input{device-types/spi/description.tex}
>> +\section{SPI Controller Device}\label{sec:Device Types / SPI Controller
>> Device}
>> +
>> ...
> 
> Ah no, the inclusion is of course in the right place where you put it in
> context.tex; the *patch hunk* should be moved from patch 1 to patch 2,
> that's all. Hope that's more clear now :)
> 
Hello Cornelia,

Oh I think I get it. The patch 1 should be split into two. Changing the 
wording is still in patch 1 and the line 
"\input{device-types/spi/description.tex}" should be move to patch 2.

Thank you very much for your detailed explanations, I will do so in next 
patch.

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

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

[Harald Mommer]

Hello,

On 12.12.23 04:33, Haixu Cui wrote:
> +\field{bits_per_word_mask} is a mask indicating which values of bits_per_word are supported.
> +If bit n of \field{bits_per_word_mask} is set, the bits_per_word with value (n+1) is supported.
> +If all bits are not set, bits_per_word can be any value from 1 to 32.

"If all bits are not set" is not easy to understand as it means "if no 
bit is set".

You can of course specify the value 0 this way to make code more 
complicated.

To make code simple, 0 would be not given this special meaning. To allow 
any value between 1 and 32 solely the value 0xFFFFFFFF would be allowed. 
Which is already possible now without this additional sentence for 0. 
Simply checking whether the respective bit is set in the mask and done 
without having to add senseless additional code to handle the special 
case that the mask is 0.

Giving 0 the same meaning as 0xFFFFFFFF brings nothing except confusion.



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

[Haixu Cui]

Hello Harald,

On 12/21/2023 7:25 PM, Harald Mommer wrote:
> Hello,
> 
> On 12.12.23 04:33, Haixu Cui wrote:
>> +\field{bits_per_word_mask} is a mask indicating which values of 
>> bits_per_word are supported.
>> +If bit n of \field{bits_per_word_mask} is set, the bits_per_word with 
>> value (n+1) is supported.
>> +If all bits are not set, bits_per_word can be any value from 1 to 32.
> 
> "If all bits are not set" is not easy to understand as it means "if no 
> bit is set".
> 
> You can of course specify the value 0 this way to make code more 
> complicated.
> 
> To make code simple, 0 would be not given this special meaning. To allow 
> any value between 1 and 32 solely the value 0xFFFFFFFF would be allowed. 
> Which is already possible now without this additional sentence for 0. 
> Simply checking whether the respective bit is set in the mask and done 
> without having to add senseless additional code to handle the special 
> case that the mask is 0.
> 
> Giving 0 the same meaning as 0xFFFFFFFF brings nothing except confusion.
> 
Here I refer to the __spi_validate_bits_per_word in drivers/spi/spi.c, 
if bits_per_word_mask of spi controller is 0, no check will be done for 
bits_per_word parameter.

Above is just the Linux mechanism, I agree with you, 0 and 0xFFFFFFFF 
have the same meaning will cause confusion in this spec. I prefer 
treating 0 as an invalid value because the backend must support at least 
one bits_per_word value.

How about updating as:
For \field{bits_per_word_mask}, 0 is invalid because at least one 
bits_per_word value should be supported.

Thanks & 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 V9 2/2] virtio-spi: add the device specification

[Harald Mommer]

Hello Haixu,

On 22.12.23 03:28, Haixu Cui wrote:
> Hello Harald,
>
> On 12/21/2023 7:25 PM, Harald Mommer wrote:
>> Hello,
>>
>> On 12.12.23 04:33, Haixu Cui wrote:
>>> +\field{bits_per_word_mask} is a mask indicating which values of 
>>> bits_per_word are supported.
>>> +If bit n of \field{bits_per_word_mask} is set, the bits_per_word 
>>> with value (n+1) is supported.
>>> +If all bits are not set, bits_per_word can be any value from 1 to 32.
>>
>> "If all bits are not set" is not easy to understand as it means "if 
>> no bit is set".
>>
>> You can of course specify the value 0 this way to make code more 
>> complicated.
>>
>> To make code simple, 0 would be not given this special meaning. To 
>> allow any value between 1 and 32 solely the value 0xFFFFFFFF would be 
>> allowed. Which is already possible now without this additional 
>> sentence for 0. Simply checking whether the respective bit is set in 
>> the mask and done without having to add senseless additional code to 
>> handle the special case that the mask is 0.
>>
>> Giving 0 the same meaning as 0xFFFFFFFF brings nothing except confusion.
>>
> Here I refer to the __spi_validate_bits_per_word in drivers/spi/spi.c, 
> if bits_per_word_mask of spi controller is 0, no check will be done 
> for bits_per_word parameter.
>
> Above is just the Linux mechanism, I agree with you, 0 and 0xFFFFFFFF 
> have the same meaning will cause confusion in this spec. I prefer 
> treating 0 as an invalid value because the backend must support at 
> least one bits_per_word value.
>
> How about updating as:
> For \field{bits_per_word_mask}, 0 is invalid because at least one 
> bits_per_word value should be supported.
>
> Thanks & Regards
> Haixu Cui

Interesting. I know that I had already a comment on this previously. And 
also now. But sometimes I'm wrong. And here probably in both cases but 
at least with my last comment.

  * @bits_per_word_mask: A mask indicating which values of bits_per_word are
  *    supported by the driver. Bit n indicates that a bits_per_word n+1 is
  *    suported. If set, the SPI core will reject any transfer with an
  *    unsupported bits_per_word. If not set, this value is simply ignored,
  *    and it's up to the individual driver to perform any validation.

Tried to find out why this is so in Linux. The function 
__spi_validate_bits_per_word() was introduced by commit 63ab645f4d8b. 
The member variable bits_per_word_mask in struct spi_master was already 
introduced by 543bb255a198. The new variable was introduced in a way so 
that if not set the behavior would not change for existing SPI drivers 
not (yet) setting the field. At least I deduce that looking into the 
commits. Made a lot of sense to introduce bits_per_word_mask in commit 
543bb255a198 in Linux this way to make a change without causing other 
changes everywhere. The virtio SPI specification is new, no need to be 
backwards compatible here so there is no need to have 0 there as "don't 
check" value. So your proposal to make 0 the invalid value is a good 
one. So I thought.

On the other hand: The function __spi_validate_bits_per_word() does not 
check for anything if bits_per_word_mask is 0. With 0 it does not check 
for bits_per_word > 32 and it does not check for bits_per_word being a 
power of 2. It never checks for bits_per_word > 0 but this is a 
different issue.

1.) Now I'm looking into a data sheet from Freescale

https://www.manualslib.com/manual/1376296/Freescale-Semiconductor-Mk22fn256vdc12.html?page=1053

and see that the chip can use any frame sizes from 4 to 32. Means also 
frame sizes which are not a power of 2. Nothing I've seen before but it 
exists. So the value 0 is indeed needed to allow for those values.

The chip can only support 4 to 32 bits, so with the 32 bit restriction 
in the V9 draft we would be fine.

2.) 
https://community.nxp.com/t5/S32K/How-to-send-receive-64bit-or-longer-message-using-SPI/m-p/1476343#M15986

There are Bits/frame of 8, 10, 24, 32, 40, 64 mentioned in the example. 
Not only that some of those are no power of 2 and cannot be represented 
in this bit mask not having 0 but we have also 64 bit frame size. Means 
restriction to 32 bits has also to fall to support something like this.

I looked further and found a data sheet from NXP in the internet which 
should not be there. Maximum frame size for this chip is everything 
between 8 bits and 4096-bits with some values in between excluded. As 
bits_per_word is an u8 in Linux and in the draft spec something so 
strange like this could not be supported by virtio spi nor in Linux.

=> The normal case for the majority of people is 8, 16 and maybe also 32 
bit SPI.

=> The strange case is non power of 2 values which cannot be represented 
in the mask not allowing 0. The V9 draft spec is perfect for this.

=> Another strange case is big frame sizes > 32 bit.

The sentence in draft V7 (on which I moaned, sorry for this) was most 
probably the perfect one: "If not set, there is no limitation for 
bits_per_word."

Regards
Harald



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

[Haixu Cui]

Hello Harald,

On 12/22/2023 10:32 PM, Harald Mommer wrote:
> Hello Haixu,
> 
> On 22.12.23 03:28, Haixu Cui wrote:
>> Hello Harald,
>>
>> On 12/21/2023 7:25 PM, Harald Mommer wrote:
>>> Hello,
>>>
>>> On 12.12.23 04:33, Haixu Cui wrote:
>>>> +\field{bits_per_word_mask} is a mask indicating which values of 
>>>> bits_per_word are supported.
>>>> +If bit n of \field{bits_per_word_mask} is set, the bits_per_word 
>>>> with value (n+1) is supported.
>>>> +If all bits are not set, bits_per_word can be any value from 1 to 32.
>>>
>>> "If all bits are not set" is not easy to understand as it means "if 
>>> no bit is set".
>>>
>>> You can of course specify the value 0 this way to make code more 
>>> complicated.
>>>
>>> To make code simple, 0 would be not given this special meaning. To 
>>> allow any value between 1 and 32 solely the value 0xFFFFFFFF would be 
>>> allowed. Which is already possible now without this additional 
>>> sentence for 0. Simply checking whether the respective bit is set in 
>>> the mask and done without having to add senseless additional code to 
>>> handle the special case that the mask is 0.
>>>
>>> Giving 0 the same meaning as 0xFFFFFFFF brings nothing except confusion.
>>>
>> Here I refer to the __spi_validate_bits_per_word in drivers/spi/spi.c, 
>> if bits_per_word_mask of spi controller is 0, no check will be done 
>> for bits_per_word parameter.
>>
>> Above is just the Linux mechanism, I agree with you, 0 and 0xFFFFFFFF 
>> have the same meaning will cause confusion in this spec. I prefer 
>> treating 0 as an invalid value because the backend must support at 
>> least one bits_per_word value.
>>
>> How about updating as:
>> For \field{bits_per_word_mask}, 0 is invalid because at least one 
>> bits_per_word value should be supported.
>>
>> Thanks & Regards
>> Haixu Cui
> 
> Interesting. I know that I had already a comment on this previously. And 
> also now. But sometimes I'm wrong. And here probably in both cases but 
> at least with my last comment.
> 
>   * @bits_per_word_mask: A mask indicating which values of bits_per_word 
> are
>   *    supported by the driver. Bit n indicates that a bits_per_word n+1 is
>   *    suported. If set, the SPI core will reject any transfer with an
>   *    unsupported bits_per_word. If not set, this value is simply ignored,
>   *    and it's up to the individual driver to perform any validation.
> 
> Tried to find out why this is so in Linux. The function 
> __spi_validate_bits_per_word() was introduced by commit 63ab645f4d8b. 
> The member variable bits_per_word_mask in struct spi_master was already 
> introduced by 543bb255a198. The new variable was introduced in a way so 
> that if not set the behavior would not change for existing SPI drivers 
> not (yet) setting the field. At least I deduce that looking into the 
> commits. Made a lot of sense to introduce bits_per_word_mask in commit 
> 543bb255a198 in Linux this way to make a change without causing other 
> changes everywhere. The virtio SPI specification is new, no need to be 
> backwards compatible here so there is no need to have 0 there as "don't 
> check" value. So your proposal to make 0 the invalid value is a good 
> one. So I thought.
> 
> On the other hand: The function __spi_validate_bits_per_word() does not 
> check for anything if bits_per_word_mask is 0. With 0 it does not check 
> for bits_per_word > 32 and it does not check for bits_per_word being a 
> power of 2. It never checks for bits_per_word > 0 but this is a 
> different issue.
> 
> 1.) Now I'm looking into a data sheet from Freescale
> 
> https://www.manualslib.com/manual/1376296/Freescale-Semiconductor-Mk22fn256vdc12.html?page=1053
> 
> and see that the chip can use any frame sizes from 4 to 32. Means also 
> frame sizes which are not a power of 2. Nothing I've seen before but it 
> exists. So the value 0 is indeed needed to allow for those values.
> 
> The chip can only support 4 to 32 bits, so with the 32 bit restriction 
> in the V9 draft we would be fine.
> 
> 2.) 
> https://community.nxp.com/t5/S32K/How-to-send-receive-64bit-or-longer-message-using-SPI/m-p/1476343#M15986
> 
> There are Bits/frame of 8, 10, 24, 32, 40, 64 mentioned in the example. 
> Not only that some of those are no power of 2 and cannot be represented 
> in this bit mask not having 0 but we have also 64 bit frame size. Means 
> restriction to 32 bits has also to fall to support something like this.
> 
> I looked further and found a data sheet from NXP in the internet which 
> should not be there. Maximum frame size for this chip is everything 
> between 8 bits and 4096-bits with some values in between excluded. As 
> bits_per_word is an u8 in Linux and in the draft spec something so 
> strange like this could not be supported by virtio spi nor in Linux.
> 
> => The normal case for the majority of people is 8, 16 and maybe also 32 
> bit SPI.
> 
> => The strange case is non power of 2 values which cannot be represented 
> in the mask not allowing 0. The V9 draft spec is perfect for this.
> 
> => Another strange case is big frame sizes > 32 bit.
> 
> The sentence in draft V7 (on which I moaned, sorry for this) was most 
> probably the perfect one: "If not set, there is no limitation for 
> bits_per_word."

Thank you very much for your detailed explanations.

There are such strange usage scenarios that frame size exceeds 32 bits, 
even 4096 bits. For these strange cases, It's impossible to use only 32 
bits bits_per_word_mask to enumerate all the conditions. Seems that a 
similar situation exists in Linux spi driver also.

Assume a case where the spi controller supports the frame size to be any 
value from 16 to 64, 32 bits mask can only cover the 16 ~ 32 conditions.

Even so, it's not a good idea to expand the bits_per_word_mask, 32 bits 
can cover most of the cases, except for some situations that are not 
commonly used. Besides, it's hard to determine the bit width to cover 
all the cases, and expanding will make the logic more complicated. To 
summarize, I think the following statement is probably proper:

"\field{bits_per_word_mask} is a mask indicating which values of
bits_per_word are supported. If bit n of \field{bits_per_word_mask} is 
set, the bits_per_word with value (n+1) is supported. If 
\field{bits_per_word_mask} is 0, there is no limitation for bits_per_word."

Do you think it is acceptable and appropriate?

Much appreciated, again.

Thanks
Haixu Cui

> 
> Regards
> Harald
> 
> 


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

[Harald Mommer]

Hello Haixu,
>
> There are such strange usage scenarios that frame size exceeds 32 
> bits, even 4096 bits. For these strange cases, It's impossible to use 
> only 32 bits bits_per_word_mask to enumerate all the conditions. Seems 
> that a similar situation exists in Linux spi driver also.
>
> Assume a case where the spi controller supports the frame size to be 
> any value from 16 to 64, 32 bits mask can only cover the 16 ~ 32 
> conditions.
>
> Even so, it's not a good idea to expand the bits_per_word_mask, 32 
> bits can cover most of the cases, except for some situations that are 
> not commonly used. Besides, it's hard to determine the bit width to 
> cover all the cases, and expanding will make the logic more 
> complicated. To summarize, I think the following statement is probably 
> proper:
>
> "\field{bits_per_word_mask} is a mask indicating which values of
> bits_per_word are supported. If bit n of \field{bits_per_word_mask} is 
> set, the bits_per_word with value (n+1) is supported. If 
> \field{bits_per_word_mask} is 0, there is no limitation for 
> bits_per_word."
>
> Do you think it is acceptable and appropriate?

Perfect.

Currently in the middle (almost done) of updating my software to draft V10.

Regards
Harald Mommer



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

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

[Cornelia Huck]

On Tue, Dec 12 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 host.
>
> This patch adds the specification for virtio-spi.
>
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  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

Oh, and I just noticed that you also need to include
{device,driver}-conformance.tex in conformance.tex.


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

[Haixu Cui]

On 12/22/2023 8:21 PM, Cornelia Huck wrote:
> On Tue, Dec 12 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 host.
>>
>> This patch adds the specification for virtio-spi.
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   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
> 
> Oh, and I just noticed that you also need to include
> {device,driver}-conformance.tex in conformance.tex.
> 

I will add the device and driver in conformance.tex in patch 2.

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

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

[Haixu Cui]

Hi Cornelia,

     In patch V9, all comments have been fixed. Could you please help to 
review the new patch. And if there are no other comments, can you help 
to merge it as the initial version of virtio-spi. Thank you so much for 
your support.

Thanks
Haixu Cui

On 12/12/2023 11:33 AM, 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 host,
> 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                             |   3 +-
>   device-types/spi/description.tex        | 281 ++++++++++++++++++++++++
>   device-types/spi/device-conformance.tex |   7 +
>   device-types/spi/driver-conformance.tex |   7 +
>   4 files changed, 297 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
> 

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