devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
@ 2021-12-03 18:17 Dinh Nguyen
  2021-12-03 18:17 ` [PATCHv2 2/3] ARM: dts: socfpga: change qspi to "cdns,qspi-nor-ver-00-10" Dinh Nguyen
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-03 18:17 UTC (permalink / raw)
  To: devicetree; +Cc: dinguyen, broonie, robh+dt, p.yadav

The QSPI controller on Intel's SoCFPGA platform does not implement the
CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
results in a crash.

The module/revision ID is written in the MODULE_ID register. For this
variance, bits 23-8 is 0x0010.

Introduce the dts binding "cdns,qspi-nor-ver-00-10" to differentiate the
hardware.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v2: change binding to "cdns,qspi-nor-0010" to be more generic for other
    platforms
---
 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index ca155abbda7a..2833e1c8841d 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -29,6 +29,7 @@ properties:
               - ti,am654-ospi
               - intel,lgm-qspi
               - xlnx,versal-ospi-1.0
+              - cdns,qspi-nor-ver-00-10
           - const: cdns,qspi-nor
       - const: cdns,qspi-nor
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCHv2 2/3] ARM: dts: socfpga: change qspi to "cdns,qspi-nor-ver-00-10"
  2021-12-03 18:17 [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Dinh Nguyen
@ 2021-12-03 18:17 ` Dinh Nguyen
  2021-12-03 18:17 ` [PATCH 3/3] spi: cadence-quadspi: change socfpga-qspi " Dinh Nguyen
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-03 18:17 UTC (permalink / raw)
  To: devicetree; +Cc: dinguyen, broonie, robh+dt, p.yadav

Because of commit 9cb2ff111712 ("spi: cadence-quadspi: Disable Auto-HW polling"),
which does a write to the CQSPI_REG_WR_COMPLETION_CTRL register
regardless of any condition. Well, the Cadence QuadSPI controller on
Intel's SoCFPGA platforms does not implement the
CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
results in a crash!

So starting with v5.16, I introduced the patch
98d948eb833 ("spi: cadence-quadspi: fix write completion support"),
which adds the dts property "cdns,qspi-nor-ver-00-10" that is specific for
versions that doesn't have the CQSPI_REG_WR_COMPLETION_CTRL register implemented.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v2: use both "cdns,qspi-nor" and "cdns,qspi-nor-0010"
---
 arch/arm/boot/dts/socfpga.dtsi                    | 2 +-
 arch/arm/boot/dts/socfpga_arria10.dtsi            | 2 +-
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 2 +-
 arch/arm64/boot/dts/intel/socfpga_agilex.dtsi     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 0b021eef0b53..c1f85d57ee96 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -782,7 +782,7 @@ ocram: sram@ffff0000 {
 		};
 
 		qspi: spi@ff705000 {
-			compatible = "cdns,qspi-nor";
+			compatible = "cdns,qspi-nor-ver-00-10", "cdns,qspi-nor";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xff705000 0x1000>,
diff --git a/arch/arm/boot/dts/socfpga_arria10.dtsi b/arch/arm/boot/dts/socfpga_arria10.dtsi
index a574ea91d9d3..98166ad0b098 100644
--- a/arch/arm/boot/dts/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10.dtsi
@@ -756,7 +756,7 @@ usb0-ecc@ff8c8800 {
 		};
 
 		qspi: spi@ff809000 {
-			compatible = "cdns,qspi-nor";
+			compatible = "cdns,qspi-nor-ver-00-10", "cdns,qspi-nor";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xff809000 0x100>,
diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index d301ac0d406b..aac29503a3a3 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -594,7 +594,7 @@ emac0-tx-ecc@ff8c0400 {
 		};
 
 		qspi: spi@ff8d2000 {
-			compatible = "cdns,qspi-nor";
+			compatible =  "cdns,qspi-nor-ver-00-10", "cdns,qspi-nor";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xff8d2000 0x100>,
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
index 163f33b46e4f..c72d5c9253fb 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
@@ -628,7 +628,7 @@ sdmmca-ecc@ff8c8c00 {
 		};
 
 		qspi: spi@ff8d2000 {
-			compatible = "cdns,qspi-nor";
+			compatible = "cdns,qspi-nor-ver-00-10", "cdns,qspi-nor";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xff8d2000 0x100>,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] spi: cadence-quadspi: change socfpga-qspi to "cdns,qspi-nor-ver-00-10"
  2021-12-03 18:17 [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Dinh Nguyen
  2021-12-03 18:17 ` [PATCHv2 2/3] ARM: dts: socfpga: change qspi to "cdns,qspi-nor-ver-00-10" Dinh Nguyen
@ 2021-12-03 18:17 ` Dinh Nguyen
  2021-12-03 18:18 ` [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Mark Brown
  2021-12-06 10:22 ` Pratyush Yadav
  3 siblings, 0 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-03 18:17 UTC (permalink / raw)
  To: devicetree; +Cc: dinguyen, broonie, robh+dt, p.yadav

To make the "intel,socfpga-qspi" more generic, change the binding to
"cdns,qspi-nor-ver-00-10". The "0010" represents the Module/Revision ID
number that is in the MODULE_ID register.

Fixes: f0234e62e4 ("spi: cadence-quadspi: fix write completion support")
Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 drivers/spi/spi-cadence-quadspi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index b808c94641fa..b1421bcce67f 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1869,7 +1869,7 @@ static const struct cqspi_driver_platdata intel_lgm_qspi = {
 	.quirks = CQSPI_DISABLE_DAC_MODE,
 };
 
-static const struct cqspi_driver_platdata socfpga_qspi = {
+static const struct cqspi_driver_platdata cdns_qspi_0010 = {
 	.quirks = CQSPI_NO_SUPPORT_WR_COMPLETION,
 };
 
@@ -1902,8 +1902,8 @@ static const struct of_device_id cqspi_dt_ids[] = {
 		.data = (void *)&versal_ospi,
 	},
 	{
-		.compatible = "intel,socfpga-qspi",
-		.data = (void *)&socfpga_qspi,
+		.compatible = "cdns,qspi-nor-ver-00-10",
+		.data = (void *)&cdns_qspi_0010,
 	},
 	{ /* end of table */ }
 };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-03 18:17 [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Dinh Nguyen
  2021-12-03 18:17 ` [PATCHv2 2/3] ARM: dts: socfpga: change qspi to "cdns,qspi-nor-ver-00-10" Dinh Nguyen
  2021-12-03 18:17 ` [PATCH 3/3] spi: cadence-quadspi: change socfpga-qspi " Dinh Nguyen
@ 2021-12-03 18:18 ` Mark Brown
  2021-12-03 19:05   ` Dinh Nguyen
  2021-12-06 10:22 ` Pratyush Yadav
  3 siblings, 1 reply; 14+ messages in thread
From: Mark Brown @ 2021-12-03 18:18 UTC (permalink / raw)
  To: Dinh Nguyen; +Cc: devicetree, robh+dt, p.yadav

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

On Fri, Dec 03, 2021 at 12:17:12PM -0600, Dinh Nguyen wrote:
> The QSPI controller on Intel's SoCFPGA platform does not implement the
> CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
> results in a crash.

This is the third copy of this you've sent today with no differences
I've spotted between them - what's the story here?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-03 18:18 ` [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Mark Brown
@ 2021-12-03 19:05   ` Dinh Nguyen
  0 siblings, 0 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-03 19:05 UTC (permalink / raw)
  To: Mark Brown; +Cc: devicetree, robh+dt, p.yadav



On 12/3/21 12:18 PM, Mark Brown wrote:
> On Fri, Dec 03, 2021 at 12:17:12PM -0600, Dinh Nguyen wrote:
>> The QSPI controller on Intel's SoCFPGA platform does not implement the
>> CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
>> results in a crash.
> 
> This is the third copy of this you've sent today with no differences
> I've spotted between them - what's the story here?
> 

Apologies, I didn't get a confirmation that the 1st 2 copies went 
through. Please ignore the 1st and 2nd copies.

Sorry for the noise!

Dinh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-03 18:17 [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Dinh Nguyen
                   ` (2 preceding siblings ...)
  2021-12-03 18:18 ` [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Mark Brown
@ 2021-12-06 10:22 ` Pratyush Yadav
  2021-12-08 23:45   ` Dinh Nguyen
  3 siblings, 1 reply; 14+ messages in thread
From: Pratyush Yadav @ 2021-12-06 10:22 UTC (permalink / raw)
  To: Dinh Nguyen; +Cc: devicetree, broonie, robh+dt

On 03/12/21 12:17PM, Dinh Nguyen wrote:
> The QSPI controller on Intel's SoCFPGA platform does not implement the
> CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
> results in a crash.
> 
> The module/revision ID is written in the MODULE_ID register. For this
> variance, bits 23-8 is 0x0010.

When I looked at your original patches I was under the impression that 
this was a SoCFPGA specific thing and did not apply to other 
implementation of the IP in general.

If this is indeed a generic thing and we can detect it via the MODULE_ID 
register [0], then why not just read that register at probe time and 
apply this quirk based on the ID? Why then do we need a separate 
compatible at all?

[0] I would like to see it stated explicitly somewhere that version 
0x0010 does not support the WR_COMPLETION_CTRL register.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-06 10:22 ` Pratyush Yadav
@ 2021-12-08 23:45   ` Dinh Nguyen
  2021-12-13 20:48     ` Rob Herring
  2021-12-14 20:05     ` Pratyush Yadav
  0 siblings, 2 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-08 23:45 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Dinh Nguyen, devicetree, broonie, robh+dt

On Mon, Dec 6, 2021 at 9:51 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> On 03/12/21 12:17PM, Dinh Nguyen wrote:
> > The QSPI controller on Intel's SoCFPGA platform does not implement the
> > CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
> > results in a crash.
> >
> > The module/revision ID is written in the MODULE_ID register. For this
> > variance, bits 23-8 is 0x0010.
>
> When I looked at your original patches I was under the impression that
> this was a SoCFPGA specific thing and did not apply to other
> implementation of the IP in general.
>
> If this is indeed a generic thing and we can detect it via the MODULE_ID
> register [0], then why not just read that register at probe time and
> apply this quirk based on the ID? Why then do we need a separate
> compatible at all?
>
> [0] I would like to see it stated explicitly somewhere that version
> 0x0010 does not support the WR_COMPLETION_CTRL register.
>

I cannot for sure confirm that this condition applies to only 0x0010
version of the
IP. I can verify that the IP that is in all 3 generations of SoCFPGA
devices, all have
MODULE_ID value of 0x0010 and all do not have the WR_COMPLETION_CTRL
register implemented.

I'm almost certain this feature is not SoCFPGA specific, but
since I only had SoCFPGA hardware, that was my initial patch. I made the mistake
of not CC'ing the devicetree maintainers until I sent the DTS binding
patch change,
and he rightly suggested making the binding something more generic.

I do like your idea of making a determination in the driver without
being dependent
on a dts binding. I'd like to know the impetus behind your original
patch of removing the
dependency of "if (f_pdata->dtr)"  for the write to the WR_COMPLETION_CTRL
register? Perhaps there's some other common property that we can key
off for why the register
is not implemented?

Dinh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-08 23:45   ` Dinh Nguyen
@ 2021-12-13 20:48     ` Rob Herring
  2021-12-13 21:21       ` Dinh Nguyen
  2021-12-14 20:05     ` Pratyush Yadav
  1 sibling, 1 reply; 14+ messages in thread
From: Rob Herring @ 2021-12-13 20:48 UTC (permalink / raw)
  To: Dinh Nguyen; +Cc: Pratyush Yadav, Dinh Nguyen, devicetree, broonie

On Wed, Dec 08, 2021 at 05:45:31PM -0600, Dinh Nguyen wrote:
> On Mon, Dec 6, 2021 at 9:51 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > On 03/12/21 12:17PM, Dinh Nguyen wrote:
> > > The QSPI controller on Intel's SoCFPGA platform does not implement the
> > > CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
> > > results in a crash.
> > >
> > > The module/revision ID is written in the MODULE_ID register. For this
> > > variance, bits 23-8 is 0x0010.
> >
> > When I looked at your original patches I was under the impression that
> > this was a SoCFPGA specific thing and did not apply to other
> > implementation of the IP in general.
> >
> > If this is indeed a generic thing and we can detect it via the MODULE_ID
> > register [0], then why not just read that register at probe time and
> > apply this quirk based on the ID? Why then do we need a separate
> > compatible at all?
> >
> > [0] I would like to see it stated explicitly somewhere that version
> > 0x0010 does not support the WR_COMPLETION_CTRL register.
> >
> 
> I cannot for sure confirm that this condition applies to only 0x0010
> version of the
> IP. I can verify that the IP that is in all 3 generations of SoCFPGA
> devices, all have
> MODULE_ID value of 0x0010 and all do not have the WR_COMPLETION_CTRL
> register implemented.

Then for the same reason, you shouldn't be trying to have a 'generic' 
compatible.

> 
> I'm almost certain this feature is not SoCFPGA specific, but
> since I only had SoCFPGA hardware, that was my initial patch. I made the mistake
> of not CC'ing the devicetree maintainers until I sent the DTS binding
> patch change,
> and he rightly suggested making the binding something more generic.

That is completely the opposite of what I said. You had something 
genericish to Intel platforms. You should make the compatible(s) SoC 
specific.


> I do like your idea of making a determination in the driver without
> being dependent
> on a dts binding. I'd like to know the impetus behind your original
> patch of removing the
> dependency of "if (f_pdata->dtr)"  for the write to the WR_COMPLETION_CTRL
> register? Perhaps there's some other common property that we can key
> off for why the register
> is not implemented?
> 
> Dinh
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-13 20:48     ` Rob Herring
@ 2021-12-13 21:21       ` Dinh Nguyen
  0 siblings, 0 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-13 21:21 UTC (permalink / raw)
  To: Rob Herring; +Cc: Pratyush Yadav, Dinh Nguyen, devicetree, broonie

On Mon, Dec 13, 2021 at 2:48 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Dec 08, 2021 at 05:45:31PM -0600, Dinh Nguyen wrote:
> > On Mon, Dec 6, 2021 at 9:51 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > On 03/12/21 12:17PM, Dinh Nguyen wrote:
> > > > The QSPI controller on Intel's SoCFPGA platform does not implement the
> > > > CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
> > > > results in a crash.
> > > >
> > > > The module/revision ID is written in the MODULE_ID register. For this
> > > > variance, bits 23-8 is 0x0010.
> > >
> > > When I looked at your original patches I was under the impression that
> > > this was a SoCFPGA specific thing and did not apply to other
> > > implementation of the IP in general.
> > >
> > > If this is indeed a generic thing and we can detect it via the MODULE_ID
> > > register [0], then why not just read that register at probe time and
> > > apply this quirk based on the ID? Why then do we need a separate
> > > compatible at all?
> > >
> > > [0] I would like to see it stated explicitly somewhere that version
> > > 0x0010 does not support the WR_COMPLETION_CTRL register.
> > >
> >
> > I cannot for sure confirm that this condition applies to only 0x0010
> > version of the
> > IP. I can verify that the IP that is in all 3 generations of SoCFPGA
> > devices, all have
> > MODULE_ID value of 0x0010 and all do not have the WR_COMPLETION_CTRL
> > register implemented.
>
> Then for the same reason, you shouldn't be trying to have a 'generic'
> compatible.
>
> >
> > I'm almost certain this feature is not SoCFPGA specific, but
> > since I only had SoCFPGA hardware, that was my initial patch. I made the mistake
> > of not CC'ing the devicetree maintainers until I sent the DTS binding
> > patch change,
> > and he rightly suggested making the binding something more generic.
>
> That is completely the opposite of what I said. You had something
> genericish to Intel platforms. You should make the compatible(s) SoC
> specific.
>
>

Sorry, I must have misunderstood. The same QSPI controller is used across the
entire SoCFPGA class of SoCs, so I don't know what you mean by SoC specific?

Dinh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-08 23:45   ` Dinh Nguyen
  2021-12-13 20:48     ` Rob Herring
@ 2021-12-14 20:05     ` Pratyush Yadav
  2021-12-15 15:36       ` Dinh Nguyen
  1 sibling, 1 reply; 14+ messages in thread
From: Pratyush Yadav @ 2021-12-14 20:05 UTC (permalink / raw)
  To: Dinh Nguyen; +Cc: Dinh Nguyen, devicetree, broonie, robh+dt

On 08/12/21 05:45PM, Dinh Nguyen wrote:
> On Mon, Dec 6, 2021 at 9:51 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > On 03/12/21 12:17PM, Dinh Nguyen wrote:
> > > The QSPI controller on Intel's SoCFPGA platform does not implement the
> > > CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
> > > results in a crash.
> > >
> > > The module/revision ID is written in the MODULE_ID register. For this
> > > variance, bits 23-8 is 0x0010.
> >
> > When I looked at your original patches I was under the impression that
> > this was a SoCFPGA specific thing and did not apply to other
> > implementation of the IP in general.
> >
> > If this is indeed a generic thing and we can detect it via the MODULE_ID
> > register [0], then why not just read that register at probe time and
> > apply this quirk based on the ID? Why then do we need a separate
> > compatible at all?
> >
> > [0] I would like to see it stated explicitly somewhere that version
> > 0x0010 does not support the WR_COMPLETION_CTRL register.
> >
> 
> I cannot for sure confirm that this condition applies to only 0x0010
> version of the
> IP. I can verify that the IP that is in all 3 generations of SoCFPGA
> devices, all have
> MODULE_ID value of 0x0010 and all do not have the WR_COMPLETION_CTRL
> register implemented.

I agree with Rob here. If you are not sure that this is a generic IP 
thing then you should not use a generic compatible.

> 
> I'm almost certain this feature is not SoCFPGA specific, but
> since I only had SoCFPGA hardware, that was my initial patch. I made the mistake
> of not CC'ing the devicetree maintainers until I sent the DTS binding
> patch change,
> and he rightly suggested making the binding something more generic.
> 
> I do like your idea of making a determination in the driver without
> being dependent
> on a dts binding. I'd like to know the impetus behind your original
> patch of removing the
> dependency of "if (f_pdata->dtr)"  for the write to the WR_COMPLETION_CTRL
> register? Perhaps there's some other common property that we can key
> off for why the register
> is not implemented?

Please read the comment just above that line ;-)

  /*
   * SPI NAND flashes require the address of the status register to be
   * passed in the Read SR command. Also, some SPI NOR flashes like the
   * cypress Semper flash expect a 4-byte dummy address in the Read SR
   * command in DTR mode.
   *
   * But this controller does not support address phase in the Read SR
   * command when doing auto-HW polling. So, disable write completion
   * polling on the controller's side. spinand and spi-nor will take
   * care of polling the status register.
   */

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-14 20:05     ` Pratyush Yadav
@ 2021-12-15 15:36       ` Dinh Nguyen
  2021-12-16 18:58         ` Pratyush Yadav
  0 siblings, 1 reply; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-15 15:36 UTC (permalink / raw)
  To: Pratyush Yadav, Dinh Nguyen; +Cc: devicetree, broonie, robh+dt



On 12/14/21 2:05 PM, Pratyush Yadav wrote:
> On 08/12/21 05:45PM, Dinh Nguyen wrote:
>> On Mon, Dec 6, 2021 at 9:51 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>>>
>>> On 03/12/21 12:17PM, Dinh Nguyen wrote:
>>>> The QSPI controller on Intel's SoCFPGA platform does not implement the
>>>> CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
>>>> results in a crash.
>>>>
>>>> The module/revision ID is written in the MODULE_ID register. For this
>>>> variance, bits 23-8 is 0x0010.
>>>
>>> When I looked at your original patches I was under the impression that
>>> this was a SoCFPGA specific thing and did not apply to other
>>> implementation of the IP in general.
>>>
>>> If this is indeed a generic thing and we can detect it via the MODULE_ID
>>> register [0], then why not just read that register at probe time and
>>> apply this quirk based on the ID? Why then do we need a separate
>>> compatible at all?
>>>
>>> [0] I would like to see it stated explicitly somewhere that version
>>> 0x0010 does not support the WR_COMPLETION_CTRL register.
>>>
>>
>> I cannot for sure confirm that this condition applies to only 0x0010
>> version of the
>> IP. I can verify that the IP that is in all 3 generations of SoCFPGA
>> devices, all have
>> MODULE_ID value of 0x0010 and all do not have the WR_COMPLETION_CTRL
>> register implemented.
> 
> I agree with Rob here. If you are not sure that this is a generic IP
> thing then you should not use a generic compatible.
>


I think using the binding of "intel,socfpga-qspi" should be fine? If we 
go by the MODULE_ID value as a indicator of versions, then the version 
hasn't changed for all revisions of the SoCFPGA, dating back to the 
original Cyclone5, which was introduced in 2012.

Thanks,
Dinh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
  2021-12-15 15:36       ` Dinh Nguyen
@ 2021-12-16 18:58         ` Pratyush Yadav
  0 siblings, 0 replies; 14+ messages in thread
From: Pratyush Yadav @ 2021-12-16 18:58 UTC (permalink / raw)
  To: Dinh Nguyen; +Cc: Dinh Nguyen, devicetree, broonie, robh+dt

On 15/12/21 09:36AM, Dinh Nguyen wrote:
> 
> 
> On 12/14/21 2:05 PM, Pratyush Yadav wrote:
> > On 08/12/21 05:45PM, Dinh Nguyen wrote:
> > > On Mon, Dec 6, 2021 at 9:51 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> > > > 
> > > > On 03/12/21 12:17PM, Dinh Nguyen wrote:
> > > > > The QSPI controller on Intel's SoCFPGA platform does not implement the
> > > > > CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
> > > > > results in a crash.
> > > > > 
> > > > > The module/revision ID is written in the MODULE_ID register. For this
> > > > > variance, bits 23-8 is 0x0010.
> > > > 
> > > > When I looked at your original patches I was under the impression that
> > > > this was a SoCFPGA specific thing and did not apply to other
> > > > implementation of the IP in general.
> > > > 
> > > > If this is indeed a generic thing and we can detect it via the MODULE_ID
> > > > register [0], then why not just read that register at probe time and
> > > > apply this quirk based on the ID? Why then do we need a separate
> > > > compatible at all?
> > > > 
> > > > [0] I would like to see it stated explicitly somewhere that version
> > > > 0x0010 does not support the WR_COMPLETION_CTRL register.
> > > > 
> > > 
> > > I cannot for sure confirm that this condition applies to only 0x0010
> > > version of the
> > > IP. I can verify that the IP that is in all 3 generations of SoCFPGA
> > > devices, all have
> > > MODULE_ID value of 0x0010 and all do not have the WR_COMPLETION_CTRL
> > > register implemented.
> > 
> > I agree with Rob here. If you are not sure that this is a generic IP
> > thing then you should not use a generic compatible.
> > 
> 
> 
> I think using the binding of "intel,socfpga-qspi" should be fine? If we go
> by the MODULE_ID value as a indicator of versions, then the version hasn't
> changed for all revisions of the SoCFPGA, dating back to the original
> Cyclone5, which was introduced in 2012.

Yes, I think you should keep using the SoC specific binding unless you 
can find some documentation from Cadence that says all parts with this 
MODULE_ID value don't have the WR_COMPLETION_CTRL register.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
@ 2021-12-03 17:32 Dinh Nguyen
  0 siblings, 0 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-03 17:32 UTC (permalink / raw)
  To: devicetree; +Cc: dinguyen, broonie, robh+dt, p.yadav

The QSPI controller on Intel's SoCFPGA platform does not implement the
CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
results in a crash.

The module/revision ID is written in the MODULE_ID register. For this
variance, bits 23-8 is 0x0010.

Introduce the dts binding "cdns,qspi-nor-ver-00-10" to differentiate the
hardware.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v2: change binding to "cdns,qspi-nor-0010" to be more generic for other
    platforms
---
 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index ca155abbda7a..2833e1c8841d 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -29,6 +29,7 @@ properties:
               - ti,am654-ospi
               - intel,lgm-qspi
               - xlnx,versal-ospi-1.0
+              - cdns,qspi-nor-ver-00-10
           - const: cdns,qspi-nor
       - const: cdns,qspi-nor
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"
@ 2021-12-03 14:01 Dinh Nguyen
  0 siblings, 0 replies; 14+ messages in thread
From: Dinh Nguyen @ 2021-12-03 14:01 UTC (permalink / raw)
  To: devicetree; +Cc: dinguyen, broonie, robh+dt, p.yadav

The QSPI controller on Intel's SoCFPGA platform does not implement the
CQSPI_REG_WR_COMPLETION_CTRL register, thus a write to this register
results in a crash.

The module/revision ID is written in the MODULE_ID register. For this
variance, bits 23-8 is 0x0010.

Introduce the dts binding "cdns,qspi-nor-ver-00-10" to differentiate the
hardware.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v2: change binding to "cdns,qspi-nor-0010" to be more generic for other
    platforms
---
 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index ca155abbda7a..2833e1c8841d 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -29,6 +29,7 @@ properties:
               - ti,am654-ospi
               - intel,lgm-qspi
               - xlnx,versal-ospi-1.0
+              - cdns,qspi-nor-ver-00-10
           - const: cdns,qspi-nor
       - const: cdns,qspi-nor
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-12-16 18:58 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-03 18:17 [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Dinh Nguyen
2021-12-03 18:17 ` [PATCHv2 2/3] ARM: dts: socfpga: change qspi to "cdns,qspi-nor-ver-00-10" Dinh Nguyen
2021-12-03 18:17 ` [PATCH 3/3] spi: cadence-quadspi: change socfpga-qspi " Dinh Nguyen
2021-12-03 18:18 ` [PATCHv2 1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" Mark Brown
2021-12-03 19:05   ` Dinh Nguyen
2021-12-06 10:22 ` Pratyush Yadav
2021-12-08 23:45   ` Dinh Nguyen
2021-12-13 20:48     ` Rob Herring
2021-12-13 21:21       ` Dinh Nguyen
2021-12-14 20:05     ` Pratyush Yadav
2021-12-15 15:36       ` Dinh Nguyen
2021-12-16 18:58         ` Pratyush Yadav
  -- strict thread matches above, loose matches on Subject: below --
2021-12-03 17:32 Dinh Nguyen
2021-12-03 14:01 Dinh Nguyen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).