2023-05-04 06:08:23

by Claudiu Beznea

[permalink] [raw]
Subject: [PATCH 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml

Convert Atmel PMC documentation to yaml.

Signed-off-by: Claudiu Beznea <[email protected]>
---
.../devicetree/bindings/clock/at91-clock.txt | 28 ---
.../bindings/clock/atmel,at91rm9200-pmc.yaml | 161 ++++++++++++++++++
2 files changed, 161 insertions(+), 28 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml

diff --git a/Documentation/devicetree/bindings/clock/at91-clock.txt b/Documentation/devicetree/bindings/clock/at91-clock.txt
index 13f45db3b66d..57394785d3b0 100644
--- a/Documentation/devicetree/bindings/clock/at91-clock.txt
+++ b/Documentation/devicetree/bindings/clock/at91-clock.txt
@@ -28,31 +28,3 @@ For example:
#clock-cells = <0>;
};

-Power Management Controller (PMC):
-
-Required properties:
-- compatible : shall be "atmel,<chip>-pmc", "syscon" or
- "microchip,sam9x60-pmc"
- <chip> can be: at91rm9200, at91sam9260, at91sam9261,
- at91sam9263, at91sam9g45, at91sam9n12, at91sam9rl, at91sam9g15,
- at91sam9g25, at91sam9g35, at91sam9x25, at91sam9x35, at91sam9x5,
- sama5d2, sama5d3 or sama5d4.
-- #clock-cells : from common clock binding; shall be set to 2. The first entry
- is the type of the clock (core, system, peripheral or generated) and the
- second entry its index as provided by the datasheet
-- clocks : Must contain an entry for each entry in clock-names.
-- clock-names: Must include the following entries: "slow_clk", "main_xtal"
-
-Optional properties:
-- atmel,osc-bypass : boolean property. Set this when a clock signal is directly
- provided on XIN.
-
-For example:
- pmc: pmc@f0018000 {
- compatible = "atmel,sama5d4-pmc", "syscon";
- reg = <0xf0018000 0x120>;
- interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
- #clock-cells = <2>;
- clocks = <&clk32k>, <&main_xtal>;
- clock-names = "slow_clk", "main_xtal";
- };
diff --git a/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
new file mode 100644
index 000000000000..c4023c3a85f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
@@ -0,0 +1,161 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/clock/atmel,at91rm9200-pmc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Atmel Power Management Controller (PMC)
+
+maintainers:
+ - Claudiu Beznea <[email protected]>
+
+description:
+ The power management controller optimizes power consumption by controlling all
+ system and user peripheral clocks. The PMC enables/disables the clock inputs
+ to many of the peripherals and to the processor.
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - const: atmel,at91sam9260-pmc
+ - const: syscon
+ - items:
+ - enum:
+ - atmel,at91sam9g20-pmc
+ - enum:
+ - atmel,at91sam9260-pmc
+ - const: syscon
+ - items:
+ - enum:
+ - atmel,at91sam9g15-pmc
+ - atmel,at91sam9g25-pmc
+ - atmel,at91sam9g35-pmc
+ - atmel,at91sam9x25-pmc
+ - atmel,at91sam9x35-pmc
+ - enum:
+ - atmel,at91sam9x5-pmc
+ - const: syscon
+ - items:
+ - enum:
+ - atmel,at91sam9g45-pmc
+ - atmel,at91sam9n12-pmc
+ - atmel,at91sam9rl-pmc
+ - atmel,at91rm9200-pmc
+ - atmel,sama5d4-pmc
+ - atmel,sama5d3-pmc
+ - atmel,sama5d2-pmc
+ - microchip,sam9x60-pmc
+ - microchip,sama7g5-pmc
+ - const: syscon
+
+ reg:
+ maxItems: 1
+
+ "#clock-cells":
+ const: 2
+
+ clocks:
+ minItems: 2
+ maxItems: 3
+
+ clock-names:
+ minItems: 2
+ maxItems: 3
+
+ interrupts:
+ maxItems: 1
+
+ atmel,osc-bypass:
+ type: boolean
+ description: set when a clock signal is directly provided on XIN
+
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - "#clock-cells"
+ - clocks
+ - clock-names
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - microchip,sam9x60-pmc
+ - microchip,sama7g5-pmc
+ then:
+ properties:
+ clocks:
+ minItems: 3
+ maxItems: 3
+ clock-names:
+ items:
+ - const: td_slck
+ - const: md_slck
+ - const: main_xtal
+ required:
+ - clock-names
+ - clocks
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - atmel,at91rm9200-pmc
+ - atmel,at91sam9260-pmc
+ - atmel,at91sam9g20-pmc
+ then:
+ properties:
+ clocks:
+ minItems: 2
+ maxItems: 2
+ clock-names:
+ items:
+ - const: slow_xtal
+ - const: main_xtal
+ required:
+ - clock-names
+ - clocks
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - atmel,sama5d4-pmc
+ - atmel,sama5d3-pmc
+ - atmel,sama5d2-pmc
+ then:
+ properties:
+ clocks:
+ minItems: 2
+ maxItems: 2
+ clock-names:
+ items:
+ - const: slow_clk
+ - const: main_xtal
+ required:
+ - clock-names
+ - clocks
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ pmc: clock-controller@f0018000 {
+ compatible = "atmel,sama5d4-pmc", "syscon";
+ reg = <0xf0018000 0x120>;
+ interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+ #clock-cells = <2>;
+ clocks = <&clk32k>, <&main_xtal>;
+ clock-names = "slow_clk", "main_xtal";
+ };
+
+...
--
2.34.1


2023-05-04 12:50:16

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml

On 04/05/2023 08:07, Claudiu Beznea wrote:
> Convert Atmel PMC documentation to yaml.
>
> Signed-off-by: Claudiu Beznea <[email protected]>

Thank you for your patch. There is something to discuss/improve.


> diff --git a/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
> new file mode 100644
> index 000000000000..c4023c3a85f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
> @@ -0,0 +1,161 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/clock/atmel,at91rm9200-pmc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes from both.

> +
> +title: Atmel Power Management Controller (PMC)
> +
> +maintainers:
> + - Claudiu Beznea <[email protected]>
> +
> +description:
> + The power management controller optimizes power consumption by controlling all
> + system and user peripheral clocks. The PMC enables/disables the clock inputs
> + to many of the peripherals and to the processor.
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - const: atmel,at91sam9260-pmc

Why this is separate, not part of bottom enum?

> + - const: syscon

> + - items:
> + - enum:
> + - atmel,at91sam9g20-pmc
> + - enum:
> + - atmel,at91sam9260-pmc

This should be const. You cannot have here different compatibles.

> + - const: syscon
> + - items:
> + - enum:
> + - atmel,at91sam9g15-pmc
> + - atmel,at91sam9g25-pmc
> + - atmel,at91sam9g35-pmc
> + - atmel,at91sam9x25-pmc
> + - atmel,at91sam9x35-pmc
> + - enum:
> + - atmel,at91sam9x5-pmc
> + - const: syscon
> + - items:
> + - enum:
> + - atmel,at91sam9g45-pmc
> + - atmel,at91sam9n12-pmc
> + - atmel,at91sam9rl-pmc
> + - atmel,at91rm9200-pmc

Order by name?

> + - atmel,sama5d4-pmc
> + - atmel,sama5d3-pmc
> + - atmel,sama5d2-pmc
> + - microchip,sam9x60-pmc
> + - microchip,sama7g5-pmc
> + - const: syscon
> +
> + reg:
> + maxItems: 1
> +
> + "#clock-cells":
> + const: 2

Explain what the cells are for in description. Having '2' for clock
controller is not obvious.

> +
> + clocks:
> + minItems: 2
> + maxItems: 3
> +
> + clock-names:
> + minItems: 2
> + maxItems: 3
> +
> + interrupts:
> + maxItems: 1
> +
> + atmel,osc-bypass:
> + type: boolean
> + description: set when a clock signal is directly provided on XIN
> +
> +

Just one blank line.

> +required:
> + - compatible
> + - reg
> + - interrupts
> + - "#clock-cells"
> + - clocks
> + - clock-names

Keep the same order here as they appear in properties:.


> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - microchip,sam9x60-pmc
> + - microchip,sama7g5-pmc
> + then:
> + properties:
> + clocks:
> + minItems: 3
> + maxItems: 3
> + clock-names:
> + items:
> + - const: td_slck
> + - const: md_slck
> + - const: main_xtal
> + required:
> + - clock-names
> + - clocks

Drop required: here. It's already in top-level. Same in places below.

> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - atmel,at91rm9200-pmc
> + - atmel,at91sam9260-pmc
> + - atmel,at91sam9g20-pmc
> + then:
> + properties:
> + clocks:
> + minItems: 2
> + maxItems: 2
> + clock-names:
> + items:
> + - const: slow_xtal
> + - const: main_xtal
> + required:
> + - clock-names
> + - clocks
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - atmel,sama5d4-pmc
> + - atmel,sama5d3-pmc
> + - atmel,sama5d2-pmc
> + then:
> + properties:
> + clocks:
> + minItems: 2
> + maxItems: 2
> + clock-names:
> + items:
> + - const: slow_clk
> + - const: main_xtal
> + required:
> + - clock-names
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + pmc: clock-controller@f0018000 {
> + compatible = "atmel,sama5d4-pmc", "syscon";
> + reg = <0xf0018000 0x120>;
> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;

interrupt looks a bit odd. Are you sure it is correct?

> + #clock-cells = <2>;
> + clocks = <&clk32k>, <&main_xtal>;
> + clock-names = "slow_clk", "main_xtal";
> + };
> +
> +...

Best regards,
Krzysztof

2023-05-05 13:09:10

by Claudiu Beznea

[permalink] [raw]
Subject: Re: [PATCH 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml

On 04.05.2023 15:47, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 04/05/2023 08:07, Claudiu Beznea wrote:
>> Convert Atmel PMC documentation to yaml.
>>
>> Signed-off-by: Claudiu Beznea <[email protected]>
>
> Thank you for your patch. There is something to discuss/improve.
>
>
>> diff --git a/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
>> new file mode 100644
>> index 000000000000..c4023c3a85f2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
>> @@ -0,0 +1,161 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/clock/atmel,at91rm9200-pmc.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>
> Drop quotes from both.
>
>> +
>> +title: Atmel Power Management Controller (PMC)
>> +
>> +maintainers:
>> + - Claudiu Beznea <[email protected]>
>> +
>> +description:
>> + The power management controller optimizes power consumption by controlling all
>> + system and user peripheral clocks. The PMC enables/disables the clock inputs
>> + to many of the peripherals and to the processor.
>> +
>> +properties:
>> + compatible:
>> + oneOf:
>> + - items:
>> + - const: atmel,at91sam9260-pmc
>
> Why this is separate, not part of bottom enum?

Current device trees uses the following compatibles (among others):
- "atmel,at91sam9260-pmc, "syscon" or
- "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc, "syscon"

I haven't found another way to make dtbs_check happy.
Is there another way for this?

>
>> + - const: syscon
>
>> + - items:
>> + - enum:
>> + - atmel,at91sam9g20-pmc
>> + - enum:
>> + - atmel,at91sam9260-pmc
>
> This should be const. You cannot have here different compatibles.
>
>> + - const: syscon
>> + - items:
>> + - enum:
>> + - atmel,at91sam9g15-pmc
>> + - atmel,at91sam9g25-pmc
>> + - atmel,at91sam9g35-pmc
>> + - atmel,at91sam9x25-pmc
>> + - atmel,at91sam9x35-pmc
>> + - enum:
>> + - atmel,at91sam9x5-pmc
>> + - const: syscon
>> + - items:
>> + - enum:
>> + - atmel,at91sam9g45-pmc
>> + - atmel,at91sam9n12-pmc
>> + - atmel,at91sam9rl-pmc
>> + - atmel,at91rm9200-pmc
>
> Order by name?
>
>> + - atmel,sama5d4-pmc
>> + - atmel,sama5d3-pmc
>> + - atmel,sama5d2-pmc
>> + - microchip,sam9x60-pmc
>> + - microchip,sama7g5-pmc
>> + - const: syscon
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + "#clock-cells":
>> + const: 2
>
> Explain what the cells are for in description. Having '2' for clock
> controller is not obvious.
>
>> +
>> + clocks:
>> + minItems: 2
>> + maxItems: 3
>> +
>> + clock-names:
>> + minItems: 2
>> + maxItems: 3
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + atmel,osc-bypass:
>> + type: boolean
>> + description: set when a clock signal is directly provided on XIN
>> +
>> +
>
> Just one blank line.
>
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - "#clock-cells"
>> + - clocks
>> + - clock-names
>
> Keep the same order here as they appear in properties:.
>
>
>> +
>> +allOf:
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - microchip,sam9x60-pmc
>> + - microchip,sama7g5-pmc
>> + then:
>> + properties:
>> + clocks:
>> + minItems: 3
>> + maxItems: 3
>> + clock-names:
>> + items:
>> + - const: td_slck
>> + - const: md_slck
>> + - const: main_xtal
>> + required:
>> + - clock-names
>> + - clocks
>
> Drop required: here. It's already in top-level. Same in places below.
>
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - atmel,at91rm9200-pmc
>> + - atmel,at91sam9260-pmc
>> + - atmel,at91sam9g20-pmc
>> + then:
>> + properties:
>> + clocks:
>> + minItems: 2
>> + maxItems: 2
>> + clock-names:
>> + items:
>> + - const: slow_xtal
>> + - const: main_xtal
>> + required:
>> + - clock-names
>> + - clocks
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - atmel,sama5d4-pmc
>> + - atmel,sama5d3-pmc
>> + - atmel,sama5d2-pmc
>> + then:
>> + properties:
>> + clocks:
>> + minItems: 2
>> + maxItems: 2
>> + clock-names:
>> + items:
>> + - const: slow_clk
>> + - const: main_xtal
>> + required:
>> + - clock-names
>> + - clocks
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> + pmc: clock-controller@f0018000 {
>> + compatible = "atmel,sama5d4-pmc", "syscon";
>> + reg = <0xf0018000 0x120>;
>> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
>
> interrupt looks a bit odd. Are you sure it is correct?

This example is from SAMA5D4 SoC which uses a vendor specific interrupt
controller (Atmel AIC) where:
- 1st cell is the interrupt number
- 2nd cell is the interrupt type (level/edge sensitive)
- 3rd cell is the IRQ priority

Thank you,
Claudiu

>
>> + #clock-cells = <2>;
>> + clocks = <&clk32k>, <&main_xtal>;
>> + clock-names = "slow_clk", "main_xtal";
>> + };
>> +
>> +...
>
> Best regards,
> Krzysztof
>

2023-05-05 14:15:52

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml

On 05/05/2023 14:46, [email protected] wrote:
> On 04.05.2023 15:47, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 04/05/2023 08:07, Claudiu Beznea wrote:
>>> Convert Atmel PMC documentation to yaml.
>>>
>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>
>>> diff --git a/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
>>> new file mode 100644
>>> index 000000000000..c4023c3a85f2
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
>>> @@ -0,0 +1,161 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: "http://devicetree.org/schemas/clock/atmel,at91rm9200-pmc.yaml#"
>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>
>> Drop quotes from both.
>>
>>> +
>>> +title: Atmel Power Management Controller (PMC)
>>> +
>>> +maintainers:
>>> + - Claudiu Beznea <[email protected]>
>>> +
>>> +description:
>>> + The power management controller optimizes power consumption by controlling all
>>> + system and user peripheral clocks. The PMC enables/disables the clock inputs
>>> + to many of the peripherals and to the processor.
>>> +
>>> +properties:
>>> + compatible:
>>> + oneOf:
>>> + - items:
>>> + - const: atmel,at91sam9260-pmc
>>
>> Why this is separate, not part of bottom enum?
>
> Current device trees uses the following compatibles (among others):
> - "atmel,at91sam9260-pmc, "syscon" or
> - "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc, "syscon"
>
> I haven't found another way to make dtbs_check happy.
> Is there another way for this?

I mean the enum at the bottom of all compatibles.

>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> +
>>> + pmc: clock-controller@f0018000 {
>>> + compatible = "atmel,sama5d4-pmc", "syscon";
>>> + reg = <0xf0018000 0x120>;
>>> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
>>
>> interrupt looks a bit odd. Are you sure it is correct?
>
> This example is from SAMA5D4 SoC which uses a vendor specific interrupt
> controller (Atmel AIC) where:
> - 1st cell is the interrupt number
> - 2nd cell is the interrupt type (level/edge sensitive)
> - 3rd cell is the IRQ priority

ok

Best regards,
Krzysztof