OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


Subject: Re: [PATCH v4] virtio-spi: add the device specification


Hello Haixu,

this delay stuff causes some headache.

On 24.10.23 14:53, Haixu Cui wrote:
virtio-spi is a virtual SPI master and it allows a guest to operate and
use the physical SPI master controlled by the host.

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui<quic_haixcui@quicinc.com>
---
  device-types/spi/description.tex        | 206 ++++++++++++++++++++++++
  device-types/spi/device-conformance.tex |   7 +
  device-types/spi/driver-conformance.tex |   7 +
  3 files changed, 220 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..5dbceaf
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,206 @@
+\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
+
+virtio-spi is a virtual SPI (Serial Peripheral Interface) master and it allows
+a guest to operate and use the physical SPI master controlled by the host.
+
+virtio-spi has a single virtqueue. SPI transfer requests are placed into
+the virtqueue, and serviced by the physical SPI master.
+
+In a typical host and guest architecture with virtio-spi, Virtio SPI driver is
+the front-end existing in the guest kernel, and Virtio SPI device acts as the
+back-end in the host platform.
+
+\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.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+	le16 bus_num;
+	le16 chip_select_max_number;
+	le8 cs_timing_setting_enable;
The naming "cs_timing_setting_enable" may be sub-optimal as also timings which have nothing to do with CS (chip select) are affected. See below where it's discussed in more detail.
+	le8 reserved[3];

There is no "le8". This is "u8".

BTW, I currently in my Linux driver code announce support for 8 and 16 bit SPI word length out of the blue, 125000000 bps transfer speed out of the blue not knowing what my back end device really supports. In struct spi_master the mode member is set to all kinds of bits hoping for the best without really knowing whether the virtio SPI device supports all of this.

For a first specification version this may be acceptable that there is announced something based on nothing but I already see that the information provided in the config space will not be the last word for all times looking at my code.

+};
+\end{lstlisting}
+
+The \field{bus_num} indicates the physical SPI master assigned to guest.
+
+The \field{chip_select_max_number} is the maximum number of chipselect the physical SPI master supports.
+
+The \field{cs_timing_setting_enable} indicates if the physical SPI master supporting cs timing setting:
+	0: physical SPI master doesn't support cs timing setting;
+	1: physical SPI master supports cs timing setting.
+
+The \field{reserved} is for alignment purpose, also for future extension.
+
+\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device / Device Initialization}
+
+\begin{enumerate}
+\item The Virtio SPI driver configures and initializes the virtqueue.
+\end{enumerate}
+
+\subsection{Device Operation}\label{sec:Device Types / SPI Master Device / Device Operation}
+
+\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI Master Device / Device Operation: Request Queue}
+
+Virtio SPI driver enqueues requests to the virtqueue, and they are used by
+Virtio SPI device. Each request represents one SPI tranfer and is of the form:
+
+\begin{lstlisting}
+struct virtio_spi_transfer_head {
+        u8 slave_id;
+        u8 bits_per_word;
+        u8 cs_change;
+        u8 tx_nbits;
+        u8 rx_nbits;
+	 u8 reserved[3];
You may not see it in your E-Mail client, but there is a tab in front of "u8 reserved[3];" instead of spaces. This destroys the formatting of the generated PDF.
+        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}

I see a virtqueue as a communication channel over which all kinds of messages can be transferred. The existing message may have to be enhanced in the future in some way nobody today thinks about with the outcome that we may have in the future the need to distinguish between different kinds of messages.

Adding a flags field or a message type at the top of struct virtio_spi_transfer_head to become more future proof? It may never be needed, so I'm also fine without this small addition but it is worth to think about this for some minutes.

+
+\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{slave_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 bus width for write transfer:
+        0,1: bus width is 1, also known as SINGLE;
+	2  : bus width is 2, also known as DUAL;
+	4  : bus width is 4, also known as QUAD;
+	8  : bus width is 8, also known as OCTAL;
+	other values are invalid.
+
+The \field{rx_nbits} indicates bus width for read transfer:
+        0,1: bus width is 1, also known as SINGLE;
+	2  : bus width is 2, also known as DUAL;
+	4  : bus width is 4, also known as QUAD;
+	8  : bus width is 8, also known as OCTAL;
+	other values are invalid.
+
+The \field{reserved} is for alignement, also for further extension.

alignment

+
+The \field{mode} indicates how data is clocked out and in. 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 to the device.
+
+The final \field{result} is the transfer result, either VIRTIO_SPI_TRANS_OK for success
+or VIRTIO_SPI_TRANS_ERR for error.
+
+\begin{lstlisting}
+#define VIRTIO_SPI_TRANS_OK     0
+#define VIRTIO_SPI_TRANS_ERR    1
+\end{lstlisting}
+
+\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / SPI Master Device / Device Operation: Operation Status}
+
+Fields in structure \field{virtio_spi_transfer_head} are written by Virtio SPI driver, while
+\field{result} in structure \field{virtio_spi_transfer_result} is written by Virtio SPI device.
+
+virtio-spi supports three transfer types:
+        1) half-duplex read;
+        2) half-duplex write;
+        3) full-duplex read and write.
+
+For half-duplex read transfer, \field{rx_buf} is filled by Virtio SPI device and consumed
+by Virtio SPI driver. For half-duplex write transfer, \field{tx_buf} is filled by Virtio
+SPI driver and consumed by Virtio SPI device. And for full-duplex read and write transfer,
+both \field{tx_buf} and \field{rx_buf} are used.
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / SPI Master Device / Device Operation}
+
+The Virtio SPI driver MUST send transfer requests on the requestq virtqueue.
+
+Fields in structure \field{virtio_spi_transfer_head} MUST be filled by Virtio SPI driver
+and MUST be readable for Virtio SPI device.
+
+Structure \field{virtio_spi_transfer_result} MUST be filled by Virtio SPI device
+and MUST be writable for Virtio SPI device.
+
+For half-duplex read, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
+\field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For half-duplex write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
+\field{tx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For full-duplex read and write, Virtio SPI driver MUST send structure \field{virtio_spi_transfer_head},
+\field{tx_buf}, \field{rx_buf} and structure \field{virtio_spi_transfer_result} to SPI Virtio Device in order.
+
+For half-duplex write or full-duplex read and write transfer, Virtio SPI driver MUST not use \field{rx_buf}
+if the \field{result} returned from Virtio SPI device is VIRTIO_SPI_TRANS_ERR.
+
+If \field{cs_timing_setting_enable} in structure \field{virtio_spi_config} is 0, while \field{cs_setup_ns},
+\field{cs_setup_ns} and \field{cs_change_delay_inactive_ns} of the transfer are not all zero, Virtio
+SPI driver MUST print a warning log to alert that the cs timing won't be set as expected.

We are talking about

ÂÂÂ __le32 word_delay_ns;
ÂÂÂ __le32 cs_setup_ns;
ÂÂ Â__le32 cs_delay_hold_ns;
ÂÂ Â__le32 cs_change_delay_inactive_ns;

1.) cs_setup_ns is a duplicate. cs_delay_hold_ns is missing. word_delay_ns is also missing!

2.) There is on Linux no problem with cs_change_delay_inactive_ns. For e.g. 4.14 this is in struct spi_transfer xfer the member delay_usecs, for latest kernels this is delay (or cs_change_delay, currently unsure). There should also be no problem to have this because on every platform after a SPI transaction has been completed such a delay could be introduced simply by calling some Delay() function. This one probably can be supported everywhere.

2.) The most important field, le32 word_delay_ns is also missing.

ÂÂÂÂÂ - This one has nothing to do with CS, so the config space name prefixed by cs_ is misnamed ÂÂÂÂÂ - I have no idea how to support this word_delay_ns on older Linux versions and on some hardware platforms

3.) Logging a warning. This is an unusual requirement. When the driver can know from the config space it should handle according to the information it got from there. I propose to get somewhat harder here.

Device requirement: The device MUST reject a message by some tbd error if tbd delay timing is not supported but the requested value is not zero .

Driver requirement: If the device did not announce support of tbd delay timings in the config space the driver SHOULD not sent a delay timing not equal to zero but should immediately reject the message.

And I think best is not to define 0 and 1 for cs_timing_setting_enable but

#define SPI_HAVE_WORD_DELAY 0x01
#define SPI_HAVE_CS_SETUP 0x02
#define SPI_HAVE_CS_HOLD 0x04
#define SPI_HAVE_CS_CHANGE_INACTIVE 0x08 /* Only if not required to be supported always by the device */

so the driver knows the delays supported by the device exactly.

BTW, in my driver code I have currently still a problem with the mappings of

ÂÂÂ __le32 word_delay_ns;
ÂÂÂ __le32 cs_setup_ns;
ÂÂ Â__le32 cs_delay_hold_ns;
ÂÂ Â__le32 cs_change_delay_inactive_ns;

to Linux struct spi_transfer word_delay, delay, cs_change_delay.

4 values to be filled in struct spi_transfer_head from 3 values in struct spi_transfer of latest Linux. Maybe I just did not get from where to get cleanly the 4th value even from the newest Linux driver environment.

We have between Linux 4.14 and latest some intermediate states in struct spi_transfer and someone (could be me) may have to cope with this.

+\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.
diff --git a/device-types/spi/device-conformance.tex b/device-types/spi/device-conformance.tex
new file mode 100644
index 0000000..3e771bc
--- /dev/null
+++ b/device-types/spi/device-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Master Device Conformance}\label{sec:Conformance / Device Conformance / SPI Master Device Conformance}
+
+An SPI Master device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / SPI Master Device / Device Operation}
+\end{itemize}
diff --git a/device-types/spi/driver-conformance.tex b/device-types/spi/driver-conformance.tex
new file mode 100644
index 0000000..3c965ef
--- /dev/null
+++ b/device-types/spi/driver-conformance.tex
@@ -0,0 +1,7 @@
+\conformance{\subsection}{SPI Master Driver Conformance}\label{sec:Conformance / Driver Conformance / SPI Master Driver Conformance}
+
+An SPI Master driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / SPI Master Device / Device Operation}
+\end{itemize}

We may discuss further based on code, internal paperwork done so the Linux driver which was done internally at OpenSynergy can go out now as RFC as well.

Regards
Harald Mommer



[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]