by Tudor Ambarus

[permalink] [raw]

Subject: Re: [PATCH 1/4] spi: spi-mem: Allow specifying the byte order in DTR mode

On 3/2/22 12:02, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Tudor,

Hi, Pratyush,

>
> I'm reviewing the code here. I still have not thought through the
> discussion about Kconfig option yet.
>
> On 18/02/22 04:58PM, Tudor Ambarus wrote:
>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
>> when configured in DTR mode. The byte order of 16-bit words is swapped
>
> s/DTR mode/ Octal DTR mode/
>
> I don't think this would apply to a 4D-4D-4D flash since it would only
> transmit one byte per clock cycle.

From what I see, flashes that claim "QPI DTR support" they actually support
4S-4D-4D. JESD251-1 talks about 4S-4D-4D too. So data is latched on both rising
and falling edges of the clock. But I'm ok with your proposal because we don't
have any proof if there are any QPI DTR flashes that swap bytes in DTR.

>
>> when read or written in Double Transfer Rate (DTR) mode compared to
>> Single Transfer Rate (STR) mode. If one writes D0 D1 D2 D3 bytes using
>> 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will read back
>> D1 D0 D3 D2. Swapping the bytes is a bad design decision because this may
>> introduce some endianness problems. It can affect the boot sequence if the
>> entire boot sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode.
>> Fortunately there are controllers that can swap back the bytes at runtime,
>> fixing the endiannesses. Provide a way for the upper layers to specify the
>> byte order in DTR mode.
>>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> include/linux/spi/spi-mem.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
>> index 85e2ff7b840d..e1878417420c 100644
>> --- a/include/linux/spi/spi-mem.h
>> +++ b/include/linux/spi/spi-mem.h
>> @@ -89,6 +89,8 @@ enum spi_mem_data_dir {
>> * @dummy.dtr: whether the dummy bytes should be sent in DTR mode or not
>> * @data.buswidth: number of IO lanes used to send/receive the data
>> * @data.dtr: whether the data should be sent in DTR mode or not
>> + * @data.dtr_bswap16: whether the byte order of 16-bit words is swapped when
>> + * read or written in DTR mode compared to STR mode.
>> * @data.dir: direction of the transfer
>> * @data.nbytes: number of data bytes to send/receive. Can be zero if the
>> * operation does not involve transferring data
>> @@ -119,6 +121,7 @@ struct spi_mem_op {
>> struct {
>> u8 buswidth;
>> u8 dtr : 1;
>> + u8 dtr_bswap16 : 1;

but I would keep this name here as it is, without prepending octal.

>
> You also need to add this capability to spi_controller_mem_caps and
> update spi_mem_default_supports_op() to check for it.

sure, will do.

Thanks!
ta
>
>> enum spi_mem_data_dir dir;
>> unsigned int nbytes;
>> union {
>> --
>> 2.25.1
>>
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.

2022-03-11 22:16:36

by Pratyush Yadav

[permalink] [raw]

Subject: Re: [PATCH 1/4] spi: spi-mem: Allow specifying the byte order in DTR mode

On 10/03/22 05:31AM, [email protected] wrote:
> On 3/2/22 12:02, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Hi Tudor,
>
> Hi, Pratyush,
>
> >
> > I'm reviewing the code here. I still have not thought through the
> > discussion about Kconfig option yet.
> >
> > On 18/02/22 04:58PM, Tudor Ambarus wrote:
> >> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
> >> when configured in DTR mode. The byte order of 16-bit words is swapped
> >
> > s/DTR mode/ Octal DTR mode/
> >
> > I don't think this would apply to a 4D-4D-4D flash since it would only
> > transmit one byte per clock cycle.
>
> From what I see, flashes that claim "QPI DTR support" they actually support
> 4S-4D-4D. JESD251-1 talks about 4S-4D-4D too. So data is latched on both rising
> and falling edges of the clock. But I'm ok with your proposal because we don't
> have any proof if there are any QPI DTR flashes that swap bytes in DTR.

I think this problem fundamentally applies to Octal DTR and above (if
there is ever 16-line DTR (hexadecimal DTR?) in the future). In your 4D
data phase, you can only send _one_ byte per cycle. So the byte order
inter-cycle does not matter as it does in 8D mode. Similarly, for a
16-line STR this would also apply, since that has 2 bytes per cycle. For
a 16-line DTR there are now 4 bytes per cycle and so on.

And the BFPT bit that you use to enable this swap also says "Byte order
in 8D-8D-8D mode". So I really don't think it makes sense for QPI DTR.

>
> >
> >> when read or written in Double Transfer Rate (DTR) mode compared to
> >> Single Transfer Rate (STR) mode. If one writes D0 D1 D2 D3 bytes using
> >> 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will read back
> >> D1 D0 D3 D2. Swapping the bytes is a bad design decision because this may
> >> introduce some endianness problems. It can affect the boot sequence if the
> >> entire boot sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode.
> >> Fortunately there are controllers that can swap back the bytes at runtime,
> >> fixing the endiannesses. Provide a way for the upper layers to specify the
> >> byte order in DTR mode.
> >>
> >> Signed-off-by: Tudor Ambarus <[email protected]>
> >> ---
> >> include/linux/spi/spi-mem.h | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> >> index 85e2ff7b840d..e1878417420c 100644
> >> --- a/include/linux/spi/spi-mem.h
> >> +++ b/include/linux/spi/spi-mem.h
> >> @@ -89,6 +89,8 @@ enum spi_mem_data_dir {
> >> * @dummy.dtr: whether the dummy bytes should be sent in DTR mode or not
> >> * @data.buswidth: number of IO lanes used to send/receive the data
> >> * @data.dtr: whether the data should be sent in DTR mode or not
> >> + * @data.dtr_bswap16: whether the byte order of 16-bit words is swapped when
> >> + * read or written in DTR mode compared to STR mode.
> >> * @data.dir: direction of the transfer
> >> * @data.nbytes: number of data bytes to send/receive. Can be zero if the
> >> * operation does not involve transferring data
> >> @@ -119,6 +121,7 @@ struct spi_mem_op {
> >> struct {
> >> u8 buswidth;
> >> u8 dtr : 1;
> >> + u8 dtr_bswap16 : 1;
>
> but I would keep this name here as it is, without prepending octal.

I won't nitpick much on the member name as long as the comment
describing its role is clear enough.

>
> >
> > You also need to add this capability to spi_controller_mem_caps and
> > update spi_mem_default_supports_op() to check for it.
>
> sure, will do.
>
> Thanks!
> ta
> >
> >> enum spi_mem_data_dir dir;
> >> unsigned int nbytes;
> >> union {
> >> --
> >> 2.25.1
> >>
> >
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.