Hi Viresh,
Thank you for your comments and please refer to my replies below
each comment.
On 11/27/2023 6:17 PM, Viresh Kumar wrote:
Hi Haixu,
Cornelia has already covered most of the issues, I will try to add few more
things I noticed.
On 24-11-23, 15:20, Haixu Cui wrote:
virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it
allows
a guest to operate and use the host SPI controller.
This patch adds the specification for virtio-spi.
Signed-off-by: Haixu Cui <[email protected]>
---
device-types/spi/description.tex | 281 ++++++++++++++++++++++++
device-types/spi/device-conformance.tex | 7 +
device-types/spi/driver-conformance.tex | 7 +
3 files changed, 295 insertions(+)
create mode 100644 device-types/spi/description.tex
create mode 100644 device-types/spi/device-conformance.tex
create mode 100644 device-types/spi/driver-conformance.tex
diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
new file mode 100644
index 0000000..8ca8c0d
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,281 @@
+\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
Not sure if we should use "Master" anywhere..
Maybe: s/SPI Master Device/SPI Device/ ?
Applies elsewhere too..
Master here shows the spec is for SPI master rather than SPI slave.
But Master is outdated term and will be replaced by "SPI controller
device" in next patch.
+
+virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it
allows
Maybe:
s/virtio-spi/The Virtio SPI device/ (this applies at multiple places)
s/and it allows/that allows/
?
Okay, "The Virtio SPI device" is much better than virtio-spi.
+a guest to operate and use the host SPI controller. Host SPI controller may be
a
Maybe: The host SPI controller ... ?
+physical controller, or a virtual one(e.g. controller emulated by the software
+running in the host).
+
+virtio-spi has a single virtqueue. SPI transfer requests are placed into
The Virtio SPI device..
Likewise.
+the virtqueue, and serviced by the host SPI controller.
+
+In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
+the front-end running in the guest kernel, and Virtio SPI device acts as the
+back-end in the host.
+
+\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
+45
+
+\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device /
Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature
bits}
+
+None
+
+\subsection{Device configuration layout}\label{sec:Device Types / SPI Master
Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for Virtio
SPI driver.
+The config space shows the features and settings supported by the host SPI
controller.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+ u8 chip_select_max_number;
+ u8 cs_change_supported;
+ u8 tx_nbits_supported;
+ u8 rx_nbits_supported;
+ le32 bits_per_word_mask;
+ le32 mode_func_supported;
+ le32 max_freq_hz;
+ le32 max_word_delay_ns;
+ le32 max_cs_setup_ns;
+ le32 max_cs_hold_ns;
+ le32 max_cs_inactive_ns;
+};
+\end{lstlisting}
+
+The \field{chip_select_max_number} is the maximum number of chipselect the
host SPI controller supports.
+
+The \field{cs_change_supported} indicates if the host SPI controller supports
to toggle chipselect
+after each transfer in one message:
+ 0: supported;
+ 1: unsupported, means chipselect will keep active when executing the
message transaction.
+Note: Message here contains a sequence of SPI transfer >
I would rather suggest using '1' for supported and '0' for unsupported, like you
have done for LSB first, loopback, etc. later..
OK, will update in next patch.
+The \field{tx_nbits_supported} indicates the supported number of bit for
writing:
+ bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
+ bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
+ bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
Maybe you can mention first that, a set bit means feature is supported,
otherwise now. And then you can just write:
bit 0: 2-bit transfer
bit 1: 4-bit transfer
bit 2: 8-bit transfer
+ other bits are reserved as 0, 1-bit transfer is always supported.
Maybe write as: other bits are reserved and MUST be set to 0 by the device.
Agree. Will update.
+
+The \field{rx_nbits_supported} indicates the supported number of bit for
reading:
+ bit 0: 2-bit transfer, 0 for unsupported and 1 for supported;
+ bit 1: 4-bit transfer, 0 for unsupported and 1 for supported;
+ bit 2: 8-bit transfer, 0 for unsupported and 1 for supported;
+ other bits are reserved as 0, 1-bit transfer is always supported.
Same here..
+
+The \field{bits_per_word_mask} is a mask indicating which values of
bits_per_word
+are supported. If not set, no limitation for bits_per_word.
+Note: bits_per_word corresponds to \field{bits_per_word} field in
+structure \field{virtio_spi_transfer_head}, see below.
+
+The \field{mode_func_supported} indicates the following features are supported
or not:
+ bit 0-1: CPHA feature,
+ 0b00: supports CPHA=0 and CPHA=1;
+ 0b01: supports CPHA=0 only;
+ 0b10: supports CPHA=1 only;
+ 0b11: invalid, should support as least one CPHA setting.
+
+ bit 2-3: CPOL feature,
+ 0b00: supports CPOL=0 and CPOL=1;
+ 0b01: supports CPOL=0 only;
+ 0b10: supports CPOL=1 only;
+ 0b11: invalid, should support as least one CPOL setting.
Maybe explain what CPHA and CPOL are ..
I will add Note to explain these terms here.
+
+ bit 4: chipselect active high feature, 0 for unsupported and 1 for
supported,
+ chipselect active low should always be supported.
+
+ bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB
first should always be
+ supported.
+
+ bit 6: loopback mode feature, 0 for unsupported and 1 for supported,
normal mode
+ should always be supported.
+
+The \field{max_freq_hz} is the maximum clock rate supported in Hz unit, 0
means no
+limitation for transfer speed.
+
+The \field{max_word_delay_ns} is the maximum word delay supported in ns unit,
0 means
+word delay feature is unsupported.
+Note: Just as one message contains a sequence of transfers, one transfer may
contain
+a sequence of word.
+
+The \field{max_cs_setup_ns} is the maximum delay supported after chipselect is
asserted, in ns unit,
+0 means delay is not supported to introduce after chipselect is asserted.
+
+The \field{max_cs_hold_ns} is the maximum delay supported before chipselect is
deasserted, in ns unit,
+0 means delay is not supported to introduce before chipselect is deasserted.
+
+The \field{max_cs_incative_ns} is the maximum delay supported after chipselect
is deasserted, in ns unit,
+0 means delay is not supported to introduce after chipselect is deasserted.
+
+\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device
/ Device Initialization}
+
+\begin{enumerate}
+\item The Virtio SPI driver configures and initializes the virtqueue.
+\end{enumerate}
+
+\subsection{Device Operation}\label{sec:Device Types / SPI Master Device /
Device Operation}
+
+\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI
Master Device / Device Operation: Request Queue}
+
+Virtio SPI driver enqueues requests to the virtqueue, and they are used by
Virtio SPI device.
The Virtio SPI driver..
+Each request represents one SPI tranfer and is of the form:
+
+\begin{lstlisting}
+struct virtio_spi_transfer_head {
+ u8 chip_select_id;
+ u8 bits_per_word;
+ u8 cs_change;
+ u8 tx_nbits;
+ u8 rx_nbits;
+ u8 reserved[3];
+ le32 mode;
+ le32 freq;
+ le32 word_delay_ns;
+ le32 cs_setup_ns;
+ le32 cs_delay_hold_ns;
+ le32 cs_change_delay_inactive_ns;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_transfer_result {
+ u8 result;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_transfer_req {
+ struct virtio_spi_transfer_head head;
+ u8 tx_buf[];
+ u8 rx_buf[];
+ struct virtio_spi_transfer_result result;
+};
+\end{lstlisting}
+
+The \field{chip_select_id} indicates the chipselect index the SPI transfer
used.
+
+The \field{bits_per_word} indicates the number of bits in each SPI transfer
word.
+
+The \field{cs_change} indicates whether to deselect device before starting the
+next SPI transfer, 0 means chipselect keep asserted and 1 means chipselect
deasserted
+then asserted again.
+
+The \field{tx_nbits} indicates number of bits used for writing:
+ 0,1: 1-bit transfer, also known as SINGLE;
+ 2 : 2-bit transfer, also known as DUAL;
+ 4 : 4-bit transfer, also known as QUAD;
+ 8 : 8-bit transfer, also known as OCTAL;
+ other values are invalid.
+
+The \field{rx_nbits} indicates number of bits used for reading:
+ 0,1: 1-bit transfer, also known as SINGLE;
+ 2 : 2-bit transfer, also known as DUAL;
+ 4 : 4-bit transfer, also known as QUAD;
+ 8 : 8-bit transfer, also known as OCTAL;
+ other values are invalid.
Agree. Will update.
The description of the above two fields look exactly same, can we define this
just once and mention it applies for both ?
+The \field{reserved} is for alignement, also for further extension.
+
+The \field{mode} indicates some transfer settings. Bit definitions as follows:
+ bit 0: CPHA, determines the timing (i.e. phase) of the data bits
+ relative to the clock pulses.
+ bit 1: CPOL, determines the polarity of the clock.
+ bit 2: CS_HIGH, if 1, chipselect active high, else active low.
+ bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
+ first, else LSB first.
+ bit 4: LOOP, if 1, device is in loopback mode, else normal mode.
+
+The \field{freq} indicates the SPI transfer speed in Hz.
+
+The \field{word_delay_ns} indicates delay to be inserted between consecutive
+words of a transfer, in ns unit.
+
+The \field{cs_setup_ns} indicates delay to be introduced after chipselect
+is asserted, in ns unit.
+
+The \field{cs_delay_hold_ns} indicates delay to be introduced before chipselect
+is deasserted, in ns unit.
+
+The \field{cs_change_delay_inactive_ns} indicates delay to be introduced after
+chipselect is deasserted and before next asserted, in ns unit.
+
+The \field{tx_buf} is the buffer for data sent to the device.
+
+The \field{rx_buf} is the buffer for data received from the device.
+
+The final \field{result} is the transfer result, VIRTIO_SPI_TRANS_OK for
success,
+VIRTIO_SPI_PARAM_ERR for parameter error, which means the parameters in
+\field{virtio_spi_transfer_head} are not all valid, or some fields are set as
non-zero values
+but the corresponding features are not supported by host, while
+VIRTIO_SPI_TRANS_ERR for transfer error, means that the parameters are all
+valid but trasnfer process failed.
+
+\begin{lstlisting}
+#define VIRTIO_SPI_TRANS_OK 0
+#define VIRTIO_SPI_PARAM_ERR 1
+#define VIRTIO_SPI_TRANS_ERR 2
+\end{lstlisting}
+
+\subsubsection{Device Operation: Operation Status}\label{sec:Device Types /
SPI Master Device / Device Operation: Operation Status}
+
+Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI
driver, while
+\field{result} in structure \field{virtio_spi_transfer_result} is written by
Virtio SPI device.
+
+virtio-spi supports three transfer types:
+ 1) half-duplex read;
+ 2) half-duplex write;
+ 3) full-duplex read and write.
+
+For half-duplex read transfer, \field{rx_buf} is filled by Virtio SPI device
and consumed
See, you are already using "Virtio SPI device/driver" here. Just use the same
throughout instead of "virtio-spi".
+by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled
by Virtio
+SPI driver and consumed by Virtio SPI device. And for full-duplex read and
write transfer,
+both \field{tx_buf} and \field{rx_buf} are used.
Should the length of both the buffers in full-duplex mode be same ? If yes, then
this should be mentioned (in case it is not).
No, there is no such limitation. Write and read buffers may be different
is size.
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Master
Device / Device Operation}
+
+The Virtio SPI driver MUST send transfer requests on the requestq virtqueue.
+
+Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio
SPI driver
+and MUST be readable for Virtio SPI device.
+
+Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI
device
+and MUST be writable for Virtio SPI device.
+
+For half-duplex read, Virtio SPI driver MUST send structure
\field{virtio_spi_transfer_head},
+\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio
Device in order.
s/structure \field{virtio_spi_transfer_head}/\field{struct
virtio_spi_transfer_head}/ ?
That should automatically add struct before its name.
Got it. I will update the statement.
+
+For half-duplex write, Virtio SPI driver MUST send structure
\field{virtio_spi_transfer_head},
+\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio
Device in order.
+
+For full-duplex read and write, Virtio SPI driver MUST send structure
\field{virtio_spi_transfer_head},
+\field{tx_buf}, \field{rx_buf} and structure
\field{virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For half-duplex read or full-duplex read and write transfer, Virtio SPI driver
MUST not use \field{rx_buf}
s/ or /, /
+if the \field{result} returned from Virtio SPI device is not
VIRTIO_SPI_TRANS_OK.
+
+For each transfer request, Virtio SPI driver MUST check the fields in
structure \field{virtio_spi_transfer_head}
s/driver/device/
+and MUST reject the request if any filed is invalid or enabling the features
not supported by host.
s/filed/field/
s/features not supported by host/features is not supported by the host/
As Cornelia has suggested, I will remove this requirement and reserve
the following one:
Virtio SPI device MUST verify the parameters in
\field{virtio_spi_transfer_head} after receiving the request,
and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR
if not all parameters are valid or some host SPI controller unsupported
features are set.
+For example, if \field{freq} in structure \field{virtio_spi_transfer_head} is
greater than \field{max_freq_hz}
+in config space, or if \field{max_cs_setup_ns} in config space is 0 while
\field{cs_setup_ns} in structure
+\field{virtio_spi_transfer_head} is non-zero, Virtio SPI driver will reject
such request.
As I said earlier, maybe we should talk about length of the rx/tx buffers for
full-duplex transfers here.
>> +
+\devicenormative{\subsubsection}{Device Operation}{Device Types / SPI Master
Device / Device Operation}
+
+Virtio SPI device MUST set all the fields of the structure
\field{virtio_spi_config} before
+they are read by Virtio SPI driver.
+
+Virtio SPI device MUST set the structure \field{virtio_spi_transfer_result}
before sending
+it back to Virtio SPI driver.
+
+Virtio SPI device MUST be able to identify the transfer type according to the
received
+virtqueue descriptors.
+
+Virtio SPI device MUST NOT change the data in \field{tx_buf} if transfer type
is half-duplex write
+or full-duplex read and write.
The device MUST NOT change the contents of the tx_buf ever, i.e. for every mode
there is. In half-duplex read mode, there is no tx_buf, but just rx_buf.
Sorry I don'y understand here. In this statement, the transfer type is
either half-duplex write or full-duplex read and write. half-duplex read
type has no tx_buf, so I didn't mention it.
Looking forware to your comments.
+
+Virtio SPI device MUST verify the parameters in
\field{virtio_spi_transfer_head} after receiving the request,
+and MUST set \field{virtio_spi_transfer_result} as VIRTIO_SPI_PARAM_ERR if not
all parameters are valid or
+some host SPI controller unsupported features are set.
diff --git a/device-types/spi/device-conformance.tex
b/device-types/spi/device-conformance.tex
new file mode 100644
index 0000000..3e771bc
--- /dev/null
+++ b/device-types/spi/device-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Master Device Conformance}\label{sec:Conformance
/ Device Conformance / SPI Master Device Conformance}
+
+An SPI Master device MUST conform to the following normative statements:
The Virtio SPI device
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / SPI Master Device / Device Operation}
+\end{itemize}
diff --git a/device-types/spi/driver-conformance.tex
b/device-types/spi/driver-conformance.tex
new file mode 100644
index 0000000..3c965ef
--- /dev/null
+++ b/device-types/spi/driver-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Master Driver Conformance}\label{sec:Conformance
/ Driver Conformance / SPI Master Driver Conformance}
+
+An SPI Master driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
+\end{itemize}
Thanks.
Thank you very much again for your supports.
Best Regards
Haixu Cui
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
