Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Tue, 15 Mar 2022 08:24:17 +0100
From:   Michael Walle <michael@walle.cc>
To:     Tudor.Ambarus@microchip.com
Cc:     p.yadav@ti.com, miquel.raynal@bootlin.com, richard@nod.at,
        vigneshr@ti.com, linux-mtd@lists.infradead.org,
        linux-kernel@vger.kernel.org, heiko.thiery@gmail.com
Subject: Re: [PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't
 specify it
In-Reply-To: <33464af7-b445-6229-a02d-703a5ce6b5ef@microchip.com>
References: <20220304185137.3376011-1-michael@walle.cc>
 <7f339d0c-5ca9-261c-a545-d4ebf3bda140@microchip.com>
 <92cde38c-d398-44f4-26f8-ef4919f5944e@microchip.com>
 <7f947928e7189f98eb950828990b3920@walle.cc>
 <91393780-1521-09b7-8dea-14c65e18b37e@microchip.com>
 <0cf8dbbf4ad005abd3db825fb257dedd@walle.cc>
 <33464af7-b445-6229-a02d-703a5ce6b5ef@microchip.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <77c2c64f362b08cbbdab517bbaa49101@walle.cc>
Precedence: bulk

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.
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.

-michael