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

[<Tudor.Ambarus@microchip.com>]

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 <tudor.ambarus@microchip.com>
>> ---
>>  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.