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

2023-11-30 Thread Haixu Cui

Hi Viresh,

On 12/1/2023 1:44 PM, Viresh Kumar wrote:

On 01-12-23, 13:35, Haixu Cui wrote:

Hi Viresh,

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


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


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


Yes, it is. In this spec, for half/full duplex, I use "transfer type", 
and for line number, just use "n-bit transfer", to avoid ambiguity.



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


That's fine too.


OK, thank you.

Best Regards
Haixu Cui

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



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

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

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

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

That's fine too.

-- 
viresh

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



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

2023-11-30 Thread Haixu Cui




On 12/1/2023 12:32 PM, Viresh Kumar wrote:

On 01-12-23, 12:05, Haixu Cui wrote:

Looks good. Will update as follows:

\field{tx_nbits_supported} and \field{rx_nbits_supported} indicate the
different n-bit transfer modes supported by the device, for writing and
reading respectively. SINGLE is always supported. A set bit here indicates
that the corresponding n-bit transfer is supported, otherwise not:
 bit 0: DUAL;
 bit 1: QUAD;
 bit 2: OCTAL;
 other bits are reserved and must be set as 0 by the device.

Note: Transfer bit options are commonly used in SPI:


Maybe:

Note: The commonly used SPI transfer modes are:


Hi Viresh,

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



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

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


Thanks & BR
Haixu Cui

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



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

2023-11-30 Thread Viresh Kumar
On 01-12-23, 12:05, Haixu Cui wrote:
> Looks good. Will update as follows:
> 
> \field{tx_nbits_supported} and \field{rx_nbits_supported} indicate the
> different n-bit transfer modes supported by the device, for writing and
> reading respectively. SINGLE is always supported. A set bit here indicates
> that the corresponding n-bit transfer is supported, otherwise not:
> bit 0: DUAL;
> bit 1: QUAD;
> bit 2: OCTAL;
> other bits are reserved and must be set as 0 by the device.
> 
> Note: Transfer bit options are commonly used in SPI:

Maybe:

Note: The commonly used SPI transfer modes are:

-- 
viresh

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



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

2023-11-30 Thread Haixu Cui

Hi Huck,

On 11/30/2023 7:38 PM, Cornelia Huck wrote:

On Thu, Nov 30 2023, Haixu Cui  wrote:


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

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui 
---
  device-types/spi/description.tex| 288 
  device-types/spi/device-conformance.tex |   7 +
  device-types/spi/driver-conformance.tex |   7 +
  3 files changed, 302 insertions(+)
  create mode 100644 device-types/spi/description.tex
  create mode 100644 device-types/spi/device-conformance.tex
  create mode 100644 device-types/spi/driver-conformance.tex

diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
new file mode 100644
index 000..c4816a6
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,288 @@
+\section{SPI Controller Device}\label{sec:Device Types / SPI Controller Device}
+
+The Virtio SPI (Serial Peripheral Interface) device is a virtual SPI 
controller that
+allows the driver to operate and use the SPI controller under the control of 
the device,
+either a physical SPI controller, or an emulated one.
+
+The Virtio SPI device has a single virtqueue. SPI transfer requests are placed 
into
+the virtqueue, and serviced by the device.


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


Okay, I will add it back to illustrate the host&guest scenario.



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


s/for Virtio SPI driver/for the driver/


Okay.



(matches what is said for other device types)


+The config space shows the features and settings supported by the device.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+   u8 cs_max_number;
+   u8 cs_change_supported;
+   u8 tx_nbits_supported;
+   u8 rx_nbits_supported;
+   le32 bits_per_word_mask;
+   le32 mode_func_supported;
+   le32 max_freq_hz;
+   le32 max_word_delay_ns;
+   le32 max_cs_setup_ns;
+   le32 max_cs_hold_ns;
+   le32 max_cs_inactive_ns;
+};
+\end{lstlisting}
+
+\field{cs_max_number} is the maximum number of chipselect the device supports.
+
+Note: chipselect is an electrical signal controlled by the SPI controller, 
used to select the
+SPI slaves connected to the controller.


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


In Wikipedia,

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


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


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





+
+\field{cs_change_supported} indicates if the device supports to toggle 
chipselect
+after each transfer in one message:
+0: unsupported, means chipselect will keep active when executing the 
message transaction;
+1: supported.
+
+Note: Message here contains a sequence of SPI transfers.
+
+\field{tx_nbits_supported} indicates the supported number of bit for writing, 
SINGLE is always
+supported. In the bit definition below, a set bit means the transfer is 
supported, otherwise not:
+bit 0: DUAL;
+bit 1: QUAD;
+bit 2: OCTAL;
+other bits are reserved and must be set to 0 by the device.
+
+Note: Transfer bit options are commonly used in SPI:
+\begin{itemize}
+\item SINGLE: 1-bit transfer
+\item DUAL: 2-bit transfer
+\item QUAD: 4-bit transfer
+\item OCTAL: 8-bit transfer
+\end{itemize}
+
+\field{rx_nbits_supported} indicates the supported number of bit for reading, 
SINGLE is always
+supported. In the bit definition below, a set bit means the transfer is 
supported, otherwise not:
+bit 0: DUAL;
+bit 1: QUAD;
+bit 2: OCTAL;
+other bits are reserved and must be set to 0 by the device.
+
+\field{bits_per_word_mask} is a mask indicating which values of bits_per_word 
are supported.
+If not set, no limitation for bits_per_word.


s/no limitation/there is no limitation/


Okay.



+
+Note: bits_per

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

2023-11-30 Thread Haixu Cui

Hi Viresh,

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

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

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

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui 
---
  device-types/spi/description.tex| 288 
  device-types/spi/device-conformance.tex |   7 +
  device-types/spi/driver-conformance.tex |   7 +
  3 files changed, 302 insertions(+)
  create mode 100644 device-types/spi/description.tex
  create mode 100644 device-types/spi/device-conformance.tex
  create mode 100644 device-types/spi/driver-conformance.tex

diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
new file mode 100644
index 000..c4816a6
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,288 @@
+\section{SPI Controller Device}\label{sec:Device Types / SPI Controller Device}
+
+The Virtio SPI (Serial Peripheral Interface) device is a virtual SPI 
controller that
+allows the driver to operate and use the SPI controller under the control of 
the device,


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

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


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


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


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





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


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



Okay.


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


s/always available/mandatory/


Okay.


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


Will update the whole spec.



+The config space shows the features and settings supported by the device.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+   u8 cs_max_number;
+   u8 cs_change_supported;
+   u8 tx_nbits_supported;
+   u8 rx_nbits_supported;
+   le32 bits_per_word_mask;
+   le32 mode_func_supported;
+   le32 max_freq_hz;
+   le32 max_word_delay_ns;
+   le32 max_cs_setup_ns;
+   le32 max_cs_hold_ns;
+   le32 max_cs_inactive_ns;
+};
+\end{lstlisting}
+
+\field{cs_max_number} is the maximum number of chipselect the device supports.
+
+Note: chipselect is an electrical signal controlled by the SPI controller, 
used to select the
+SPI slaves connected to the controller.


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

Suggest rewriting as:

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


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



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


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


Okay.



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


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


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

Re: [virtio-dev] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification

2023-11-30 Thread Jonathon Reinhart
On Wed, Nov 29, 2023 at 5:00 AM Viresh Kumar  wrote:
>
> On 29-11-23, 16:54, Haixu Cui wrote:
> > On 11/29/2023 4:33 PM, Cornelia Huck wrote:
> > > On Wed, Nov 29 2023, Haixu Cui  wrote:
> > > > On 11/29/2023 3:30 PM, Viresh Kumar wrote:
> > > > > On 28-11-23, 20:58, Haixu Cui wrote:
> > > > > > On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> > > > > Hmm, I worked with a SPI controller over a decade ago, and I must be 
> > > > > forgetting
> > > > > something here I guess. But from whatever little I remember, with 
> > > > > full-duplex
> > > > > transfer, data flows on both MOSI and MISO lines as soon as clock 
> > > > > signal is
> > > > > applied.  And so amount of data sent is always be equal to amount of 
> > > > > data
> > > > > received by both sides.
> > > > >
> > > > > Also if I see Linux's implementation of the `struct spi_transfer` 
> > > > > [1], I see
> > > > > `tx_buf`, `rx_buf` and a single `len` field, which applies to both 
> > > > > the buffers.
> > > > > Which I guess is indicating that both buffers are supposed to be of 
> > > > > same length.
> > > > >
> > > > > What am I missing ?
> > > >
> > > > Oh so sorry for that. And I don't make it clear. Yes, tx_buf and rx_buf
> > > > have the same size, Linux has such restriction. Just as you mention,
> > > > kernel level spi_transfer has single "len", the same for
> > > > spi_ioc_transfer passed from the userland.
> > > >
> > > > But I am not sure if this is in the scope of the spec. Because this is
> > > > ensured by Linux, but Virtio SPI driver won't also can't verify this.
> > > > This is a prerequisite for virtio spi processing requests.
>
> I don't think this is a Linux only thing. This is how the SPI protocol is
> designed to begin with. A clock is applied, data from FIFOs of both master and
> slaves gets exchanged over the MOSI and MISO lines... And the length is 
> _always_
> the same. We are talking about the full-duplex mode of the hardware here and
> this is how it works AFAICT.
>

IMO the length doesn't _need_ to be the same. Sure, the clock always
runs, and the shift registers of the master and slave are always
clocking in data from their respective input lines. But that doesn't
mean that (all of) the received data actually *means* anything -- that
is determined by the application protocol.

For example, the initiator (master) might want to send 100 bytes but
only cares about the first 8 bytes received. Why should it need to
provide a 100 byte buffer for that received data? Certainly the stack
should be capable of discarding the other 92 bytes?

For a given SPI transfer, the clock needs to run for
(max(num_tx_bytes, num_rx_bytes) * word_size) cycles.

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



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

2023-11-30 Thread Cornelia Huck
On Thu, Nov 30 2023, Haixu Cui  wrote:

> The Virtio SPI (Serial Peripheral Interface) device is a virtual
> SPI controller that allows the driver to operate and use the SPI
> controller under the control of the device.
>
> This patch adds the specification for virtio-spi.
>
> Signed-off-by: Haixu Cui 
> ---
>  device-types/spi/description.tex| 288 
>  device-types/spi/device-conformance.tex |   7 +
>  device-types/spi/driver-conformance.tex |   7 +
>  3 files changed, 302 insertions(+)
>  create mode 100644 device-types/spi/description.tex
>  create mode 100644 device-types/spi/device-conformance.tex
>  create mode 100644 device-types/spi/driver-conformance.tex
>
> diff --git a/device-types/spi/description.tex 
> b/device-types/spi/description.tex
> new file mode 100644
> index 000..c4816a6
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,288 @@
> +\section{SPI Controller Device}\label{sec:Device Types / SPI Controller 
> Device}
> +
> +The Virtio SPI (Serial Peripheral Interface) device is a virtual SPI 
> controller that
> +allows the driver to operate and use the SPI controller under the control of 
> the device,
> +either a physical SPI controller, or an emulated one.
> +
> +The Virtio SPI device has a single virtqueue. SPI transfer requests are 
> placed into
> +the virtqueue, and serviced by the device.

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

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

s/for Virtio SPI driver/for the driver/

(matches what is said for other device types)

> +The config space shows the features and settings supported by the device.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> + u8 cs_max_number;
> + u8 cs_change_supported;
> + u8 tx_nbits_supported;
> + u8 rx_nbits_supported;
> + le32 bits_per_word_mask;
> + le32 mode_func_supported;
> + le32 max_freq_hz;
> + le32 max_word_delay_ns;
> + le32 max_cs_setup_ns;
> + le32 max_cs_hold_ns;
> + le32 max_cs_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +\field{cs_max_number} is the maximum number of chipselect the device 
> supports.
> +
> +Note: chipselect is an electrical signal controlled by the SPI controller, 
> used to select the
> +SPI slaves connected to the controller.

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

> +
> +\field{cs_change_supported} indicates if the device supports to toggle 
> chipselect
> +after each transfer in one message:
> +0: unsupported, means chipselect will keep active when executing the 
> message transaction;
> +1: supported.
> +
> +Note: Message here contains a sequence of SPI transfers.
> +
> +\field{tx_nbits_supported} indicates the supported number of bit for 
> writing, SINGLE is always
> +supported. In the bit definition below, a set bit means the transfer is 
> supported, otherwise not:
> +bit 0: DUAL;
> +bit 1: QUAD;
> +bit 2: OCTAL;
> +other bits are reserved and must be set to 0 by the device.
> +
> +Note: Transfer bit options are commonly used in SPI:
> +\begin{itemize}
> +\item SINGLE: 1-bit transfer
> +\item DUAL: 2-bit transfer
> +\item QUAD: 4-bit transfer
> +\item OCTAL: 8-bit transfer
> +\end{itemize}
> +
> +\field{rx_nbits_supported} indicates the supported number of bit for 
> reading, SINGLE is always
> +supported. In the bit definition below, a set bit means the transfer is 
> supported, otherwise not:
> +bit 0: DUAL;
> +bit 1: QUAD;
> +bit 2: OCTAL;
> +other bits are reserved and must be set to 0 by the device.
> +
> +\field{bits_per_word_mask} is a mask indicating which values of 
> bits_per_word are supported.
> +If not set, no limitation for bits_per_word.

s/no limitation/there is no limitation/

> +
> +Note: bits_per_word corresponds to \field{bits_per_word} in \field{struct 
> virtio_spi_transfer_head}.
> +
> +\field{mode_func_supported} indicates whether the following features are 
> supported or not:
> +bit 0-1: CPHA feature,
> +0b00: supports CPHA=0 and CPHA=1;
> +0b01: supports CPHA=0 only;
> +0b10: supp

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

2023-11-30 Thread Viresh Kumar
On 30-11-23, 17:22, Haixu Cui wrote:
> The Virtio SPI (Serial Peripheral Interface) device is a virtual
> SPI controller that allows the driver to operate and use the SPI
> controller under the control of the device.
> 
> This patch adds the specification for virtio-spi.
> 
> Signed-off-by: Haixu Cui 
> ---
>  device-types/spi/description.tex| 288 
>  device-types/spi/device-conformance.tex |   7 +
>  device-types/spi/driver-conformance.tex |   7 +
>  3 files changed, 302 insertions(+)
>  create mode 100644 device-types/spi/description.tex
>  create mode 100644 device-types/spi/device-conformance.tex
>  create mode 100644 device-types/spi/driver-conformance.tex
> 
> diff --git a/device-types/spi/description.tex 
> b/device-types/spi/description.tex
> new file mode 100644
> index 000..c4816a6
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,288 @@
> +\section{SPI Controller Device}\label{sec:Device Types / SPI Controller 
> Device}
> +
> +The Virtio SPI (Serial Peripheral Interface) device is a virtual SPI 
> controller that
> +allows the driver to operate and use the SPI controller under the control of 
> the device,

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

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

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

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

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

s/always available/mandatory/

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

> +The config space shows the features and settings supported by the device.
> +
> +\begin{lstlisting}
> +struct virtio_spi_config {
> + u8 cs_max_number;
> + u8 cs_change_supported;
> + u8 tx_nbits_supported;
> + u8 rx_nbits_supported;
> + le32 bits_per_word_mask;
> + le32 mode_func_supported;
> + le32 max_freq_hz;
> + le32 max_word_delay_ns;
> + le32 max_cs_setup_ns;
> + le32 max_cs_hold_ns;
> + le32 max_cs_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +\field{cs_max_number} is the maximum number of chipselect the device 
> supports.
> +
> +Note: chipselect is an electrical signal controlled by the SPI controller, 
> used to select the
> +SPI slaves connected to the controller.

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

Suggest rewriting as:

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

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

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

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

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

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

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

"
\field{tx_nbits_supported} and \field{rx_nbits_supported} indicate the different
n-bit transfer modes 

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

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

Reviewed-by: Viresh Kumar 

-- 
viresh

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



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

2023-11-30 Thread Haixu Cui
The Virtio SPI (Serial Peripheral Interface) device is a virtual
SPI controller that allows the driver to operate and use the SPI
controller under the control of the device.

This patch adds the specification for virtio-spi.

Signed-off-by: Haixu Cui 
---
 device-types/spi/description.tex| 288 
 device-types/spi/device-conformance.tex |   7 +
 device-types/spi/driver-conformance.tex |   7 +
 3 files changed, 302 insertions(+)
 create mode 100644 device-types/spi/description.tex
 create mode 100644 device-types/spi/device-conformance.tex
 create mode 100644 device-types/spi/driver-conformance.tex

diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
new file mode 100644
index 000..c4816a6
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,288 @@
+\section{SPI Controller Device}\label{sec:Device Types / SPI Controller Device}
+
+The Virtio SPI (Serial Peripheral Interface) device is a virtual SPI 
controller that
+allows the driver to operate and use the SPI controller under the control of 
the device,
+either a physical SPI controller, or an emulated one.
+
+The Virtio SPI device has a single virtqueue. SPI transfer requests are placed 
into
+the virtqueue, and serviced by the device.
+
+\subsection{Device ID}\label{sec:Device Types / SPI Controller Device / Device 
ID}
+45
+
+\subsection{Virtqueues}\label{sec:Device Types / SPI Controller Device / 
Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / SPI Controller Device / 
Feature bits}
+
+None
+
+\subsection{Device configuration layout}\label{sec:Device Types / SPI 
Controller Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for Virtio 
SPI driver.
+The config space shows the features and settings supported by the device.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+   u8 cs_max_number;
+   u8 cs_change_supported;
+   u8 tx_nbits_supported;
+   u8 rx_nbits_supported;
+   le32 bits_per_word_mask;
+   le32 mode_func_supported;
+   le32 max_freq_hz;
+   le32 max_word_delay_ns;
+   le32 max_cs_setup_ns;
+   le32 max_cs_hold_ns;
+   le32 max_cs_inactive_ns;
+};
+\end{lstlisting}
+
+\field{cs_max_number} is the maximum number of chipselect the device supports.
+
+Note: chipselect is an electrical signal controlled by the SPI controller, 
used to select the
+SPI slaves connected to the controller.
+
+\field{cs_change_supported} indicates if the device supports to toggle 
chipselect
+after each transfer in one message:
+0: unsupported, means chipselect will keep active when executing the 
message transaction;
+1: supported.
+
+Note: Message here contains a sequence of SPI transfers.
+
+\field{tx_nbits_supported} indicates the supported number of bit for writing, 
SINGLE is always
+supported. In the bit definition below, a set bit means the transfer is 
supported, otherwise not:
+bit 0: DUAL;
+bit 1: QUAD;
+bit 2: OCTAL;
+other bits are reserved and must be set to 0 by the device.
+
+Note: Transfer bit options are commonly used in SPI:
+\begin{itemize}
+\item SINGLE: 1-bit transfer
+\item DUAL: 2-bit transfer
+\item QUAD: 4-bit transfer
+\item OCTAL: 8-bit transfer
+\end{itemize}
+
+\field{rx_nbits_supported} indicates the supported number of bit for reading, 
SINGLE is always
+supported. In the bit definition below, a set bit means the transfer is 
supported, otherwise not:
+bit 0: DUAL;
+bit 1: QUAD;
+bit 2: OCTAL;
+other bits are reserved and must be set to 0 by the device.
+
+\field{bits_per_word_mask} is a mask indicating which values of bits_per_word 
are supported.
+If not set, no limitation for bits_per_word.
+
+Note: bits_per_word corresponds to \field{bits_per_word} in \field{struct 
virtio_spi_transfer_head}.
+
+\field{mode_func_supported} indicates whether the following features are 
supported or not:
+bit 0-1: CPHA feature,
+0b00: supports CPHA=0 and CPHA=1;
+0b01: supports CPHA=0 only;
+0b10: supports CPHA=1 only;
+0b11: invalid, must support as least one CPHA setting.
+
+bit 2-3: CPOL feature,
+0b00: supports CPOL=0 and CPOL=1;
+0b01: supports CPOL=0 only;
+0b10: supports CPOL=1 only;
+0b11: invalid, must support as least one CPOL setting.
+
+bit 4: chipselect active high feature, 0 for unsupported and 1 for 
supported,
+chipselect active low must always be supported.
+
+bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB 
first must always be
+supported.
+
+bit 6: loopback mode feature, 0 for unsupported and 1 for supported, 
normal mode
+must always be supported.
+
+Note: CPOL is clock polarity and CPHA is clock phase. If CPO

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

2023-11-30 Thread Haixu Cui
SPI master is an outdated term and should use SPI controller.

Signed-off-by: Haixu Cui 
---
 content.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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


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



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

2023-11-30 Thread Haixu Cui
The Virtio SPI (Serial Peripheral Interface) device is a virtual SPI controller 
that
allows the driver to operate and use the SPI controller under the control of 
the device,
either a physical SPI controller, or an emulated one.

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

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

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

-- 
2.17.1


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



[virtio-dev] Re: [virtio-comment] [PATCH v2 0/1] Define low power mode for devices

2023-11-30 Thread Michael S. Tsirkin
On Mon, Nov 13, 2023 at 03:19:49PM +0900, David Stevens wrote:
> The virtio spec currently does not include the concept of device power
> management. The lack means that there is no good action drivers can take
> when they are requested to put the device into a low power state (e.g.
> when a guest is entering a system-wide low power state like S0ix/S3).
> Stateless devices can be handled - albeit inefficiently - by resetting
> and reinitialzing the device. However, stateful devices cannot support
> this situation. This patch defines a low power mode for devices that can
> be used in this situation.
> 
> Low power mode is mostly defined at the transport layer, and all
> device-side power optimizations are optional. This avoids the need for
> invasive device-by-device definitions. It also pushes the requirement
> onto the device side, to simplify what driver side changes are
> necessary to just [1].

Great idea.
This couldn't have come at a better time, too.
Please do remember to CC reviewers directly in the future, though.


> I believe this patch may address the virtio-gpu issue which [2] is
> trying to address by avoiding the reset altogether when the guest enters
> S3.
> 
> [1] 
> https://lore.kernel.org/lkml/20231113055138.117392-1-steve...@chromium.org/
> [2] https://lore.kernel.org/lkml/20230919114242.2283646-1-jiqian.c...@amd.com/
> 
> v1 -> v2:
>  - Define virtio-pci support on top of PCI power management.
>  - Add more conformance requirements.
> 
> David Stevens (1):
>   Define a low power mode for devices
> 
>  content.tex   | 45 +
>  transport-pci.tex |  7 +++
>  2 files changed, 52 insertions(+)
> 
> -- 
> 2.42.0.869.gea05f2083d-goog
> 
> 
> 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-subscr...@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org
> List help: virtio-comment-h...@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-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



[virtio-dev] Re: [virtio-comment] [PATCH v2 1/1] Define a low power mode for devices

2023-11-30 Thread Michael S. Tsirkin
On Mon, Nov 13, 2023 at 03:19:50PM +0900, David Stevens wrote:
> Define a low power mode for virtio devices where the devices are
> expected to maintain their state. This gives drivers an option for power
> management besides simply resetting their device. In the virtualization
> use case, this allows the guest to be suspended even with stateful
> virtio devices like gpu and fs.
> 
> Low power mode is primarily defined at the transport layer. The only
> part that depends on device-type specific details is whether a given
> virtqueue is device driven or driver driven.
> 
> This change only defines the transport-specific implementation for
> Virtio over PCI.
> ---
>  content.tex   | 45 +
>  transport-pci.tex |  7 +++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 0a62dce5f65f..7016778c38d6 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -502,6 +502,51 @@ \section{Exporting Objects}\label{sec:Basic Facilities 
> of a Virtio Device / Expo
>  types. It is RECOMMENDED that devices generate version 4
>  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>  
> +\section{Low Power Mode}\label{sec:Basic Facilities of a Virtio Device / Low 
> Power Mode}




> +
> +Low power mode is a state a driver can put its device into to try to
> +reduce the resource consumption of the device.

This sounds somewhat vague and the grammar is convoluted. E.g. who
tries what?

How about something like:
Devices and drivers can implement a low power mode: in this mode device
state is maintained but driver does not access the device, allowing the
device to reduce the power consumption.


> While in low power
> +mode, a device can generate wakeup events to inform its driver about
> +device driven events.

using the term events here is confusing because it already
has a meaning in the spec solely related to ring (discussed
in the context of device event suppression).
And "driven" is too close to "driver" and
confuses me.

I think these are
really vq notifications and config change events right?
Maybe just say so instead of coming up with new terms.


> How a device is put into and taken out of low
> +power mode is transport specific, as is how wakeup events are
> +implemented.
> +

I would move the definition of device driven queues here and
add the definition of driver driven queues.

And add a high level explanation of how device and driver
interact (repeating a bit what normative sections say,
this duplication is normal).


> +\devicenormative{\subsection}{Low Power Mode}{Basic Facilities of a Virtio
> +Device / Low Power Mode}
> +
> +A device in low power mode MUST maintain its state such that all
> +driver visible state after exiting low power mode exactly matches
> +driver visible state before entering low power mode.
> +
> +A device in low power mode MUST continue to operate device driven
> +queues. Device driven queues are queues where the driver provides

makes available is the term we use

> +buffers to the device that the device holds onto for an indeterminate
> +time until some device-side event occurs (e.g. event queues, rx
> +queues). When sending a used buffer notification for such a queue, the
> +device MUST generate a wakeup event.

is it only when sending notification? not when buffers are used?
also what does "when" mean here? after sending a notification?

> +
> +A device in low power mode MUST continue to send configuration change
> +notifications. When sending a configuration change notification, the
> +device MUST generate a wakeup event.
> +
> +A device in low power mode SHOULD NOT generate wakeup events for
> +driver driven queues. A device SHOULD stop sending used buffer
> +notifications for such queues until after exiting the low power state.
> +
> +A device in low power mode SHOULD minimize its resource usage,
> +although what steps to take are specific to a particular device
> +implementation.
> +
> +\drivernormative{\subsection}{Low Power Mode}{Basic Facilities of a Virtio
> +Device / Low Power Mode}
> +
> +A driver MUST not interact with a device in low power mode except
> +to take the device out of low power mode

i get this
in a transport specific manner, yes?

> or to handle wakeup events
> +generated by the device.

i don't get this. what does "handle" mean here?


> +
> +A driver MAY use wakeup events as a trigger to take the device out of
> +low power mode, but it MAY also ignore wakeup events.
> +
>  \input{admin.tex}
>  
>  \chapter{General Initialization And Device Operation}\label{sec:General 
> Initialization And Device Operation}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index a5c6719ea871..6694e0f72d46 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -1212,3 +1212,10 @@ \subsubsection{Driver Handling 
> Interrupts}\label{sec:Virtio Transport Options /
>  re-examine the configuration space to see what changed.
>  \end{itemize}
>

[virtio-dev] Re: [PATCH v2 0/1] Define low power mode for devices

2023-11-30 Thread David Stevens
On Mon, Nov 13, 2023 at 3:20 PM David Stevens  wrote:
>
> The virtio spec currently does not include the concept of device power
> management. The lack means that there is no good action drivers can take
> when they are requested to put the device into a low power state (e.g.
> when a guest is entering a system-wide low power state like S0ix/S3).
> Stateless devices can be handled - albeit inefficiently - by resetting
> and reinitialzing the device. However, stateful devices cannot support
> this situation. This patch defines a low power mode for devices that can
> be used in this situation.
>
> Low power mode is mostly defined at the transport layer, and all
> device-side power optimizations are optional. This avoids the need for
> invasive device-by-device definitions. It also pushes the requirement
> onto the device side, to simplify what driver side changes are
> necessary to just [1].
>
> I believe this patch may address the virtio-gpu issue which [2] is
> trying to address by avoiding the reset altogether when the guest enters
> S3.
>
> [1] 
> https://lore.kernel.org/lkml/20231113055138.117392-1-steve...@chromium.org/
> [2] https://lore.kernel.org/lkml/20230919114242.2283646-1-jiqian.c...@amd.com/
>
> v1 -> v2:
>  - Define virtio-pci support on top of PCI power management.
>  - Add more conformance requirements.

Friendly ping, and directly CC'ing reviewers.

-David

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