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

[Pratyush Yadav]

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.

So from my side, NACK!

> 
> Signed-off-by: Bert Vermeulen <bert@xxxxxxxx>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index e2a43d39eb5f..6fedc425bcf7 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -456,10 +456,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  	/* Number of address bytes. */
>  	switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
>  	case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
> -	case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>  		nor->addr_width = 3;
>  		break;
>  
> +	case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>  	case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
>  		nor->addr_width = 4;
>  		break;

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9acd7fa80be6ee14aecdc54429f2a48e56224e8

-- 
Regards,
Pratyush Yadav
Texas Instruments India