| Message ID | 20250108012846.3275443-3-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | qcom: Add an SoC PM driver for sc7180 using PM domains | expand |
On 8.01.2025 2:28 AM, Stephen Boyd wrote: > Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up > of multiple devices that have their own bindings, therefore this binding > is for a "bus" that is the SoC node. > > TODO: Document all child nodes. This is woefully incomplete but at least > shows what is involved with describing an SoC node in dt schema. I'm not sure I'm a fan, because... [...] > + > +properties: > + compatible: > + items: > + - const: qcom,soc-sc7180 > + - const: simple-bus > + > + '#address-cells': > + const: 2 > + > + '#size-cells': > + const: 2 > + > + clock-controller@100000: > + $ref: /schemas/clock/qcom,gcc-sc7180.yaml# > + > + watchdog@17c10000: > + $ref: /schemas/watchdog/qcom-wdt.yaml# > + > +required: > + - compatible > + - '#address-cells' > + - '#size-cells' > + - clock-controller@100000 > + - watchdog@17c10000 > + > +additionalProperties: false ..this approach will make any dt node addition under /soc require an additional bindings change, which sounds like absolute madness I think additionalProperties: true would be sufficient here, like in Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml Konrad
Quoting Konrad Dybcio (2025-01-09 06:05:14) > On 8.01.2025 2:28 AM, Stephen Boyd wrote: > > Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up > > of multiple devices that have their own bindings, therefore this binding > > is for a "bus" that is the SoC node. > > > > TODO: Document all child nodes. This is woefully incomplete but at least > > shows what is involved with describing an SoC node in dt schema. > > I'm not sure I'm a fan, because... > > [...] > > > + > > +properties: > > + compatible: > > + items: > > + - const: qcom,soc-sc7180 > > + - const: simple-bus > > + > > + '#address-cells': > > + const: 2 > > + > > + '#size-cells': > > + const: 2 > > + > > + clock-controller@100000: > > + $ref: /schemas/clock/qcom,gcc-sc7180.yaml# > > + > > + watchdog@17c10000: > > + $ref: /schemas/watchdog/qcom-wdt.yaml# > > + > > +required: > > + - compatible > > + - '#address-cells' > > + - '#size-cells' > > + - clock-controller@100000 > > + - watchdog@17c10000 > > + > > +additionalProperties: false > > ..this approach will make any dt node addition under /soc require > an additional bindings change, which sounds like absolute madness We should pretty much know what nodes go under here though, because it's a chip that exists and doesn't change after the fact. I agree that it will be annoying during early development when everyone is modifying the same file to add their node, but that problem also exists with the dts files today so it doesn't seem like total madness. It's also nice to be able to look at one file and quickly find all the schemas for the SoC used, like a table of contents almost or a memory map for the chip. One thing that I find annoying is that you have to put the whole soc node and child nodes in the example. Maybe we can omit the example because there are so many child nodes. > > I think additionalProperties: true would be sufficient here, like in > Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml > Ok. That binding looks to be for the efuse properties of the SoC node itself? I was hoping to find another example of this "describe the whole SoC" sort of binding but that doesn't match. Is there one already out there? Should I move this binding to bindings/soc/qcom instead of bindings/bus?
On 9.01.2025 10:51 PM, Stephen Boyd wrote: > Quoting Konrad Dybcio (2025-01-09 06:05:14) >> On 8.01.2025 2:28 AM, Stephen Boyd wrote: >>> Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up >>> of multiple devices that have their own bindings, therefore this binding >>> is for a "bus" that is the SoC node. >>> >>> TODO: Document all child nodes. This is woefully incomplete but at least >>> shows what is involved with describing an SoC node in dt schema. >> >> I'm not sure I'm a fan, because... >> >> [...] >> >>> + >>> +properties: >>> + compatible: >>> + items: >>> + - const: qcom,soc-sc7180 >>> + - const: simple-bus >>> + >>> + '#address-cells': >>> + const: 2 >>> + >>> + '#size-cells': >>> + const: 2 >>> + >>> + clock-controller@100000: >>> + $ref: /schemas/clock/qcom,gcc-sc7180.yaml# >>> + >>> + watchdog@17c10000: >>> + $ref: /schemas/watchdog/qcom-wdt.yaml# >>> + >>> +required: >>> + - compatible >>> + - '#address-cells' >>> + - '#size-cells' >>> + - clock-controller@100000 >>> + - watchdog@17c10000 >>> + >>> +additionalProperties: false >> >> ..this approach will make any dt node addition under /soc require >> an additional bindings change, which sounds like absolute madness > > We should pretty much know what nodes go under here though, because it's > a chip that exists and doesn't change after the fact. I agree that it > will be annoying during early development when everyone is modifying the > same file to add their node, but that problem also exists with the dts > files today so it doesn't seem like total madness. It's also nice to be > able to look at one file and quickly find all the schemas for the SoC > used, like a table of contents almost or a memory map for the chip. > > One thing that I find annoying is that you have to put the whole soc > node and child nodes in the example. Maybe we can omit the example > because there are so many child nodes. I guess I could get behind both your and my points.. My main worry is the day-1-support-1234-long-patch-series where 5-10% of nodes is likely to need some remodeling, with some hw needing to be re-described in a different way before getting merged. Rebasing that will be an even bigger mess, but I suppose it's doable.. The same stands for the every-now-and-then occasion when we decide that e.g. block X is not really a clock-controller, but rather a power-manager or something.. If someone wants to rely on stable bindings in their non-Linux OS project, requiring constant node names is one more potential point of failure. >> I think additionalProperties: true would be sufficient here, like in >> Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml >> > > Ok. That binding looks to be for the efuse properties of the SoC node > itself? I was hoping to find another example of this "describe the whole > SoC" sort of binding but that doesn't match. Is there one already out > there? Should I move this binding to bindings/soc/qcom instead of > bindings/bus? I don't think anyone has done that in the past.. maaaaybe Documentation/devicetree/bindings/bus/st,stm32mp25-rifsc.yaml gets close with a loose "anything with a @unit-address" as "Peripherals", but that's not what you're looking for.. As for the directory, it seems to be all over the place for other uses of "xyz", "simple-bus": Documentation/devicetree/bindings/arm/arm,realview.yaml Documentation/devicetree/bindings/arm/arm,vexpress-juno.yaml Documentation/devicetree/bindings/arm/microchip,sparx5.yaml Documentation/devicetree/bindings/bus/fsl,spba-bus.yaml Documentation/devicetree/bindings/bus/st,stm32-etzpc.yaml Documentation/devicetree/bindings/bus/st,stm32mp25-rifsc.yaml Documentation/devicetree/bindings/net/marvell,dfx-server.yaml Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,qe.yaml Documentation/devicetree/bindings/soc/imx/fsl,aips-bus.yaml Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml Konrad
On Thu, Jan 09, 2025 at 01:51:12PM -0800, Stephen Boyd wrote: > Quoting Konrad Dybcio (2025-01-09 06:05:14) > > On 8.01.2025 2:28 AM, Stephen Boyd wrote: > > > Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up > > > of multiple devices that have their own bindings, therefore this binding > > > is for a "bus" that is the SoC node. > > > > > > TODO: Document all child nodes. This is woefully incomplete but at least > > > shows what is involved with describing an SoC node in dt schema. > > > > I'm not sure I'm a fan, because... > > > > [...] > > > > > + > > > +properties: > > > + compatible: > > > + items: > > > + - const: qcom,soc-sc7180 > > > + - const: simple-bus > > > + > > > + '#address-cells': > > > + const: 2 > > > + > > > + '#size-cells': > > > + const: 2 > > > + > > > + clock-controller@100000: > > > + $ref: /schemas/clock/qcom,gcc-sc7180.yaml# This makes the above schema be applied twice. Once here and then when the compatible matches. That can be avoided by just listing a compatible. The QCom display bindings follow that style. > > > + > > > + watchdog@17c10000: > > > + $ref: /schemas/watchdog/qcom-wdt.yaml# > > > + > > > +required: > > > + - compatible > > > + - '#address-cells' > > > + - '#size-cells' > > > + - clock-controller@100000 > > > + - watchdog@17c10000 > > > + > > > +additionalProperties: false > > > > ..this approach will make any dt node addition under /soc require > > an additional bindings change, which sounds like absolute madness > > We should pretty much know what nodes go under here though, because it's > a chip that exists and doesn't change after the fact. I agree that it > will be annoying during early development when everyone is modifying the > same file to add their node, but that problem also exists with the dts > files today so it doesn't seem like total madness. It's also nice to be > able to look at one file and quickly find all the schemas for the SoC > used, like a table of contents almost or a memory map for the chip. I don't really care for listing everything either. We could just generate all the schemas used. Either "give me all the schemas matching some compatible patter" or "give me all the schemas used to validate the DTB". The latter was possible on a per node basis, but I think I dropped that when I changed how we select schemas to apply. Speaking of memory maps, I would like a tool which could dump memory map from .dts. My primary reason is to find overlapping regions. > One thing that I find annoying is that you have to put the whole soc > node and child nodes in the example. Maybe we can omit the example > because there are so many child nodes. > > > > > I think additionalProperties: true would be sufficient here, like in > > Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml > > No. You can do: additionalProperties: type: object Or a patternProperties entry requiring '@' in the name. > Ok. That binding looks to be for the efuse properties of the SoC node > itself? I was hoping to find another example of this "describe the whole > SoC" sort of binding but that doesn't match. Is there one already out > there? Should I move this binding to bindings/soc/qcom instead of > bindings/bus? bindings/bus The 'soc' nodes here aren't really for the whole SoC. Cpus aren't in the soc node. They are for buses. We should allow for there to be more than 1 for instance. Rob
Quoting Rob Herring (2025-01-10 05:58:43) > On Thu, Jan 09, 2025 at 01:51:12PM -0800, Stephen Boyd wrote: > > Quoting Konrad Dybcio (2025-01-09 06:05:14) > > > On 8.01.2025 2:28 AM, Stephen Boyd wrote: > > > > + > > > > + '#size-cells': > > > > + const: 2 > > > > + > > > > + clock-controller@100000: > > > > + $ref: /schemas/clock/qcom,gcc-sc7180.yaml# > > This makes the above schema be applied twice. Once here and then when > the compatible matches. That can be avoided by just listing a > compatible. The QCom display bindings follow that style. Cool thanks! > > > > > + > > > > + watchdog@17c10000: > > > > + $ref: /schemas/watchdog/qcom-wdt.yaml# > > > > + > > > > +required: > > > > + - compatible > > > > + - '#address-cells' > > > > + - '#size-cells' > > > > + - clock-controller@100000 > > > > + - watchdog@17c10000 > > > > + > > > > +additionalProperties: false > > > > > > ..this approach will make any dt node addition under /soc require > > > an additional bindings change, which sounds like absolute madness > > > > We should pretty much know what nodes go under here though, because it's > > a chip that exists and doesn't change after the fact. I agree that it > > will be annoying during early development when everyone is modifying the > > same file to add their node, but that problem also exists with the dts > > files today so it doesn't seem like total madness. It's also nice to be > > able to look at one file and quickly find all the schemas for the SoC > > used, like a table of contents almost or a memory map for the chip. > > I don't really care for listing everything either. > > We could just generate all the schemas used. Either "give me all the > schemas matching some compatible patter" or "give me all the schemas > used to validate the DTB". The latter was possible on a per node basis, > but I think I dropped that when I changed how we select schemas to > apply. It is good enough to list compatible and properties like address-cells and size-cells and then have patternProperties requiring '@' in the name? > > Speaking of memory maps, I would like a tool which could dump memory map > from .dts. My primary reason is to find overlapping regions. Sounds cool. I don't have any need for that though so I'm not going to jump at writing such a tool. > > > One thing that I find annoying is that you have to put the whole soc > > node and child nodes in the example. Maybe we can omit the example > > because there are so many child nodes. > > > > > > > > I think additionalProperties: true would be sufficient here, like in > > > Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml > > > > > No. You can do: > > additionalProperties: > type: object > > Or a patternProperties entry requiring '@' in the name. This is to make sure only child nodes can be added, right? I like checking for '@' in the name so that random nodes can't be added that don't have a reg property.
diff --git a/Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml b/Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml new file mode 100644 index 000000000000..56f8b75ecdab --- /dev/null +++ b/Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml @@ -0,0 +1,76 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/bus/qcom,soc-sc7180.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm SC7180 SoC + +maintainers: + - Bjorn Andersson <andersson@kernel.org> + - Konrad Dybcio <konradybcio@kernel.org> + +description: + Qualcomm's SC7180 System on a Chip (SoC). + +properties: + compatible: + items: + - const: qcom,soc-sc7180 + - const: simple-bus + + '#address-cells': + const: 2 + + '#size-cells': + const: 2 + + clock-controller@100000: + $ref: /schemas/clock/qcom,gcc-sc7180.yaml# + + watchdog@17c10000: + $ref: /schemas/watchdog/qcom-wdt.yaml# + +required: + - compatible + - '#address-cells' + - '#size-cells' + - clock-controller@100000 + - watchdog@17c10000 + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + compatible = "qcom,soc-sc7180", "simple-bus"; + + // TODO: Is it possible to ignore the details? + clock-controller@100000 { + compatible = "qcom,gcc-sc7180"; + reg = <0 0x00100000 0 0x1f0000>; + clocks = <&rpmhcc_RPMH_CXO_CLK>, + <&rpmhcc_RPMH_CXO_CLK_A>, + <&sleep_clk>; + clock-names = "bi_tcxo", "bi_tcxo_ao", "sleep_clk"; + #clock-cells = <1>; + #reset-cells = <1>; + #power-domain-cells = <1>; + power-domains = <&rpmhpd_SC7180_CX>; + }; + + watchdog@17c10000 { + compatible = "qcom,apss-wdt-sc7180", "qcom,kpss-wdt"; + reg = <0 0x17c10000 0 0x1000>; + clocks = <&sleep_clk>; + interrupts = <GIC_SPI 0 IRQ_TYPE_EDGE_RISING>; + }; + }; + +...
Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up of multiple devices that have their own bindings, therefore this binding is for a "bus" that is the SoC node. TODO: Document all child nodes. This is woefully incomplete but at least shows what is involved with describing an SoC node in dt schema. Cc: Rob Herring <robh@kernel.org> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Conor Dooley <conor+dt@kernel.org> Cc: <devicetree@vger.kernel.org> Cc: Bjorn Andersson <andersson@kernel.org> Cc: Konrad Dybcio <konradybcio@kernel.org> Cc: <linux-arm-msm@vger.kernel.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- .../bindings/bus/qcom,soc-sc7180.yaml | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml