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@xxxxxxx>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 cloctmp_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 clif (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); -- 2.32.0
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
