Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

On 3/15/22 08:08, Vignesh Raghavendra wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi,

Hi,

> 
> On 11/03/22 1:31 pm, Tudor Ambarus wrote:
>> 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 is a bad design decision because
>> it 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 can swap the bytes back at
>> runtime, addressing the flash's endiannesses requirements.
>> If the controllers are not capable of swapping the bytes, the protocol is
>> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the swapping
>> of the bytes is always done regardless if it's a data or register access,
>> so that we comply with the JESD216 requirements: "Byte order of 16-bit
>> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
>>
> 
> Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is

no worries

> quite restrictive IMO.
> 
> AFAIK, SFDP standard does not dictate how data should be stored in flash
> or how SW should interpret after reading data from flash. It merely
> indicates endian-ness compared to 1-1-1 mode.
> 
> So, its up to various system SWs like bootloader/Linux to work according
> to pre-aligned layout as there is no rule that data needs to be stored
> in byte order.
> 
> We have two types of controllers:
> 
> 1. SPI controllers supporting swapping endian-ness on the fly:
> -> For such flashes, better choice is to have SWAP option always
> enabled. So that data written in 8D-8D-8D mode can be read correctly in
> 1-1-1 mode and vice-versa.
> ( I am assuming SWAP option of controller is only effective in 8D-8D-8D
> mode and is NOP in 1-1-1 or other modes)
> 
> But, its possible that "ROM" or other non-upgradable SWs may choose not
> make to use of this SWAP option of HW to keep things simple in which
> case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
> don't always have knowledge of flash and cannot be forced to have a
> constraint to enable byte swap on read.
> 
> So, IMO, its best left to system integrators to specify whether or not
> SWAP option needs to be enabled (perhaps via DT as its per flash
> specific property?)

we can't use DT for configuration, maybe a Kconfig instead. Are there any
other options?

> 
> 2.  SPI controllers don't support endian-ness SWAP on the fly:
> It is still possible to reliably read and write data as long as its
> written and read back in same mode.
> 
> De-rating speeds because of absence of this support would mean reduction
> of speed by **16 times** (maybe even higher as 8D mode tends to support
> higher bus freqs). Swapping bytes in Linux before writing or after
> reading is not an option either as it negatively impacts performance.
> 
> Asking ROM/bootloaders to swap bytes based on SFDP indication is
> restrictive too as it involves boot time penalty and most systems with
> OSPI flashes are using them to achieve super fast boot times.
> 
> One more case to consider is flashes that dont have SFDP table to
> indicate byte order but follow Macronix's convention. In such cases, its
> better for SPI NOR layer to be as dumb as possible and not really do any
> byte swapping, leaving it up to user space to handle/interpret data
> appropriately.
> 
> Also, Macronix is probably in violation of xSPI spec (JESD251A 6.9.5.2
> 8D-8D-8D Profile 1.0) where diagrams clearly show data output should be
> D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
> providing flash agnostic way of switching to 8D mode and reading data in
> 8D mode) would not care about SFDP bit indicating byteorder and its up
> to flasher programs to take care of the same

This is a bit in contradiction, because if the ROMs follow xSPI, thus little
endian byte order, they should swap the bytes.

> 
> IMO, kernel device drivers should just provide access to underlying HW
> and not have too much intelligence to interpret data/take decisions
> 
> So, simpler constraint to put is:
> Flasher programs should program data in the same mode in which
> ROM/bootloder/Linux is expected to read the data on that system.

No, this constraint doesn't cover all possible cases: take a 1-1-1 ROMcode,
8D-8D-8D for other bootloaders and kernel. You need to dynamically change modes
in the flasher program in order to address this use case, which is a no go.
 
> 
> For Macronix like flashes, if one has a ROM/bootloader that only
> supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, then
> please generate a byte-swapped image offline and flash it. Don't impose

we can't do that, see the example from above.

> penalty on systems that do best to handle this messy situation.
> I see this as the only option with least performance penalty.
> 

I take from this that we should let the byte swap be user-configurable,
thus a Kconfig. Which I'm not against it, but it will give users
headaches to sync all the software components. Making such a decision
implies that users know SPI NOR internal details, which also is a bit
stretched. Let's sync so that we move forward with this. Opinions? 

Cheers,
ta