Re: [PATCH 1/2] mtd: spi-nor: Make SFDP-based 4B_OPCODE support detection works correctly

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Fri, 19 Oct 2018 09:50:31 +0200
Cyrille Pitchen - M19942 <cyrille.pitchen@microchip.com> wrote:

> Hi Boris,
> 
> looks good, just a small remark below:
> 
> Le 17/10/2018 à 16:44, Boris Brezillon a écrit :
> > Some flash_info entries have the SPI_NOR_4B_OPCODES to let the core
> > know that the flash supports 4B opcode. While this solution works fine
> > for id-based caps detection, it doesn't work that well when relying on
> > SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so that
> > spi_nor_parse_bfpt() can add it when the BFPT_DWORD1_ADDRESS_BYTES
> > field is set to BFPT_DWORD1_ADDRESS_BYTES_4_ONLY.
> > 
> > Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++---
> >  include/linux/mtd/spi-nor.h   |  1 +
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index 9407ca5f9443..85e57e9ea1b5 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -2643,6 +2643,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >  		break;
> >  
> >  	case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
> > +		nor->flags |= SNOR_F_4B_OPCODES;  
> 
> if spi_nor_parse_sdfp() fails, there is a kind of roll-back operation done
> in spi_nor_init_params() to set the struct spi_nor *nor back to its previous
> state.
> 
> 		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> 			nor->addr_width = 0;
> 			nor->mtd.erasesize = 0;
> 		} else {
> 			[...]
> 
> maybe "nor->flags &= ~SNOR_F_4B_OPCODES;" should be added there.

Actually, it should be

	if (!(info->flags & SPI_NOR_4B_OPCODES))
		nor->flags &= ~SNOR_F_4B_OPCODES;

but yes, this is missing. I'll fix that in v3.

> If this roll-back block grows too much, maybe we could introduce a
> void spi_nor_roll_back_sfdp(struct spi_nor *nor) function.
> Also it would make the roll back operation more explicit.

I'm wondering why we revert everything when a single bit is bit
reported as inconsistent in the SFDP table? I mean, it's not unusual
for NOR vendors to make mistake, and we should probably allow
vendor/chip specific code to fix the SFDP table at runtime instead of
discarding all the useful information we might have extracted.

Note that we recently introduced such a ->fixup() hook for the ONFI
param page in the raw NAND framework [1]. 

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/nand/raw?h=v4.19-rc8&id=00ce4e039ad5bded462931606c3063ff691964b7