Convert kpss-gcc driver Documentation to yaml.
Add #clock-cells additional binding to required bindings and example
as it's a required binding for clock-output-names.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
.../bindings/arm/msm/qcom,kpss-gcc.txt | 44 ------------
.../bindings/arm/msm/qcom,kpss-gcc.yaml | 68 +++++++++++++++++++
2 files changed, 68 insertions(+), 44 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
deleted file mode 100644
index e628758950e1..000000000000
--- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
+++ /dev/null
@@ -1,44 +0,0 @@
-Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
-
-PROPERTIES
-
-- compatible:
- Usage: required
- Value type: <string>
- Definition: should be one of the following. The generic compatible
- "qcom,kpss-gcc" should also be included.
- "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
- "qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
- "qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
- "qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
-
-- reg:
- Usage: required
- Value type: <prop-encoded-array>
- Definition: base address and size of the register region
-
-- clocks:
- Usage: required
- Value type: <prop-encoded-array>
- Definition: reference to the pll parents.
-
-- clock-names:
- Usage: required
- Value type: <stringlist>
- Definition: must be "pll8_vote", "pxo".
-
-- clock-output-names:
- Usage: required
- Value type: <string>
- Definition: Name of the output clock. Typically acpu_l2_aux indicating
- an L2 cache auxiliary clock.
-
-Example:
-
- l2cc: clock-controller@2011000 {
- compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
- reg = <0x2011000 0x1000>;
- clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
- clock-names = "pll8_vote", "pxo";
- clock-output-names = "acpu_l2_aux";
- };
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
new file mode 100644
index 000000000000..20ee182eb16f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
+
+maintainers:
+ - Ansuel Smith <ansuelsmth@gmail.com>
+
+description: |
+ Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
+ to control L2 mux (in the current implementation).
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,kpss-gcc-ipq8064
+ - qcom,kpss-gcc-apq8064
+ - qcom,kpss-gcc-msm8974
+ - qcom,kpss-gcc-msm8960
+ - const: qcom,kpss-gcc
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: phandle to pll8_vote
+ - description: phandle to pxo_board
+
+ clock-names:
+ items:
+ - const: pll8_vote
+ - const: pxo
+
+ clock-output-names:
+ const: acpu_l2_aux
+
+ '#clock-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - clock-output-names
+ - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,gcc-ipq806x.h>
+
+ clock-controller@2011000 {
+ compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
+ reg = <0x2011000 0x1000>;
+ clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
+ clock-names = "pll8_vote", "pxo";
+ clock-output-names = "acpu_l2_aux";
+ #clock-cells = <0>;
+ };
+...
+
--
2.34.1On Sat, 30 Apr 2022 08:01:25 +0200, Ansuel Smith wrote: > Convert kpss-gcc driver Documentation to yaml. > Add #clock-cells additional binding to required bindings and example > as it's a required binding for clock-output-names. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > .../bindings/arm/msm/qcom,kpss-gcc.txt | 44 ------------ > .../bindings/arm/msm/qcom,kpss-gcc.yaml | 68 +++++++++++++++++++ > 2 files changed, 68 insertions(+), 44 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt > create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml > Running 'make dtbs_check' with the schema in this patch gives the following warnings. Consider if they are expected or the schema is incorrect. These may not be new warnings. Note that it is not yet a requirement to have 0 warnings for dtbs_check. This will change in the future. Full log is available here: https://patchwork.ozlabs.org/patch/ clock-controller@2011000: '#clock-cells' is a required property arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dtb arch/arm/boot/dts/qcom-apq8064-cm-qs600.dtb arch/arm/boot/dts/qcom-apq8064-ifc6410.dtb arch/arm/boot/dts/qcom-apq8064-sony-xperia-yuga.dtb arch/arm/boot/dts/qcom-ipq8064-ap148.dtb arch/arm/boot/dts/qcom-ipq8064-rb3011.dtb arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dtb arch/arm/boot/dts/qcom-msm8960-cdp.dtb clock-controller@2011000: 'clock-names' is a required property arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dtb arch/arm/boot/dts/qcom-apq8064-cm-qs600.dtb arch/arm/boot/dts/qcom-apq8064-ifc6410.dtb arch/arm/boot/dts/qcom-apq8064-sony-xperia-yuga.dtb arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dtb arch/arm/boot/dts/qcom-msm8960-cdp.dtb clock-controller@2011000: 'clock-output-names' is a required property arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dtb arch/arm/boot/dts/qcom-apq8064-cm-qs600.dtb arch/arm/boot/dts/qcom-apq8064-ifc6410.dtb arch/arm/boot/dts/qcom-apq8064-sony-xperia-yuga.dtb arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dtb arch/arm/boot/dts/qcom-msm8960-cdp.dtb clock-controller@2011000: 'clocks' is a required property arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dtb arch/arm/boot/dts/qcom-apq8064-cm-qs600.dtb arch/arm/boot/dts/qcom-apq8064-ifc6410.dtb arch/arm/boot/dts/qcom-apq8064-sony-xperia-yuga.dtb arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dtb arch/arm/boot/dts/qcom-msm8960-cdp.dtb clock-controller@2011000: compatible:0: 'qcom,kpss-gcc' is not one of ['qcom,kpss-gcc-ipq8064', 'qcom,kpss-gcc-apq8064', 'qcom,kpss-gcc-msm8974', 'qcom,kpss-gcc-msm8960'] arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dtb arch/arm/boot/dts/qcom-apq8064-cm-qs600.dtb arch/arm/boot/dts/qcom-apq8064-ifc6410.dtb arch/arm/boot/dts/qcom-apq8064-sony-xperia-yuga.dtb arch/arm/boot/dts/qcom-ipq8064-ap148.dtb arch/arm/boot/dts/qcom-ipq8064-rb3011.dtb arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dtb arch/arm/boot/dts/qcom-msm8960-cdp.dtb clock-controller@2011000: compatible:1: 'qcom,kpss-gcc' was expected arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dtb arch/arm/boot/dts/qcom-apq8064-cm-qs600.dtb arch/arm/boot/dts/qcom-apq8064-ifc6410.dtb arch/arm/boot/dts/qcom-apq8064-sony-xperia-yuga.dtb arch/arm/boot/dts/qcom-ipq8064-ap148.dtb arch/arm/boot/dts/qcom-ipq8064-rb3011.dtb arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dtb arch/arm/boot/dts/qcom-msm8960-cdp.dtb clock-controller@2082000: '#clock-cells' is a required property arch/arm/boot/dts/qcom-apq8060-dragonboard.dtb arch/arm/boot/dts/qcom-msm8660-surf.dtb clock-controller@2082000: 'clock-names' is a required property arch/arm/boot/dts/qcom-apq8060-dragonboard.dtb arch/arm/boot/dts/qcom-msm8660-surf.dtb clock-controller@2082000: 'clock-output-names' is a required property arch/arm/boot/dts/qcom-apq8060-dragonboard.dtb arch/arm/boot/dts/qcom-msm8660-surf.dtb clock-controller@2082000: 'clocks' is a required property arch/arm/boot/dts/qcom-apq8060-dragonboard.dtb arch/arm/boot/dts/qcom-msm8660-surf.dtb clock-controller@2082000: compatible:0: 'qcom,kpss-gcc' is not one of ['qcom,kpss-gcc-ipq8064', 'qcom,kpss-gcc-apq8064', 'qcom,kpss-gcc-msm8974', 'qcom,kpss-gcc-msm8960'] arch/arm/boot/dts/qcom-apq8060-dragonboard.dtb arch/arm/boot/dts/qcom-msm8660-surf.dtb clock-controller@2082000: compatible:1: 'qcom,kpss-gcc' was expected arch/arm/boot/dts/qcom-apq8060-dragonboard.dtb arch/arm/boot/dts/qcom-msm8660-surf.dtb
On 30/04/2022 08:01, Ansuel Smith wrote: > Convert kpss-gcc driver Documentation to yaml. > Add #clock-cells additional binding to required bindings and example > as it's a required binding for clock-output-names. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> (...) > +properties: > + compatible: > + items: > + - enum: > + - qcom,kpss-gcc-ipq8064 > + - qcom,kpss-gcc-apq8064 > + - qcom,kpss-gcc-msm8974 > + - qcom,kpss-gcc-msm8960 > + - const: qcom,kpss-gcc > + > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: phandle to pll8_vote > + - description: phandle to pxo_board > + > + clock-names: > + items: > + - const: pll8_vote > + - const: pxo > + > + clock-output-names: > + const: acpu_l2_aux It does not make sense having a constant output name. What is the meaning this property in such case? The original binding did not enforce it. Best regards, Krzysztof
On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote: > On 30/04/2022 08:01, Ansuel Smith wrote: > > Convert kpss-gcc driver Documentation to yaml. > > Add #clock-cells additional binding to required bindings and example > > as it's a required binding for clock-output-names. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > (...) > > > +properties: > > + compatible: > > + items: > > + - enum: > > + - qcom,kpss-gcc-ipq8064 > > + - qcom,kpss-gcc-apq8064 > > + - qcom,kpss-gcc-msm8974 > > + - qcom,kpss-gcc-msm8960 > > + - const: qcom,kpss-gcc > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + items: > > + - description: phandle to pll8_vote > > + - description: phandle to pxo_board > > + > > + clock-names: > > + items: > > + - const: pll8_vote > > + - const: pxo > > + > > + clock-output-names: > > + const: acpu_l2_aux > > It does not make sense having a constant output name. What is the > meaning this property in such case? The original binding did not enforce it. > > > > Best regards, > Krzysztof Mh. Should I just drop the const and put a description referring to an advised name? The driver with the kpss-gcc hardcode the name to acpu_l2_aux that's why I thought it was a correct conversion using a const but I assume this is another problem of not making a correct 1:1 conversion and adding fixes on pure conversion. Think I should drop it and put a description to it. (and then later fix it when I will push the other series with all the tweaks) What do you think? -- Ansuel
On Sat 30 Apr 02:42 CDT 2022, Ansuel Smith wrote: > On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote: > > On 30/04/2022 08:01, Ansuel Smith wrote: > > > Convert kpss-gcc driver Documentation to yaml. > > > Add #clock-cells additional binding to required bindings and example > > > as it's a required binding for clock-output-names. > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > > > (...) > > > > > +properties: > > > + compatible: > > > + items: > > > + - enum: > > > + - qcom,kpss-gcc-ipq8064 > > > + - qcom,kpss-gcc-apq8064 > > > + - qcom,kpss-gcc-msm8974 > > > + - qcom,kpss-gcc-msm8960 > > > + - const: qcom,kpss-gcc > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + items: > > > + - description: phandle to pll8_vote > > > + - description: phandle to pxo_board > > > + > > > + clock-names: > > > + items: > > > + - const: pll8_vote > > > + - const: pxo > > > + > > > + clock-output-names: > > > + const: acpu_l2_aux > > > > It does not make sense having a constant output name. What is the > > meaning this property in such case? The original binding did not enforce it. > > > > > > > > Best regards, > > Krzysztof > > Mh. Should I just drop the const and put a description referring to an > advised name? The driver with the kpss-gcc hardcode the name to > acpu_l2_aux that's why I thought it was a correct conversion using a > const but I assume this is another problem of not making a correct 1:1 > conversion and adding fixes on pure conversion. > Think I should drop it and put a description to it. (and then later fix > it when I will push the other series with all the tweaks) > > What do you think? > The typical reason for using clock-output-names is that we have some consumer that finds the clock based on global name lookup. Over time we've been moving these to use .fw_name or .index based lookup, which removes this problem. But I don't see that being the case here. So my suggestion is that you just drop clock-output-names from the binding, which will solve Krzysztof's objection. From there we can review what needs to be done in the Linux driver to work with the improved binding. Regards, Bjorn
On 30/04/2022 09:42, Ansuel Smith wrote: > On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote: >> On 30/04/2022 08:01, Ansuel Smith wrote: >>> Convert kpss-gcc driver Documentation to yaml. >>> Add #clock-cells additional binding to required bindings and example >>> as it's a required binding for clock-output-names. >>> >>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> >> >> >> (...) >> >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - qcom,kpss-gcc-ipq8064 >>> + - qcom,kpss-gcc-apq8064 >>> + - qcom,kpss-gcc-msm8974 >>> + - qcom,kpss-gcc-msm8960 >>> + - const: qcom,kpss-gcc >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + items: >>> + - description: phandle to pll8_vote >>> + - description: phandle to pxo_board >>> + >>> + clock-names: >>> + items: >>> + - const: pll8_vote >>> + - const: pxo >>> + >>> + clock-output-names: >>> + const: acpu_l2_aux >> >> It does not make sense having a constant output name. What is the >> meaning this property in such case? The original binding did not enforce it. >> >> >> >> Best regards, >> Krzysztof > > Mh. Should I just drop the const and put a description referring to an > advised name? The driver with the kpss-gcc hardcode the name to > acpu_l2_aux that's why I thought it was a correct conversion using a > const but I assume this is another problem of not making a correct 1:1 > conversion and adding fixes on pure conversion. Hard-coding a name by implementation is not a reason to put that name in DTS. DTS is not a place for values stored in the driver. > Think I should drop it and put a description to it. (and then later fix > it when I will push the other series with all the tweaks) The driver kpss-gcc does not use this property at all, so I am not sure if there is a point to even keep it. Any other user of bindings makes use of the property? Best regards, Krzysztof
On Sun, May 01, 2022 at 10:26:47AM +0200, Krzysztof Kozlowski wrote: > On 30/04/2022 09:42, Ansuel Smith wrote: > > On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote: > >> On 30/04/2022 08:01, Ansuel Smith wrote: > >>> Convert kpss-gcc driver Documentation to yaml. > >>> Add #clock-cells additional binding to required bindings and example > >>> as it's a required binding for clock-output-names. > >>> > >>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > >> > >> > >> (...) > >> > >>> +properties: > >>> + compatible: > >>> + items: > >>> + - enum: > >>> + - qcom,kpss-gcc-ipq8064 > >>> + - qcom,kpss-gcc-apq8064 > >>> + - qcom,kpss-gcc-msm8974 > >>> + - qcom,kpss-gcc-msm8960 > >>> + - const: qcom,kpss-gcc > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + clocks: > >>> + items: > >>> + - description: phandle to pll8_vote > >>> + - description: phandle to pxo_board > >>> + > >>> + clock-names: > >>> + items: > >>> + - const: pll8_vote > >>> + - const: pxo > >>> + > >>> + clock-output-names: > >>> + const: acpu_l2_aux > >> > >> It does not make sense having a constant output name. What is the > >> meaning this property in such case? The original binding did not enforce it. > >> > >> > >> > >> Best regards, > >> Krzysztof > > > > Mh. Should I just drop the const and put a description referring to an > > advised name? The driver with the kpss-gcc hardcode the name to > > acpu_l2_aux that's why I thought it was a correct conversion using a > > const but I assume this is another problem of not making a correct 1:1 > > conversion and adding fixes on pure conversion. > > Hard-coding a name by implementation is not a reason to put that name in > DTS. DTS is not a place for values stored in the driver. > > > Think I should drop it and put a description to it. (and then later fix > > it when I will push the other series with all the tweaks) > > The driver kpss-gcc does not use this property at all, so I am not sure > if there is a point to even keep it. Any other user of bindings makes > use of the property? > > > Best regards, > Krzysztof The idea is that you put the clk name in 'clock-output-names' and the driver needs to have support for it (and set the clk name based on the name defined in the dts) This driver doesn't have support for it and is actually hardcoded. So you are right and I should just drop it. But now another question... Since #clock-cells was added as a requirement for clock-output-names, should I drop also that? In theory #clock-cells should always be declared for clock providers, is it right to add it in the conversion commit or I should put this change in another commit? (since it's now an addition and now something required to fix a bot warning) -- Ansuel
On 02/05/2022 12:40, Ansuel Smith wrote: > > The idea is that you put the clk name in 'clock-output-names' and the > driver needs to have support for it (and set the clk name based on the > name defined in the dts) > > This driver doesn't have support for it and is actually hardcoded. > So you are right and I should just drop it. > > But now another question... Since #clock-cells was added as a > requirement for clock-output-names, should I drop also that? > > In theory #clock-cells should always be declared for clock providers, is > it right to add it in the conversion commit or I should put this change > in another commit? (since it's now an addition and now something required > to fix a bot warning) These are not the best bindings to convert, if you are not into the qcom DTS and drivers. :) It looks like the bindings were added to match current Linux implementation and in this implementation the device is not used in DTS as a clock provider (even though it registers a clock) but as a syscon. I am not even sure if it is used as a clock provider outside of DTS (through using a fixed clock name in some clock consumer). Probably this should be made either a proper clock controller or something stripped down to the point matching current usage (accepting the fact that bindings are incomplete). Anyway your choice should be made according to how this device and its driver fit to entire system. IOW, it's not a simple binding conversion and you should not just convert it to make dtbs_check happy. Best regards, Krzysztof
On Mon, May 02, 2022 at 10:18:31PM +0200, Krzysztof Kozlowski wrote: > On 02/05/2022 12:40, Ansuel Smith wrote: > > > > The idea is that you put the clk name in 'clock-output-names' and the > > driver needs to have support for it (and set the clk name based on the > > name defined in the dts) > > > > This driver doesn't have support for it and is actually hardcoded. > > So you are right and I should just drop it. > > > > But now another question... Since #clock-cells was added as a > > requirement for clock-output-names, should I drop also that? > > > > In theory #clock-cells should always be declared for clock providers, is > > it right to add it in the conversion commit or I should put this change > > in another commit? (since it's now an addition and now something required > > to fix a bot warning) > > These are not the best bindings to convert, if you are not into the qcom > DTS and drivers. :) > As I said some times ago main problem of these thing is that they are ancient and they were not in a good state from the start. So it's hard to fix this in a sane way. (i'm trying tho so sorry for any mess i'm making in the process) > It looks like the bindings were added to match current Linux > implementation and in this implementation the device is not used in DTS > as a clock provider (even though it registers a clock) but as a syscon. > I am not even sure if it is used as a clock provider outside of DTS > (through using a fixed clock name in some clock consumer). > > Probably this should be made either a proper clock controller or > something stripped down to the point matching current usage (accepting > the fact that bindings are incomplete). Anyway your choice should be > made according to how this device and its driver fit to entire system. > IOW, it's not a simple binding conversion and you should not just > convert it to make dtbs_check happy. I wonder if I should just drop this patch from the series and push it later in another series as a rework directly (with also the other relevant changes to fix the current problem we have with all the dts). The intention of this series was to try to push small changes that could be merged in a short time. > > Best regards, > Krzysztof -- Ansuel
© 2016 - 2025 Red Hat, Inc.