Re: [PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't specify it

[Michael Walle]

Am 2022-03-07 10:23, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
> On 3/7/22 09:12, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> > the content is safe
> >
> > On 3/4/22 20:51, Michael Walle wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you 
> > > know the content is safe
> > >
> > > While the first version of JESD216 specify the opcode for 4 bit I/O
> > > accesses, it lacks information on how to actually enable this mode.
> > >
> > > For now, the one set in spi_nor_init_default_params() will be used.
> > > But this one is likely wrong for some flashes, in particular the
> > > Macronix MX25L12835F. Thus we need to clear the enable method when
> > > parsing the SFDP. Flashes with such an SFDP revision will have to use 
> > > a
> > > flash (and SFDP revision) specific fixup.
> > >
> > > This might break quad I/O for some flashes which relied on the
> > > spi_nor_sr2_bit1_quad_enable() that was formerly set. If your bisect
> > > turns up this commit, you'll probably have to set the proper
> > > quad_enable method in a post_bfpt() fixup for your flash.
> >
> > Right, I meant adding a paragraph such as the one from above.
> >
> > > Signed-off-by: Michael Walle <michael@xxxxxxxx>
> > > Tested-by: Heiko Thiery <heiko.thiery@xxxxxxxxx>
> > > ---
> > > changes since RFC:
> > >  - reworded commit message
> > >  - added comment about post_bfpt hook
> > >
> > > Tudor, I'm not sure what you meant with
> > >   Maybe you can update the commit message and explain why would some
> > >   flashes fail to enable quad mode, similar to what I did.
> > >
> > > It doesn't work because the wrong method is chosen? ;)
> > >
> > >  drivers/mtd/spi-nor/sfdp.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> > > index a5211543d30d..6bba9b601846 100644
> > > --- a/drivers/mtd/spi-nor/sfdp.c
> > > +++ b/drivers/mtd/spi-nor/sfdp.c
> > > @@ -549,6 +549,16 @@ static int spi_nor_parse_bfpt(struct spi_nor 
> > > *nor,
> > >         map->uniform_erase_type = map->uniform_region.offset &
> > >                                   SNOR_ERASE_TYPE_MASK;
> > >
> > > +       /*
> > > +        * The first JESD216 revision doesn't specify a method to 
> > > enable
> > > +        * quad mode. spi_nor_init_default_params() will set a legacy
> > > +        * default method to enable quad mode. We have to disable it
> > > +        * again.
> > > +        * Flashes with this JESD216 revision need to set the 
> > > quad_enable
> > > +        * method in their post_bfpt() fixup if they want to use quad 
> > > I/O.
> > > +        */
> >
> > Great. Looks good to me. I'll change the subject to "mtd: spi-nor: 
> > sfdp:"
> > when applying.
>
> As we talked on the meeting, we can instead move the default quad mode 
> init
> to the deprecated way of initializing the params, or/and to where 
> SKIP_SFDP
> is used. This way you'll no longer need to clear it here.

Mh, I just had a look and I'm not sure it will work there,
because in the deprecated way, the SFDP is still parsed and
thus we might still have the wrong enable method for flashes
which don't have PARSE_SFDP set.

-michael