| Message ID | 20241215053639.738890-1-anarsoul@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: allwinner: a64: fix video output on Pinebook | expand |
On Sun, Dec 15, 2024 at 1:36 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote: > > Since commit ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux"), > TCON0 clock parent is always set to PLL_MIPI, but unfortunately it breaks > video output on Pinebook. > > I did an experiment: I manually configured PLL_MIPI and PLL_VIDEO0_2X > to the same clock rate and flipped the switch with devmem. Experiment clearly > showed that whenever PLL_MIPI is selected as TCON0 clock parent, the video > output stops working. > > To fix the issue, I partially reverted mentioned commit and added explicit > TCON0 clock parent assignment to device tree. By default, it will be > PLL_MIPI, and the only users with RGB output - Pinebook and Teres-I will > override it in their dts. > > Vasily Khoruzhick (3): > dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI > arm64: dts: allwinner: a64: explicitly assign clock parent for TCON0 > clk: sunxi-ng: a64: stop force-selecting PLL-MIPI as TCON0 parent Looks good to me. Reviewed-by: Chen-Yu Tsai <wens@csie.org> I'll wait for a bit for Andre to comment. > arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts | 2 ++ > arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 ++ > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 2 ++ > drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 11 ----------- > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++ > 6 files changed, 8 insertions(+), 13 deletions(-) > > -- > 2.47.1 >
On Sat, 14 Dec 2024 21:34:56 -0800 Vasily Khoruzhick <anarsoul@gmail.com> wrote: Hi Vasily, thanks for tracking this issue down and sending the fixes! > Since commit ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux"), > TCON0 clock parent is always set to PLL_MIPI, but unfortunately it breaks > video output on Pinebook. > > I did an experiment: I manually configured PLL_MIPI and PLL_VIDEO0_2X > to the same clock rate and flipped the switch with devmem. Experiment clearly > showed that whenever PLL_MIPI is selected as TCON0 clock parent, the video > output stops working. That is good info, together with what Roman reported in that patch mentioned above it seems to confirm that the parent clock selection also determines the output path of TCON0. Since there does not seem to be another register or switch setting the path (ignoring the pinmux), I think a DT solution is appropriate here, and assigned-clock-parents is the right way to go. So the patch series looks good to me in general, but we thought that of Roman's series as well, so I would really like to see a Tested-by: from a Pinephone user and ideally a confirmation from Roman that this still works for him. Also I second Dragan's comments about copying the rationale into at least the commit messages (if not in comments). Having explanations in the cover letter is good, but having it in the git repo is much better - as the cover letter will only be in the email archives. Cheers, Andre > > To fix the issue, I partially reverted mentioned commit and added explicit > TCON0 clock parent assignment to device tree. By default, it will be > PLL_MIPI, and the only users with RGB output - Pinebook and Teres-I will > override it in their dts. > > Vasily Khoruzhick (3): > dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI > arm64: dts: allwinner: a64: explicitly assign clock parent for TCON0 > clk: sunxi-ng: a64: stop force-selecting PLL-MIPI as TCON0 parent > > arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts | 2 ++ > arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 ++ > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 2 ++ > drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 11 ----------- > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++ > 6 files changed, 8 insertions(+), 13 deletions(-) >
Hi Vasily, On 2024-12-14 at 21:34:56 -0800, Vasily Khoruzhick <anarsoul@gmail.com> wrote: > Since commit ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux"), > TCON0 clock parent is always set to PLL_MIPI, but unfortunately it breaks > video output on Pinebook. > > I did an experiment: I manually configured PLL_MIPI and PLL_VIDEO0_2X > to the same clock rate and flipped the switch with devmem. Experiment clearly > showed that whenever PLL_MIPI is selected as TCON0 clock parent, the video > output stops working. > > To fix the issue, I partially reverted mentioned commit and added explicit > TCON0 clock parent assignment to device tree. By default, it will be > PLL_MIPI, and the only users with RGB output - Pinebook and Teres-I will > override it in their dts. I've successfully tested this series on my pinephone where it still correctly selects PLL_MIPI. Hence, Tested-by: Frank Oltmanns <frank@oltmanns.dev> # on pinephone I've also tested it on Ondřej's downstream kernel (added him to cc), where also the HDMI output continues to work. Thank you and best regards, Frank > Vasily Khoruzhick (3): > dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI > arm64: dts: allwinner: a64: explicitly assign clock parent for TCON0 > clk: sunxi-ng: a64: stop force-selecting PLL-MIPI as TCON0 parent > > arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts | 2 ++ > arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 ++ > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 2 ++ > drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 11 ----------- > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++ > 6 files changed, 8 insertions(+), 13 deletions(-)
Since commit ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux"), TCON0 clock parent is always set to PLL_MIPI, but unfortunately it breaks video output on Pinebook. I did an experiment: I manually configured PLL_MIPI and PLL_VIDEO0_2X to the same clock rate and flipped the switch with devmem. Experiment clearly showed that whenever PLL_MIPI is selected as TCON0 clock parent, the video output stops working. To fix the issue, I partially reverted mentioned commit and added explicit TCON0 clock parent assignment to device tree. By default, it will be PLL_MIPI, and the only users with RGB output - Pinebook and Teres-I will override it in their dts. Vasily Khoruzhick (3): dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI arm64: dts: allwinner: a64: explicitly assign clock parent for TCON0 clk: sunxi-ng: a64: stop force-selecting PLL-MIPI as TCON0 parent arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts | 2 ++ arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 ++ arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 2 ++ drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 11 ----------- drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++ 6 files changed, 8 insertions(+), 13 deletions(-)