Re: [virtio-dev] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
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
[virtio-dev] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
On 11/30/2023 12:13 PM, Viresh Kumar wrote:
On 29-11-23, 13:42, Cornelia Huck wrote:
On Wed, Nov 29 2023, Viresh Kumar wrote:
On 29-11-23, 18:31, Haixu Cui wrote:
Hi Viresh,
Thank you for your helpful comments. In next patch, I will clearly point
this out:
Great, finally we are on the same page. Thanks Haixu.
"For full-duplex read and write transfer, both \field{tx_buf} and
\field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
as \field{rx_buf}."
Suggest rewriting as:
In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
buffers MUST be same.
Is that in a non-normative section? (Sorry, I've lost track here...)
Hi Viresh，
I think yes, though I am not sure where Haixu will put it eventually :)
I would like to put it in normative section, both for driver and device.
If
so, I would say:
"The length of both buffers has to be the same."
That's better. Thanks.
device MUST fail the transfer and notify the driver.
"notify the driver" == "set an appropriate error value"?
Can this be covered by one of the existing error values?
I think yes, this can be handled by the Param error.
Yes, this seems still in scope of parameter checking.
Thanks & 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
[virtio-dev] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
Hi Huck,
On 11/29/2023 8:42 PM, Cornelia Huck wrote:
On Wed, Nov 29 2023, Viresh Kumar wrote:
On 29-11-23, 18:31, Haixu Cui wrote:
Hi Viresh,
Thank you for your helpful comments. In next patch, I will clearly point
this out:
Great, finally we are on the same page. Thanks Haixu.
"For full-duplex read and write transfer, both \field{tx_buf} and
\field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
as \field{rx_buf}."
Suggest rewriting as:
In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
buffers MUST be same.
Is that in a non-normative section? (Sorry, I've lost track here...) If
so, I would say:
"The length of both buffers has to be the same."
This is non-normative section.
And in drivernormative section, I will add a requirement:
"For full-duplex transfer, Virtio SPI driver MUST guarantee the write
transfer size is equal to the read transfer size"
Maybe:
drivernormative:
In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
length of both \field{tx_buf} and \field{rx_buf} are same.
s/are same/is the same/
devicenormative:
In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the
s/are same/is the same/
device MUST fail the transfer and notify the driver.
"notify the driver" == "set an appropriate error value"?
Can this be covered by one of the existing error values?
I'd like to use VIRTIO_SPI_PARAM_ERR to indicates the buffer size
inequality issue here because this is in the scope of parameter checking.
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
[virtio-dev] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
On 29-11-23, 13:42, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Viresh Kumar wrote:
>
> > On 29-11-23, 18:31, Haixu Cui wrote:
> >> Hi Viresh,
> >>
> >> Thank you for your helpful comments. In next patch, I will clearly
> >> point
> >> this out:
> >
> > Great, finally we are on the same page. Thanks Haixu.
> >
> >> "For full-duplex read and write transfer, both \field{tx_buf} and
> >> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
> >> as \field{rx_buf}."
> >
> > Suggest rewriting as:
> >
> > In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are
> > sent by
> > the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both
> > the
> > buffers MUST be same.
>
> Is that in a non-normative section? (Sorry, I've lost track here...)
I think yes, though I am not sure where Haixu will put it eventually :)
> If
> so, I would say:
>
> "The length of both buffers has to be the same."
That's better. Thanks.
> > device MUST fail the transfer and notify the driver.
>
> "notify the driver" == "set an appropriate error value"?
>
> Can this be covered by one of the existing error values?
I think yes, this can be handled by the Param error.
--
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] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
On Wed, Nov 29 2023, Viresh Kumar wrote:
> On 29-11-23, 18:31, Haixu Cui wrote:
>> Hi Viresh,
>>
>> Thank you for your helpful comments. In next patch, I will clearly point
>> this out:
>
> Great, finally we are on the same page. Thanks Haixu.
>
>> "For full-duplex read and write transfer, both \field{tx_buf} and
>> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
>> as \field{rx_buf}."
>
> Suggest rewriting as:
>
> In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent
> by
> the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
> buffers MUST be same.
Is that in a non-normative section? (Sorry, I've lost track here...) If
so, I would say:
"The length of both buffers has to be the same."
>
>> And in drivernormative section, I will add a requirement:
>> "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
>> transfer size is equal to the read transfer size"
>
> Maybe:
>
> drivernormative:
>
> In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
> length of both \field{tx_buf} and \field{rx_buf} are same.
s/are same/is the same/
>
> devicenormative:
>
> In full-duplex transfer mode, the Virtio SPI device MUST verify that the
> length
> of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch,
> the
s/are same/is the same/
> device MUST fail the transfer and notify the driver.
"notify the driver" == "set an appropriate error value"?
Can this be covered by one of the existing error values?
-
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
On 29-11-23, 18:31, Haixu Cui wrote:
> Hi Viresh,
>
> Thank you for your helpful comments. In next patch, I will clearly point
> this out:
Great, finally we are on the same page. Thanks Haixu.
> "For full-duplex read and write transfer, both \field{tx_buf} and
> \field{rx_buf} are used and the buffer size of \field{tx_buf} must be same
> as \field{rx_buf}."
Suggest rewriting as:
In full-duplex transfer mode, both \field{tx_buf} and \field{rx_buf} are sent by
the driver, \field{tx_buf} followed by \field{rx_buf}. The length of both the
buffers MUST be same.
> And in drivernormative section, I will add a requirement:
> "For full-duplex transfer, Virtio SPI driver MUST guarantee the write
> transfer size is equal to the read transfer size"
Maybe:
drivernormative:
In full-duplex transfer mode, the Virtio SPI driver MUST guarantee that the
length of both \field{tx_buf} and \field{rx_buf} are same.
devicenormative:
In full-duplex transfer mode, the Virtio SPI device MUST verify that the length
of both \field{tx_buf} and \field{rx_buf} are same. In case of any mismatch, the
device MUST fail the transfer and notify the driver.
> In fact, in my prototype, I only perform the transfers with same tx/rx
> length.
Great.
Thanks.
--
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] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
On 11/29/2023 6:00 PM, 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.
So the spec should clearly point this out to avoid any mistakes. Or if you have
a usecase where you can have different lengths for tx and rx buffers, that I am
not aware of ?
Hi Viresh,
Thank you for your helpful comments. In next patch, I will clearly
point this out:
"For full-duplex read and write transfer, both \field{tx_buf} and
\field{rx_buf} are used and the buffer size of \field{tx_buf} must be
same as \field{rx_buf}."
And in drivernormative section, I will add a requirement:
"For full-duplex transfer, Virtio SPI driver MUST guarantee the
write transfer size is equal to the read transfer size"
In fact, in my prototype, I only perform the transfers with same
tx/rx length.
Much appreciated.
Best Regards
Haixu Cui
What is your suggestion? How about adding some descriptions here, like
"for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
by the kernel"?
We must not really make any assumptions in the spec about concrete
implementations (here, the Linux kernel), as someone implementing it in
a different environment will need to make explicit choices.
I agree. I pointed this out since this is how the protocol works.
So, if tx_buf and rx_buf are required to be of the same size, it needs
to be explicitly stated in the spec, or an implementation might choose
to do it differently.
--
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] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
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. So the spec should clearly point this out to avoid any mistakes. Or if you have a usecase where you can have different lengths for tx and rx buffers, that I am not aware of ? > > > What is your suggestion? How about adding some descriptions here, like > > > "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed > > > by the kernel"? > > > > We must not really make any assumptions in the spec about concrete > > implementations (here, the Linux kernel), as someone implementing it in > > a different environment will need to make explicit choices. I agree. I pointed this out since this is how the protocol works. > > So, if tx_buf and rx_buf are required to be of the same size, it needs > > to be explicitly stated in the spec, or an implementation might choose > > to do it differently. -- 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] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
On 11/29/2023 4:33 PM, Cornelia Huck wrote:
On Wed, Nov 29 2023, Haixu Cui wrote:
Hi Viresh,
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:
On 24-11-23, 15:20, Haixu Cui wrote:
+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.
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.
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.
What is your suggestion? How about adding some descriptions here, like
"for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
by the kernel"?
We must not really make any assumptions in the spec about concrete
implementations (here, the Linux kernel), as someone implementing it in
a different environment will need to make explicit choices.
So, if tx_buf and rx_buf are required to be of the same size, it needs
to be explicitly stated in the spec, or an implementation might choose
to do it differently.
Okay! Thanks Huck for guidance and thanks Viresh for pointing it out.
Much appreciated.
Best Regards
Haixu Cui
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-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/
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] [PATCH v5] virtio-spi: add the device specification
On Wed, Nov 29 2023, Haixu Cui wrote:
> Hi Viresh,
>
> 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:
On 24-11-23, 15:20, Haixu Cui wrote:
> +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.
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.
>>
>> 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.
>
> What is your suggestion? How about adding some descriptions here, like
> "for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
> by the kernel"?
We must not really make any assumptions in the spec about concrete
implementations (here, the Linux kernel), as someone implementing it in
a different environment will need to make explicit choices.
So, if tx_buf and rx_buf are required to be of the same size, it needs
to be explicitly stated in the spec, or an implementation might choose
to do it differently.
-
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 Viresh,
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:
On 24-11-23, 15:20, Haixu Cui wrote:
+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.
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.
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.
What is your suggestion? How about adding some descriptions here, like
"for full-duplex, tx_buf and rx_buf are same in size, this is guaranteed
by the kernel"?
+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.
Sorry for not being clear earlier. I was suggesting you to just write this
instead:
The Virtio SPI device MUST NOT change the data in the \field{tx_buf} field.
You don't really need to specify the modes here, as we are talking about tx_buf,
which isn't available in the half-duplex read mode but all others.
OK, thank you for your explanation. I will remove the "if" statement here.
-- > Viresh
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/spi/spi.h#n1031
Thanks again.
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
[virtio-dev] Re: [virtio-comment] [PATCH] [PATCH v5] virtio-spi: add the device specification
On 28-11-23, 20:58, Haixu Cui wrote:
> On 11/27/2023 6:17 PM, Viresh Kumar wrote:
> > On 24-11-23, 15:20, Haixu Cui wrote:
> > > +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.
> >
> > 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.
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 ?
> > > +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.
Sorry for not being clear earlier. I was suggesting you to just write this
instead:
The Virtio SPI device MUST NOT change the data in the \field{tx_buf} field.
You don't really need to specify the modes here, as we are talking about tx_buf,
which isn't available in the half-duplex read mode but all others.
--
Viresh
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/spi/spi.h#n1031
-
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 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
---
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..
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
[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
