diff mbox series

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

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

Commit Message

Tudor Ambarus July 27, 2021, 4:52 a.m. UTC
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(-)

Comments

Pratyush Yadav Aug. 17, 2021, 12:21 p.m. UTC | #1
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
>
Tudor Ambarus Oct. 4, 2021, 3:52 a.m. UTC | #2
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
Pratyush Yadav Oct. 11, 2021, 6:15 a.m. UTC | #3
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 mbox series

Patch

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, },