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.
