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

[Potthuri, Sai Krishna]

Hi,

> -----Original Message-----
> From: Sai Krishna Potthuri
> Sent: Tuesday, May 31, 2022 1:43 PM
> To: Pratyush Yadav <p.yadav@xxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> spi@xxxxxxxxxxxxxxx; saikrishna12468@xxxxxxxxx; Srinivas Goud
> <sgoud@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; Radhey Shyam
> Pandey <radheys@xxxxxxxxxx>
> Subject: RE: [PATCH 2/2] spi: cadence-quadspi: Add support for OSPI device
> reset
> 
> 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;
> +       }
Any suggestions on this approach?

Regards
Sai Krishna
> 
> 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.