linux-kernel - Re: [PATCH 4/4] dt-binding:perf: Add Amlogic DDR PMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Jul 2022 13:54:37 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Jiucheng Xu <jiucheng.xu@...ogic.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, devicetree@...r.kernel.org
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Chris Healy <cphealy@...il.com>
Subject: Re: [PATCH 4/4] dt-binding:perf: Add Amlogic DDR PMU

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@...ogic.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@...ogic.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@...ogic.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/
>