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

[Pratyush Yadav <p.yadav@xxxxxx>]

On 10/03/22 05:31AM, Tudor.Ambarus@xxxxxxxxxxxxx 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 <tudor.ambarus@xxxxxxxxxxxxx>
> >> ---
> >>  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.