Re: [virtio-dev][PATCH V9 2/2] virtio-spi: add the device specification
Hello Harald,
On 12/21/2023 7:25 PM, Harald Mommer wrote:
Hello,
On 12.12.23 04:33, Haixu Cui wrote:
+\field{bits_per_word_mask} is a mask indicating which values of
bits_per_word are supported.
+If bit n of \field{bits_per_word_mask} is set, the bits_per_word with
value (n+1) is supported.
+If all bits are not set, bits_per_word can be any value from 1 to 32.
"If all bits are not set" is not easy to understand as it means "if no
bit is set".
You can of course specify the value 0 this way to make code more
complicated.
To make code simple, 0 would be not given this special meaning. To allow
any value between 1 and 32 solely the value 0x would be allowed.
Which is already possible now without this additional sentence for 0.
Simply checking whether the respective bit is set in the mask and done
without having to add senseless additional code to handle the special
case that the mask is 0.
Giving 0 the same meaning as 0x brings nothing except confusion.
Here I refer to the __spi_validate_bits_per_word in drivers/spi/spi.c,
if bits_per_word_mask of spi controller is 0, no check will be done for
bits_per_word parameter.
Above is just the Linux mechanism, I agree with you, 0 and 0x
have the same meaning will cause confusion in this spec. I prefer
treating 0 as an invalid value because the backend must support at least
one bits_per_word value.
How about updating as:
For \field{bits_per_word_mask}, 0 is invalid because at least one
bits_per_word value should be supported.
Thanks & 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 V9 2/2] virtio-spi: add the device specification
Hello,
On 12.12.23 04:33, Haixu Cui wrote:
+\field{bits_per_word_mask} is a mask indicating which values of bits_per_word
are supported.
+If bit n of \field{bits_per_word_mask} is set, the bits_per_word with value
(n+1) is supported.
+If all bits are not set, bits_per_word can be any value from 1 to 32.
"If all bits are not set" is not easy to understand as it means "if no
bit is set".
You can of course specify the value 0 this way to make code more
complicated.
To make code simple, 0 would be not given this special meaning. To allow
any value between 1 and 32 solely the value 0x would be allowed.
Which is already possible now without this additional sentence for 0.
Simply checking whether the respective bit is set in the mask and done
without having to add senseless additional code to handle the special
case that the mask is 0.
Giving 0 the same meaning as 0x brings nothing except confusion.
-
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 V9 2/2] virtio-spi: add the device specification
On Thu, Dec 21 2023, Haixu Cui wrote:
> Hi Cornelia,
>
> On 12/21/2023 4:44 PM, Cornelia Huck wrote:
>> On Thu, Dec 21 2023, Haixu Cui wrote:
Please move inclusion of this new file into content.tex here, instead of
including a not-yet-existing file in the previous patch.
(...)
>>>
>>>
>>> Sorry, I don't quite understand here. Maybe I should not add the
>>> following line in content.tex, is that so?
>>>
>>> \input{device-types/spi/description.tex}
>>
>> No, please add this line; but do so in this patch instead of the
>> previous one that only changes the wording.
>
> I am not clear where to put this line? In the commit message or in the
> main text, e.g. at the start of the description.tex? As follows:
>
> diff --git a/device-types/spi/description.tex
> b/device-types/spi/description.tex
> new file mode 100644
> index 000..b76258c
> --- /dev/null
> +++ b/device-types/spi/description.tex
> @@ -0,0 +1,281 @@
> +\input{device-types/spi/description.tex}
> +\section{SPI Controller Device}\label{sec:Device Types / SPI Controller
> Device}
> +
> ...
Ah no, the inclusion is of course in the right place where you put it in
context.tex; the *patch hunk* should be moved from patch 1 to patch 2,
that's all. Hope that's more clear now :)
-
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 V9 2/2] virtio-spi: add the device specification
Hi Cornelia,
On 12/21/2023 4:44 PM, Cornelia Huck wrote:
On Thu, Dec 21 2023, Haixu Cui wrote:
Hi Cornelia,
Much appreciated for your comments and please refer to my response.
On 12/18/2023 7:04 PM, Cornelia Huck wrote:
On Tue, Dec 12 2023, Haixu Cui wrote:
[BTW: A changelog would be useful, either in the patch or in the cover
letter.]
Sure, I summarize the major changes between these versions and will add
in next patch:
changelog:
v8->v9:
- add explanation of bits_per_word_mask in config space
v7->v8:
- change device to host
v6->v7:
- fix the format problem, syntax problem
v5->v6:
- use driver/device instead guest/host
- add the definition of some terminologies
- use controller instead of master throughout the spec
- add buffer length validation for full-duplex transfer
v4->v5:
- use controller instead of master
- fix indentation issue
- extend the config space to expose the backend supported features
- add another result value to indicate parameter error
- add device and driver requirement about parameter checkig
v3->v4:
- fix the spell error
- bus_num is not SOC-specific, remove it
- add driver requirement to deal with the situation that the cs delay
parameters are not 0 but the backend doesn't support cs timing setting
v2->v3
- remove unnecessary statements and driver implimentation details
- add the parameters about cs timing delay and transfer delay
- use "le32" instead of "u32"
- swap the rx_buf and tx_buf in the request format
- add the parameters about transfer bit width
v1->v2:
- explain SPI when it is firstly used
- update the ambiguous expression of virtqueue
v0->v1:
- add definition of abbreviation SPI
- remove the ID
Thanks. Might be best to add this in the cover letter.
In next patch I will add it in cover letter.
---
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 host.
This patch adds the specification for virtio-spi.
Signed-off-by: Haixu Cui
Reviewed-by: Viresh Kumar
---
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
Please move inclusion of this new file into content.tex here, instead of
including a not-yet-existing file in the previous patch.
(...)
Sorry, I don't quite understand here. Maybe I should not add the
following line in content.tex, is that so?
\input{device-types/spi/description.tex}
No, please add this line; but do so in this patch instead of the
previous one that only changes the wording.
I am not clear where to put this line? In the commit message or in the
main text, e.g. at the start of the description.tex? As follows:
diff --git a/device-types/spi/description.tex
b/device-types/spi/description.tex
new file mode 100644
index 000..b76258c
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,281 @@
+\input{device-types/spi/description.tex}
+\section{SPI Controller Device}\label{sec:Device Types / SPI Controller
Device}
+
...
+\field{mode_func_supported} indicates whether the following features are
supported or not:
+bit 0-1: CPHA feature,
+0b00: invalid, must support as least one CPHA setting.
+0b01: supports CPHA=0 only;
+0b10: supports CPHA=1 only;
+0b11: supports CPHA=0 and CPHA=1;
+
+bit 2-3: CPOL feature,
+0b00: invalid, must support as least one CPOL setting.
+0b01: supports CPOL=0 only;
+0b10: supports CPOL=1 only;
+0b11: supports CPOL=0 and CPOL=1;
+
+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 CPOL is 0, the clock
idles at the logical
+low voltage, otherwise it idles at the logical high voltage. CPHA determines
how data is outputted
+and sampled.
Shouldn't you also specify what CPHA==0 and CPHA==1 mean?
Yes, I will add the following explanation:
If CPHA is 0, the first bit is outputted immediately when chipselect is
active and subsequent bits are outputted on the clock edges when clock
s/when clock/when the clock/
transitions from active level to idle level. Data is sampled on the
clock edges when clock transitions
Re: [virtio-dev][PATCH V9 2/2] virtio-spi: add the device specification
On Thu, Dec 21 2023, Haixu Cui wrote:
> Hi Cornelia,
> Much appreciated for your comments and please refer to my response.
>
> On 12/18/2023 7:04 PM, Cornelia Huck wrote:
>> On Tue, Dec 12 2023, Haixu Cui wrote:
>>
>> [BTW: A changelog would be useful, either in the patch or in the cover
>> letter.]
>
> Sure, I summarize the major changes between these versions and will add
> in next patch:
>
> changelog:
> v8->v9:
> - add explanation of bits_per_word_mask in config space
> v7->v8:
> - change device to host
> v6->v7:
> - fix the format problem, syntax problem
> v5->v6:
> - use driver/device instead guest/host
> - add the definition of some terminologies
> - use controller instead of master throughout the spec
> - add buffer length validation for full-duplex transfer
> v4->v5:
> - use controller instead of master
> - fix indentation issue
> - extend the config space to expose the backend supported features
> - add another result value to indicate parameter error
> - add device and driver requirement about parameter checkig
> v3->v4:
> - fix the spell error
> - bus_num is not SOC-specific, remove it
> - add driver requirement to deal with the situation that the cs delay
> parameters are not 0 but the backend doesn't support cs timing setting
> v2->v3
> - remove unnecessary statements and driver implimentation details
> - add the parameters about cs timing delay and transfer delay
> - use "le32" instead of "u32"
> - swap the rx_buf and tx_buf in the request format
> - add the parameters about transfer bit width
> v1->v2:
> - explain SPI when it is firstly used
> - update the ambiguous expression of virtqueue
> v0->v1:
> - add definition of abbreviation SPI
> - remove the ID
Thanks. Might be best to add this in the cover letter.
> ---
>
>>
>>> 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 host.
>>>
>>> This patch adds the specification for virtio-spi.
>>>
>>> Signed-off-by: Haixu Cui
>>> Reviewed-by: Viresh Kumar
>>> ---
>>> 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
>>
>> Please move inclusion of this new file into content.tex here, instead of
>> including a not-yet-existing file in the previous patch.
>>
>> (...)
>
>
> Sorry, I don't quite understand here. Maybe I should not add the
> following line in content.tex, is that so?
>
> \input{device-types/spi/description.tex}
No, please add this line; but do so in this patch instead of the
previous one that only changes the wording.
>
>>
>>> +\field{mode_func_supported} indicates whether the following features are
>>> supported or not:
>>> +bit 0-1: CPHA feature,
>>> +0b00: invalid, must support as least one CPHA setting.
>>> +0b01: supports CPHA=0 only;
>>> +0b10: supports CPHA=1 only;
>>> +0b11: supports CPHA=0 and CPHA=1;
>>> +
>>> +bit 2-3: CPOL feature,
>>> +0b00: invalid, must support as least one CPOL setting.
>>> +0b01: supports CPOL=0 only;
>>> +0b10: supports CPOL=1 only;
>>> +0b11: supports CPOL=0 and CPOL=1;
>>> +
>>> +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 CPOL is 0, the
>>> clock idles at the logical
>>> +low voltage, otherwise it idles at the logical high voltage. CPHA
>>> determines how data is outputted
>>> +and sampled.
>>
>> Shouldn't you also specify what CPHA==0 and CPHA==1 mean?
>>
>
> Yes, I will add the following explanation:
>
> If CPHA is 0, the first bit is outputted immediately when chipselect is
> active and subsequent bits are outputted on the clock edges when clock
s/when clock/when the clock/
> transitions from active level to idle level. Data is sampled on the
> clock edges when clock transitions from idle level to active level.
same
>
> If CPHA is 1, the first bit is outputted on the fist clock edge after
> chipselect is active, subsequent bits are outputted on the clock edges
> when clock transitions from idle level to active level. Data is
