Split the PLL setup code into computation and update functions; compute the PLL values during atomic checks, update the PLL during atomic commits; cleanup the whole thing.
The current PLL setup code for mgag200 mixes up computation if the PLL values and programming the HW. Both is done during atomic commits. As the computation phase can fail, the patch splits the functions and moves the computation to atomic-check phase. The PLL values are stores as part of the CRTC's atomic state.
As the PLL code is currently unmaintainable, apply various cleanups. For example, split functions that handle multiple HW revisions, constify values, move compute and update code to distict locations, and unify the representation of the PLL's values.
Tested on G200EH by setting modes in Weston, fbdev, and Xorg. Further testing is welcome.
Thomas Zimmermann (12): drm/mgag200: Select clock in PLL update functions drm/mgag200: Return errno codes from PLL compute functions drm/mgag200: Remove P_ARRAY_SIZE drm/mgag200: Split PLL setup into compute and update functions drm/mgag200: Introduce separate variable for PLL S parameter drm/mgag200: Store values (not bits) in struct mgag200_pll_values drm/mgag200: Split several PLL functions by device type drm/mgag200: Separate PLL compute and update functions from each other drm/mgag200: Split PLL computation for G200SE drm/mgag200: Declare PLL clock constants static const drm/mgag200: Introduce custom CRTC state drm/mgag200: Compute PLL values during atomic check
drivers/gpu/drm/drm_simple_kms_helper.c | 39 +- drivers/gpu/drm/mgag200/mgag200_drv.h | 28 + drivers/gpu/drm/mgag200/mgag200_mode.c | 965 +++++++++++++++--------- include/drm/drm_simple_kms_helper.h | 27 + 4 files changed, 720 insertions(+), 339 deletions(-)
-- 2.32.0
Put the clock-selection code into each of the PLL-update functions to make them select the correct pixel clock.
The pixel clock for video output was not actually set before programming the clock's values. It worked because the device had the correct clock pre-set.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O") Cc: Sam Ravnborg sam@ravnborg.org Cc: Emil Velikov emil.velikov@collabora.com Cc: Dave Airlie airlied@redhat.com Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.9+ --- drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------ 1 file changed, 37 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 3b3059f471c2..482843ebb69f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -130,6 +130,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) long ref_clk = mdev->model.g200.ref_clk; long p_clk_min = mdev->model.g200.pclk_min; long p_clk_max = mdev->model.g200.pclk_max; + u8 misc;
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock); @@ -174,6 +175,11 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n", clock, f_vco, m, n, p, s);
+ misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + WREG_DAC(MGA1064_PIX_PLLC_M, m); WREG_DAC(MGA1064_PIX_PLLC_N, n); WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3))); @@ -194,6 +200,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) unsigned int pvalues_e4[P_ARRAY_SIZE] = {16, 14, 12, 10, 8, 6, 4, 2, 1}; unsigned int fvv; unsigned int i; + u8 misc;
if (unique_rev_id <= 0x03) {
@@ -289,6 +296,11 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) return 1; }
+ misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + WREG_DAC(MGA1064_PIX_PLLC_M, m); WREG_DAC(MGA1064_PIX_PLLC_N, n); WREG_DAC(MGA1064_PIX_PLLC_P, p); @@ -312,7 +324,7 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) unsigned int computed; int i, j, tmpcount, vcount; bool pll_locked = false; - u8 tmp; + u8 tmp, misc;
m = n = p = 0;
@@ -385,6 +397,11 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) } }
+ misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + for (i = 0; i <= 32 && pll_locked == false; i++) { if (i > 0) { WREG8(MGAREG_CRTC_INDEX, 0x1e); @@ -489,7 +506,7 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) unsigned int testp, testm, testn; unsigned int p, m, n; unsigned int computed; - u8 tmp; + u8 tmp, misc;
m = n = p = 0; vcomax = 550000; @@ -522,6 +539,11 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) } }
+ misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS; @@ -583,7 +605,7 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) unsigned int p, m, n; unsigned int computed; int i, j, tmpcount, vcount; - u8 tmp; + u8 tmp, misc; bool pll_locked = false;
m = n = p = 0; @@ -654,6 +676,12 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) } } } + + misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + for (i = 0; i <= 32 && pll_locked == false; i++) { WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); @@ -714,6 +742,7 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) unsigned int p, m, n; unsigned int computed, vco; int tmp; + u8 misc;
m = n = p = 0; vcomax = 1488000; @@ -754,6 +783,11 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) } }
+ misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS; @@ -787,8 +821,6 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock) { - u8 misc; - switch(mdev->type) { case G200_PCI: case G200_AGP: @@ -808,11 +840,6 @@ static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock) return mga_g200er_set_plls(mdev, clock); }
- misc = RREG8(MGA_MISC_IN); - misc &= ~MGAREG_MISC_CLK_SEL_MASK; - misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; - WREG8(MGA_MISC_OUT, misc); - return 0; }
Hi Thomas,
On Mon, Jul 05, 2021 at 02:45:04PM +0200, Thomas Zimmermann wrote:
Put the clock-selection code into each of the PLL-update functions to make them select the correct pixel clock.
The pixel clock for video output was not actually set before programming the clock's values. It worked because the device had the correct clock pre-set.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O") Cc: Sam Ravnborg sam@ravnborg.org Cc: Emil Velikov emil.velikov@collabora.com Cc: Dave Airlie airlied@redhat.com Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.9+
drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------ 1 file changed, 37 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 3b3059f471c2..482843ebb69f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -130,6 +130,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) long ref_clk = mdev->model.g200.ref_clk; long p_clk_min = mdev->model.g200.pclk_min; long p_clk_max = mdev->model.g200.pclk_max;
u8 misc;
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock);
@@ -174,6 +175,11 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n", clock, f_vco, m, n, p, s);
- misc = RREG8(MGA_MISC_IN);
- misc &= ~MGAREG_MISC_CLK_SEL_MASK;
- misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
- WREG8(MGA_MISC_OUT, misc);
This chunk is repeated a number of times. Any good reason why this is not a small helper?
Sam
Hi
Am 09.07.21 um 20:50 schrieb Sam Ravnborg:
Hi Thomas,
On Mon, Jul 05, 2021 at 02:45:04PM +0200, Thomas Zimmermann wrote:
Put the clock-selection code into each of the PLL-update functions to make them select the correct pixel clock.
The pixel clock for video output was not actually set before programming the clock's values. It worked because the device had the correct clock pre-set.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O") Cc: Sam Ravnborg sam@ravnborg.org Cc: Emil Velikov emil.velikov@collabora.com Cc: Dave Airlie airlied@redhat.com Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.9+
drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------ 1 file changed, 37 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 3b3059f471c2..482843ebb69f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -130,6 +130,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) long ref_clk = mdev->model.g200.ref_clk; long p_clk_min = mdev->model.g200.pclk_min; long p_clk_max = mdev->model.g200.pclk_max;
u8 misc;
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock);
@@ -174,6 +175,11 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n", clock, f_vco, m, n, p, s);
- misc = RREG8(MGA_MISC_IN);
- misc &= ~MGAREG_MISC_CLK_SEL_MASK;
- misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
- WREG8(MGA_MISC_OUT, misc);
This chunk is repeated a number of times. Any good reason why this is not a small helper?
Good point. I'll make a helper from this.
Best regards Thomas
Sam
Return -EINVAL if there's no PLL configuration for the given pixel clock.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 482843ebb69f..045a20055515 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -134,7 +134,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock); - return 1; + return -EINVAL; }
if (clock < p_clk_min >> 3) @@ -293,7 +293,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
if (delta > permitteddelta) { pr_warn("PLL delta too large\n"); - return 1; + return -EINVAL; }
misc = RREG8(MGA_MISC_IN);
On Mon, Jul 05, 2021 at 02:45:05PM +0200, Thomas Zimmermann wrote:
Return -EINVAL if there's no PLL configuration for the given pixel clock.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 482843ebb69f..045a20055515 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -134,7 +134,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock);
return 1;
return -EINVAL;}
if (clock < p_clk_min >> 3)
@@ -293,7 +293,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
if (delta > permitteddelta) { pr_warn("PLL delta too large\n");
return 1;
return -EINVAL;}
misc = RREG8(MGA_MISC_IN);
The return value is ignored but I assume it makes sense in a later patch. Should mgag200_crtc_set_plls() return -EINVAL if there was no match? Today it returns 0 - which is not an error.
Sam
Hi
Am 09.07.21 um 20:53 schrieb Sam Ravnborg:
On Mon, Jul 05, 2021 at 02:45:05PM +0200, Thomas Zimmermann wrote:
Return -EINVAL if there's no PLL configuration for the given pixel clock.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 482843ebb69f..045a20055515 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -134,7 +134,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock);
return 1;
return -EINVAL;}
if (clock < p_clk_min >> 3)
@@ -293,7 +293,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
if (delta > permitteddelta) { pr_warn("PLL delta too large\n");
return 1;
return -EINVAL;}
misc = RREG8(MGA_MISC_IN);
The return value is ignored but I assume it makes sense in a later patch. Should mgag200_crtc_set_plls() return -EINVAL if there was no match? Today it returns 0 - which is not an error.
Indeed. Patch 12 moves some of this functionality into the atomic check, where it will be tested for success.
Not handling the type in the switch is actually a driver bug. I'll see if I can add a rsp error in one of the patches.
Best regards Thomas
Sam
Replace P_ARRAY_SIZE by array pre-initializing and ARRAY_SIZE(). No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 045a20055515..fa06f1994d68 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -187,17 +187,16 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) return 0; }
-#define P_ARRAY_SIZE 9 - static int mga_g200se_set_plls(struct mga_device *mdev, long clock) { + static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1}; + u32 unique_rev_id = mdev->model.g200se.unique_rev_id; unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta, permitteddelta; unsigned int testp, testm, testn; unsigned int p, m, n; unsigned int computed; - unsigned int pvalues_e4[P_ARRAY_SIZE] = {16, 14, 12, 10, 8, 6, 4, 2, 1}; unsigned int fvv; unsigned int i; u8 misc; @@ -252,7 +251,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) /* Permited delta is 0.5% as VESA Specification */ permitteddelta = clock * 5 / 1000;
- for (i = 0 ; i < P_ARRAY_SIZE ; i++) { + for (i = 0 ; i < ARRAY_SIZE(pvalues_e4); i++) { testp = pvalues_e4[i];
if ((clock * testp) > vcomax)
On Mon, Jul 05, 2021 at 02:45:06PM +0200, Thomas Zimmermann wrote:
Replace P_ARRAY_SIZE by array pre-initializing and ARRAY_SIZE(). No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Acked-by: Sam Ravnborg sam@ravnborg.org
The _set_plls() functions compute a pixel clock's PLL values and program the hardware accordingly. This happens during atomic commits.
For atomic modesetting, it's better to separate computation and programming from each other. This will allow to compute the PLL value during atomic checks and catch unsupported modes early.
Split the PLL setup into a compute and a update functions, and call them one after the other. Computed PLL values are store in struct mgag200_pll_values. No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_drv.h | 17 ++ drivers/gpu/drm/mgag200/mgag200_mode.c | 234 +++++++++++++++++++------ 2 files changed, 196 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index f7a0537c0d0a..fca3904fde0c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -110,6 +110,23 @@ #define MGAG200_MAX_FB_HEIGHT 4096 #define MGAG200_MAX_FB_WIDTH 4096
+/* + * Stores parameters for programming the PLLs + * + * Fref: reference frequency (A: 25.175 Mhz, B: 28.361, C: XX Mhz) + * Fo: output frequency + * Fvco = Fref * (N / M) + * Fo = Fvco / P + * + * S = [0..3] + */ +struct mgag200_pll_values { + unsigned int m; + unsigned int n; + unsigned int p; + unsigned int s; +}; + #define to_mga_connector(x) container_of(x, struct mga_connector, base)
struct mga_i2c_chan { diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index fa06f1994d68..961bd128fea3 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -114,7 +114,8 @@ static inline void mga_wait_busy(struct mga_device *mdev) * PLL setup */
-static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { struct drm_device *dev = &mdev->base; const int post_div_max = 7; @@ -130,7 +131,6 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) long ref_clk = mdev->model.g200.ref_clk; long p_clk_min = mdev->model.g200.pclk_min; long p_clk_max = mdev->model.g200.pclk_max; - u8 misc;
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock); @@ -175,19 +175,34 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n", clock, f_vco, m, n, p, s);
+ pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; +} + +static void mgag200_set_pixpll_g200(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp; + misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- WREG_DAC(MGA1064_PIX_PLLC_M, m); - WREG_DAC(MGA1064_PIX_PLLC_N, n); - WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3))); - - return 0; + xpixpllcm = pixpllc->m; + xpixpllcn = pixpllc->n; + xpixpllcp = pixpllc->p | (pixpllc->s << 3); + WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp); }
-static int mga_g200se_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
@@ -199,7 +214,6 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) unsigned int computed; unsigned int fvv; unsigned int i; - u8 misc;
if (unique_rev_id <= 0x03) {
@@ -295,35 +309,47 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) return -EINVAL; }
+ pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = 0; + + return 0; +} + +static void mgag200_set_pixpll_g200se(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + u32 unique_rev_id = mdev->model.g200se.unique_rev_id; + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp; + misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- WREG_DAC(MGA1064_PIX_PLLC_M, m); - WREG_DAC(MGA1064_PIX_PLLC_N, n); - WREG_DAC(MGA1064_PIX_PLLC_P, p); + xpixpllcm = pixpllc->m; + xpixpllcn = pixpllc->n; + xpixpllcp = pixpllc->p | (pixpllc->s << 3); + WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
if (unique_rev_id >= 0x04) { WREG_DAC(0x1a, 0x09); msleep(20); WREG_DAC(0x1a, 0x01); - } - - return 0; }
-static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn, testp2; unsigned int p, m, n; unsigned int computed; - int i, j, tmpcount, vcount; - bool pll_locked = false; - u8 tmp, misc;
m = n = p = 0;
@@ -396,11 +422,30 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) } }
+ pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = 0; + + return 0; +} + +static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; + int i, j, tmpcount, vcount; + bool pll_locked = false; + misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
+ xpixpllcm = pixpllc->m; + xpixpllcn = pixpllc->n; + xpixpllcp = pixpllc->p | (pixpllc->s << 3); + for (i = 0; i <= 32 && pll_locked == false; i++) { if (i > 0) { WREG8(MGAREG_CRTC_INDEX, 0x1e); @@ -441,9 +486,9 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) udelay(50);
/* program pixel pll register */ - WREG_DAC(MGA1064_WB_PIX_PLLC_N, n); - WREG_DAC(MGA1064_WB_PIX_PLLC_M, m); - WREG_DAC(MGA1064_WB_PIX_PLLC_P, p); + WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
udelay(50);
@@ -491,21 +536,21 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) udelay(5); } } + WREG8(DAC_INDEX, MGA1064_REMHEADCTL); tmp = RREG8(DAC_DATA); tmp &= ~MGA1064_REMHEADCTL_CLKDIS; WREG_DAC(MGA1064_REMHEADCTL, tmp); - return 0; }
-static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n; unsigned int computed; - u8 tmp, misc;
m = n = p = 0; vcomax = 550000; @@ -538,11 +583,28 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) } }
+ pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = 0; + + return 0; +} + +static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; + misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
+ xpixpllcm = pixpllc->m; + xpixpllcn = pixpllc->n; + xpixpllcp = pixpllc->p | (pixpllc->s << 3); + WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS; @@ -561,9 +623,9 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) tmp |= MGA1064_PIX_CLK_CTL_CLK_POW_DOWN; WREG8(DAC_DATA, tmp);
- WREG_DAC(MGA1064_EV_PIX_PLLC_M, m); - WREG_DAC(MGA1064_EV_PIX_PLLC_N, n); - WREG_DAC(MGA1064_EV_PIX_PLLC_P, p); + WREG_DAC(MGA1064_EV_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_EV_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_EV_PIX_PLLC_P, xpixpllcp);
udelay(50);
@@ -592,20 +654,16 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) tmp = RREG8(DAC_DATA); tmp &= ~MGA1064_PIX_CLK_CTL_CLK_DIS; WREG8(DAC_DATA, tmp); - - return 0; }
-static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n; unsigned int computed; - int i, j, tmpcount, vcount; - u8 tmp, misc; - bool pll_locked = false;
m = n = p = 0;
@@ -676,11 +734,30 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) } }
+ pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = 0; + + return 0; +} + +static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; + int i, j, tmpcount, vcount; + bool pll_locked = false; + misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
+ xpixpllcm = pixpllc->m; + xpixpllcn = pixpllc->n; + xpixpllcp = pixpllc->p | (pixpllc->s << 3); + for (i = 0; i <= 32 && pll_locked == false; i++) { WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); @@ -698,9 +775,9 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock)
udelay(500);
- WREG_DAC(MGA1064_EH_PIX_PLLC_M, m); - WREG_DAC(MGA1064_EH_PIX_PLLC_N, n); - WREG_DAC(MGA1064_EH_PIX_PLLC_P, p); + WREG_DAC(MGA1064_EH_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_EH_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_EH_PIX_PLLC_P, xpixpllcp);
udelay(500);
@@ -728,11 +805,10 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) udelay(5); } } - - return 0; }
-static int mga_g200er_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { static const unsigned int m_div_val[] = { 1, 2, 4, 8 }; unsigned int vcomax, vcomin, pllreffreq; @@ -740,8 +816,6 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) int testr, testn, testm, testo; unsigned int p, m, n; unsigned int computed, vco; - int tmp; - u8 misc;
m = n = p = 0; vcomax = 1488000; @@ -782,11 +856,28 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) } }
+ pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = 0; + + return 0; +} + +static void mgag200_set_pixpll_g200er(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; + misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
+ xpixpllcm = pixpllc->m; + xpixpllcn = pixpllc->n; + xpixpllcp = pixpllc->p | (pixpllc->s << 3); + WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS; @@ -809,37 +900,70 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
udelay(500);
- WREG_DAC(MGA1064_ER_PIX_PLLC_N, n); - WREG_DAC(MGA1064_ER_PIX_PLLC_M, m); - WREG_DAC(MGA1064_ER_PIX_PLLC_P, p); + WREG_DAC(MGA1064_ER_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_ER_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_ER_PIX_PLLC_P, xpixpllcp);
udelay(50); - - return 0; }
-static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock) +static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock) { + struct mgag200_pll_values pixpll; + int ret; + switch(mdev->type) { case G200_PCI: case G200_AGP: - return mgag200_g200_set_plls(mdev, clock); + ret = mgag200_compute_pixpll_values_g200(mdev, clock, &pixpll); + break; case G200_SE_A: case G200_SE_B: - return mga_g200se_set_plls(mdev, clock); + ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll); + break; case G200_WB: case G200_EW3: - return mga_g200wb_set_plls(mdev, clock); + ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll); + break; case G200_EV: - return mga_g200ev_set_plls(mdev, clock); + ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll); + break; case G200_EH: case G200_EH3: - return mga_g200eh_set_plls(mdev, clock); + ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll); + break; case G200_ER: - return mga_g200er_set_plls(mdev, clock); + ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll); + break; }
- return 0; + if (ret) + return; + + switch (mdev->type) { + case G200_PCI: + case G200_AGP: + mgag200_set_pixpll_g200(mdev, &pixpll); + break; + case G200_SE_A: + case G200_SE_B: + mgag200_set_pixpll_g200se(mdev, &pixpll); + break; + case G200_WB: + case G200_EW3: + mgag200_set_pixpll_g200wb(mdev, &pixpll); + break; + case G200_EV: + mgag200_set_pixpll_g200ev(mdev, &pixpll); + break; + case G200_EH: + case G200_EH3: + mgag200_set_pixpll_g200eh(mdev, &pixpll); + break; + case G200_ER: + mgag200_set_pixpll_g200er(mdev, &pixpll); + break; + } }
static void mgag200_g200wb_hold_bmc(struct mga_device *mdev)
Ho Thomas,
On Mon, Jul 05, 2021 at 02:45:07PM +0200, Thomas Zimmermann wrote:
The _set_plls() functions compute a pixel clock's PLL values and program the hardware accordingly. This happens during atomic commits.
For atomic modesetting, it's better to separate computation and programming from each other. This will allow to compute the PLL value during atomic checks and catch unsupported modes early.
Split the PLL setup into a compute and a update functions, and call them one after the other. Computed PLL values are store in struct mgag200_pll_values. No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/mgag200/mgag200_drv.h | 17 ++ drivers/gpu/drm/mgag200/mgag200_mode.c | 234 +++++++++++++++++++------ 2 files changed, 196 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index f7a0537c0d0a..fca3904fde0c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -110,6 +110,23 @@ #define MGAG200_MAX_FB_HEIGHT 4096 #define MGAG200_MAX_FB_WIDTH 4096
+/*
- Stores parameters for programming the PLLs
- Fref: reference frequency (A: 25.175 Mhz, B: 28.361, C: XX Mhz)
- Fo: output frequency
- Fvco = Fref * (N / M)
- Fo = Fvco / P
- S = [0..3]
- */
+struct mgag200_pll_values {
- unsigned int m;
Why not u8?
- unsigned int n;
Why not u8?
- unsigned int p;
Why not u8?
- unsigned int s;
+};
#define to_mga_connector(x) container_of(x, struct mga_connector, base)
struct mga_i2c_chan { diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index fa06f1994d68..961bd128fea3 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -114,7 +114,8 @@ static inline void mga_wait_busy(struct mga_device *mdev)
- PLL setup
*/
-static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc){ struct drm_device *dev = &mdev->base; const int post_div_max = 7; @@ -130,7 +131,6 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) long ref_clk = mdev->model.g200.ref_clk; long p_clk_min = mdev->model.g200.pclk_min; long p_clk_max = mdev->model.g200.pclk_max;
u8 misc;
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock);
@@ -175,19 +175,34 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n", clock, f_vco, m, n, p, s);
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = s;
- return 0;
+}
+static void mgag200_set_pixpll_g200(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
- u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
Strange names, but whatever.
- misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- WREG_DAC(MGA1064_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
- return 0;
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
- WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm);
- WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn);
- WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
I cannot see why this code does not look like this: WREG_DAC(MGA1064_PIX_PLLC_M, pixpllc->m); WREG_DAC(MGA1064_PIX_PLLC_N, pixpllc->n); WREG_DAC(MGA1064_PIX_PLLC_P, pixpllc->p | (pixpllc->s << 3));
Same goes for all the functions in the following.
}
-static int mga_g200se_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc){ static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
@@ -199,7 +214,6 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) unsigned int computed; unsigned int fvv; unsigned int i;
u8 misc;
if (unique_rev_id <= 0x03) {
@@ -295,35 +309,47 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) return -EINVAL; }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200se(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
- u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
- u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
- misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- WREG_DAC(MGA1064_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_PIX_PLLC_P, p);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
The "(pixpllc->s << 3)" look like a copy error. No harm as s is 0 but confusing.
WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm);
WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn);
WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
if (unique_rev_id >= 0x04) { WREG_DAC(0x1a, 0x09); msleep(20); WREG_DAC(0x1a, 0x01);
- }
- return 0;
}
-static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc){ unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn, testp2; unsigned int p, m, n; unsigned int computed;
int i, j, tmpcount, vcount;
bool pll_locked = false;
u8 tmp, misc;
m = n = p = 0;
@@ -396,11 +422,30 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200wb(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
int i, j, tmpcount, vcount;
bool pll_locked = false;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
The (pixpllc->s << 3) looks wrong here too.
- for (i = 0; i <= 32 && pll_locked == false; i++) { if (i > 0) { WREG8(MGAREG_CRTC_INDEX, 0x1e);
@@ -441,9 +486,9 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) udelay(50);
/* program pixel pll register */
WREG_DAC(MGA1064_WB_PIX_PLLC_N, n);WREG_DAC(MGA1064_WB_PIX_PLLC_M, m);WREG_DAC(MGA1064_WB_PIX_PLLC_P, p);
WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn);WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm);WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);udelay(50);
@@ -491,21 +536,21 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) udelay(5); } }
- WREG8(DAC_INDEX, MGA1064_REMHEADCTL); tmp = RREG8(DAC_DATA); tmp &= ~MGA1064_REMHEADCTL_CLKDIS; WREG_DAC(MGA1064_REMHEADCTL, tmp);
- return 0;
}
-static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc){ unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n; unsigned int computed;
u8 tmp, misc;
m = n = p = 0; vcomax = 550000;
@@ -538,11 +583,28 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200ev(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -561,9 +623,9 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) tmp |= MGA1064_PIX_CLK_CTL_CLK_POW_DOWN; WREG8(DAC_DATA, tmp);
- WREG_DAC(MGA1064_EV_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_EV_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_EV_PIX_PLLC_P, p);
WREG_DAC(MGA1064_EV_PIX_PLLC_M, xpixpllcm);
WREG_DAC(MGA1064_EV_PIX_PLLC_N, xpixpllcn);
WREG_DAC(MGA1064_EV_PIX_PLLC_P, xpixpllcp);
udelay(50);
@@ -592,20 +654,16 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) tmp = RREG8(DAC_DATA); tmp &= ~MGA1064_PIX_CLK_CTL_CLK_DIS; WREG8(DAC_DATA, tmp);
- return 0;
}
-static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc){ unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n; unsigned int computed;
int i, j, tmpcount, vcount;
u8 tmp, misc;
bool pll_locked = false;
m = n = p = 0;
@@ -676,11 +734,30 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200eh(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
int i, j, tmpcount, vcount;
bool pll_locked = false;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
Again (pixpllc->s << 3)
- for (i = 0; i <= 32 && pll_locked == false; i++) { WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA);
@@ -698,9 +775,9 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock)
udelay(500);
WREG_DAC(MGA1064_EH_PIX_PLLC_M, m);WREG_DAC(MGA1064_EH_PIX_PLLC_N, n);WREG_DAC(MGA1064_EH_PIX_PLLC_P, p);
WREG_DAC(MGA1064_EH_PIX_PLLC_M, xpixpllcm);WREG_DAC(MGA1064_EH_PIX_PLLC_N, xpixpllcn);WREG_DAC(MGA1064_EH_PIX_PLLC_P, xpixpllcp);udelay(500);
@@ -728,11 +805,10 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) udelay(5); } }
- return 0;
}
-static int mga_g200er_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc){ static const unsigned int m_div_val[] = { 1, 2, 4, 8 }; unsigned int vcomax, vcomin, pllreffreq; @@ -740,8 +816,6 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) int testr, testn, testm, testo; unsigned int p, m, n; unsigned int computed, vco;
int tmp;
u8 misc;
m = n = p = 0; vcomax = 1488000;
@@ -782,11 +856,28 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200er(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
Again (pixpllc->s << 3)
- WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -809,37 +900,70 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
udelay(500);
- WREG_DAC(MGA1064_ER_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_ER_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_ER_PIX_PLLC_P, p);
WREG_DAC(MGA1064_ER_PIX_PLLC_N, xpixpllcn);
WREG_DAC(MGA1064_ER_PIX_PLLC_M, xpixpllcm);
WREG_DAC(MGA1064_ER_PIX_PLLC_P, xpixpllcp);
udelay(50);
- return 0;
}
-static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock) +static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock) {
Makes sense, you can forget my earlier comment about return 0 in this function.
- struct mgag200_pll_values pixpll;
- int ret;
- switch(mdev->type) { case G200_PCI: case G200_AGP:
return mgag200_g200_set_plls(mdev, clock);
ret = mgag200_compute_pixpll_values_g200(mdev, clock, &pixpll);break;
return mga_g200se_set_plls(mdev, clock);
ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll);break;
return mga_g200wb_set_plls(mdev, clock);
ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll);break;
return mga_g200ev_set_plls(mdev, clock);
ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll);break;
return mga_g200eh_set_plls(mdev, clock);
ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll);break;
return mga_g200er_set_plls(mdev, clock);
ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll);break;
- return 0;
- if (ret)
return;- switch (mdev->type) {
- case G200_PCI:
- case G200_AGP:
mgag200_set_pixpll_g200(mdev, &pixpll);break;- case G200_SE_A:
- case G200_SE_B:
mgag200_set_pixpll_g200se(mdev, &pixpll);break;- case G200_WB:
- case G200_EW3:
mgag200_set_pixpll_g200wb(mdev, &pixpll);break;- case G200_EV:
mgag200_set_pixpll_g200ev(mdev, &pixpll);break;- case G200_EH:
- case G200_EH3:
mgag200_set_pixpll_g200eh(mdev, &pixpll);break;- case G200_ER:
mgag200_set_pixpll_g200er(mdev, &pixpll);break;- }
}
static void mgag200_g200wb_hold_bmc(struct mga_device *mdev)
2.32.0
Hi
Am 09.07.21 um 21:12 schrieb Sam Ravnborg:
Ho Thomas,
On Mon, Jul 05, 2021 at 02:45:07PM +0200, Thomas Zimmermann wrote:
The _set_plls() functions compute a pixel clock's PLL values and program the hardware accordingly. This happens during atomic commits.
For atomic modesetting, it's better to separate computation and programming from each other. This will allow to compute the PLL value during atomic checks and catch unsupported modes early.
Split the PLL setup into a compute and a update functions, and call them one after the other. Computed PLL values are store in struct mgag200_pll_values. No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/mgag200/mgag200_drv.h | 17 ++ drivers/gpu/drm/mgag200/mgag200_mode.c | 234 +++++++++++++++++++------ 2 files changed, 196 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index f7a0537c0d0a..fca3904fde0c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -110,6 +110,23 @@ #define MGAG200_MAX_FB_HEIGHT 4096 #define MGAG200_MAX_FB_WIDTH 4096
+/*
- Stores parameters for programming the PLLs
- Fref: reference frequency (A: 25.175 Mhz, B: 28.361, C: XX Mhz)
- Fo: output frequency
- Fvco = Fref * (N / M)
- Fo = Fvco / P
- S = [0..3]
- */
+struct mgag200_pll_values {
- unsigned int m;
Why not u8?
- unsigned int n;
Why not u8?
- unsigned int p;
Why not u8?
- unsigned int s;
+};
First of all, thanks for going through these changes. The current code is nightmarish. So patch 4 and later is where the fun happens. :)
These fields are supposed to be values as used by the function that controls the PLL's output frequency; so they are unsigned int. u8 would be for in-register values; which can be different.
The MGA manual explains how to program this.
https://manualzz.com/viewer_next/web/manual?file=%2F%2Fs3p.manualzz.com%2Fst...
Apparently, there's one way of doing this. And still each hardware revision manages to require it's own code.
#define to_mga_connector(x) container_of(x, struct mga_connector, base)
struct mga_i2c_chan {
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index fa06f1994d68..961bd128fea3 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -114,7 +114,8 @@ static inline void mga_wait_busy(struct mga_device *mdev)
- PLL setup
*/
-static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc)@@ -130,7 +131,6 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) long ref_clk = mdev->model.g200.ref_clk; long p_clk_min = mdev->model.g200.pclk_min; long p_clk_max = mdev->model.g200.pclk_max;
u8 misc;
if (clock > p_clk_max) { drm_err(dev, "Pixel Clock %ld too high\n", clock);
@@ -175,19 +175,34 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock) drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n", clock, f_vco, m, n, p, s);
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = s;
- return 0;
+}
+static void mgag200_set_pixpll_g200(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
- u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
Strange names, but whatever.
Those are the names of the registers; as used in the MGA manual. I try to use official names wherever possible. Others can then read the code and grep the register manual for the names.
Note that xpixpllcp stores p and s; so it's not a 1-1 relationship with the values in struct mageg200_pll_values.
- misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- WREG_DAC(MGA1064_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
- return 0;
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
- WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm);
- WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn);
- WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
I cannot see why this code does not look like this: WREG_DAC(MGA1064_PIX_PLLC_M, pixpllc->m); WREG_DAC(MGA1064_PIX_PLLC_N, pixpllc->n); WREG_DAC(MGA1064_PIX_PLLC_P, pixpllc->p | (pixpllc->s << 3));
Readability for the reviewer. :) And I usually prefer to compute a register's value in one statement, and read/write the register in separate statements. I hope this isn't too much of an issue.
Same goes for all the functions in the following.
}
-static int mga_g200se_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc)@@ -199,7 +214,6 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) unsigned int computed; unsigned int fvv; unsigned int i;
u8 misc;
if (unique_rev_id <= 0x03) {
@@ -295,35 +309,47 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock) return -EINVAL; }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200se(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
- u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
- u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
- misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- WREG_DAC(MGA1064_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_PIX_PLLC_P, p);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
The "(pixpllc->s << 3)" look like a copy error. No harm as s is 0 but confusing.
I mentioned this above. xpixpllcp stores both, p and s. Here and below.
WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm);
WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn);
WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
if (unique_rev_id >= 0x04) { WREG_DAC(0x1a, 0x09); msleep(20); WREG_DAC(0x1a, 0x01);
- }
- return 0; }
-static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc)
int i, j, tmpcount, vcount;
bool pll_locked = false;
u8 tmp, misc;
m = n = p = 0;
@@ -396,11 +422,30 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200wb(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
int i, j, tmpcount, vcount;
bool pll_locked = false;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
The (pixpllc->s << 3) looks wrong here too.
- for (i = 0; i <= 32 && pll_locked == false; i++) { if (i > 0) { WREG8(MGAREG_CRTC_INDEX, 0x1e);
@@ -441,9 +486,9 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) udelay(50);
/* program pixel pll register */
WREG_DAC(MGA1064_WB_PIX_PLLC_N, n);WREG_DAC(MGA1064_WB_PIX_PLLC_M, m);WREG_DAC(MGA1064_WB_PIX_PLLC_P, p);
WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn);WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm);WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);udelay(50);
@@ -491,21 +536,21 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock) udelay(5); } }
- WREG8(DAC_INDEX, MGA1064_REMHEADCTL); tmp = RREG8(DAC_DATA); tmp &= ~MGA1064_REMHEADCTL_CLKDIS; WREG_DAC(MGA1064_REMHEADCTL, tmp);
- return 0; }
-static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc)
u8 tmp, misc;
m = n = p = 0; vcomax = 550000;
@@ -538,11 +583,28 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200ev(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -561,9 +623,9 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) tmp |= MGA1064_PIX_CLK_CTL_CLK_POW_DOWN; WREG8(DAC_DATA, tmp);
- WREG_DAC(MGA1064_EV_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_EV_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_EV_PIX_PLLC_P, p);
WREG_DAC(MGA1064_EV_PIX_PLLC_M, xpixpllcm);
WREG_DAC(MGA1064_EV_PIX_PLLC_N, xpixpllcn);
WREG_DAC(MGA1064_EV_PIX_PLLC_P, xpixpllcp);
udelay(50);
@@ -592,20 +654,16 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock) tmp = RREG8(DAC_DATA); tmp &= ~MGA1064_PIX_CLK_CTL_CLK_DIS; WREG8(DAC_DATA, tmp);
- return 0; }
-static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc)
int i, j, tmpcount, vcount;
u8 tmp, misc;
bool pll_locked = false;
m = n = p = 0;
@@ -676,11 +734,30 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200eh(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
int i, j, tmpcount, vcount;
bool pll_locked = false;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
Again (pixpllc->s << 3)
- for (i = 0; i <= 32 && pll_locked == false; i++) { WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA);
@@ -698,9 +775,9 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock)
udelay(500);
WREG_DAC(MGA1064_EH_PIX_PLLC_M, m);WREG_DAC(MGA1064_EH_PIX_PLLC_N, n);WREG_DAC(MGA1064_EH_PIX_PLLC_P, p);
WREG_DAC(MGA1064_EH_PIX_PLLC_M, xpixpllcm);WREG_DAC(MGA1064_EH_PIX_PLLC_N, xpixpllcn);WREG_DAC(MGA1064_EH_PIX_PLLC_P, xpixpllcp);udelay(500);
@@ -728,11 +805,10 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock) udelay(5); } }
- return 0; }
-static int mga_g200er_set_plls(struct mga_device *mdev, long clock) +static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long clock,
struct mgag200_pll_values *pixpllc)@@ -740,8 +816,6 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) int testr, testn, testm, testo; unsigned int p, m, n; unsigned int computed, vco;
int tmp;
u8 misc;
m = n = p = 0; vcomax = 1488000;
@@ -782,11 +856,28 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) } }
- pixpllc->m = m;
- pixpllc->n = n;
- pixpllc->p = p;
- pixpllc->s = 0;
- return 0;
+}
+static void mgag200_set_pixpll_g200er(struct mga_device *mdev,
const struct mgag200_pll_values *pixpllc)+{
u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN); misc &= ~MGAREG_MISC_CLK_SEL_MASK; misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
xpixpllcm = pixpllc->m;
xpixpllcn = pixpllc->n;
xpixpllcp = pixpllc->p | (pixpllc->s << 3);
Again (pixpllc->s << 3)
- WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -809,37 +900,70 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
udelay(500);
- WREG_DAC(MGA1064_ER_PIX_PLLC_N, n);
- WREG_DAC(MGA1064_ER_PIX_PLLC_M, m);
- WREG_DAC(MGA1064_ER_PIX_PLLC_P, p);
WREG_DAC(MGA1064_ER_PIX_PLLC_N, xpixpllcn);
WREG_DAC(MGA1064_ER_PIX_PLLC_M, xpixpllcm);
WREG_DAC(MGA1064_ER_PIX_PLLC_P, xpixpllcp);
udelay(50);
- return 0; }
-static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock) +static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock) {
Makes sense, you can forget my earlier comment about return 0 in this function.
Yes. The tests are in the compute functions, which will be allowed to fail during atomic check.
Best regards Thomas
The S parameter is controls the loop filter bandwidth when programming the PLL. It's currently stored as part of P (i.e., the clock divider.) Add a separate variable for S prepares the PLL code for a further refactoring.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 36 ++++++++++++-------------- 1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 961bd128fea3..9f6fe7673674 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -210,18 +210,17 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta, permitteddelta; unsigned int testp, testm, testn; - unsigned int p, m, n; + unsigned int p, m, n, s; unsigned int computed; unsigned int fvv; unsigned int i;
- if (unique_rev_id <= 0x03) { + m = n = p = s = 0;
- m = n = p = 0; + if (unique_rev_id <= 0x03) { vcomax = 320000; vcomin = 160000; pllreffreq = 25000; - delta = 0xffffffff; permitteddelta = clock * 5 / 1000;
@@ -249,9 +248,6 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl } } } else { - - - m = n = p = 0; vcomax = 1600000; vcomin = 800000; pllreffreq = 25000; @@ -312,7 +308,7 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl pixpllc->m = m; pixpllc->n = n; pixpllc->p = p; - pixpllc->s = 0; + pixpllc->s = s;
return 0; } @@ -348,10 +344,10 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn, testp2; - unsigned int p, m, n; + unsigned int p, m, n, s; unsigned int computed;
- m = n = p = 0; + m = n = p = s = 0;
delta = 0xffffffff;
@@ -425,7 +421,7 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl pixpllc->m = m; pixpllc->n = n; pixpllc->p = p; - pixpllc->s = 0; + pixpllc->s = s;
return 0; } @@ -549,10 +545,10 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn; - unsigned int p, m, n; + unsigned int p, m, n, s; unsigned int computed;
- m = n = p = 0; + m = n = p = s = 0; vcomax = 550000; vcomin = 150000; pllreffreq = 50000; @@ -586,7 +582,7 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl pixpllc->m = m; pixpllc->n = n; pixpllc->p = p; - pixpllc->s = 0; + pixpllc->s = s;
return 0; } @@ -662,10 +658,10 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; unsigned int testp, testm, testn; - unsigned int p, m, n; + unsigned int p, m, n, s; unsigned int computed;
- m = n = p = 0; + m = n = p = s = 0;
if (mdev->type == G200_EH3) { vcomax = 3000000; @@ -737,7 +733,7 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl pixpllc->m = m; pixpllc->n = n; pixpllc->p = p; - pixpllc->s = 0; + pixpllc->s = s;
return 0; } @@ -814,10 +810,10 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; int testr, testn, testm, testo; - unsigned int p, m, n; + unsigned int p, m, n, s; unsigned int computed, vco;
- m = n = p = 0; + m = n = p = s = 0; vcomax = 1488000; vcomin = 1056000; pllreffreq = 48000; @@ -859,7 +855,7 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl pixpllc->m = m; pixpllc->n = n; pixpllc->p = p; - pixpllc->s = 0; + pixpllc->s = s;
return 0; }
On Mon, Jul 05, 2021 at 02:45:08PM +0200, Thomas Zimmermann wrote:
The S parameter is controls the loop filter bandwidth when programming the PLL. It's currently stored as part of P (i.e., the clock divider.) Add a separate variable for S prepares the PLL code for a further refactoring.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
I guess I will soon learn why they all had s << 3.
This patch is Acked-by: Sam Ravnborg sam@ravnborg.org
The fields in struct mgag200_pll_values currently hold the bits of each register. Store the PLL values instead and let the PLL-update code figure out the bits for each register.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 153 +++++++++++++++---------- 1 file changed, 91 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 9f6fe7673674..7d6707bd6c27 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -123,7 +123,7 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc const int in_div_max = 6; const int feed_div_min = 7; const int feed_div_max = 127; - u8 testm, testn; + u8 testp, testm, testn; u8 n = 0, m = 0, p, s; long f_vco; long computed; @@ -141,10 +141,11 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc clock = p_clk_min >> 3;
f_vco = clock; - for (p = 0; - p <= post_div_max && f_vco < p_clk_min; - p = (p << 1) + 1, f_vco <<= 1) + for (testp = 0; + testp <= post_div_max && f_vco < p_clk_min; + testp = (testp << 1) + 1, f_vco <<= 1) ; + p = testp + 1;
delta = clock;
@@ -157,12 +158,12 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc tmp_delta = computed - f_vco; if (tmp_delta < delta) { delta = tmp_delta; - m = testm; - n = testn; + m = testm + 1; + n = testn + 1; } } } - f_vco = ref_clk * (n + 1) / (m + 1); + f_vco = ref_clk * n / m; if (f_vco < 100000) s = 0; else if (f_vco < 140000) @@ -186,6 +187,7 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc static void mgag200_set_pixpll_g200(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
misc = RREG8(MGA_MISC_IN); @@ -193,9 +195,14 @@ static void mgag200_set_pixpll_g200(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m; - xpixpllcn = pixpllc->n; - xpixpllcp = pixpllc->p | (pixpllc->s << 3); + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = pixpllcm; + xpixpllcn = pixpllcn; + xpixpllcp = (pixpllcs << 3) | pixpllcp; WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp); @@ -240,9 +247,9 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - m = testm - 1; - n = testn - 1; - p = testp - 1; + m = testm; + n = testn; + p = testp; } } } @@ -280,22 +287,19 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
if (tmpdelta < delta) { delta = tmpdelta; - m = testm - 1; - n = testn - 1; - p = testp - 1; + m = testm; + n = testn; + p = testp; } } } }
- fvv = pllreffreq * (n + 1) / (m + 1); + fvv = pllreffreq * n / m; fvv = (fvv - 800000) / 50000; - if (fvv > 15) fvv = 15; - - p |= (fvv << 4); - m |= 0x80; + s = fvv << 1;
clock = clock / 2; } @@ -317,6 +321,7 @@ static void mgag200_set_pixpll_g200se(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { u32 unique_rev_id = mdev->model.g200se.unique_rev_id; + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
misc = RREG8(MGA_MISC_IN); @@ -324,9 +329,14 @@ static void mgag200_set_pixpll_g200se(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m; - xpixpllcn = pixpllc->n; - xpixpllcp = pixpllc->p | (pixpllc->s << 3); + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = pixpllcm | ((pixpllcn & BIT(8)) >> 1); + xpixpllcn = pixpllcn; + xpixpllcp = (pixpllcs << 3) | pixpllcp; WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp); @@ -352,7 +362,6 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl delta = 0xffffffff;
if (mdev->type == G200_EW3) { - vcomax = 800000; vcomin = 400000; pllreffreq = 25000; @@ -375,19 +384,16 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - m = ((testn & 0x100) >> 1) | - (testm); - n = (testn & 0xFF); - p = ((testn & 0x600) >> 3) | - (testp2 << 3) | - (testp); + m = testm + 1; + n = testn + 1; + p = testp + 1; + s = testp2; } } } } } } else { - vcomax = 550000; vcomin = 150000; pllreffreq = 48000; @@ -408,10 +414,10 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - n = testn - 1; - m = (testm - 1) | - ((n >> 1) & 0x80); - p = testp - 1; + n = testn; + m = testm; + p = testp; + s = 0; } } } @@ -429,6 +435,7 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; int i, j, tmpcount, vcount; bool pll_locked = false; @@ -438,9 +445,14 @@ static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m; - xpixpllcn = pixpllc->n; - xpixpllcp = pixpllc->p | (pixpllc->s << 3); + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = ((pixpllcn & BIT(8)) >> 1) | pixpllcm; + xpixpllcn = pixpllcn; + xpixpllcp = ((pixpllcn & GENMASK(10, 9)) >> 3) | (pixpllcs << 3) | pixpllcp;
for (i = 0; i <= 32 && pll_locked == false; i++) { if (i > 0) { @@ -571,9 +583,9 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - n = testn - 1; - m = testm - 1; - p = testp - 1; + n = testn; + m = testm; + p = testp; } } } @@ -590,6 +602,7 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN); @@ -597,9 +610,14 @@ static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m; - xpixpllcn = pixpllc->n; - xpixpllcp = pixpllc->p | (pixpllc->s << 3); + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = pixpllcm; + xpixpllcn = pixpllcn; + xpixpllcp = (pixpllcs << 3) | pixpllcp;
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA); @@ -685,9 +703,9 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - n = testn; - m = testm; - p = testp; + n = testn + 1; + m = testm + 1; + p = testp + 1; } if (delta == 0) break; @@ -719,12 +737,10 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - n = testn - 1; - m = (testm - 1); - p = testp - 1; + n = testn; + m = testm; + p = testp; } - if ((clock * testp) >= 600000) - p |= 0x80; } } } @@ -741,6 +757,7 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; int i, j, tmpcount, vcount; bool pll_locked = false; @@ -750,9 +767,14 @@ static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m; - xpixpllcn = pixpllc->n; - xpixpllcp = pixpllc->p | (pixpllc->s << 3); + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = ((pixpllcn & BIT(8)) >> 1) | pixpllcm; + xpixpllcn = pixpllcn; + xpixpllcp = (pixpllcs << 3) | pixpllcp;
for (i = 0; i <= 32 && pll_locked == false; i++) { WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); @@ -843,9 +865,10 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - m = testm | (testo << 3); - n = testn; - p = testr | (testr << 3); + m = (testm | (testo << 3)) + 1; + n = testn + 1; + p = testr + 1; + s = testr; } } } @@ -863,6 +886,7 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200er(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN); @@ -870,9 +894,14 @@ static void mgag200_set_pixpll_g200er(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m; - xpixpllcn = pixpllc->n; - xpixpllcp = pixpllc->p | (pixpllc->s << 3); + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = pixpllcm; + xpixpllcn = pixpllcn; + xpixpllcp = (pixpllcs << 3) | pixpllcp;
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA);
Hi Thomas,
On Mon, Jul 05, 2021 at 02:45:09PM +0200, Thomas Zimmermann wrote:
The fields in struct mgag200_pll_values currently hold the bits of each register. Store the PLL values instead and let the PLL-update code figure out the bits for each register.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
I gave up trying to follow the changes in this patch. Also I was left with the impression that this was no win in readability at it looks to me like changes with a high risk to introduce a hard-to-find bug. Your changelog did not justify why this change is a win, only what is does. But whatever works better for you I guess..
Sam
drivers/gpu/drm/mgag200/mgag200_mode.c | 153 +++++++++++++++---------- 1 file changed, 91 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 9f6fe7673674..7d6707bd6c27 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -123,7 +123,7 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc const int in_div_max = 6; const int feed_div_min = 7; const int feed_div_max = 127;
- u8 testm, testn;
- u8 testp, testm, testn; u8 n = 0, m = 0, p, s; long f_vco; long computed;
@@ -141,10 +141,11 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc clock = p_clk_min >> 3;
f_vco = clock;
- for (p = 0;
p <= post_div_max && f_vco < p_clk_min;p = (p << 1) + 1, f_vco <<= 1)
for (testp = 0;
testp <= post_div_max && f_vco < p_clk_min;testp = (testp << 1) + 1, f_vco <<= 1);
p = testp + 1;
delta = clock;
@@ -157,12 +158,12 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc tmp_delta = computed - f_vco; if (tmp_delta < delta) { delta = tmp_delta;
m = testm;n = testn;
m = testm + 1;n = testn + 1; }
- f_vco = ref_clk * (n + 1) / (m + 1);
- f_vco = ref_clk * n / m; if (f_vco < 100000) s = 0; else if (f_vco < 140000)
@@ -186,6 +187,7 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc static void mgag200_set_pixpll_g200(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
misc = RREG8(MGA_MISC_IN);
@@ -193,9 +195,14 @@ static void mgag200_set_pixpll_g200(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
- pixpllcm = pixpllc->m - 1;
- pixpllcn = pixpllc->n - 1;
- pixpllcp = pixpllc->p - 1;
- pixpllcs = pixpllc->s;
- xpixpllcm = pixpllcm;
- xpixpllcn = pixpllcn;
- xpixpllcp = (pixpllcs << 3) | pixpllcp; WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
@@ -240,9 +247,9 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
m = testm - 1;n = testn - 1;p = testp - 1;
m = testm;n = testn;p = testp; } } }@@ -280,22 +287,19 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
if (tmpdelta < delta) { delta = tmpdelta;
m = testm - 1;n = testn - 1;p = testp - 1;
m = testm;n = testn;p = testp; } } }
fvv = pllreffreq * (n + 1) / (m + 1);
fvv = pllreffreq * n / m;
- if (fvv > 15) fvv = 15;
p |= (fvv << 4);m |= 0x80;
s = fvv << 1;clock = clock / 2; }
@@ -317,6 +321,7 @@ static void mgag200_set_pixpll_g200se(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
misc = RREG8(MGA_MISC_IN);
@@ -324,9 +329,14 @@ static void mgag200_set_pixpll_g200se(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
- pixpllcm = pixpllc->m - 1;
- pixpllcn = pixpllc->n - 1;
- pixpllcp = pixpllc->p - 1;
- pixpllcs = pixpllc->s;
- xpixpllcm = pixpllcm | ((pixpllcn & BIT(8)) >> 1);
- xpixpllcn = pixpllcn;
- xpixpllcp = (pixpllcs << 3) | pixpllcp; WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
@@ -352,7 +362,6 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl delta = 0xffffffff;
if (mdev->type == G200_EW3) {
- vcomax = 800000; vcomin = 400000; pllreffreq = 25000;
@@ -375,19 +384,16 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
m = ((testn & 0x100) >> 1) |(testm);n = (testn & 0xFF);p = ((testn & 0x600) >> 3) |(testp2 << 3) |(testp);
m = testm + 1;n = testn + 1;p = testp + 1;s = testp2; } } } }
- vcomax = 550000; vcomin = 150000; pllreffreq = 48000;
@@ -408,10 +414,10 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn - 1;m = (testm - 1) |((n >> 1) & 0x80);p = testp - 1;
n = testn;m = testm;p = testp;s = 0; } } }@@ -429,6 +435,7 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
- unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; int i, j, tmpcount, vcount; bool pll_locked = false;
@@ -438,9 +445,14 @@ static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = ((pixpllcn & BIT(8)) >> 1) | pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = ((pixpllcn & GENMASK(10, 9)) >> 3) | (pixpllcs << 3) | pixpllcp;
for (i = 0; i <= 32 && pll_locked == false; i++) { if (i > 0) {
@@ -571,9 +583,9 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn - 1;m = testm - 1;p = testp - 1;
n = testn;m = testm;p = testp; } }@@ -590,6 +602,7 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN);
@@ -597,9 +610,14 @@ static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = (pixpllcs << 3) | pixpllcp;
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA);
@@ -685,9 +703,9 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn;m = testm;p = testp;
n = testn + 1;m = testm + 1;p = testp + 1; } if (delta == 0) break;@@ -719,12 +737,10 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn - 1;m = (testm - 1);p = testp - 1;
n = testn;m = testm;p = testp; }
if ((clock * testp) >= 600000)p |= 0x80; } }@@ -741,6 +757,7 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
- unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; int i, j, tmpcount, vcount; bool pll_locked = false;
@@ -750,9 +767,14 @@ static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = ((pixpllcn & BIT(8)) >> 1) | pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = (pixpllcs << 3) | pixpllcp;
for (i = 0; i <= 32 && pll_locked == false; i++) { WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
@@ -843,9 +865,10 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
m = testm | (testo << 3);n = testn;p = testr | (testr << 3);
m = (testm | (testo << 3)) + 1;n = testn + 1;p = testr + 1;s = testr; } } }@@ -863,6 +886,7 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200er(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN);
@@ -870,9 +894,14 @@ static void mgag200_set_pixpll_g200er(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = (pixpllcs << 3) | pixpllcp;
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA);
-- 2.32.0
Hi
Am 10.07.21 um 09:06 schrieb Sam Ravnborg:
Hi Thomas,
On Mon, Jul 05, 2021 at 02:45:09PM +0200, Thomas Zimmermann wrote:
The fields in struct mgag200_pll_values currently hold the bits of each register. Store the PLL values instead and let the PLL-update code figure out the bits for each register.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
I gave up trying to follow the changes in this patch.
I cannot blame you.
Also I was left with the impression that this was no win in readability at it looks to me like changes with a high risk to introduce a hard-to-find bug. Your changelog did not justify why this change is a win, only what is does. But whatever works better for you I guess..
As I mention, struct mgag200_pll_values should store the PLL values. But the individual compute and set functions for each hardware revision mess this up completely. Sometimes they use 'function values' sometimes they use 'register values'. If you'd try to debug a failed modeset operation and have to look at the PLL, the stored values would be meaningless, because there's simply no logic behind it.
The purpose of this patch is to make all compute functions store function values in the struct and make all update function compute the register values internally.
Do you think the change would be easier to understand if I change the original _set_plls() functions *before* splitting them into compute and update steps?
Best regards Thomas
Sam
drivers/gpu/drm/mgag200/mgag200_mode.c | 153 +++++++++++++++---------- 1 file changed, 91 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 9f6fe7673674..7d6707bd6c27 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -123,7 +123,7 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc const int in_div_max = 6; const int feed_div_min = 7; const int feed_div_max = 127;
- u8 testm, testn;
- u8 testp, testm, testn; u8 n = 0, m = 0, p, s; long f_vco; long computed;
@@ -141,10 +141,11 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc clock = p_clk_min >> 3;
f_vco = clock;
- for (p = 0;
p <= post_div_max && f_vco < p_clk_min;p = (p << 1) + 1, f_vco <<= 1)
for (testp = 0;
testp <= post_div_max && f_vco < p_clk_min;testp = (testp << 1) + 1, f_vco <<= 1);
p = testp + 1;
delta = clock;
@@ -157,12 +158,12 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc tmp_delta = computed - f_vco; if (tmp_delta < delta) { delta = tmp_delta;
m = testm;n = testn;
m = testm + 1;n = testn + 1; }
- f_vco = ref_clk * (n + 1) / (m + 1);
- f_vco = ref_clk * n / m; if (f_vco < 100000) s = 0; else if (f_vco < 140000)
@@ -186,6 +187,7 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc static void mgag200_set_pixpll_g200(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
misc = RREG8(MGA_MISC_IN);
@@ -193,9 +195,14 @@ static void mgag200_set_pixpll_g200(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
- pixpllcm = pixpllc->m - 1;
- pixpllcn = pixpllc->n - 1;
- pixpllcp = pixpllc->p - 1;
- pixpllcs = pixpllc->s;
- xpixpllcm = pixpllcm;
- xpixpllcn = pixpllcn;
- xpixpllcp = (pixpllcs << 3) | pixpllcp; WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
@@ -240,9 +247,9 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
m = testm - 1;n = testn - 1;p = testp - 1;
m = testm;n = testn;p = testp; } } }@@ -280,22 +287,19 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
if (tmpdelta < delta) { delta = tmpdelta;
m = testm - 1;n = testn - 1;p = testp - 1;
m = testm;n = testn;p = testp; } } }
fvv = pllreffreq * (n + 1) / (m + 1);
fvv = pllreffreq * n / m;
- if (fvv > 15) fvv = 15;
p |= (fvv << 4);m |= 0x80;
s = fvv << 1;clock = clock / 2; }
@@ -317,6 +321,7 @@ static void mgag200_set_pixpll_g200se(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp;
misc = RREG8(MGA_MISC_IN);
@@ -324,9 +329,14 @@ static void mgag200_set_pixpll_g200se(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
- pixpllcm = pixpllc->m - 1;
- pixpllcn = pixpllc->n - 1;
- pixpllcp = pixpllc->p - 1;
- pixpllcs = pixpllc->s;
- xpixpllcm = pixpllcm | ((pixpllcn & BIT(8)) >> 1);
- xpixpllcn = pixpllcn;
- xpixpllcp = (pixpllcs << 3) | pixpllcp; WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp);
@@ -352,7 +362,6 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl delta = 0xffffffff;
if (mdev->type == G200_EW3) {
- vcomax = 800000; vcomin = 400000; pllreffreq = 25000;
@@ -375,19 +384,16 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
m = ((testn & 0x100) >> 1) |(testm);n = (testn & 0xFF);p = ((testn & 0x600) >> 3) |(testp2 << 3) |(testp);
m = testm + 1;n = testn + 1;p = testp + 1;s = testp2; } } } }
- vcomax = 550000; vcomin = 150000; pllreffreq = 48000;
@@ -408,10 +414,10 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn - 1;m = (testm - 1) |((n >> 1) & 0x80);p = testp - 1;
n = testn;m = testm;p = testp;s = 0; } } }@@ -429,6 +435,7 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
- unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; int i, j, tmpcount, vcount; bool pll_locked = false;
@@ -438,9 +445,14 @@ static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = ((pixpllcn & BIT(8)) >> 1) | pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = ((pixpllcn & GENMASK(10, 9)) >> 3) | (pixpllcs << 3) | pixpllcp;
for (i = 0; i <= 32 && pll_locked == false; i++) { if (i > 0) {
@@ -571,9 +583,9 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn - 1;m = testm - 1;p = testp - 1;
n = testn;m = testm;p = testp; } }@@ -590,6 +602,7 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN);
@@ -597,9 +610,14 @@ static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = (pixpllcs << 3) | pixpllcp;
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA);
@@ -685,9 +703,9 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn;m = testm;p = testp;
n = testn + 1;m = testm + 1;p = testp + 1; } if (delta == 0) break;@@ -719,12 +737,10 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
n = testn - 1;m = (testm - 1);p = testp - 1;
n = testn;m = testm;p = testp; }
if ((clock * testp) >= 600000)p |= 0x80; } }@@ -741,6 +757,7 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
- unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp; int i, j, tmpcount, vcount; bool pll_locked = false;
@@ -750,9 +767,14 @@ static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = ((pixpllcn & BIT(8)) >> 1) | pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = (pixpllcs << 3) | pixpllcp;
for (i = 0; i <= 32 && pll_locked == false; i++) { WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
@@ -843,9 +865,10 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta;
m = testm | (testo << 3);n = testn;p = testr | (testr << 3);
m = (testm | (testo << 3)) + 1;n = testn + 1;p = testr + 1;s = testr; } } }@@ -863,6 +886,7 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl static void mgag200_set_pixpll_g200er(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) {
unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; u8 misc, xpixpllcm, xpixpllcn, xpixpllcp, tmp;
misc = RREG8(MGA_MISC_IN);
@@ -870,9 +894,14 @@ static void mgag200_set_pixpll_g200er(struct mga_device *mdev, misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; WREG8(MGA_MISC_OUT, misc);
- xpixpllcm = pixpllc->m;
- xpixpllcn = pixpllc->n;
- xpixpllcp = pixpllc->p | (pixpllc->s << 3);
pixpllcm = pixpllc->m - 1;
pixpllcn = pixpllc->n - 1;
pixpllcp = pixpllc->p - 1;
pixpllcs = pixpllc->s;
xpixpllcm = pixpllcm;
xpixpllcn = pixpllcn;
xpixpllcp = (pixpllcs << 3) | pixpllcp;
WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL); tmp = RREG8(DAC_DATA);
-- 2.32.0
Hi Thomas,
As I mention, struct mgag200_pll_values should store the PLL values. But the individual compute and set functions for each hardware revision mess this up completely. Sometimes they use 'function values' sometimes they use 'register values'. If you'd try to debug a failed modeset operation and have to look at the PLL, the stored values would be meaningless, because there's simply no logic behind it.
So this is the reason for this chage - and then it makes perferct sense to do it. Without this explanation is was to my eay useless chrunch, but this explanation makes sense.
The purpose of this patch is to make all compute functions store function values in the struct and make all update function compute the register values internally.
Do you think the change would be easier to understand if I change the original _set_plls() functions *before* splitting them into compute and update steps?
Na, this would still be very difficult to track down. The only way I would trust myself doing a proper review would be do code it myself and compare the final result. Alas, I am not going to do this.
I will take a look again when you post the next revision, and unless I stumble on something I can ack the code as in I have at least looked at all the code changes. That should be enough to have it committed and the time will tell if there is some fall-out.
Sam
Several PLL functions compute values for different device types. Split them up to make the code more readable. No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 256 ++++++++++++++----------- 1 file changed, 146 insertions(+), 110 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 7d6707bd6c27..d3b15e60f057 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -353,7 +353,7 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl { unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta; - unsigned int testp, testm, testn, testp2; + unsigned int testp, testm, testn; unsigned int p, m, n, s; unsigned int computed;
@@ -361,64 +361,29 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
delta = 0xffffffff;
- if (mdev->type == G200_EW3) { - vcomax = 800000; - vcomin = 400000; - pllreffreq = 25000; - - for (testp = 1; testp < 8; testp++) { - for (testp2 = 1; testp2 < 8; testp2++) { - if (testp < testp2) - continue; - if ((clock * testp * testp2) > vcomax) - continue; - if ((clock * testp * testp2) < vcomin) - continue; - for (testm = 1; testm < 26; testm++) { - for (testn = 32; testn < 2048 ; testn++) { - computed = (pllreffreq * testn) / - (testm * testp * testp2); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - m = testm + 1; - n = testn + 1; - p = testp + 1; - s = testp2; - } - } - } - } - } - } else { - vcomax = 550000; - vcomin = 150000; - pllreffreq = 48000; + vcomax = 550000; + vcomin = 150000; + pllreffreq = 48000;
- for (testp = 1; testp < 9; testp++) { - if (clock * testp > vcomax) - continue; - if (clock * testp < vcomin) - continue; + for (testp = 1; testp < 9; testp++) { + if (clock * testp > vcomax) + continue; + if (clock * testp < vcomin) + continue;
- for (testm = 1; testm < 17; testm++) { - for (testn = 1; testn < 151; testn++) { - computed = (pllreffreq * testn) / - (testm * testp); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - n = testn; - m = testm; - p = testp; - s = 0; - } + for (testm = 1; testm < 17; testm++) { + for (testn = 1; testn < 151; testn++) { + computed = (pllreffreq * testn) / (testm * testp); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + n = testn; + m = testm; + p = testp; + s = 0; } } } @@ -681,66 +646,30 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl
m = n = p = s = 0;
- if (mdev->type == G200_EH3) { - vcomax = 3000000; - vcomin = 1500000; - pllreffreq = 25000; + vcomax = 800000; + vcomin = 400000; + pllreffreq = 33333;
- delta = 0xffffffff; + delta = 0xffffffff;
- testp = 0; + for (testp = 16; testp > 0; testp >>= 1) { + if (clock * testp > vcomax) + continue; + if (clock * testp < vcomin) + continue;
- for (testm = 150; testm >= 6; testm--) { - if (clock * testm > vcomax) - continue; - if (clock * testm < vcomin) - continue; - for (testn = 120; testn >= 60; testn--) { - computed = (pllreffreq * testn) / testm; + for (testm = 1; testm < 33; testm++) { + for (testn = 17; testn < 257; testn++) { + computed = (pllreffreq * testn) / (testm * testp); if (computed > clock) tmpdelta = computed - clock; else tmpdelta = clock - computed; if (tmpdelta < delta) { delta = tmpdelta; - n = testn + 1; - m = testm + 1; - p = testp + 1; - } - if (delta == 0) - break; - } - if (delta == 0) - break; - } - } else { - - vcomax = 800000; - vcomin = 400000; - pllreffreq = 33333; - - delta = 0xffffffff; - - for (testp = 16; testp > 0; testp >>= 1) { - if (clock * testp > vcomax) - continue; - if (clock * testp < vcomin) - continue; - - for (testm = 1; testm < 33; testm++) { - for (testn = 17; testn < 257; testn++) { - computed = (pllreffreq * testn) / - (testm * testp); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - n = testn; - m = testm; - p = testp; - } + n = testn; + m = testm; + p = testp; } } } @@ -825,6 +754,57 @@ static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, } }
+static int mgag200_compute_pixpll_values_g200eh3(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta; + unsigned int testp, testm, testn; + unsigned int p, m, n, s; + unsigned int computed; + + m = n = p = s = 0; + + vcomax = 3000000; + vcomin = 1500000; + pllreffreq = 25000; + + delta = 0xffffffff; + + testp = 0; + + for (testm = 150; testm >= 6; testm--) { + if (clock * testm > vcomax) + continue; + if (clock * testm < vcomin) + continue; + for (testn = 120; testn >= 60; testn--) { + computed = (pllreffreq * testn) / testm; + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + n = testn + 1; + m = testm + 1; + p = testp + 1; + } + if (delta == 0) + break; + } + if (delta == 0) + break; + } + + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; +} + static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { @@ -932,6 +912,58 @@ static void mgag200_set_pixpll_g200er(struct mga_device *mdev, udelay(50); }
+static int mgag200_compute_pixpll_values_g200ew3(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta; + unsigned int testp, testm, testn, testp2; + unsigned int p, m, n, s; + unsigned int computed; + + m = n = p = s = 0; + + delta = 0xffffffff; + + vcomax = 800000; + vcomin = 400000; + pllreffreq = 25000; + + for (testp = 1; testp < 8; testp++) { + for (testp2 = 1; testp2 < 8; testp2++) { + if (testp < testp2) + continue; + if ((clock * testp * testp2) > vcomax) + continue; + if ((clock * testp * testp2) < vcomin) + continue; + for (testm = 1; testm < 26; testm++) { + for (testn = 32; testn < 2048 ; testn++) { + computed = (pllreffreq * testn) / (testm * testp * testp2); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + m = testm + 1; + n = testn + 1; + p = testp + 1; + s = testp2; + } + } + } + } + } + + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; +} + static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock) { struct mgag200_pll_values pixpll; @@ -947,19 +979,23 @@ static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock) ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll); break; case G200_WB: - case G200_EW3: ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll); break; case G200_EV: ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll); break; case G200_EH: - case G200_EH3: ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll); break; + case G200_EH3: + ret = mgag200_compute_pixpll_values_g200eh3(mdev, clock, &pixpll); + break; case G200_ER: ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll); break; + case G200_EW3: + ret = mgag200_compute_pixpll_values_g200ew3(mdev, clock, &pixpll); + break; }
if (ret)
Move PLL compute and update functionality to distict places within the file. Contains some minor style cleanups, but no functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 737 +++++++++++++------------ 1 file changed, 369 insertions(+), 368 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index d3b15e60f057..72fdf242cac7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -184,30 +184,6 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc return 0; }
-static void mgag200_set_pixpll_g200(struct mga_device *mdev, - const struct mgag200_pll_values *pixpllc) -{ - unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; - u8 misc, xpixpllcm, xpixpllcn, xpixpllcp; - - misc = RREG8(MGA_MISC_IN); - misc &= ~MGAREG_MISC_CLK_SEL_MASK; - misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; - WREG8(MGA_MISC_OUT, misc); - - pixpllcm = pixpllc->m - 1; - pixpllcn = pixpllc->n - 1; - pixpllcp = pixpllc->p - 1; - pixpllcs = pixpllc->s; - - xpixpllcm = pixpllcm; - xpixpllcn = pixpllcn; - xpixpllcp = (pixpllcs << 3) | pixpllcp; - WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); - WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); - WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp); -} - static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { @@ -223,12 +199,12 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl unsigned int i;
m = n = p = s = 0; + delta = 0xffffffff;
if (unique_rev_id <= 0x03) { vcomax = 320000; vcomin = 160000; pllreffreq = 25000; - delta = 0xffffffff; permitteddelta = clock * 5 / 1000;
for (testp = 8; testp > 0; testp /= 2) { @@ -261,10 +237,8 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
if (clock < 25000) clock = 25000; - clock = clock * 2;
- delta = 0xFFFFFFFF; /* Permited delta is 0.5% as VESA Specification */ permitteddelta = clock * 5 / 1000;
@@ -317,38 +291,55 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl return 0; }
-static void mgag200_set_pixpll_g200se(struct mga_device *mdev, - const struct mgag200_pll_values *pixpllc) +static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { - u32 unique_rev_id = mdev->model.g200se.unique_rev_id; - unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; - u8 misc, xpixpllcm, xpixpllcn, xpixpllcp; + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta; + unsigned int testp, testm, testn; + unsigned int p, m, n, s; + unsigned int computed;
- misc = RREG8(MGA_MISC_IN); - misc &= ~MGAREG_MISC_CLK_SEL_MASK; - misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; - WREG8(MGA_MISC_OUT, misc); + m = n = p = s = 0; + delta = 0xffffffff;
- pixpllcm = pixpllc->m - 1; - pixpllcn = pixpllc->n - 1; - pixpllcp = pixpllc->p - 1; - pixpllcs = pixpllc->s; + vcomax = 550000; + vcomin = 150000; + pllreffreq = 48000;
- xpixpllcm = pixpllcm | ((pixpllcn & BIT(8)) >> 1); - xpixpllcn = pixpllcn; - xpixpllcp = (pixpllcs << 3) | pixpllcp; - WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); - WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); - WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp); + for (testp = 1; testp < 9; testp++) { + if (clock * testp > vcomax) + continue; + if (clock * testp < vcomin) + continue;
- if (unique_rev_id >= 0x04) { - WREG_DAC(0x1a, 0x09); - msleep(20); - WREG_DAC(0x1a, 0x01); + for (testm = 1; testm < 17; testm++) { + for (testn = 1; testn < 151; testn++) { + computed = (pllreffreq * testn) / (testm * testp); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + n = testn; + m = testm; + p = testp; + s = 0; + } + } + } } + + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; }
-static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock, +static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { unsigned int vcomax, vcomin, pllreffreq; @@ -358,21 +349,68 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl unsigned int computed;
m = n = p = s = 0; - delta = 0xffffffff;
vcomax = 550000; vcomin = 150000; - pllreffreq = 48000; + pllreffreq = 50000;
- for (testp = 1; testp < 9; testp++) { + for (testp = 16; testp > 0; testp--) { if (clock * testp > vcomax) continue; if (clock * testp < vcomin) continue;
- for (testm = 1; testm < 17; testm++) { - for (testn = 1; testn < 151; testn++) { + for (testn = 1; testn < 257; testn++) { + for (testm = 1; testm < 17; testm++) { + computed = (pllreffreq * testn) / + (testm * testp); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + n = testn; + m = testm; + p = testp; + } + } + } + } + + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; +} + +static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta; + unsigned int testp, testm, testn; + unsigned int p, m, n, s; + unsigned int computed; + + m = n = p = s = 0; + delta = 0xffffffff; + + vcomax = 800000; + vcomin = 400000; + pllreffreq = 33333; + + for (testp = 16; testp > 0; testp >>= 1) { + if (clock * testp > vcomax) + continue; + if (clock * testp < vcomin) + continue; + + for (testm = 1; testm < 33; testm++) { + for (testn = 17; testn < 257; testn++) { computed = (pllreffreq * testn) / (testm * testp); if (computed > clock) tmpdelta = computed - clock; @@ -383,7 +421,6 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl n = testn; m = testm; p = testp; - s = 0; } } } @@ -397,6 +434,256 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl return 0; }
+static int mgag200_compute_pixpll_values_g200eh3(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta; + unsigned int testp, testm, testn; + unsigned int p, m, n, s; + unsigned int computed; + + m = n = p = s = 0; + delta = 0xffffffff; + testp = 0; + + vcomax = 3000000; + vcomin = 1500000; + pllreffreq = 25000; + + for (testm = 150; testm >= 6; testm--) { + if (clock * testm > vcomax) + continue; + if (clock * testm < vcomin) + continue; + for (testn = 120; testn >= 60; testn--) { + computed = (pllreffreq * testn) / testm; + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + n = testn + 1; + m = testm + 1; + p = testp + 1; + } + if (delta == 0) + break; + } + if (delta == 0) + break; + } + + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; +} + +static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + static const unsigned int m_div_val[] = { 1, 2, 4, 8 }; + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta; + int testr, testn, testm, testo; + unsigned int p, m, n, s; + unsigned int computed, vco; + + vcomax = 1488000; + vcomin = 1056000; + pllreffreq = 48000; + + m = n = p = s = 0; + delta = 0xffffffff; + + for (testr = 0; testr < 4; testr++) { + if (delta == 0) + break; + for (testn = 5; testn < 129; testn++) { + if (delta == 0) + break; + for (testm = 3; testm >= 0; testm--) { + if (delta == 0) + break; + for (testo = 5; testo < 33; testo++) { + vco = pllreffreq * (testn + 1) / + (testr + 1); + if (vco < vcomin) + continue; + if (vco > vcomax) + continue; + computed = vco / (m_div_val[testm] * (testo + 1)); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + m = (testm | (testo << 3)) + 1; + n = testn + 1; + p = testr + 1; + s = testr; + } + } + } + } + } + + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; +} + +static int mgag200_compute_pixpll_values_g200ew3(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta; + unsigned int testp, testm, testn, testp2; + unsigned int p, m, n, s; + unsigned int computed; + + m = n = p = s = 0; + delta = 0xffffffff; + + vcomax = 800000; + vcomin = 400000; + pllreffreq = 25000; + + for (testp = 1; testp < 8; testp++) { + for (testp2 = 1; testp2 < 8; testp2++) { + if (testp < testp2) + continue; + if ((clock * testp * testp2) > vcomax) + continue; + if ((clock * testp * testp2) < vcomin) + continue; + for (testm = 1; testm < 26; testm++) { + for (testn = 32; testn < 2048 ; testn++) { + computed = (pllreffreq * testn) / (testm * testp * testp2); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + m = testm + 1; + n = testn + 1; + p = testp + 1; + s = testp2; + } + } + } + } + } + + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s; + + return 0; +} + +static int mgag200_compute_pixpll_values(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpll) +{ + int ret; + + switch (mdev->type) { + case G200_PCI: + case G200_AGP: + ret = mgag200_compute_pixpll_values_g200(mdev, clock, pixpll); + break; + case G200_SE_A: + case G200_SE_B: + ret = mgag200_compute_pixpll_values_g200se(mdev, clock, pixpll); + break; + case G200_WB: + ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, pixpll); + break; + case G200_EV: + ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, pixpll); + break; + case G200_EH: + ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, pixpll); + break; + case G200_EH3: + ret = mgag200_compute_pixpll_values_g200eh3(mdev, clock, pixpll); + break; + case G200_ER: + ret = mgag200_compute_pixpll_values_g200er(mdev, clock, pixpll); + break; + case G200_EW3: + ret = mgag200_compute_pixpll_values_g200ew3(mdev, clock, pixpll); + break; + } + + return ret; +} + +static void mgag200_set_pixpll_g200(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp; + + misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = pixpllcm; + xpixpllcn = pixpllcn; + xpixpllcp = (pixpllcs << 3) | pixpllcp; + WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp); +} + +static void mgag200_set_pixpll_g200se(struct mga_device *mdev, + const struct mgag200_pll_values *pixpllc) +{ + u32 unique_rev_id = mdev->model.g200se.unique_rev_id; + unsigned int pixpllcm, pixpllcn, pixpllcp, pixpllcs; + u8 misc, xpixpllcm, xpixpllcn, xpixpllcp; + + misc = RREG8(MGA_MISC_IN); + misc &= ~MGAREG_MISC_CLK_SEL_MASK; + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + + pixpllcm = pixpllc->m - 1; + pixpllcn = pixpllc->n - 1; + pixpllcp = pixpllc->p - 1; + pixpllcs = pixpllc->s; + + xpixpllcm = pixpllcm | ((pixpllcn & BIT(8)) >> 1); + xpixpllcn = pixpllcn; + xpixpllcp = (pixpllcs << 3) | pixpllcp; + WREG_DAC(MGA1064_PIX_PLLC_M, xpixpllcm); + WREG_DAC(MGA1064_PIX_PLLC_N, xpixpllcn); + WREG_DAC(MGA1064_PIX_PLLC_P, xpixpllcp); + + if (unique_rev_id >= 0x04) { + WREG_DAC(0x1a, 0x09); + msleep(20); + WREG_DAC(0x1a, 0x01); + } +} + static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { @@ -500,68 +787,20 @@ static void mgag200_set_pixpll_g200wb(struct mga_device *mdev, vcount = RREG8(MGAREG_VCOUNT);
for (j = 0; j < 30 && pll_locked == false; j++) { - tmpcount = RREG8(MGAREG_VCOUNT); - if (tmpcount < vcount) - vcount = 0; - if ((tmpcount - vcount) > 2) - pll_locked = true; - else - udelay(5); - } - } - - WREG8(DAC_INDEX, MGA1064_REMHEADCTL); - tmp = RREG8(DAC_DATA); - tmp &= ~MGA1064_REMHEADCTL_CLKDIS; - WREG_DAC(MGA1064_REMHEADCTL, tmp); -} - -static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long clock, - struct mgag200_pll_values *pixpllc) -{ - unsigned int vcomax, vcomin, pllreffreq; - unsigned int delta, tmpdelta; - unsigned int testp, testm, testn; - unsigned int p, m, n, s; - unsigned int computed; - - m = n = p = s = 0; - vcomax = 550000; - vcomin = 150000; - pllreffreq = 50000; - - delta = 0xffffffff; - - for (testp = 16; testp > 0; testp--) { - if (clock * testp > vcomax) - continue; - if (clock * testp < vcomin) - continue; - - for (testn = 1; testn < 257; testn++) { - for (testm = 1; testm < 17; testm++) { - computed = (pllreffreq * testn) / - (testm * testp); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - n = testn; - m = testm; - p = testp; - } - } + tmpcount = RREG8(MGAREG_VCOUNT); + if (tmpcount < vcount) + vcount = 0; + if ((tmpcount - vcount) > 2) + pll_locked = true; + else + udelay(5); } }
- pixpllc->m = m; - pixpllc->n = n; - pixpllc->p = p; - pixpllc->s = s; - - return 0; + WREG8(DAC_INDEX, MGA1064_REMHEADCTL); + tmp = RREG8(DAC_DATA); + tmp &= ~MGA1064_REMHEADCTL_CLKDIS; + WREG_DAC(MGA1064_REMHEADCTL, tmp); }
static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, @@ -635,54 +874,6 @@ static void mgag200_set_pixpll_g200ev(struct mga_device *mdev, WREG8(DAC_DATA, tmp); }
-static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long clock, - struct mgag200_pll_values *pixpllc) -{ - unsigned int vcomax, vcomin, pllreffreq; - unsigned int delta, tmpdelta; - unsigned int testp, testm, testn; - unsigned int p, m, n, s; - unsigned int computed; - - m = n = p = s = 0; - - vcomax = 800000; - vcomin = 400000; - pllreffreq = 33333; - - delta = 0xffffffff; - - for (testp = 16; testp > 0; testp >>= 1) { - if (clock * testp > vcomax) - continue; - if (clock * testp < vcomin) - continue; - - for (testm = 1; testm < 33; testm++) { - for (testn = 17; testn < 257; testn++) { - computed = (pllreffreq * testn) / (testm * testp); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - n = testn; - m = testm; - p = testp; - } - } - } - } - - pixpllc->m = m; - pixpllc->n = n; - pixpllc->p = p; - pixpllc->s = s; - - return 0; -} - static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { @@ -754,115 +945,6 @@ static void mgag200_set_pixpll_g200eh(struct mga_device *mdev, } }
-static int mgag200_compute_pixpll_values_g200eh3(struct mga_device *mdev, long clock, - struct mgag200_pll_values *pixpllc) -{ - unsigned int vcomax, vcomin, pllreffreq; - unsigned int delta, tmpdelta; - unsigned int testp, testm, testn; - unsigned int p, m, n, s; - unsigned int computed; - - m = n = p = s = 0; - - vcomax = 3000000; - vcomin = 1500000; - pllreffreq = 25000; - - delta = 0xffffffff; - - testp = 0; - - for (testm = 150; testm >= 6; testm--) { - if (clock * testm > vcomax) - continue; - if (clock * testm < vcomin) - continue; - for (testn = 120; testn >= 60; testn--) { - computed = (pllreffreq * testn) / testm; - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - n = testn + 1; - m = testm + 1; - p = testp + 1; - } - if (delta == 0) - break; - } - if (delta == 0) - break; - } - - pixpllc->m = m; - pixpllc->n = n; - pixpllc->p = p; - pixpllc->s = s; - - return 0; -} - -static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long clock, - struct mgag200_pll_values *pixpllc) -{ - static const unsigned int m_div_val[] = { 1, 2, 4, 8 }; - unsigned int vcomax, vcomin, pllreffreq; - unsigned int delta, tmpdelta; - int testr, testn, testm, testo; - unsigned int p, m, n, s; - unsigned int computed, vco; - - m = n = p = s = 0; - vcomax = 1488000; - vcomin = 1056000; - pllreffreq = 48000; - - delta = 0xffffffff; - - for (testr = 0; testr < 4; testr++) { - if (delta == 0) - break; - for (testn = 5; testn < 129; testn++) { - if (delta == 0) - break; - for (testm = 3; testm >= 0; testm--) { - if (delta == 0) - break; - for (testo = 5; testo < 33; testo++) { - vco = pllreffreq * (testn + 1) / - (testr + 1); - if (vco < vcomin) - continue; - if (vco > vcomax) - continue; - computed = vco / (m_div_val[testm] * (testo + 1)); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - m = (testm | (testo << 3)) + 1; - n = testn + 1; - p = testr + 1; - s = testr; - } - } - } - } - } - - pixpllc->m = m; - pixpllc->n = n; - pixpllc->p = p; - pixpllc->s = s; - - return 0; -} - static void mgag200_set_pixpll_g200er(struct mga_device *mdev, const struct mgag200_pll_values *pixpllc) { @@ -912,121 +994,38 @@ static void mgag200_set_pixpll_g200er(struct mga_device *mdev, udelay(50); }
-static int mgag200_compute_pixpll_values_g200ew3(struct mga_device *mdev, long clock, - struct mgag200_pll_values *pixpllc) -{ - unsigned int vcomax, vcomin, pllreffreq; - unsigned int delta, tmpdelta; - unsigned int testp, testm, testn, testp2; - unsigned int p, m, n, s; - unsigned int computed; - - m = n = p = s = 0; - - delta = 0xffffffff; - - vcomax = 800000; - vcomin = 400000; - pllreffreq = 25000; - - for (testp = 1; testp < 8; testp++) { - for (testp2 = 1; testp2 < 8; testp2++) { - if (testp < testp2) - continue; - if ((clock * testp * testp2) > vcomax) - continue; - if ((clock * testp * testp2) < vcomin) - continue; - for (testm = 1; testm < 26; testm++) { - for (testn = 32; testn < 2048 ; testn++) { - computed = (pllreffreq * testn) / (testm * testp * testp2); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - m = testm + 1; - n = testn + 1; - p = testp + 1; - s = testp2; - } - } - } - } - } - - pixpllc->m = m; - pixpllc->n = n; - pixpllc->p = p; - pixpllc->s = s; - - return 0; -} - -static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock) +static void mgag200_set_pixpll(struct mga_device *mdev, const struct mgag200_pll_values *pixpll) { - struct mgag200_pll_values pixpll; - int ret; - - switch(mdev->type) { - case G200_PCI: - case G200_AGP: - ret = mgag200_compute_pixpll_values_g200(mdev, clock, &pixpll); - break; - case G200_SE_A: - case G200_SE_B: - ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll); - break; - case G200_WB: - ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll); - break; - case G200_EV: - ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll); - break; - case G200_EH: - ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll); - break; - case G200_EH3: - ret = mgag200_compute_pixpll_values_g200eh3(mdev, clock, &pixpll); - break; - case G200_ER: - ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll); - break; - case G200_EW3: - ret = mgag200_compute_pixpll_values_g200ew3(mdev, clock, &pixpll); - break; - } - - if (ret) - return; - switch (mdev->type) { case G200_PCI: case G200_AGP: - mgag200_set_pixpll_g200(mdev, &pixpll); + mgag200_set_pixpll_g200(mdev, pixpll); break; case G200_SE_A: case G200_SE_B: - mgag200_set_pixpll_g200se(mdev, &pixpll); + mgag200_set_pixpll_g200se(mdev, pixpll); break; case G200_WB: case G200_EW3: - mgag200_set_pixpll_g200wb(mdev, &pixpll); + mgag200_set_pixpll_g200wb(mdev, pixpll); break; case G200_EV: - mgag200_set_pixpll_g200ev(mdev, &pixpll); + mgag200_set_pixpll_g200ev(mdev, pixpll); break; case G200_EH: case G200_EH3: - mgag200_set_pixpll_g200eh(mdev, &pixpll); + mgag200_set_pixpll_g200eh(mdev, pixpll); break; case G200_ER: - mgag200_set_pixpll_g200er(mdev, &pixpll); + mgag200_set_pixpll_g200er(mdev, pixpll); break; } }
+/* + * Modesetting helpers + */ + static void mgag200_g200wb_hold_bmc(struct mga_device *mdev) { u8 tmp; @@ -1790,13 +1789,15 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, .y1 = 0, .y2 = fb->height, }; + struct mgag200_pll_values pixpll;
if (mdev->type == G200_WB || mdev->type == G200_EW3) mgag200_g200wb_hold_bmc(mdev);
mgag200_set_format_regs(mdev, fb); mgag200_set_mode_regs(mdev, adjusted_mode); - mgag200_crtc_set_plls(mdev, adjusted_mode->clock); + mgag200_compute_pixpll_values(mdev, adjusted_mode->clock, &pixpll); + mgag200_set_pixpll(mdev, &pixpll);
if (mdev->type == G200_ER) mgag200_g200er_reset_tagfifo(mdev);
The compute function for G200SE pixle PLLs handles two revisions with different algorithms. Split it accordingly to make it readable.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 165 +++++++++++++++---------- 1 file changed, 97 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 72fdf242cac7..99b35e4f9353 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -184,100 +184,118 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc return 0; }
-static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock, - struct mgag200_pll_values *pixpllc) +static int mgag200_compute_pixpll_values_g200se_00(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) { - static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1}; - - u32 unique_rev_id = mdev->model.g200se.unique_rev_id; unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta, permitteddelta; unsigned int testp, testm, testn; unsigned int p, m, n, s; unsigned int computed; - unsigned int fvv; - unsigned int i;
m = n = p = s = 0; delta = 0xffffffff;
- if (unique_rev_id <= 0x03) { - vcomax = 320000; - vcomin = 160000; - pllreffreq = 25000; - permitteddelta = clock * 5 / 1000; + vcomax = 320000; + vcomin = 160000; + pllreffreq = 25000; + permitteddelta = clock * 5 / 1000;
- for (testp = 8; testp > 0; testp /= 2) { - if (clock * testp > vcomax) - continue; - if (clock * testp < vcomin) - continue; + for (testp = 8; testp > 0; testp /= 2) { + if (clock * testp > vcomax) + continue; + if (clock * testp < vcomin) + continue;
- for (testn = 17; testn < 256; testn++) { - for (testm = 1; testm < 32; testm++) { - computed = (pllreffreq * testn) / - (testm * testp); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; - if (tmpdelta < delta) { - delta = tmpdelta; - m = testm; - n = testn; - p = testp; - } + for (testn = 17; testn < 256; testn++) { + for (testm = 1; testm < 32; testm++) { + computed = (pllreffreq * testn) / (testm * testp); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + if (tmpdelta < delta) { + delta = tmpdelta; + m = testm; + n = testn; + p = testp; } } } - } else { - vcomax = 1600000; - vcomin = 800000; - pllreffreq = 25000; + }
- if (clock < 25000) - clock = 25000; - clock = clock * 2; + if (delta > permitteddelta) { + pr_warn("PLL delta too large\n"); + return -EINVAL; + }
- /* Permited delta is 0.5% as VESA Specification */ - permitteddelta = clock * 5 / 1000; + pixpllc->m = m; + pixpllc->n = n; + pixpllc->p = p; + pixpllc->s = s;
- for (i = 0 ; i < ARRAY_SIZE(pvalues_e4); i++) { - testp = pvalues_e4[i]; + return 0; +}
- if ((clock * testp) > vcomax) - continue; - if ((clock * testp) < vcomin) - continue; +static int mgag200_compute_pixpll_values_g200se_04(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
- for (testn = 50; testn <= 256; testn++) { - for (testm = 1; testm <= 32; testm++) { - computed = (pllreffreq * testn) / - (testm * testp); - if (computed > clock) - tmpdelta = computed - clock; - else - tmpdelta = clock - computed; + unsigned int vcomax, vcomin, pllreffreq; + unsigned int delta, tmpdelta, permitteddelta; + unsigned int testp, testm, testn; + unsigned int p, m, n, s; + unsigned int computed; + unsigned int fvv; + unsigned int i;
- if (tmpdelta < delta) { - delta = tmpdelta; - m = testm; - n = testn; - p = testp; - } + m = n = p = s = 0; + delta = 0xffffffff; + + vcomax = 1600000; + vcomin = 800000; + pllreffreq = 25000; + + if (clock < 25000) + clock = 25000; + clock = clock * 2; + + /* Permited delta is 0.5% as VESA Specification */ + permitteddelta = clock * 5 / 1000; + + for (i = 0 ; i < ARRAY_SIZE(pvalues_e4); i++) { + testp = pvalues_e4[i]; + + if ((clock * testp) > vcomax) + continue; + if ((clock * testp) < vcomin) + continue; + + for (testn = 50; testn <= 256; testn++) { + for (testm = 1; testm <= 32; testm++) { + computed = (pllreffreq * testn) / (testm * testp); + if (computed > clock) + tmpdelta = computed - clock; + else + tmpdelta = clock - computed; + + if (tmpdelta < delta) { + delta = tmpdelta; + m = testm; + n = testn; + p = testp; } } } - - fvv = pllreffreq * n / m; - fvv = (fvv - 800000) / 50000; - if (fvv > 15) - fvv = 15; - s = fvv << 1; - - clock = clock / 2; }
+ fvv = pllreffreq * n / m; + fvv = (fvv - 800000) / 50000; + if (fvv > 15) + fvv = 15; + s = fvv << 1; + if (delta > permitteddelta) { pr_warn("PLL delta too large\n"); return -EINVAL; @@ -291,6 +309,17 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl return 0; }
+static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock, + struct mgag200_pll_values *pixpllc) +{ + u32 unique_rev_id = mdev->model.g200se.unique_rev_id; + + if (unique_rev_id >= 0x04) + return mgag200_compute_pixpll_values_g200se_04(mdev, clock, pixpllc); + else + return mgag200_compute_pixpll_values_g200se_00(mdev, clock, pixpllc); +} + static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) {
Move the PLL constants to the RO data section by declaring them as static const. No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 70 ++++++++++++-------------- 1 file changed, 31 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 99b35e4f9353..5da72ebd8a7f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -187,7 +187,10 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc static int mgag200_compute_pixpll_values_g200se_00(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { - unsigned int vcomax, vcomin, pllreffreq; + static const unsigned int vcomax = 320000; + static const unsigned int vcomin = 160000; + static const unsigned int pllreffreq = 25000; + unsigned int delta, tmpdelta, permitteddelta; unsigned int testp, testm, testn; unsigned int p, m, n, s; @@ -196,9 +199,6 @@ static int mgag200_compute_pixpll_values_g200se_00(struct mga_device *mdev, long m = n = p = s = 0; delta = 0xffffffff;
- vcomax = 320000; - vcomin = 160000; - pllreffreq = 25000; permitteddelta = clock * 5 / 1000;
for (testp = 8; testp > 0; testp /= 2) { @@ -241,8 +241,10 @@ static int mgag200_compute_pixpll_values_g200se_04(struct mga_device *mdev, long struct mgag200_pll_values *pixpllc) { static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1}; + static const unsigned int vcomax = 1600000; + static const unsigned int vcomin = 800000; + static const unsigned int pllreffreq = 25000;
- unsigned int vcomax, vcomin, pllreffreq; unsigned int delta, tmpdelta, permitteddelta; unsigned int testp, testm, testn; unsigned int p, m, n, s; @@ -253,10 +255,6 @@ static int mgag200_compute_pixpll_values_g200se_04(struct mga_device *mdev, long m = n = p = s = 0; delta = 0xffffffff;
- vcomax = 1600000; - vcomin = 800000; - pllreffreq = 25000; - if (clock < 25000) clock = 25000; clock = clock * 2; @@ -323,7 +321,10 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { - unsigned int vcomax, vcomin, pllreffreq; + static const unsigned int vcomax = 550000; + static const unsigned int vcomin = 150000; + static const unsigned int pllreffreq = 48000; + unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n, s; @@ -332,10 +333,6 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl m = n = p = s = 0; delta = 0xffffffff;
- vcomax = 550000; - vcomin = 150000; - pllreffreq = 48000; - for (testp = 1; testp < 9; testp++) { if (clock * testp > vcomax) continue; @@ -371,7 +368,10 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { - unsigned int vcomax, vcomin, pllreffreq; + static const unsigned int vcomax = 550000; + static const unsigned int vcomin = 150000; + static const unsigned int pllreffreq = 50000; + unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n, s; @@ -380,10 +380,6 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl m = n = p = s = 0; delta = 0xffffffff;
- vcomax = 550000; - vcomin = 150000; - pllreffreq = 50000; - for (testp = 16; testp > 0; testp--) { if (clock * testp > vcomax) continue; @@ -419,7 +415,10 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { - unsigned int vcomax, vcomin, pllreffreq; + static const unsigned int vcomax = 800000; + static const unsigned int vcomin = 400000; + static const unsigned int pllreffreq = 33333; + unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n, s; @@ -428,10 +427,6 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl m = n = p = s = 0; delta = 0xffffffff;
- vcomax = 800000; - vcomin = 400000; - pllreffreq = 33333; - for (testp = 16; testp > 0; testp >>= 1) { if (clock * testp > vcomax) continue; @@ -466,7 +461,10 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl static int mgag200_compute_pixpll_values_g200eh3(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { - unsigned int vcomax, vcomin, pllreffreq; + static const unsigned int vcomax = 3000000; + static const unsigned int vcomin = 1500000; + static const unsigned int pllreffreq = 25000; + unsigned int delta, tmpdelta; unsigned int testp, testm, testn; unsigned int p, m, n, s; @@ -476,10 +474,6 @@ static int mgag200_compute_pixpll_values_g200eh3(struct mga_device *mdev, long c delta = 0xffffffff; testp = 0;
- vcomax = 3000000; - vcomin = 1500000; - pllreffreq = 25000; - for (testm = 150; testm >= 6; testm--) { if (clock * testm > vcomax) continue; @@ -516,16 +510,15 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl struct mgag200_pll_values *pixpllc) { static const unsigned int m_div_val[] = { 1, 2, 4, 8 }; - unsigned int vcomax, vcomin, pllreffreq; + static const unsigned int vcomax = 1488000; + static const unsigned int vcomin = 1056000; + static const unsigned int pllreffreq = 48000; + unsigned int delta, tmpdelta; int testr, testn, testm, testo; unsigned int p, m, n, s; unsigned int computed, vco;
- vcomax = 1488000; - vcomin = 1056000; - pllreffreq = 48000; - m = n = p = s = 0; delta = 0xffffffff;
@@ -573,7 +566,10 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl static int mgag200_compute_pixpll_values_g200ew3(struct mga_device *mdev, long clock, struct mgag200_pll_values *pixpllc) { - unsigned int vcomax, vcomin, pllreffreq; + static const unsigned int vcomax = 800000; + static const unsigned int vcomin = 400000; + static const unsigned int pllreffreq = 25000; + unsigned int delta, tmpdelta; unsigned int testp, testm, testn, testp2; unsigned int p, m, n, s; @@ -582,10 +578,6 @@ static int mgag200_compute_pixpll_values_g200ew3(struct mga_device *mdev, long c m = n = p = s = 0; delta = 0xffffffff;
- vcomax = 800000; - vcomin = 400000; - pllreffreq = 25000; - for (testp = 1; testp < 8; testp++) { for (testp2 = 1; testp2 < 8; testp2++) { if (testp < testp2)
The CRTC state in mgag200 will hold PLL values for modeset operations. Simple KMS helpers already support custom state for planes. Extend the helpers to support custom CRTC state as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_simple_kms_helper.c | 39 +++++++++++++++++++-- drivers/gpu/drm/mgag200/mgag200_drv.h | 9 +++++ drivers/gpu/drm/mgag200/mgag200_mode.c | 46 +++++++++++++++++++++++++ include/drm/drm_simple_kms_helper.h | 27 +++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 735f4f34bcc4..72989ed1baba 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -145,6 +145,39 @@ static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = { .atomic_disable = drm_simple_kms_crtc_disable, };
+static void drm_simple_kms_crtc_reset(struct drm_crtc *crtc) +{ + struct drm_simple_display_pipe *pipe; + + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc); + if (!pipe->funcs || !pipe->funcs->reset_crtc) + return drm_atomic_helper_crtc_reset(crtc); + + return pipe->funcs->reset_crtc(pipe); +} + +static struct drm_crtc_state *drm_simple_kms_crtc_duplicate_state(struct drm_crtc *crtc) +{ + struct drm_simple_display_pipe *pipe; + + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc); + if (!pipe->funcs || !pipe->funcs->duplicate_crtc_state) + return drm_atomic_helper_crtc_duplicate_state(crtc); + + return pipe->funcs->duplicate_crtc_state(pipe); +} + +static void drm_simple_kms_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state) +{ + struct drm_simple_display_pipe *pipe; + + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc); + if (!pipe->funcs || !pipe->funcs->destroy_crtc_state) + drm_atomic_helper_crtc_destroy_state(crtc, state); + else + pipe->funcs->destroy_crtc_state(pipe, state); +} + static int drm_simple_kms_crtc_enable_vblank(struct drm_crtc *crtc) { struct drm_simple_display_pipe *pipe; @@ -168,12 +201,12 @@ static void drm_simple_kms_crtc_disable_vblank(struct drm_crtc *crtc) }
static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = { - .reset = drm_atomic_helper_crtc_reset, + .reset = drm_simple_kms_crtc_reset, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, + .atomic_duplicate_state = drm_simple_kms_crtc_duplicate_state, + .atomic_destroy_state = drm_simple_kms_crtc_destroy_state, .enable_vblank = drm_simple_kms_crtc_enable_vblank, .disable_vblank = drm_simple_kms_crtc_disable_vblank, }; diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index fca3904fde0c..c813d6c15f70 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -127,6 +127,15 @@ struct mgag200_pll_values { unsigned int s; };
+struct mgag200_crtc_state { + struct drm_crtc_state base; +}; + +static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base) +{ + return container_of(base, struct mgag200_crtc_state, base); +} + #define to_mga_connector(x) container_of(x, struct mga_connector, base)
struct mga_i2c_chan { diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 5da72ebd8a7f..6a5c419c6641 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1886,6 +1886,49 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->map[0]); }
+static struct drm_crtc_state * +mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe *pipe) +{ + struct drm_crtc *crtc = &pipe->crtc; + struct drm_crtc_state *crtc_state = crtc->state; + struct mgag200_crtc_state *new_mgag200_crtc_state; + + if (!crtc_state) + return NULL; + + new_mgag200_crtc_state = kzalloc(sizeof(*new_mgag200_crtc_state), GFP_KERNEL); + if (!new_mgag200_crtc_state) + return NULL; + __drm_atomic_helper_crtc_duplicate_state(crtc, &new_mgag200_crtc_state->base); + + return &new_mgag200_crtc_state->base; +} + +static void mgag200_simple_display_pipe_destroy_crtc_state(struct drm_simple_display_pipe *pipe, + struct drm_crtc_state *crtc_state) +{ + struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); + + __drm_atomic_helper_crtc_destroy_state(&mgag200_crtc_state->base); + kfree(mgag200_crtc_state); +} + +static void mgag200_simple_display_pipe_reset_crtc(struct drm_simple_display_pipe *pipe) +{ + struct drm_crtc *crtc = &pipe->crtc; + struct mgag200_crtc_state *mgag200_crtc_state; + + if (crtc->state) { + mgag200_simple_display_pipe_destroy_crtc_state(pipe, crtc->state); + crtc->state = NULL; /* must be set to NULL here */ + } + + mgag200_crtc_state = kzalloc(sizeof(*mgag200_crtc_state), GFP_KERNEL); + if (!mgag200_crtc_state) + return; + __drm_atomic_helper_crtc_reset(crtc, &mgag200_crtc_state->base); +} + static const struct drm_simple_display_pipe_funcs mgag200_simple_display_pipe_funcs = { .mode_valid = mgag200_simple_display_pipe_mode_valid, @@ -1893,6 +1936,9 @@ mgag200_simple_display_pipe_funcs = { .disable = mgag200_simple_display_pipe_disable, .check = mgag200_simple_display_pipe_check, .update = mgag200_simple_display_pipe_update, + .reset_crtc = mgag200_simple_display_pipe_reset_crtc, + .duplicate_crtc_state = mgag200_simple_display_pipe_duplicate_crtc_state, + .destroy_crtc_state = mgag200_simple_display_pipe_destroy_crtc_state, DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS, };
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index cf07132d4ee8..0b3647e614dd 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -153,6 +153,33 @@ struct drm_simple_display_pipe_funcs { */ void (*disable_vblank)(struct drm_simple_display_pipe *pipe);
+ /** + * @reset_crtc: + * + * Optional, called by &drm_crtc_funcs.reset. Please read the + * documentation for the &drm_crtc_funcs.reset hook for more details. + */ + void (*reset_crtc)(struct drm_simple_display_pipe *pipe); + + /** + * @duplicate_crtc_state: + * + * Optional, called by &drm_crtc_funcs.atomic_duplicate_state. Please + * read the documentation for the &drm_crtc_funcs.atomic_duplicate_state + * hook for more details. + */ + struct drm_crtc_state * (*duplicate_crtc_state)(struct drm_simple_display_pipe *pipe); + + /** + * @destroy_crtc_state: + * + * Optional, called by &drm_crtc_funcs.atomic_destroy_state. Please + * read the documentation for the &drm_crtc_funcs.atomic_destroy_state + * hook for more details. + */ + void (*destroy_crtc_state)(struct drm_simple_display_pipe *pipe, + struct drm_crtc_state *crtc_state); + /** * @reset_plane: *
Hi Thomas,
On Mon, Jul 05, 2021 at 02:45:14PM +0200, Thomas Zimmermann wrote:
The CRTC state in mgag200 will hold PLL values for modeset operations. Simple KMS helpers already support custom state for planes. Extend the helpers to support custom CRTC state as well.
This should be split in two patches - so the changes to drm_simple_kms_helper can get more attention.
The patch as such looks fine so when split you can add my: Acked-by: Sam Ravnborg sam@ravnborg.org on the mgag200 parts.
And Reviewed-by: Sam Ravnborg sam@ravnborg.org on the drm_simple_kms_helper parts.
Sam
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_simple_kms_helper.c | 39 +++++++++++++++++++-- drivers/gpu/drm/mgag200/mgag200_drv.h | 9 +++++ drivers/gpu/drm/mgag200/mgag200_mode.c | 46 +++++++++++++++++++++++++ include/drm/drm_simple_kms_helper.h | 27 +++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 735f4f34bcc4..72989ed1baba 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -145,6 +145,39 @@ static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = { .atomic_disable = drm_simple_kms_crtc_disable, };
+static void drm_simple_kms_crtc_reset(struct drm_crtc *crtc) +{
- struct drm_simple_display_pipe *pipe;
- pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
- if (!pipe->funcs || !pipe->funcs->reset_crtc)
return drm_atomic_helper_crtc_reset(crtc);- return pipe->funcs->reset_crtc(pipe);
+}
+static struct drm_crtc_state *drm_simple_kms_crtc_duplicate_state(struct drm_crtc *crtc) +{
- struct drm_simple_display_pipe *pipe;
- pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
- if (!pipe->funcs || !pipe->funcs->duplicate_crtc_state)
return drm_atomic_helper_crtc_duplicate_state(crtc);- return pipe->funcs->duplicate_crtc_state(pipe);
+}
+static void drm_simple_kms_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state) +{
- struct drm_simple_display_pipe *pipe;
- pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
- if (!pipe->funcs || !pipe->funcs->destroy_crtc_state)
drm_atomic_helper_crtc_destroy_state(crtc, state);- else
pipe->funcs->destroy_crtc_state(pipe, state);+}
static int drm_simple_kms_crtc_enable_vblank(struct drm_crtc *crtc) { struct drm_simple_display_pipe *pipe; @@ -168,12 +201,12 @@ static void drm_simple_kms_crtc_disable_vblank(struct drm_crtc *crtc) }
static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
- .reset = drm_atomic_helper_crtc_reset,
- .reset = drm_simple_kms_crtc_reset, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip,
- .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
- .atomic_duplicate_state = drm_simple_kms_crtc_duplicate_state,
- .atomic_destroy_state = drm_simple_kms_crtc_destroy_state, .enable_vblank = drm_simple_kms_crtc_enable_vblank, .disable_vblank = drm_simple_kms_crtc_disable_vblank,
}; diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index fca3904fde0c..c813d6c15f70 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -127,6 +127,15 @@ struct mgag200_pll_values { unsigned int s; };
+struct mgag200_crtc_state {
- struct drm_crtc_state base;
+};
+static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base) +{
- return container_of(base, struct mgag200_crtc_state, base);
+}
#define to_mga_connector(x) container_of(x, struct mga_connector, base)
struct mga_i2c_chan { diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 5da72ebd8a7f..6a5c419c6641 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1886,6 +1886,49 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->map[0]); }
+static struct drm_crtc_state * +mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe *pipe) +{
- struct drm_crtc *crtc = &pipe->crtc;
- struct drm_crtc_state *crtc_state = crtc->state;
- struct mgag200_crtc_state *new_mgag200_crtc_state;
- if (!crtc_state)
return NULL;- new_mgag200_crtc_state = kzalloc(sizeof(*new_mgag200_crtc_state), GFP_KERNEL);
- if (!new_mgag200_crtc_state)
return NULL;- __drm_atomic_helper_crtc_duplicate_state(crtc, &new_mgag200_crtc_state->base);
- return &new_mgag200_crtc_state->base;
+}
+static void mgag200_simple_display_pipe_destroy_crtc_state(struct drm_simple_display_pipe *pipe,
struct drm_crtc_state *crtc_state)+{
- struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
- __drm_atomic_helper_crtc_destroy_state(&mgag200_crtc_state->base);
- kfree(mgag200_crtc_state);
+}
+static void mgag200_simple_display_pipe_reset_crtc(struct drm_simple_display_pipe *pipe) +{
- struct drm_crtc *crtc = &pipe->crtc;
- struct mgag200_crtc_state *mgag200_crtc_state;
- if (crtc->state) {
mgag200_simple_display_pipe_destroy_crtc_state(pipe, crtc->state);crtc->state = NULL; /* must be set to NULL here */- }
- mgag200_crtc_state = kzalloc(sizeof(*mgag200_crtc_state), GFP_KERNEL);
- if (!mgag200_crtc_state)
return;- __drm_atomic_helper_crtc_reset(crtc, &mgag200_crtc_state->base);
+}
static const struct drm_simple_display_pipe_funcs mgag200_simple_display_pipe_funcs = { .mode_valid = mgag200_simple_display_pipe_mode_valid, @@ -1893,6 +1936,9 @@ mgag200_simple_display_pipe_funcs = { .disable = mgag200_simple_display_pipe_disable, .check = mgag200_simple_display_pipe_check, .update = mgag200_simple_display_pipe_update,
- .reset_crtc = mgag200_simple_display_pipe_reset_crtc,
- .duplicate_crtc_state = mgag200_simple_display_pipe_duplicate_crtc_state,
- .destroy_crtc_state = mgag200_simple_display_pipe_destroy_crtc_state, DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
};
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index cf07132d4ee8..0b3647e614dd 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -153,6 +153,33 @@ struct drm_simple_display_pipe_funcs { */ void (*disable_vblank)(struct drm_simple_display_pipe *pipe);
- /**
* @reset_crtc:** Optional, called by &drm_crtc_funcs.reset. Please read the* documentation for the &drm_crtc_funcs.reset hook for more details.*/- void (*reset_crtc)(struct drm_simple_display_pipe *pipe);
- /**
* @duplicate_crtc_state:** Optional, called by &drm_crtc_funcs.atomic_duplicate_state. Please* read the documentation for the &drm_crtc_funcs.atomic_duplicate_state* hook for more details.*/- struct drm_crtc_state * (*duplicate_crtc_state)(struct drm_simple_display_pipe *pipe);
- /**
* @destroy_crtc_state:** Optional, called by &drm_crtc_funcs.atomic_destroy_state. Please* read the documentation for the &drm_crtc_funcs.atomic_destroy_state* hook for more details.*/- void (*destroy_crtc_state)(struct drm_simple_display_pipe *pipe,
struct drm_crtc_state *crtc_state);- /**
- @reset_plane:
-- 2.32.0
PLL setup can fail if the display mode's clock is not supported by any PLL configuration. Compute the PLL values during atomic check, so that atomic commits can fail at the appropriate time. If successful, use the values in the atomic-update phase.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/mgag200/mgag200_drv.h | 2 ++ drivers/gpu/drm/mgag200/mgag200_mode.c | 20 +++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index c813d6c15f70..6473e9c037d0 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -129,6 +129,8 @@ struct mgag200_pll_values {
struct mgag200_crtc_state { struct drm_crtc_state base; + + struct mgag200_pll_values pixpll; };
static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base) diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 6a5c419c6641..55c4f76175bd 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1802,6 +1802,7 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_device *dev = crtc->dev; struct mga_device *mdev = to_mga_device(dev); struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode; + struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); struct drm_framebuffer *fb = plane_state->fb; struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_rect fullscreen = { @@ -1810,15 +1811,13 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, .y1 = 0, .y2 = fb->height, }; - struct mgag200_pll_values pixpll;
if (mdev->type == G200_WB || mdev->type == G200_EW3) mgag200_g200wb_hold_bmc(mdev);
mgag200_set_format_regs(mdev, fb); mgag200_set_mode_regs(mdev, adjusted_mode); - mgag200_compute_pixpll_values(mdev, adjusted_mode->clock, &pixpll); - mgag200_set_pixpll(mdev, &pixpll); + mgag200_set_pixpll(mdev, &mgag200_crtc_state->pixpll);
if (mdev->type == G200_ER) mgag200_g200er_reset_tagfifo(mdev); @@ -1852,8 +1851,12 @@ mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state) { struct drm_plane *plane = plane_state->plane; + struct drm_device *dev = plane->dev; + struct mga_device *mdev = to_mga_device(dev); + struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); struct drm_framebuffer *new_fb = plane_state->fb; struct drm_framebuffer *fb = NULL; + int ret;
if (!new_fb) return 0; @@ -1864,6 +1867,13 @@ mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe, if (!fb || (fb->format != new_fb->format)) crtc_state->mode_changed = true; /* update PLL settings */
+ if (crtc_state->mode_changed) { + ret = mgag200_compute_pixpll_values(mdev, crtc_state->mode.clock, + &mgag200_crtc_state->pixpll); + if (ret) + return ret; + } + return 0; }
@@ -1891,6 +1901,7 @@ mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe { struct drm_crtc *crtc = &pipe->crtc; struct drm_crtc_state *crtc_state = crtc->state; + struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state); struct mgag200_crtc_state *new_mgag200_crtc_state;
if (!crtc_state) @@ -1901,6 +1912,9 @@ mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe return NULL; __drm_atomic_helper_crtc_duplicate_state(crtc, &new_mgag200_crtc_state->base);
+ memcpy(&new_mgag200_crtc_state->pixpll, &mgag200_crtc_state->pixpll, + sizeof(new_mgag200_crtc_state->pixpll)); + return &new_mgag200_crtc_state->base; }
dri-devel@lists.freedesktop.org
-
Sam Ravnborg -
Thomas Zimmermann