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

[Harald Mommer <harald.mommer@opensynergy.com>]

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 0xFFFFFFFF 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 0xFFFFFFFF 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 0xFFFFFFFF 
> 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



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