[virtio-dev] Re: [virtio-comment] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification
On Mon, Nov 27 2023, Cornelia Huck wrote:
> On Mon, Nov 27 2023, Haixu Cui wrote:
>
>> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>>> On Fri, Nov 24 2023, Haixu Cui wrote:
+The \field{chip_select_max_number} is the maximum number of chipselect
the host SPI controller supports.
>>>
>>> "chipselect" is probably a known term for people familiar with SPI -- is
>>> there any definition of those terms that the spec can point to?
>>
>> Just as Mark said, there is no formal spec for SPI, so no standard spec
>> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
>> below.
>
> If we have nothing to point to, it is probably best to simply
> expand/explain the terms on their first usage.
>
>>> Can we point to some documentation that explains CPHA and CPOL?
>>
>> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in
>> wikipedia(Clock polarity and phase chapter):
>>
>> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>>
>> How about copying some concise information from wikipedia as Note? Or is
>> referring to such webpage acceptable in this spec.
>
> Not sure if we can do an outright copy (licence compatibility), but
> paraphrasing should be fine. (I'd rather not directly reference the
> site, because the content is not guaranteed to be stable, but we could
> maybe add it as "further reading".)
Looking again, the kernel doc referenced by Mark
(https://www.kernel.org/doc/html/v6.6/driver-api/spi.html) might also be
a good candidate, especially as we can refer to a specific version.
-
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: [PATCH] [PATCH v5] virtio-spi: add the device specification
On Mon, Nov 27 2023, Haixu Cui wrote:
> On 11/24/2023 11:46 PM, Cornelia Huck wrote:
>> On Fri, Nov 24 2023, Haixu Cui wrote:
>>> +The \field{chip_select_max_number} is the maximum number of chipselect the
>>> host SPI controller supports.
>>
>> "chipselect" is probably a known term for people familiar with SPI -- is
>> there any definition of those terms that the spec can point to?
>
> Just as Mark said, there is no formal spec for SPI, so no standard spec
> for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
> below.
If we have nothing to point to, it is probably best to simply
expand/explain the terms on their first usage.
>> Can we point to some documentation that explains CPHA and CPOL?
>
> Here. No standard SPI spec to point to. CPOL/CPHA have definitions in
> wikipedia(Clock polarity and phase chapter):
>
> https://en.wikipedia.org/wiki/Serial_Peripheral_Interface
>
> How about copying some concise information from wikipedia as Note? Or is
> referring to such webpage acceptable in this spec.
Not sure if we can do an outright copy (licence compatibility), but
paraphrasing should be fine. (I'd rather not directly reference the
site, because the content is not guaranteed to be stable, but we could
maybe add it as "further reading".)
>>> +For each transfer request, Virtio SPI driver MUST check the fields in
>>> structure \field{virtio_spi_transfer_head}
>>> +and MUST reject the request if any filed is invalid or enabling the
>>> features not supported by host.
>>
>> s/filed/field/
>> s/host/device/
>>
>> Also, isn't the rejecting supposed to be done by the device, as the
>> driver is the party enqueueing the requests? Or do I have some kind of
>> fundamental misunderstanding?
>
> It may be better to filter some invalid requests by driver, as in the
> request header there are many parameters, and some of them are not
> supported by device, so it's quite possible that many requests invalid
> for the device. So if driver can do the first filter, such invalid
> requests will not be sent at all, this will conserve virtqueue and
> system overhead.
>
> And this is why exposing device supported features in the config space,
> it ensures that almost all requests in virtqueue are nice to the backend.
>
> device also will verify the requests again, as the following requirement:
> 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 device unsupported features are set.
>
> Although checking the requests twice seems a little redundant, it is
> more efficient comparing with sending some invalid requests to the device.
>
> What is your opinion? Do you think it is acceptable?
Thanks for your explanation, I think we simply have some terminology
issues. In the virtio spec, "driver" refers to one side of the
driver/device pair, and is used to describe how to communicate with the
device. In this case, "driver" would be the entity interacting with the
device, regardless of how it is implemented, and would be responsible
for sending the requests. Any filtering that would be done in a concrete
implementation (i.e. if you have one component generating the requests,
and then another component filtering them before actually putting them
into the queue) is out of scope for this spec -- we can maybe specify
that the driver should not send invalid requests, but I'm not sure that
this is actually needed.
> Once again, thanks a lot for your support.
You're welcome!
-
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: [PATCH] [PATCH v5] virtio-spi: add the device specification
Hi Huck,
Really appreciate for your comments and please refer to my replies
below each comment.
On 11/24/2023 11:46 PM, Cornelia Huck wrote:
On Fri, Nov 24 2023, 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.
As Mark has already reviewed it from the SPI POV (thanks!), I'm now
looking at the virtio side.
Signed-off-by: Haixu Cui
---
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 000..8ca8c0d
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,281 @@
+\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
+
+virtio-spi is a virtual SPI (Serial Peripheral Interface) controller and it
allows
+a guest to operate and use the host SPI controller. Host SPI controller may be
a
+physical controller, or a virtual one(e.g. controller emulated by the software
Missing space before the opening bracket.
+running in the host).
+
+virtio-spi has a single virtqueue. SPI transfer requests are placed into
+the virtqueue, and serviced by the host SPI controller.
Is there any way to express all of this without referring to "host" and
"guest" here? (The paragraph below giving it as an example is fine.)
Something like "The virtio-spi device is a virtual SPI controller. It
allows the driver to operate and use an SPI controller under the control
of the device, either a physical controller, or an emulated one."
Okay, in next patch, guest will be replaced by driver
+
+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.
"chipselect" is probably a known term for people familiar with SPI -- is
there any definition of those terms that the spec can point to?
Just as Mark said, there is no formal spec for SPI, so no standard spec
for such terms referring to. The same for CPHA/CPOL/LSB/MSB, please see
below.
Also, it should be either "The field \field{whatever}" or
"\field{whatever}"; "The \field{whatever}" looks good in the LaTeX
source, but not in the end result.
Okay, use "\field{whatever}".
+
+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.
+
+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;
+other bits are reserved as 0, 1-bit transfer is always supported.
+
+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.
+
+The \field{bits_per_word_mask} is a mask indicating which
[virtio-dev] Re: [PATCH] [PATCH v5] virtio-spi: add the device specification
Hi Mark, Thank you, really appreciate. Best Regards Haixu Cui On 11/24/2023 8:48 PM, Mark Brown wrote: On Fri, Nov 24, 2023 at 03:20:15PM +0800, 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. This looks fine from a SPI point of view, I don't really have any idea about what makes sense or is idomatic at the virtio level so can't really review there: Reviewed-by: Mark Brown - 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] virtio-snd comments/questions
On Wed, Nov 22, 2023 at 06:59:15PM +, Radu Ocica wrote: > Hi Matias, > > > > > > > > > Could you elaborate on this? Which audio backend would not support > > > > PAUSE? I am not very familiar with audio engines. > > > > > > For instance in linux, alsa-lib has a flag that specifies pause support > > > by a PCM stream: > > > #define SNDRV_PCM_INFO_PAUSE (1UL<<19) /* pause > > > ioctl is supported */ > > > The snd_pcm_pause API can only be used successfully if the PCM stream has > > > this bit set in its PCM info. > > > A similar concept exists in QNX sound. > > > > > > Oh, I see. I was unaware of this flag. Not supporting PAUSE in the host > > sound card may not be a problem. In our case, the pw backend in the host > > stops playing when a guest sends STOP immediately. As soon as START is > > sent again, pw begins consuming from the tx queue. Tx buffers are only > > signaled as complete after START is reissued. In the meantime, the msgs > > are in the available ring of the tx queue. > > > > However, if the host stream does not support pause the only option > > > left is to perform a drop operation on the host stream and in this > > > case all pending messages in the tx/rx queue need to be returned to > > > the guest as completed, because at the next start possibly all > > > messages will be needed to prime the host stream (in the case of > > > playback). > > > If STOP is sent and there is still msgs in the TX queue, can't the > > device just stop playing until START is issued again? Why does the > > device require to drop the msgs in this case? > Invoking the pcm drop API on the host has been the modality chosen by us to > stop playing, with immediate effect. Whatever io messages were buffered at > the time by the host pcm device need then to be returned to the guest driver > as completed. Messages in the TX queue at the time of the STOP request can be > left untouched. Depending on the relationship between number of periods used > by the guest device vs number of periods used by the host device, the TX > queue might be normally empty though (for instance if host and guest device > use the same number of periods, all pcm data received from the guest driver > would end up immediately in the backend buffer). > The way you describe STOP it sounds like you just stop writing new data to > the host pcm device, without invoking any API to explicitly stop it (if pause > API not available for the host pcm device). This allows for currently backend > buffered data to continue playing and correspondingly io messages need to be > returned to the guest driver as they are consumed. If no START is received > before the backend buffered data completes playing, the host device then > underruns. At this point it either continues running by injecting silence or > stops, depending on setup options. The effective stop of sound playback > occurs when the backend buffered data completes playing, which might be after > a considerable delay, depending on backend buffer size. Keeping the host > device running and injecting silence sounds like the easiest way to avoid the > need to re-prepare the host pcm device when restarting. Have I captured your > approach accurately? For example, in the audio backend for pipewire, the STOP cmd stops playing, i.e., the device stops to consume from the tx queue. This cmd translates to something like `stream.set_active(false)`. In the pw backend, the consumption happens from a thread named the threadloop. When setting `set_active(false)`, such a thread stops to execute. There is no need to insert silence from the audio backend. The threadloop starts to execute again and thus consuming from tx when issuing `set_active(true)` during the START cmd. Matias - 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] [PATCH v5] virtio-spi: add the device specification
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
> ---
> 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 000..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..
> +
> +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/
?
> +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..
> +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..
> +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.
> +
> +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
