[PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't specify it
Pratyush Yadav
p.yadav at ti.com
Wed Mar 16 12:07:34 PDT 2022
On 15/03/22 07:47AM, Tudor.Ambarus at microchip.com wrote:
> On 3/15/22 09:24, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Am 2022-03-15 06:55, schrieb Tudor.Ambarus at microchip.com:
> >> On 3/14/22 22:42, Michael Walle wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >>> the content is safe
> >>>
> >>> Am 2022-03-09 05:49, schrieb Tudor.Ambarus at microchip.com:
> >>>> On 3/7/22 20:56, Michael Walle wrote:
> >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>>>> know
> >>>>> the content is safe
> >>>>>
> >>>>> Am 2022-03-07 10:23, schrieb Tudor.Ambarus at microchip.com:
> >>>>>> On 3/7/22 09:12, Tudor.Ambarus at microchip.com 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 at walle.cc>
> >>>>>>>> Tested-by: Heiko Thiery <heiko.thiery at gmail.com>
> >>>>>>>> ---
> >>>>>>>> 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.
> >>>>
> >>>> Moving the default quad_enable method to
> >>>> spi_nor_no_sfdp_init_params(),
> >>>> thus also for spi_nor_init_params_deprecated() because it calls
> >>>> spi_nor_no_sfdp_init_params(), will not change the behavior for the
> >>>> deprecated way of initializing the params, isn't it?
> >>>
> >>> What do you mean? The behavior is not changed and the bug is not
> >>> fixed for the flashes which use the deprecated way. It will get
> >>> overwritten by the spi_nor_parse_sfdp call in
> >>> spi_nor_sfdp_init_params_deprecated().
> >>
> >> right, it will not change the logic for the deprecated way of
> >> initializing
> >> the params.
> >>
> >>>
> >>>> A more reason
> >>>> to use PARSE_SFDP/SKIP_SFDP, we'll get rid of the deprecated params
> >>>> init at some point.
> >>>>
> >>>> No new fixes for spi_nor_init_params_deprecated().
> >>>
> >>> Hm, so we deliberately won't fix known bugs there? I'm not sure
> >>> I'd agree here. Esp. because it is hard to debug and might even
> >>> depend on non-volatile state of the flash.
> >>>
> >>
> >> even more a reason to switch to the recommended way of initializing
> >> the flash. We'll get rid of the deprecated code anyway, no?
> >
> > I get your point. But I disagree with you on that point :) Features?
> > sure we can say this shouldn't go to any deprectated code flow and
> > might poke users to post a patch. But bug fixes? I don't think
> > we should hold these back.
>
> Why to fix something that never worked in a deprecated code path? It's
> equivalent to adding new support, no?
I have not followed this discussion very closely but this argument makes
sense to me. If something never worked in the deprecated path then we
have don't have to fix it.
>
> > Correct me if I'm wrong, but we can get rid of the deprecated way
> > only if all the flashes are converted to PARSE_SFDP or SKIP_SFDP,
> > right? And I don't see this happening anytime soon.
>
> Right. I vote to don't queue any new patches for deprecated code paths,
> new support or fixes. But I'm not completely against it, I don't see
> the point, that's all. Let's sync with Pratyush and Vignesh too.
I agree with no new features for deprecated path. But I think we should
still take in bug fixes.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
More information about the linux-mtd
mailing list