Re: [PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't specify it
From: Michael Walle
Date: Tue Mar 15 2022 - 03:24:24 EST
Am 2022-03-15 06:55, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
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@xxxxxxxxxxxxx:
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@xxxxxxxxxxxxx:
On 3/7/22 09:12, Tudor.Ambarus@xxxxxxxxxxxxx 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@xxxxxxxx>
Tested-by: Heiko Thiery <heiko.thiery@xxxxxxxxx>
---
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