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

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Quentin Schulz <quentin.schulz at cherry.de>
> Sent: Monday, October 28, 2024 3:48 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; u-boot at lists.denx.de
> Cc: Simek, Michal <michal.simek at amd.com>; jagan at amarulasolutions.com;
> vigneshr at ti.com; p.yadav at ti.com; trini at konsulko.com; tudor.ambarus at linaro.org;
> marex at denx.de; sjg at chromium.org; git (AMD-Xilinx) <git at amd.com>
> Subject: Re: [PATCH] mtd: spi-nor: Add support for flash reset
> 
> 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 at 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?
These delays have been updated by looking at different flash vendor datasheets as per below reference
https://github.com/torvalds/linux/blob/master/drivers/mtd/spi-nor/core.c#L3428

Do you have anything with respect to these delay values?

Thanks
Venkatesh
> 
> > +	}
> > +	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