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%2Fstore%2Fdata%2F024667257.pdf%3Fk%3DEwAAAXqa--YRAAACWCABFrit5RcAiDXxvUVLzg4j6q39p78ki8fJxGpPn7F6&img=%2F%2Fs3.manualzz.com%2Fstore%2Fdata%2F024667257_1-acc0a233d724738660be61a3ba8e1135&ads=true#G10.1092838

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

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

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

Yes. The tests are in the compute functions, which will be allowed to 
fail during atomic check.

Best regards
Thomas

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