[PATCHv2,1/3] dt-bindings: spi: cadence-quadspi: document "cdns,qspi-nor-ver-00-10"

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(+)

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 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