RE: [PATCH 2/2] spi: cadence-quadspi: Add support for OSPI device reset

[Sai Krishna Potthuri <lakshmis@xxxxxxxxxx>]

Hi Pratyush,

> -----Original Message-----
> From: Pratyush Yadav <p.yadav@xxxxxx>
> Sent: Wednesday, April 6, 2022 12:48 AM
> To: Sai Krishna Potthuri <lakshmis@xxxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> spi@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; git
> <git@xxxxxxxxxx>; saikrishna12468@xxxxxxxxx; Srinivas Goud
> <sgoud@xxxxxxxxxx>
> Subject: Re: [PATCH 2/2] spi: cadence-quadspi: Add support for OSPI device
> reset
> 
> On 05/04/22 04:30PM, Sai Krishna Potthuri wrote:
> > Cadence OSPI controller always start in SDR mode and it doesn't know
> > the current mode of the flash device (SDR or DDR). This creates issue
> > during Cadence OSPI driver probe if OSPI flash device is in DDR mode.
> > This patch add OSPI flash device reset using HW reset pin for Xilinx
> > Versal platform, this will make sure both Controller and Flash device
> > are in same mode (SDR).
> 
> Is this supposed to reset the OSPI flash or the controller? If you are resetting
> it in the flash then you should handle this from the flash driver, not from
> here.
I am handling OSPI flash reset here. Agree, controlling or issuing the flash reset 
should be from the flash driver and not from the controller driver but handling
the reset might depends on the platform and should be in the controller driver. 
One platform might be handling the reset through GPIO and others might handle 
it differently via some system level control registers or even controller registers.
To support this platform specific implementation i am thinking to provide a
"hw_reset" hook in the spi_controller_mem_ops structure and this will be
accessed or called from spi-nor if  "broken-flash-reset" property is not set.
Whichever controller driver registers for hw_reset hook, they can have their own
implementation to reset the flash device based on the platform.
Do you think this approach works? Please suggest.

Code snippet like below.

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 2ba044d0d5e5..b8240dfb246d 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -285,6 +285,7 @@ struct spi_controller_mem_ops {
                           unsigned long initial_delay_us,
                           unsigned long polling_rate_us,
                           unsigned long timeout_ms);
+       int (*hw_reset)(struct spi_mem *mem);

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index e8de4f5017cd..9ac2c2c30443 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -598,6 +598,27 @@ static void devm_spi_mem_dirmap_release(struct device *dev, void *res)
        spi_mem_dirmap_destroy(desc);
 }
+int spi_mem_hw_reset(struct spi_mem *mem)
+{
+       struct spi_controller *ctlr = mem->spi->controller;
+
+       if (ctlr->mem_ops && ctlr->mem_ops->hw_reset)
+               return ctlr->mem_ops->hw_reset(mem);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(spi_mem_hw_reset);

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index b4f141ad9c9c..2c09c733bb8b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2966,6 +2962,7 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
 int spi_nor_scan(struct spi_nor *nor, const char *name,
                 const struct spi_nor_hwcaps *hwcaps)
 {
+       struct device_node *np = spi_nor_get_flash_node(nor);
        const struct flash_info *info;
        struct device *dev = nor->dev;
        struct mtd_info *mtd = &nor->mtd;
@@ -2995,6 +2992,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
        if (!nor->bouncebuf)
                return -ENOMEM;
 
+       if (of_property_read_bool(np, "broken-flash-reset")) {
+               nor->flags |= SNOR_F_BROKEN_RESET;
+       } else {
+               ret = spi_mem_hw_reset(nor->spimem);
+               if (ret)
+                       return ret;
+       }

Regards
Sai Krishna
> 
> Also, as of now at least, SPI NOR only works when the flash is in SDR mode.
> For TI platforms, we reset the flash in the bootloader (U-Boot), before
> handing control off to the kernel. If you do want to properly handle flashes
> that are handed to the kernel in DDR mode, I would suggest you update SPI
> NOR instead to detect the flash mode and work from there. This would also
> allow us to support flashes that boot in DDR mode, so would still be in DDR
> mode even after a reset.
> 
> > Xilinx Versal platform has a dedicated pin used for OSPI device reset.
> > As part of the reset sequence, configure the pin to enable hysteresis
> > and set the direction of the pin to output before toggling the pin.
> > Provided the required delay ranges while toggling the pin to meet the
> > most of the OSPI flash devices reset pulse width, reset recovery and
> > CS high to reset high timings.
> >
> > Signed-off-by: Sai Krishna Potthuri
> > <lakshmi.sai.krishna.potthuri@xxxxxxxxxx>
> [...]
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.