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

[<Tudor.Ambarus@microchip.com>]

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