[PATCH 00/12] mgag200: Refactor PLL setup

[PATCH 00/12] mgag200: Refactor PLL setup

[Thomas Zimmermann]

Split the PLL setup code into computation and update functions; compute
the PLL values during atomic checks, update the PLL during atomic commits;
cleanup the whole thing.

The current PLL setup code for mgag200 mixes up computation if the PLL
values and programming the HW. Both is done during atomic commits. As the
computation phase can fail, the patch splits the functions and moves
the computation to atomic-check phase. The PLL values are stores as part
of the CRTC's atomic state.

As the PLL code is currently unmaintainable, apply various cleanups. For
example, split functions that handle multiple HW revisions, constify values,
move compute and update code to distict locations, and unify the
representation of the PLL's values.

Tested on G200EH by setting modes in Weston, fbdev, and Xorg. Further
testing is welcome.

Thomas Zimmermann (12):
  drm/mgag200: Select clock in PLL update functions
  drm/mgag200: Return errno codes from PLL compute functions
  drm/mgag200: Remove P_ARRAY_SIZE
  drm/mgag200: Split PLL setup into compute and update functions
  drm/mgag200: Introduce separate variable for PLL S parameter
  drm/mgag200: Store values (not bits) in struct mgag200_pll_values
  drm/mgag200: Split several PLL functions by device type
  drm/mgag200: Separate PLL compute and update functions from each other
  drm/mgag200: Split PLL computation for G200SE
  drm/mgag200: Declare PLL clock constants static const
  drm/mgag200: Introduce custom CRTC state
  drm/mgag200: Compute PLL values during atomic check

 drivers/gpu/drm/drm_simple_kms_helper.c |  39 +-
 drivers/gpu/drm/mgag200/mgag200_drv.h   |  28 +
 drivers/gpu/drm/mgag200/mgag200_mode.c  | 965 +++++++++++++++---------
 include/drm/drm_simple_kms_helper.h     |  27 +
 4 files changed, 720 insertions(+), 339 deletions(-)

--
2.32.0

[PATCH 01/12] drm/mgag200: Select clock in PLL update functions

[Thomas Zimmermann]

Put the clock-selection code into each of the PLL-update functions to
make them select the correct pixel clock.

The pixel clock for video output was not actually set before programming
the clock's values. It worked because the device had the correct clock
pre-set.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O")
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.9+
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 3b3059f471c2..482843ebb69f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -130,6 +130,7 @@ 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);
@@ -174,6 +175,11 @@ 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);
 
+	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)));
@@ -194,6 +200,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 	unsigned int pvalues_e4[P_ARRAY_SIZE] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
 	unsigned int fvv;
 	unsigned int i;
+	u8 misc;
 
 	if (unique_rev_id <= 0x03) {
 
@@ -289,6 +296,11 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 		return 1;
 	}
 
+	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);
@@ -312,7 +324,7 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock)
 	unsigned int computed;
 	int i, j, tmpcount, vcount;
 	bool pll_locked = false;
-	u8 tmp;
+	u8 tmp, misc;
 
 	m = n = p = 0;
 
@@ -385,6 +397,11 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock)
 		}
 	}
 
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	for (i = 0; i <= 32 && pll_locked == false; i++) {
 		if (i > 0) {
 			WREG8(MGAREG_CRTC_INDEX, 0x1e);
@@ -489,7 +506,7 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock)
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n;
 	unsigned int computed;
-	u8 tmp;
+	u8 tmp, misc;
 
 	m = n = p = 0;
 	vcomax = 550000;
@@ -522,6 +539,11 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock)
 		}
 	}
 
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
 	tmp = RREG8(DAC_DATA);
 	tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -583,7 +605,7 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock)
 	unsigned int p, m, n;
 	unsigned int computed;
 	int i, j, tmpcount, vcount;
-	u8 tmp;
+	u8 tmp, misc;
 	bool pll_locked = false;
 
 	m = n = p = 0;
@@ -654,6 +676,12 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock)
 			}
 		}
 	}
+
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	for (i = 0; i <= 32 && pll_locked == false; i++) {
 		WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
 		tmp = RREG8(DAC_DATA);
@@ -714,6 +742,7 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
 	unsigned int p, m, n;
 	unsigned int computed, vco;
 	int tmp;
+	u8 misc;
 
 	m = n = p = 0;
 	vcomax = 1488000;
@@ -754,6 +783,11 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
 		}
 	}
 
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
 	tmp = RREG8(DAC_DATA);
 	tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -787,8 +821,6 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
 
 static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
 {
-	u8 misc;
-
 	switch(mdev->type) {
 	case G200_PCI:
 	case G200_AGP:
@@ -808,11 +840,6 @@ static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
 		return mga_g200er_set_plls(mdev, clock);
 	}
 
-	misc = RREG8(MGA_MISC_IN);
-	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
-	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
-	WREG8(MGA_MISC_OUT, misc);
-
 	return 0;
 }
 
-- 
2.32.0

[PATCH 01/12] drm/mgag200: Select clock in PLL update functions

[Thomas Zimmermann]

Put the clock-selection code into each of the PLL-update functions to
make them select the correct pixel clock.

The pixel clock for video output was not actually set before programming
the clock's values. It worked because the device had the correct clock
pre-set.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O")
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.9+
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 3b3059f471c2..482843ebb69f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -130,6 +130,7 @@ 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);
@@ -174,6 +175,11 @@ 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);
 
+	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)));
@@ -194,6 +200,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 	unsigned int pvalues_e4[P_ARRAY_SIZE] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
 	unsigned int fvv;
 	unsigned int i;
+	u8 misc;
 
 	if (unique_rev_id <= 0x03) {
 
@@ -289,6 +296,11 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 		return 1;
 	}
 
+	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);
@@ -312,7 +324,7 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock)
 	unsigned int computed;
 	int i, j, tmpcount, vcount;
 	bool pll_locked = false;
-	u8 tmp;
+	u8 tmp, misc;
 
 	m = n = p = 0;
 
@@ -385,6 +397,11 @@ static int mga_g200wb_set_plls(struct mga_device *mdev, long clock)
 		}
 	}
 
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	for (i = 0; i <= 32 && pll_locked == false; i++) {
 		if (i > 0) {
 			WREG8(MGAREG_CRTC_INDEX, 0x1e);
@@ -489,7 +506,7 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock)
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n;
 	unsigned int computed;
-	u8 tmp;
+	u8 tmp, misc;
 
 	m = n = p = 0;
 	vcomax = 550000;
@@ -522,6 +539,11 @@ static int mga_g200ev_set_plls(struct mga_device *mdev, long clock)
 		}
 	}
 
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
 	tmp = RREG8(DAC_DATA);
 	tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -583,7 +605,7 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock)
 	unsigned int p, m, n;
 	unsigned int computed;
 	int i, j, tmpcount, vcount;
-	u8 tmp;
+	u8 tmp, misc;
 	bool pll_locked = false;
 
 	m = n = p = 0;
@@ -654,6 +676,12 @@ static int mga_g200eh_set_plls(struct mga_device *mdev, long clock)
 			}
 		}
 	}
+
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	for (i = 0; i <= 32 && pll_locked == false; i++) {
 		WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
 		tmp = RREG8(DAC_DATA);
@@ -714,6 +742,7 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
 	unsigned int p, m, n;
 	unsigned int computed, vco;
 	int tmp;
+	u8 misc;
 
 	m = n = p = 0;
 	vcomax = 1488000;
@@ -754,6 +783,11 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
 		}
 	}
 
+	misc = RREG8(MGA_MISC_IN);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
 	WREG8(DAC_INDEX, MGA1064_PIX_CLK_CTL);
 	tmp = RREG8(DAC_DATA);
 	tmp |= MGA1064_PIX_CLK_CTL_CLK_DIS;
@@ -787,8 +821,6 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
 
 static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
 {
-	u8 misc;
-
 	switch(mdev->type) {
 	case G200_PCI:
 	case G200_AGP:
@@ -808,11 +840,6 @@ static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
 		return mga_g200er_set_plls(mdev, clock);
 	}
 
-	misc = RREG8(MGA_MISC_IN);
-	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
-	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
-	WREG8(MGA_MISC_OUT, misc);
-
 	return 0;
 }
 
-- 
2.32.0

Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions

[Sam Ravnborg]

Hi Thomas,

On Mon, Jul 05, 2021 at 02:45:04PM +0200, Thomas Zimmermann wrote:
> Put the clock-selection code into each of the PLL-update functions to
> make them select the correct pixel clock.
> 
> The pixel clock for video output was not actually set before programming
> the clock's values. It worked because the device had the correct clock
> pre-set.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O")
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.9+
> ---
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 3b3059f471c2..482843ebb69f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -130,6 +130,7 @@ 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);
> @@ -174,6 +175,11 @@ 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);
>  
> +	misc = RREG8(MGA_MISC_IN);
> +	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
> +	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
> +	WREG8(MGA_MISC_OUT, misc);

This chunk is repeated a number of times.
Any good reason why this is not a small helper?

	Sam

Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2296 bytes --]

Hi

Am 09.07.21 um 20:50 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Mon, Jul 05, 2021 at 02:45:04PM +0200, Thomas Zimmermann wrote:
>> Put the clock-selection code into each of the PLL-update functions to
>> make them select the correct pixel clock.
>>
>> The pixel clock for video output was not actually set before programming
>> the clock's values. It worked because the device had the correct clock
>> pre-set.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O")
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Emil Velikov <emil.velikov@collabora.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v5.9+
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 3b3059f471c2..482843ebb69f 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -130,6 +130,7 @@ 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);
>> @@ -174,6 +175,11 @@ 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);
>>   
>> +	misc = RREG8(MGA_MISC_IN);
>> +	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
>> +	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
>> +	WREG8(MGA_MISC_OUT, misc);
> 
> This chunk is repeated a number of times.
> Any good reason why this is not a small helper?

Good point. I'll make a helper from this.

Best regards
Thomas

> 
> 	Sam
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2296 bytes --]

Hi

Am 09.07.21 um 20:50 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Mon, Jul 05, 2021 at 02:45:04PM +0200, Thomas Zimmermann wrote:
>> Put the clock-selection code into each of the PLL-update functions to
>> make them select the correct pixel clock.
>>
>> The pixel clock for video output was not actually set before programming
>> the clock's values. It worked because the device had the correct clock
>> pre-set.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O")
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Emil Velikov <emil.velikov@collabora.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v5.9+
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 3b3059f471c2..482843ebb69f 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -130,6 +130,7 @@ 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);
>> @@ -174,6 +175,11 @@ 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);
>>   
>> +	misc = RREG8(MGA_MISC_IN);
>> +	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
>> +	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
>> +	WREG8(MGA_MISC_OUT, misc);
> 
> This chunk is repeated a number of times.
> Any good reason why this is not a small helper?

Good point. I'll make a helper from this.

Best regards
Thomas

> 
> 	Sam
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions

[Thomas Zimmermann]

Return -EINVAL if there's no PLL configuration for the given pixel
clock.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 482843ebb69f..045a20055515 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -134,7 +134,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
 
 	if (clock > p_clk_max) {
 		drm_err(dev, "Pixel Clock %ld too high\n", clock);
-		return 1;
+		return -EINVAL;
 	}
 
 	if (clock < p_clk_min >> 3)
@@ -293,7 +293,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 
 	if (delta > permitteddelta) {
 		pr_warn("PLL delta too large\n");
-		return 1;
+		return -EINVAL;
 	}
 
 	misc = RREG8(MGA_MISC_IN);
-- 
2.32.0

Re: [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions

[Sam Ravnborg]

On Mon, Jul 05, 2021 at 02:45:05PM +0200, Thomas Zimmermann wrote:
> Return -EINVAL if there's no PLL configuration for the given pixel
> clock.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 482843ebb69f..045a20055515 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -134,7 +134,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
>  
>  	if (clock > p_clk_max) {
>  		drm_err(dev, "Pixel Clock %ld too high\n", clock);
> -		return 1;
> +		return -EINVAL;
>  	}
>  
>  	if (clock < p_clk_min >> 3)
> @@ -293,7 +293,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
>  
>  	if (delta > permitteddelta) {
>  		pr_warn("PLL delta too large\n");
> -		return 1;
> +		return -EINVAL;
>  	}
>  
>  	misc = RREG8(MGA_MISC_IN);

The return value is ignored but I assume it makes sense in a later
patch. Should mgag200_crtc_set_plls() return -EINVAL if there was no
match? Today it returns 0 - which is not an error.

	Sam

Re: [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1856 bytes --]

Hi

Am 09.07.21 um 20:53 schrieb Sam Ravnborg:
> On Mon, Jul 05, 2021 at 02:45:05PM +0200, Thomas Zimmermann wrote:
>> Return -EINVAL if there's no PLL configuration for the given pixel
>> clock.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 482843ebb69f..045a20055515 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -134,7 +134,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
>>   
>>   	if (clock > p_clk_max) {
>>   		drm_err(dev, "Pixel Clock %ld too high\n", clock);
>> -		return 1;
>> +		return -EINVAL;
>>   	}
>>   
>>   	if (clock < p_clk_min >> 3)
>> @@ -293,7 +293,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
>>   
>>   	if (delta > permitteddelta) {
>>   		pr_warn("PLL delta too large\n");
>> -		return 1;
>> +		return -EINVAL;
>>   	}
>>   
>>   	misc = RREG8(MGA_MISC_IN);
> 
> The return value is ignored but I assume it makes sense in a later
> patch. Should mgag200_crtc_set_plls() return -EINVAL if there was no
> match? Today it returns 0 - which is not an error.

Indeed. Patch 12 moves some of this functionality into the atomic check, 
where it will be tested for success.

Not handling the type in the switch is actually a driver bug. I'll see 
if I can add a rsp error in one of the patches.

Best regards
Thomas

> 
> 	Sam
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH 03/12] drm/mgag200: Remove P_ARRAY_SIZE

[Thomas Zimmermann]

Replace P_ARRAY_SIZE by array pre-initializing and ARRAY_SIZE(). No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 045a20055515..fa06f1994d68 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -187,17 +187,16 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
 	return 0;
 }
 
-#define P_ARRAY_SIZE 9
-
 static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 {
+	static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
+
 	u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n;
 	unsigned int computed;
-	unsigned int pvalues_e4[P_ARRAY_SIZE] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
 	unsigned int fvv;
 	unsigned int i;
 	u8 misc;
@@ -252,7 +251,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 		/* Permited delta is 0.5% as VESA Specification */
 		permitteddelta = clock * 5 / 1000;
 
-		for (i = 0 ; i < P_ARRAY_SIZE ; i++) {
+		for (i = 0 ; i < ARRAY_SIZE(pvalues_e4); i++) {
 			testp = pvalues_e4[i];
 
 			if ((clock * testp) > vcomax)
-- 
2.32.0

Re: [PATCH 03/12] drm/mgag200: Remove P_ARRAY_SIZE

[Sam Ravnborg]

On Mon, Jul 05, 2021 at 02:45:06PM +0200, Thomas Zimmermann wrote:
> Replace P_ARRAY_SIZE by array pre-initializing and ARRAY_SIZE(). No
> functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Sam Ravnborg <sam@ravnborg.org>

[PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions

[Thomas Zimmermann]

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;
+	unsigned int n;
+	unsigned int p;
+	unsigned int s;
+};
+
 #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;
+
 	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);
 }
 
-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);
+	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);
+
 	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);
+
 	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);
+
 	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)
 {
+	struct mgag200_pll_values pixpll;
+	int ret;
+
 	switch(mdev->type) {
 	case G200_PCI:
 	case G200_AGP:
-		return mgag200_g200_set_plls(mdev, clock);
+		ret = mgag200_compute_pixpll_values_g200(mdev, clock, &pixpll);
+		break;
 	case G200_SE_A:
 	case G200_SE_B:
-		return mga_g200se_set_plls(mdev, clock);
+		ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll);
+		break;
 	case G200_WB:
 	case G200_EW3:
-		return mga_g200wb_set_plls(mdev, clock);
+		ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll);
+		break;
 	case G200_EV:
-		return mga_g200ev_set_plls(mdev, clock);
+		ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll);
+		break;
 	case G200_EH:
 	case G200_EH3:
-		return mga_g200eh_set_plls(mdev, clock);
+		ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll);
+		break;
 	case G200_ER:
-		return mga_g200er_set_plls(mdev, clock);
+		ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll);
+		break;
 	}
 
-	return 0;
+	if (ret)
+		return;
+
+	switch (mdev->type) {
+	case G200_PCI:
+	case G200_AGP:
+		mgag200_set_pixpll_g200(mdev, &pixpll);
+		break;
+	case G200_SE_A:
+	case G200_SE_B:
+		mgag200_set_pixpll_g200se(mdev, &pixpll);
+		break;
+	case G200_WB:
+	case G200_EW3:
+		mgag200_set_pixpll_g200wb(mdev, &pixpll);
+		break;
+	case G200_EV:
+		mgag200_set_pixpll_g200ev(mdev, &pixpll);
+		break;
+	case G200_EH:
+	case G200_EH3:
+		mgag200_set_pixpll_g200eh(mdev, &pixpll);
+		break;
+	case G200_ER:
+		mgag200_set_pixpll_g200er(mdev, &pixpll);
+		break;
+	}
 }
 
 static void mgag200_g200wb_hold_bmc(struct mga_device *mdev)
-- 
2.32.0

Re: [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions

[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;
> +};
> +
>  #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.
> +
>  	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));


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.


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


> +	struct mgag200_pll_values pixpll;
> +	int ret;
> +
>  	switch(mdev->type) {
>  	case G200_PCI:
>  	case G200_AGP:
> -		return mgag200_g200_set_plls(mdev, clock);
> +		ret = mgag200_compute_pixpll_values_g200(mdev, clock, &pixpll);
> +		break;
>  	case G200_SE_A:
>  	case G200_SE_B:
> -		return mga_g200se_set_plls(mdev, clock);
> +		ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll);
> +		break;
>  	case G200_WB:
>  	case G200_EW3:
> -		return mga_g200wb_set_plls(mdev, clock);
> +		ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll);
> +		break;
>  	case G200_EV:
> -		return mga_g200ev_set_plls(mdev, clock);
> +		ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll);
> +		break;
>  	case G200_EH:
>  	case G200_EH3:
> -		return mga_g200eh_set_plls(mdev, clock);
> +		ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll);
> +		break;
>  	case G200_ER:
> -		return mga_g200er_set_plls(mdev, clock);
> +		ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll);
> +		break;
>  	}
>  
> -	return 0;
> +	if (ret)
> +		return;
> +
> +	switch (mdev->type) {
> +	case G200_PCI:
> +	case G200_AGP:
> +		mgag200_set_pixpll_g200(mdev, &pixpll);
> +		break;
> +	case G200_SE_A:
> +	case G200_SE_B:
> +		mgag200_set_pixpll_g200se(mdev, &pixpll);
> +		break;
> +	case G200_WB:
> +	case G200_EW3:
> +		mgag200_set_pixpll_g200wb(mdev, &pixpll);
> +		break;
> +	case G200_EV:
> +		mgag200_set_pixpll_g200ev(mdev, &pixpll);
> +		break;
> +	case G200_EH:
> +	case G200_EH3:
> +		mgag200_set_pixpll_g200eh(mdev, &pixpll);
> +		break;
> +	case G200_ER:
> +		mgag200_set_pixpll_g200er(mdev, &pixpll);
> +		break;
> +	}
>  }
>  
>  static void mgag200_g200wb_hold_bmc(struct mga_device *mdev)
> -- 
> 2.32.0

Re: [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 16394 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH 05/12] drm/mgag200: Introduce separate variable for PLL S parameter

[Thomas Zimmermann]

The S parameter is controls the loop filter bandwidth when programming
the PLL. It's currently stored as part of P (i.e., the clock divider.)
Add a separate variable for S prepares the PLL code for a further
refactoring.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 36 ++++++++++++--------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 961bd128fea3..9f6fe7673674 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -210,18 +210,17 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
-	unsigned int p, m, n;
+	unsigned int p, m, n, s;
 	unsigned int computed;
 	unsigned int fvv;
 	unsigned int i;
 
-	if (unique_rev_id <= 0x03) {
+	m = n = p = s = 0;
 
-		m = n = p = 0;
+	if (unique_rev_id <= 0x03) {
 		vcomax = 320000;
 		vcomin = 160000;
 		pllreffreq = 25000;
-
 		delta = 0xffffffff;
 		permitteddelta = clock * 5 / 1000;
 
@@ -249,9 +248,6 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 			}
 		}
 	} else {
-
-
-		m = n = p = 0;
 		vcomax        = 1600000;
 		vcomin        = 800000;
 		pllreffreq    = 25000;
@@ -312,7 +308,7 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
-	pixpllc->s = 0;
+	pixpllc->s = s;
 
 	return 0;
 }
@@ -348,10 +344,10 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn, testp2;
-	unsigned int p, m, n;
+	unsigned int p, m, n, s;
 	unsigned int computed;
 
-	m = n = p = 0;
+	m = n = p = s = 0;
 
 	delta = 0xffffffff;
 
@@ -425,7 +421,7 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
-	pixpllc->s = 0;
+	pixpllc->s = s;
 
 	return 0;
 }
@@ -549,10 +545,10 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn;
-	unsigned int p, m, n;
+	unsigned int p, m, n, s;
 	unsigned int computed;
 
-	m = n = p = 0;
+	m = n = p = s = 0;
 	vcomax = 550000;
 	vcomin = 150000;
 	pllreffreq = 50000;
@@ -586,7 +582,7 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
-	pixpllc->s = 0;
+	pixpllc->s = s;
 
 	return 0;
 }
@@ -662,10 +658,10 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn;
-	unsigned int p, m, n;
+	unsigned int p, m, n, s;
 	unsigned int computed;
 
-	m = n = p = 0;
+	m = n = p = s = 0;
 
 	if (mdev->type == G200_EH3) {
 		vcomax = 3000000;
@@ -737,7 +733,7 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
-	pixpllc->s = 0;
+	pixpllc->s = s;
 
 	return 0;
 }
@@ -814,10 +810,10 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta;
 	int testr, testn, testm, testo;
-	unsigned int p, m, n;
+	unsigned int p, m, n, s;
 	unsigned int computed, vco;
 
-	m = n = p = 0;
+	m = n = p = s = 0;
 	vcomax = 1488000;
 	vcomin = 1056000;
 	pllreffreq = 48000;
@@ -859,7 +855,7 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
-	pixpllc->s = 0;
+	pixpllc->s = s;
 
 	return 0;
 }
-- 
2.32.0

Re: [PATCH 05/12] drm/mgag200: Introduce separate variable for PLL S parameter

[Sam Ravnborg]

On Mon, Jul 05, 2021 at 02:45:08PM +0200, Thomas Zimmermann wrote:
> The S parameter is controls the loop filter bandwidth when programming
> the PLL. It's currently stored as part of P (i.e., the clock divider.)
> Add a separate variable for S prepares the PLL code for a further
> refactoring.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

I guess I will soon learn why they all had s << 3.

This patch is
Acked-by: Sam Ravnborg <sam@ravnborg.org>

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

[Thomas Zimmermann]

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

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

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

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

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 13835 bytes --]

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

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Sam Ravnborg]

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

[PATCH 07/12] drm/mgag200: Split several PLL functions by device type

[Thomas Zimmermann]

Several PLL functions compute values for different device types. Split
them up to make the code more readable. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 256 ++++++++++++++-----------
 1 file changed, 146 insertions(+), 110 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 7d6707bd6c27..d3b15e60f057 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -353,7 +353,7 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 {
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta;
-	unsigned int testp, testm, testn, testp2;
+	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
 	unsigned int computed;
 
@@ -361,64 +361,29 @@ 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;
-
-		for (testp = 1; testp < 8; testp++) {
-			for (testp2 = 1; testp2 < 8; testp2++) {
-				if (testp < testp2)
-					continue;
-				if ((clock * testp * testp2) > vcomax)
-					continue;
-				if ((clock * testp * testp2) < vcomin)
-					continue;
-				for (testm = 1; testm < 26; testm++) {
-					for (testn = 32; testn < 2048 ; testn++) {
-						computed = (pllreffreq * testn) /
-							(testm * testp * testp2);
-						if (computed > clock)
-							tmpdelta = computed - clock;
-						else
-							tmpdelta = clock - computed;
-						if (tmpdelta < delta) {
-							delta = tmpdelta;
-							m = testm + 1;
-							n = testn + 1;
-							p = testp + 1;
-							s = testp2;
-						}
-					}
-				}
-			}
-		}
-	} else {
-		vcomax = 550000;
-		vcomin = 150000;
-		pllreffreq = 48000;
+	vcomax = 550000;
+	vcomin = 150000;
+	pllreffreq = 48000;
 
-		for (testp = 1; testp < 9; testp++) {
-			if (clock * testp > vcomax)
-				continue;
-			if (clock * testp < vcomin)
-				continue;
+	for (testp = 1; testp < 9; testp++) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
 
-			for (testm = 1; testm < 17; testm++) {
-				for (testn = 1; testn < 151; testn++) {
-					computed = (pllreffreq * testn) /
-						(testm * testp);
-					if (computed > clock)
-						tmpdelta = computed - clock;
-					else
-						tmpdelta = clock - computed;
-					if (tmpdelta < delta) {
-						delta = tmpdelta;
-						n = testn;
-						m = testm;
-						p = testp;
-						s = 0;
-					}
+		for (testm = 1; testm < 17; testm++) {
+			for (testn = 1; testn < 151; testn++) {
+				computed = (pllreffreq * testn) / (testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+				if (tmpdelta < delta) {
+					delta = tmpdelta;
+					n = testn;
+					m = testm;
+					p = testp;
+					s = 0;
 				}
 			}
 		}
@@ -681,66 +646,30 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl
 
 	m = n = p = s = 0;
 
-	if (mdev->type == G200_EH3) {
-		vcomax = 3000000;
-		vcomin = 1500000;
-		pllreffreq = 25000;
+	vcomax = 800000;
+	vcomin = 400000;
+	pllreffreq = 33333;
 
-		delta = 0xffffffff;
+	delta = 0xffffffff;
 
-		testp = 0;
+	for (testp = 16; testp > 0; testp >>= 1) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
 
-		for (testm = 150; testm >= 6; testm--) {
-			if (clock * testm > vcomax)
-				continue;
-			if (clock * testm < vcomin)
-				continue;
-			for (testn = 120; testn >= 60; testn--) {
-				computed = (pllreffreq * testn) / testm;
+		for (testm = 1; testm < 33; testm++) {
+			for (testn = 17; testn < 257; testn++) {
+				computed = (pllreffreq * testn) / (testm * testp);
 				if (computed > clock)
 					tmpdelta = computed - clock;
 				else
 					tmpdelta = clock - computed;
 				if (tmpdelta < delta) {
 					delta = tmpdelta;
-					n = testn + 1;
-					m = testm + 1;
-					p = testp + 1;
-				}
-				if (delta == 0)
-					break;
-			}
-			if (delta == 0)
-				break;
-		}
-	} else {
-
-		vcomax = 800000;
-		vcomin = 400000;
-		pllreffreq = 33333;
-
-		delta = 0xffffffff;
-
-		for (testp = 16; testp > 0; testp >>= 1) {
-			if (clock * testp > vcomax)
-				continue;
-			if (clock * testp < vcomin)
-				continue;
-
-			for (testm = 1; testm < 33; testm++) {
-				for (testn = 17; testn < 257; testn++) {
-					computed = (pllreffreq * testn) /
-						(testm * testp);
-					if (computed > clock)
-						tmpdelta = computed - clock;
-					else
-						tmpdelta = clock - computed;
-					if (tmpdelta < delta) {
-						delta = tmpdelta;
-						n = testn;
-						m = testm;
-						p = testp;
-					}
+					n = testn;
+					m = testm;
+					p = testp;
 				}
 			}
 		}
@@ -825,6 +754,57 @@ static void mgag200_set_pixpll_g200eh(struct mga_device *mdev,
 	}
 }
 
+static int mgag200_compute_pixpll_values_g200eh3(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, s;
+	unsigned int computed;
+
+	m = n = p = s = 0;
+
+	vcomax = 3000000;
+	vcomin = 1500000;
+	pllreffreq = 25000;
+
+	delta = 0xffffffff;
+
+	testp = 0;
+
+	for (testm = 150; testm >= 6; testm--) {
+		if (clock * testm > vcomax)
+			continue;
+		if (clock * testm < vcomin)
+			continue;
+		for (testn = 120; testn >= 60; testn--) {
+			computed = (pllreffreq * testn) / testm;
+			if (computed > clock)
+				tmpdelta = computed - clock;
+			else
+				tmpdelta = clock - computed;
+			if (tmpdelta < delta) {
+				delta = tmpdelta;
+				n = testn + 1;
+				m = testm + 1;
+				p = testp + 1;
+			}
+			if (delta == 0)
+				break;
+		}
+		if (delta == 0)
+			break;
+	}
+
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
+
+	return 0;
+}
+
 static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long clock,
 						struct mgag200_pll_values *pixpllc)
 {
@@ -932,6 +912,58 @@ static void mgag200_set_pixpll_g200er(struct mga_device *mdev,
 	udelay(50);
 }
 
+static int mgag200_compute_pixpll_values_g200ew3(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, s;
+	unsigned int computed;
+
+	m = n = p = s = 0;
+
+	delta = 0xffffffff;
+
+	vcomax = 800000;
+	vcomin = 400000;
+	pllreffreq = 25000;
+
+	for (testp = 1; testp < 8; testp++) {
+		for (testp2 = 1; testp2 < 8; testp2++) {
+			if (testp < testp2)
+				continue;
+			if ((clock * testp * testp2) > vcomax)
+				continue;
+			if ((clock * testp * testp2) < vcomin)
+				continue;
+			for (testm = 1; testm < 26; testm++) {
+				for (testn = 32; testn < 2048 ; testn++) {
+					computed = (pllreffreq * testn) / (testm * testp * testp2);
+					if (computed > clock)
+						tmpdelta = computed - clock;
+					else
+						tmpdelta = clock - computed;
+					if (tmpdelta < delta) {
+						delta = tmpdelta;
+						m = testm + 1;
+						n = testn + 1;
+						p = testp + 1;
+						s = testp2;
+					}
+				}
+			}
+		}
+	}
+
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
+
+	return 0;
+}
+
 static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
 {
 	struct mgag200_pll_values pixpll;
@@ -947,19 +979,23 @@ static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
 		ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll);
 		break;
 	case G200_WB:
-	case G200_EW3:
 		ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll);
 		break;
 	case G200_EV:
 		ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll);
 		break;
 	case G200_EH:
-	case G200_EH3:
 		ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll);
 		break;
+	case G200_EH3:
+		ret = mgag200_compute_pixpll_values_g200eh3(mdev, clock, &pixpll);
+		break;
 	case G200_ER:
 		ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll);
 		break;
+	case G200_EW3:
+		ret = mgag200_compute_pixpll_values_g200ew3(mdev, clock, &pixpll);
+		break;
 	}
 
 	if (ret)
-- 
2.32.0

[PATCH 08/12] drm/mgag200: Separate PLL compute and update functions from each other

[Thomas Zimmermann]

Move PLL compute and update functionality to distict places within
the file. Contains some minor style cleanups, but no functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 737 +++++++++++++------------
 1 file changed, 369 insertions(+), 368 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index d3b15e60f057..72fdf242cac7 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -184,30 +184,6 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc
 	return 0;
 }
 
-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);
-	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
-	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
-	WREG8(MGA_MISC_OUT, misc);
-
-	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);
-}
-
 static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock,
 						struct mgag200_pll_values *pixpllc)
 {
@@ -223,12 +199,12 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 	unsigned int i;
 
 	m = n = p = s = 0;
+	delta = 0xffffffff;
 
 	if (unique_rev_id <= 0x03) {
 		vcomax = 320000;
 		vcomin = 160000;
 		pllreffreq = 25000;
-		delta = 0xffffffff;
 		permitteddelta = clock * 5 / 1000;
 
 		for (testp = 8; testp > 0; testp /= 2) {
@@ -261,10 +237,8 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 
 		if (clock < 25000)
 			clock = 25000;
-
 		clock = clock * 2;
 
-		delta = 0xFFFFFFFF;
 		/* Permited delta is 0.5% as VESA Specification */
 		permitteddelta = clock * 5 / 1000;
 
@@ -317,38 +291,55 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 	return 0;
 }
 
-static void mgag200_set_pixpll_g200se(struct mga_device *mdev,
-				      const struct mgag200_pll_values *pixpllc)
+static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock,
+						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;
+	unsigned int vcomax, vcomin, pllreffreq;
+	unsigned int delta, tmpdelta;
+	unsigned int testp, testm, testn;
+	unsigned int p, m, n, s;
+	unsigned int computed;
 
-	misc = RREG8(MGA_MISC_IN);
-	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
-	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
-	WREG8(MGA_MISC_OUT, misc);
+	m = n = p = s = 0;
+	delta = 0xffffffff;
 
-	pixpllcm = pixpllc->m - 1;
-	pixpllcn = pixpllc->n - 1;
-	pixpllcp = pixpllc->p - 1;
-	pixpllcs = pixpllc->s;
+	vcomax = 550000;
+	vcomin = 150000;
+	pllreffreq = 48000;
 
-	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);
+	for (testp = 1; testp < 9; testp++) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
 
-	if (unique_rev_id >= 0x04) {
-		WREG_DAC(0x1a, 0x09);
-		msleep(20);
-		WREG_DAC(0x1a, 0x01);
+		for (testm = 1; testm < 17; testm++) {
+			for (testn = 1; testn < 151; testn++) {
+				computed = (pllreffreq * testn) / (testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+				if (tmpdelta < delta) {
+					delta = tmpdelta;
+					n = testn;
+					m = testm;
+					p = testp;
+					s = 0;
+				}
+			}
+		}
 	}
+
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
+
+	return 0;
 }
 
-static int mgag200_compute_pixpll_values_g200wb(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;
@@ -358,21 +349,68 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 	unsigned int computed;
 
 	m = n = p = s = 0;
-
 	delta = 0xffffffff;
 
 	vcomax = 550000;
 	vcomin = 150000;
-	pllreffreq = 48000;
+	pllreffreq = 50000;
 
-	for (testp = 1; testp < 9; testp++) {
+	for (testp = 16; testp > 0; testp--) {
 		if (clock * testp > vcomax)
 			continue;
 		if (clock * testp < vcomin)
 			continue;
 
-		for (testm = 1; testm < 17; testm++) {
-			for (testn = 1; testn < 151; testn++) {
+		for (testn = 1; testn < 257; testn++) {
+			for (testm = 1; testm < 17; testm++) {
+				computed = (pllreffreq * testn) /
+					(testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+				if (tmpdelta < delta) {
+					delta = tmpdelta;
+					n = testn;
+					m = testm;
+					p = testp;
+				}
+			}
+		}
+	}
+
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
+
+	return 0;
+}
+
+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, s;
+	unsigned int computed;
+
+	m = n = p = s = 0;
+	delta = 0xffffffff;
+
+	vcomax = 800000;
+	vcomin = 400000;
+	pllreffreq = 33333;
+
+	for (testp = 16; testp > 0; testp >>= 1) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
+
+		for (testm = 1; testm < 33; testm++) {
+			for (testn = 17; testn < 257; testn++) {
 				computed = (pllreffreq * testn) / (testm * testp);
 				if (computed > clock)
 					tmpdelta = computed - clock;
@@ -383,7 +421,6 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 					n = testn;
 					m = testm;
 					p = testp;
-					s = 0;
 				}
 			}
 		}
@@ -397,6 +434,256 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 	return 0;
 }
 
+static int mgag200_compute_pixpll_values_g200eh3(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, s;
+	unsigned int computed;
+
+	m = n = p = s = 0;
+	delta = 0xffffffff;
+	testp = 0;
+
+	vcomax = 3000000;
+	vcomin = 1500000;
+	pllreffreq = 25000;
+
+	for (testm = 150; testm >= 6; testm--) {
+		if (clock * testm > vcomax)
+			continue;
+		if (clock * testm < vcomin)
+			continue;
+		for (testn = 120; testn >= 60; testn--) {
+			computed = (pllreffreq * testn) / testm;
+			if (computed > clock)
+				tmpdelta = computed - clock;
+			else
+				tmpdelta = clock - computed;
+			if (tmpdelta < delta) {
+				delta = tmpdelta;
+				n = testn + 1;
+				m = testm + 1;
+				p = testp + 1;
+			}
+			if (delta == 0)
+				break;
+		}
+		if (delta == 0)
+			break;
+	}
+
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
+
+	return 0;
+}
+
+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;
+	unsigned int delta, tmpdelta;
+	int testr, testn, testm, testo;
+	unsigned int p, m, n, s;
+	unsigned int computed, vco;
+
+	vcomax = 1488000;
+	vcomin = 1056000;
+	pllreffreq = 48000;
+
+	m = n = p = s = 0;
+	delta = 0xffffffff;
+
+	for (testr = 0; testr < 4; testr++) {
+		if (delta == 0)
+			break;
+		for (testn = 5; testn < 129; testn++) {
+			if (delta == 0)
+				break;
+			for (testm = 3; testm >= 0; testm--) {
+				if (delta == 0)
+					break;
+				for (testo = 5; testo < 33; testo++) {
+					vco = pllreffreq * (testn + 1) /
+						(testr + 1);
+					if (vco < vcomin)
+						continue;
+					if (vco > vcomax)
+						continue;
+					computed = vco / (m_div_val[testm] * (testo + 1));
+					if (computed > clock)
+						tmpdelta = computed - clock;
+					else
+						tmpdelta = clock - computed;
+					if (tmpdelta < delta) {
+						delta = tmpdelta;
+						m = (testm | (testo << 3)) + 1;
+						n = testn + 1;
+						p = testr + 1;
+						s = testr;
+					}
+				}
+			}
+		}
+	}
+
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
+
+	return 0;
+}
+
+static int mgag200_compute_pixpll_values_g200ew3(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, s;
+	unsigned int computed;
+
+	m = n = p = s = 0;
+	delta = 0xffffffff;
+
+	vcomax = 800000;
+	vcomin = 400000;
+	pllreffreq = 25000;
+
+	for (testp = 1; testp < 8; testp++) {
+		for (testp2 = 1; testp2 < 8; testp2++) {
+			if (testp < testp2)
+				continue;
+			if ((clock * testp * testp2) > vcomax)
+				continue;
+			if ((clock * testp * testp2) < vcomin)
+				continue;
+			for (testm = 1; testm < 26; testm++) {
+				for (testn = 32; testn < 2048 ; testn++) {
+					computed = (pllreffreq * testn) / (testm * testp * testp2);
+					if (computed > clock)
+						tmpdelta = computed - clock;
+					else
+						tmpdelta = clock - computed;
+					if (tmpdelta < delta) {
+						delta = tmpdelta;
+						m = testm + 1;
+						n = testn + 1;
+						p = testp + 1;
+						s = testp2;
+					}
+				}
+			}
+		}
+	}
+
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
+
+	return 0;
+}
+
+static int mgag200_compute_pixpll_values(struct mga_device *mdev, long clock,
+					 struct mgag200_pll_values *pixpll)
+{
+	int ret;
+
+	switch (mdev->type) {
+	case G200_PCI:
+	case G200_AGP:
+		ret = mgag200_compute_pixpll_values_g200(mdev, clock, pixpll);
+		break;
+	case G200_SE_A:
+	case G200_SE_B:
+		ret = mgag200_compute_pixpll_values_g200se(mdev, clock, pixpll);
+		break;
+	case G200_WB:
+		ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, pixpll);
+		break;
+	case G200_EV:
+		ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, pixpll);
+		break;
+	case G200_EH:
+		ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, pixpll);
+		break;
+	case G200_EH3:
+		ret = mgag200_compute_pixpll_values_g200eh3(mdev, clock, pixpll);
+		break;
+	case G200_ER:
+		ret = mgag200_compute_pixpll_values_g200er(mdev, clock, pixpll);
+		break;
+	case G200_EW3:
+		ret = mgag200_compute_pixpll_values_g200ew3(mdev, clock, pixpll);
+		break;
+	}
+
+	return ret;
+}
+
+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);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
+	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);
+}
+
+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);
+	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
+	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
+	WREG8(MGA_MISC_OUT, misc);
+
+	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);
+
+	if (unique_rev_id >= 0x04) {
+		WREG_DAC(0x1a, 0x09);
+		msleep(20);
+		WREG_DAC(0x1a, 0x01);
+	}
+}
+
 static void mgag200_set_pixpll_g200wb(struct mga_device *mdev,
 				      const struct mgag200_pll_values *pixpllc)
 {
@@ -500,68 +787,20 @@ static void mgag200_set_pixpll_g200wb(struct mga_device *mdev,
 		vcount = RREG8(MGAREG_VCOUNT);
 
 		for (j = 0; j < 30 && pll_locked == false; j++) {
-			tmpcount = RREG8(MGAREG_VCOUNT);
-			if (tmpcount < vcount)
-				vcount = 0;
-			if ((tmpcount - vcount) > 2)
-				pll_locked = true;
-			else
-				udelay(5);
-		}
-	}
-
-	WREG8(DAC_INDEX, MGA1064_REMHEADCTL);
-	tmp = RREG8(DAC_DATA);
-	tmp &= ~MGA1064_REMHEADCTL_CLKDIS;
-	WREG_DAC(MGA1064_REMHEADCTL, tmp);
-}
-
-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, s;
-	unsigned int computed;
-
-	m = n = p = s = 0;
-	vcomax = 550000;
-	vcomin = 150000;
-	pllreffreq = 50000;
-
-	delta = 0xffffffff;
-
-	for (testp = 16; testp > 0; testp--) {
-		if (clock * testp > vcomax)
-			continue;
-		if (clock * testp < vcomin)
-			continue;
-
-		for (testn = 1; testn < 257; testn++) {
-			for (testm = 1; testm < 17; testm++) {
-				computed = (pllreffreq * testn) /
-					(testm * testp);
-				if (computed > clock)
-					tmpdelta = computed - clock;
-				else
-					tmpdelta = clock - computed;
-				if (tmpdelta < delta) {
-					delta = tmpdelta;
-					n = testn;
-					m = testm;
-					p = testp;
-				}
-			}
+			tmpcount = RREG8(MGAREG_VCOUNT);
+			if (tmpcount < vcount)
+				vcount = 0;
+			if ((tmpcount - vcount) > 2)
+				pll_locked = true;
+			else
+				udelay(5);
 		}
 	}
 
-	pixpllc->m = m;
-	pixpllc->n = n;
-	pixpllc->p = p;
-	pixpllc->s = s;
-
-	return 0;
+	WREG8(DAC_INDEX, MGA1064_REMHEADCTL);
+	tmp = RREG8(DAC_DATA);
+	tmp &= ~MGA1064_REMHEADCTL_CLKDIS;
+	WREG_DAC(MGA1064_REMHEADCTL, tmp);
 }
 
 static void mgag200_set_pixpll_g200ev(struct mga_device *mdev,
@@ -635,54 +874,6 @@ static void mgag200_set_pixpll_g200ev(struct mga_device *mdev,
 	WREG8(DAC_DATA, tmp);
 }
 
-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, s;
-	unsigned int computed;
-
-	m = n = p = s = 0;
-
-	vcomax = 800000;
-	vcomin = 400000;
-	pllreffreq = 33333;
-
-	delta = 0xffffffff;
-
-	for (testp = 16; testp > 0; testp >>= 1) {
-		if (clock * testp > vcomax)
-			continue;
-		if (clock * testp < vcomin)
-			continue;
-
-		for (testm = 1; testm < 33; testm++) {
-			for (testn = 17; testn < 257; testn++) {
-				computed = (pllreffreq * testn) / (testm * testp);
-				if (computed > clock)
-					tmpdelta = computed - clock;
-				else
-					tmpdelta = clock - computed;
-				if (tmpdelta < delta) {
-					delta = tmpdelta;
-					n = testn;
-					m = testm;
-					p = testp;
-				}
-			}
-		}
-	}
-
-	pixpllc->m = m;
-	pixpllc->n = n;
-	pixpllc->p = p;
-	pixpllc->s = s;
-
-	return 0;
-}
-
 static void mgag200_set_pixpll_g200eh(struct mga_device *mdev,
 				      const struct mgag200_pll_values *pixpllc)
 {
@@ -754,115 +945,6 @@ static void mgag200_set_pixpll_g200eh(struct mga_device *mdev,
 	}
 }
 
-static int mgag200_compute_pixpll_values_g200eh3(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, s;
-	unsigned int computed;
-
-	m = n = p = s = 0;
-
-	vcomax = 3000000;
-	vcomin = 1500000;
-	pllreffreq = 25000;
-
-	delta = 0xffffffff;
-
-	testp = 0;
-
-	for (testm = 150; testm >= 6; testm--) {
-		if (clock * testm > vcomax)
-			continue;
-		if (clock * testm < vcomin)
-			continue;
-		for (testn = 120; testn >= 60; testn--) {
-			computed = (pllreffreq * testn) / testm;
-			if (computed > clock)
-				tmpdelta = computed - clock;
-			else
-				tmpdelta = clock - computed;
-			if (tmpdelta < delta) {
-				delta = tmpdelta;
-				n = testn + 1;
-				m = testm + 1;
-				p = testp + 1;
-			}
-			if (delta == 0)
-				break;
-		}
-		if (delta == 0)
-			break;
-	}
-
-	pixpllc->m = m;
-	pixpllc->n = n;
-	pixpllc->p = p;
-	pixpllc->s = s;
-
-	return 0;
-}
-
-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;
-	unsigned int delta, tmpdelta;
-	int testr, testn, testm, testo;
-	unsigned int p, m, n, s;
-	unsigned int computed, vco;
-
-	m = n = p = s = 0;
-	vcomax = 1488000;
-	vcomin = 1056000;
-	pllreffreq = 48000;
-
-	delta = 0xffffffff;
-
-	for (testr = 0; testr < 4; testr++) {
-		if (delta == 0)
-			break;
-		for (testn = 5; testn < 129; testn++) {
-			if (delta == 0)
-				break;
-			for (testm = 3; testm >= 0; testm--) {
-				if (delta == 0)
-					break;
-				for (testo = 5; testo < 33; testo++) {
-					vco = pllreffreq * (testn + 1) /
-						(testr + 1);
-					if (vco < vcomin)
-						continue;
-					if (vco > vcomax)
-						continue;
-					computed = vco / (m_div_val[testm] * (testo + 1));
-					if (computed > clock)
-						tmpdelta = computed - clock;
-					else
-						tmpdelta = clock - computed;
-					if (tmpdelta < delta) {
-						delta = tmpdelta;
-						m = (testm | (testo << 3)) + 1;
-						n = testn + 1;
-						p = testr + 1;
-						s = testr;
-					}
-				}
-			}
-		}
-	}
-
-	pixpllc->m = m;
-	pixpllc->n = n;
-	pixpllc->p = p;
-	pixpllc->s = s;
-
-	return 0;
-}
-
 static void mgag200_set_pixpll_g200er(struct mga_device *mdev,
 				      const struct mgag200_pll_values *pixpllc)
 {
@@ -912,121 +994,38 @@ static void mgag200_set_pixpll_g200er(struct mga_device *mdev,
 	udelay(50);
 }
 
-static int mgag200_compute_pixpll_values_g200ew3(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, s;
-	unsigned int computed;
-
-	m = n = p = s = 0;
-
-	delta = 0xffffffff;
-
-	vcomax = 800000;
-	vcomin = 400000;
-	pllreffreq = 25000;
-
-	for (testp = 1; testp < 8; testp++) {
-		for (testp2 = 1; testp2 < 8; testp2++) {
-			if (testp < testp2)
-				continue;
-			if ((clock * testp * testp2) > vcomax)
-				continue;
-			if ((clock * testp * testp2) < vcomin)
-				continue;
-			for (testm = 1; testm < 26; testm++) {
-				for (testn = 32; testn < 2048 ; testn++) {
-					computed = (pllreffreq * testn) / (testm * testp * testp2);
-					if (computed > clock)
-						tmpdelta = computed - clock;
-					else
-						tmpdelta = clock - computed;
-					if (tmpdelta < delta) {
-						delta = tmpdelta;
-						m = testm + 1;
-						n = testn + 1;
-						p = testp + 1;
-						s = testp2;
-					}
-				}
-			}
-		}
-	}
-
-	pixpllc->m = m;
-	pixpllc->n = n;
-	pixpllc->p = p;
-	pixpllc->s = s;
-
-	return 0;
-}
-
-static void mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
+static void mgag200_set_pixpll(struct mga_device *mdev, const struct mgag200_pll_values *pixpll)
 {
-	struct mgag200_pll_values pixpll;
-	int ret;
-
-	switch(mdev->type) {
-	case G200_PCI:
-	case G200_AGP:
-		ret = mgag200_compute_pixpll_values_g200(mdev, clock, &pixpll);
-		break;
-	case G200_SE_A:
-	case G200_SE_B:
-		ret = mgag200_compute_pixpll_values_g200se(mdev, clock, &pixpll);
-		break;
-	case G200_WB:
-		ret = mgag200_compute_pixpll_values_g200wb(mdev, clock, &pixpll);
-		break;
-	case G200_EV:
-		ret = mgag200_compute_pixpll_values_g200ev(mdev, clock, &pixpll);
-		break;
-	case G200_EH:
-		ret = mgag200_compute_pixpll_values_g200eh(mdev, clock, &pixpll);
-		break;
-	case G200_EH3:
-		ret = mgag200_compute_pixpll_values_g200eh3(mdev, clock, &pixpll);
-		break;
-	case G200_ER:
-		ret = mgag200_compute_pixpll_values_g200er(mdev, clock, &pixpll);
-		break;
-	case G200_EW3:
-		ret = mgag200_compute_pixpll_values_g200ew3(mdev, clock, &pixpll);
-		break;
-	}
-
-	if (ret)
-		return;
-
 	switch (mdev->type) {
 	case G200_PCI:
 	case G200_AGP:
-		mgag200_set_pixpll_g200(mdev, &pixpll);
+		mgag200_set_pixpll_g200(mdev, pixpll);
 		break;
 	case G200_SE_A:
 	case G200_SE_B:
-		mgag200_set_pixpll_g200se(mdev, &pixpll);
+		mgag200_set_pixpll_g200se(mdev, pixpll);
 		break;
 	case G200_WB:
 	case G200_EW3:
-		mgag200_set_pixpll_g200wb(mdev, &pixpll);
+		mgag200_set_pixpll_g200wb(mdev, pixpll);
 		break;
 	case G200_EV:
-		mgag200_set_pixpll_g200ev(mdev, &pixpll);
+		mgag200_set_pixpll_g200ev(mdev, pixpll);
 		break;
 	case G200_EH:
 	case G200_EH3:
-		mgag200_set_pixpll_g200eh(mdev, &pixpll);
+		mgag200_set_pixpll_g200eh(mdev, pixpll);
 		break;
 	case G200_ER:
-		mgag200_set_pixpll_g200er(mdev, &pixpll);
+		mgag200_set_pixpll_g200er(mdev, pixpll);
 		break;
 	}
 }
 
+/*
+ * Modesetting helpers
+ */
+
 static void mgag200_g200wb_hold_bmc(struct mga_device *mdev)
 {
 	u8 tmp;
@@ -1790,13 +1789,15 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
+	struct mgag200_pll_values pixpll;
 
 	if (mdev->type == G200_WB || mdev->type == G200_EW3)
 		mgag200_g200wb_hold_bmc(mdev);
 
 	mgag200_set_format_regs(mdev, fb);
 	mgag200_set_mode_regs(mdev, adjusted_mode);
-	mgag200_crtc_set_plls(mdev, adjusted_mode->clock);
+	mgag200_compute_pixpll_values(mdev, adjusted_mode->clock, &pixpll);
+	mgag200_set_pixpll(mdev, &pixpll);
 
 	if (mdev->type == G200_ER)
 		mgag200_g200er_reset_tagfifo(mdev);
-- 
2.32.0

[PATCH 09/12] drm/mgag200: Split PLL computation for G200SE

[Thomas Zimmermann]

The compute function for G200SE pixle PLLs handles two revisions with
different algorithms. Split it accordingly to make it readable.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 165 +++++++++++++++----------
 1 file changed, 97 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 72fdf242cac7..99b35e4f9353 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -184,100 +184,118 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc
 	return 0;
 }
 
-static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock,
-						struct mgag200_pll_values *pixpllc)
+static int mgag200_compute_pixpll_values_g200se_00(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};
-
-	u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
 	unsigned int computed;
-	unsigned int fvv;
-	unsigned int i;
 
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
-	if (unique_rev_id <= 0x03) {
-		vcomax = 320000;
-		vcomin = 160000;
-		pllreffreq = 25000;
-		permitteddelta = clock * 5 / 1000;
+	vcomax = 320000;
+	vcomin = 160000;
+	pllreffreq = 25000;
+	permitteddelta = clock * 5 / 1000;
 
-		for (testp = 8; testp > 0; testp /= 2) {
-			if (clock * testp > vcomax)
-				continue;
-			if (clock * testp < vcomin)
-				continue;
+	for (testp = 8; testp > 0; testp /= 2) {
+		if (clock * testp > vcomax)
+			continue;
+		if (clock * testp < vcomin)
+			continue;
 
-			for (testn = 17; testn < 256; testn++) {
-				for (testm = 1; testm < 32; testm++) {
-					computed = (pllreffreq * testn) /
-						(testm * testp);
-					if (computed > clock)
-						tmpdelta = computed - clock;
-					else
-						tmpdelta = clock - computed;
-					if (tmpdelta < delta) {
-						delta = tmpdelta;
-						m = testm;
-						n = testn;
-						p = testp;
-					}
+		for (testn = 17; testn < 256; testn++) {
+			for (testm = 1; testm < 32; testm++) {
+				computed = (pllreffreq * testn) / (testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+				if (tmpdelta < delta) {
+					delta = tmpdelta;
+					m = testm;
+					n = testn;
+					p = testp;
 				}
 			}
 		}
-	} else {
-		vcomax        = 1600000;
-		vcomin        = 800000;
-		pllreffreq    = 25000;
+	}
 
-		if (clock < 25000)
-			clock = 25000;
-		clock = clock * 2;
+	if (delta > permitteddelta) {
+		pr_warn("PLL delta too large\n");
+		return -EINVAL;
+	}
 
-		/* Permited delta is 0.5% as VESA Specification */
-		permitteddelta = clock * 5 / 1000;
+	pixpllc->m = m;
+	pixpllc->n = n;
+	pixpllc->p = p;
+	pixpllc->s = s;
 
-		for (i = 0 ; i < ARRAY_SIZE(pvalues_e4); i++) {
-			testp = pvalues_e4[i];
+	return 0;
+}
 
-			if ((clock * testp) > vcomax)
-				continue;
-			if ((clock * testp) < vcomin)
-				continue;
+static int mgag200_compute_pixpll_values_g200se_04(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};
 
-			for (testn = 50; testn <= 256; testn++) {
-				for (testm = 1; testm <= 32; testm++) {
-					computed = (pllreffreq * testn) /
-						(testm * testp);
-					if (computed > clock)
-						tmpdelta = computed - clock;
-					else
-						tmpdelta = clock - computed;
+	unsigned int vcomax, vcomin, pllreffreq;
+	unsigned int delta, tmpdelta, permitteddelta;
+	unsigned int testp, testm, testn;
+	unsigned int p, m, n, s;
+	unsigned int computed;
+	unsigned int fvv;
+	unsigned int i;
 
-					if (tmpdelta < delta) {
-						delta = tmpdelta;
-						m = testm;
-						n = testn;
-						p = testp;
-					}
+	m = n = p = s = 0;
+	delta = 0xffffffff;
+
+	vcomax        = 1600000;
+	vcomin        = 800000;
+	pllreffreq    = 25000;
+
+	if (clock < 25000)
+		clock = 25000;
+	clock = clock * 2;
+
+	/* Permited delta is 0.5% as VESA Specification */
+	permitteddelta = clock * 5 / 1000;
+
+	for (i = 0 ; i < ARRAY_SIZE(pvalues_e4); i++) {
+		testp = pvalues_e4[i];
+
+		if ((clock * testp) > vcomax)
+			continue;
+		if ((clock * testp) < vcomin)
+			continue;
+
+		for (testn = 50; testn <= 256; testn++) {
+			for (testm = 1; testm <= 32; testm++) {
+				computed = (pllreffreq * testn) / (testm * testp);
+				if (computed > clock)
+					tmpdelta = computed - clock;
+				else
+					tmpdelta = clock - computed;
+
+				if (tmpdelta < delta) {
+					delta = tmpdelta;
+					m = testm;
+					n = testn;
+					p = testp;
 				}
 			}
 		}
-
-		fvv = pllreffreq * n / m;
-		fvv = (fvv - 800000) / 50000;
-		if (fvv > 15)
-			fvv = 15;
-		s = fvv << 1;
-
-		clock = clock / 2;
 	}
 
+	fvv = pllreffreq * n / m;
+	fvv = (fvv - 800000) / 50000;
+	if (fvv > 15)
+		fvv = 15;
+	s = fvv << 1;
+
 	if (delta > permitteddelta) {
 		pr_warn("PLL delta too large\n");
 		return -EINVAL;
@@ -291,6 +309,17 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 	return 0;
 }
 
+static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long clock,
+						struct mgag200_pll_values *pixpllc)
+{
+	u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
+
+	if (unique_rev_id >= 0x04)
+		return mgag200_compute_pixpll_values_g200se_04(mdev, clock, pixpllc);
+	else
+		return mgag200_compute_pixpll_values_g200se_00(mdev, clock, pixpllc);
+}
+
 static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock,
 						struct mgag200_pll_values *pixpllc)
 {
-- 
2.32.0

[PATCH 10/12] drm/mgag200: Declare PLL clock constants static const

[Thomas Zimmermann]

Move the PLL constants to the RO data section by declaring them as
static const. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 70 ++++++++++++--------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 99b35e4f9353..5da72ebd8a7f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -187,7 +187,10 @@ static int mgag200_compute_pixpll_values_g200(struct mga_device *mdev, long cloc
 static int mgag200_compute_pixpll_values_g200se_00(struct mga_device *mdev, long clock,
 						   struct mgag200_pll_values *pixpllc)
 {
-	unsigned int vcomax, vcomin, pllreffreq;
+	static const unsigned int vcomax = 320000;
+	static const unsigned int vcomin = 160000;
+	static const unsigned int pllreffreq = 25000;
+
 	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
@@ -196,9 +199,6 @@ static int mgag200_compute_pixpll_values_g200se_00(struct mga_device *mdev, long
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
-	vcomax = 320000;
-	vcomin = 160000;
-	pllreffreq = 25000;
 	permitteddelta = clock * 5 / 1000;
 
 	for (testp = 8; testp > 0; testp /= 2) {
@@ -241,8 +241,10 @@ static int mgag200_compute_pixpll_values_g200se_04(struct mga_device *mdev, long
 						   struct mgag200_pll_values *pixpllc)
 {
 	static const unsigned int pvalues_e4[] = {16, 14, 12, 10, 8, 6, 4, 2, 1};
+	static const unsigned int vcomax = 1600000;
+	static const unsigned int vcomin = 800000;
+	static const unsigned int pllreffreq = 25000;
 
-	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
@@ -253,10 +255,6 @@ static int mgag200_compute_pixpll_values_g200se_04(struct mga_device *mdev, long
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
-	vcomax        = 1600000;
-	vcomin        = 800000;
-	pllreffreq    = 25000;
-
 	if (clock < 25000)
 		clock = 25000;
 	clock = clock * 2;
@@ -323,7 +321,10 @@ static int mgag200_compute_pixpll_values_g200se(struct mga_device *mdev, long cl
 static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long clock,
 						struct mgag200_pll_values *pixpllc)
 {
-	unsigned int vcomax, vcomin, pllreffreq;
+	static const unsigned int vcomax = 550000;
+	static const unsigned int vcomin = 150000;
+	static const unsigned int pllreffreq = 48000;
+
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
@@ -332,10 +333,6 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
-	vcomax = 550000;
-	vcomin = 150000;
-	pllreffreq = 48000;
-
 	for (testp = 1; testp < 9; testp++) {
 		if (clock * testp > vcomax)
 			continue;
@@ -371,7 +368,10 @@ static int mgag200_compute_pixpll_values_g200wb(struct mga_device *mdev, long cl
 static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long clock,
 						struct mgag200_pll_values *pixpllc)
 {
-	unsigned int vcomax, vcomin, pllreffreq;
+	static const unsigned int vcomax = 550000;
+	static const unsigned int vcomin = 150000;
+	static const unsigned int pllreffreq = 50000;
+
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
@@ -380,10 +380,6 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
-	vcomax = 550000;
-	vcomin = 150000;
-	pllreffreq = 50000;
-
 	for (testp = 16; testp > 0; testp--) {
 		if (clock * testp > vcomax)
 			continue;
@@ -419,7 +415,10 @@ static int mgag200_compute_pixpll_values_g200ev(struct mga_device *mdev, long cl
 static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long clock,
 						struct mgag200_pll_values *pixpllc)
 {
-	unsigned int vcomax, vcomin, pllreffreq;
+	static const unsigned int vcomax = 800000;
+	static const unsigned int vcomin = 400000;
+	static const unsigned int pllreffreq = 33333;
+
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
@@ -428,10 +427,6 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
-	vcomax = 800000;
-	vcomin = 400000;
-	pllreffreq = 33333;
-
 	for (testp = 16; testp > 0; testp >>= 1) {
 		if (clock * testp > vcomax)
 			continue;
@@ -466,7 +461,10 @@ static int mgag200_compute_pixpll_values_g200eh(struct mga_device *mdev, long cl
 static int mgag200_compute_pixpll_values_g200eh3(struct mga_device *mdev, long clock,
 						 struct mgag200_pll_values *pixpllc)
 {
-	unsigned int vcomax, vcomin, pllreffreq;
+	static const unsigned int vcomax = 3000000;
+	static const unsigned int vcomin = 1500000;
+	static const unsigned int pllreffreq = 25000;
+
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
@@ -476,10 +474,6 @@ static int mgag200_compute_pixpll_values_g200eh3(struct mga_device *mdev, long c
 	delta = 0xffffffff;
 	testp = 0;
 
-	vcomax = 3000000;
-	vcomin = 1500000;
-	pllreffreq = 25000;
-
 	for (testm = 150; testm >= 6; testm--) {
 		if (clock * testm > vcomax)
 			continue;
@@ -516,16 +510,15 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl
 						struct mgag200_pll_values *pixpllc)
 {
 	static const unsigned int m_div_val[] = { 1, 2, 4, 8 };
-	unsigned int vcomax, vcomin, pllreffreq;
+	static const unsigned int vcomax = 1488000;
+	static const unsigned int vcomin = 1056000;
+	static const unsigned int pllreffreq = 48000;
+
 	unsigned int delta, tmpdelta;
 	int testr, testn, testm, testo;
 	unsigned int p, m, n, s;
 	unsigned int computed, vco;
 
-	vcomax = 1488000;
-	vcomin = 1056000;
-	pllreffreq = 48000;
-
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
@@ -573,7 +566,10 @@ static int mgag200_compute_pixpll_values_g200er(struct mga_device *mdev, long cl
 static int mgag200_compute_pixpll_values_g200ew3(struct mga_device *mdev, long clock,
 						 struct mgag200_pll_values *pixpllc)
 {
-	unsigned int vcomax, vcomin, pllreffreq;
+	static const unsigned int vcomax = 800000;
+	static const unsigned int vcomin = 400000;
+	static const unsigned int pllreffreq = 25000;
+
 	unsigned int delta, tmpdelta;
 	unsigned int testp, testm, testn, testp2;
 	unsigned int p, m, n, s;
@@ -582,10 +578,6 @@ static int mgag200_compute_pixpll_values_g200ew3(struct mga_device *mdev, long c
 	m = n = p = s = 0;
 	delta = 0xffffffff;
 
-	vcomax = 800000;
-	vcomin = 400000;
-	pllreffreq = 25000;
-
 	for (testp = 1; testp < 8; testp++) {
 		for (testp2 = 1; testp2 < 8; testp2++) {
 			if (testp < testp2)
-- 
2.32.0

[PATCH 11/12] drm/mgag200: Introduce custom CRTC state

[Thomas Zimmermann]

The CRTC state in mgag200 will hold PLL values for modeset operations.
Simple KMS helpers already support custom state for planes. Extend the
helpers to support custom CRTC state as well.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 39 +++++++++++++++++++--
 drivers/gpu/drm/mgag200/mgag200_drv.h   |  9 +++++
 drivers/gpu/drm/mgag200/mgag200_mode.c  | 46 +++++++++++++++++++++++++
 include/drm/drm_simple_kms_helper.h     | 27 +++++++++++++++
 4 files changed, 118 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 735f4f34bcc4..72989ed1baba 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -145,6 +145,39 @@ static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
 	.atomic_disable = drm_simple_kms_crtc_disable,
 };
 
+static void drm_simple_kms_crtc_reset(struct drm_crtc *crtc)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->reset_crtc)
+		return drm_atomic_helper_crtc_reset(crtc);
+
+	return pipe->funcs->reset_crtc(pipe);
+}
+
+static struct drm_crtc_state *drm_simple_kms_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->duplicate_crtc_state)
+		return drm_atomic_helper_crtc_duplicate_state(crtc);
+
+	return pipe->funcs->duplicate_crtc_state(pipe);
+}
+
+static void drm_simple_kms_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->destroy_crtc_state)
+		drm_atomic_helper_crtc_destroy_state(crtc, state);
+	else
+		pipe->funcs->destroy_crtc_state(pipe, state);
+}
+
 static int drm_simple_kms_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct drm_simple_display_pipe *pipe;
@@ -168,12 +201,12 @@ static void drm_simple_kms_crtc_disable_vblank(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
-	.reset = drm_atomic_helper_crtc_reset,
+	.reset = drm_simple_kms_crtc_reset,
 	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_duplicate_state = drm_simple_kms_crtc_duplicate_state,
+	.atomic_destroy_state = drm_simple_kms_crtc_destroy_state,
 	.enable_vblank = drm_simple_kms_crtc_enable_vblank,
 	.disable_vblank = drm_simple_kms_crtc_disable_vblank,
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index fca3904fde0c..c813d6c15f70 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -127,6 +127,15 @@ struct mgag200_pll_values {
 	unsigned int s;
 };
 
+struct mgag200_crtc_state {
+	struct drm_crtc_state base;
+};
+
+static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base)
+{
+	return container_of(base, struct mgag200_crtc_state, base);
+}
+
 #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 5da72ebd8a7f..6a5c419c6641 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1886,6 +1886,49 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->map[0]);
 }
 
+static struct drm_crtc_state *
+mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe *pipe)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_crtc_state *crtc_state = crtc->state;
+	struct mgag200_crtc_state *new_mgag200_crtc_state;
+
+	if (!crtc_state)
+		return NULL;
+
+	new_mgag200_crtc_state = kzalloc(sizeof(*new_mgag200_crtc_state), GFP_KERNEL);
+	if (!new_mgag200_crtc_state)
+		return NULL;
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &new_mgag200_crtc_state->base);
+
+	return &new_mgag200_crtc_state->base;
+}
+
+static void mgag200_simple_display_pipe_destroy_crtc_state(struct drm_simple_display_pipe *pipe,
+							   struct drm_crtc_state *crtc_state)
+{
+	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
+
+	__drm_atomic_helper_crtc_destroy_state(&mgag200_crtc_state->base);
+	kfree(mgag200_crtc_state);
+}
+
+static void mgag200_simple_display_pipe_reset_crtc(struct drm_simple_display_pipe *pipe)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct mgag200_crtc_state *mgag200_crtc_state;
+
+	if (crtc->state) {
+		mgag200_simple_display_pipe_destroy_crtc_state(pipe, crtc->state);
+		crtc->state = NULL; /* must be set to NULL here */
+	}
+
+	mgag200_crtc_state = kzalloc(sizeof(*mgag200_crtc_state), GFP_KERNEL);
+	if (!mgag200_crtc_state)
+		return;
+	__drm_atomic_helper_crtc_reset(crtc, &mgag200_crtc_state->base);
+}
+
 static const struct drm_simple_display_pipe_funcs
 mgag200_simple_display_pipe_funcs = {
 	.mode_valid = mgag200_simple_display_pipe_mode_valid,
@@ -1893,6 +1936,9 @@ mgag200_simple_display_pipe_funcs = {
 	.disable    = mgag200_simple_display_pipe_disable,
 	.check	    = mgag200_simple_display_pipe_check,
 	.update	    = mgag200_simple_display_pipe_update,
+	.reset_crtc = mgag200_simple_display_pipe_reset_crtc,
+	.duplicate_crtc_state = mgag200_simple_display_pipe_duplicate_crtc_state,
+	.destroy_crtc_state = mgag200_simple_display_pipe_destroy_crtc_state,
 	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
 };
 
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index cf07132d4ee8..0b3647e614dd 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -153,6 +153,33 @@ struct drm_simple_display_pipe_funcs {
 	 */
 	void (*disable_vblank)(struct drm_simple_display_pipe *pipe);
 
+	/**
+	 * @reset_crtc:
+	 *
+	 * Optional, called by &drm_crtc_funcs.reset. Please read the
+	 * documentation for the &drm_crtc_funcs.reset hook for more details.
+	 */
+	void (*reset_crtc)(struct drm_simple_display_pipe *pipe);
+
+	/**
+	 * @duplicate_crtc_state:
+	 *
+	 * Optional, called by &drm_crtc_funcs.atomic_duplicate_state. Please
+	 * read the documentation for the &drm_crtc_funcs.atomic_duplicate_state
+	 * hook for more details.
+	 */
+	struct drm_crtc_state * (*duplicate_crtc_state)(struct drm_simple_display_pipe *pipe);
+
+	/**
+	 * @destroy_crtc_state:
+	 *
+	 * Optional, called by &drm_crtc_funcs.atomic_destroy_state. Please
+	 * read the documentation for the &drm_crtc_funcs.atomic_destroy_state
+	 * hook for more details.
+	 */
+	void (*destroy_crtc_state)(struct drm_simple_display_pipe *pipe,
+				   struct drm_crtc_state *crtc_state);
+
 	/**
 	 * @reset_plane:
 	 *
-- 
2.32.0

Re: [PATCH 11/12] drm/mgag200: Introduce custom CRTC state

[Sam Ravnborg]

Hi Thomas,

On Mon, Jul 05, 2021 at 02:45:14PM +0200, Thomas Zimmermann wrote:
> The CRTC state in mgag200 will hold PLL values for modeset operations.
> Simple KMS helpers already support custom state for planes. Extend the
> helpers to support custom CRTC state as well.

This should be split in two patches - so the changes to
drm_simple_kms_helper can get more attention.

The patch as such looks fine so when split you can add my:
Acked-by: Sam Ravnborg <sam@ravnborg.org>
on the mgag200 parts.

And
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
on the drm_simple_kms_helper parts.

	Sam

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 39 +++++++++++++++++++--
>  drivers/gpu/drm/mgag200/mgag200_drv.h   |  9 +++++
>  drivers/gpu/drm/mgag200/mgag200_mode.c  | 46 +++++++++++++++++++++++++
>  include/drm/drm_simple_kms_helper.h     | 27 +++++++++++++++
>  4 files changed, 118 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 735f4f34bcc4..72989ed1baba 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -145,6 +145,39 @@ static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
>  	.atomic_disable = drm_simple_kms_crtc_disable,
>  };
>  
> +static void drm_simple_kms_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> +	if (!pipe->funcs || !pipe->funcs->reset_crtc)
> +		return drm_atomic_helper_crtc_reset(crtc);
> +
> +	return pipe->funcs->reset_crtc(pipe);
> +}
> +
> +static struct drm_crtc_state *drm_simple_kms_crtc_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> +	if (!pipe->funcs || !pipe->funcs->duplicate_crtc_state)
> +		return drm_atomic_helper_crtc_duplicate_state(crtc);
> +
> +	return pipe->funcs->duplicate_crtc_state(pipe);
> +}
> +
> +static void drm_simple_kms_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> +	if (!pipe->funcs || !pipe->funcs->destroy_crtc_state)
> +		drm_atomic_helper_crtc_destroy_state(crtc, state);
> +	else
> +		pipe->funcs->destroy_crtc_state(pipe, state);
> +}
> +
>  static int drm_simple_kms_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct drm_simple_display_pipe *pipe;
> @@ -168,12 +201,12 @@ static void drm_simple_kms_crtc_disable_vblank(struct drm_crtc *crtc)
>  }
>  
>  static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
> -	.reset = drm_atomic_helper_crtc_reset,
> +	.reset = drm_simple_kms_crtc_reset,
>  	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
> -	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_duplicate_state = drm_simple_kms_crtc_duplicate_state,
> +	.atomic_destroy_state = drm_simple_kms_crtc_destroy_state,
>  	.enable_vblank = drm_simple_kms_crtc_enable_vblank,
>  	.disable_vblank = drm_simple_kms_crtc_disable_vblank,
>  };
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index fca3904fde0c..c813d6c15f70 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -127,6 +127,15 @@ struct mgag200_pll_values {
>  	unsigned int s;
>  };
>  
> +struct mgag200_crtc_state {
> +	struct drm_crtc_state base;
> +};
> +
> +static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base)
> +{
> +	return container_of(base, struct mgag200_crtc_state, base);
> +}
> +
>  #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 5da72ebd8a7f..6a5c419c6641 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1886,6 +1886,49 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>  		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->map[0]);
>  }
>  
> +static struct drm_crtc_state *
> +mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe *pipe)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct drm_crtc_state *crtc_state = crtc->state;
> +	struct mgag200_crtc_state *new_mgag200_crtc_state;
> +
> +	if (!crtc_state)
> +		return NULL;
> +
> +	new_mgag200_crtc_state = kzalloc(sizeof(*new_mgag200_crtc_state), GFP_KERNEL);
> +	if (!new_mgag200_crtc_state)
> +		return NULL;
> +	__drm_atomic_helper_crtc_duplicate_state(crtc, &new_mgag200_crtc_state->base);
> +
> +	return &new_mgag200_crtc_state->base;
> +}
> +
> +static void mgag200_simple_display_pipe_destroy_crtc_state(struct drm_simple_display_pipe *pipe,
> +							   struct drm_crtc_state *crtc_state)
> +{
> +	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
> +
> +	__drm_atomic_helper_crtc_destroy_state(&mgag200_crtc_state->base);
> +	kfree(mgag200_crtc_state);
> +}
> +
> +static void mgag200_simple_display_pipe_reset_crtc(struct drm_simple_display_pipe *pipe)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct mgag200_crtc_state *mgag200_crtc_state;
> +
> +	if (crtc->state) {
> +		mgag200_simple_display_pipe_destroy_crtc_state(pipe, crtc->state);
> +		crtc->state = NULL; /* must be set to NULL here */
> +	}
> +
> +	mgag200_crtc_state = kzalloc(sizeof(*mgag200_crtc_state), GFP_KERNEL);
> +	if (!mgag200_crtc_state)
> +		return;
> +	__drm_atomic_helper_crtc_reset(crtc, &mgag200_crtc_state->base);
> +}
> +
>  static const struct drm_simple_display_pipe_funcs
>  mgag200_simple_display_pipe_funcs = {
>  	.mode_valid = mgag200_simple_display_pipe_mode_valid,
> @@ -1893,6 +1936,9 @@ mgag200_simple_display_pipe_funcs = {
>  	.disable    = mgag200_simple_display_pipe_disable,
>  	.check	    = mgag200_simple_display_pipe_check,
>  	.update	    = mgag200_simple_display_pipe_update,
> +	.reset_crtc = mgag200_simple_display_pipe_reset_crtc,
> +	.duplicate_crtc_state = mgag200_simple_display_pipe_duplicate_crtc_state,
> +	.destroy_crtc_state = mgag200_simple_display_pipe_destroy_crtc_state,
>  	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
>  };
>  
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index cf07132d4ee8..0b3647e614dd 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -153,6 +153,33 @@ struct drm_simple_display_pipe_funcs {
>  	 */
>  	void (*disable_vblank)(struct drm_simple_display_pipe *pipe);
>  
> +	/**
> +	 * @reset_crtc:
> +	 *
> +	 * Optional, called by &drm_crtc_funcs.reset. Please read the
> +	 * documentation for the &drm_crtc_funcs.reset hook for more details.
> +	 */
> +	void (*reset_crtc)(struct drm_simple_display_pipe *pipe);
> +
> +	/**
> +	 * @duplicate_crtc_state:
> +	 *
> +	 * Optional, called by &drm_crtc_funcs.atomic_duplicate_state. Please
> +	 * read the documentation for the &drm_crtc_funcs.atomic_duplicate_state
> +	 * hook for more details.
> +	 */
> +	struct drm_crtc_state * (*duplicate_crtc_state)(struct drm_simple_display_pipe *pipe);
> +
> +	/**
> +	 * @destroy_crtc_state:
> +	 *
> +	 * Optional, called by &drm_crtc_funcs.atomic_destroy_state. Please
> +	 * read the documentation for the &drm_crtc_funcs.atomic_destroy_state
> +	 * hook for more details.
> +	 */
> +	void (*destroy_crtc_state)(struct drm_simple_display_pipe *pipe,
> +				   struct drm_crtc_state *crtc_state);
> +
>  	/**
>  	 * @reset_plane:
>  	 *
> -- 
> 2.32.0

[PATCH 12/12] drm/mgag200: Compute PLL values during atomic check

[Thomas Zimmermann]

PLL setup can fail if the display mode's clock is not supported by
any PLL configuration. Compute the PLL values during atomic check, so
that atomic commits can fail at the appropriate time. If successful,
use the values in the atomic-update phase.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  2 ++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 20 +++++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index c813d6c15f70..6473e9c037d0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -129,6 +129,8 @@ struct mgag200_pll_values {
 
 struct mgag200_crtc_state {
 	struct drm_crtc_state base;
+
+	struct mgag200_pll_values pixpll;
 };
 
 static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 6a5c419c6641..55c4f76175bd 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1802,6 +1802,7 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	struct drm_device *dev = crtc->dev;
 	struct mga_device *mdev = to_mga_device(dev);
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_rect fullscreen = {
@@ -1810,15 +1811,13 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
-	struct mgag200_pll_values pixpll;
 
 	if (mdev->type == G200_WB || mdev->type == G200_EW3)
 		mgag200_g200wb_hold_bmc(mdev);
 
 	mgag200_set_format_regs(mdev, fb);
 	mgag200_set_mode_regs(mdev, adjusted_mode);
-	mgag200_compute_pixpll_values(mdev, adjusted_mode->clock, &pixpll);
-	mgag200_set_pixpll(mdev, &pixpll);
+	mgag200_set_pixpll(mdev, &mgag200_crtc_state->pixpll);
 
 	if (mdev->type == G200_ER)
 		mgag200_g200er_reset_tagfifo(mdev);
@@ -1852,8 +1851,12 @@ mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
 				  struct drm_crtc_state *crtc_state)
 {
 	struct drm_plane *plane = plane_state->plane;
+	struct drm_device *dev = plane->dev;
+	struct mga_device *mdev = to_mga_device(dev);
+	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	struct drm_framebuffer *new_fb = plane_state->fb;
 	struct drm_framebuffer *fb = NULL;
+	int ret;
 
 	if (!new_fb)
 		return 0;
@@ -1864,6 +1867,13 @@ mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
 	if (!fb || (fb->format != new_fb->format))
 		crtc_state->mode_changed = true; /* update PLL settings */
 
+	if (crtc_state->mode_changed) {
+		ret = mgag200_compute_pixpll_values(mdev, crtc_state->mode.clock,
+						    &mgag200_crtc_state->pixpll);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1891,6 +1901,7 @@ mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe
 {
 	struct drm_crtc *crtc = &pipe->crtc;
 	struct drm_crtc_state *crtc_state = crtc->state;
+	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	struct mgag200_crtc_state *new_mgag200_crtc_state;
 
 	if (!crtc_state)
@@ -1901,6 +1912,9 @@ mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe
 		return NULL;
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &new_mgag200_crtc_state->base);
 
+	memcpy(&new_mgag200_crtc_state->pixpll, &mgag200_crtc_state->pixpll,
+	       sizeof(new_mgag200_crtc_state->pixpll));
+
 	return &new_mgag200_crtc_state->base;
 }
 
-- 
2.32.0