Depends on:
https://lore.kernel.org/lkml/[email protected]/
Get the 4-Byte Address mode method from BFPT and favor it in the detriment
of the "default" set_4byte_addr_mode method or the methods set by vendors.
This may introduce some regressions if flashes have wrong BFPT data. The
fix is to introduce post_bfpt() hooks and fix where needed. We should let
the core/sfdp do the params initialization, and do vendor specific updates
just where needed.
v3:
- Move the newly introduced BFPT definitions in sfdp.h, as they may be used by
manufacturer drivers to handle flash ID collisions.
- Drop "mtd: spi-nor: sfdp: Keep SFDP definitions private" patch for the same
reason as above
- Collect R-b tags
v2: Drop quad enable patch
Tudor Ambarus (3):
mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode methods
mtd: spi-nor: Update name and description of the set_4byte_addr_mode
BFPT methods
mtd: spi-nor: Favor the BFPT-parsed set_4byte_addr_mode method
drivers/mtd/spi-nor/core.c | 70 ++------------------
drivers/mtd/spi-nor/core.h | 1 -
drivers/mtd/spi-nor/macronix.c | 9 ++-
drivers/mtd/spi-nor/micron-st.c | 32 ++-------
drivers/mtd/spi-nor/sfdp.c | 113 ++++++++++++++++++++++++++++++++
drivers/mtd/spi-nor/sfdp.h | 30 +++++++++
drivers/mtd/spi-nor/winbond.c | 18 +++--
7 files changed, 174 insertions(+), 99 deletions(-)
--
2.25.1
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 <[email protected]>
Reviewed-by: Pratyush Yadav <[email protected]>
---
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;
+
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 = {
--
2.25.1
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 <[email protected]>
>> Reviewed-by: Pratyush Yadav <[email protected]>
>> ---
>> 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
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 <[email protected]>
> Reviewed-by: Pratyush Yadav <[email protected]>
> ---
> 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 14/04/22 09:32AM, [email protected] 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.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.