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

["Potthuri, Sai Krishna" <sai.krishna.potthuri@xxxxxxx>]

Hi Pratyush,

> -----Original Message-----
> From: Pratyush Yadav <p.yadav@xxxxxx>
> Sent: Tuesday, June 21, 2022 2:47 PM
> To: Sai Krishna Potthuri <lakshmis@xxxxxxxxxx>
> 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
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> 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.
I found this link which does the similar implementation like adding gpio
based reset controller driver but looks like this idea was dropped due to various
reasons.
https://lore.kernel.org/lkml/322faa05-240e-0fd4-8ceb-68f77e871cf6@xxxxxxxx/T/#m6c676fe25453525aecb26c71f3f3a5bad5e3e923

My understanding after going through the discussion is, we should live
with 'reset-gpios' property to register the GPIO based reset pin and make
use of the gpio driver calls(gpiod_set_value).
I may need to do this at SPI NOR layer instead of handling it in the driver.

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