Hi,
> -----Original Message-----
> From: Quentin Schulz <quentin.sch...@cherry.de>
> Sent: Monday, October 28, 2024 3:48 PM
> To: Abbarapu, Venkatesh <venkatesh.abbar...@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.si...@amd.com>; ja...@amarulasolutions.com;
> vigne...@ti.com; p.ya...@ti.com; tr...@konsulko.com; tudor.amba...@linaro.org;
> ma...@denx.de; s...@chromium.org; git (AMD-Xilinx) <g...@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.abbar...@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
