Re: [PATCH v3 3/3] mtd: spi-nor: Favor the BFPT-parsed set_4byte_addr_mode method

From: Michael Walle
Date: Thu Apr 14 2022 - 05:21:58 EST


Am 2022-04-11 14:53, schrieb Tudor Ambarus:
JESD216 SFDP defines in the BFPT standard methods to enter and exit the
4-Byte Address Mode. The flash parameters and settings that are retrieved
from SFDP have higher precedence than the static initialized ones, because
they should be more accurate and less error prone than those initialized
statically. Favor the BFPT-parsed set_4byte_addr_mode method and use the
generic core methods where possible.
This patch may introduce regressions in case BFPT contains wrong data. The
fix is to introduce a post_bfpt() fixup hook and update the wrong BFPT
data.

Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
Reviewed-by: Pratyush Yadav <p.yadav@xxxxxx>
---
v3: no changes

drivers/mtd/spi-nor/core.c | 7 ++++++-
drivers/mtd/spi-nor/macronix.c | 10 ++++++++--
drivers/mtd/spi-nor/micron-st.c | 9 ++++++---
3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 4d45cda4f9d3..888516d98884 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2416,6 +2416,8 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
*/
static void spi_nor_late_init_params(struct spi_nor *nor)
{
+ struct spi_nor_flash_parameter *params = nor->params;
+
if (nor->manufacturer && nor->manufacturer->fixups &&
nor->manufacturer->fixups->late_init)
nor->manufacturer->fixups->late_init(nor);
@@ -2423,6 +2425,10 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
if (nor->info->fixups && nor->info->fixups->late_init)
nor->info->fixups->late_init(nor);

+ /* Default method kept for backward compatibility. */
+ if (!params->set_4byte_addr_mode)
+ params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;

Can this be moved past..

+
spi_nor_init_flags(nor);
spi_nor_init_fixup_flags(nor);

.. these two lines, so it is next to the "set default
locking ops"?


@@ -2490,7 +2496,6 @@ static void spi_nor_init_default_params(struct
spi_nor *nor)
struct device_node *np = spi_nor_get_flash_node(nor);

params->quad_enable = spi_nor_sr2_bit1_quad_enable;
- params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
params->otp.org = &info->otp_org;

/* Default to 16-bit Write Status (01h) Command */
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 85e8655d362c..c267cbcc7f1d 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -105,12 +105,18 @@ static const struct flash_info macronix_nor_parts[] = {
static void macronix_nor_default_init(struct spi_nor *nor)
{
nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
- nor->params->set_4byte_addr_mode =
- spi_nor_set_4byte_addr_mode_en4b_ex4b;
+}
+
+static void macronix_nor_late_init(struct spi_nor *nor)
+{
+ if (!nor->params->set_4byte_addr_mode)
+ nor->params->set_4byte_addr_mode =
+ spi_nor_set_4byte_addr_mode_en4b_ex4b;

This is more of a general question. Can we have this in one
line? IMHO this looks awful and since linux nowadays relaxed the
80 chars rule a bit and we have such long names.. I think it makes
sense to allow some lines to be longer than 80 chars.

}

static const struct spi_nor_fixups macronix_nor_fixups = {
.default_init = macronix_nor_default_init,
+ .late_init = macronix_nor_late_init,
};

const struct spi_nor_manufacturer spi_nor_macronix = {
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 2d1cbb1f37c8..9e9b107f2018 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -414,14 +414,17 @@ static void micron_st_nor_default_init(struct
spi_nor *nor)
nor->flags |= SNOR_F_HAS_LOCK;
nor->flags &= ~SNOR_F_HAS_16BIT_SR;
nor->params->quad_enable = NULL;
- nor->params->set_4byte_addr_mode =
- spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
}

static void micron_st_nor_late_init(struct spi_nor *nor)
{
+ struct spi_nor_flash_parameter *params = nor->params;
+
if (nor->info->mfr_flags & USE_FSR)
- nor->params->ready = micron_st_nor_ready;
+ params->ready = micron_st_nor_ready;
+ if (!params->set_4byte_addr_mode)
+ params->set_4byte_addr_mode =
+ spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;

same here.

-michael