Re: [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.
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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux