[PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Johan Jonker]

Current dts files with RK3188/RK3066 'cru' nodes are manually verified.
In order to automate this process rockchip,rk3188-cru.txt has to be
converted to YAML.

Changed:
  Add properties to fix notifications by clocks.yaml for example:
    clocks
    assigned-clock-rates
    assigned-clocks

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 .../bindings/clock/rockchip,rk3188-cru.txt    | 61 --------------
 .../bindings/clock/rockchip,rk3188-cru.yaml   | 81 +++++++++++++++++++
 2 files changed, 81 insertions(+), 61 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
 create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml

diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
deleted file mode 100644
index 7f368530a..000000000
--- a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
+++ /dev/null
@@ -1,61 +0,0 @@
-* Rockchip RK3188/RK3066 Clock and Reset Unit
-
-The RK3188/RK3066 clock controller generates and supplies clock to various
-controllers within the SoC and also implements a reset controller for SoC
-peripherals.
-
-Required Properties:
-
-- compatible: should be "rockchip,rk3188-cru", "rockchip,rk3188a-cru" or
-			"rockchip,rk3066a-cru"
-- reg: physical base address of the controller and length of memory mapped
-  region.
-- #clock-cells: should be 1.
-- #reset-cells: should be 1.
-
-Optional Properties:
-
-- rockchip,grf: phandle to the syscon managing the "general register files"
-  If missing pll rates are not changeable, due to the missing pll lock status.
-
-Each clock is assigned an identifier and client nodes can use this identifier
-to specify the clock which they consume. All available clocks are defined as
-preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
-dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
-Similar macros exist for the reset sources in these files.
-
-External clocks:
-
-There are several clocks that are generated outside the SoC. It is expected
-that they are defined using standard clock bindings with following
-clock-output-names:
- - "xin24m" - crystal input - required,
- - "xin32k" - rtc clock - optional,
- - "xin27m" - 27mhz crystal input on rk3066 - optional,
- - "ext_hsadc" - external HSADC clock - optional,
- - "ext_cif0" - external camera clock - optional,
- - "ext_rmii" - external RMII clock - optional,
- - "ext_jtag" - externalJTAG clock - optional
-
-Example: Clock controller node:
-
-	cru: cru@20000000 {
-		compatible = "rockchip,rk3188-cru";
-		reg = <0x20000000 0x1000>;
-		rockchip,grf = <&grf>;
-
-		#clock-cells = <1>;
-		#reset-cells = <1>;
-	};
-
-Example: UART controller node that consumes the clock generated by the clock
-  controller:
-
-	uart0: serial@10124000 {
-		compatible = "snps,dw-apb-uart";
-		reg = <0x10124000 0x400>;
-		interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
-		reg-shift = <2>;
-		reg-io-width = <1>;
-		clocks = <&cru SCLK_UART0>;
-	};
diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
new file mode 100644
index 000000000..136a9771e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/rockchip,rk3188-cru.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip RK3188/RK3066 Clock and Reset Unit (CRU)
+
+maintainers:
+  - Heiko Stuebner <heiko@sntech.de>
+
+description: |
+  The RK3188/RK3066 clock controller generates and supplies clocks to various
+  controllers within the SoC and also implements a reset controller for SoC
+  peripherals.
+  Each clock is assigned an identifier and client nodes can use this identifier
+  to specify the clock which they consume. All available clocks are defined as
+  preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
+  dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
+  Similar macros exist for the reset sources in these files.
+  There are several clocks that are generated outside the SoC. It is expected
+  that they are defined using standard clock bindings with following
+  clock-output-names:
+    - "xin24m"    - crystal input                 - required
+    - "xin32k"    - RTC clock                     - optional
+    - "xin27m"    - 27mhz crystal input on RK3066 - optional
+    - "ext_hsadc" - external HSADC clock          - optional
+    - "ext_cif0"  - external camera clock         - optional
+    - "ext_rmii"  - external RMII clock           - optional
+    - "ext_jtag"  - external JTAG clock           - optional
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3066a-cru
+      - rockchip,rk3188-cru
+      - rockchip,rk3188a-cru
+
+  reg:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 1
+
+  "#reset-cells":
+    const: 1
+
+  clocks:
+    minItems: 1
+
+  assigned-clock-rates:
+    minItems: 1
+    maxItems: 64
+
+  assigned-clocks:
+    minItems: 1
+    maxItems: 64
+
+  rockchip,grf:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon managing the "general register files" (GRF),
+      if missing pll rates are not changeable, due to the missing pll lock status.
+
+required:
+  - compatible
+  - reg
+  - "#clock-cells"
+  - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    cru: cru@20000000 {
+      compatible = "rockchip,rk3188-cru";
+      reg = <0x20000000 0x1000>;
+      rockchip,grf = <&grf>;
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+    };
-- 
2.20.1

Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Krzysztof Kozlowski]

On 24/03/2022 14:32, Johan Jonker wrote:
> Current dts files with RK3188/RK3066 'cru' nodes are manually verified.
> In order to automate this process rockchip,rk3188-cru.txt has to be
> converted to YAML.
> 
> Changed:
>   Add properties to fix notifications by clocks.yaml for example:
>     clocks
>     assigned-clock-rates
>     assigned-clocks
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  .../bindings/clock/rockchip,rk3188-cru.txt    | 61 --------------
>  .../bindings/clock/rockchip,rk3188-cru.yaml   | 81 +++++++++++++++++++
>  2 files changed, 81 insertions(+), 61 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
>  create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
> deleted file mode 100644
> index 7f368530a..000000000
> --- a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
> +++ /dev/null
> @@ -1,61 +0,0 @@
> -* Rockchip RK3188/RK3066 Clock and Reset Unit
> -
> -The RK3188/RK3066 clock controller generates and supplies clock to various
> -controllers within the SoC and also implements a reset controller for SoC
> -peripherals.
> -
> -Required Properties:
> -
> -- compatible: should be "rockchip,rk3188-cru", "rockchip,rk3188a-cru" or
> -			"rockchip,rk3066a-cru"
> -- reg: physical base address of the controller and length of memory mapped
> -  region.
> -- #clock-cells: should be 1.
> -- #reset-cells: should be 1.
> -
> -Optional Properties:
> -
> -- rockchip,grf: phandle to the syscon managing the "general register files"
> -  If missing pll rates are not changeable, due to the missing pll lock status.
> -
> -Each clock is assigned an identifier and client nodes can use this identifier
> -to specify the clock which they consume. All available clocks are defined as
> -preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
> -dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
> -Similar macros exist for the reset sources in these files.
> -
> -External clocks:
> -
> -There are several clocks that are generated outside the SoC. It is expected
> -that they are defined using standard clock bindings with following
> -clock-output-names:
> - - "xin24m" - crystal input - required,
> - - "xin32k" - rtc clock - optional,
> - - "xin27m" - 27mhz crystal input on rk3066 - optional,
> - - "ext_hsadc" - external HSADC clock - optional,
> - - "ext_cif0" - external camera clock - optional,
> - - "ext_rmii" - external RMII clock - optional,
> - - "ext_jtag" - externalJTAG clock - optional
> -
> -Example: Clock controller node:
> -
> -	cru: cru@20000000 {
> -		compatible = "rockchip,rk3188-cru";
> -		reg = <0x20000000 0x1000>;
> -		rockchip,grf = <&grf>;
> -
> -		#clock-cells = <1>;
> -		#reset-cells = <1>;
> -	};
> -
> -Example: UART controller node that consumes the clock generated by the clock
> -  controller:
> -
> -	uart0: serial@10124000 {
> -		compatible = "snps,dw-apb-uart";
> -		reg = <0x10124000 0x400>;
> -		interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
> -		reg-shift = <2>;
> -		reg-io-width = <1>;
> -		clocks = <&cru SCLK_UART0>;
> -	};
> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
> new file mode 100644
> index 000000000..136a9771e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/rockchip,rk3188-cru.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip RK3188/RK3066 Clock and Reset Unit (CRU)
> +
> +maintainers:
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +description: |
> +  The RK3188/RK3066 clock controller generates and supplies clocks to various
> +  controllers within the SoC and also implements a reset controller for SoC
> +  peripherals.
> +  Each clock is assigned an identifier and client nodes can use this identifier
> +  to specify the clock which they consume. All available clocks are defined as
> +  preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
> +  dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
> +  Similar macros exist for the reset sources in these files.
> +  There are several clocks that are generated outside the SoC. It is expected
> +  that they are defined using standard clock bindings with following
> +  clock-output-names:
> +    - "xin24m"    - crystal input                 - required
> +    - "xin32k"    - RTC clock                     - optional
> +    - "xin27m"    - 27mhz crystal input on RK3066 - optional
> +    - "ext_hsadc" - external HSADC clock          - optional
> +    - "ext_cif0"  - external camera clock         - optional
> +    - "ext_rmii"  - external RMII clock           - optional
> +    - "ext_jtag"  - external JTAG clock           - optional
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3066a-cru
> +      - rockchip,rk3188-cru
> +      - rockchip,rk3188a-cru
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  "#reset-cells":
> +    const: 1
> +
> +  clocks:
> +    minItems: 1

You need maxItems. Would be also nice to describe what should be here as
input


> +
> +  assigned-clock-rates:
> +    minItems: 1
> +    maxItems: 64
> +
> +  assigned-clocks:
> +    minItems: 1
> +    maxItems: 64

Both assigned-xxx should not be necessary. Did you try validate the dtbs
without these?

I guess you added "clocks" above because of these, so you need to
correct the DTS because asigned-xxx depend on clocks property.

> +
> +  rockchip,grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the syscon managing the "general register files" (GRF),
> +      if missing pll rates are not changeable, due to the missing pll lock status.
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#clock-cells"
> +  - "#reset-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    cru: cru@20000000 {

Node name: clock-controller

> +      compatible = "rockchip,rk3188-cru";
> +      reg = <0x20000000 0x1000>;
> +      rockchip,grf = <&grf>;
> +      #clock-cells = <1>;
> +      #reset-cells = <1>;
> +    };


Best regards,
Krzysztof

Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Johan Jonker]

Hi Heiko, Krzysztof,

Question for the Rockchip clock maintainer:
What clock should be used here and other SoCs with several clock parents
in the tree?

The clock.yaml produces a lot off notifications like:

/arch/arm/boot/dts/rk3036-evb.dtb: clock-controller@20000000: 'clocks'
is a dependency of 'assigned-clocks'

Same for power-controller:

/arch/arm/boot/dts/rk3288-evb-rk808.dtb: power-controller: 'clocks' is a
dependency of 'assigned-clocks'

Just followed rk3399.yaml that got approved recent.
The current CRU nodes are in use for a long time this way.
Converting a simple .txt comes with a lot of extra work each time. :\
Please advise.

Johan

===
dependencies:
  assigned-clocks: [clocks]

ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make dtbs_check
DT_SCHEMA_FILES=~/.local/lib/python3.7/site-packages/dtschema/schemas/clock/clock.yaml

===

See /Documentation/devicetree/bindings/clock/ grep for "assigned":

From cirrus,lochnagar.yaml:

  clocks: true
  assigned-clocks: true
  assigned-clock-parents: true

Is this a possible solution?
So we we don't have to be specific.


On 3/24/22 15:28, Krzysztof Kozlowski wrote:
> On 24/03/2022 14:32, Johan Jonker wrote:
>> Current dts files with RK3188/RK3066 'cru' nodes are manually verified.
>> In order to automate this process rockchip,rk3188-cru.txt has to be
>> converted to YAML.
>>
>> Changed:
>>   Add properties to fix notifications by clocks.yaml for example:
>>     clocks
>>     assigned-clock-rates
>>     assigned-clocks
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  .../bindings/clock/rockchip,rk3188-cru.txt    | 61 --------------
>>  .../bindings/clock/rockchip,rk3188-cru.yaml   | 81 +++++++++++++++++++
>>  2 files changed, 81 insertions(+), 61 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
>>  create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
>> deleted file mode 100644
>> index 7f368530a..000000000
>> --- a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
>> +++ /dev/null
>> @@ -1,61 +0,0 @@
>> -* Rockchip RK3188/RK3066 Clock and Reset Unit
>> -
>> -The RK3188/RK3066 clock controller generates and supplies clock to various
>> -controllers within the SoC and also implements a reset controller for SoC
>> -peripherals.
>> -
>> -Required Properties:
>> -
>> -- compatible: should be "rockchip,rk3188-cru", "rockchip,rk3188a-cru" or
>> -			"rockchip,rk3066a-cru"
>> -- reg: physical base address of the controller and length of memory mapped
>> -  region.
>> -- #clock-cells: should be 1.
>> -- #reset-cells: should be 1.
>> -
>> -Optional Properties:
>> -
>> -- rockchip,grf: phandle to the syscon managing the "general register files"
>> -  If missing pll rates are not changeable, due to the missing pll lock status.
>> -
>> -Each clock is assigned an identifier and client nodes can use this identifier
>> -to specify the clock which they consume. All available clocks are defined as
>> -preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
>> -dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
>> -Similar macros exist for the reset sources in these files.
>> -
>> -External clocks:
>> -
>> -There are several clocks that are generated outside the SoC. It is expected
>> -that they are defined using standard clock bindings with following
>> -clock-output-names:
>> - - "xin24m" - crystal input - required,
>> - - "xin32k" - rtc clock - optional,
>> - - "xin27m" - 27mhz crystal input on rk3066 - optional,
>> - - "ext_hsadc" - external HSADC clock - optional,
>> - - "ext_cif0" - external camera clock - optional,
>> - - "ext_rmii" - external RMII clock - optional,
>> - - "ext_jtag" - externalJTAG clock - optional
>> -
>> -Example: Clock controller node:
>> -
>> -	cru: cru@20000000 {
>> -		compatible = "rockchip,rk3188-cru";
>> -		reg = <0x20000000 0x1000>;
>> -		rockchip,grf = <&grf>;
>> -
>> -		#clock-cells = <1>;
>> -		#reset-cells = <1>;
>> -	};
>> -
>> -Example: UART controller node that consumes the clock generated by the clock
>> -  controller:
>> -
>> -	uart0: serial@10124000 {
>> -		compatible = "snps,dw-apb-uart";
>> -		reg = <0x10124000 0x400>;
>> -		interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
>> -		reg-shift = <2>;
>> -		reg-io-width = <1>;
>> -		clocks = <&cru SCLK_UART0>;
>> -	};
>> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
>> new file mode 100644
>> index 000000000..136a9771e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
>> @@ -0,0 +1,81 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/rockchip,rk3188-cru.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Rockchip RK3188/RK3066 Clock and Reset Unit (CRU)
>> +
>> +maintainers:
>> +  - Heiko Stuebner <heiko@sntech.de>
>> +
>> +description: |
>> +  The RK3188/RK3066 clock controller generates and supplies clocks to various
>> +  controllers within the SoC and also implements a reset controller for SoC
>> +  peripherals.
>> +  Each clock is assigned an identifier and client nodes can use this identifier
>> +  to specify the clock which they consume. All available clocks are defined as
>> +  preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
>> +  dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
>> +  Similar macros exist for the reset sources in these files.
>> +  There are several clocks that are generated outside the SoC. It is expected
>> +  that they are defined using standard clock bindings with following
>> +  clock-output-names:
>> +    - "xin24m"    - crystal input                 - required
>> +    - "xin32k"    - RTC clock                     - optional
>> +    - "xin27m"    - 27mhz crystal input on RK3066 - optional
>> +    - "ext_hsadc" - external HSADC clock          - optional
>> +    - "ext_cif0"  - external camera clock         - optional
>> +    - "ext_rmii"  - external RMII clock           - optional
>> +    - "ext_jtag"  - external JTAG clock           - optional
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - rockchip,rk3066a-cru
>> +      - rockchip,rk3188-cru
>> +      - rockchip,rk3188a-cru
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#clock-cells":
>> +    const: 1
>> +
>> +  "#reset-cells":
>> +    const: 1
>> +
>> +  clocks:
>> +    minItems: 1
> 

> You need maxItems. Would be also nice to describe what should be here as
> input

Just followed rockchip,rk3399-cru.yaml that got rob+dt's ack.

Is this dependency only for simple clock consumers suitable for complex
clock trees with pll's, mux, etc? Shouldn't that be relaxed a bit?

> 
> 
>> +
>> +  assigned-clock-rates:
>> +    minItems: 1
>> +    maxItems: 64
>> +
>> +  assigned-clocks:
>> +    minItems: 1
>> +    maxItems: 64
> 
> Both assigned-xxx should not be necessary. Did you try validate the dtbs
> without these?

That's how it's currently done for Rockchip cru's.


ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml

> 
> I guess you added "clocks" above because of these, so you need to
> correct the DTS because asigned-xxx depend on clocks property.

See comment above.

> 
>> +
>> +  rockchip,grf:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to the syscon managing the "general register files" (GRF),
>> +      if missing pll rates are not changeable, due to the missing pll lock status.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#clock-cells"
>> +  - "#reset-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    cru: cru@20000000 {
> 
> Node name: clock-controller

Which schema?

> 
>> +      compatible = "rockchip,rk3188-cru";
>> +      reg = <0x20000000 0x1000>;
>> +      rockchip,grf = <&grf>;
>> +      #clock-cells = <1>;
>> +      #reset-cells = <1>;
>> +    };
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Stephen Boyd]

Quoting Johan Jonker (2022-03-24 12:51:36)
> Hi Heiko, Krzysztof,
> 
> Question for the Rockchip clock maintainer:
> What clock should be used here and other SoCs with several clock parents
> in the tree?
> 
> The clock.yaml produces a lot off notifications like:
> 
> /arch/arm/boot/dts/rk3036-evb.dtb: clock-controller@20000000: 'clocks'
> is a dependency of 'assigned-clocks'

'clocks' is not a dependency of 'assigned-clocks'. The dt-schema should
be fixed to remove that requirement.

Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Krzysztof Kozlowski]

On 25/03/2022 01:51, Stephen Boyd wrote:
> Quoting Johan Jonker (2022-03-24 12:51:36)
>> Hi Heiko, Krzysztof,
>>
>> Question for the Rockchip clock maintainer:
>> What clock should be used here and other SoCs with several clock parents
>> in the tree?
>>
>> The clock.yaml produces a lot off notifications like:
>>
>> /arch/arm/boot/dts/rk3036-evb.dtb: clock-controller@20000000: 'clocks'
>> is a dependency of 'assigned-clocks'
> 
> 'clocks' is not a dependency of 'assigned-clocks'. The dt-schema should
> be fixed to remove that requirement.

If the driver does not have any clock inputs ("clocks" property), why
does it care about some clock frequencies and parents?

The clocks is the logical dependency of assigned-clocks, because
otherwise hardware description is not complete.

What should be here for Rockhip? We had similar cases like this for many
drivers, I was fixing some of Exynos as well. In my case usually the
root/external clock was missing, so I supplied is as input clock to the
clock controller.


Best regards,
Krzysztof

Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Krzysztof Kozlowski]

On 24/03/2022 20:51, Johan Jonker wrote:
> Hi Heiko, Krzysztof,
> 
> Question for the Rockchip clock maintainer:
> What clock should be used here and other SoCs with several clock parents
> in the tree?
> 
> The clock.yaml produces a lot off notifications like:
> 
> /arch/arm/boot/dts/rk3036-evb.dtb: clock-controller@20000000: 'clocks'
> is a dependency of 'assigned-clocks'
> 
> Same for power-controller:
> 
> /arch/arm/boot/dts/rk3288-evb-rk808.dtb: power-controller: 'clocks' is a
> dependency of 'assigned-clocks'
> 
> Just followed rk3399.yaml that got approved recent.
> The current CRU nodes are in use for a long time this way.
> Converting a simple .txt comes with a lot of extra work each time. :\
> Please advise.

Same for me, same for me...

I replied to this part to Stephen's email. Here I'll reply to other parts:

(...)

>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - rockchip,rk3066a-cru
>>> +      - rockchip,rk3188-cru
>>> +      - rockchip,rk3188a-cru
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  "#clock-cells":
>>> +    const: 1
>>> +
>>> +  "#reset-cells":
>>> +    const: 1
>>> +
>>> +  clocks:
>>> +    minItems: 1
>>
> 
>> You need maxItems. Would be also nice to describe what should be here as
>> input
> 
> Just followed rockchip,rk3399-cru.yaml that got rob+dt's ack.
> 
> Is this dependency only for simple clock consumers suitable for complex
> clock trees with pll's, mux, etc? Shouldn't that be relaxed a bit?

minItems means that your clock controller can take any number of other
clock inputs. This is not true, for sure. Your device probably has fixed
number of external clocks (usually provided on the board if this is main
SoC clock controller).

> 
>>
>>
>>> +
>>> +  assigned-clock-rates:
>>> +    minItems: 1
>>> +    maxItems: 64
>>> +
>>> +  assigned-clocks:
>>> +    minItems: 1
>>> +    maxItems: 64
>>
>> Both assigned-xxx should not be necessary. Did you try validate the dtbs
>> without these?
> 
> That's how it's currently done for Rockchip cru's.

I understand, but if you remove assigned-xxx from schema what happens?

> 
> 
> ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml

There is no such file in my tree...

> 
>>
>> I guess you added "clocks" above because of these, so you need to
>> correct the DTS because asigned-xxx depend on clocks property.
> 
> See comment above.
> 
>>
>>> +
>>> +  rockchip,grf:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      Phandle to the syscon managing the "general register files" (GRF),
>>> +      if missing pll rates are not changeable, due to the missing pll lock status.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - "#clock-cells"
>>> +  - "#reset-cells"
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    cru: cru@20000000 {
>>
>> Node name: clock-controller
> 
> Which schema?

Devicetree specification (see chapter for node naming). Nodes should
have generic name. There is also list of example names to use.


Best regards,
Krzysztof

Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Stephen Boyd]

Quoting Krzysztof Kozlowski (2022-03-25 00:31:25)
> On 25/03/2022 01:51, Stephen Boyd wrote:
> > Quoting Johan Jonker (2022-03-24 12:51:36)
> >> Hi Heiko, Krzysztof,
> >>
> >> Question for the Rockchip clock maintainer:
> >> What clock should be used here and other SoCs with several clock parents
> >> in the tree?
> >>
> >> The clock.yaml produces a lot off notifications like:
> >>
> >> /arch/arm/boot/dts/rk3036-evb.dtb: clock-controller@20000000: 'clocks'
> >> is a dependency of 'assigned-clocks'
> > 
> > 'clocks' is not a dependency of 'assigned-clocks'. The dt-schema should
> > be fixed to remove that requirement.
> 
> If the driver does not have any clock inputs ("clocks" property), why
> does it care about some clock frequencies and parents?

Because it's a clock provider itself. In this case I suspect because
this is a clock-controller node it was skipping describing some crystal
input though. Maybe it wants to configure the various PLLs in the
clock-controller for a particular board. I can imagine some node with
#clock-cells may want to configure the frequency of the clock outputs or
configure the clk parents for a certain board/SoC. In that case there
may not be any clocks property, but we still want to configure things.

> 
> The clocks is the logical dependency of assigned-clocks, because
> otherwise hardware description is not complete.

Sure, but also #clock-cells indicating that this is a clock-controller
itself means something. The existing bindings are what they are so
forcing bindings to be updated to comply with having a 'clocks' property
doesn't seem very nice.

> 
> What should be here for Rockhip? We had similar cases like this for many
> drivers, I was fixing some of Exynos as well. In my case usually the
> root/external clock was missing, so I supplied is as input clock to the
> clock controller.
> 

Can the schema consider either #clock-cells or clocks? I think that will
work for most cases. It would also be good to have a comment in the
schema or more detail around the definition of assigned-clocks in
bindings/clock/clock-bindings.txt that clocks or #clock-cells are
required. It would be super cool if assigned-clocks only applied if
#clock-cells was present, otherwise clocks property applies, but I doubt
we can do that anymore given how long the binding has been around.

Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to YAML

[Rob Herring]

On Thu, Mar 31, 2022 at 5:38 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Krzysztof Kozlowski (2022-03-25 00:31:25)
> > On 25/03/2022 01:51, Stephen Boyd wrote:
> > > Quoting Johan Jonker (2022-03-24 12:51:36)
> > >> Hi Heiko, Krzysztof,
> > >>
> > >> Question for the Rockchip clock maintainer:
> > >> What clock should be used here and other SoCs with several clock parents
> > >> in the tree?
> > >>
> > >> The clock.yaml produces a lot off notifications like:
> > >>
> > >> /arch/arm/boot/dts/rk3036-evb.dtb: clock-controller@20000000: 'clocks'
> > >> is a dependency of 'assigned-clocks'
> > >
> > > 'clocks' is not a dependency of 'assigned-clocks'. The dt-schema should
> > > be fixed to remove that requirement.
> >
> > If the driver does not have any clock inputs ("clocks" property), why
> > does it care about some clock frequencies and parents?
>
> Because it's a clock provider itself. In this case I suspect because
> this is a clock-controller node it was skipping describing some crystal
> input though. Maybe it wants to configure the various PLLs in the
> clock-controller for a particular board. I can imagine some node with
> #clock-cells may want to configure the frequency of the clock outputs or
> configure the clk parents for a certain board/SoC. In that case there
> may not be any clocks property, but we still want to configure things.
>
> >
> > The clocks is the logical dependency of assigned-clocks, because
> > otherwise hardware description is not complete.
>
> Sure, but also #clock-cells indicating that this is a clock-controller
> itself means something. The existing bindings are what they are so
> forcing bindings to be updated to comply with having a 'clocks' property
> doesn't seem very nice.
>
> >
> > What should be here for Rockhip? We had similar cases like this for many
> > drivers, I was fixing some of Exynos as well. In my case usually the
> > root/external clock was missing, so I supplied is as input clock to the
> > clock controller.
> >
>
> Can the schema consider either #clock-cells or clocks? I think that will
> work for most cases. It would also be good to have a comment in the
> schema or more detail around the definition of assigned-clocks in
> bindings/clock/clock-bindings.txt that clocks or #clock-cells are
> required. It would be super cool if assigned-clocks only applied if
> #clock-cells was present, otherwise clocks property applies, but I doubt
> we can do that anymore given how long the binding has been around.

I've just made a schema change to allow this. This will still require
'assigned-clocks' to be explicitly listed in the clock controller
schemas if 'clocks' is not present.

Please don't add to clock-bindings.txt, but improve the schema
instead. I need Samsung's permission to relicense all the
assigned-clocks text though.

Rob