2023-07-14 05:32:59

by Jaewon Kim

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

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 <[email protected]>
---
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



2023-07-14 05:40:39

by Krzysztof Kozlowski

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

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 <[email protected]>
> ---
> 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


2023-07-14 05:41:22

by Jaewon Kim

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

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 <[email protected]>
>> ---
>> 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


2023-07-14 06:16:21

by Chanho Park

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

> > + 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


2023-07-14 06:28:40

by Jaewon Kim

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


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


2023-07-14 07:06:24

by Krzysztof Kozlowski

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

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