Re: [PATCH v2 09/35] mtd: spi-nor: atmel: Use flash late_init() for locking

Pratyush Yadav
2021-08-16T19:07:50Z

On 27/07/21 07:51AM, Tudor Ambarus wrote:
> Locking is not described in JESD216 SFDP standard, place the locking
> init in late_init().

You are chaning the order of setting the locking ops here. Earlier, they
were set before we parsed SFDP. Now they are set after we parse SFDP.
Though I don't see it making much of a difference.

>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> drivers/mtd/spi-nor/atmel.c | 30 +++++++++++-------------------
> 1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
> index 1fea5cab492c..b937ef734e55 100644
> --- a/drivers/mtd/spi-nor/atmel.c
> +++ b/drivers/mtd/spi-nor/atmel.c
> @@ -48,15 +48,11 @@ static const struct spi_nor_locking_ops atmel_at25fs_locking_ops = {
> .is_locked = atmel_at25fs_is_locked,
> };
>
> -static void atmel_at25fs_default_init(struct spi_nor *nor)
> +static void atmel_at25fs_late_init(struct spi_nor *nor)
> {
> nor->params->locking_ops = &atmel_at25fs_locking_ops;
> }
>
> -static const struct spi_nor_fixups atmel_at25fs_fixups = {
> - .default_init = atmel_at25fs_default_init,
> -};
> -
> /**
> * atmel_set_global_protection - Do a Global Protect or Unprotect command
> * @nor: pointer to 'struct spi_nor'
> @@ -146,34 +142,30 @@ static const struct spi_nor_locking_ops atmel_global_protection_ops = {
> .is_locked = atmel_is_global_protected,
> };
>
> -static void atmel_global_protection_default_init(struct spi_nor *nor)
> +static void atmel_global_protection_late_init(struct spi_nor *nor)
> {
> nor->params->locking_ops = &atmel_global_protection_ops;
> }
>
> -static const struct spi_nor_fixups atmel_global_protection_fixups = {
> - .default_init = atmel_global_protection_default_init,
> -};
> -
> static const struct flash_info atmel_parts[] = {
> /* Atmel -- some are (confusingly) marketed as "DataFlash" */
> { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K | SPI_NOR_HAS_LOCK)
> - .fixups = &atmel_at25fs_fixups },
> + .late_init = atmel_at25fs_late_init },
> { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_HAS_LOCK)
> - .fixups = &atmel_at25fs_fixups },
> + .late_init = atmel_at25fs_late_init },
>
> { "at25df041a", INFO(0x1f4401, 0, 64 * 1024, 8,
> SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> - .fixups = &atmel_global_protection_fixups },
> + .late_init = atmel_global_protection_late_init },

Won't you be better off setting this in the manufacturer late_init()? It
seems common for most atmel flashes.

Of course, this would cause a problem for atmel flashes that don't have
this at all, since we would set locking for those as well. But I think
we can avoid that by checking for SNOR_F_HAS_LOCK in
spi_nor_register_locking_ops().

> { "at25df321", INFO(0x1f4700, 0, 64 * 1024, 64,
> SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> - .fixups = &atmel_global_protection_fixups },
> + .late_init = atmel_global_protection_late_init },
> { "at25df321a", INFO(0x1f4701, 0, 64 * 1024, 64,
> SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> - .fixups = &atmel_global_protection_fixups },
> + .late_init = atmel_global_protection_late_init },
> { "at25df641", INFO(0x1f4800, 0, 64 * 1024, 128,
> SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> - .fixups = &atmel_global_protection_fixups },
> + .late_init = atmel_global_protection_late_init },
>
> { "at25sl321", INFO(0x1f4216, 0, 64 * 1024, 64,
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> @@ -181,13 +173,13 @@ static const struct flash_info atmel_parts[] = {
> { "at26f004", INFO(0x1f0400, 0, 64 * 1024, 8, SECT_4K) },
> { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16,
> SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> - .fixups = &atmel_global_protection_fixups },
> + .late_init = atmel_global_protection_late_init },
> { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32,
> SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> - .fixups = &atmel_global_protection_fixups },
> + .late_init = atmel_global_protection_late_init },
> { "at26df321", INFO(0x1f4700, 0, 64 * 1024, 64,
> SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> - .fixups = &atmel_global_protection_fixups },
> + .late_init = atmel_global_protection_late_init },
>
> { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
> };
> --
> 2.25.1
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.