| Message ID | 20220411125346.118274-4-tudor.ambarus@microchip.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode | expand |
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@microchip.com> > Reviewed-by: Pratyush Yadav <p.yadav@ti.com> > --- > 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
On 4/14/22 12:21, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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@microchip.com> >> Reviewed-by: Pratyush Yadav <p.yadav@ti.com> >> --- >> 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"? > sure >> >> @@ -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. I still split my terminal in multiple windows and pretty much read code in a 80 chars limit. But I won't argue with you, it seems you care about it, so I'll change it. > >> } >> >> 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. > sure Thanks, ta
On 14/04/22 09:32AM, Tudor.Ambarus@microchip.com wrote: > On 4/14/22 12:21, Michael Walle wrote: > >> +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. > > I still split my terminal in multiple windows and pretty much read > code in a 80 chars limit. But I won't argue with you, it seems you > care about it, so I'll change it. FWIW, I do the same. I won't like it much if line lengths get too long. I won't take any sides here though, either should be fine as long as the result at least fits in 100 chars.
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; + spi_nor_init_flags(nor); spi_nor_init_fixup_flags(nor); @@ -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; } 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; } static const struct spi_nor_fixups micron_st_nor_fixups = {