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

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

[Sai Krishna Potthuri]

This patch series update dt-binding and OSPI driver to support OSPI flash
device reset on Xilinx Versal platform.

BRANCH: for-next

Sai Krishna Potthuri (2):
  dt-bindings: spi: cadence-quadspi: Add reset-gpios for Xilinx Versal
    OSPI
  spi: cadence-quadspi: Add support for OSPI device reset

 .../bindings/spi/cdns,qspi-nor.yaml           |  7 ++
 drivers/spi/spi-cadence-quadspi.c             | 72 +++++++++++++++++++
 2 files changed, 79 insertions(+)

-- 
2.17.1

[PATCH 1/2] dt-bindings: spi: cadence-quadspi: Add reset-gpios for Xilinx Versal OSPI

[Sai Krishna Potthuri]

Add reset-gpios property to the properties list and marked as required
for Xilinx Versal OSPI compatible. This is used to perform the HW reset
for OSPI flash device.

Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com>
---
 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index 0a537fa3a641..56001eaf6365 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -19,6 +19,7 @@ allOf:
     then:
       required:
         - power-domains
+        - reset-gpios
 
 properties:
   compatible:
@@ -78,6 +79,12 @@ properties:
   power-domains:
     maxItems: 1
 
+  reset-gpios:
+    description:
+      Contains a phandle and GPIO specifier for reset with the default active
+      state.
+    maxItems: 1
+
   resets:
     maxItems: 2
 
-- 
2.17.1

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

[Sai Krishna Potthuri]

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).
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@xilinx.com>
---
 drivers/spi/spi-cadence-quadspi.c | 72 +++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index b808c94641fa..6e5b5b180347 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -21,6 +21,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -96,6 +97,7 @@ struct cqspi_driver_platdata {
 	int (*indirect_read_dma)(struct cqspi_flash_pdata *f_pdata,
 				 u_char *rxbuf, loff_t from_addr, size_t n_rx);
 	u32 (*get_dma_status)(struct cqspi_st *cqspi);
+	int (*device_hw_reset)(struct cqspi_st *cqspi);
 };
 
 /* Operation timeout value */
@@ -281,6 +283,7 @@ struct cqspi_driver_platdata {
 #define CQSPI_DMA_UNALIGN		0x3
 
 #define CQSPI_REG_VERSAL_DMA_VAL		0x602
+#define CQSPI_VERSAL_MIO_NODE_ID_12	0x14108027
 
 static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clr)
 {
@@ -835,6 +838,68 @@ static int cqspi_indirect_read_execute(struct cqspi_flash_pdata *f_pdata,
 	return ret;
 }
 
+static int cqspi_versal_device_reset(struct cqspi_st *cqspi)
+{
+	struct platform_device *pdev = cqspi->pdev;
+	int ret;
+	int gpio;
+	enum of_gpio_flags flags;
+
+	gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+				       "reset-gpios", 0, &flags);
+	if (!gpio_is_valid(gpio))
+		return gpio;
+
+	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
+				    "flash-reset");
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to get reset-gpios: %d\n", ret);
+		return ret;
+	}
+
+	/* Request for PIN */
+	ret = zynqmp_pm_pinctrl_request(CQSPI_VERSAL_MIO_NODE_ID_12);
+	if (ret)
+		return ret;
+
+	/* Enable hysteresis in cmos receiver */
+	ret = zynqmp_pm_pinctrl_set_config(CQSPI_VERSAL_MIO_NODE_ID_12,
+					   PM_PINCTRL_CONFIG_SCHMITT_CMOS,
+					   PM_PINCTRL_INPUT_TYPE_SCHMITT);
+	if (ret)
+		return ret;
+
+	/* Set the direction as output and enable the output */
+	gpio_direction_output(gpio, 1);
+
+	/*
+	 * Experimental Minimum Chip select high to Reset delay value
+	 * based on the supported OSPI flash device spec.
+	 */
+	usleep_range(1, 5);
+
+	/* Set value 0 to pin */
+	gpio_set_value(gpio, 0);
+
+	/*
+	 * Experimental Minimum Reset pulse width value based on the
+	 * supported OSPI flash device spec.
+	 */
+	usleep_range(10, 15);
+
+	/* Set value 1 to pin */
+	gpio_set_value(gpio, 1);
+
+	/*
+	 * Experimental Minimum Reset recovery delay value based on the
+	 * supported OSPI flash device spec.
+	 */
+	usleep_range(35, 40);
+
+	return 0;
+}
+
 static int cqspi_versal_indirect_read_dma(struct cqspi_flash_pdata *f_pdata,
 					  u_char *rxbuf, loff_t from_addr,
 					  size_t n_rx)
@@ -1783,6 +1848,12 @@ static int cqspi_probe(struct platform_device *pdev)
 		goto probe_setup_failed;
 	}
 
+	if (ddata->device_hw_reset) {
+		ret = ddata->device_hw_reset(cqspi);
+		if (ret)
+			goto probe_setup_failed;
+	}
+
 	if (cqspi->use_direct_mode) {
 		ret = cqspi_request_mmap_dma(cqspi);
 		if (ret == -EPROBE_DEFER)
@@ -1878,6 +1949,7 @@ static const struct cqspi_driver_platdata versal_ospi = {
 	.quirks = CQSPI_DISABLE_DAC_MODE | CQSPI_SUPPORT_EXTERNAL_DMA,
 	.indirect_read_dma = cqspi_versal_indirect_read_dma,
 	.get_dma_status = cqspi_get_versal_dma_status,
+	.device_hw_reset = cqspi_versal_device_reset,
 };
 
 static const struct of_device_id cqspi_dt_ids[] = {
-- 
2.17.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

On Tue, Apr 05, 2022 at 04:30:37PM +0530, Sai Krishna Potthuri wrote:

> +static int cqspi_versal_device_reset(struct cqspi_st *cqspi)
> +{
> +	struct platform_device *pdev = cqspi->pdev;
> +	int ret;
> +	int gpio;
> +	enum of_gpio_flags flags;
> +
> +	gpio = of_get_named_gpio_flags(pdev->dev.of_node,
> +				       "reset-gpios", 0, &flags);
> +	if (!gpio_is_valid(gpio))
> +		return gpio;
> +
> +	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> +				    "flash-reset");

You should use the gpiod APIs here which should mean you don't need any
DT specific code to parse the flags.  At some point the old GPIO APIs
will be removed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Pratyush Yadav]

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.

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@xilinx.com>
[...]

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 1/2] dt-bindings: spi: cadence-quadspi: Add reset-gpios for Xilinx Versal OSPI

[Rob Herring]

On Tue, Apr 05, 2022 at 04:30:36PM +0530, Sai Krishna Potthuri wrote:
> Add reset-gpios property to the properties list and marked as required
> for Xilinx Versal OSPI compatible. This is used to perform the HW reset
> for OSPI flash device.
> 
> Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com>
> ---
>  Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> index 0a537fa3a641..56001eaf6365 100644
> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> @@ -19,6 +19,7 @@ allOf:
>      then:
>        required:
>          - power-domains
> +        - reset-gpios
>  
>  properties:
>    compatible:
> @@ -78,6 +79,12 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  reset-gpios:
> +    description:
> +      Contains a phandle and GPIO specifier for reset with the default active
> +      state.

Other than 'for reset' that's every GPIO property. Just drop unless you 
have something specific to cdns,qspi-nor to say.

However, a reset for the flash device belongs in the flash device node, 
not the controller node.

Rob

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

[Sai Krishna Potthuri]

Hi Pratyush,

> -----Original Message-----
> From: Pratyush Yadav <p.yadav@ti.com>
> Sent: Wednesday, April 6, 2022 12:48 AM
> To: Sai Krishna Potthuri <lakshmis@xilinx.com>
> Cc: Mark Brown <broonie@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> spi@vger.kernel.org; Michal Simek <michals@xilinx.com>; git
> <git@xilinx.com>; saikrishna12468@gmail.com; Srinivas Goud
> <sgoud@xilinx.com>
> 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@xilinx.com>
> [...]
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.

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@ti.com>
> Cc: Mark Brown <broonie@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> spi@vger.kernel.org; saikrishna12468@gmail.com; Srinivas Goud
> <sgoud@xilinx.com>; Michal Simek <michals@xilinx.com>; Radhey Shyam
> Pandey <radheys@xilinx.com>
> Subject: RE: [PATCH 2/2] spi: cadence-quadspi: Add support for OSPI device
> reset
> 
> Hi Pratyush,
> 
> > -----Original Message-----
> > From: Pratyush Yadav <p.yadav@ti.com>
> > Sent: Wednesday, April 6, 2022 12:48 AM
> > To: Sai Krishna Potthuri <lakshmis@xilinx.com>
> > Cc: Mark Brown <broonie@kernel.org>; Rob Herring
> <robh+dt@kernel.org>;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > spi@vger.kernel.org; Michal Simek <michals@xilinx.com>; git
> > <git@xilinx.com>; saikrishna12468@gmail.com; Srinivas Goud
> > <sgoud@xilinx.com>
> > 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@xilinx.com>
> > [...]
> >
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.

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@ti.com>
> > Sent: Wednesday, April 6, 2022 12:48 AM
> > To: Sai Krishna Potthuri <lakshmis@xilinx.com>
> > Cc: Mark Brown <broonie@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > spi@vger.kernel.org; Michal Simek <michals@xilinx.com>; git
> > <git@xilinx.com>; saikrishna12468@gmail.com; Srinivas Goud
> > <sgoud@xilinx.com>
> > 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@xilinx.com>
> > [...]
> > 
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Potthuri, Sai Krishna]

Hi Pratyush,

> -----Original Message-----
> From: Pratyush Yadav <p.yadav@ti.com>
> Sent: Tuesday, June 21, 2022 2:47 PM
> To: Sai Krishna Potthuri <lakshmis@xilinx.com>
> Cc: Mark Brown <broonie@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> spi@vger.kernel.org; saikrishna12468@gmail.com; Srinivas Goud
> <sgoud@xilinx.com>; Michal Simek <michals@xilinx.com>; Radhey Shyam
> Pandey <radheys@xilinx.com>
> 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@ti.com>
> > > Sent: Wednesday, April 6, 2022 12:48 AM
> > > To: Sai Krishna Potthuri <lakshmis@xilinx.com>
> > > Cc: Mark Brown <broonie@kernel.org>; Rob Herring
> > > <robh+dt@kernel.org>; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux- spi@vger.kernel.org; Michal Simek
> > > <michals@xilinx.com>; git <git@xilinx.com>;
> > > saikrishna12468@gmail.com; Srinivas Goud <sgoud@xilinx.com>
> > > 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@seco.com/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@xilinx.com>
> > > [...]
> > >
> > > --
> > > Regards,
> > > Pratyush Yadav
> > > Texas Instruments Inc.
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.