diff mbox series

[v4,2/4] arm64: dts: exynos: add initial CMU clock nodes in ExynosAuto v920

Message ID 20240722223333.1137947-3-sunyeal.hong@samsung.com (mailing list archive)
State New
Headers show
Series [v4,1/4] dt-bindings: clock: add ExynosAuto v920 SoC CMU bindings | expand

Commit Message

Sunyeal Hong July 22, 2024, 10:33 p.m. UTC
Add cmu_top, cmu_peric0 clock nodes and
switch USI clocks instead of dummy fixed-rate-clock.

Signed-off-by: Sunyeal Hong <sunyeal.hong@samsung.com>
---
 .../arm64/boot/dts/exynos/exynosautov920.dtsi | 40 +++++++++++++------
 1 file changed, 27 insertions(+), 13 deletions(-)

Comments

Krzysztof Kozlowski July 24, 2024, 10:13 a.m. UTC | #1
On 23/07/2024 00:33, Sunyeal Hong wrote:
> Add cmu_top, cmu_peric0 clock nodes and
> switch USI clocks instead of dummy fixed-rate-clock.
> 
> Signed-off-by: Sunyeal Hong <sunyeal.hong@samsung.com>

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

>  	cpus: cpus {
>  		#address-cells = <2>;
>  		#size-cells = <0>;
> @@ -182,6 +172,28 @@ chipid@10000000 {
>  			reg = <0x10000000 0x24>;
>  		};
>  
> +		cmu_peric0: clock-controller@10800000 {
> +			compatible = "samsung,exynosautov920-cmu-peric0";
> +			reg = <0x10800000 0x8000>;
> +			#clock-cells = <1>;
> +
> +			clocks = <&xtcxo>,
> +				 <&cmu_top DOUT_CLKCMU_PERIC0_NOC>,
> +				 <&cmu_top DOUT_CLKCMU_PERIC0_IP>;
> +			clock-names = "oscclk",
> +				      "noc",
> +				      "ip";
> +		};
> +
> +		cmu_top: clock-controller@11000000 {

This does not look ordered. Please read DTS coding style.

Best regards,
Krzysztof
Tudor Ambarus July 24, 2024, 11:17 a.m. UTC | #2
Hi, Sunyeal,

I quickly skimmed over the series and I fail to see where/how the HW
auto clock gating is enabled/configured. Would you please add more
details on how this works?

On 7/22/24 11:33 PM, Sunyeal Hong wrote:
> Add cmu_top, cmu_peric0 clock nodes and
> switch USI clocks instead of dummy fixed-rate-clock.
> 
> Signed-off-by: Sunyeal Hong <sunyeal.hong@samsung.com>
> ---
>  .../arm64/boot/dts/exynos/exynosautov920.dtsi | 40 +++++++++++++------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
> index c1c8566d74f5..54fc32074379 100644
> --- a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi


cut

> @@ -224,7 +237,8 @@ serial_0: serial@10880000 {
>  				interrupts = <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
>  				pinctrl-names = "default";
>  				pinctrl-0 = <&uart0_bus>;
> -				clocks = <&clock_usi>, <&clock_usi>;
> +				clocks = <&cmu_peric0 CLK_MOUT_PERIC0_NOC_USER>,

isn't this MUX common to multiple GATEs? Wouldn't turning it off affect
other users than the serial?

Thanks,
ta

> +					 <&cmu_peric0 CLK_DOUT_PERIC0_USI00_USI>;
>  				clock-names = "uart", "clk_uart_baud0";
>  				samsung,uart-fifosize = <256>;
>  				status = "disabled";
Sunyeal Hong July 25, 2024, 1:28 a.m. UTC | #3
Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Wednesday, July 24, 2024 7:14 PM
> To: Sunyeal Hong <sunyeal.hong@samsung.com>; Sylwester Nawrocki
> <s.nawrocki@samsung.com>; Chanwoo Choi <cw00.choi@samsung.com>; Alim
> Akhtar <alim.akhtar@samsung.com>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring
> <robh@kernel.org>; Conor Dooley <conor+dt@kernel.org>
> Cc: linux-samsung-soc@vger.kernel.org; linux-clk@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v4 2/4] arm64: dts: exynos: add initial CMU clock
> nodes in ExynosAuto v920
> 
> On 23/07/2024 00:33, Sunyeal Hong wrote:
> > Add cmu_top, cmu_peric0 clock nodes and switch USI clocks instead of
> > dummy fixed-rate-clock.
> >
> > Signed-off-by: Sunyeal Hong <sunyeal.hong@samsung.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> >  	cpus: cpus {
> >  		#address-cells = <2>;
> >  		#size-cells = <0>;
> > @@ -182,6 +172,28 @@ chipid@10000000 {
> >  			reg = <0x10000000 0x24>;
> >  		};
> >
> > +		cmu_peric0: clock-controller@10800000 {
> > +			compatible = "samsung,exynosautov920-cmu-peric0";
> > +			reg = <0x10800000 0x8000>;
> > +			#clock-cells = <1>;
> > +
> > +			clocks = <&xtcxo>,
> > +				 <&cmu_top DOUT_CLKCMU_PERIC0_NOC>,
> > +				 <&cmu_top DOUT_CLKCMU_PERIC0_IP>;
> > +			clock-names = "oscclk",
> > +				      "noc",
> > +				      "ip";
> > +		};
> > +
> > +		cmu_top: clock-controller@11000000 {
> 
> This does not look ordered. Please read DTS coding style.
> 
I will modify it by referring to the Order of Nodes item in the dts coding style guide document.

> Best regards,
> Krzysztof

Thanks,
Sunyeal Hong.
Sunyeal Hong July 25, 2024, 3:09 a.m. UTC | #4
Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <tudor.ambarus@linaro.org>
> Sent: Wednesday, July 24, 2024 8:18 PM
> To: Sunyeal Hong <sunyeal.hong@samsung.com>; Krzysztof Kozlowski
> <krzk@kernel.org>; Sylwester Nawrocki <s.nawrocki@samsung.com>; Chanwoo
> Choi <cw00.choi@samsung.com>; Alim Akhtar <alim.akhtar@samsung.com>;
> Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@kernel.org>; Rob Herring <robh@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>
> Cc: linux-samsung-soc@vger.kernel.org; linux-clk@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v4 2/4] arm64: dts: exynos: add initial CMU clock
> nodes in ExynosAuto v920
> 
> Hi, Sunyeal,
> 
> I quickly skimmed over the series and I fail to see where/how the HW auto
> clock gating is enabled/configured. Would you please add more details on
> how this works?
>
The HW auto clock gating function is activated in the bootloader stage. So we don't have to control it in the kernel.

> On 7/22/24 11:33 PM, Sunyeal Hong wrote:
> > Add cmu_top, cmu_peric0 clock nodes and switch USI clocks instead of
> > dummy fixed-rate-clock.
> >
> > Signed-off-by: Sunyeal Hong <sunyeal.hong@samsung.com>
> > ---
> >  .../arm64/boot/dts/exynos/exynosautov920.dtsi | 40
> > +++++++++++++------
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
> > b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
> > index c1c8566d74f5..54fc32074379 100644
> > --- a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
> > +++ b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
> 
> 
> cut
> 
> > @@ -224,7 +237,8 @@ serial_0: serial@10880000 {
> >  				interrupts = <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
> >  				pinctrl-names = "default";
> >  				pinctrl-0 = <&uart0_bus>;
> > -				clocks = <&clock_usi>, <&clock_usi>;
> > +				clocks = <&cmu_peric0 CLK_MOUT_PERIC0_NOC_USER>,
> 
> isn't this MUX common to multiple GATEs? Wouldn't turning it off affect
> other users than the serial?
> 
> Thanks,
> ta
> 
I don’t think there will be any problems you are worried about through the enable count of CCF.

Thanks,
Sunyeal Hong.
> > +					 <&cmu_peric0 CLK_DOUT_PERIC0_USI00_USI>;
> >  				clock-names = "uart", "clk_uart_baud0";
> >  				samsung,uart-fifosize = <256>;
> >  				status = "disabled";
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
index c1c8566d74f5..54fc32074379 100644
--- a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
@@ -6,6 +6,7 @@ 
  *
  */
 
+#include <dt-bindings/clock/samsung,exynosautov920.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/soc/samsung,exynos-usi.h>
 
@@ -38,17 +39,6 @@  xtcxo: clock {
 		clock-output-names = "oscclk";
 	};
 
-	/*
-	 * FIXME: Keep the stub clock for serial driver, until proper clock
-	 * driver is implemented.
-	 */
-	clock_usi: clock-usi {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <200000000>;
-		clock-output-names = "usi";
-	};
-
 	cpus: cpus {
 		#address-cells = <2>;
 		#size-cells = <0>;
@@ -182,6 +172,28 @@  chipid@10000000 {
 			reg = <0x10000000 0x24>;
 		};
 
+		cmu_peric0: clock-controller@10800000 {
+			compatible = "samsung,exynosautov920-cmu-peric0";
+			reg = <0x10800000 0x8000>;
+			#clock-cells = <1>;
+
+			clocks = <&xtcxo>,
+				 <&cmu_top DOUT_CLKCMU_PERIC0_NOC>,
+				 <&cmu_top DOUT_CLKCMU_PERIC0_IP>;
+			clock-names = "oscclk",
+				      "noc",
+				      "ip";
+		};
+
+		cmu_top: clock-controller@11000000 {
+			compatible = "samsung,exynosautov920-cmu-top";
+			reg = <0x11000000 0x8000>;
+			#clock-cells = <1>;
+
+			clocks = <&xtcxo>;
+			clock-names = "oscclk";
+		};
+
 		gic: interrupt-controller@10400000 {
 			compatible = "arm,gic-v3";
 			#interrupt-cells = <3>;
@@ -213,7 +225,8 @@  usi_0: usi@108800c0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
-			clocks = <&clock_usi>, <&clock_usi>;
+			clocks = <&cmu_peric0 CLK_MOUT_PERIC0_NOC_USER>,
+				 <&cmu_peric0 CLK_DOUT_PERIC0_USI00_USI>;
 			clock-names = "pclk", "ipclk";
 			status = "disabled";
 
@@ -224,7 +237,8 @@  serial_0: serial@10880000 {
 				interrupts = <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&uart0_bus>;
-				clocks = <&clock_usi>, <&clock_usi>;
+				clocks = <&cmu_peric0 CLK_MOUT_PERIC0_NOC_USER>,
+					 <&cmu_peric0 CLK_DOUT_PERIC0_USI00_USI>;
 				clock-names = "uart", "clk_uart_baud0";
 				samsung,uart-fifosize = <256>;
 				status = "disabled";