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

[Pratyush Yadav]

On 30/09/20 09:57AM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> 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.

FWIW, I took a quick look at the RFC patches and I agree that patches 
1/3 and 3/3 are good and I'll add them to my series. I don't like 2/3 
mainly because of the same concerns that Vignesh has: wearing down the 
NV bits. Also IMO we should avoid touching NV bits as much as we can 
because I had used some on the Cypress flash during development, and I 
ended up "bricking" the flash a few times (for example when I used the 
wrong dummy cycles by mistake or the controller driver did something 
wrong). Since I knew exactly which registers I was touching I could 
recover it but the process is quite painful.

> 
> > 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.

Ok, I'll include them next time. I tested this series on the CQSPI and 
TI QSPI controllers on MT25Q, S25FL for regressions, and on MT35 and S28 
for Octal DTR.

-- 
Regards,
Pratyush Yadav
Texas Instruments India