From: <Tudor.Ambarus@microchip.com>
To: <michael@walle.cc>
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
Date: Tue, 15 Mar 2022 05:55:49 +0000 [thread overview]
Message-ID: <33464af7-b445-6229-a02d-703a5ce6b5ef@microchip.com> (raw)
In-Reply-To: <0cf8dbbf4ad005abd3db825fb257dedd@walle.cc>
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?
next prev parent reply other threads:[~2022-03-15 5:56 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-04 18:51 [PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't specify it Michael Walle
2022-03-07 7:12 ` Tudor.Ambarus
2022-03-07 9:23 ` Tudor.Ambarus
2022-03-07 18:56 ` Michael Walle
2022-03-09 4:49 ` Tudor.Ambarus
2022-03-14 20:42 ` Michael Walle
2022-03-15 5:55 ` Tudor.Ambarus [this message]
2022-03-15 7:24 ` Michael Walle
2022-03-15 7:47 ` Tudor.Ambarus
2022-03-16 19:07 ` Pratyush Yadav
2022-07-19 4:57 ` Tudor.Ambarus
2022-07-20 13:48 ` Michael Walle
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=33464af7-b445-6229-a02d-703a5ce6b5ef@microchip.com \
--to=tudor.ambarus@microchip.com \
--cc=heiko.thiery@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=michael@walle.cc \
--cc=miquel.raynal@bootlin.com \
--cc=p.yadav@ti.com \
--cc=richard@nod.at \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).