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

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

[Vasily Khoruzhick]

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

-- 
2.47.1

[PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Vasily Khoruzhick]

These will be used to explicitly select TCON0 clock parent in dts

Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
 include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
index a8c11c0b4e06..dfba88a5ad0f 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
@@ -21,7 +21,6 @@
 
 /* PLL_VIDEO0 exported for HDMI PHY */
 
-#define CLK_PLL_VIDEO0_2X		8
 #define CLK_PLL_VE			9
 #define CLK_PLL_DDR0			10
 
@@ -32,7 +31,6 @@
 #define CLK_PLL_PERIPH1_2X		14
 #define CLK_PLL_VIDEO1			15
 #define CLK_PLL_GPU			16
-#define CLK_PLL_MIPI			17
 #define CLK_PLL_HSIC			18
 #define CLK_PLL_DE			19
 #define CLK_PLL_DDR1			20
diff --git a/include/dt-bindings/clock/sun50i-a64-ccu.h b/include/dt-bindings/clock/sun50i-a64-ccu.h
index 175892189e9d..4f220ea7a23c 100644
--- a/include/dt-bindings/clock/sun50i-a64-ccu.h
+++ b/include/dt-bindings/clock/sun50i-a64-ccu.h
@@ -44,7 +44,9 @@
 #define _DT_BINDINGS_CLK_SUN50I_A64_H_
 
 #define CLK_PLL_VIDEO0		7
+#define CLK_PLL_VIDEO0_2X	8
 #define CLK_PLL_PERIPH0		11
+#define CLK_PLL_MIPI		17
 
 #define CLK_CPUX		21
 #define CLK_BUS_MIPI_DSI	28
-- 
2.47.1

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Dragan Simic]

Hello Vasily,

On 2024-12-15 06:34, Vasily Khoruzhick wrote:
> These will be used to explicitly select TCON0 clock parent in dts
> 
> Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in 
> TCON0 mux")
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Looking good to me, as a preparatory patch.  Please feel free
to include

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
>  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
> b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
> index a8c11c0b4e06..dfba88a5ad0f 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
> @@ -21,7 +21,6 @@
> 
>  /* PLL_VIDEO0 exported for HDMI PHY */
> 
> -#define CLK_PLL_VIDEO0_2X		8
>  #define CLK_PLL_VE			9
>  #define CLK_PLL_DDR0			10
> 
> @@ -32,7 +31,6 @@
>  #define CLK_PLL_PERIPH1_2X		14
>  #define CLK_PLL_VIDEO1			15
>  #define CLK_PLL_GPU			16
> -#define CLK_PLL_MIPI			17
>  #define CLK_PLL_HSIC			18
>  #define CLK_PLL_DE			19
>  #define CLK_PLL_DDR1			20
> diff --git a/include/dt-bindings/clock/sun50i-a64-ccu.h
> b/include/dt-bindings/clock/sun50i-a64-ccu.h
> index 175892189e9d..4f220ea7a23c 100644
> --- a/include/dt-bindings/clock/sun50i-a64-ccu.h
> +++ b/include/dt-bindings/clock/sun50i-a64-ccu.h
> @@ -44,7 +44,9 @@
>  #define _DT_BINDINGS_CLK_SUN50I_A64_H_
> 
>  #define CLK_PLL_VIDEO0		7
> +#define CLK_PLL_VIDEO0_2X	8
>  #define CLK_PLL_PERIPH0		11
> +#define CLK_PLL_MIPI		17
> 
>  #define CLK_CPUX		21
>  #define CLK_BUS_MIPI_DSI	28

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Krzysztof Kozlowski]

On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote:
> These will be used to explicitly select TCON0 clock parent in dts
> 
> Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
>  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 +

You cannot combine these changes.

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Vasily Khoruzhick]

On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote:
> > These will be used to explicitly select TCON0 clock parent in dts
> >
> > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
> >  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 +

Hi Krzysztof,

> You cannot combine these changes.

The patch basically moves defines out from ccu-sun50i-a64.h to
sun50i-a64-ccu.h. How do I split the change without introducing
compilation failure?

> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.

Yeah, it is not clear what do you want me to do, assuming the previous
similar change to sun50i-a64-ccu.h did essentially the same, see
71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9

Regards,
Vasily

> Best regards,
> Krzysztof
>

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Andre Przywara]

On Tue, 17 Dec 2024 10:00:45 -0800
Vasily Khoruzhick <anarsoul@gmail.com> wrote:

Hi,

> On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote:  
> > > These will be used to explicitly select TCON0 clock parent in dts
> > >
> > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > ---
> > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
> > >  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 +  
> 
> Hi Krzysztof,
> 
> > You cannot combine these changes.  
> 
> The patch basically moves defines out from ccu-sun50i-a64.h to
> sun50i-a64-ccu.h. How do I split the change without introducing
> compilation failure?

You can just have the binding part first, adding the (same) definition
to the binding headers. As long as the #define's are not conflicting,
this is fine.
Then remove the now redundant definitions in the kernel headers, with a
subsequent patch.

Cheers,
Andre


> 
> > Please run scripts/checkpatch.pl and fix reported warnings. Then please
> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
> > Some warnings can be ignored, especially from --strict run, but the code
> > here looks like it needs a fix. Feel free to get in touch if the warning
> > is not clear.  
> 
> Yeah, it is not clear what do you want me to do, assuming the previous
> similar change to sun50i-a64-ccu.h did essentially the same, see
> 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9
> 
> Regards,
> Vasily
> 
> > Best regards,
> > Krzysztof
> >  
> 

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Dragan Simic]

Hello all,

On 2024-12-17 22:15, Andre Przywara wrote:
> On Tue, 17 Dec 2024 10:00:45 -0800
> Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>> On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> 
>> wrote:
>> >
>> > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote:
>> > > These will be used to explicitly select TCON0 clock parent in dts
>> > >
>> > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
>> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>> > > ---
>> > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
>> > >  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 +
>> 
>> > You cannot combine these changes.
>> 
>> The patch basically moves defines out from ccu-sun50i-a64.h to
>> sun50i-a64-ccu.h. How do I split the change without introducing
>> compilation failure?
> 
> You can just have the binding part first, adding the (same) definition
> to the binding headers. As long as the #define's are not conflicting,
> this is fine.
> Then remove the now redundant definitions in the kernel headers, with a
> subsequent patch.

Yes, that would be a way to make it formally correct, but also much
less readable and understandable later, as part of the source code
repository.  FWIW, I find this to be an example of the form being
more important than the actual function.

>> > Please run scripts/checkpatch.pl and fix reported warnings. Then please
>> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
>> > Some warnings can be ignored, especially from --strict run, but the code
>> > here looks like it needs a fix. Feel free to get in touch if the warning
>> > is not clear.
>> 
>> Yeah, it is not clear what do you want me to do, assuming the previous
>> similar change to sun50i-a64-ccu.h did essentially the same, see
>> 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Andre Przywara]

On Tue, 17 Dec 2024 23:02:43 +0100
Dragan Simic <dsimic@manjaro.org> wrote:

Hi Dragan,

> Hello all,
> 
> On 2024-12-17 22:15, Andre Przywara wrote:
> > On Tue, 17 Dec 2024 10:00:45 -0800
> > Vasily Khoruzhick <anarsoul@gmail.com> wrote:  
> >> On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> 
> >> wrote:  
> >> >
> >> > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote:  
> >> > > These will be used to explicitly select TCON0 clock parent in dts
> >> > >
> >> > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
> >> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> >> > > ---
> >> > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
> >> > >  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 +  
> >>   
> >> > You cannot combine these changes.  
> >> 
> >> The patch basically moves defines out from ccu-sun50i-a64.h to
> >> sun50i-a64-ccu.h. How do I split the change without introducing
> >> compilation failure?  
> > 
> > You can just have the binding part first, adding the (same) definition
> > to the binding headers. As long as the #define's are not conflicting,
> > this is fine.
> > Then remove the now redundant definitions in the kernel headers, with a
> > subsequent patch.  
> 
> Yes, that would be a way to make it formally correct, but also much
> less readable and understandable later, as part of the source code
> repository.  FWIW, I find this to be an example of the form being
> more important than the actual function.

Not sure I understand your last sentence, exactly, but what
Krzysztof pointed out is that one part (the header change in
include/dt-bindings) is a DT binding patch, so part of a spec, if you
like, the other is Linux *code*. There is the DT rebasing repo, which
cherry-picks DT patches, so they form a separate history there, and
Linux code has no place in there. U-Boot for instance pull this
repo now on a regular basis. So keeping those things strictly separate
is really important here.

Cheers,
Andre.

> >> > Please run scripts/checkpatch.pl and fix reported warnings. Then please
> >> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
> >> > Some warnings can be ignored, especially from --strict run, but the code
> >> > here looks like it needs a fix. Feel free to get in touch if the warning
> >> > is not clear.  
> >> 
> >> Yeah, it is not clear what do you want me to do, assuming the previous
> >> similar change to sun50i-a64-ccu.h did essentially the same, see
> >> 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9  
> 

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Dragan Simic]

Hello Andre,

On 2024-12-18 02:38, Andre Przywara wrote:
> On Tue, 17 Dec 2024 23:02:43 +0100
> Dragan Simic <dsimic@manjaro.org> wrote:
>> On 2024-12-17 22:15, Andre Przywara wrote:
>> > On Tue, 17 Dec 2024 10:00:45 -0800
>> > Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>> >> On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org>
>> >> wrote:
>> >> >
>> >> > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote:
>> >> > > These will be used to explicitly select TCON0 clock parent in dts
>> >> > >
>> >> > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
>> >> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>> >> > > ---
>> >> > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
>> >> > >  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 +
>> >>
>> >> > You cannot combine these changes.
>> >>
>> >> The patch basically moves defines out from ccu-sun50i-a64.h to
>> >> sun50i-a64-ccu.h. How do I split the change without introducing
>> >> compilation failure?
>> >
>> > You can just have the binding part first, adding the (same) definition
>> > to the binding headers. As long as the #define's are not conflicting,
>> > this is fine.
>> > Then remove the now redundant definitions in the kernel headers, with a
>> > subsequent patch.
>> 
>> Yes, that would be a way to make it formally correct, but also much
>> less readable and understandable later, as part of the source code
>> repository.  FWIW, I find this to be an example of the form being
>> more important than the actual function.
> 
> Not sure I understand your last sentence, exactly,

Ah, sorry, I also saw the need to expand that sentence a bit, but only
after I had already sent my message. :/

> but what Krzysztof pointed out is that one part (the header change in
> include/dt-bindings) is a DT binding patch, so part of a spec, if you
> like, the other is Linux *code*. There is the DT rebasing repo, which
> cherry-picks DT patches, so they form a separate history there, and
> Linux code has no place in there. U-Boot for instance pull this
> repo now on a regular basis. So keeping those things strictly separate
> is really important here.

Thanks a lot for your detailed explanation and for reminding me about
the DT rebasing repository. [*]  I forgot about it for a moment, despite
being active in U-Boot development when the repository was introduced,
so yes, you're right that we're now in need to keep the bindings and the
kernel code separate in patches.

However, I can't stop myself from noticing that the way Krzysztof 
replied
left a lot to be desired when it comes to friendliness in general.  Such
an approach is probably what contributed to me forgetting about the DT
rebasing repository for a moment.

[*] 
https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/

>> >> > Please run scripts/checkpatch.pl and fix reported warnings. Then please
>> >> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
>> >> > Some warnings can be ignored, especially from --strict run, but the code
>> >> > here looks like it needs a fix. Feel free to get in touch if the warning
>> >> > is not clear.
>> >>
>> >> Yeah, it is not clear what do you want me to do, assuming the previous
>> >> similar change to sun50i-a64-ccu.h did essentially the same, see
>> >> 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9

Re: [PATCH 1/3] dt-bindings: clock: sunxi: Export PLL_VIDEO_2X and PLL_MIPI

[Vasily Khoruzhick]

On Tue, Dec 17, 2024 at 1:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Tue, 17 Dec 2024 10:00:45 -0800
> Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> Hi,
>
> > On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote:
> > > > These will be used to explicitly select TCON0 clock parent in dts
> > > >
> > > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
> > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > ---
> > > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 2 --
> > > >  include/dt-bindings/clock/sun50i-a64-ccu.h | 2 +
> >
> > Hi Krzysztof,
> >
> > > You cannot combine these changes.
> >
> > The patch basically moves defines out from ccu-sun50i-a64.h to
> > sun50i-a64-ccu.h. How do I split the change without introducing
> > compilation failure?
>
> You can just have the binding part first, adding the (same) definition
> to the binding headers. As long as the #define's are not conflicting,
> this is fine.
> Then remove the now redundant definitions in the kernel headers, with a
> subsequent patch.

Thanks for the suggestion! I'll address it in v2.

[PATCH 2/3] arm64: dts: allwinner: a64: explicitly assign clock parent for TCON0

[Vasily Khoruzhick]

TCON0 seems to need a different clock parent depending on output type.
For RGB it has to be PLL-VIDEO0-2X, while for DSI it has to be PLL-MIPI

Video output doesn't work if incorrect clock is assigned.

On my Pinebook I manually configured PLL-VIDEO0-2X and PLL-MIPI to the same
rate, and while video output works fine with PLL-VIDEO0-2X, it doesn't
work at all (as in no picture) with PLL-MIPI.

Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 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 ++
 3 files changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
index 379c2c8466f5..86d44349e095 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
@@ -390,6 +390,8 @@ &sound {
 &tcon0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&lcd_rgb666_pins>;
+	assigned-clocks = <&ccu CLK_TCON0>;
+	assigned-clock-parents = <&ccu CLK_PLL_VIDEO0_2X>;
 
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
index b407e1dd08a7..ec055510af8b 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
@@ -369,6 +369,8 @@ &sound {
 &tcon0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&lcd_rgb666_pins>;
+	assigned-clocks = <&ccu CLK_TCON0>;
+	assigned-clock-parents = <&ccu CLK_PLL_VIDEO0_2X>;
 
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index a5c3920e0f04..0fecf0abb204 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -445,6 +445,8 @@ tcon0: lcd-controller@1c0c000 {
 			clock-names = "ahb", "tcon-ch0";
 			clock-output-names = "tcon-data-clock";
 			#clock-cells = <0>;
+			assigned-clocks = <&ccu CLK_TCON0>;
+			assigned-clock-parents = <&ccu CLK_PLL_MIPI>;
 			resets = <&ccu RST_BUS_TCON0>, <&ccu RST_BUS_LVDS>;
 			reset-names = "lcd", "lvds";
 
-- 
2.47.1

Re: [PATCH 2/3] arm64: dts: allwinner: a64: explicitly assign clock parent for TCON0

[Dragan Simic]

Hello Vasily,

On 2024-12-15 06:34, Vasily Khoruzhick wrote:
> TCON0 seems to need a different clock parent depending on output type.
> For RGB it has to be PLL-VIDEO0-2X, while for DSI it has to be PLL-MIPI
> 
> Video output doesn't work if incorrect clock is assigned.
> 
> On my Pinebook I manually configured PLL-VIDEO0-2X and PLL-MIPI to the 
> same
> rate, and while video output works fine with PLL-VIDEO0-2X, it doesn't
> work at all (as in no picture) with PLL-MIPI.
> 
> Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in 
> TCON0 mux")
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Thanks for fixing this issue!  I've been banging my head for a while
about how to fix it without introducing new DT properties, and I hope
that the maintainers will like your approach.

I'd suggest that the patch description is improved further a bit, by
incorporating some parts of the good description of the issue that's
already in the cover letter.  With that addressed, please feel free
to include

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  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 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> index 379c2c8466f5..86d44349e095 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> @@ -390,6 +390,8 @@ &sound {
>  &tcon0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&lcd_rgb666_pins>;
> +	assigned-clocks = <&ccu CLK_TCON0>;
> +	assigned-clock-parents = <&ccu CLK_PLL_VIDEO0_2X>;
> 
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> index b407e1dd08a7..ec055510af8b 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> @@ -369,6 +369,8 @@ &sound {
>  &tcon0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&lcd_rgb666_pins>;
> +	assigned-clocks = <&ccu CLK_TCON0>;
> +	assigned-clock-parents = <&ccu CLK_PLL_VIDEO0_2X>;
> 
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index a5c3920e0f04..0fecf0abb204 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -445,6 +445,8 @@ tcon0: lcd-controller@1c0c000 {
>  			clock-names = "ahb", "tcon-ch0";
>  			clock-output-names = "tcon-data-clock";
>  			#clock-cells = <0>;
> +			assigned-clocks = <&ccu CLK_TCON0>;
> +			assigned-clock-parents = <&ccu CLK_PLL_MIPI>;
>  			resets = <&ccu RST_BUS_TCON0>, <&ccu RST_BUS_LVDS>;
>  			reset-names = "lcd", "lvds";

[PATCH 3/3] clk: sunxi-ng: a64: stop force-selecting PLL-MIPI as TCON0 parent

[Vasily Khoruzhick]

Force selecting PLL-MIPI as TCON0 parent breaks video output on Pinebook
that uses RGB to eDP bridge.

TCON0 clock parent will be selected in the device tree instead.

Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 3a7d61c81667..cc8de0bfbc67 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -534,12 +534,6 @@ static SUNXI_CCU_M_WITH_MUX_GATE(de_clk, "de", de_parents,
 				 0x104, 0, 4, 24, 3, BIT(31),
 				 CLK_SET_RATE_PARENT);
 
-/*
- * DSI output seems to work only when PLL_MIPI selected. Set it and prevent
- * the mux from reparenting.
- */
-#define SUN50I_A64_TCON0_CLK_REG	0x118
-
 static const char * const tcon0_parents[] = { "pll-mipi", "pll-video0-2x" };
 static const u8 tcon0_table[] = { 0, 2, };
 static SUNXI_CCU_MUX_TABLE_WITH_GATE_CLOSEST(tcon0_clk, "tcon0", tcon0_parents,
@@ -959,11 +953,6 @@ static int sun50i_a64_ccu_probe(struct platform_device *pdev)
 
 	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
 
-	/* Set PLL MIPI as parent for TCON0 */
-	val = readl(reg + SUN50I_A64_TCON0_CLK_REG);
-	val &= ~GENMASK(26, 24);
-	writel(val | (0 << 24), reg + SUN50I_A64_TCON0_CLK_REG);
-
 	ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_a64_ccu_desc);
 	if (ret)
 		return ret;
-- 
2.47.1

Re: [PATCH 3/3] clk: sunxi-ng: a64: stop force-selecting PLL-MIPI as TCON0 parent

[Dragan Simic]

Hello Vasily,

On 2024-12-15 06:34, Vasily Khoruzhick wrote:
> Force selecting PLL-MIPI as TCON0 parent breaks video output on 
> Pinebook
> that uses RGB to eDP bridge.
> 
> TCON0 clock parent will be selected in the device tree instead.
> 
> Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in 
> TCON0 mux")
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Looking good to me, as a patch that completes the panel bugfix.
Thanks once again for the patches!

I'd suggest that the patch description is improved further a bit,
by incorporating some parts of the good description of the issue
that's already in the cover letter.  In particular, I'd suggest
that you describe that the patch partially reverts an earlier
commit, etc.

Additionally, I'd suggest that the comment block deleted below is
actually adjusted and copied to the addition to the A64 SoC dtsi,
which is performed in the first patch in your series.  That might
be of high value later.

With that addressed, please feel free to include

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> index 3a7d61c81667..cc8de0bfbc67 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> @@ -534,12 +534,6 @@ static SUNXI_CCU_M_WITH_MUX_GATE(de_clk, "de", 
> de_parents,
>  				 0x104, 0, 4, 24, 3, BIT(31),
>  				 CLK_SET_RATE_PARENT);
> 
> -/*
> - * DSI output seems to work only when PLL_MIPI selected. Set it and 
> prevent
> - * the mux from reparenting.
> - */
> -#define SUN50I_A64_TCON0_CLK_REG	0x118
> -
>  static const char * const tcon0_parents[] = { "pll-mipi", 
> "pll-video0-2x" };
>  static const u8 tcon0_table[] = { 0, 2, };
>  static SUNXI_CCU_MUX_TABLE_WITH_GATE_CLOSEST(tcon0_clk, "tcon0", 
> tcon0_parents,
> @@ -959,11 +953,6 @@ static int sun50i_a64_ccu_probe(struct
> platform_device *pdev)
> 
>  	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
> 
> -	/* Set PLL MIPI as parent for TCON0 */
> -	val = readl(reg + SUN50I_A64_TCON0_CLK_REG);
> -	val &= ~GENMASK(26, 24);
> -	writel(val | (0 << 24), reg + SUN50I_A64_TCON0_CLK_REG);
> -
>  	ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_a64_ccu_desc);
>  	if (ret)
>  		return ret;

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

[Chen-Yu Tsai]

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
>

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

[Andre Przywara]

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

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

[Frank Oltmanns]

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