diff mbox series

[v4,4/4] dt-binding: perf: Add Amlogic DDR PMU

Message ID 20220805071426.2598818-4-jiucheng.xu@amlogic.com (mailing list archive)
State Superseded
Headers show
Series [v4,1/4] perf/amlogic: Add support for Amlogic meson G12 SoC DDR PMU driver | expand

Commit Message

Jiucheng Xu Aug. 5, 2022, 7:14 a.m. UTC 
Add binding documentation for the Amlogic G12 series DDR
performance monitor unit.

Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com>
---
Changes v3 -> v4:
  - Fix "$id: relative path/filename doesn't match actual path or
    filename" warning

Changes v2 -> v3:
  - Remove oneOf
  - Add descriptions
  - Fix compiling warning

Changes v1 -> v2:
  - Rename file, from aml_ddr_pmu.yaml to amlogic,g12_ddr_pmu.yaml
  - Delete "model", "dmc_nr", "chann_nr" new properties
  - Fix compiling error
---
 .../bindings/perf/amlogic,g12-ddr-pmu.yaml    | 52 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml

Comments

Krzysztof Kozlowski Aug. 5, 2022, 8:21 a.m. UTC | #1 
On 05/08/2022 09:14, Jiucheng Xu wrote:
> Add binding documentation for the Amlogic G12 series DDR
> performance monitor unit.
> 

Bindings go first in the series, before their implementation.

> Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com>
> ---
> Changes v3 -> v4:
>   - Fix "$id: relative path/filename doesn't match actual path or
>     filename" warning
> 
> Changes v2 -> v3:
>   - Remove oneOf
>   - Add descriptions
>   - Fix compiling warning
> 
> Changes v1 -> v2:
>   - Rename file, from aml_ddr_pmu.yaml to amlogic,g12_ddr_pmu.yaml
>   - Delete "model", "dmc_nr", "chann_nr" new properties
>   - Fix compiling error
> ---
>  .../bindings/perf/amlogic,g12-ddr-pmu.yaml    | 52 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
> new file mode 100644
> index 000000000000..4d018574ffd1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/perf/amlogic,g12-ddr-pmu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic G12 DDR performance monitor
> +
> +maintainers:
> +  - Jiucheng Xu <jiucheng.xu@amlogic.com>
> +
> +description: |
> +  Amlogic G12 series SoC integrate DDR bandwidth monitor.
> +  A timer is inside and can generate interrupt when timeout.
> +  The bandwidth is counted in the timer ISR. Different platform
> +  has different subset of event format attribute.
> +
> +properties:
> +  compatible:
> +    items:

You have only one item, so remove "items".

> +      - enum:
> +          - amlogic,g12b-ddr-pmu
> +          - amlogic,g12a-ddr-pmu
> +          - amlogic,sm1-ddr-pmu
> +
> +  reg:
> +    items:
> +      - description: Physical address of DMC bandwidth register
> +          and size of the configuration address space.

Skip obvious pieces, so only:

description: DMC bandwidth register space

> +      - description: Physical address of DMC PLL register and
> +          size of the configuration address space.

description: DMC PLL bandwidth register space


> +
> +  interrupts:
> +    items:
> +      - description: The IRQ of the inside timer timeout.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    ddr_pmu@ff638000 {

No underscores in node names, generic node names, so just "pmu".
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +        compatible = "amlogic,g12a-ddr-pmu";
> +        reg = <0xff638000 0x100
> +               0xff638c00 0x100>;

This is still not fixed. Binding says you have two items, but you
declared here only one item.

Best regards,
Krzysztof
Jiucheng Xu Aug. 5, 2022, 10 a.m. UTC | #2 
On 2022/8/5 16:21, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
>
> On 05/08/2022 09:14, Jiucheng Xu wrote:
>> Add binding documentation for the Amlogic G12 series DDR
>> performance monitor unit.
>>
> Bindings go first in the series, before their implementation.
>
>> Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com>
>> ---
>> Changes v3 -> v4:
>>    - Fix "$id: relative path/filename doesn't match actual path or
>>      filename" warning
>>
>> Changes v2 -> v3:
>>    - Remove oneOf
>>    - Add descriptions
>>    - Fix compiling warning
>>
>> Changes v1 -> v2:
>>    - Rename file, from aml_ddr_pmu.yaml to amlogic,g12_ddr_pmu.yaml
>>    - Delete "model", "dmc_nr", "chann_nr" new properties
>>    - Fix compiling error
>> ---
>>   .../bindings/perf/amlogic,g12-ddr-pmu.yaml    | 52 +++++++++++++++++++
>>   MAINTAINERS                                   |  1 +
>>   2 files changed, 53 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
>> new file mode 100644
>> index 000000000000..4d018574ffd1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/perf/amlogic,g12-ddr-pmu.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic G12 DDR performance monitor
>> +
>> +maintainers:
>> +  - Jiucheng Xu <jiucheng.xu@amlogic.com>
>> +
>> +description: |
>> +  Amlogic G12 series SoC integrate DDR bandwidth monitor.
>> +  A timer is inside and can generate interrupt when timeout.
>> +  The bandwidth is counted in the timer ISR. Different platform
>> +  has different subset of event format attribute.
>> +
>> +properties:
>> +  compatible:
>> +    items:
> You have only one item, so remove "items".
>
>> +      - enum:
>> +          - amlogic,g12b-ddr-pmu
>> +          - amlogic,g12a-ddr-pmu
>> +          - amlogic,sm1-ddr-pmu
>> +
>> +  reg:
>> +    items:
>> +      - description: Physical address of DMC bandwidth register
>> +          and size of the configuration address space.
> Skip obvious pieces, so only:
>
> description: DMC bandwidth register space
>
>> +      - description: Physical address of DMC PLL register and
>> +          size of the configuration address space.
> description: DMC PLL bandwidth register space
>
>
>> +
>> +  interrupts:
>> +    items:
>> +      - description: The IRQ of the inside timer timeout.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    ddr_pmu@ff638000 {
> No underscores in node names, generic node names, so just "pmu".
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
>> +        compatible = "amlogic,g12a-ddr-pmu";
>> +        reg = <0xff638000 0x100
>> +               0xff638c00 0x100>;
> This is still not fixed. Binding says you have two items, but you
> declared here only one item.

Okay, I will change it as your comments.


> Best regards,
> Krzysztof
>
Jiucheng Xu Aug. 15, 2022, 9:04 a.m. UTC | #3 
On 2022/8/5 18:00, Jiucheng Xu wrote:
>
> On 2022/8/5 16:21, Krzysztof Kozlowski wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On 05/08/2022 09:14, Jiucheng Xu wrote:
>>> Add binding documentation for the Amlogic G12 series DDR
>>> performance monitor unit.
>>>
>> Bindings go first in the series, before their implementation.
>>
>>> Signed-off-by: Jiucheng Xu <jiucheng.xu@amlogic.com>
>>> ---
>>> Changes v3 -> v4:
>>>    - Fix "$id: relative path/filename doesn't match actual path or
>>>      filename" warning
>>>
>>> Changes v2 -> v3:
>>>    - Remove oneOf
>>>    - Add descriptions
>>>    - Fix compiling warning
>>>
>>> Changes v1 -> v2:
>>>    - Rename file, from aml_ddr_pmu.yaml to amlogic,g12_ddr_pmu.yaml
>>>    - Delete "model", "dmc_nr", "chann_nr" new properties
>>>    - Fix compiling error
>>> ---
>>>   .../bindings/perf/amlogic,g12-ddr-pmu.yaml    | 52 
>>> +++++++++++++++++++
>>>   MAINTAINERS                                   |  1 +
>>>   2 files changed, 53 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml 
>>> b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
>>> new file mode 100644
>>> index 000000000000..4d018574ffd1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
>>> @@ -0,0 +1,52 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/perf/amlogic,g12-ddr-pmu.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Amlogic G12 DDR performance monitor
>>> +
>>> +maintainers:
>>> +  - Jiucheng Xu <jiucheng.xu@amlogic.com>
>>> +
>>> +description: |
>>> +  Amlogic G12 series SoC integrate DDR bandwidth monitor.
>>> +  A timer is inside and can generate interrupt when timeout.
>>> +  The bandwidth is counted in the timer ISR. Different platform
>>> +  has different subset of event format attribute.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>> You have only one item, so remove "items".

Hi Krzysztof,

I have tried to remove "items", but error comes.

properties:
   compatible:
     - enum:
       - amlogic,g12a-ddr-pmu
       - amlogic,g12b-ddr-pmu
       - amlogic,sm1-ddr-pmu

Do I get misunderstand? I think the "item" is necessary.

>>
>>> +      - enum:
>>> +          - amlogic,g12b-ddr-pmu
>>> +          - amlogic,g12a-ddr-pmu
>>> +          - amlogic,sm1-ddr-pmu
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: Physical address of DMC bandwidth register
>>> +          and size of the configuration address space.
>> Skip obvious pieces, so only:
>>
>> description: DMC bandwidth register space
>>
>>> +      - description: Physical address of DMC PLL register and
>>> +          size of the configuration address space.
>> description: DMC PLL bandwidth register space
>>
>>
>>> +
>>> +  interrupts:
>>> +    items:
>>> +      - description: The IRQ of the inside timer timeout.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +    ddr_pmu@ff638000 {
>> No underscores in node names, generic node names, so just "pmu".
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation 
>>
>>
>>> +        compatible = "amlogic,g12a-ddr-pmu";
>>> +        reg = <0xff638000 0x100
>>> +               0xff638c00 0x100>;
>> This is still not fixed. Binding says you have two items, but you
>> declared here only one item.
>
> Okay, I will change it as your comments.
>
>
>> Best regards,
>> Krzysztof
>>
Krzysztof Kozlowski Aug. 16, 2022, 5:57 a.m. UTC | #4 
On 15/08/2022 12:04, Jiucheng Xu wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>> You have only one item, so remove "items".
> 
> Hi Krzysztof,
> 
> I have tried to remove "items", but error comes.
> 
> properties:
>    compatible:
>      - enum:
>        - amlogic,g12a-ddr-pmu
>        - amlogic,g12b-ddr-pmu
>        - amlogic,sm1-ddr-pmu
> 
> Do I get misunderstand? I think the "item" is necessary.

And how other bindings are doing it?

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
new file mode 100644
index 000000000000..4d018574ffd1
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
@@ -0,0 +1,52 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/amlogic,g12-ddr-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic G12 DDR performance monitor
+
+maintainers:
+  - Jiucheng Xu <jiucheng.xu@amlogic.com>
+
+description: |
+  Amlogic G12 series SoC integrate DDR bandwidth monitor.
+  A timer is inside and can generate interrupt when timeout.
+  The bandwidth is counted in the timer ISR. Different platform
+  has different subset of event format attribute.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - amlogic,g12b-ddr-pmu
+          - amlogic,g12a-ddr-pmu
+          - amlogic,sm1-ddr-pmu
+
+  reg:
+    items:
+      - description: Physical address of DMC bandwidth register
+          and size of the configuration address space.
+      - description: Physical address of DMC PLL register and
+          size of the configuration address space.
+
+  interrupts:
+    items:
+      - description: The IRQ of the inside timer timeout.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    ddr_pmu@ff638000 {
+        compatible = "amlogic,g12a-ddr-pmu";
+        reg = <0xff638000 0x100
+               0xff638c00 0x100>;
+        interrupts = <GIC_SPI 52 IRQ_TYPE_EDGE_RISING>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 8ee68e699e6d..67c2c9e8c4ea 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/meson-ddr-pmu.rst
+F:	Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
 F:	drivers/perf/amlogic/
 F:	include/soc/amlogic/