| Message ID | 20220115210907.451189-2-michael@walle.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Revert "arm64: dts: ls1028a-rdb: enable pwm0" | expand |
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 >
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
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
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>;
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(-)