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

[Haixu Cui <quic_haixcui@quicinc.com>]

Hello Harald,

On 12/22/2023 10:32 PM, Harald Mommer wrote:
> 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."

Thank you very much for your detailed explanations.

There are such strange usage scenarios that frame size exceeds 32 bits, 
even 4096 bits. For these strange cases, It's impossible to use only 32 
bits bits_per_word_mask to enumerate all the conditions. Seems that a 
similar situation exists in Linux spi driver also.

Assume a case where the spi controller supports the frame size to be any 
value from 16 to 64, 32 bits mask can only cover the 16 ~ 32 conditions.

Even so, it's not a good idea to expand the bits_per_word_mask, 32 bits 
can cover most of the cases, except for some situations that are not 
commonly used. Besides, it's hard to determine the bit width to cover 
all the cases, and expanding will make the logic more complicated. To 
summarize, I think the following statement is probably proper:

"\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 
\field{bits_per_word_mask} is 0, there is no limitation for bits_per_word."

Do you think it is acceptable and appropriate?

Much appreciated, again.

Thanks
Haixu Cui

> Regards
> Harald