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

Tudor.Ambarus
2021-10-04T03:53:23Z

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