| Message ID | 20180920224055.164856-1-ryandcase@chromium.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation | expand |
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. > +
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.
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. > > +
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.
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.
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
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.
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).
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
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.
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
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>; + }; + };