Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Pratyush Yadav <me@xxxxxxxxxxxxxxxxx>]

On 21/04/20 11:30AM, Yicong Yang wrote:
> As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
> bit[23:22] and 1111b of bit[19:16] represent variable
> {address length, read latency}, which means "the {address length,
> read latency} last set in memory device and this same value is used in the
> configuration dectection command". Currently we use address length
> and dummy byte of struct spi_nor in such conditions. But the value
> are 0 as they are not set at the time, which will lead to
> wrong perform of the dectection command.
> 
> As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
> with 3-byte address and 8 dummy cycles, use the same values in
> variable situations to perform correct dectection command.
> 
> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>

FWIW, I posted a fix for the address width problem here [0], though in a 
slightly different way. I'll re-roll the series soon, since it is based 
on the code structure before the split.

> ---
>  drivers/mtd/spi-nor/sfdp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index f6038d3..27a8faa 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
>  		return 4;
>  	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>  	default:
> -		return nor->addr_width;
> +		return 3;

If in the future someone implements SFDP parsing in 8D-8D-8D mode, they 
would want to use an address length of 4. Similarly, if someone has a 
use-case where they have to configure their flash to a 4-byte addressing 
mode in a non-volatile way, they would set nor->addr_width in a 
flash-specific hook (like the post-BFPT hook) and would expect that 
width to be used here as well.

So I think instead of setting it in stone like this, we should instead 
set the default nor->addr_width to 3 if it is configurable, and then let 
flashes fix it up if they need to. This is what my patch [0] does.

>  	}
>  }
> 
> @@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
> 
>  	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
> -		return nor->read_dummy;
> +		return 8;

Same comment here. Set nor->read_dummy to a sane default (== 8) and then 
let flash-specific hooks change it if they need to. This is tricky 
though, since BFPT doesn't tell us the dummy cycles needed. Still, I 
think if we set it in say spi_nor_parse_smpt(), it would be a bit more 
flexible.

>  	return read_dummy;
>  }

[0] https://lore.kernel.org/linux-mtd/20200313154645.29293-6-p.yadav@xxxxxx/

-- 
Regards,
Pratyush Yadav

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