[PATCH v2 0/3] rk3328 cru dt-binding conversion

[PATCH v2 0/3] rk3328 cru dt-binding conversion

[Heiko Stuebner]

Johan already did the heavy lifting of converting the binding in [0].
The binding conversion itself already got a Reviewed-by from dt-people
only dropping the superfluous compatibles needed a bit more investigation.

So I did go through the mainline kernel and also the vendor kernel,
looking for any obscure usage.

The cru should really only be accessed by the defined clock and reset
subsystems of the kernel and thankfully there no users of the cru-dt-node
as syscon and also no syscon-get-from-compatible instances for either
the rockchip,rk3328-cru nor the strange rockchip,cru in either kernel
sources.


[0] https://lore.kernel.org/all/20220329163016.27458-1-jbx6244@gmail.com/

changes in v2:
- explain that the superfluous compatibles are not used at all

Johan Jonker (3):
  dt-bindings: clock: convert rockchip,rk3328-cru.txt to YAML
  arm64: dts: rockchip: fix compatible string rk3328 cru node
  arm64: rockchip: add clocks property to cru node rk3328

 .../bindings/clock/rockchip,rk3328-cru.txt    | 58 ---------------
 .../bindings/clock/rockchip,rk3328-cru.yaml   | 74 +++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328.dtsi      |  4 +-
 3 files changed, 77 insertions(+), 59 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
 create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.yaml

-- 
2.43.0

[PATCH v2 1/3] dt-bindings: clock: convert rockchip,rk3328-cru.txt to YAML

[Heiko Stuebner]

From: Johan Jonker <jbx6244@gmail.com>

Convert RK3328 clock controller bindings to DT schema

Changes against original bindings:
 - Add clocks and clock-names as the device has at least one input clock.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
[add Krzysztof's review from v1, shorten commit description]
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../bindings/clock/rockchip,rk3328-cru.txt    | 58 ---------------
 .../bindings/clock/rockchip,rk3328-cru.yaml   | 74 +++++++++++++++++++
 2 files changed, 74 insertions(+), 58 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
 create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.yaml

diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt b/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
deleted file mode 100644
index 904ae682ea90..000000000000
--- a/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.txt
+++ /dev/null
@@ -1,58 +0,0 @@
-* Rockchip RK3328 Clock and Reset Unit
-
-The RK3328 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,rk3328-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/rk3328-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,
- - "clkin_i2s" - external I2S clock - optional,
- - "gmac_clkin" - external GMAC clock - optional
- - "phy_50m_out" - output clock of the pll in the mac phy
- - "hdmi_phy" - output clock of the hdmi phy pll - optional
-
-Example: Clock controller node:
-
-	cru: clock-controller@ff440000 {
-		compatible = "rockchip,rk3328-cru";
-		reg = <0x0 0xff440000 0x0 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@ff120000 {
-		compatible = "snps,dw-apb-uart";
-		reg = <0xff120000 0x100>;
-		interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
-		reg-shift = <2>;
-		reg-io-width = <4>;
-		clocks = <&cru SCLK_UART0>;
-	};
diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.yaml
new file mode 100644
index 000000000000..f079c7a2559b
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/rockchip,rk3328-cru.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/rockchip,rk3328-cru.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip RK3328 Clock and Reset Unit (CRU)
+
+maintainers:
+  - Elaine Zhang <zhangqing@rock-chips.com>
+  - Heiko Stuebner <heiko@sntech.de>
+
+description: |
+  The RK3328 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/rk3328-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,
+    - "clkin_i2s"   - external I2S clock                     - optional,
+    - "gmac_clkin"  - external GMAC clock                    - optional
+    - "phy_50m_out" - output clock of the pll in the mac phy
+    - "hdmi_phy"    - output clock of the hdmi phy pll       - optional
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3328-cru
+
+  reg:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 1
+
+  "#reset-cells":
+    const: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: xin24m
+
+  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: clock-controller@ff440000 {
+      compatible = "rockchip,rk3328-cru";
+      reg = <0xff440000 0x1000>;
+      rockchip,grf = <&grf>;
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+    };
-- 
2.43.0

[PATCH v2 2/3] arm64: dts: rockchip: fix compatible string rk3328 cru node

[Heiko Stuebner]

From: Johan Jonker <jbx6244@gmail.com>

The cru node references undocumented compatibles of "rockchip,cru" and also
marks it as syscon.

A general rockchip,cru is way too generic to ever be used anywhere, so
needs to go away, similarly the cru should not be written to from other
places, instead regular clock routines should be used.

Both mainline Linux as well as the vendor-kernel up to their 6.1 branch
only reference the cru via the normal assigned-clocks, clocks and resets
properties and do not get a syscon from the node.

Similarly, there is no syscon access by compatible both in mainline
nor the vendor-kernel up to their 6.1 branch, through either the
rockchip,rk3328-cru nor rockchip,cru compatibles.

So these two really are unused in all publically visible places.

Sidenote: the vendor-kernel does pretty crazy stuff in the camera interface
and tdm driver, where they map the cru separately and set clock muxes and
gates directly. This should of course never reach mainline anyway.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
[update commit message, to explain the unused compatibles]
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index 16b4faa22e4f..cf98f1af659e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -813,7 +813,7 @@ hdmiphy: phy@ff430000 {
 	};
 
 	cru: clock-controller@ff440000 {
-		compatible = "rockchip,rk3328-cru", "rockchip,cru", "syscon";
+		compatible = "rockchip,rk3328-cru";
 		reg = <0x0 0xff440000 0x0 0x1000>;
 		rockchip,grf = <&grf>;
 		#clock-cells = <1>;
-- 
2.43.0

Re: [PATCH v2 2/3] arm64: dts: rockchip: fix compatible string rk3328 cru node

[Krzysztof Kozlowski]

On 30/09/2024 23:50, Heiko Stuebner wrote:
> From: Johan Jonker <jbx6244@gmail.com>
> 
> The cru node references undocumented compatibles of "rockchip,cru" and also
> marks it as syscon.
> 
> A general rockchip,cru is way too generic to ever be used anywhere, so
> needs to go away, similarly the cru should not be written to from other
> places, instead regular clock routines should be used.
> 
> Both mainline Linux as well as the vendor-kernel up to their 6.1 branch
> only reference the cru via the normal assigned-clocks, clocks and resets
> properties and do not get a syscon from the node.
> 
> Similarly, there is no syscon access by compatible both in mainline
> nor the vendor-kernel up to their 6.1 branch, through either the
> rockchip,rk3328-cru nor rockchip,cru compatibles.
> 
> So these two really are unused in all publically visible places.
> 
> Sidenote: the vendor-kernel does pretty crazy stuff in the camera interface
> and tdm driver, where they map the cru separately and set clock muxes and
> gates directly. This should of course never reach mainline anyway.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> [update commit message, to explain the unused compatibles]
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 3/3] arm64: rockchip: add clocks property to cru node rk3328

[Heiko Stuebner]

From: Johan Jonker <jbx6244@gmail.com>

Add clocks property to rk3328 cru node to fix warnings like:
'clocks' is a dependency of 'assigned-clocks'

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index cf98f1af659e..b5cbe7cab10b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -815,6 +815,8 @@ hdmiphy: phy@ff430000 {
 	cru: clock-controller@ff440000 {
 		compatible = "rockchip,rk3328-cru";
 		reg = <0x0 0xff440000 0x0 0x1000>;
+		clocks = <&xin24m>;
+		clock-names = "xin24m";
 		rockchip,grf = <&grf>;
 		#clock-cells = <1>;
 		#reset-cells = <1>;
-- 
2.43.0

Re: [PATCH v2 0/3] rk3328 cru dt-binding conversion

[Heiko Stuebner]

On Mon, 30 Sep 2024 23:49:58 +0200, Heiko Stuebner wrote:
> Johan already did the heavy lifting of converting the binding in [0].
> The binding conversion itself already got a Reviewed-by from dt-people
> only dropping the superfluous compatibles needed a bit more investigation.
> 
> So I did go through the mainline kernel and also the vendor kernel,
> looking for any obscure usage.
> 
> [...]

Applied, thanks!

[1/3] dt-bindings: clock: convert rockchip,rk3328-cru.txt to YAML
      commit: 5011cc7ad9aeea98029385f8a0e81a0ebfc45bed
[2/3] arm64: dts: rockchip: fix compatible string rk3328 cru node
      commit: 17a50042b9f63f7c5e9d7f1d1a285387e2b2d955
[3/3] arm64: rockchip: add clocks property to cru node rk3328
      commit: bc639b0ff7a8121bd72954bf8354a81f074dbf42

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>