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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

On Wed, 5 Dec 2018 16:26:06 +0000
<Tudor.Ambarus@xxxxxxxxxxxxx> wrote:

> On 12/05/2018 06:00 PM, Boris Brezillon wrote:
> > On Wed, 5 Dec 2018 15:48:46 +0000
> > <Tudor.Ambarus@xxxxxxxxxxxxx> 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.  
> 
> you'll have an extra byte of address that has a tiny impact on performance on
> small requests. I see it as a fix, we should do what's best to do. anyway ...

No, see the checks that are done in this patch: to use 4B_CODES the
flag should be set and the NOR should be larger than 16MB. The flag
does not mean "use 4B opcodes", it meas "4B opcodes are supported".

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/