| Message ID | 20210727045222.905056-20-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
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@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. > - */ > -#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. > + 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. > -- > 2.25.1 >
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@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. Cheers, ta
On 04/10/21 03:52AM, Tudor.Ambarus@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@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
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. - */ -#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; + 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, },
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@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(-)