Re: [RFC PATCH 2/3] mtd: spi-nor: sfdp: fix spi_nor_read_sfdp()

[Pratyush Yadav <p.yadav@ti.com>]

On 16/03/21 12:15PM, Michael Walle wrote:
> Am 2021-03-16 12:04, schrieb Pratyush Yadav:
> > On 12/03/21 08:05PM, Michael Walle wrote:
> > > If spi_nor_read_sfdp() is used after probe, we have to set read_proto
> > > and the read dirmap.
> > > 
> > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > ---
> > >  drivers/mtd/spi-nor/sfdp.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> > > index b1814afefc33..47634ec9b899 100644
> > > --- a/drivers/mtd/spi-nor/sfdp.c
> > > +++ b/drivers/mtd/spi-nor/sfdp.c
> > > @@ -179,19 +179,27 @@ static int spi_nor_read_sfdp(struct spi_nor
> > > *nor, u32 addr,
> > >  			     size_t len, void *buf)
> > >  {
> > >  	u8 addr_width, read_opcode, read_dummy;
> > > +	struct spi_mem_dirmap_desc *rdesc;
> > > +	enum spi_nor_protocol read_proto;
> > >  	int ret;
> > > 
> > >  	read_opcode = nor->read_opcode;
> > > +	read_proto = nor->read_proto;
> > > +	rdesc = nor->dirmap.rdesc;
> > >  	addr_width = nor->addr_width;
> > >  	read_dummy = nor->read_dummy;
> > > 
> > >  	nor->read_opcode = SPINOR_OP_RDSFDP;
> > > +	nor->read_proto = SNOR_PROTO_1_1_1;
> > > +	nor->dirmap.rdesc = NULL;
> > >  	nor->addr_width = 3;
> > >  	nor->read_dummy = 8;
> > 
> > NACK. You can't assume the device is still in 1S-1S-1S mode after probe.
> > For example, the s28hs512t flash is switched to 8D-8D-8D mode by the
> > time the probe finishes so this would be an invalid command. Same for
> > any flash that goes into a stateful mode.
> 
> I see.
> 
> > And you can't even keep using nor->read_proto to read SFDP because the
> > Read SFDP command might not be supported in all modes. xSPI spec
> > (JESD251) says that the Read SFDP command is optional in 8D-8D-8D mode.
> > 
> > I think the best approach for this would be to cache the entire SFDP
> > table at parse time. This obviously comes with a memory overhead but I
> > don't think it would be too big. For example, the sfdp table on
> > s28hs512t is 491 bytes and it has 6 tables. Anyway, if the memory usage
> > is too much of a problem we can put the feature behind a config.
> 
> I don't like to have it a config option, because then, if you really
> need it, i.e. some user has an unknown flash, it might not be there.

Right. Then let's hope people don't mind us using up an extra half 
kilobyte or so.

> 
> The next question would be, should I leave the current parsing code
> as is or should I also change that to use the sftp data cache. I'd
> prefer to leave it as is.

For this series its fine if you leave it as is. But eventually it would 
be a good idea to convert all SFDP parsers to use the cache to reduce 
the number of SFDP reads and potentially speed up flash initialization a 
bit.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.