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

[Pratyush Yadav]

On 31/05/22 08:12AM, Sai Krishna Potthuri wrote:
> 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);

Hmm, wouldn't it be better to register the controller as a reset 
provider and then teach SPI NOR to call reset_control_assert()? This way 
you can cleanly handle GPIO based resets as well as MMIO register based 
reset using the CQSPI_REG_CONFIG bit 5.

How I am thinking it should work in your case is you can create a GPIO 
based reset controller driver (I wonder why this hasn't been done yet) 
that toggles a given GPIO line based on reset_control_assert() or 
reset_control_deassert() calls [0]. Then in the SPI NOR DT node you just 
do resets = <&your_reset device>. On a platform which supports reset via 
bit 5 of CQSPI_REG_CONFIG, they can do resets = <&cqspi_controller>.

I am not particularly familiar with the details of the reset framework 
so I would like to hear what others think, but I think it is a good 
proposal to start with.

[0] Or, you could register the GPIO driver itself as a reset controller. 
    I am not sure which works better.

> 
> 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.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.