[U-Boot] [PATCH] sunxi: video: HDMI: Fix clock setup

[U-Boot] [PATCH] sunxi: video: HDMI: Fix clock setup

[Jernej Skrabec]

Currently, HDMI driver doesn't consider minimum and maximum allowed rate
of pll3 (video PLL). It works most of the time, but not always.

Consider monitor with resolution 1920x1200, which has pixel clock rate
of 154 MHz. Current code would determine that pll3 rate has to be set to
154 MHz. However, minimum supported rate is 192 MHz. In this case video
output just won't work.

The reason why the driver is written in the way it is, is that at the
time HDMI PHY and clock configuration wasn't fully understood. But now
we have needed knowledge, so the issue can be fixed.

With this fix, clock configuration routine uses full range (1-16) for
clock divider instead of limited one (1, 2, 4, 11). It also considers
minimum and maximum allowed rate for pll3.

Fixes: 56009451d843 ("sunxi: video: Add A64/H3/H5 HDMI driver")
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
Hi!

Unfortunately, issue fixed with patch here has influence on Linux too.
In Linux, minimum and maximum rate is considered only when changing rate.
But because U-Boot already set PLL to "correct" value, Linux clock driver
doesn't see the need to change it. I know that this is Linux driver issue
but let's start at the source of the issue and fix it.

It would be very nice if this can go in 2019.04 release.

Best regards,
Jernej

 drivers/video/sunxi/sunxi_dw_hdmi.c | 62 +++++++++++++++++------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/drivers/video/sunxi/sunxi_dw_hdmi.c b/drivers/video/sunxi/sunxi_dw_hdmi.c
index 9dbea649a0..6fe1aa7ee4 100644
--- a/drivers/video/sunxi/sunxi_dw_hdmi.c
+++ b/drivers/video/sunxi/sunxi_dw_hdmi.c
@@ -132,7 +132,7 @@ static int sunxi_dw_hdmi_wait_for_hpd(void)
 	return -1;
 }
 
-static void sunxi_dw_hdmi_phy_set(uint clock)
+static void sunxi_dw_hdmi_phy_set(uint clock, int phy_div)
 {
 	struct sunxi_hdmi_phy * const phy =
 		(struct sunxi_hdmi_phy *)(SUNXI_HDMI_BASE + HDMI_PHY_OFFS);
@@ -146,7 +146,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
 	switch (div) {
 	case 1:
 		writel(0x30dc5fc0, &phy->pll);
-		writel(0x800863C0, &phy->clk);
+		writel(0x800863C0 | (phy_div - 1), &phy->clk);
 		mdelay(10);
 		writel(0x00000001, &phy->unk3);
 		setbits_le32(&phy->pll, BIT(25));
@@ -164,7 +164,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
 		break;
 	case 2:
 		writel(0x39dc5040, &phy->pll);
-		writel(0x80084381, &phy->clk);
+		writel(0x80084380 | (phy_div - 1), &phy->clk);
 		mdelay(10);
 		writel(0x00000001, &phy->unk3);
 		setbits_le32(&phy->pll, BIT(25));
@@ -178,7 +178,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
 		break;
 	case 4:
 		writel(0x39dc5040, &phy->pll);
-		writel(0x80084343, &phy->clk);
+		writel(0x80084340 | (phy_div - 1), &phy->clk);
 		mdelay(10);
 		writel(0x00000001, &phy->unk3);
 		setbits_le32(&phy->pll, BIT(25));
@@ -192,7 +192,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
 		break;
 	case 11:
 		writel(0x39dc5040, &phy->pll);
-		writel(0x8008430a, &phy->clk);
+		writel(0x80084300 | (phy_div - 1), &phy->clk);
 		mdelay(10);
 		writel(0x00000001, &phy->unk3);
 		setbits_le32(&phy->pll, BIT(25));
@@ -207,36 +207,46 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
 	}
 }
 
-static void sunxi_dw_hdmi_pll_set(uint clk_khz)
+static void sunxi_dw_hdmi_pll_set(uint clk_khz, int *phy_div)
 {
-	int value, n, m, div = 0, diff;
-	int best_n = 0, best_m = 0, best_diff = 0x0FFFFFFF;
-
-	div = sunxi_dw_hdmi_get_divider(clk_khz * 1000);
+	int value, n, m, div, diff;
+	int best_n = 0, best_m = 0, best_div = 0, best_diff = 0x0FFFFFFF;
 
 	/*
 	 * Find the lowest divider resulting in a matching clock. If there
 	 * is no match, pick the closest lower clock, as monitors tend to
 	 * not sync to higher frequencies.
 	 */
-	for (m = 1; m <= 16; m++) {
-		n = (m * div * clk_khz) / 24000;
-
-		if ((n >= 1) && (n <= 128)) {
-			value = (24000 * n) / m / div;
-			diff = clk_khz - value;
-			if (diff < best_diff) {
-				best_diff = diff;
-				best_m = m;
-				best_n = n;
+	for (div = 1; div <= 16; div++) {
+		int target = clk_khz * div;
+
+		if (target < 192000)
+			continue;
+		if (target > 912000)
+			continue;
+
+		for (m = 1; m <= 16; m++) {
+			n = (m * target) / 24000;
+
+			if (n >= 1 && n <= 128) {
+				value = (24000 * n) / m / div;
+				diff = clk_khz - value;
+				if (diff < best_diff) {
+					best_diff = diff;
+					best_m = m;
+					best_n = n;
+					best_div = div;
+				}
 			}
 		}
 	}
 
+	*phy_div = best_div;
+
 	clock_set_pll3_factors(best_m, best_n);
 	debug("dotclock: %dkHz = %dkHz: (24MHz * %d) / %d / %d\n",
-	      clk_khz, (clock_get_pll3() / 1000) / div,
-	      best_n, best_m, div);
+	      clk_khz, (clock_get_pll3() / 1000) / best_div,
+	      best_n, best_m, best_div);
 }
 
 static void sunxi_dw_hdmi_lcdc_init(int mux, const struct display_timing *edid,
@@ -244,7 +254,7 @@ static void sunxi_dw_hdmi_lcdc_init(int mux, const struct display_timing *edid,
 {
 	struct sunxi_ccm_reg * const ccm =
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
-	int div = sunxi_dw_hdmi_get_divider(edid->pixelclock.typ);
+	int div = clock_get_pll3() / edid->pixelclock.typ;
 	struct sunxi_lcdc_reg *lcdc;
 
 	if (mux == 0) {
@@ -276,8 +286,10 @@ static void sunxi_dw_hdmi_lcdc_init(int mux, const struct display_timing *edid,
 
 static int sunxi_dw_hdmi_phy_cfg(struct dw_hdmi *hdmi, uint mpixelclock)
 {
-	sunxi_dw_hdmi_pll_set(mpixelclock/1000);
-	sunxi_dw_hdmi_phy_set(mpixelclock);
+	int phy_div;
+
+	sunxi_dw_hdmi_pll_set(mpixelclock / 1000, &phy_div);
+	sunxi_dw_hdmi_phy_set(mpixelclock, phy_div);
 
 	return 0;
 }
-- 
2.21.0

[U-Boot] [PATCH] sunxi: video: HDMI: Fix clock setup

[Maxime Ripard]

On Sun, Mar 24, 2019 at 07:26:40PM +0100, Jernej Skrabec wrote:
> Currently, HDMI driver doesn't consider minimum and maximum allowed rate
> of pll3 (video PLL). It works most of the time, but not always.
>
> Consider monitor with resolution 1920x1200, which has pixel clock rate
> of 154 MHz. Current code would determine that pll3 rate has to be set to
> 154 MHz. However, minimum supported rate is 192 MHz. In this case video
> output just won't work.
>
> The reason why the driver is written in the way it is, is that at the
> time HDMI PHY and clock configuration wasn't fully understood. But now
> we have needed knowledge, so the issue can be fixed.
>
> With this fix, clock configuration routine uses full range (1-16) for
> clock divider instead of limited one (1, 2, 4, 11). It also considers
> minimum and maximum allowed rate for pll3.
>
> Fixes: 56009451d843 ("sunxi: video: Add A64/H3/H5 HDMI driver")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190325/dcedc1be/attachment.sig>

[U-Boot] [PATCH] sunxi: video: HDMI: Fix clock setup

[Jagan Teki]

On Sun, Mar 24, 2019 at 11:56 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote:
>
> Currently, HDMI driver doesn't consider minimum and maximum allowed rate
> of pll3 (video PLL). It works most of the time, but not always.
>
> Consider monitor with resolution 1920x1200, which has pixel clock rate
> of 154 MHz. Current code would determine that pll3 rate has to be set to
> 154 MHz. However, minimum supported rate is 192 MHz. In this case video
> output just won't work.

I set the monitor with same resolution, but not sure with the pixel
clock and unable to reproduce this with couple time. any specific
usage case that the issue will reproduce ?

>
> The reason why the driver is written in the way it is, is that at the
> time HDMI PHY and clock configuration wasn't fully understood. But now
> we have needed knowledge, so the issue can be fixed.
>
> With this fix, clock configuration routine uses full range (1-16) for
> clock divider instead of limited one (1, 2, 4, 11). It also considers
> minimum and maximum allowed rate for pll3.
>
> Fixes: 56009451d843 ("sunxi: video: Add A64/H3/H5 HDMI driver")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
> Hi!
>
> Unfortunately, issue fixed with patch here has influence on Linux too.
> In Linux, minimum and maximum rate is considered only when changing rate.
> But because U-Boot already set PLL to "correct" value, Linux clock driver
> doesn't see the need to change it. I know that this is Linux driver issue
> but let's start at the source of the issue and fix it.
>
> It would be very nice if this can go in 2019.04 release.
>
> Best regards,
> Jernej
>
>  drivers/video/sunxi/sunxi_dw_hdmi.c | 62 +++++++++++++++++------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/video/sunxi/sunxi_dw_hdmi.c b/drivers/video/sunxi/sunxi_dw_hdmi.c
> index 9dbea649a0..6fe1aa7ee4 100644
> --- a/drivers/video/sunxi/sunxi_dw_hdmi.c
> +++ b/drivers/video/sunxi/sunxi_dw_hdmi.c
> @@ -132,7 +132,7 @@ static int sunxi_dw_hdmi_wait_for_hpd(void)
>         return -1;
>  }
>
> -static void sunxi_dw_hdmi_phy_set(uint clock)
> +static void sunxi_dw_hdmi_phy_set(uint clock, int phy_div)
>  {
>         struct sunxi_hdmi_phy * const phy =
>                 (struct sunxi_hdmi_phy *)(SUNXI_HDMI_BASE + HDMI_PHY_OFFS);
> @@ -146,7 +146,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
>         switch (div) {
>         case 1:
>                 writel(0x30dc5fc0, &phy->pll);
> -               writel(0x800863C0, &phy->clk);
> +               writel(0x800863C0 | (phy_div - 1), &phy->clk);
>                 mdelay(10);
>                 writel(0x00000001, &phy->unk3);
>                 setbits_le32(&phy->pll, BIT(25));
> @@ -164,7 +164,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
>                 break;
>         case 2:
>                 writel(0x39dc5040, &phy->pll);
> -               writel(0x80084381, &phy->clk);
> +               writel(0x80084380 | (phy_div - 1), &phy->clk);
>                 mdelay(10);
>                 writel(0x00000001, &phy->unk3);
>                 setbits_le32(&phy->pll, BIT(25));
> @@ -178,7 +178,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
>                 break;
>         case 4:
>                 writel(0x39dc5040, &phy->pll);
> -               writel(0x80084343, &phy->clk);
> +               writel(0x80084340 | (phy_div - 1), &phy->clk);
>                 mdelay(10);
>                 writel(0x00000001, &phy->unk3);
>                 setbits_le32(&phy->pll, BIT(25));
> @@ -192,7 +192,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
>                 break;
>         case 11:
>                 writel(0x39dc5040, &phy->pll);
> -               writel(0x8008430a, &phy->clk);
> +               writel(0x80084300 | (phy_div - 1), &phy->clk);
>                 mdelay(10);
>                 writel(0x00000001, &phy->unk3);
>                 setbits_le32(&phy->pll, BIT(25));
> @@ -207,36 +207,46 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
>         }
>  }
>
> -static void sunxi_dw_hdmi_pll_set(uint clk_khz)
> +static void sunxi_dw_hdmi_pll_set(uint clk_khz, int *phy_div)
>  {
> -       int value, n, m, div = 0, diff;
> -       int best_n = 0, best_m = 0, best_diff = 0x0FFFFFFF;
> -
> -       div = sunxi_dw_hdmi_get_divider(clk_khz * 1000);
> +       int value, n, m, div, diff;
> +       int best_n = 0, best_m = 0, best_div = 0, best_diff = 0x0FFFFFFF;
>
>         /*
>          * Find the lowest divider resulting in a matching clock. If there
>          * is no match, pick the closest lower clock, as monitors tend to
>          * not sync to higher frequencies.
>          */
> -       for (m = 1; m <= 16; m++) {
> -               n = (m * div * clk_khz) / 24000;
> -
> -               if ((n >= 1) && (n <= 128)) {
> -                       value = (24000 * n) / m / div;
> -                       diff = clk_khz - value;
> -                       if (diff < best_diff) {
> -                               best_diff = diff;
> -                               best_m = m;
> -                               best_n = n;
> +       for (div = 1; div <= 16; div++) {
> +               int target = clk_khz * div;
> +
> +               if (target < 192000)
> +                       continue;
> +               if (target > 912000)
> +                       continue;

I think this can be max allowed rate by PLL_VIDEO0 can't it be 1008 or 600 ?

[U-Boot] [PATCH] sunxi: video: HDMI: Fix clock setup

[Jernej Škrabec]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 7275 bytes --]

Dne četrtek, 28. marec 2019 ob 08:52:48 CET je Jagan Teki napisal(a):
> On Sun, Mar 24, 2019 at 11:56 PM Jernej Skrabec <jernej.skrabec@siol.net> 
wrote:
> > Currently, HDMI driver doesn't consider minimum and maximum allowed rate
> > of pll3 (video PLL). It works most of the time, but not always.
> > 
> > Consider monitor with resolution 1920x1200, which has pixel clock rate
> > of 154 MHz. Current code would determine that pll3 rate has to be set to
> > 154 MHz. However, minimum supported rate is 192 MHz. In this case video
> > output just won't work.
> 
> I set the monitor with same resolution, but not sure with the pixel
> clock and unable to reproduce this with couple time. any specific
> usage case that the issue will reproduce ?

Just enable debug output in sunxi_dw_hdmi.c and clock parameters should be 
displayed during boot (dotclock == pixel clock).

While I don't have such monitor, I got report about non working video output 
on such monitor. This patch solved it.

Issue is most apparent when pixel clock is between 148.5 MHz and 192 MHz. Even 
then, it might work sometimes or unstable. I can tell you that I managed to 
have stable output even when PLL3 was set to 128 MHz, but only if correct PLL 
factors were used. If I set different ones (with same output frequency), it 
didn't work. I imagine it might also depend on a chip batch. Some chips may 
better handle lower rates than others.

At the end, I think there is a reason why Allwinner provided clock driver 
doesn't use frequencies below 192 MHz.

> 
> > The reason why the driver is written in the way it is, is that at the
> > time HDMI PHY and clock configuration wasn't fully understood. But now
> > we have needed knowledge, so the issue can be fixed.
> > 
> > With this fix, clock configuration routine uses full range (1-16) for
> > clock divider instead of limited one (1, 2, 4, 11). It also considers
> > minimum and maximum allowed rate for pll3.
> > 
> > Fixes: 56009451d843 ("sunxi: video: Add A64/H3/H5 HDMI driver")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > Hi!
> > 
> > Unfortunately, issue fixed with patch here has influence on Linux too.
> > In Linux, minimum and maximum rate is considered only when changing rate.
> > But because U-Boot already set PLL to "correct" value, Linux clock driver
> > doesn't see the need to change it. I know that this is Linux driver issue
> > but let's start at the source of the issue and fix it.
> > 
> > It would be very nice if this can go in 2019.04 release.
> > 
> > Best regards,
> > Jernej
> > 
> >  drivers/video/sunxi/sunxi_dw_hdmi.c | 62 +++++++++++++++++------------
> >  1 file changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/video/sunxi/sunxi_dw_hdmi.c
> > b/drivers/video/sunxi/sunxi_dw_hdmi.c index 9dbea649a0..6fe1aa7ee4 100644
> > --- a/drivers/video/sunxi/sunxi_dw_hdmi.c
> > +++ b/drivers/video/sunxi/sunxi_dw_hdmi.c
> > @@ -132,7 +132,7 @@ static int sunxi_dw_hdmi_wait_for_hpd(void)
> > 
> >         return -1;
> >  
> >  }
> > 
> > -static void sunxi_dw_hdmi_phy_set(uint clock)
> > +static void sunxi_dw_hdmi_phy_set(uint clock, int phy_div)
> > 
> >  {
> >  
> >         struct sunxi_hdmi_phy * const phy =
> >         
> >                 (struct sunxi_hdmi_phy *)(SUNXI_HDMI_BASE +
> >                 HDMI_PHY_OFFS);
> > 
> > @@ -146,7 +146,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
> > 
> >         switch (div) {
> >         
> >         case 1:
> >                 writel(0x30dc5fc0, &phy->pll);
> > 
> > -               writel(0x800863C0, &phy->clk);
> > +               writel(0x800863C0 | (phy_div - 1), &phy->clk);
> > 
> >                 mdelay(10);
> >                 writel(0x00000001, &phy->unk3);
> >                 setbits_le32(&phy->pll, BIT(25));
> > 
> > @@ -164,7 +164,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
> > 
> >                 break;
> >         
> >         case 2:
> >                 writel(0x39dc5040, &phy->pll);
> > 
> > -               writel(0x80084381, &phy->clk);
> > +               writel(0x80084380 | (phy_div - 1), &phy->clk);
> > 
> >                 mdelay(10);
> >                 writel(0x00000001, &phy->unk3);
> >                 setbits_le32(&phy->pll, BIT(25));
> > 
> > @@ -178,7 +178,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
> > 
> >                 break;
> >         
> >         case 4:
> >                 writel(0x39dc5040, &phy->pll);
> > 
> > -               writel(0x80084343, &phy->clk);
> > +               writel(0x80084340 | (phy_div - 1), &phy->clk);
> > 
> >                 mdelay(10);
> >                 writel(0x00000001, &phy->unk3);
> >                 setbits_le32(&phy->pll, BIT(25));
> > 
> > @@ -192,7 +192,7 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
> > 
> >                 break;
> >         
> >         case 11:
> >                 writel(0x39dc5040, &phy->pll);
> > 
> > -               writel(0x8008430a, &phy->clk);
> > +               writel(0x80084300 | (phy_div - 1), &phy->clk);
> > 
> >                 mdelay(10);
> >                 writel(0x00000001, &phy->unk3);
> >                 setbits_le32(&phy->pll, BIT(25));
> > 
> > @@ -207,36 +207,46 @@ static void sunxi_dw_hdmi_phy_set(uint clock)
> > 
> >         }
> >  
> >  }
> > 
> > -static void sunxi_dw_hdmi_pll_set(uint clk_khz)
> > +static void sunxi_dw_hdmi_pll_set(uint clk_khz, int *phy_div)
> > 
> >  {
> > 
> > -       int value, n, m, div = 0, diff;
> > -       int best_n = 0, best_m = 0, best_diff = 0x0FFFFFFF;
> > -
> > -       div = sunxi_dw_hdmi_get_divider(clk_khz * 1000);
> > +       int value, n, m, div, diff;
> > +       int best_n = 0, best_m = 0, best_div = 0, best_diff = 0x0FFFFFFF;
> > 
> >         /*
> >         
> >          * Find the lowest divider resulting in a matching clock. If there
> >          * is no match, pick the closest lower clock, as monitors tend to
> >          * not sync to higher frequencies.
> >          */
> > 
> > -       for (m = 1; m <= 16; m++) {
> > -               n = (m * div * clk_khz) / 24000;
> > -
> > -               if ((n >= 1) && (n <= 128)) {
> > -                       value = (24000 * n) / m / div;
> > -                       diff = clk_khz - value;
> > -                       if (diff < best_diff) {
> > -                               best_diff = diff;
> > -                               best_m = m;
> > -                               best_n = n;
> > +       for (div = 1; div <= 16; div++) {
> > +               int target = clk_khz * div;
> > +
> > +               if (target < 192000)
> > +                       continue;
> > +               if (target > 912000)
> > +                       continue;
> 
> I think this can be max allowed rate by PLL_VIDEO0 can't it be 1008 or 600 ?

I'm not sure I understand the question. Anyway, 912 MHz is taken from 
Allwinner clock driver: https://github.com/Allwinner-Homlet/H3-BSP4.4-linux/
blob/master/drivers/clk/sunxi/clk-sun8iw7_tbl.c#L130

Maximum pixel clock supported by HDMI is 297 MHz, but using broader PLL range 
in combination with TCON and PHY clock dividers (both dividers are in range of 
1-16) allows better fine tunning pixel clock and thus more standard and non-
standard resolutions can be supported.

Best regards,
Jernej

[U-Boot] [PATCH] sunxi: video: HDMI: Fix clock setup

[Anatolij Gustschin]

On Sun, 24 Mar 2019 19:26:40 +0100
Jernej Skrabec jernej.skrabec at siol.net wrote:
...
>  drivers/video/sunxi/sunxi_dw_hdmi.c | 62 +++++++++++++++++------------
>  1 file changed, 37 insertions(+), 25 deletions(-)

Applied to u-boot-video/master, thanks!

--
Anatolij