[PATCH 1/2] Revert "arm64: dts: ls1028a-rdb: enable pwm0"

[PATCH 1/2] Revert "arm64: dts: ls1028a-rdb: enable pwm0"

[Michael Walle]

We need to revert commit 71799672ea24d ("arm64: dts: ls1028a: add
flextimer based pwm nodes"). To not break the LS1028A-RDB board dts, we
also need to revert this commit.

This reverts commit b2e2d3e02fb60f4d45843ceebef0465c59c8d392.

Fixes: b2e2d3e02fb6 ("arm64: dts: ls1028a-rdb: enable pwm0")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
index 68c31cb8eead..6d26b010187c 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
@@ -286,10 +286,6 @@ &optee {
 	status = "okay";
 };
 
-&pwm0 {
-	status = "okay";
-};
-
 &sai4 {
 	status = "okay";
 };
-- 
2.30.2

[PATCH 2/2] Revert "arm64: dts: ls1028a: add flextimer based pwm nodes"

[Michael Walle]

The changes to the device tree look very wrong. There are now two
devices with the same base address: pwm0 and ftm_alarm0. Both are using
the Flex Timer Module. It seems like this should either be one driver or
and MFD driver. Either way, there should only be one node in the device
tree. Revert the offending changes to avoid getting a broken device tree
in circulation.

This reverts commit 71799672ea24d2935322d655d2407c1ccd7db9ff.

Fixes: 71799672ea24 ("arm64: dts: ls1028a: add flextimer based pwm nodes")
Signed-off-by: Michael Walle <michael@walle.cc>
---

Btw, the nodes are also not sorted by the address. Otherwise it should have
been spotted easily.

 .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 95 -------------------
 1 file changed, 95 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
index 1ef26cbc79cb..d3cc8cbaf768 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -66,13 +66,6 @@ CPU_PW20: cpu-pw20 {
 		};
 	};
 
-	rtc_clk: rtc-clk {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <32768>;
-		clock-output-names = "rtc_clk";
-	};
-
 	sysclk: sysclk {
 		compatible = "fixed-clock";
 		#clock-cells = <0>;
@@ -1245,94 +1238,6 @@ ierb@1f0800000 {
 			reg = <0x01 0xf0800000 0x0 0x10000>;
 		};
 
-		pwm0: pwm@2800000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2800000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
-		pwm1: pwm@2810000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2810000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
-		pwm2: pwm@2820000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2820000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
-		pwm3: pwm@2830000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2830000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
-		pwm4: pwm@2840000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2840000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
-		pwm5: pwm@2850000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2850000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
-		pwm6: pwm@2860000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2860000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
-		pwm7: pwm@2870000 {
-			compatible = "fsl,vf610-ftm-pwm";
-			#pwm-cells = <3>;
-			reg = <0x0 0x2870000 0x0 0x10000>;
-			clock-names = "ftm_sys", "ftm_ext",
-				      "ftm_fix", "ftm_cnt_clk_en";
-			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
-				 <&rtc_clk>, <&clockgen 4 1>;
-			status = "disabled";
-		};
-
 		rcpm: power-controller@1e34040 {
 			compatible = "fsl,ls1028a-rcpm", "fsl,qoriq-rcpm-2.1+";
 			reg = <0x0 0x1e34040 0x0 0x1c>;
-- 
2.30.2

Re: [PATCH 2/2] Revert "arm64: dts: ls1028a: add flextimer based pwm nodes"

[Shawn Guo]

On Sat, Jan 15, 2022 at 10:09:07PM +0100, Michael Walle wrote:
> The changes to the device tree look very wrong. There are now two
> devices with the same base address: pwm0 and ftm_alarm0. Both are using
> the Flex Timer Module. It seems like this should either be one driver or
> and MFD driver. Either way, there should only be one node in the device
> tree. Revert the offending changes to avoid getting a broken device tree
> in circulation.

Why not just fix the conflicting nodes?

Shawn

> 
> This reverts commit 71799672ea24d2935322d655d2407c1ccd7db9ff.
> 
> Fixes: 71799672ea24 ("arm64: dts: ls1028a: add flextimer based pwm nodes")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> 
> Btw, the nodes are also not sorted by the address. Otherwise it should have
> been spotted easily.
> 
>  .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 95 -------------------
>  1 file changed, 95 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> index 1ef26cbc79cb..d3cc8cbaf768 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> @@ -66,13 +66,6 @@ CPU_PW20: cpu-pw20 {
>  		};
>  	};
>  
> -	rtc_clk: rtc-clk {
> -		compatible = "fixed-clock";
> -		#clock-cells = <0>;
> -		clock-frequency = <32768>;
> -		clock-output-names = "rtc_clk";
> -	};
> -
>  	sysclk: sysclk {
>  		compatible = "fixed-clock";
>  		#clock-cells = <0>;
> @@ -1245,94 +1238,6 @@ ierb@1f0800000 {
>  			reg = <0x01 0xf0800000 0x0 0x10000>;
>  		};
>  
> -		pwm0: pwm@2800000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2800000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
> -		pwm1: pwm@2810000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2810000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
> -		pwm2: pwm@2820000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2820000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
> -		pwm3: pwm@2830000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2830000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
> -		pwm4: pwm@2840000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2840000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
> -		pwm5: pwm@2850000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2850000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
> -		pwm6: pwm@2860000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2860000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
> -		pwm7: pwm@2870000 {
> -			compatible = "fsl,vf610-ftm-pwm";
> -			#pwm-cells = <3>;
> -			reg = <0x0 0x2870000 0x0 0x10000>;
> -			clock-names = "ftm_sys", "ftm_ext",
> -				      "ftm_fix", "ftm_cnt_clk_en";
> -			clocks = <&clockgen 4 1>, <&clockgen 4 1>,
> -				 <&rtc_clk>, <&clockgen 4 1>;
> -			status = "disabled";
> -		};
> -
>  		rcpm: power-controller@1e34040 {
>  			compatible = "fsl,ls1028a-rcpm", "fsl,qoriq-rcpm-2.1+";
>  			reg = <0x0 0x1e34040 0x0 0x1c>;
> -- 
> 2.30.2
> 

Re: [PATCH 2/2] Revert "arm64: dts: ls1028a: add flextimer based pwm nodes"

[Michael Walle]

Am 2022-01-29 06:42, schrieb Shawn Guo:
> On Sat, Jan 15, 2022 at 10:09:07PM +0100, Michael Walle wrote:
>> The changes to the device tree look very wrong. There are now two
>> devices with the same base address: pwm0 and ftm_alarm0. Both are 
>> using
>> the Flex Timer Module. It seems like this should either be one driver 
>> or
>> and MFD driver. Either way, there should only be one node in the 
>> device
>> tree. Revert the offending changes to avoid getting a broken device 
>> tree
>> in circulation.
> 
> Why not just fix the conflicting nodes?

And how would you fix it? There are two conflicting drivers. Like
I said, maybe it should be an MFD driver, see for example
Documentation/devicetree/bindings/mfd/atmel-flexcom.txt, which also
have a common block which you can switch between different modes. But
that would mean a lot of work which I don't think will happen until
the next merge window, let alone that this is not a bugfix.

Therefore, the only sensible thing is to revert the latest changes,
so that you don't have a broken device tree released with 5.17 and
work on a proper support for the next release.

And TBH, I'd expect that NXP will fix this.

-michael

Re: [PATCH 2/2] Revert "arm64: dts: ls1028a: add flextimer based pwm nodes"

[Michael Walle]

Am 2022-01-29 11:12, schrieb Michael Walle:
> Am 2022-01-29 06:42, schrieb Shawn Guo:
>> On Sat, Jan 15, 2022 at 10:09:07PM +0100, Michael Walle wrote:
>>> The changes to the device tree look very wrong. There are now two
>>> devices with the same base address: pwm0 and ftm_alarm0. Both are 
>>> using
>>> the Flex Timer Module. It seems like this should either be one driver 
>>> or
>>> and MFD driver. Either way, there should only be one node in the 
>>> device
>>> tree. Revert the offending changes to avoid getting a broken device 
>>> tree
>>> in circulation.
>> 
>> Why not just fix the conflicting nodes?
> 
> And how would you fix it? There are two conflicting drivers. Like
> I said, maybe it should be an MFD driver, see for example
> Documentation/devicetree/bindings/mfd/atmel-flexcom.txt, which also
> have a common block which you can switch between different modes. But
> that would mean a lot of work which I don't think will happen until
> the next merge window, let alone that this is not a bugfix.
> 
> Therefore, the only sensible thing is to revert the latest changes,
> so that you don't have a broken device tree released with 5.17 and
> work on a proper support for the next release.
> 
> And TBH, I'd expect that NXP will fix this.

I've just asked Rob and it is ok to have two nodes which have the
same reg on the same parent if one (or both) are disabled.

So you can drop this patch, but someone should really sort these
nodes by address.

I still think it would have been nicer the one proper driver for
the flextimer.

-michael