[PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Ryan Case]

From: Girish Mahadevan <girishm@codeaurora.org>

Bindings for Qualcomm Quad SPI used on SoCs such as sdm845.

Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Signed-off-by: Ryan Case <ryandcase@chromium.org>
---

Changes in v2:
- Added commit text
- Removed invalid property
- Updated example to match sdm845 with attached spi-nor

 .../bindings/spi/qcom,spi-qcom-qspi.txt       | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
new file mode 100644
index 000000000000..ecfb1e2bd520
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
@@ -0,0 +1,36 @@
+Qualcomm Quad Serial Peripheral Interface (QSPI)
+
+The QSPI controller allows SPI protocol communication in single, dual, or quad
+wire transmission modes for read/write access to slaves such as NOR flash.
+
+Required properties:
+- compatible:	Should contain:
+		"qcom,sdm845-qspi"
+- reg:		Should contain the base register location and length.
+- interrupts:	Interrupt number used by the controller.
+- clocks:	Should contain the core and AHB clock.
+- clock-names:	Should be "core" for core clock and "iface" for AHB clock.
+
+SPI slave nodes must be children of the SPI master node and can contain
+properties described in Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+
+	qspi: qspi@88df000 {
+		compatible = "qcom,sdm845-qspi";
+		reg = <0x88df000 0x600>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
+		clock-names = "iface", "core";
+		clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
+			 <&gcc GCC_QSPI_CORE_CLK>;
+
+		device@0 {
+			compatible = "jedec,spi-nor";
+			reg = <0>;
+			spi-max-frequency = <25000000>;
+			spi-tx-bus-width = <2>;
+			spi-rx-bus-width = <2>;
+		};
+	};
-- 
2.19.0.444.g18242da7ef-goog

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Stephen Boyd]

Quoting Ryan Case (2018-09-20 15:40:54)
> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> new file mode 100644
> index 000000000000..ecfb1e2bd520
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> @@ -0,0 +1,36 @@
> +Qualcomm Quad Serial Peripheral Interface (QSPI)
> +
> +The QSPI controller allows SPI protocol communication in single, dual, or quad
> +wire transmission modes for read/write access to slaves such as NOR flash.
> +
> +Required properties:
> +- compatible:  Should contain:
> +               "qcom,sdm845-qspi"

Does someone have a more generic compatible string that can be added
here to indicate the type of quad SPI controller this is? I really doubt
this is a one-off hardware block for the specific SDM845 SoC. 

> +- reg:         Should contain the base register location and length.
> +- interrupts:  Interrupt number used by the controller.
> +- clocks:      Should contain the core and AHB clock.
> +- clock-names: Should be "core" for core clock and "iface" for AHB clock.
> +

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Mark Brown]

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

On Fri, Sep 21, 2018 at 10:30:57AM -0700, Stephen Boyd wrote:
> Quoting Ryan Case (2018-09-20 15:40:54)

> > +Required properties:
> > +- compatible:  Should contain:
> > +               "qcom,sdm845-qspi"

> Does someone have a more generic compatible string that can be added
> here to indicate the type of quad SPI controller this is? I really doubt
> this is a one-off hardware block for the specific SDM845 SoC. 

The idiom for DT is supposed to be to use only device specific names
unfortunately.

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

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Trent Piepho]

On Fri, 2018-09-21 at 10:39 -0700, Mark Brown wrote:
> On Fri, Sep 21, 2018 at 10:30:57AM -0700, Stephen Boyd wrote:
> > Quoting Ryan Case (2018-09-20 15:40:54)
> > > +Required properties:
> > > +- compatible:  Should contain:
> > > +               "qcom,sdm845-qspi"
> > Does someone have a more generic compatible string that can be added
> > here to indicate the type of quad SPI controller this is? I really doubt
> > this is a one-off hardware block for the specific SDM845 SoC. 
> 
> The idiom for DT is supposed to be to use only device specific names
> unfortunately.

Basically the "first" device the driver can control has it's specific
name used as the generic string.  This is used in place of some
internal codename for the core.

Then a newer device will have "foo,XYZ200", "foo,XYZ100" as compatible,
where the 100 was the first device and the 200 is new one.  Maybe the
driver cares, or will care, about what device this or maybe it can
drive the device fine without needing to know more than the generic.

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Doug Anderson]

Hi,

On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Ryan Case (2018-09-20 15:40:54)
> > diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> > new file mode 100644
> > index 000000000000..ecfb1e2bd520
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> > @@ -0,0 +1,36 @@
> > +Qualcomm Quad Serial Peripheral Interface (QSPI)
> > +
> > +The QSPI controller allows SPI protocol communication in single, dual, or quad
> > +wire transmission modes for read/write access to slaves such as NOR flash.
> > +
> > +Required properties:
> > +- compatible:  Should contain:
> > +               "qcom,sdm845-qspi"
>
> Does someone have a more generic compatible string that can be added
> here to indicate the type of quad SPI controller this is? I really doubt
> this is a one-off hardware block for the specific SDM845 SoC.

The compatible string used to be "qcom,qspi-v1".  ...but Rob Herring
requested [1] "an SoC specific compatible string".  While we could do
a compatible string like:

  "qcom,sdm845-qspi", "qcom,qspi-v1".

I'm curious if that buys us anything.  From all my previous experience
with device tree it is fine to name a compatible string for a
component based on the first SoC that used it.  If we later find that
this is also used in an "msm1234" we could always later do the
compatible string for that device as:

  "qcom, msm1234-qspi", "qcom,sdm845-qspi"

...and we don't need to try to come up with a generic name.
Obviously, though, I'll cede to whatever Rob says here though.

-Doug


[1] http://lkml.kernel.org/r/20180716222721.GA12854@rob-hp-laptop


>
> > +- reg:         Should contain the base register location and length.
> > +- interrupts:  Interrupt number used by the controller.
> > +- clocks:      Should contain the core and AHB clock.
> > +- clock-names: Should be "core" for core clock and "iface" for AHB clock.
> > +

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Stephen Boyd]

Quoting Doug Anderson (2018-09-21 10:40:14)
> Hi,
> 
> On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Ryan Case (2018-09-20 15:40:54)
> > > diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> > > new file mode 100644
> > > index 000000000000..ecfb1e2bd520
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> > > @@ -0,0 +1,36 @@
> > > +Qualcomm Quad Serial Peripheral Interface (QSPI)
> > > +
> > > +The QSPI controller allows SPI protocol communication in single, dual, or quad
> > > +wire transmission modes for read/write access to slaves such as NOR flash.
> > > +
> > > +Required properties:
> > > +- compatible:  Should contain:
> > > +               "qcom,sdm845-qspi"
> >
> > Does someone have a more generic compatible string that can be added
> > here to indicate the type of quad SPI controller this is? I really doubt
> > this is a one-off hardware block for the specific SDM845 SoC.
> 
> The compatible string used to be "qcom,qspi-v1".  ...but Rob Herring
> requested [1] "an SoC specific compatible string".  While we could do
> a compatible string like:
> 
>   "qcom,sdm845-qspi", "qcom,qspi-v1".
> 
> I'm curious if that buys us anything.  From all my previous experience
> with device tree it is fine to name a compatible string for a
> component based on the first SoC that used it.  If we later find that
> this is also used in an "msm1234" we could always later do the
> compatible string for that device as:
> 
>   "qcom, msm1234-qspi", "qcom,sdm845-qspi"
> 
> ...and we don't need to try to come up with a generic name.
> Obviously, though, I'll cede to whatever Rob says here though.
> 

It seems that everybody has misunderstood my email. Let me try to
clarify.

I'm not saying to replace the sdm845 qspi compatible with a generic one.
I'm recommending that a generic one is added in addition to the SoC
specific one. That way, we get to put the generic compatible string in
the driver and not need to update the driver compatible string array
each time a new SoC comes out with a new compatible string.

If it turns out later that we need to handle some bug in that specific
SoC compatible string then we're good to go and we can handle it by
matching the more specific SoC version compatible.

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Doug Anderson]

Stephen
On Fri, Sep 21, 2018 at 11:40 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2018-09-21 10:40:14)
> > Hi,
> >
> > On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Ryan Case (2018-09-20 15:40:54)
> > > > diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> > > > new file mode 100644
> > > > index 000000000000..ecfb1e2bd520
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> > > > @@ -0,0 +1,36 @@
> > > > +Qualcomm Quad Serial Peripheral Interface (QSPI)
> > > > +
> > > > +The QSPI controller allows SPI protocol communication in single, dual, or quad
> > > > +wire transmission modes for read/write access to slaves such as NOR flash.
> > > > +
> > > > +Required properties:
> > > > +- compatible:  Should contain:
> > > > +               "qcom,sdm845-qspi"
> > >
> > > Does someone have a more generic compatible string that can be added
> > > here to indicate the type of quad SPI controller this is? I really doubt
> > > this is a one-off hardware block for the specific SDM845 SoC.
> >
> > The compatible string used to be "qcom,qspi-v1".  ...but Rob Herring
> > requested [1] "an SoC specific compatible string".  While we could do
> > a compatible string like:
> >
> >   "qcom,sdm845-qspi", "qcom,qspi-v1".
> >
> > I'm curious if that buys us anything.  From all my previous experience
> > with device tree it is fine to name a compatible string for a
> > component based on the first SoC that used it.  If we later find that
> > this is also used in an "msm1234" we could always later do the
> > compatible string for that device as:
> >
> >   "qcom, msm1234-qspi", "qcom,sdm845-qspi"
> >
> > ...and we don't need to try to come up with a generic name.
> > Obviously, though, I'll cede to whatever Rob says here though.
> >
>
> It seems that everybody has misunderstood my email. Let me try to
> clarify.
>
> I'm not saying to replace the sdm845 qspi compatible with a generic one.
> I'm recommending that a generic one is added in addition to the SoC
> specific one. That way, we get to put the generic compatible string in
> the driver and not need to update the driver compatible string array
> each time a new SoC comes out with a new compatible string.
>
> If it turns out later that we need to handle some bug in that specific
> SoC compatible string then we're good to go and we can handle it by
> matching the more specific SoC version compatible.

I don't think I misunderstood.  I was suggesting that I believe that
the device tree way is to use the first SoC as the generic one.  In
other words to support sdm845 and msm1234, we do:

A)
on sdm845: "qcom,sdm845-qspi"
on msm1234 (later): "qcom, msm1234-qspi", "qcom,sdm845-qspi"

What you are suggesting (I think) is:

B)
on sdm845: "qcom,sdm845-qspi", "qcom,qspi-v1"
on msm1234 (later): "qcom, msm1234-qspi", "qcom,qspi-v1"

If Rob likes B) better then it's fine with me, it was just my
understanding that A) was the suggested way to do things (even if it
is decidedly non-symmetric).


NOTE: Even with A) there's no need to update the driver for msm1234
since it has the fallback to sdm845.

-Doug

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Mark Brown]

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

On Fri, Sep 21, 2018 at 11:40:24AM -0700, Stephen Boyd wrote:

> It seems that everybody has misunderstood my email. Let me try to
> clarify.

> I'm not saying to replace the sdm845 qspi compatible with a generic one.
> I'm recommending that a generic one is added in addition to the SoC
> specific one. That way, we get to put the generic compatible string in
> the driver and not need to update the driver compatible string array
> each time a new SoC comes out with a new compatible string.

> If it turns out later that we need to handle some bug in that specific
> SoC compatible string then we're good to go and we can handle it by
> matching the more specific SoC version compatible.

Right, the policy is generally not to have these strings at all.  IIRC
the argument is that they tend to either become unclear as the marketing
and technology changes.

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

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Stephen Boyd]

Quoting Mark Brown (2018-09-21 11:51:06)
> On Fri, Sep 21, 2018 at 11:40:24AM -0700, Stephen Boyd wrote:
> 
> > It seems that everybody has misunderstood my email. Let me try to
> > clarify.
> 
> > I'm not saying to replace the sdm845 qspi compatible with a generic one.
> > I'm recommending that a generic one is added in addition to the SoC
> > specific one. That way, we get to put the generic compatible string in
> > the driver and not need to update the driver compatible string array
> > each time a new SoC comes out with a new compatible string.
> 
> > If it turns out later that we need to handle some bug in that specific
> > SoC compatible string then we're good to go and we can handle it by
> > matching the more specific SoC version compatible.
> 
> Right, the policy is generally not to have these strings at all.  IIRC
> the argument is that they tend to either become unclear as the marketing
> and technology changes.

Where is this policy documented? Is it on the list somewhere or written
in Documentation/devicetree/? From my read of Rob's comment in the
previous version of this patch, all that was asked was to add another
compatible string for the specific SoC.

I find the approach of picking the first SoC that the driver works on to
be obtuse. I don't want to be reading some SoC DTS and see another SoC
marketing number in the compatible string because it makes it confusing
to explain to someone that yes these different SoCs are related to each
other, but no, that SoC isn't this SoC. Sure it all works and everything
is technically fine, but my aesthetically pleasing alarms go off and I
don't see any particular downside to having two compatibles.

The upside is that things aren't confusing and drivers don't get
continual SoC churn updates because the compatible describes the SoC
(qcom,sdm845-qspi) and the programming model (qcom,qspi-v1).

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Doug Anderson]

Hi,

On Sat, Sep 22, 2018 at 8:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Mark Brown (2018-09-21 11:51:06)
> > On Fri, Sep 21, 2018 at 11:40:24AM -0700, Stephen Boyd wrote:
> >
> > > It seems that everybody has misunderstood my email. Let me try to
> > > clarify.
> >
> > > I'm not saying to replace the sdm845 qspi compatible with a generic one.
> > > I'm recommending that a generic one is added in addition to the SoC
> > > specific one. That way, we get to put the generic compatible string in
> > > the driver and not need to update the driver compatible string array
> > > each time a new SoC comes out with a new compatible string.
> >
> > > If it turns out later that we need to handle some bug in that specific
> > > SoC compatible string then we're good to go and we can handle it by
> > > matching the more specific SoC version compatible.
> >
> > Right, the policy is generally not to have these strings at all.  IIRC
> > the argument is that they tend to either become unclear as the marketing
> > and technology changes.
>
> Where is this policy documented? Is it on the list somewhere or written
> in Documentation/devicetree/?

I don't of it being documented anywhere, but it's what I've been told
before.  I spent a bit of time to find a specific example but I
couldn't.  As with a lot of device tree stuff it historically has been
a bunch of word-of-mouth type stuff.  It does look like people are
moving towards a more formal spec at
<https://github.com/devicetree-org/devicetree-specification/> but it
doesn't include this guideline.


> From my read of Rob's comment in the
> previous version of this patch, all that was asked was to add another
> compatible string for the specific SoC.
>
> I find the approach of picking the first SoC that the driver works on to
> be obtuse. I don't want to be reading some SoC DTS and see another SoC
> marketing number in the compatible string because it makes it confusing
> to explain to someone that yes these different SoCs are related to each
> other, but no, that SoC isn't this SoC. Sure it all works and everything
> is technically fine, but my aesthetically pleasing alarms go off and I
> don't see any particular downside to having two compatibles.
>
> The upside is that things aren't confusing and drivers don't get
> continual SoC churn updates because the compatible describes the SoC
> (qcom,sdm845-qspi) and the programming model (qcom,qspi-v1).

IIUC previous suggestions about just naming it based on the first SoC
was due to the difficulty of coming up with a good generic name to
give something.  For instance you definitely wouldn't want to name it
"qcom-qspi-sdm8xx" because you have no idea what future SoCs will be
numbered.

In the case here calling it "qcom,qspi-v1" is better than that and if
Rob gives the thumbs up then I won't object to it.  In general though
looking at other device tree bindings this doesn't seem like a thing
commonly done.  Perhaps if we decide it's useful here we should start
suggesting it everywhere...

-Doug



-Doug

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Trent Piepho]

On Mon, 2018-09-24 at 10:13 -0700, Doug Anderson wrote:
> IIUC previous suggestions about just naming it based on the first SoC
> was due to the difficulty of coming up with a good generic name to
> give something.  For instance you definitely wouldn't want to name it
> "qcom-qspi-sdm8xx" because you have no idea what future SoCs will be
> numbered.

And the hypothetical sdm899 might use a non-compatible device that uses
a different driver, and that really makes "qcom-qspi-sdm8xx" look dumb.

> 
> In the case here calling it "qcom,qspi-v1" is better than that and if
> Rob gives the thumbs up then I won't object to it.  In general though
> looking at other device tree bindings this doesn't seem like a thing
> commonly done.  Perhaps if we decide it's useful here we should start
> suggesting it everywhere...

It would help if the programming model or IP core name or whatever this
is using was mentioned in the public reference manual for the SoC. 
Then is a lot more clear that a number of different SoCs all have the
same quad spi controller inside them.

Usually it's not like that.  The RMs just say, "it's got a SPI master
with these registers."  What SoCs use the same IP module, which
different?  When did they make a new version?  The silicon vendors
don't tell you this.  So any name we make up for the IP module likely
doesn't match reality.

Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Doug Anderson]

Hi,
On Mon, Sep 24, 2018 at 11:23 AM Trent Piepho <tpiepho@impinj.com> wrote:
>
> On Mon, 2018-09-24 at 10:13 -0700, Doug Anderson wrote:
> > IIUC previous suggestions about just naming it based on the first SoC
> > was due to the difficulty of coming up with a good generic name to
> > give something.  For instance you definitely wouldn't want to name it
> > "qcom-qspi-sdm8xx" because you have no idea what future SoCs will be
> > numbered.
>
> And the hypothetical sdm899 might use a non-compatible device that uses
> a different driver, and that really makes "qcom-qspi-sdm8xx" look dumb.
>
> >
> > In the case here calling it "qcom,qspi-v1" is better than that and if
> > Rob gives the thumbs up then I won't object to it.  In general though
> > looking at other device tree bindings this doesn't seem like a thing
> > commonly done.  Perhaps if we decide it's useful here we should start
> > suggesting it everywhere...
>
> It would help if the programming model or IP core name or whatever this
> is using was mentioned in the public reference manual for the SoC.
> Then is a lot more clear that a number of different SoCs all have the
> same quad spi controller inside them.
>
> Usually it's not like that.  The RMs just say, "it's got a SPI master
> with these registers."  What SoCs use the same IP module, which
> different?  When did they make a new version?  The silicon vendors
> don't tell you this.  So any name we make up for the IP module likely
> doesn't match reality.

Note that Rob did recently give a positive review to a similar binding.  See:

https://lore.kernel.org/patchwork/patch/979432/

Specifically the text from that binding was:

+                  Qcom SoCs must contain, as below, SoC-specific compatibles
+                  along with "qcom,smmu-v2":
+                  "qcom,msm8996-smmu-v2", "qcom,smmu-v2",
+                  "qcom,sdm845-smmu-v2", "qcom,smmu-v2".

Given Rob's positive review there, it seems like it would be fine to do:

  "qcom,sdm845-qspi", "qcom,qspi-v1".

NOTE: In our case we don't need the "-v1" in SoC-specific case since
there's only one Quad SPI driver there.  As I understand it the reason
we needed the -v2 in the SoC-specific case for the SMMU was that there
are two totally different SMMUs in SDM845.  You can see history in the
v15 patch <https://lore.kernel.org/patchwork/patch/977888/>


-Doug