diff mbox series

[07/27] drm/rockchip: dw_hdmi: Use auto-generated tables

Message ID 20220126145549.617165-8-s.hauer@pengutronix.de (mailing list archive)
State New
Headers show
Series drm/rockchip: RK356x VOP2 support | expand

Commit Message

Sascha Hauer Jan. 26, 2022, 2:55 p.m. UTC
From: Douglas Anderson <dianders at chromium.org>

The previous tables for mpll_cfg and curr_ctrl were created using the
20-pages of example settings provided by the PHY vendor.  Those
example settings weren't particularly dense, so there were places
where we were guessing what the settings would be for 10-bit and
12-bit (not that we use those anyway).  It was also always a lot of
extra work every time we wanted to add a new clock rate since we had
to cross-reference several tables.

In <http://crosreview.com/285855> I've gone through the work to figure
out how to generate this table automatically.  Let's now use the
automatically generated table and then we'll never need to look at it
again.

We only support 8-bit mode right now and only support a small number
of clock rates and and I've verified that the only 8-bit rate that was
affected was 148.5.  That mode appears to have been wrong in the old
table.

Changes since v3:
- new patch

Signed-off-by: Douglas Anderson <dianders at chromium.org>
Signed-off-by: Yakir Yang <ykk at rock-chips.com>
Reviewed-by: Stéphane Marchesin <marcheu at chromium.org>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 130 +++++++++++---------
 1 file changed, 69 insertions(+), 61 deletions(-)

Comments

Doug Anderson Jan. 26, 2022, 3:54 p.m. UTC | #1
Hi,

On Wed, Jan 26, 2022 at 6:58 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> From: Douglas Anderson <dianders at chromium.org>
>
> The previous tables for mpll_cfg and curr_ctrl were created using the
> 20-pages of example settings provided by the PHY vendor.  Those
> example settings weren't particularly dense, so there were places
> where we were guessing what the settings would be for 10-bit and
> 12-bit (not that we use those anyway).  It was also always a lot of
> extra work every time we wanted to add a new clock rate since we had
> to cross-reference several tables.
>
> In <http://crosreview.com/285855> I've gone through the work to figure

The `crosreview.com` syntax doesn't seem to work anymore. Could maybe
update to https://crrev.com/c/285855 ?

> out how to generate this table automatically.  Let's now use the
> automatically generated table and then we'll never need to look at it
> again.
>
> We only support 8-bit mode right now and only support a small number
> of clock rates and and I've verified that the only 8-bit rate that was
> affected was 148.5.  That mode appears to have been wrong in the old
> table.
>
> Changes since v3:
> - new patch
>
> Signed-off-by: Douglas Anderson <dianders at chromium.org>
> Signed-off-by: Yakir Yang <ykk at rock-chips.com>

Should probably change the "at" to "@" ?

> Reviewed-by: Stéphane Marchesin <marcheu at chromium.org>

In general shouldn't carry downstream reviews when posting upstream
unless you're certain that the person intended it...


It's been a long time, but in general I think I was fairly confident
in the numbers that my script pumped out, at least given the caveat of
no pixel repetition and 8-bit only. That being said, I didn't have any
inside knowledge of the hardware and just figured out formulas that
seemed to match the table that I had. YMMV.

I'll also say that when I did a rebase of veyron (rk3288-based
Chromebook) to 4.19 about 2.5 years ago, I ended up squashing several
of these patches into 1. That can be found at
<https://crrev.com/c/1661056>. That also has details about why some of
these patches never made it upstream. The main reason, at least in the
case of rk3288, was the PLL sharing problem that nobody ever solved.
rk3288 didn't have quite enough PLLs and thus, if you were using both
VOPs, one of the VOPs was going to be severely limited in what pixel
clocks it could make. There was no framework deciding which VOP should
be limited and how the system should react to this...


In any case, I'm pretty far disconnected from all this stuff now, but
I wish you the best of luck in trying to get it all solved!

-Doug
Sascha Hauer Jan. 27, 2022, 11:20 a.m. UTC | #2
On Wed, Jan 26, 2022 at 07:54:53AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jan 26, 2022 at 6:58 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > From: Douglas Anderson <dianders at chromium.org>
> >
> > The previous tables for mpll_cfg and curr_ctrl were created using the
> > 20-pages of example settings provided by the PHY vendor.  Those
> > example settings weren't particularly dense, so there were places
> > where we were guessing what the settings would be for 10-bit and
> > 12-bit (not that we use those anyway).  It was also always a lot of
> > extra work every time we wanted to add a new clock rate since we had
> > to cross-reference several tables.
> >
> > In <http://crosreview.com/285855> I've gone through the work to figure
> 
> The `crosreview.com` syntax doesn't seem to work anymore. Could maybe
> update to https://crrev.com/c/285855 ?

did that, thanks.

> 
> > out how to generate this table automatically.  Let's now use the
> > automatically generated table and then we'll never need to look at it
> > again.
> >
> > We only support 8-bit mode right now and only support a small number
> > of clock rates and and I've verified that the only 8-bit rate that was
> > affected was 148.5.  That mode appears to have been wrong in the old
> > table.
> >
> > Changes since v3:
> > - new patch
> >
> > Signed-off-by: Douglas Anderson <dianders at chromium.org>
> > Signed-off-by: Yakir Yang <ykk at rock-chips.com>
> 
> Should probably change the "at" to "@" ?

yes.

> 
> > Reviewed-by: Stéphane Marchesin <marcheu at chromium.org>
> 
> In general shouldn't carry downstream reviews when posting upstream
> unless you're certain that the person intended it...
> 
> 
> It's been a long time, but in general I think I was fairly confident
> in the numbers that my script pumped out, at least given the caveat of
> no pixel repetition and 8-bit only. That being said, I didn't have any
> inside knowledge of the hardware and just figured out formulas that
> seemed to match the table that I had. YMMV.

Rockchip adopted this patch for their downstream Kernel, so they haven't
been able to come up with better values. Given that you created this
patch for a fairly old SoC and the values are still usable on rk3568 I
think that's the way to go.

> 
> I'll also say that when I did a rebase of veyron (rk3288-based
> Chromebook) to 4.19 about 2.5 years ago, I ended up squashing several
> of these patches into 1. That can be found at
> <https://crrev.com/c/1661056>. That also has details about why some of
> these patches never made it upstream. The main reason, at least in the
> case of rk3288, was the PLL sharing problem that nobody ever solved.
> rk3288 didn't have quite enough PLLs and thus, if you were using both
> VOPs, one of the VOPs was going to be severely limited in what pixel
> clocks it could make. There was no framework deciding which VOP should
> be limited and how the system should react to this...
> 
> 
> In any case, I'm pretty far disconnected from all this stuff now, but
> I wish you the best of luck in trying to get it all solved!

Nah, sorry, I won't get that all solved ;)

I am not that interested in rk3288, my main concern is the rk3568. That
one has two spare PLLs for three heads instead of one PLL for two heads,
so the problem is less pressing.

I'll stick to this version of the patch instead of the combined one
because this patch seems to be correct regardless of the PLL problem.

Thanks for the link though, it gives me some insights why things are the
way they are currently.

Sascha
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index b9928e622adf..160107b333ef 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -87,80 +87,88 @@  struct rockchip_hdmi {
 
 static const struct dw_hdmi_mpll_config rockchip_mpll_cfg[] = {
 	{
-		27000000, {
-			{ 0x00b3, 0x0000},
-			{ 0x2153, 0x0000},
-			{ 0x40f3, 0x0000}
+		30666000, {
+			{ 0x00b3, 0x0000 },
+			{ 0x2153, 0x0000 },
+			{ 0x40f3, 0x0000 },
 		},
-	}, {
-		36000000, {
-			{ 0x00b3, 0x0000},
-			{ 0x2153, 0x0000},
-			{ 0x40f3, 0x0000}
+	},  {
+		36800000, {
+			{ 0x00b3, 0x0000 },
+			{ 0x2153, 0x0000 },
+			{ 0x40a2, 0x0001 },
 		},
-	}, {
-		40000000, {
-			{ 0x00b3, 0x0000},
-			{ 0x2153, 0x0000},
-			{ 0x40f3, 0x0000}
+	},  {
+		46000000, {
+			{ 0x00b3, 0x0000 },
+			{ 0x2142, 0x0001 },
+			{ 0x40a2, 0x0001 },
 		},
-	}, {
-		54000000, {
-			{ 0x0072, 0x0001},
-			{ 0x2142, 0x0001},
-			{ 0x40a2, 0x0001},
+	},  {
+		61333000, {
+			{ 0x0072, 0x0001 },
+			{ 0x2142, 0x0001 },
+			{ 0x40a2, 0x0001 },
 		},
-	}, {
-		65000000, {
-			{ 0x0072, 0x0001},
-			{ 0x2142, 0x0001},
-			{ 0x40a2, 0x0001},
+	},  {
+		73600000, {
+			{ 0x0072, 0x0001 },
+			{ 0x2142, 0x0001 },
+			{ 0x4061, 0x0002 },
 		},
-	}, {
-		66000000, {
-			{ 0x013e, 0x0003},
-			{ 0x217e, 0x0002},
-			{ 0x4061, 0x0002}
+	},  {
+		92000000, {
+			{ 0x0072, 0x0001 },
+			{ 0x2145, 0x0002 },
+			{ 0x4061, 0x0002 },
 		},
-	}, {
-		74250000, {
-			{ 0x0072, 0x0001},
-			{ 0x2145, 0x0002},
-			{ 0x4061, 0x0002}
+	},  {
+		122666000, {
+			{ 0x0051, 0x0002 },
+			{ 0x2145, 0x0002 },
+			{ 0x4061, 0x0002 },
 		},
-	}, {
-		83500000, {
-			{ 0x0072, 0x0001},
+	},  {
+		147200000, {
+			{ 0x0051, 0x0002 },
+			{ 0x2145, 0x0002 },
+			{ 0x4064, 0x0003 },
 		},
-	}, {
-		108000000, {
-			{ 0x0051, 0x0002},
-			{ 0x2145, 0x0002},
-			{ 0x4061, 0x0002}
+	},  {
+		184000000, {
+			{ 0x0051, 0x0002 },
+			{ 0x214c, 0x0003 },
+			{ 0x4064, 0x0003 },
 		},
-	}, {
-		106500000, {
-			{ 0x0051, 0x0002},
-			{ 0x2145, 0x0002},
-			{ 0x4061, 0x0002}
+	},  {
+		226666000, {
+			{ 0x0040, 0x0003 },
+			{ 0x214c, 0x0003 },
+			{ 0x4064, 0x0003 },
 		},
-	}, {
-		146250000, {
-			{ 0x0051, 0x0002},
-			{ 0x2145, 0x0002},
-			{ 0x4061, 0x0002}
+	},  {
+		272000000, {
+			{ 0x0040, 0x0003 },
+			{ 0x214c, 0x0003 },
+			{ 0x5a64, 0x0003 },
 		},
-	}, {
-		148500000, {
-			{ 0x0051, 0x0003},
-			{ 0x214c, 0x0003},
-			{ 0x4064, 0x0003}
+	},  {
+		340000000, {
+			{ 0x0040, 0x0003 },
+			{ 0x3b4c, 0x0003 },
+			{ 0x5a64, 0x0003 },
 		},
-	}, {
+	},  {
+		600000000, {
+			{ 0x1a40, 0x0003 },
+			{ 0x3b4c, 0x0003 },
+			{ 0x5a64, 0x0003 },
+		},
+	},  {
 		~0UL, {
-			{ 0x00a0, 0x000a },
-			{ 0x2001, 0x000f },
-			{ 0x4002, 0x000f },
+			{ 0x0000, 0x0000 },
+			{ 0x0000, 0x0000 },
+			{ 0x0000, 0x0000 },
 		},
 	}
 };