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

[Rob Herring <robh@kernel.org>]

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
>