Re: [PATCH v13 00/15] mtd: spi-nor: add xSPI Octal DTR support

[Tudor.Ambarus]

On 9/16/20 3:44 PM, Pratyush Yadav wrote:
> Hi,

Hello,

> 
> This series adds support for Octal DTR flashes in the SPI NOR framework,
> and then adds hooks for the Cypress Semper and Micron Xcella flashes to
> allow running them in Octal DTR mode. This series assumes that the flash
> is handed to the kernel in Legacy SPI mode.
> 

I like this series. There are some comments that can be addressed, no big
deal though.

I think that we shouldn't let the door open for users with flashes that
enter in a X-X-X mode in a non-volatile way. Think of two flashes that have
the same X-X-X mode enable sequence, but in which only the EN bit differs:
for one the EN bit is volatile and for the other it is non-volatile. Users
of the later flash that try to enable the X-X-X mode (using our code) will
end up with the flash in a mode from which they can't recover. Thus my advice
is to consider by default all the flashes, as X-X-X mode non-volatile flashes,
and to not let them use the X-X-X mode enable methods. Flashes that can enter
X-X-X modes in a volatile way, should discover this capability by parsing the
optional SFDP SCCR Map. Those that don't define this table, should pass this
capability as a flash_info flag when declaring the flash. With these, users
should be conscious about the V or NV modes, and the risk to end up with
flashes for which there is no software to recover is diminished. This is what
I tried in RFC 1/3 and RFC 3/3. I think those 2 patches should be part of
this series. 14/15 and 15/15 should be updated accordingly. RFC 2/3 has room
for discussions because it provides access system-wise, while ideally would be
to do it at flash granularity. I'll wait for your feedback on those.

> Tested on TI J721E EVM with 1-bit ECC on the Cypress flash.

As a tip, when introducing some big changes to the core, would be nice to be
assured that things that worked previously are still working now.
An erase-write-read-back test in Quad SPI would suffice. Probably you have
already did that, but I haven't seen it mentioned.

Cheers,
ta