[PATCH v7 2/7] spi: spi-mem: Allow specifying the byte order in DTR mode

[Tudor Ambarus]

On 1/5/24 12:48, Michael Walle wrote:
> Hi,
> 
>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit
>> boundary when configured in Octal DTR mode. The byte order of
>> 16-bit words is swapped when read or written in Octal Double
>> Transfer Rate (DTR) mode compared to Single Transfer Rate (STR)
>> modes. 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 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. So we must swap the bytes
>> back to have the same byte order as in STR modes. Fortunately there
>> are controllers that could swap the bytes back at runtime,
>> addressing the flash's endiannesses requirements. Provide a way for
>> the upper layers to specify the byte order in Octal DTR mode.
>>
>> Merge Tudor's patch and add modifications for suiting newer version
>> of Linux kernel.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus at linaro.org>
>> Signed-off-by: JaimeLiao <jaimeliao at mxic.com.tw>
>> ---
>>  drivers/spi/spi-mem.c       | 4 ++++
>>  include/linux/spi/spi-mem.h | 6 ++++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>> index edd7430d4c05..9c03b5617fff 100644
>> --- a/drivers/spi/spi-mem.c
>> +++ b/drivers/spi/spi-mem.c
>> @@ -172,6 +172,10 @@ bool spi_mem_default_supports_op(struct spi_mem
>> *mem,
>>          if (!spi_mem_controller_is_capable(ctlr, dtr))
>>              return false;
>>
>> +        if (op->data.dtr_swab16 &&
>> +            !(spi_mem_controller_is_capable(ctlr, dtr_swab16)))
> 
> unnecessary parentheses.
> 
>> +            return false;
>> +
>>          if (op->cmd.nbytes != 2)
>>              return false;
>>      } else {
>> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
>> index 6b0a7dc48a4b..d4935c5c3c7a 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_swab16: whether the byte order of 16-bit words is
>> swapped when read
>> + *             or written in Octal DTR mode compared to STR mode.
> 
> maybe just swap16? I'm not sure. Doesn't really apply to DTR, because
> it is not a thing for 4bit DTR for example. Just for 8d8d8d and "faster"
> because there you transmit more than one byte in one clock cycle.

renaming to swap16 would be good, yes.

> 
> "whether bytes within a 16-bit word should be swapped. Some flashes will
> swap the data in 8D mode. In that case, this should be set to true
> to instruct the controller to swap the data back on the fly.
> 
>>   * @data.ecc: whether error correction is required or not
>>   * @data.dir: direction of the transfer
>>   * @data.nbytes: number of data bytes to send/receive. Can be zero if
>> the
>> @@ -123,6 +125,7 @@ struct spi_mem_op {
>>      struct {
>>          u8 buswidth;
>>          u8 dtr : 1;
>> +        u8 dtr_swab16 : 1;
>>          u8 ecc : 1;
>>          u8 __pad : 6;
> 
> __pad : 5;
> 
> Does anyone know if this is really necessary?
> 

it's to mitigate a gcc-{12, 13} bug, see
71c8f9cf2623d0db79665f876b95afcdd8214aec

No idea if the bug was fixed in the meantime.

>>          enum spi_mem_data_dir dir;
>> @@ -294,10 +297,13 @@ struct spi_controller_mem_ops {
>>  /**
>>   * struct spi_controller_mem_caps - SPI memory controller capabilities
>>   * @dtr: Supports DTR operations
>> + * @dtr_swab16: Supports swapping bytes on a 16 bit boundary when
>> configured in
>> + *        Octal DTR
> 
> I guess the same comment as above applies, doesn't really have something
> to do with dtr, but only 8d.

OK.
Thanks,
ta
> 
> -michael
> 
>>   * @ecc: Supports operations with error correction
>>   */
>>  struct spi_controller_mem_caps {
>>      bool dtr;
>> +    bool dtr_swab16;
>>      bool ecc;
>>  };