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

[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?