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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

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 ...
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/