[PATCH v6 0/6] spi: cadence-quadspi: Add QSPI controller support for Intel LGM SoC

[PATCH v6 0/6] spi: cadence-quadspi: Add QSPI controller support for Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

Add QSPI controller support for Intel LGM SoC.

Note from Vignesh(mtd subsystem maintainer):
This series is a subset of "[PATCH v12 0/4] spi: cadence-quadspi: Add
support for the Cadence QSPI controller" by Ramuthevar,Vadivel MuruganX
<vadivel.muruganx.ramuthevar@linux.intel.com> that intended to move
cadence-quadspi driver to spi-mem framework

Those patches were trying to accomplish too many things in a single set
of patches and need to split into smaller patches. This is reduced
version of above series.

Changes that are intended to make migration easy are split into separate
patches. Patches 1 to 3 drop features that cannot be supported under
spi-mem at the moment (backward compatibility is maintained).
Patch 4-5 are trivial cleanups. Patch 6 does the actual conversion to
spi-mem and patch 7 moves the driver to drivers/spi folder.

I have tested both INDAC mode (used by non TI platforms like Altera
SoCFPGA) and DAC mode (used by TI platforms) on TI EVMs.

Patches to move move bindings over to
"Documentation/devicetree/bindings/spi/" directory and also conversion
of bindig doc to YAML will be posted separately.  Support for Intel
platform would follow that.

Reference:
        https://lkml.org/lkml/2020/6/1/50

---
v6:
  - Rob's review comments update
  - add compatible string in properly aligned
  - remove cadence-qspi extra comaptible string in example
v5:
  - Rob's review comments update
  - const with single compatible string kept
v4:
  - Rob's review comments update
  - remove '|' no formatting to preserve
  - child node attributes follows under 'properties' under '@[0-9a-f]+$'.
v3:
  - Pratyush review comments update
  - CQSPI_SUPPORTS_MULTI_CHIPSELECT macro used instead of cqspi->use_direct_mode
  - disable DAC support placed in end of controller_init
v2:
  - Rob's review comments update for dt-bindings
  - add 'oneOf' for compatible selection
  - drop un-neccessary descriptions
  - add the cdns,is-decoded-cs and cdns,rclk-en properties as schema
  - remove 'allOf' in not required place
  - add AdditionalProperties false
  - add minItems/maxItems for qspi reset attributes

resend-v1:
  - As per Mark's suggestion , reorder the patch series 1-3 driver
    support patches, series 4-6 dt-bindings patches.
v1:
  - initial version



Ramuthevar Vadivel Murugan (6):
  spi: cadence-quadspi: Add QSPI support for Intel LGM SoC
  spi: cadence-quadspi: Disable the DAC for Intel LGM SoC
  spi: cadence-quadspi: Add multi-chipselect support for Intel LGM SoC
  spi: Move cadence-quadspi.txt to Documentation/devicetree/bindings/spi
  dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml
  dt-bindings: spi: Add compatible for Intel LGM SoC

 .../devicetree/bindings/mtd/cadence-quadspi.txt    |  67 ---------
 .../devicetree/bindings/spi/cadence-quadspi.yaml   | 150 +++++++++++++++++++++
 drivers/spi/Kconfig                                |   2 +-
 drivers/spi/spi-cadence-quadspi.c                  |  31 +++++
 4 files changed, 182 insertions(+), 68 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
 create mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.yaml

-- 
2.11.0

[PATCH v6 1/6] spi: cadence-quadspi: Add QSPI support for Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Add QSPI controller support for Intel LGM SoC.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/spi/Kconfig               | 2 +-
 drivers/spi/spi-cadence-quadspi.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index d2c976e55b8b..926da61eee5a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -203,7 +203,7 @@ config SPI_CADENCE
 
 config SPI_CADENCE_QUADSPI
 	tristate "Cadence Quad SPI controller"
-	depends on OF && (ARM || ARM64 || COMPILE_TEST)
+	depends on OF && (ARM || ARM64 || X86 || COMPILE_TEST)
 	help
 	  Enable support for the Cadence Quad SPI Flash controller.
 
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 40938cf3806d..d7b10c46fa70 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1401,6 +1401,9 @@ static const struct of_device_id cqspi_dt_ids[] = {
 		.compatible = "ti,am654-ospi",
 		.data = &am654_ospi,
 	},
+	{
+		.compatible = "intel,lgm-qspi",
+	},
 	{ /* end of table */ }
 };
 
-- 
2.11.0

[PATCH v6 2/6] spi: cadence-quadspi: Disable the DAC for Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use
Direct Access Controller(DAC).

This patch adds a quirk to disable the Direct Access Controller
for data transfer instead it uses indirect data transfer.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index d7b10c46fa70..6d6f7c440ece 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1107,6 +1107,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi)
 	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
 
 	cqspi_controller_enable(cqspi, 1);
+
+	/* Disable direct access controller */
+	if (!cqspi->use_direct_mode) {
+		reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
+		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
+		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
+	}
 }
 
 static int cqspi_request_mmap_dma(struct cqspi_st *cqspi)
@@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = {
 	.quirks = CQSPI_NEEDS_WR_DELAY,
 };
 
+static const struct cqspi_driver_platdata intel_lgm_qspi = {
+	.quirks = CQSPI_DISABLE_DAC_MODE,
+};
+
 static const struct of_device_id cqspi_dt_ids[] = {
 	{
 		.compatible = "cdns,qspi-nor",
@@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = {
 	},
 	{
 		.compatible = "intel,lgm-qspi",
+		.data = &intel_lgm_qspi,
 	},
 	{ /* end of table */ }
 };
-- 
2.11.0

[PATCH v6 3/6] spi: cadence-quadspi: Add multi-chipselect support for Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Add multiple chipselect support for Intel LGM SoCs,
currently QSPI-NOR and QSPI-NAND supported.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/spi/spi-cadence-quadspi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 6d6f7c440ece..c4440797db43 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -38,6 +38,7 @@
 
 /* Capabilities */
 #define CQSPI_SUPPORTS_OCTAL		BIT(0)
+#define CQSPI_SUPPORTS_MULTI_CHIPSELECT BIT(1)
 
 struct cqspi_st;
 
@@ -75,6 +76,7 @@ struct cqspi_st {
 	bool			is_decoded_cs;
 	u32			fifo_depth;
 	u32			fifo_width;
+	u32			num_chipselect;
 	bool			rclk_en;
 	u32			trigger_address;
 	u32			wr_delay;
@@ -1049,6 +1051,7 @@ static int cqspi_of_get_flash_pdata(struct platform_device *pdev,
 
 static int cqspi_of_get_pdata(struct cqspi_st *cqspi)
 {
+	const struct cqspi_driver_platdata *ddata;
 	struct device *dev = &cqspi->pdev->dev;
 	struct device_node *np = dev->of_node;
 
@@ -1070,6 +1073,15 @@ static int cqspi_of_get_pdata(struct cqspi_st *cqspi)
 		return -ENXIO;
 	}
 
+	ddata  = of_device_get_match_data(dev);
+	if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT) {
+		if (of_property_read_u32(np, "num-chipselect",
+					 &cqspi->num_chipselect)) {
+			dev_err(dev, "couldn't determine number of cs\n");
+			return -ENXIO;
+		}
+	}
+
 	cqspi->rclk_en = of_property_read_bool(np, "cdns,rclk-en");
 
 	return 0;
@@ -1307,6 +1319,9 @@ static int cqspi_probe(struct platform_device *pdev)
 	cqspi->current_cs = -1;
 	cqspi->sclk = 0;
 
+	if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT)
+		master->num_chipselect = cqspi->num_chipselect;
+
 	ret = cqspi_setup_flash(cqspi);
 	if (ret) {
 		dev_err(dev, "failed to setup flash parameters %d\n", ret);
@@ -1396,6 +1411,7 @@ static const struct cqspi_driver_platdata am654_ospi = {
 };
 
 static const struct cqspi_driver_platdata intel_lgm_qspi = {
+	.hwcaps_mask = CQSPI_SUPPORTS_MULTI_CHIPSELECT,
 	.quirks = CQSPI_DISABLE_DAC_MODE,
 };
 
-- 
2.11.0

[PATCH v6 4/6] spi: Move cadence-quadspi.txt to Documentation/devicetree/bindings/spi

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Move the Documentation/devicetree/bindings/mtd/cadence-quadspi.txt to
Documentation/devicetree/bindings/spi/

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/{mtd => spi}/cadence-quadspi.txt | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename Documentation/devicetree/bindings/{mtd => spi}/cadence-quadspi.txt (100%)

diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
similarity index 100%
rename from Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
rename to Documentation/devicetree/bindings/spi/cadence-quadspi.txt
-- 
2.11.0

[PATCH v6 5/6] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Convert the cadence-quadspi.txt documentation to cadence-quadspi.yaml
remove the cadence-quadspi.txt from Documentation/devicetree/bindings/spi/

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 .../devicetree/bindings/spi/cadence-quadspi.txt    |  67 ---------
 .../devicetree/bindings/spi/cadence-quadspi.yaml   | 149 +++++++++++++++++++++
 2 files changed, 149 insertions(+), 67 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.txt
 create mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.yaml

diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
deleted file mode 100644
index 945be7d5b236..000000000000
--- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
+++ /dev/null
@@ -1,67 +0,0 @@
-* Cadence Quad SPI controller
-
-Required properties:
-- compatible : should be one of the following:
-	Generic default - "cdns,qspi-nor".
-	For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
-	For TI AM654 SoC  - "ti,am654-ospi", "cdns,qspi-nor".
-- reg : Contains two entries, each of which is a tuple consisting of a
-	physical address and length. The first entry is the address and
-	length of the controller register set. The second entry is the
-	address and length of the QSPI Controller data area.
-- interrupts : Unit interrupt specifier for the controller interrupt.
-- clocks : phandle to the Quad SPI clock.
-- cdns,fifo-depth : Size of the data FIFO in words.
-- cdns,fifo-width : Bus width of the data FIFO in bytes.
-- cdns,trigger-address : 32-bit indirect AHB trigger address.
-
-Optional properties:
-- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
-- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch
-  the read data rather than the QSPI clock. Make sure that QSPI return
-  clock is populated on the board before using this property.
-
-Optional subnodes:
-Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
-custom properties:
-- cdns,read-delay : Delay for read capture logic, in clock cycles
-- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
-                  mode chip select outputs are de-asserted between
-		  transactions.
-- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
-                  de-activated and the activation of another.
-- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
-                  transaction and deasserting the device chip select
-		  (qspi_n_ss_out).
-- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
-                  and first bit transfer.
-- resets	: Must contain an entry for each entry in reset-names.
-		  See ../reset/reset.txt for details.
-- reset-names	: Must include either "qspi" and/or "qspi-ocp".
-
-Example:
-
-	qspi: spi@ff705000 {
-		compatible = "cdns,qspi-nor";
-		#address-cells = <1>;
-		#size-cells = <0>;
-		reg = <0xff705000 0x1000>,
-		      <0xffa00000 0x1000>;
-		interrupts = <0 151 4>;
-		clocks = <&qspi_clk>;
-		cdns,is-decoded-cs;
-		cdns,fifo-depth = <128>;
-		cdns,fifo-width = <4>;
-		cdns,trigger-address = <0x00000000>;
-		resets = <&rst QSPI_RESET>, <&rst QSPI_OCP_RESET>;
-		reset-names = "qspi", "qspi-ocp";
-
-		flash0: n25q00@0 {
-			...
-			cdns,read-delay = <4>;
-			cdns,tshsl-ns = <50>;
-			cdns,tsd2d-ns = <50>;
-			cdns,tchsh-ns = <4>;
-			cdns,tslch-ns = <4>;
-		};
-	};
diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
new file mode 100644
index 000000000000..ec22b040d804
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
@@ -0,0 +1,149 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/cadence-quadspi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence Quad SPI controller
+
+maintainers:
+  - Vadivel Murugan <vadivel.muruganx.ramuthevar@intel.com>
+
+allOf:
+  - $ref: "spi-controller.yaml#"
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - ti,k2g-qspi
+              - ti,am654-ospi
+          - const: cdns,qspi-nor
+
+  reg:
+    items:
+      - description: the controller register set
+      - description: the controller data area
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  cdns,fifo-depth:
+    description:
+      Size of the data FIFO in words.
+    $ref: "/schemas/types.yaml#/definitions/uint32"
+    enum: [ 128, 256 ]
+    default: 128
+
+  cdns,fifo-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Bus width of the data FIFO in bytes.
+    default: 4
+
+  cdns,trigger-address:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      32-bit indirect AHB trigger address.
+
+  cdns,is-decoded-cs:
+    type: boolean
+    description:
+      Flag to indicate whether decoder is used or not.
+
+  cdns,rclk-en:
+    type: boolean
+    description:
+      Flag to indicate that QSPI return clock is used to latch the read
+      data rather than the QSPI clock. Make sure that QSPI return clock
+      is populated on the board before using this property.
+
+  resets:
+    maxItems : 2
+
+  reset-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      enum: [ qspi, qspi-ocp ]
+
+# subnode's properties
+patternProperties:
+  "@[0-9a-f]+$":
+    type: object
+    description:
+      flash device uses the subnodes below defined properties.
+    properties:
+      cdns,read-delay:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          Delay for read capture logic, in clock cycles.
+
+      cdns,tshsl-ns:
+        description:
+          Delay in nanoseconds for the length that the master mode chip select
+          outputs are de-asserted between transactions.
+
+      cdns,tsd2d-ns:
+        description:
+          Delay in nanoseconds between one chip select being de-activated
+          and the activation of another.
+
+      cdns,tchsh-ns:
+        description:
+          Delay in nanoseconds between last bit of current transaction and
+          deasserting the device chip select (qspi_n_ss_out).
+
+      cdns,tslch-ns:
+        description:
+          Delay in nanoseconds between setting qspi_n_ss_out low and
+          first bit transfer.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - cdns,fifo-depth
+  - cdns,fifo-width
+  - cdns,trigger-address
+  - cdns,is-decoded-cs
+  - cdns,rclk-en
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+  - |
+    qspi: spi@ff705000 {
+      compatible = "cadence,qspi","cdns,qpsi-nor";
+      #address-cells = <1>;
+      #size-cells = <0>;
+      reg = <0xff705000 0x1000>,
+            <0xffa00000 0x1000>;
+      interrupts = <0 151 4>;
+      clocks = <&qspi_clk>;
+      cdns,fifo-depth = <128>;
+      cdns,fifo-width = <4>;
+      cdns,trigger-address = <0x00000000>;
+      resets = <&rst 0x1>, <&rst 0x2>;
+      reset-names = "qspi", "qspi-ocp";
+
+      flash@0 {
+              compatible = "jedec,spi-nor";
+              reg = <0x0>;
+              cdns,read-delay = <4>;
+              cdns,tshsl-ns = <50>;
+              cdns,tsd2d-ns = <50>;
+              cdns,tchsh-ns = <4>;
+              cdns,tslch-ns = <4>;
+     };
+
+    };
+
+...
-- 
2.11.0

[PATCH v6 6/6] dt-bindings: spi: Add compatible for Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Add compatible for Intel LGM SoC.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 Documentation/devicetree/bindings/spi/cadence-quadspi.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
index ec22b040d804..58ecdab939df 100644
--- a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
+++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
@@ -19,6 +19,7 @@ properties:
           - enum:
               - ti,k2g-qspi
               - ti,am654-ospi
+              - intel,lgm-qspi
           - const: cdns,qspi-nor
 
   reg:
-- 
2.11.0

Re: [PATCH v6 5/6] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml

[Rob Herring]

On Fri, Oct 30, 2020 at 01:31:52PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> Convert the cadence-quadspi.txt documentation to cadence-quadspi.yaml
> remove the cadence-quadspi.txt from Documentation/devicetree/bindings/spi/
> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> ---
>  .../devicetree/bindings/spi/cadence-quadspi.txt    |  67 ---------
>  .../devicetree/bindings/spi/cadence-quadspi.yaml   | 149 +++++++++++++++++++++
>  2 files changed, 149 insertions(+), 67 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.txt
>  create mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> deleted file mode 100644
> index 945be7d5b236..000000000000
> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -* Cadence Quad SPI controller
> -
> -Required properties:
> -- compatible : should be one of the following:
> -	Generic default - "cdns,qspi-nor".
> -	For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
> -	For TI AM654 SoC  - "ti,am654-ospi", "cdns,qspi-nor".
> -- reg : Contains two entries, each of which is a tuple consisting of a
> -	physical address and length. The first entry is the address and
> -	length of the controller register set. The second entry is the
> -	address and length of the QSPI Controller data area.
> -- interrupts : Unit interrupt specifier for the controller interrupt.
> -- clocks : phandle to the Quad SPI clock.
> -- cdns,fifo-depth : Size of the data FIFO in words.
> -- cdns,fifo-width : Bus width of the data FIFO in bytes.
> -- cdns,trigger-address : 32-bit indirect AHB trigger address.
> -
> -Optional properties:
> -- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
> -- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch
> -  the read data rather than the QSPI clock. Make sure that QSPI return
> -  clock is populated on the board before using this property.
> -
> -Optional subnodes:
> -Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
> -custom properties:
> -- cdns,read-delay : Delay for read capture logic, in clock cycles
> -- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
> -                  mode chip select outputs are de-asserted between
> -		  transactions.
> -- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
> -                  de-activated and the activation of another.
> -- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
> -                  transaction and deasserting the device chip select
> -		  (qspi_n_ss_out).
> -- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
> -                  and first bit transfer.
> -- resets	: Must contain an entry for each entry in reset-names.
> -		  See ../reset/reset.txt for details.
> -- reset-names	: Must include either "qspi" and/or "qspi-ocp".
> -
> -Example:
> -
> -	qspi: spi@ff705000 {
> -		compatible = "cdns,qspi-nor";
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -		reg = <0xff705000 0x1000>,
> -		      <0xffa00000 0x1000>;
> -		interrupts = <0 151 4>;
> -		clocks = <&qspi_clk>;
> -		cdns,is-decoded-cs;
> -		cdns,fifo-depth = <128>;
> -		cdns,fifo-width = <4>;
> -		cdns,trigger-address = <0x00000000>;
> -		resets = <&rst QSPI_RESET>, <&rst QSPI_OCP_RESET>;
> -		reset-names = "qspi", "qspi-ocp";
> -
> -		flash0: n25q00@0 {
> -			...
> -			cdns,read-delay = <4>;
> -			cdns,tshsl-ns = <50>;
> -			cdns,tsd2d-ns = <50>;
> -			cdns,tchsh-ns = <4>;
> -			cdns,tslch-ns = <4>;
> -		};
> -	};
> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> new file mode 100644
> index 000000000000..ec22b040d804
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/cadence-quadspi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cadence Quad SPI controller
> +
> +maintainers:
> +  - Vadivel Murugan <vadivel.muruganx.ramuthevar@intel.com>
> +
> +allOf:
> +  - $ref: "spi-controller.yaml#"
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:

You don't need 'oneOf' if there is only one entry...

So you've dropped 'cdns,qspi-nor' alone being valid. Granted, the txt 
file was fuzzy as to whether or not that was valid. So you have to look 
at all the dts files and see. I prefer we don't allow that and require a 
more specific compatible, but if there's a bunch then we should allow 
for it. The commit message should summarize what you decide.

> +          - enum:
> +              - ti,k2g-qspi
> +              - ti,am654-ospi
> +          - const: cdns,qspi-nor

> +examples:
> +  - |
> +    qspi: spi@ff705000 {
> +      compatible = "cadence,qspi","cdns,qpsi-nor";

And you missed fixing this.

> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      reg = <0xff705000 0x1000>,
> +            <0xffa00000 0x1000>;
> +      interrupts = <0 151 4>;
> +      clocks = <&qspi_clk>;
> +      cdns,fifo-depth = <128>;
> +      cdns,fifo-width = <4>;
> +      cdns,trigger-address = <0x00000000>;
> +      resets = <&rst 0x1>, <&rst 0x2>;
> +      reset-names = "qspi", "qspi-ocp";
> +
> +      flash@0 {
> +              compatible = "jedec,spi-nor";
> +              reg = <0x0>;
> +              cdns,read-delay = <4>;
> +              cdns,tshsl-ns = <50>;
> +              cdns,tsd2d-ns = <50>;
> +              cdns,tchsh-ns = <4>;
> +              cdns,tslch-ns = <4>;
> +     };
> +
> +    };
> +
> +...
> -- 
> 2.11.0
> 

Re: [PATCH v6 5/6] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

Thank you for the review comments...

On 30/10/2020 11:18 pm, Rob Herring wrote:
> On Fri, Oct 30, 2020 at 01:31:52PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>
>> Convert the cadence-quadspi.txt documentation to cadence-quadspi.yaml
>> remove the cadence-quadspi.txt from Documentation/devicetree/bindings/spi/
>>
>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> ---
>>   .../devicetree/bindings/spi/cadence-quadspi.txt    |  67 ---------
>>   .../devicetree/bindings/spi/cadence-quadspi.yaml   | 149 +++++++++++++++++++++
>>   2 files changed, 149 insertions(+), 67 deletions(-)
>>   delete mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.txt
>>   create mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
>> deleted file mode 100644
>> index 945be7d5b236..000000000000
>> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
>> +++ /dev/null
>> @@ -1,67 +0,0 @@
>> -* Cadence Quad SPI controller
>> -
>> -Required properties:
>> -- compatible : should be one of the following:
>> -	Generic default - "cdns,qspi-nor".
>> -	For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
>> -	For TI AM654 SoC  - "ti,am654-ospi", "cdns,qspi-nor".
>> -- reg : Contains two entries, each of which is a tuple consisting of a
>> -	physical address and length. The first entry is the address and
>> -	length of the controller register set. The second entry is the
>> -	address and length of the QSPI Controller data area.
>> -- interrupts : Unit interrupt specifier for the controller interrupt.
>> -- clocks : phandle to the Quad SPI clock.
>> -- cdns,fifo-depth : Size of the data FIFO in words.
>> -- cdns,fifo-width : Bus width of the data FIFO in bytes.
>> -- cdns,trigger-address : 32-bit indirect AHB trigger address.
>> -
>> -Optional properties:
>> -- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
>> -- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch
>> -  the read data rather than the QSPI clock. Make sure that QSPI return
>> -  clock is populated on the board before using this property.
>> -
>> -Optional subnodes:
>> -Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
>> -custom properties:
>> -- cdns,read-delay : Delay for read capture logic, in clock cycles
>> -- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
>> -                  mode chip select outputs are de-asserted between
>> -		  transactions.
>> -- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
>> -                  de-activated and the activation of another.
>> -- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
>> -                  transaction and deasserting the device chip select
>> -		  (qspi_n_ss_out).
>> -- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
>> -                  and first bit transfer.
>> -- resets	: Must contain an entry for each entry in reset-names.
>> -		  See ../reset/reset.txt for details.
>> -- reset-names	: Must include either "qspi" and/or "qspi-ocp".
>> -
>> -Example:
>> -
>> -	qspi: spi@ff705000 {
>> -		compatible = "cdns,qspi-nor";
>> -		#address-cells = <1>;
>> -		#size-cells = <0>;
>> -		reg = <0xff705000 0x1000>,
>> -		      <0xffa00000 0x1000>;
>> -		interrupts = <0 151 4>;
>> -		clocks = <&qspi_clk>;
>> -		cdns,is-decoded-cs;
>> -		cdns,fifo-depth = <128>;
>> -		cdns,fifo-width = <4>;
>> -		cdns,trigger-address = <0x00000000>;
>> -		resets = <&rst QSPI_RESET>, <&rst QSPI_OCP_RESET>;
>> -		reset-names = "qspi", "qspi-ocp";
>> -
>> -		flash0: n25q00@0 {
>> -			...
>> -			cdns,read-delay = <4>;
>> -			cdns,tshsl-ns = <50>;
>> -			cdns,tsd2d-ns = <50>;
>> -			cdns,tchsh-ns = <4>;
>> -			cdns,tslch-ns = <4>;
>> -		};
>> -	};
>> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>> new file mode 100644
>> index 000000000000..ec22b040d804
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>> @@ -0,0 +1,149 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/cadence-quadspi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Cadence Quad SPI controller
>> +
>> +maintainers:
>> +  - Vadivel Murugan <vadivel.muruganx.ramuthevar@intel.com>
>> +
>> +allOf:
>> +  - $ref: "spi-controller.yaml#"
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
> 
> You don't need 'oneOf' if there is only one entry...
> 
> So you've dropped 'cdns,qspi-nor' alone being valid. Granted, the txt
> file was fuzzy as to whether or not that was valid. So you have to look
> at all the dts files and see. I prefer we don't allow that and require a
> more specific compatible, but if there's a bunch then we should allow
> for it. The commit message should summarize what you decide.
we need bunch of compatibles as below, TI, Altera and Intel uses 
different compatible's so we added 'oneOf'.

cdns,qspi-nor can be dropped instead I can add cadence,qspi ,because 
this driver suuports qspi-nor and qspi-nand as well.

Sure, let me go through other documentation files for reference.

> 
>> +          - enum:
>> +              - ti,k2g-qspi
>> +              - ti,am654-ospi
>> +          - const: cdns,qspi-nor
> 
>> +examples:
>> +  - |
>> +    qspi: spi@ff705000 {
>> +      compatible = "cadence,qspi","cdns,qpsi-nor";
> 
> And you missed fixing this.
Yes, fixed by "cadence,qspi" keeping alone, need to remove 
cdns,qspi-nor, thanks!

Regards
Vadivel

> 
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      reg = <0xff705000 0x1000>,
>> +            <0xffa00000 0x1000>;
>> +      interrupts = <0 151 4>;
>> +      clocks = <&qspi_clk>;
>> +      cdns,fifo-depth = <128>;
>> +      cdns,fifo-width = <4>;
>> +      cdns,trigger-address = <0x00000000>;
>> +      resets = <&rst 0x1>, <&rst 0x2>;
>> +      reset-names = "qspi", "qspi-ocp";
>> +
>> +      flash@0 {
>> +              compatible = "jedec,spi-nor";
>> +              reg = <0x0>;
>> +              cdns,read-delay = <4>;
>> +              cdns,tshsl-ns = <50>;
>> +              cdns,tsd2d-ns = <50>;
>> +              cdns,tchsh-ns = <4>;
>> +              cdns,tslch-ns = <4>;
>> +     };
>> +
>> +    };
>> +
>> +...
>> -- 
>> 2.11.0
>>

Re: [PATCH v6 2/6] spi: cadence-quadspi: Disable the DAC for Intel LGM SoC

[Pratyush Yadav]

On 30/10/20 01:31PM, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use
> Direct Access Controller(DAC).
> 
> This patch adds a quirk to disable the Direct Access Controller
> for data transfer instead it uses indirect data transfer.
> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index d7b10c46fa70..6d6f7c440ece 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1107,6 +1107,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi)
>  	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
>  
>  	cqspi_controller_enable(cqspi, 1);
> +
> +	/* Disable direct access controller */
> +	if (!cqspi->use_direct_mode) {
> +		reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> +		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> +		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> +	}

You did not address my comment here from last time around [0]. Please 
replace this hunk with the one below and test it. Also mention in the 
commit message that the DAC bit resets to 1 so there is no need to 
explicitly set it.

--- 8< ---
diff --git a/drivers/spi/spi-cadence-quadspi.c 
b/drivers/spi/spi-cadence-quadspi.c
index d7ad8b198a11..d2c5d448a944 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -2156,10 +2156,12 @@ static void cqspi_controller_init(struct cqspi_st *cqspi)
 	writel(cqspi->fifo_depth * cqspi->fifo_width / 8,
 	       cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);
 
-	/* Enable Direct Access Controller */
-	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
-	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
-	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
+	/* Disable Direct Access Controller */
+	if (!cqspi->use_dac_mode) {
+		reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
+		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
+		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
+	}
 
 	cqspi_controller_enable(cqspi, 1);
 }
--- >8 ---

Same disclaimer as last time: not tested at all.

[0] https://lore.kernel.org/linux-spi/20201022090146.2uj5gfx73dsfumjl@ti.com/

PS: Please Cc me in the next revision. I missed 3 revisions in between 
because I'm not subscribed to this list. Otherwise I would have sent 
this much sooner :-)

>  }
>  
>  static int cqspi_request_mmap_dma(struct cqspi_st *cqspi)
> @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = {
>  	.quirks = CQSPI_NEEDS_WR_DELAY,
>  };
>  
> +static const struct cqspi_driver_platdata intel_lgm_qspi = {
> +	.quirks = CQSPI_DISABLE_DAC_MODE,
> +};
> +
>  static const struct of_device_id cqspi_dt_ids[] = {
>  	{
>  		.compatible = "cdns,qspi-nor",
> @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = {
>  	},
>  	{
>  		.compatible = "intel,lgm-qspi",
> +		.data = &intel_lgm_qspi,
>  	},
>  	{ /* end of table */ }
>  };
> -- 
> 2.11.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India

Re: [PATCH v6 2/6] spi: cadence-quadspi: Disable the DAC for Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Pratyush,

Thank you for the review comments...

On 4/11/2020 12:09 am, Pratyush Yadav wrote:
> On 30/10/20 01:31PM, Ramuthevar,Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>
>> On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use
>> Direct Access Controller(DAC).
>>
>> This patch adds a quirk to disable the Direct Access Controller
>> for data transfer instead it uses indirect data transfer.
>>
>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> ---
>>   drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
>> index d7b10c46fa70..6d6f7c440ece 100644
>> --- a/drivers/spi/spi-cadence-quadspi.c
>> +++ b/drivers/spi/spi-cadence-quadspi.c
>> @@ -1107,6 +1107,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi)
>>   	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
>>   
>>   	cqspi_controller_enable(cqspi, 1);
>> +
>> +	/* Disable direct access controller */
>> +	if (!cqspi->use_direct_mode) {
>> +		reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
>> +	}
> 
> You did not address my comment here from last time around [0]. Please
> replace this hunk with the one below and test it. Also mention in the
> commit message that the DAC bit resets to 1 so there is no need to
> explicitly set it.
Really sorry for that, I will add the below patch as you have suggested 
and test & confirm , thanks!
> 
> --- 8< ---
> diff --git a/drivers/spi/spi-cadence-quadspi.c
> b/drivers/spi/spi-cadence-quadspi.c
> index d7ad8b198a11..d2c5d448a944 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -2156,10 +2156,12 @@ static void cqspi_controller_init(struct cqspi_st *cqspi)
>   	writel(cqspi->fifo_depth * cqspi->fifo_width / 8,
>   	       cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);
>   
> -	/* Enable Direct Access Controller */
> -	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> -	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> -	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> +	/* Disable Direct Access Controller */
> +	if (!cqspi->use_dac_mode) {
> +		reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> +		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> +		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> +	}
>   
>   	cqspi_controller_enable(cqspi, 1);
>   }
> --- >8 ---
> 
> Same disclaimer as last time: not tested at all.
> 
> [0] https://lore.kernel.org/linux-spi/20201022090146.2uj5gfx73dsfumjl@ti.com/
> 
> PS: Please Cc me in the next revision. I missed 3 revisions in between
> because I'm not subscribed to this list. Otherwise I would have sent
> this much sooner :-)
Sure, I will add you in cc, btw last 3 revisions I did only Rob's review 
comments update w.r.t dt_schema.

Regards
Vadivel
> 
>>   }
>>   
>>   static int cqspi_request_mmap_dma(struct cqspi_st *cqspi)
>> @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = {
>>   	.quirks = CQSPI_NEEDS_WR_DELAY,
>>   };
>>   
>> +static const struct cqspi_driver_platdata intel_lgm_qspi = {
>> +	.quirks = CQSPI_DISABLE_DAC_MODE,
>> +};
>> +
>>   static const struct of_device_id cqspi_dt_ids[] = {
>>   	{
>>   		.compatible = "cdns,qspi-nor",
>> @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = {
>>   	},
>>   	{
>>   		.compatible = "intel,lgm-qspi",
>> +		.data = &intel_lgm_qspi,
>>   	},
>>   	{ /* end of table */ }
>>   };
>> -- 
>> 2.11.0
>>
> 

Re: [PATCH v6 5/6] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml

[Rob Herring]

On Mon, Nov 02, 2020 at 01:59:41PM +0800, Ramuthevar, Vadivel MuruganX wrote:
> Hi Rob,
> 
> Thank you for the review comments...
> 
> On 30/10/2020 11:18 pm, Rob Herring wrote:
> > On Fri, Oct 30, 2020 at 01:31:52PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> > > From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> > > 
> > > Convert the cadence-quadspi.txt documentation to cadence-quadspi.yaml
> > > remove the cadence-quadspi.txt from Documentation/devicetree/bindings/spi/
> > > 
> > > Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> > > ---
> > >   .../devicetree/bindings/spi/cadence-quadspi.txt    |  67 ---------
> > >   .../devicetree/bindings/spi/cadence-quadspi.yaml   | 149 +++++++++++++++++++++
> > >   2 files changed, 149 insertions(+), 67 deletions(-)
> > >   delete mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> > >   create mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> > > deleted file mode 100644
> > > index 945be7d5b236..000000000000
> > > --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> > > +++ /dev/null
> > > @@ -1,67 +0,0 @@
> > > -* Cadence Quad SPI controller
> > > -
> > > -Required properties:
> > > -- compatible : should be one of the following:
> > > -	Generic default - "cdns,qspi-nor".
> > > -	For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
> > > -	For TI AM654 SoC  - "ti,am654-ospi", "cdns,qspi-nor".
> > > -- reg : Contains two entries, each of which is a tuple consisting of a
> > > -	physical address and length. The first entry is the address and
> > > -	length of the controller register set. The second entry is the
> > > -	address and length of the QSPI Controller data area.
> > > -- interrupts : Unit interrupt specifier for the controller interrupt.
> > > -- clocks : phandle to the Quad SPI clock.
> > > -- cdns,fifo-depth : Size of the data FIFO in words.
> > > -- cdns,fifo-width : Bus width of the data FIFO in bytes.
> > > -- cdns,trigger-address : 32-bit indirect AHB trigger address.
> > > -
> > > -Optional properties:
> > > -- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
> > > -- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch
> > > -  the read data rather than the QSPI clock. Make sure that QSPI return
> > > -  clock is populated on the board before using this property.
> > > -
> > > -Optional subnodes:
> > > -Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
> > > -custom properties:
> > > -- cdns,read-delay : Delay for read capture logic, in clock cycles
> > > -- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
> > > -                  mode chip select outputs are de-asserted between
> > > -		  transactions.
> > > -- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
> > > -                  de-activated and the activation of another.
> > > -- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
> > > -                  transaction and deasserting the device chip select
> > > -		  (qspi_n_ss_out).
> > > -- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
> > > -                  and first bit transfer.
> > > -- resets	: Must contain an entry for each entry in reset-names.
> > > -		  See ../reset/reset.txt for details.
> > > -- reset-names	: Must include either "qspi" and/or "qspi-ocp".
> > > -
> > > -Example:
> > > -
> > > -	qspi: spi@ff705000 {
> > > -		compatible = "cdns,qspi-nor";
> > > -		#address-cells = <1>;
> > > -		#size-cells = <0>;
> > > -		reg = <0xff705000 0x1000>,
> > > -		      <0xffa00000 0x1000>;
> > > -		interrupts = <0 151 4>;
> > > -		clocks = <&qspi_clk>;
> > > -		cdns,is-decoded-cs;
> > > -		cdns,fifo-depth = <128>;
> > > -		cdns,fifo-width = <4>;
> > > -		cdns,trigger-address = <0x00000000>;
> > > -		resets = <&rst QSPI_RESET>, <&rst QSPI_OCP_RESET>;
> > > -		reset-names = "qspi", "qspi-ocp";
> > > -
> > > -		flash0: n25q00@0 {
> > > -			...
> > > -			cdns,read-delay = <4>;
> > > -			cdns,tshsl-ns = <50>;
> > > -			cdns,tsd2d-ns = <50>;
> > > -			cdns,tchsh-ns = <4>;
> > > -			cdns,tslch-ns = <4>;
> > > -		};
> > > -	};
> > > diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> > > new file mode 100644
> > > index 000000000000..ec22b040d804
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> > > @@ -0,0 +1,149 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/spi/cadence-quadspi.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Cadence Quad SPI controller
> > > +
> > > +maintainers:
> > > +  - Vadivel Murugan <vadivel.muruganx.ramuthevar@intel.com>
> > > +
> > > +allOf:
> > > +  - $ref: "spi-controller.yaml#"
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - items:
> > 
> > You don't need 'oneOf' if there is only one entry...
> > 
> > So you've dropped 'cdns,qspi-nor' alone being valid. Granted, the txt
> > file was fuzzy as to whether or not that was valid. So you have to look
> > at all the dts files and see. I prefer we don't allow that and require a
> > more specific compatible, but if there's a bunch then we should allow
> > for it. The commit message should summarize what you decide.
> we need bunch of compatibles as below, TI, Altera and Intel uses different
> compatible's so we added 'oneOf'.

Then you add oneOf when you need it. You don't for what you wrote, 
but once it is correct you will as Altera uses 'cdns,qspi-nor' alone. 

> cdns,qspi-nor can be dropped instead I can add cadence,qspi ,because this
> driver suuports qspi-nor and qspi-nand as well.

No, you can't change it because it is an ABI.

> 
> Sure, let me go through other documentation files for reference.
> 
> > 
> > > +          - enum:
> > > +              - ti,k2g-qspi
> > > +              - ti,am654-ospi
> > > +          - const: cdns,qspi-nor
> > 
> > > +examples:
> > > +  - |
> > > +    qspi: spi@ff705000 {
> > > +      compatible = "cadence,qspi","cdns,qpsi-nor";
> > 
> > And you missed fixing this.
> Yes, fixed by "cadence,qspi" keeping alone, need to remove cdns,qspi-nor,
> thanks!

Nope!

Rob

Re: [PATCH v6 6/6] dt-bindings: spi: Add compatible for Intel LGM SoC

[Rob Herring]

On Fri, Oct 30, 2020 at 01:31:53PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> Add compatible for Intel LGM SoC.
> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> ---
>  Documentation/devicetree/bindings/spi/cadence-quadspi.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> index ec22b040d804..58ecdab939df 100644
> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> @@ -19,6 +19,7 @@ properties:
>            - enum:
>                - ti,k2g-qspi
>                - ti,am654-ospi
> +              - intel,lgm-qspi

As this change shows, you don't need 'oneOf' for Intel...

>            - const: cdns,qspi-nor
>  
>    reg:
> -- 
> 2.11.0
> 

Re: [PATCH v6 3/6] spi: cadence-quadspi: Add multi-chipselect support for Intel LGM SoC

[Linus Walleij]

On Fri, Oct 30, 2020 at 6:32 AM Ramuthevar,Vadivel MuruganX
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> +       ddata  = of_device_get_match_data(dev);
> +       if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT) {
> +               if (of_property_read_u32(np, "num-chipselect",

The standard SPI bindings in spi-controller.yaml already has a binding
for this "num-cs" so please use that. It is also what your device tree
binding is referencing, so if you were using "num-chipselect" the
YAML check should give a warning?

Yours,
Linus Walleij

Re: [PATCH v6 3/6] spi: cadence-quadspi: Add multi-chipselect support for Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Linus,

Thank you for the review comments...

On 5/11/2020 3:11 pm, Linus Walleij wrote:
> On Fri, Oct 30, 2020 at 6:32 AM Ramuthevar,Vadivel MuruganX
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> +       ddata  = of_device_get_match_data(dev);
>> +       if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT) {
>> +               if (of_property_read_u32(np, "num-chipselect",
> 
> The standard SPI bindings in spi-controller.yaml already has a binding
> for this "num-cs" so please use that. It is also what your device tree
> binding is referencing, 
yes, you are point is valid, I will use that.
so if you were using "num-chipselect" the
> YAML check should give a warning?
In the example, I just converted from existing txt -to- yaml that's
why I didnt face any problem.

Regards
Vadivel
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH v6 5/6] dt-bindings: spi: Convert cadence-quadspi.txt to cadence-quadspi.yaml

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

On 5/11/2020 6:02 am, Rob Herring wrote:
> On Mon, Nov 02, 2020 at 01:59:41PM +0800, Ramuthevar, Vadivel MuruganX wrote:
>> Hi Rob,
>>
>> Thank you for the review comments...
>>
>> On 30/10/2020 11:18 pm, Rob Herring wrote:
>>> On Fri, Oct 30, 2020 at 01:31:52PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>
>>>> Convert the cadence-quadspi.txt documentation to cadence-quadspi.yaml
>>>> remove the cadence-quadspi.txt from Documentation/devicetree/bindings/spi/
>>>>
>>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>> ---
>>>>    .../devicetree/bindings/spi/cadence-quadspi.txt    |  67 ---------
>>>>    .../devicetree/bindings/spi/cadence-quadspi.yaml   | 149 +++++++++++++++++++++
>>>>    2 files changed, 149 insertions(+), 67 deletions(-)
>>>>    delete mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.txt
>>>>    create mode 100644 Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
>>>> deleted file mode 100644
>>>> index 945be7d5b236..000000000000
>>>> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
>>>> +++ /dev/null
>>>> @@ -1,67 +0,0 @@
>>>> -* Cadence Quad SPI controller
>>>> -
>>>> -Required properties:
>>>> -- compatible : should be one of the following:
>>>> -	Generic default - "cdns,qspi-nor".
>>>> -	For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
>>>> -	For TI AM654 SoC  - "ti,am654-ospi", "cdns,qspi-nor".
>>>> -- reg : Contains two entries, each of which is a tuple consisting of a
>>>> -	physical address and length. The first entry is the address and
>>>> -	length of the controller register set. The second entry is the
>>>> -	address and length of the QSPI Controller data area.
>>>> -- interrupts : Unit interrupt specifier for the controller interrupt.
>>>> -- clocks : phandle to the Quad SPI clock.
>>>> -- cdns,fifo-depth : Size of the data FIFO in words.
>>>> -- cdns,fifo-width : Bus width of the data FIFO in bytes.
>>>> -- cdns,trigger-address : 32-bit indirect AHB trigger address.
>>>> -
>>>> -Optional properties:
>>>> -- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
>>>> -- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch
>>>> -  the read data rather than the QSPI clock. Make sure that QSPI return
>>>> -  clock is populated on the board before using this property.
>>>> -
>>>> -Optional subnodes:
>>>> -Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
>>>> -custom properties:
>>>> -- cdns,read-delay : Delay for read capture logic, in clock cycles
>>>> -- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
>>>> -                  mode chip select outputs are de-asserted between
>>>> -		  transactions.
>>>> -- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
>>>> -                  de-activated and the activation of another.
>>>> -- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
>>>> -                  transaction and deasserting the device chip select
>>>> -		  (qspi_n_ss_out).
>>>> -- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
>>>> -                  and first bit transfer.
>>>> -- resets	: Must contain an entry for each entry in reset-names.
>>>> -		  See ../reset/reset.txt for details.
>>>> -- reset-names	: Must include either "qspi" and/or "qspi-ocp".
>>>> -
>>>> -Example:
>>>> -
>>>> -	qspi: spi@ff705000 {
>>>> -		compatible = "cdns,qspi-nor";
>>>> -		#address-cells = <1>;
>>>> -		#size-cells = <0>;
>>>> -		reg = <0xff705000 0x1000>,
>>>> -		      <0xffa00000 0x1000>;
>>>> -		interrupts = <0 151 4>;
>>>> -		clocks = <&qspi_clk>;
>>>> -		cdns,is-decoded-cs;
>>>> -		cdns,fifo-depth = <128>;
>>>> -		cdns,fifo-width = <4>;
>>>> -		cdns,trigger-address = <0x00000000>;
>>>> -		resets = <&rst QSPI_RESET>, <&rst QSPI_OCP_RESET>;
>>>> -		reset-names = "qspi", "qspi-ocp";
>>>> -
>>>> -		flash0: n25q00@0 {
>>>> -			...
>>>> -			cdns,read-delay = <4>;
>>>> -			cdns,tshsl-ns = <50>;
>>>> -			cdns,tsd2d-ns = <50>;
>>>> -			cdns,tchsh-ns = <4>;
>>>> -			cdns,tslch-ns = <4>;
>>>> -		};
>>>> -	};
>>>> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>>>> new file mode 100644
>>>> index 000000000000..ec22b040d804
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>>>> @@ -0,0 +1,149 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/spi/cadence-quadspi.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Cadence Quad SPI controller
>>>> +
>>>> +maintainers:
>>>> +  - Vadivel Murugan <vadivel.muruganx.ramuthevar@intel.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: "spi-controller.yaml#"
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - items:
>>>
>>> You don't need 'oneOf' if there is only one entry...
>>>
>>> So you've dropped 'cdns,qspi-nor' alone being valid. Granted, the txt
>>> file was fuzzy as to whether or not that was valid. So you have to look
>>> at all the dts files and see. I prefer we don't allow that and require a
>>> more specific compatible, but if there's a bunch then we should allow
>>> for it. The commit message should summarize what you decide.
>> we need bunch of compatibles as below, TI, Altera and Intel uses different
>> compatible's so we added 'oneOf'.
> 
> Then you add oneOf when you need it. You don't for what you wrote,
> but once it is correct you will as Altera uses 'cdns,qspi-nor' alone.
Yes, yo're right , we need oneOf in the case of adding 'cadence,qspi' 
and 'cdns,qspi-nor' two different group of items.
> 
>> cdns,qspi-nor can be dropped instead I can add cadence,qspi ,because this
>> driver suuports qspi-nor and qspi-nand as well.
> 
> No, you can't change it because it is an ABI.
Ok, Got it, thanks!

Regards
Vadivel
> 
>>
>> Sure, let me go through other documentation files for reference.
>>
>>>
>>>> +          - enum:
>>>> +              - ti,k2g-qspi
>>>> +              - ti,am654-ospi
>>>> +          - const: cdns,qspi-nor
>>>
>>>> +examples:
>>>> +  - |
>>>> +    qspi: spi@ff705000 {
>>>> +      compatible = "cadence,qspi","cdns,qpsi-nor";
>>>
>>> And you missed fixing this.
>> Yes, fixed by "cadence,qspi" keeping alone, need to remove cdns,qspi-nor,
>> thanks!
> 
> Nope!
> 
> Rob
> 

Re: [PATCH v6 6/6] dt-bindings: spi: Add compatible for Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

On 5/11/2020 6:03 am, Rob Herring wrote:
> On Fri, Oct 30, 2020 at 01:31:53PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>
>> Add compatible for Intel LGM SoC.
>>
>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> ---
>>   Documentation/devicetree/bindings/spi/cadence-quadspi.yaml | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>> index ec22b040d804..58ecdab939df 100644
>> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>> @@ -19,6 +19,7 @@ properties:
>>             - enum:
>>                 - ti,k2g-qspi
>>                 - ti,am654-ospi
>> +              - intel,lgm-qspi
> 
> As this change shows, you don't need 'oneOf' for Intel...
As we you have suggested in the previous mail, I framed like below with 
'oneOf'

properties:
   compatible:
     oneOf:
       - items:
           - enum:
               - ti,k2g-qspi
               - ti,am654-ospi
           - const: cdns,qspi-nor

       - items:
           - enum:
               - intel,lgm-qspi
               - cadence,qspi   #compatible for generic in future use
           - const: cdns,qspi-nor

so that ignoring error message warning can be avoided as well, Thanks!


Regards
Vadivel

> 
>>             - const: cdns,qspi-nor
>>   
>>     reg:
>> -- 
>> 2.11.0
>>

Re: [PATCH v6 6/6] dt-bindings: spi: Add compatible for Intel LGM SoC

[Rob Herring]

On Sun, Nov 8, 2020 at 7:49 PM Ramuthevar, Vadivel MuruganX
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>
> Hi Rob,
>
> On 5/11/2020 6:03 am, Rob Herring wrote:
> > On Fri, Oct 30, 2020 at 01:31:53PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> >> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >>
> >> Add compatible for Intel LGM SoC.
> >>
> >> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> >> ---
> >>   Documentation/devicetree/bindings/spi/cadence-quadspi.yaml | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> >> index ec22b040d804..58ecdab939df 100644
> >> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> >> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
> >> @@ -19,6 +19,7 @@ properties:
> >>             - enum:
> >>                 - ti,k2g-qspi
> >>                 - ti,am654-ospi
> >> +              - intel,lgm-qspi
> >
> > As this change shows, you don't need 'oneOf' for Intel...
> As we you have suggested in the previous mail, I framed like below with
> 'oneOf'
>
> properties:
>    compatible:
>      oneOf:
>        - items:
>            - enum:
>                - ti,k2g-qspi
>                - ti,am654-ospi
>            - const: cdns,qspi-nor
>
>        - items:
>            - enum:
>                - intel,lgm-qspi
>                - cadence,qspi   #compatible for generic in future use

Why are you not using the documented vendor prefix 'cdns'?

In any case, adding this is pointless. Your 'generic' compatible is below.

And you still don't need 'oneOf' here. The enum contents here can be
in the first 'enum'.

>            - const: cdns,qspi-nor
>
> so that ignoring error message warning can be avoided as well, Thanks!

In the example? Fix the example!


Rob

Re: [PATCH v6 6/6] dt-bindings: spi: Add compatible for Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

On 9/11/2020 11:15 pm, Rob Herring wrote:
> On Sun, Nov 8, 2020 at 7:49 PM Ramuthevar, Vadivel MuruganX
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>
>> Hi Rob,
>>
>> On 5/11/2020 6:03 am, Rob Herring wrote:
>>> On Fri, Oct 30, 2020 at 01:31:53PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>
>>>> Add compatible for Intel LGM SoC.
>>>>
>>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/spi/cadence-quadspi.yaml | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>>>> index ec22b040d804..58ecdab939df 100644
>>>> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.yaml
>>>> @@ -19,6 +19,7 @@ properties:
>>>>              - enum:
>>>>                  - ti,k2g-qspi
>>>>                  - ti,am654-ospi
>>>> +              - intel,lgm-qspi
>>>
>>> As this change shows, you don't need 'oneOf' for Intel...
>> As we you have suggested in the previous mail, I framed like below with
>> 'oneOf'
>>
>> properties:
>>     compatible:
>>       oneOf:
>>         - items:
>>             - enum:
>>                 - ti,k2g-qspi
>>                 - ti,am654-ospi
>>             - const: cdns,qspi-nor
>>
>>         - items:
>>             - enum:
>>                 - intel,lgm-qspi
>>                 - cadence,qspi   #compatible for generic in future use
> 
> Why are you not using the documented vendor prefix 'cdns'?
old document file name is cadence-quadspi.txt, so thought of keeping the 
same name.
Thank you for the suggestion, Sure, I will use it.
> 
> In any case, adding this is pointless. Your 'generic' compatible is below.
> 
> And you still don't need 'oneOf' here. The enum contents here can be
> in the first 'enum'.
Ok, Noted.

Regards
Vadivel
> 
>>             - const: cdns,qspi-nor
>>
>> so that ignoring error message warning can be avoided as well, Thanks!
> 
> In the example? Fix the example!
> 
> 
> Rob
>