| Message ID | 20220712063641.2790997-4-jiucheng.xu@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] perf/amlogic: Add support for Amlogic meson G12 SoC DDR PMU driver | expand |
On 12/07/2022 08:36, Jiucheng Xu wrote: > Add binding documentation for the Amlogic G12 series DDR > performance monitor unit. You need to fix subject - use a prefix matching subsystem. Space after each ':'. > > Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com> > --- > .../devicetree/bindings/perf/aml-ddr-pmu.yaml | 51 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 52 insertions(+) > create mode 100644 Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml > > diff --git a/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml > new file mode 100644 > index 000000000000..c586b4ab4009 > --- /dev/null > +++ b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml Filename: aml,g12-ddr-pmu.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/perf/aml-ddr-pmu.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic G12 DDR performance monitor > + > +maintainers: > + - Jiucheng Xu <jiucheng.xu@amlogic.com> > + > +properties: > + compatible: > + oneOf: > + - enum: > + - aml,g12-ddr-pmu > + - items: > + - enum: > + - aml,g12-ddr-pmu > + - const: aml,g12-ddr-pmu This does not make any sense. Why do you use two compatibles "aml,g12-ddr-pmu", "aml,g12-ddr-pmu" after each other? > + > + reg: > + maxItems: 2 You need to describe the items. > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - model > + - dmc_nr > + - chann_nr How these ended up here? No underscores. > + - reg > + - interrupts > + - interrupt-names Also something new. No. > + > +additionalProperties: false > + > +examples: > + - | > + ddr_pmu: ddr_pmu { Wrong indentation. 4 spaces for DTS example. Generic node name, so "pmu", no underscores in node names. > + compatible = "amlogic,g12-ddr-pmu"; > + model = "g12a"; > + dmc_nr = <1>; > + chann_nr = <4>; This does not pass the test. Please do not send untested bindings. Best regards, Krzysztof
On 2022-07-12 07:36, Jiucheng Xu wrote: > Add binding documentation for the Amlogic G12 series DDR > performance monitor unit. > > Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com> > --- > .../devicetree/bindings/perf/aml-ddr-pmu.yaml | 51 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 52 insertions(+) > create mode 100644 Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml > > diff --git a/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml > new file mode 100644 > index 000000000000..c586b4ab4009 > --- /dev/null > +++ b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/perf/aml-ddr-pmu.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic G12 DDR performance monitor > + > +maintainers: > + - Jiucheng Xu <jiucheng.xu@amlogic.com> > + > +properties: > + compatible: > + oneOf: > + - enum: > + - aml,g12-ddr-pmu > + - items: > + - enum: > + - aml,g12-ddr-pmu > + - const: aml,g12-ddr-pmu Judging by what the driver actually implements, this should probably be: compatible: items: - enum: - amlogic,g12a-ddr-pmu - amlogic,g12b-ddr-pmu - amlogic,sm1-ddr-pmu - const: amlogic,g12-ddr-pmu There doesn't seem much point in allowing only the common compatible without a SoC-specific identifier. Note also that "aml," is not the documented vendor prefix. > + > + reg: > + maxItems: 2 > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - model Remove this, and use the compatible strings properly as above. > + - dmc_nr > + - chann_nr I suspect those could probably be inferred from the correct compatible string, but if not (i.e. within one SoC you have multiple PMUs supporting the same events but with different numbers of usable channels), then document what exactly they mean. > + - reg > + - interrupts > + - interrupt-names As mentioned in the driver review, if you really want to use a named interrupt (which should usually be unnecessary when there's only one), it has to be a defined name. DT is not a mechanism for overriding what Linux shows in /proc/interrupts. Thanks, Robin. > + > +additionalProperties: false > + > +examples: > + - | > + ddr_pmu: ddr_pmu { > + compatible = "amlogic,g12-ddr-pmu"; > + model = "g12a"; > + dmc_nr = <1>; > + chann_nr = <4>; > + reg = <0x0 0xff638000 0x0 0x100 > + 0x0 0xff638c00 0x0 0x100>; > + interrupts = <GIC_SPI 52 IRQ_TYPE_EDGE_RISING>; > + interrupt-names = "ddr_pmu"; > + }; > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index fd2a56a339b4..ac0a1df4622d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1055,6 +1055,7 @@ M: Jiucheng Xu <jiucheng.xu@amlogic.com> > S: Supported > W: http://www.amlogic.com > F: Documentation/admin-guide/perf/aml-ddr-pmu.rst > +F: Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml > F: drivers/perf/amlogic/ > F: include/soc/amlogic/ >
On Tue, 12 Jul 2022 14:36:41 +0800, Jiucheng Xu wrote: > Add binding documentation for the Amlogic G12 series DDR > performance monitor unit. > > Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com> > --- > .../devicetree/bindings/perf/aml-ddr-pmu.yaml | 51 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 52 insertions(+) > create mode 100644 Documentation/devicetree/bindings/perf/aml-ddr-pmu.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: dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/perf/aml-ddr-pmu.example.dts' Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml:51:5: did not find expected '-' indicator make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/perf/aml-ddr-pmu.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... Traceback (most recent call last): File "/usr/bin/yamllint", line 33, in <module> sys.exit(load_entry_point('yamllint==1.26.3', 'console_scripts', 'yamllint')()) File "/usr/lib/python3/dist-packages/yamllint/cli.py", line 210, in run prob_level = show_problems(problems, file, args_format=args.format, File "/usr/lib/python3/dist-packages/yamllint/cli.py", line 106, in show_problems for problem in problems: File "/usr/lib/python3/dist-packages/yamllint/linter.py", line 203, in _run for problem in get_cosmetic_problems(buffer, conf, filepath): File "/usr/lib/python3/dist-packages/yamllint/linter.py", line 140, in get_cosmetic_problems for problem in rule.check(rule_conf, File "/usr/lib/python3/dist-packages/yamllint/rules/indentation.py", line 580, in check for problem in _check(conf, token, prev, next, nextnext, context): File "/usr/lib/python3/dist-packages/yamllint/rules/indentation.py", line 346, in _check 'wrong indentation: expected %d but found %d' % TypeError: %d format: a real number is required, not NoneType ./Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml:51:5: did not find expected '-' indicator /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml: ignoring, error parsing file make: *** [Makefile:1404: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
Thanks for your time ^^. On 7/12/2022 3:15 PM, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 12/07/2022 08:36, Jiucheng Xu wrote: >> Add binding documentation for the Amlogic G12 series DDR >> performance monitor unit. > You need to fix subject - use a prefix matching subsystem. Space after > each ':'. I will make the change. > >> Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com> >> --- >> .../devicetree/bindings/perf/aml-ddr-pmu.yaml | 51 +++++++++++++++++++ >> MAINTAINERS | 1 + >> 2 files changed, 52 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml >> >> diff --git a/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml >> new file mode 100644 >> index 000000000000..c586b4ab4009 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml > Filename: aml,g12-ddr-pmu.yaml I will make the change. > >> @@ -0,0 +1,51 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/perf/aml-ddr-pmu.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Amlogic G12 DDR performance monitor >> + >> +maintainers: >> + - Jiucheng Xu <jiucheng.xu@amlogic.com> >> + >> +properties: >> + compatible: >> + oneOf: >> + - enum: >> + - aml,g12-ddr-pmu >> + - items: >> + - enum: >> + - aml,g12-ddr-pmu >> + - const: aml,g12-ddr-pmu > This does not make any sense. Why do you use two compatibles > "aml,g12-ddr-pmu", "aml,g12-ddr-pmu" after each other? Sorry, I think I have a wrong understanding. I will make the change. > >> + >> + reg: >> + maxItems: 2 > You need to describe the items. I will make the change. > >> + >> + interrupts: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - model >> + - dmc_nr >> + - chann_nr > How these ended up here? > No underscores. I will make the change. > >> + - reg >> + - interrupts >> + - interrupt-names > Also something new. No. I will make the change. > >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + ddr_pmu: ddr_pmu { > Wrong indentation. 4 spaces for DTS example. > > Generic node name, so "pmu", no underscores in node names. Okay, I will make the change. > >> + compatible = "amlogic,g12-ddr-pmu"; >> + model = "g12a"; >> + dmc_nr = <1>; >> + chann_nr = <4>; > This does not pass the test. Please do not send untested bindings. Sorry, due to some problems, I got wrong patch sent. I will make the change. Thanks & Best Regards, Jiucheng > > > Best regards, > Krzysztof >
On 7/12/2022 8:54 PM, Robin Murphy wrote: > [ EXTERNAL EMAIL ] > > On 2022-07-12 07:36, Jiucheng Xu wrote: >> Add binding documentation for the Amlogic G12 series DDR >> performance monitor unit. >> >> Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com> >> --- >> .../devicetree/bindings/perf/aml-ddr-pmu.yaml | 51 +++++++++++++++++++ >> MAINTAINERS | 1 + >> 2 files changed, 52 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml >> >> diff --git a/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml >> b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml >> new file mode 100644 >> index 000000000000..c586b4ab4009 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml >> @@ -0,0 +1,51 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/perf/aml-ddr-pmu.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Amlogic G12 DDR performance monitor >> + >> +maintainers: >> + - Jiucheng Xu <jiucheng.xu@amlogic.com> >> + >> +properties: >> + compatible: >> + oneOf: >> + - enum: >> + - aml,g12-ddr-pmu >> + - items: >> + - enum: >> + - aml,g12-ddr-pmu >> + - const: aml,g12-ddr-pmu > > Judging by what the driver actually implements, this should probably be: > > compatible: > items: > - enum: > - amlogic,g12a-ddr-pmu > - amlogic,g12b-ddr-pmu > - amlogic,sm1-ddr-pmu > - const: amlogic,g12-ddr-pmu > > There doesn't seem much point in allowing only the common compatible > without a SoC-specific identifier. Note also that "aml," is not the > documented vendor prefix. Okay, I finally know what you mean. > >> + >> + reg: >> + maxItems: 2 >> + >> + interrupts: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - model > > Remove this, and use the compatible strings properly as above. Okay. I will make the change. > >> + - dmc_nr >> + - chann_nr > > I suspect those could probably be inferred from the correct compatible > string, but if not (i.e. within one SoC you have multiple PMUs > supporting the same events but with different numbers of usable > channels), then document what exactly they mean. > Yes, as you mentioned, these could be inferred from the compatible string. I will make the change. >> + - reg >> + - interrupts >> + - interrupt-names > > As mentioned in the driver review, if you really want to use a named > interrupt (which should usually be unnecessary when there's only one), > it has to be a defined name. DT is not a mechanism for overriding what > Linux shows in /proc/interrupts. > > Thanks, > Robin. > >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + ddr_pmu: ddr_pmu { >> + compatible = "amlogic,g12-ddr-pmu"; >> + model = "g12a"; >> + dmc_nr = <1>; >> + chann_nr = <4>; >> + reg = <0x0 0xff638000 0x0 0x100 >> + 0x0 0xff638c00 0x0 0x100>; >> + interrupts = <GIC_SPI 52 IRQ_TYPE_EDGE_RISING>; >> + interrupt-names = "ddr_pmu"; >> + }; >> + }; >> diff --git a/MAINTAINERS b/MAINTAINERS >> index fd2a56a339b4..ac0a1df4622d 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -1055,6 +1055,7 @@ M: Jiucheng Xu <jiucheng.xu@amlogic.com> >> S: Supported >> W: http://www.amlogic.com >> F: Documentation/admin-guide/perf/aml-ddr-pmu.rst >> +F: Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml >> F: drivers/perf/amlogic/ >> F: include/soc/amlogic/ >
diff --git a/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml new file mode 100644 index 000000000000..c586b4ab4009 --- /dev/null +++ b/Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/perf/aml-ddr-pmu.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic G12 DDR performance monitor + +maintainers: + - Jiucheng Xu <jiucheng.xu@amlogic.com> + +properties: + compatible: + oneOf: + - enum: + - aml,g12-ddr-pmu + - items: + - enum: + - aml,g12-ddr-pmu + - const: aml,g12-ddr-pmu + + reg: + maxItems: 2 + + interrupts: + maxItems: 1 + +required: + - compatible + - model + - dmc_nr + - chann_nr + - reg + - interrupts + - interrupt-names + +additionalProperties: false + +examples: + - | + ddr_pmu: ddr_pmu { + compatible = "amlogic,g12-ddr-pmu"; + model = "g12a"; + dmc_nr = <1>; + chann_nr = <4>; + reg = <0x0 0xff638000 0x0 0x100 + 0x0 0xff638c00 0x0 0x100>; + interrupts = <GIC_SPI 52 IRQ_TYPE_EDGE_RISING>; + interrupt-names = "ddr_pmu"; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index fd2a56a339b4..ac0a1df4622d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1055,6 +1055,7 @@ M: Jiucheng Xu <jiucheng.xu@amlogic.com> S: Supported W: http://www.amlogic.com F: Documentation/admin-guide/perf/aml-ddr-pmu.rst +F: Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml F: drivers/perf/amlogic/ F: include/soc/amlogic/
Add binding documentation for the Amlogic G12 series DDR performance monitor unit. Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com> --- .../devicetree/bindings/perf/aml-ddr-pmu.yaml | 51 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/perf/aml-ddr-pmu.yaml