| Message ID | 20230201080227.473547-1-jun.nie@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings | expand |
On 01/02/2023 09:02, Jun Nie wrote: > Add devicetree binding of Qualcomm CCI on MSM8939. Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Subject: drop second/last, redundant "dt-bindings". The "dt-bindings" prefix is already stating that these are bindings. > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > .../bindings/interconnect/qcom,cci.yaml | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml > > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml > new file mode 100644 > index 000000000000..100c440ba220 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml > @@ -0,0 +1,81 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml# Does not look like you tested the bindings. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Cache Coherent Interconnect (CCI) frequency and voltage scaling > + > +maintainers: > + - Jun Nie <jun.nie@linaro.org> > + > +description: | > + Qualcomm Cache Coherent Interconnect (CCI) is a hardware engine used by > + MSM8939. The driver is to scale its frequency and adjust the voltage in > + hardware accordingly. The voltage provider is modeled as power domain on > + MSM8939, so power domain dts node is required. Don't describe other bindings, but the hardware. Last sentence is not really related. What's more - it does not fit what you wrote below. > + > +properties: > + compatible: > + enum: > + - qcom,msm8939-cci > + > + clocks: > + maxItems: 1 > + > + operating-points-v2: true > + opp-table: > + type: object > + > +required: > + - compatible > + - clocks > + - operating-points-v2 > + - nvmem-cells ?? You cannot require properties which are not present. > + - power-domains Same here. > + > +additionalProperties: false > + > +examples: > + - | > + cci: cci { > + compatible = "qcom,msm8939-cci"; > + clocks = <&apcs2>; Messed indentation. Use 4 spaces for example indentation. > + operating-points-v2 = <&cci_opp_table>; > + power-domains = <&cpr>; > + nvmem-cells = <&cpr_efuse_speedbin_pvs>; How does it pass testing? Best regards, Krzysztof
Hi, In order to track the changes of dt-binding, please add the info as following: diff --git a/MAINTAINERS b/MAINTAINERS index 7f86d02cb427..e13e61218987 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6047,6 +6047,7 @@ S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git F: Documentation/devicetree/bindings/devfreq/ F: Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml +F: Documentation/devicetree/bindings/interconnect/qcom,cci.yam F: drivers/devfreq/ F: include/linux/devfreq.h F: include/trace/events/devfreq.h On 23. 2. 1. 17:02, Jun Nie wrote: > Add devicetree binding of Qualcomm CCI on MSM8939. > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > .../bindings/interconnect/qcom,cci.yaml | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml > > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml > new file mode 100644 > index 000000000000..100c440ba220 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml > @@ -0,0 +1,81 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Cache Coherent Interconnect (CCI) frequency and voltage scaling > + > +maintainers: > + - Jun Nie <jun.nie@linaro.org> > + > +description: | > + Qualcomm Cache Coherent Interconnect (CCI) is a hardware engine used by > + MSM8939. The driver is to scale its frequency and adjust the voltage in > + hardware accordingly. The voltage provider is modeled as power domain on > + MSM8939, so power domain dts node is required. > + > +properties: > + compatible: > + enum: > + - qcom,msm8939-cci > + > + clocks: > + maxItems: 1 > + > + operating-points-v2: true > + opp-table: > + type: object > + > +required: > + - compatible > + - clocks > + - operating-points-v2 > + - nvmem-cells > + - power-domains > + > +additionalProperties: false > + > +examples: > + - | > + cci: cci { > + compatible = "qcom,msm8939-cci"; > + clocks = <&apcs2>; > + operating-points-v2 = <&cci_opp_table>; > + power-domains = <&cpr>; > + nvmem-cells = <&cpr_efuse_speedbin_pvs>; > + }; > + > + cci_opp_table: cci-opp-table { > + compatible = "operating-points-v2"; > + > + cci_opp1: opp-200000000 { > + opp-hz = /bits/ 64 <200000000>; > + opp-supported-hw = <0x3f>; > + required-opps = <&cpr_opp3>; > + }; > + > + cci_opp2: opp-297600000 { > + opp-hz = /bits/ 64 <297600000>; > + opp-supported-hw = <0x3f>; > + required-opps = <&cpr_opp12>; > + }; > + > + cci_opp3: opp-400000000 { > + opp-hz = /bits/ 64 <400000000>; > + opp-supported-hw = <0x1>; > + required-opps = <&cpr_opp14>; > + }; > + > + cci_opp4: opp-400000000 { > + opp-hz = /bits/ 64 <400000000>; > + opp-supported-hw = <0x3e>; > + required-opps = <&cpr_opp15>; > + }; > + > + cci_opp5: opp-595200000 { > + opp-hz = /bits/ 64 <595200000>; > + opp-supported-hw = <0x3f>; > + required-opps = <&cpr_opp17>; > + }; > + };
On Wed, 01 Feb 2023 16:02:26 +0800, Jun Nie wrote: > Add devicetree binding of Qualcomm CCI on MSM8939. > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > .../bindings/interconnect/qcom,cci.yaml | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/interconnect/qcom,cci.yaml:43:1: [error] syntax error: found character '\t' that cannot start any token (syntax) dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/interconnect/qcom,cci.example.dts' Documentation/devicetree/bindings/interconnect/qcom,cci.yaml:43:1: found character '\t' that cannot start any token make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/interconnect/qcom,cci.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/interconnect/qcom,cci.yaml:43:1: found character '\t' that cannot start any token /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml: ignoring, error parsing file make: *** [Makefile:1508: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230201080227.473547-1-jun.nie@linaro.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
> > + > > +properties: > > + compatible: > > + enum: > > + - qcom,msm8939-cci > > + > > + clocks: > > + maxItems: 1 > > + > > + operating-points-v2: true > > + opp-table: > > + type: object > > + > > +required: > > + - compatible > > + - clocks > > + - operating-points-v2 > > + - nvmem-cells > > ?? You cannot require properties which are not present. > > > + - power-domains > > Same here. > So the properties should be added later, after cpr and fuse nodes are available in mainline, right? - Jun
On 02/02/2023 10:29, Jun Nie wrote: >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - qcom,msm8939-cci >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + operating-points-v2: true >>> + opp-table: >>> + type: object >>> + >>> +required: >>> + - compatible >>> + - clocks >>> + - operating-points-v2 >>> + - nvmem-cells >> >> ?? You cannot require properties which are not present. >> >>> + - power-domains >> >> Same here. >> > > So the properties should be added later, after cpr and fuse nodes are > available in mainline, right? No, binding should be complete, so why would you skip some properties? I don't see here dependency on other bindings. Unless I missed here something and there is a dependency? But then what kind? Do you reference other schema? Best regards, Krzysztof
Krzysztof Kozlowski <krzk@kernel.org> 于2023年2月2日周四 17:42写道： > > On 02/02/2023 10:29, Jun Nie wrote: > >>> + > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - qcom,msm8939-cci > >>> + > >>> + clocks: > >>> + maxItems: 1 > >>> + > >>> + operating-points-v2: true > >>> + opp-table: > >>> + type: object > >>> + > >>> +required: > >>> + - compatible > >>> + - clocks > >>> + - operating-points-v2 > >>> + - nvmem-cells > >> > >> ?? You cannot require properties which are not present. > >> > >>> + - power-domains > >> > >> Same here. > >> > > > > So the properties should be added later, after cpr and fuse nodes are > > available in mainline, right? > > No, binding should be complete, so why would you skip some properties? I > don't see here dependency on other bindings. Unless I missed here > something and there is a dependency? But then what kind? Do you > reference other schema? > > Best regards, > Krzysztof > Sorry, it is a copy/modified error with overlooking some added properties. + cci_opp3: opp-cpr14-400000000 { + opp-hz = /bits/ 64 <400000000>; + opp-supported-hw = <0x1>; + required-opps = <&cpr_opp14>; + }; + + cci_opp4: opp-cpr15-400000000 { Documentation/devicetree/bindings/interconnect/qcom,cci.example.dtb: opp-table-cci: Unevaluated properties are not allowed ('opp-cpr14-400000000', 'opp-cpr15-400000000' were unexpected) Do you know how to fix this dts check warning? The cci_opp3 and cci_opp4 have the same frequency but with different requirements to power domain. So the name of the 2 opp should be identical or with a different tag, but there are warnings for both cases for the 2 lines. Thanks! + opp-hz = /bits/ 64 <400000000>; + opp-supported-hw = <0x3e>; + required-opps = <&cpr_opp15>; + };
On 03/02/2023 04:45, Jun Nie wrote: > Krzysztof Kozlowski <krzk@kernel.org> 于2023年2月2日周四 17:42写道： >> >> On 02/02/2023 10:29, Jun Nie wrote: >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + enum: >>>>> + - qcom,msm8939-cci >>>>> + >>>>> + clocks: >>>>> + maxItems: 1 >>>>> + >>>>> + operating-points-v2: true >>>>> + opp-table: >>>>> + type: object >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - clocks >>>>> + - operating-points-v2 >>>>> + - nvmem-cells >>>> >>>> ?? You cannot require properties which are not present. >>>> >>>>> + - power-domains >>>> >>>> Same here. >>>> >>> >>> So the properties should be added later, after cpr and fuse nodes are >>> available in mainline, right? >> >> No, binding should be complete, so why would you skip some properties? I >> don't see here dependency on other bindings. Unless I missed here >> something and there is a dependency? But then what kind? Do you >> reference other schema? >> >> Best regards, >> Krzysztof >> > Sorry, it is a copy/modified error with overlooking some added properties. > > + cci_opp3: opp-cpr14-400000000 { > + opp-hz = /bits/ 64 <400000000>; > + opp-supported-hw = <0x1>; > + required-opps = <&cpr_opp14>; > + }; > + > + cci_opp4: opp-cpr15-400000000 { This does not answer my concerns at all. Please include all relevant properties in your bindings. > Documentation/devicetree/bindings/interconnect/qcom,cci.example.dtb: > opp-table-cci: Unevaluated properties are not allowed > ('opp-cpr14-400000000', 'opp-cpr15-400000000' were unexpected) > > Do you know how to fix this dts check warning? Don't add properties which are not allowed or do not name properties in a way it is not allowed. > The cci_opp3 and cci_opp4 have the same frequency but with different > requirements to So you used the same node names... how do you expect such code to compile? In current form bindings do not allow such setup. If you think that OS should allow and support it, then change the bindings. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml new file mode 100644 index 000000000000..100c440ba220 --- /dev/null +++ b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml @@ -0,0 +1,81 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Cache Coherent Interconnect (CCI) frequency and voltage scaling + +maintainers: + - Jun Nie <jun.nie@linaro.org> + +description: | + Qualcomm Cache Coherent Interconnect (CCI) is a hardware engine used by + MSM8939. The driver is to scale its frequency and adjust the voltage in + hardware accordingly. The voltage provider is modeled as power domain on + MSM8939, so power domain dts node is required. + +properties: + compatible: + enum: + - qcom,msm8939-cci + + clocks: + maxItems: 1 + + operating-points-v2: true + opp-table: + type: object + +required: + - compatible + - clocks + - operating-points-v2 + - nvmem-cells + - power-domains + +additionalProperties: false + +examples: + - | + cci: cci { + compatible = "qcom,msm8939-cci"; + clocks = <&apcs2>; + operating-points-v2 = <&cci_opp_table>; + power-domains = <&cpr>; + nvmem-cells = <&cpr_efuse_speedbin_pvs>; + }; + + cci_opp_table: cci-opp-table { + compatible = "operating-points-v2"; + + cci_opp1: opp-200000000 { + opp-hz = /bits/ 64 <200000000>; + opp-supported-hw = <0x3f>; + required-opps = <&cpr_opp3>; + }; + + cci_opp2: opp-297600000 { + opp-hz = /bits/ 64 <297600000>; + opp-supported-hw = <0x3f>; + required-opps = <&cpr_opp12>; + }; + + cci_opp3: opp-400000000 { + opp-hz = /bits/ 64 <400000000>; + opp-supported-hw = <0x1>; + required-opps = <&cpr_opp14>; + }; + + cci_opp4: opp-400000000 { + opp-hz = /bits/ 64 <400000000>; + opp-supported-hw = <0x3e>; + required-opps = <&cpr_opp15>; + }; + + cci_opp5: opp-595200000 { + opp-hz = /bits/ 64 <595200000>; + opp-supported-hw = <0x3f>; + required-opps = <&cpr_opp17>; + }; + };
Add devicetree binding of Qualcomm CCI on MSM8939. Signed-off-by: Jun Nie <jun.nie@linaro.org> --- .../bindings/interconnect/qcom,cci.yaml | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml