Messages in this thread | | | From | <> | Subject | Re: [PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't specify it | Date | Tue, 15 Mar 2022 07:47:28 +0000 |
| |
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@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@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@microchip.com: >>>>>> On 3/7/22 09:12, Tudor.Ambarus@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@walle.cc> >>>>>>>> Tested-by: Heiko Thiery <heiko.thiery@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?
> 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.
Cheers, ta
| |