Re: [PATCH] mtd: spi-nor: Add support for flash reset

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Venkatesh Yadav Abbarapu,

On 10/28/24 8:03 AM, Venkatesh Yadav Abbarapu wrote:
> Add support for spi-nor flash reset via GPIO controller
> by reading the reset-gpios property.
> 
> [Ported from Linux kernel commit
> 8f1ee9ef71d0 ("mtd: spi-nor: Add support for flash reset") ]
> 
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> ---
>   drivers/mtd/spi/spi-nor-core.c | 44 ++++++++++++++++++++++++++++++++++
>   include/linux/mtd/spi-nor.h    |  1 +
>   2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index f5c9868bbc..900f66b0e8 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -12,6 +12,7 @@
>   #include <display_options.h>
>   #include <log.h>
>   #include <watchdog.h>
> +#include <asm/gpio.h>
>   #include <dm.h>
>   #include <dm/device_compat.h>
>   #include <dm/devres.h>
> @@ -4401,7 +4402,13 @@ int spi_nor_remove(struct spi_nor *nor)
>   	    nor->flags & SNOR_F_SOFT_RESET)
>   		return spi_nor_soft_reset(nor);
>   #endif
> +	if (CONFIG_IS_ENABLED(DM_GPIO)) {
> +		if (nor->flash_gpio_reset) {
> +			struct gpio_desc *flash_gpio_reset = nor->flash_gpio_reset;
>   
> +			dm_gpio_free(flash_gpio_reset->dev, flash_gpio_reset);

Since we get the gpio descriptor with a devres-managed function, I think 
this is unnecessary as the core should do it by itself when removing

> +		}
> +	}
>   	return 0;
>   }
>   
> @@ -4448,6 +4455,37 @@ void spi_nor_set_fixups(struct spi_nor *nor)
>   #endif /* SPI_FLASH_MACRONIX */
>   }
>   
> +static int spi_nor_hw_reset(struct spi_nor *nor)
> +{
> +	struct udevice *dev = nor->spi->dev;
> +	int rc;
> +
> +	nor->flash_gpio_reset = devm_gpiod_get_optional(dev, "reset",
> +							GPIOD_IS_OUT | GPIOD_ACTIVE_LOW);
> +
> +	if (nor->flash_gpio_reset) {
> +		/*
> +		 * Experimental delay values by looking at different flash device
> +		 * vendors datasheets.
> +		 */
> +		udelay(5);
> +
> +		/* Toggle gpio to reset the flash device. */
> +		rc = dm_gpio_set_value(nor->flash_gpio_reset, 1);
> +		if (rc)
> +			return rc;
> +
> +		udelay(150);
> +
> +		rc = dm_gpio_set_value(nor->flash_gpio_reset, 0);
> +		if (rc)
> +			return rc;
> +
> +		udelay(1200);

This is a bit odd, I would have assumed a proper reset like 
mmc-pwrseq-emmc or mmc-pwrseq-simple but for nor would have been more 
appropriate than just guessing which timing would cover all NORs?

> +	}
> +	return 0;
> +}
> +
>   int spi_nor_scan(struct spi_nor *nor)
>   {
>   	struct spi_nor_flash_parameter params;
> @@ -4473,6 +4511,12 @@ int spi_nor_scan(struct spi_nor *nor)

This function also exists in spi-nor-tiny, should we add support for the 
hw-reset there as well?

Cheers,
Quentin