[PATCH v2 19/35] mtd: spi-nor: Get rid of SPI_NOR_IO_MODE_EN_VOLATILE flag

[Pratyush Yadav]

On 04/10/21 03:52AM, Tudor.Ambarus at microchip.com wrote:
> On 8/17/21 3:21 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 27/07/21 07:52AM, Tudor Ambarus wrote:
> >> Get rid of flash_info flags that indicate settings which can be
> >> discovered when parsing SFDP. It will be clearer who sets what,
> >> and we'll restrict the flash settings that a developer can choose to
> >> only settings that are not SFDP discoverable.
> >>
> >> SNOR_F_IO_MODE_EN_VOLATILE is discoverable when parsing the optional
> >> SCCR Map SFDP table. Flashes that do not define this table should set
> >> the flag in the late_init() call. Flashes that define the SFDP optional
> >> table but get the value wrong, should fix it in a post_sfdp fixup hook.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
> >> ---
> >>  drivers/mtd/spi-nor/core.c      |  3 ---
> >>  drivers/mtd/spi-nor/core.h      |  9 ++-------
> >>  drivers/mtd/spi-nor/micron-st.c | 11 ++++++++---
> >>  3 files changed, 10 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >> index 240d5c31af88..9885d434ea83 100644
> >> --- a/drivers/mtd/spi-nor/core.c
> >> +++ b/drivers/mtd/spi-nor/core.c
> >> @@ -3204,9 +3204,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> >>       if (ret)
> >>               return ret;
> >>
> >> -     if (info->flags & SPI_NOR_IO_MODE_EN_VOLATILE)
> >> -             nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
> >> -
> >>       ret = spi_nor_set_addr_width(nor);
> >>       if (ret)
> >>               return ret;
> >> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> >> index dfdc51a26cad..987797a789c8 100644
> >> --- a/drivers/mtd/spi-nor/core.h
> >> +++ b/drivers/mtd/spi-nor/core.h
> >> @@ -367,17 +367,12 @@ struct flash_info {
> >>                                        */
> >>  #define SPI_NOR_OCTAL_DTR_READ       BIT(18) /* Flash supports octal DTR Read. */
> >>  #define SPI_NOR_OCTAL_DTR_PP BIT(19) /* Flash supports Octal DTR Page Program */
> >> -#define SPI_NOR_IO_MODE_EN_VOLATILE  BIT(20) /*
> >> -                                              * Flash enables the best
> >> -                                              * available I/O mode via a
> >> -                                              * volatile bit.
> > 
> > We are losing the information contained in the comment. I think you
> > should move it to SNOR_F_IO_MODE_EN_VOLATILE's declaration.
> 
> ok
> 
> > 
> >> -                                              */
> >> -#define SPI_NOR_SWP_IS_VOLATILE      BIT(21) /*
> >> +#define SPI_NOR_SWP_IS_VOLATILE      BIT(20) /*
> >>                                        * Flash has volatile software write
> >>                                        * protection bits. Usually these will
> >>                                        * power-up in a write-protected state.
> >>                                        */
> >> -#define SPI_NOR_PARSE_SFDP   BIT(22) /*
> >> +#define SPI_NOR_PARSE_SFDP   BIT(21) /*
> >>                                        * Flash initialized based on the SFDP
> >>                                        * tables.
> >>                                        */
> >> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> >> index 72cc4673bf88..31ebd4c9b431 100644
> >> --- a/drivers/mtd/spi-nor/micron-st.c
> >> +++ b/drivers/mtd/spi-nor/micron-st.c
> >> @@ -118,13 +118,18 @@ static struct spi_nor_fixups mt35xu512aba_fixups = {
> >>       .post_sfdp = mt35xu512aba_post_sfdp_fixup,
> >>  };
> >>
> >> +static void mt35xu512aba_late_init(struct spi_nor *nor)
> >> +{
> >> +     nor->flags |= SNOR_F_4B_OPCODES;
> > 
> > Like I mentioned in the previous email, you can drop this since the
> > flash populates the 4BAIT table.
> 
> ok
> 
> > 
> >> +     nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
> >> +}
> >> +
> >>  static const struct flash_info micron_parts[] = {
> >>       { "mt35xu512aba", INFO(0x2c5b1a, 0, 128 * 1024, 512,
> >>                              SECT_4K | USE_FSR | SPI_NOR_OCTAL_READ |
> >> -                            SPI_NOR_OCTAL_DTR_READ | SPI_NOR_OCTAL_DTR_PP |
> >> -                            SPI_NOR_IO_MODE_EN_VOLATILE)
> >> +                            SPI_NOR_OCTAL_DTR_READ | SPI_NOR_OCTAL_DTR_PP)
> >>         .fixups = &mt35xu512aba_fixups,
> >> -       .late_init = snor_f_4b_opcodes, },
> >> +       .late_init = mt35xu512aba_late_init, },
> >>       { "mt35xu02g", INFO(0x2c5b1c, 0, 128 * 1024, 2048,
> >>                           SECT_4K | USE_FSR | SPI_NOR_OCTAL_READ)
> >>         .late_init = snor_f_4b_opcodes, },
> > 
> > I forgot to say this in the previous email, but since this flash is the
> > same family as the one above, it should also have the 4BAIT table, and
> > should not need this late_init. But I don't have access to this part so
> > I can't say with 100% certainty.
> > 
> 
> yeah, we don't update flash entries solely by datasheet, we can't update it without
> testing it.

Sounds fair to me. I didn't add Octal DTR support for this flash either, 
since I couldn't actually test it.

> 
> Cheers,
> ta

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.