Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Tudor.Ambarus]

On 10/1/20 9:34 AM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi,
> 
> On 01/10/20 01:56AM, Bert Vermeulen wrote:
>> Flash chips that announce BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 capability
>> get an addr_width of 3. This breaks when the flash chip is actually
>> larger than 16MB, since that requires a 4-byte address. The MX25L25635F
>> does exactly this, breaking anything over 16MB.
>>
>> spi-nor only enables 4-byte opcodes or 4-byte address mode if addr_width
>> is 4, so no 4-byte mode is ever enabled. The > 16MB check in
>> spi_nor_set_addr_width() only works if addr_width wasn't already set
>> by the SFDP, which it was.
>>
>> It could be fixed in a post_bfpt fixup for the MX25L25635F, but setting
>> addr_width to 4 when BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 is found fixes the
>> problem for all such cases.
> 
> JESD216D.01 says: "01b: 3- or 4-Byte addressing (e.g., defaults to
> 3-Byte mode; enters 4-Byte mode on command)"
> 
> So using an address width of 4 here is not necessarily the right thing
> to do. This change would break SMPT parsing for all flashes that use
> 3-byte addressing by default because SMPT parsing can involve register
> reads/writes. One such device is the Cypress S28HS flash. In fact, this
> was what prompted me to write the patch [0].
> 
> Before that patch, how did MX25L25635F decide to use 4-byte addressing?
> Coming out of BFPT parsing addr_width would still be 0. My guess is that
> it would go into spi_nor_set_addr_width() with addr_width still 0 and
> then the check for (nor->mtd.size > 0x1000000) would set it to 4. Do I
> guess correctly?
> 
> In that case maybe we can do a better job of deciding what gets priority
> in the if-else chain. For example, giving addr_width from nor->info
> precedence over the one configured by SFDP can solve this problem. Then
> all you have to do is set the addr_width in the info struct, which is
> certainly easier than adding a fixup hook. There may be a more elegant
> solution to this but I haven't given it much thought.
> 

I do agree with Pratyush that we should follow the SFDP standard
and don't change it. So the change is not acceptable. The standard
is the "law". If manufacturers mess with it, and fill bits without
respecting the standard, then we have to fix it via the post sfdp
fixup hook. SFDP is above nor->info flags, don't change the order
of the ifs.

Cheers,
ta