diff mbox series

[v2,2/3] mtd: spi-nor: add SFDP fixups for Quad Page Program

Message ID 20220809201428.118523-3-sudip.mukherjee@sifive.com
State Superseded
Delegated to: Ambarus Tudor
Headers show
Series Add support for Quad Input Page Program to is25wp256 | expand

Commit Message

Sudip Mukherjee Aug. 9, 2022, 8:14 p.m. UTC 
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 <sudip.mukherjee@sifive.com>
---
 drivers/mtd/spi-nor/core.c | 9 +++++++++
 drivers/mtd/spi-nor/core.h | 2 ++
 2 files changed, 11 insertions(+)

Comments

Tudor Ambarus Aug. 10, 2022, 8:06 a.m. UTC | #1 
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 <sudip.mukherjee@sifive.com>
> ---
>  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
>
Tudor Ambarus Aug. 10, 2022, 8:25 a.m. UTC | #2 
On 8/10/22 11:06, Tudor.Ambarus@microchip.com 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 <sudip.mukherjee@sifive.com>
>> ---
>>  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/
Sudip Mukherjee Aug. 10, 2022, 3:14 p.m. UTC | #3 
On Wed, Aug 10, 2022 at 9:06 AM <Tudor.Ambarus@microchip.com> 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
Sudip Mukherjee Aug. 10, 2022, 3:17 p.m. UTC | #4 
On Wed, Aug 10, 2022 at 9:25 AM <Tudor.Ambarus@microchip.com> wrote:
>
> On 8/10/22 11:06, Tudor.Ambarus@microchip.com 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 <sudip.mukherjee@sifive.com>
> >> ---
> >>  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
Tudor Ambarus Aug. 10, 2022, 4:48 p.m. UTC | #5 
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 <Tudor.Ambarus@microchip.com> 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/20220810155924.1366072-1-tudor.ambarus@microchip.com/T/#u
> --
> Regards
> Sudip
diff mbox series

Patch

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;