diff mbox series

arm64: dts: exynos: add pwm node for exynosautov9-sadk

Message ID 20230714051521.22720-1-jaewon02.kim@samsung.com (mailing list archive)
State New, archived
Headers show
Series arm64: dts: exynos: add pwm node for exynosautov9-sadk | expand

Commit Message

Jaewon Kim July 14, 2023, 5:15 a.m. UTC
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(+)

Comments

Krzysztof Kozlowski July 14, 2023, 5:26 a.m. UTC | #1
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
Jaewon Kim July 14, 2023, 5:28 a.m. UTC | #2
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
Chanho Park July 14, 2023, 5:59 a.m. UTC | #3
> > +			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
Jaewon Kim July 14, 2023, 6:10 a.m. UTC | #4
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
Krzysztof Kozlowski July 14, 2023, 6:29 a.m. UTC | #5
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
diff mbox series

Patch

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";
+		};
 	};
 };