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 08:25PM, Yicong Yang wrote:
> Hi Pratyush,
> 
> 
> On 2020/4/21 19:44, Pratyush Yadav wrote:
> > 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.
> 
> As I mentioned in the commit, *currently* we use 1S-1S-1S mode to read SFDP, which is also
> the last operation we did here. So I use the same address byte and dummy cycles here.
> I think we won't meet the exception you mentioned, as it will fail when parsing SFDP
> and won't come here.
> 
> I agree that currently we don't meet all the conditions. But your patch seems alike the way
> here. AS you set address width when parsing BFPT, but it'll not be changed before parsing SMPT
> here, we parse smpt once bfpt is parsed and don't do specific vendor operations, etc.
 
Even if we don't do it right now, I don't see the harm in making a 
future dev's life just a little bit easier by not forcing them to find 
this problem and fix this just the way I suggested. If you do it that 
way, you still end up using an address width of 3 so it makes no 
difference to you.

But this hypothetical situation aside, there is the problem of when 
someone wants to use a flash with the address width configured to 4 on 
boot. Once the BFPT is parsed, the post-BFPT hook runs. Here, if you are 
using a non-default address width, you'd check the flash configuration 
to see if 4-byte addressing is used, and update nor->addr_width 
accordingly. If you hard-code 3 there, then SMPT parsing would fail in 
such a situation. At $DAYJOB, we have a use-case like this where we use 
a flash configured in 4-byte addressing.

> >> @@ -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.
> 
> As I've mentioned above, it follows the behaviour in the current framework. What do you think of
> introduce new field in spi_nor to provide more flexibility:
> 
>     struct spi_nor {
>         ...
>         sfdp_addr_width;
>         sfdp_read_dummy;
>         ...
>     }
> 
> we can initialized it in the quite beginning(perhap in spi_nor_scan()) and let others to modify it.
> It will also provide sfdp access and flash access with different parameter if necessary.

I think you've misunderstood what the address width and dummy cycles are 
for. They are used for the configuration detection command, which is 
used to find out which configuration the flash is in, and then use the 
appropriate sector map for that configuration.

This configuration is unlikely to be in the SFDP table. AFAIK, SFDP 
table is static and won't change based on flash config. Instead, this 
configuration will likely be in a flash register. For example, on the 
Cypress S28 flash family, it lies in the register CFR3.

The latency cycles needed for accessing these registers are completely 
unrelated to the latency cycles needed for SFDP. As an example, on the 
S28 family, you need 8 latency cycles for reading SFDP, but need 3-6 
(configurable) cycles for reading a volatile register, like CFR3. So, 
using the SFDP address width and the SFDP latency cycles for these 
commands would be completely incorrect, and would only work by chance.

You should instead populate the dummy cycles for your flash in 
nor->read_dummy, and the address width in nor->addr_width. If these 
can't be properly detected by BFPT, please use the post-BFPT hook 
instead, which runs before SMPT (or any optional table for that matter) 
parsing.
 
> >>  	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/