2022-08-09 20:42:50

by Sudip Mukherjee

[permalink] [raw]
Subject: [PATCH v2 2/3] mtd: spi-nor: add SFDP fixups for Quad Page Program

SFDP table of some flash chips do not advertise support of Quad Input
Page Program even though it has support. Use fixup flags and add hardware
cap for these chips.

Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/mtd/spi-nor/core.c | 9 +++++++++
drivers/mtd/spi-nor/core.h | 2 ++
2 files changed, 11 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f2c64006f8d7..7542404332a5 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1962,6 +1962,12 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
if (nor->flags & SNOR_F_BROKEN_RESET)
*hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);

+ if (nor->flags & SNOR_F_HAS_QUAD_PP) {
+ *hwcaps |= SNOR_HWCAPS_PP_1_1_4;
+ spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_1_1_4],
+ SPINOR_OP_PP_1_1_4, SNOR_PROTO_1_1_4);
+ }
+
for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
int rdidx, ppidx;

@@ -2446,6 +2452,9 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)

if (fixup_flags & SPI_NOR_IO_MODE_EN_VOLATILE)
nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
+
+ if (fixup_flags & SPI_NOR_QUAD_PP)
+ nor->flags |= SNOR_F_HAS_QUAD_PP;
}

/**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 85b0cf254e97..7dbdf16a67b4 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -130,6 +130,7 @@ enum spi_nor_option_flags {
SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
SNOR_F_SOFT_RESET = BIT(12),
SNOR_F_SWP_IS_VOLATILE = BIT(13),
+ SNOR_F_HAS_QUAD_PP = BIT(14),
};

struct spi_nor_read_command {
@@ -520,6 +521,7 @@ struct flash_info {
u8 fixup_flags;
#define SPI_NOR_4B_OPCODES BIT(0)
#define SPI_NOR_IO_MODE_EN_VOLATILE BIT(1)
+#define SPI_NOR_QUAD_PP BIT(2)

u8 mfr_flags;

--
2.30.2


2022-08-10 08:35:42

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] mtd: spi-nor: add SFDP fixups for Quad Page Program

On 8/10/22 11:06, [email protected] wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 8/9/22 23:14, Sudip Mukherjee wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> SFDP table of some flash chips do not advertise support of Quad Input
>> Page Program even though it has support. Use fixup flags and add hardware
>> cap for these chips.
>>
>> Signed-off-by: Sudip Mukherjee <[email protected]>
>> ---
>> drivers/mtd/spi-nor/core.c | 9 +++++++++
>> drivers/mtd/spi-nor/core.h | 2 ++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index f2c64006f8d7..7542404332a5 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1962,6 +1962,12 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>> if (nor->flags & SNOR_F_BROKEN_RESET)
>> *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
>>
>> + if (nor->flags & SNOR_F_HAS_QUAD_PP) {
>> + *hwcaps |= SNOR_HWCAPS_PP_1_1_4;
>> + spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_1_1_4],
>> + SPINOR_OP_PP_1_1_4, SNOR_PROTO_1_1_4);
>> + }
>
> setting SPINOR_OP_PP_1_1_4 should be done in spi_nor_late_init_params().
> spi_nor_late_init_params() is used to adjust the ops supported by the flash

^ s/spi_nor_late_init_params/spi_nor_spimem_adjust_hwcaps

> with the ones supported by the controller.
>
>> +
>> for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
>> int rdidx, ppidx;
>>
>> @@ -2446,6 +2452,9 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>>
>> if (fixup_flags & SPI_NOR_IO_MODE_EN_VOLATILE)
>> nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
>> +
>> + if (fixup_flags & SPI_NOR_QUAD_PP)
>> + nor->flags |= SNOR_F_HAS_QUAD_PP;
>> }
>>
>> /**
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 85b0cf254e97..7dbdf16a67b4 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -130,6 +130,7 @@ enum spi_nor_option_flags {
>> SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
>> SNOR_F_SOFT_RESET = BIT(12),
>> SNOR_F_SWP_IS_VOLATILE = BIT(13),
>> + SNOR_F_HAS_QUAD_PP = BIT(14),
>
> you won't need this
>> };
>>
>> struct spi_nor_read_command {
>> @@ -520,6 +521,7 @@ struct flash_info {
>> u8 fixup_flags;
>> #define SPI_NOR_4B_OPCODES BIT(0)
>> #define SPI_NOR_IO_MODE_EN_VOLATILE BIT(1)
>> +#define SPI_NOR_QUAD_PP BIT(2)
>
> No, as I previously said, SPI_NOR_QUAD_PP should be declared as a
> info->flags, not as info->fixup_flags.
>
>>
>> u8 mfr_flags;
>>
>> --
>> 2.30.2
>>
>
>
> --
> Cheers,
> ta
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


--
Cheers,
ta

2022-08-10 08:46:10

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] mtd: spi-nor: add SFDP fixups for Quad Page Program

On 8/9/22 23:14, Sudip Mukherjee wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> SFDP table of some flash chips do not advertise support of Quad Input
> Page Program even though it has support. Use fixup flags and add hardware
> cap for these chips.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 9 +++++++++
> drivers/mtd/spi-nor/core.h | 2 ++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f2c64006f8d7..7542404332a5 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1962,6 +1962,12 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
> if (nor->flags & SNOR_F_BROKEN_RESET)
> *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
>
> + if (nor->flags & SNOR_F_HAS_QUAD_PP) {
> + *hwcaps |= SNOR_HWCAPS_PP_1_1_4;
> + spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_1_1_4],
> + SPINOR_OP_PP_1_1_4, SNOR_PROTO_1_1_4);
> + }

setting SPINOR_OP_PP_1_1_4 should be done in spi_nor_late_init_params().
spi_nor_late_init_params() is used to adjust the ops supported by the flash
with the ones supported by the controller.

> +
> for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
> int rdidx, ppidx;
>
> @@ -2446,6 +2452,9 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>
> if (fixup_flags & SPI_NOR_IO_MODE_EN_VOLATILE)
> nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
> +
> + if (fixup_flags & SPI_NOR_QUAD_PP)
> + nor->flags |= SNOR_F_HAS_QUAD_PP;
> }
>
> /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 85b0cf254e97..7dbdf16a67b4 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -130,6 +130,7 @@ enum spi_nor_option_flags {
> SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
> SNOR_F_SOFT_RESET = BIT(12),
> SNOR_F_SWP_IS_VOLATILE = BIT(13),
> + SNOR_F_HAS_QUAD_PP = BIT(14),

you won't need this
> };
>
> struct spi_nor_read_command {
> @@ -520,6 +521,7 @@ struct flash_info {
> u8 fixup_flags;
> #define SPI_NOR_4B_OPCODES BIT(0)
> #define SPI_NOR_IO_MODE_EN_VOLATILE BIT(1)
> +#define SPI_NOR_QUAD_PP BIT(2)

No, as I previously said, SPI_NOR_QUAD_PP should be declared as a
info->flags, not as info->fixup_flags.

>
> u8 mfr_flags;
>
> --
> 2.30.2
>


--
Cheers,
ta

2022-08-10 15:41:26

by Sudip Mukherjee

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] mtd: spi-nor: add SFDP fixups for Quad Page Program

On Wed, Aug 10, 2022 at 9:25 AM <[email protected]> wrote:
>
> On 8/10/22 11:06, [email protected] wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 8/9/22 23:14, Sudip Mukherjee wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>
> >> SFDP table of some flash chips do not advertise support of Quad Input
> >> Page Program even though it has support. Use fixup flags and add hardware
> >> cap for these chips.
> >>
> >> Signed-off-by: Sudip Mukherjee <[email protected]>
> >> ---
> >> drivers/mtd/spi-nor/core.c | 9 +++++++++
> >> drivers/mtd/spi-nor/core.h | 2 ++
> >> 2 files changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >> index f2c64006f8d7..7542404332a5 100644
> >> --- a/drivers/mtd/spi-nor/core.c
> >> +++ b/drivers/mtd/spi-nor/core.c
> >> @@ -1962,6 +1962,12 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
> >> if (nor->flags & SNOR_F_BROKEN_RESET)
> >> *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
> >>
> >> + if (nor->flags & SNOR_F_HAS_QUAD_PP) {
> >> + *hwcaps |= SNOR_HWCAPS_PP_1_1_4;
> >> + spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_1_1_4],
> >> + SPINOR_OP_PP_1_1_4, SNOR_PROTO_1_1_4);
> >> + }
> >
> > setting SPINOR_OP_PP_1_1_4 should be done in spi_nor_late_init_params().
> > spi_nor_late_init_params() is used to adjust the ops supported by the flash
>
> ^ s/spi_nor_late_init_params/spi_nor_spimem_adjust_hwcaps

So, do you mean something like this:

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f2c64006f8d7..2f41937b826d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1962,6 +1962,12 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor
*nor, u32 *hwcaps)
if (nor->flags & SNOR_F_BROKEN_RESET)
*hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);

+ if (nor->info->flags & SPI_NOR_QUAD_PP) {
+ *hwcaps |= SNOR_HWCAPS_PP_1_1_4;
+ spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_1_1_4],
+ SPINOR_OP_PP_1_1_4, SNOR_PROTO_1_1_4);
+ }
+
for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
int rdidx, ppidx;

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 85b0cf254e97..10aa1c72000f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -507,6 +507,7 @@ struct flash_info {
#define SPI_NOR_NO_ERASE BIT(6)
#define NO_CHIP_ERASE BIT(7)
#define SPI_NOR_NO_FR BIT(8)
+#define SPI_NOR_QUAD_PP BIT(9)

u8 no_sfdp_flags;
#define SPI_NOR_SKIP_SFDP BIT(0)
diff --git a/drivers/mtd/spi-nor/issi.c b/drivers/mtd/spi-nor/issi.c
index 89a66a19d754..014cd9038bed 100644
--- a/drivers/mtd/spi-nor/issi.c
+++ b/drivers/mtd/spi-nor/issi.c
@@ -71,8 +71,9 @@ static const struct flash_info issi_nor_parts[] = {
{ "is25wp128", INFO(0x9d7018, 0, 64 * 1024, 256)
NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 512)
- NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+ PARSE_SFDP
FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
+ FLAGS(SPI_NOR_QUAD_PP)
.fixups = &is25lp256_fixups },

/* PMC */


--
Regards
Sudip

2022-08-10 16:10:32

by Sudip Mukherjee

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] mtd: spi-nor: add SFDP fixups for Quad Page Program

On Wed, Aug 10, 2022 at 9:06 AM <[email protected]> wrote:
>
> On 8/9/22 23:14, Sudip Mukherjee wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > SFDP table of some flash chips do not advertise support of Quad Input
> > Page Program even though it has support. Use fixup flags and add hardware
> > cap for these chips.
> >

<snip>

> > @@ -520,6 +521,7 @@ struct flash_info {
> > u8 fixup_flags;
> > #define SPI_NOR_4B_OPCODES BIT(0)
> > #define SPI_NOR_IO_MODE_EN_VOLATILE BIT(1)
> > +#define SPI_NOR_QUAD_PP BIT(2)
>
> No, as I previously said, SPI_NOR_QUAD_PP should be declared as a
> info->flags, not as info->fixup_flags.

Sorry, I was confused as info->fixup_flags says "it indicates support
that can be discovered via SFDP ideally, but can not be discovered
for this particular flash because the SFDP table that indicates this
support is not defined by the flash."

--
Regards
Sudip

2022-08-10 16:52:37

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] mtd: spi-nor: add SFDP fixups for Quad Page Program

On 8/10/22 18:14, Sudip Mukherjee wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, Aug 10, 2022 at 9:06 AM <[email protected]> wrote:
>>
>> On 8/9/22 23:14, Sudip Mukherjee wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> SFDP table of some flash chips do not advertise support of Quad Input
>>> Page Program even though it has support. Use fixup flags and add hardware
>>> cap for these chips.
>>>
>
> <snip>
>
>>> @@ -520,6 +521,7 @@ struct flash_info {
>>> u8 fixup_flags;
>>> #define SPI_NOR_4B_OPCODES BIT(0)
>>> #define SPI_NOR_IO_MODE_EN_VOLATILE BIT(1)
>>> +#define SPI_NOR_QUAD_PP BIT(2)
>>
>> No, as I previously said, SPI_NOR_QUAD_PP should be declared as a
>> info->flags, not as info->fixup_flags.
>
> Sorry, I was confused as info->fixup_flags says "it indicates support
> that can be discovered via SFDP ideally, but can not be discovered
> for this particular flash because the SFDP table that indicates this
> support is not defined by the flash."
>
Right, I've just sent a patch addressing this, hopefully is clearer now.
Check it here:
https://lore.kernel.org/linux-mtd/[email protected]/T/#u
> --
> Regards
> Sudip


--
Cheers,
ta