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

[<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? 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().