Hello Haixu,
On 22.12.23 03:28, Haixu Cui wrote:
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
Interesting. I know that I had already a comment on this previously. And
also now. But sometimes I'm wrong. And here probably in both cases but
at least with my last comment.
* @bits_per_word_mask: A mask indicating which values of bits_per_word are
* supported by the driver. Bit n indicates that a bits_per_word n+1 is
* suported. If set, the SPI core will reject any transfer with an
* unsupported bits_per_word. If not set, this value is simply ignored,
* and it's up to the individual driver to perform any validation.
Tried to find out why this is so in Linux. The function
__spi_validate_bits_per_word() was introduced by commit 63ab645f4d8b.
The member variable bits_per_word_mask in struct spi_master was already
introduced by 543bb255a198. The new variable was introduced in a way so
that if not set the behavior would not change for existing SPI drivers
not (yet) setting the field. At least I deduce that looking into the
commits. Made a lot of sense to introduce bits_per_word_mask in commit
543bb255a198 in Linux this way to make a change without causing other
changes everywhere. The virtio SPI specification is new, no need to be
backwards compatible here so there is no need to have 0 there as "don't
check" value. So your proposal to make 0 the invalid value is a good
one. So I thought.
On the other hand: The function __spi_validate_bits_per_word() does not
check for anything if bits_per_word_mask is 0. With 0 it does not check
for bits_per_word > 32 and it does not check for bits_per_word being a
power of 2. It never checks for bits_per_word > 0 but this is a
different issue.
1.) Now I'm looking into a data sheet from Freescale
https://www.manualslib.com/manual/1376296/Freescale-Semiconductor-Mk22fn256vdc12.html?page=1053
and see that the chip can use any frame sizes from 4 to 32. Means also
frame sizes which are not a power of 2. Nothing I've seen before but it
exists. So the value 0 is indeed needed to allow for those values.
The chip can only support 4 to 32 bits, so with the 32 bit restriction
in the V9 draft we would be fine.
2.)
https://community.nxp.com/t5/S32K/How-to-send-receive-64bit-or-longer-message-using-SPI/m-p/1476343#M15986
There are Bits/frame of 8, 10, 24, 32, 40, 64 mentioned in the example.
Not only that some of those are no power of 2 and cannot be represented
in this bit mask not having 0 but we have also 64 bit frame size. Means
restriction to 32 bits has also to fall to support something like this.
I looked further and found a data sheet from NXP in the internet which
should not be there. Maximum frame size for this chip is everything
between 8 bits and 4096-bits with some values in between excluded. As
bits_per_word is an u8 in Linux and in the draft spec something so
strange like this could not be supported by virtio spi nor in Linux.
=> The normal case for the majority of people is 8, 16 and maybe also 32
bit SPI.
=> The strange case is non power of 2 values which cannot be represented
in the mask not allowing 0. The V9 draft spec is perfect for this.
=> Another strange case is big frame sizes > 32 bit.
The sentence in draft V7 (on which I moaned, sorry for this) was most
probably the perfect one: "If not set, there is no limitation for
bits_per_word."
Regards
Harald
