| Message ID | 20211203181714.3138611-1-dinguyen@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [PATCHv2,1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10" | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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?
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
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.
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
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 >
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
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. */
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
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.
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
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(+)