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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxx>
---
  .../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@xxxxxxxxxxx>
+
+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@xxxxxxxxxxx>
  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/




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux