diff mbox series

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

Message ID 20230504060729.689579-3-claudiu.beznea@microchip.com (mailing list archive)
State Changes Requested, archived
Headers show
Series dt-bindings: clocks: at91: convert to yaml | expand

Commit Message

Claudiu Beznea May 4, 2023, 6:07 a.m. UTC 
Convert Atmel PMC documentation to yaml.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 .../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

Comments

Krzysztof Kozlowski May 4, 2023, 12:47 p.m. UTC | #1 
On 04/05/2023 08:07, Claudiu Beznea wrote:
> Convert Atmel PMC documentation to yaml.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

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 <claudiu.beznea@microchip.com>
> +
> +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
Claudiu Beznea May 5, 2023, 12:46 p.m. UTC | #2 
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 <claudiu.beznea@microchip.com>
> 
> 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 <claudiu.beznea@microchip.com>
>> +
>> +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
>
Krzysztof Kozlowski May 5, 2023, 2:07 p.m. UTC | #3 
On 05/05/2023 14:46, Claudiu.Beznea@microchip.com 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 <claudiu.beznea@microchip.com>
>>
>> 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 <claudiu.beznea@microchip.com>
>>> +
>>> +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
diff mbox series

Patch

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 <claudiu.beznea@microchip.com>
+
+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";
+    };
+
+...