| Message ID | 20210705124515.27253-7-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mgag200: Refactor PLL setup | expand |
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; > 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
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; >> 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
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
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);
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(-)