Re: [PATCH 0/3] arm64: allwinner: a64: fix video output on Pinebook

[Andre Przywara <andre.przywara@arm.com>]

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(-)
>