[PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values
Thomas Zimmermann
tzimmermann at suse.de
Mon Jul 12 14:09:55 UTC 2021
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 at 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
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210712/11d1a2a2/attachment.sig>
More information about the dri-devel
mailing list