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

[Quentin Schulz]

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?

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