[PATCH] arm64: dts: exynos: add pwm node for exynosautov9-sadk

[PATCH] arm64: dts: exynos: add pwm node for exynosautov9-sadk

[Jaewon Kim]

Add pwm node to support fan on exynosautov9-sadk board.
PWM channel 3 of ExynosAutov9 is connected to fan for SoC cooling
in SADK board.

Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
---
 arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts | 6 ++++++
 arch/arm64/boot/dts/exynos/exynosautov9.dtsi     | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts b/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
index 898c2fc345ed..e717bb1cad81 100644
--- a/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
+++ b/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
@@ -50,6 +50,12 @@
 	};
 };
 
+&pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pwm_tout3>;
+	status = "okay";
+};
+
 &serial_0 {
 	pinctrl-0 = <&uart0_bus_dual>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
index d3c5cdeff47f..e8860b03fe89 100644
--- a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
@@ -1560,6 +1560,15 @@
 			samsung,syscon-phandle = <&pmu_system_controller>;
 			samsung,cluster-index = <1>;
 		};
+
+		pwm: pwm@103f0000 {
+			compatible = "samsung,exynos4210-pwm";
+			reg = <0x103f0000 0x100>;
+			samsung,pwm-outputs = <0>, <1>, <2>, <3>;
+			#pwm-cells = <3>;
+			clocks = <&xtcxo>;
+			clock-names = "timers";
+		};
 	};
 };
 
-- 
2.17.1

Re: [PATCH] arm64: dts: exynos: add pwm node for exynosautov9-sadk

[Krzysztof Kozlowski]

On 14/07/2023 07:15, Jaewon Kim wrote:
> Add pwm node to support fan on exynosautov9-sadk board.
> PWM channel 3 of ExynosAutov9 is connected to fan for SoC cooling
> in SADK board.
> 
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
>  arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts | 6 ++++++
>  arch/arm64/boot/dts/exynos/exynosautov9.dtsi     | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts b/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
> index 898c2fc345ed..e717bb1cad81 100644
> --- a/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
> +++ b/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
> @@ -50,6 +50,12 @@
>  	};
>  };
>  
> +&pwm {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pwm_tout3>;
> +	status = "okay";
> +};
> +
>  &serial_0 {
>  	pinctrl-0 = <&uart0_bus_dual>;
>  	status = "okay";
> diff --git a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
> index d3c5cdeff47f..e8860b03fe89 100644
> --- a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
> @@ -1560,6 +1560,15 @@
>  			samsung,syscon-phandle = <&pmu_system_controller>;
>  			samsung,cluster-index = <1>;
>  		};
> +
> +		pwm: pwm@103f0000 {
> +			compatible = "samsung,exynos4210-pwm";

Thanks for the patch. I think we should change existing practice for
Samsung SoC and start adding dedicated specific compatible for such
blocks. It's the same practice we follow in other SoCs. It's also
recommendation I give to other platforms.

Therefore this should be "samsung,exynosautov9-pwm",
"samsung,exynos4210-pwm". Feel free to update other DTS as well.

> +			reg = <0x103f0000 0x100>;
> +			samsung,pwm-outputs = <0>, <1>, <2>, <3>;
> +			#pwm-cells = <3>;
> +			clocks = <&xtcxo>;

This does not look like correct clock. Are you sure XTCXO goes to PWM?



Best regards,
Krzysztof

Re: [PATCH] arm64: dts: exynos: add pwm node for exynosautov9-sadk

[Jaewon Kim]

Hi Krzysztof


On 23. 7. 14. 14:26, Krzysztof Kozlowski wrote:
> On 14/07/2023 07:15, Jaewon Kim wrote:
>> Add pwm node to support fan on exynosautov9-sadk board.
>> PWM channel 3 of ExynosAutov9 is connected to fan for SoC cooling
>> in SADK board.
>>
>> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
>> ---
>>   arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts | 6 ++++++
>>   arch/arm64/boot/dts/exynos/exynosautov9.dtsi     | 9 +++++++++
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts b/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
>> index 898c2fc345ed..e717bb1cad81 100644
>> --- a/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
>> +++ b/arch/arm64/boot/dts/exynos/exynosautov9-sadk.dts
>> @@ -50,6 +50,12 @@
>>   	};
>>   };
>>   
>> +&pwm {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pwm_tout3>;
>> +	status = "okay";
>> +};
>> +
>>   &serial_0 {
>>   	pinctrl-0 = <&uart0_bus_dual>;
>>   	status = "okay";
>> diff --git a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
>> index d3c5cdeff47f..e8860b03fe89 100644
>> --- a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
>> +++ b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
>> @@ -1560,6 +1560,15 @@
>>   			samsung,syscon-phandle = <&pmu_system_controller>;
>>   			samsung,cluster-index = <1>;
>>   		};
>> +
>> +		pwm: pwm@103f0000 {
>> +			compatible = "samsung,exynos4210-pwm";
> Thanks for the patch. I think we should change existing practice for
> Samsung SoC and start adding dedicated specific compatible for such
> blocks. It's the same practice we follow in other SoCs. It's also
> recommendation I give to other platforms.
>
> Therefore this should be "samsung,exynosautov9-pwm",
> "samsung,exynos4210-pwm". Feel free to update other DTS as well.

Okay, I will update "samsung,exynosautov9-pwm" compatible in next version.


>
>> +			reg = <0x103f0000 0x100>;
>> +			samsung,pwm-outputs = <0>, <1>, <2>, <3>;
>> +			#pwm-cells = <3>;
>> +			clocks = <&xtcxo>;
> This does not look like correct clock. Are you sure XTCXO goes to PWM?
>
>
>
> Best regards,
> Krzysztof
>
>

Thanks

Jaewon Kim

RE: [PATCH] arm64: dts: exynos: add pwm node for exynosautov9-sadk

[Chanho Park]

> > +			reg = <0x103f0000 0x100>;
> > +			samsung,pwm-outputs = <0>, <1>, <2>, <3>;
> > +			#pwm-cells = <3>;
> > +			clocks = <&xtcxo>;
> 
> This does not look like correct clock. Are you sure XTCXO goes to PWM?

Yes. XTXCO is the source clock of the pwm. Unlike any other exynos SoCs, the clock is directly derived from the external OSC.
Thus, it cannot be controllable such as gating.

Best Regards,
Chanho Park

Re: [PATCH] arm64: dts: exynos: add pwm node for exynosautov9-sadk

[Jaewon Kim]

On 23. 7. 14. 14:59, Chanho Park wrote:
>>> +			reg = <0x103f0000 0x100>;
>>> +			samsung,pwm-outputs = <0>, <1>, <2>, <3>;
>>> +			#pwm-cells = <3>;
>>> +			clocks = <&xtcxo>;
>> This does not look like correct clock. Are you sure XTCXO goes to PWM?
> Yes. XTXCO is the source clock of the pwm. Unlike any other exynos SoCs, the clock is directly derived from the external OSC.
> Thus, it cannot be controllable such as gating.


Thanks Chanho.

I miss this comment.


>
> Best Regards,
> Chanho Park
>
>

Thanks

Jaewon Kim

Re: [PATCH] arm64: dts: exynos: add pwm node for exynosautov9-sadk

[Krzysztof Kozlowski]

On 14/07/2023 07:59, Chanho Park wrote:
>>> +			reg = <0x103f0000 0x100>;
>>> +			samsung,pwm-outputs = <0>, <1>, <2>, <3>;
>>> +			#pwm-cells = <3>;
>>> +			clocks = <&xtcxo>;
>>
>> This does not look like correct clock. Are you sure XTCXO goes to PWM?
> 
> Yes. XTXCO is the source clock of the pwm. Unlike any other exynos SoCs, the clock is directly derived from the external OSC.
> Thus, it cannot be controllable such as gating.

Sure, thanks for clarifying.

Best regards,
Krzysztof