Re: [PATCH v4 1/3] mtd: spi-nor: Add the SNOR_F_4B_OPCODES flag

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

On Wed, 5 Dec 2018 15:48:46 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 12/05/2018 05:19 PM, Boris Brezillon wrote:
> >>> @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> >>>  	if (info->flags & SPI_NOR_NO_FR)
> >>>  		params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> >>>  
> >>> +	if (info->flags & SPI_NOR_4B_OPCODES ||
> >>> +	    (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
> >>> +		nor->flags |= SNOR_F_4B_OPCODES;
> >>> +    
> >> you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I
> >> suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)"
> >> block.  
> > Shouldn't we override this value anyway? I mean, I thought flash_info
> > flags had precedence on the SFDP ones. Also, just because the flash is  
> 
> I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in
> the code: we are overwriting platform ID if we find a different ID in sfdp, we
> choose addr_width from SFDP even if set in info->addr_width, and we are
> overwriting all the settings based on flash_info when sfdp parsing succeeds in
> spi_nor_init_params().

Given all the "broken SFDP" problems we had, I'm not sure this was the
right decision, but that's another topic.

For this specific one, I'd really prefer to keep this code as is. Note
that the "JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M"
is later moved to a post SFDP fixup hook in my rework, which means we'll
anyway override the decision taken by the SFDP parsing.

> 
> > smaller than 16MB, doesn't mean it does not support 4B opcodes. We
> > probably won't use the 4B opcodes in that case, but still.
> >   
> 
> I agree that manufacturers have a sense of humor and this might be possible. But
> there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help
> here too.

Except there's nothing to fix in this case, we just won't use 4B
opcodes if we don't need to, that's all.