* [PATCH 01/12] drm/mgag200: Select clock in PLL update functions
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-05 12:45 ` [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions Thomas Zimmermann
` (10 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: dri-devel, Thomas Zimmermann, stable
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 01/12] drm/mgag200: Select clock in PLL update functions
@ 2021-07-05 12:45 ` Thomas Zimmermann
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: stable, Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions
2021-07-05 12:45 ` Thomas Zimmermann
(?)
@ 2021-07-09 18:50 ` Sam Ravnborg
2021-07-12 13:36 ` Thomas Zimmermann
-1 siblings, 1 reply; 27+ messages in thread
From: Sam Ravnborg @ 2021-07-09 18:50 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: John.p.donnelly, dri-devel, airlied, stable, emil.velikov
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions
2021-07-09 18:50 ` Sam Ravnborg
@ 2021-07-12 13:36 ` Thomas Zimmermann
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-12 13:36 UTC (permalink / raw)
To: Sam Ravnborg
Cc: daniel, airlied, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly, dri-devel, stable
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions
@ 2021-07-12 13:36 ` Thomas Zimmermann
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-12 13:36 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: John.p.donnelly, dri-devel, airlied, stable, emil.velikov
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
2021-07-05 12:45 ` Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-09 18:53 ` Sam Ravnborg
2021-07-05 12:45 ` [PATCH 03/12] drm/mgag200: Remove P_ARRAY_SIZE Thomas Zimmermann
` (9 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions
2021-07-05 12:45 ` [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions Thomas Zimmermann
@ 2021-07-09 18:53 ` Sam Ravnborg
2021-07-12 13:42 ` Thomas Zimmermann
0 siblings, 1 reply; 27+ messages in thread
From: Sam Ravnborg @ 2021-07-09 18:53 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions
2021-07-09 18:53 ` Sam Ravnborg
@ 2021-07-12 13:42 ` Thomas Zimmermann
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-12 13:42 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 03/12] drm/mgag200: Remove P_ARRAY_SIZE
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
2021-07-05 12:45 ` Thomas Zimmermann
2021-07-05 12:45 ` [PATCH 02/12] drm/mgag200: Return errno codes from PLL compute functions Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-09 18:53 ` Sam Ravnborg
2021-07-05 12:45 ` [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions Thomas Zimmermann
` (8 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (2 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 03/12] drm/mgag200: Remove P_ARRAY_SIZE Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-09 19:12 ` Sam Ravnborg
2021-07-05 12:45 ` [PATCH 05/12] drm/mgag200: Introduce separate variable for PLL S parameter Thomas Zimmermann
` (7 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions
2021-07-05 12:45 ` [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions Thomas Zimmermann
@ 2021-07-09 19:12 ` Sam Ravnborg
2021-07-12 14:03 ` Thomas Zimmermann
0 siblings, 1 reply; 27+ messages in thread
From: Sam Ravnborg @ 2021-07-09 19:12 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions
2021-07-09 19:12 ` Sam Ravnborg
@ 2021-07-12 14:03 ` Thomas Zimmermann
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-12 14:03 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 05/12] drm/mgag200: Introduce separate variable for PLL S parameter
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (3 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-09 19:18 ` Sam Ravnborg
2021-07-05 12:45 ` [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values Thomas Zimmermann
` (6 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 05/12] drm/mgag200: Introduce separate variable for PLL S parameter
2021-07-05 12:45 ` [PATCH 05/12] drm/mgag200: Introduce separate variable for PLL S parameter Thomas Zimmermann
@ 2021-07-09 19:18 ` Sam Ravnborg
0 siblings, 0 replies; 27+ messages in thread
From: Sam Ravnborg @ 2021-07-09 19:18 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (4 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 05/12] drm/mgag200: Introduce separate variable for PLL S parameter Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-10 7:06 ` Sam Ravnborg
2021-07-05 12:45 ` [PATCH 07/12] drm/mgag200: Split several PLL functions by device type Thomas Zimmermann
` (5 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values
2021-07-05 12:45 ` [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values Thomas Zimmermann
@ 2021-07-10 7:06 ` Sam Ravnborg
2021-07-12 14:09 ` Thomas Zimmermann
0 siblings, 1 reply; 27+ messages in thread
From: Sam Ravnborg @ 2021-07-10 7:06 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values
2021-07-10 7:06 ` Sam Ravnborg
@ 2021-07-12 14:09 ` Thomas Zimmermann
2021-07-12 14:18 ` Sam Ravnborg
0 siblings, 1 reply; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-12 14:09 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values
2021-07-12 14:09 ` Thomas Zimmermann
@ 2021-07-12 14:18 ` Sam Ravnborg
0 siblings, 0 replies; 27+ messages in thread
From: Sam Ravnborg @ 2021-07-12 14:18 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 07/12] drm/mgag200: Split several PLL functions by device type
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (5 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-05 12:45 ` [PATCH 08/12] drm/mgag200: Separate PLL compute and update functions from each other Thomas Zimmermann
` (4 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 08/12] drm/mgag200: Separate PLL compute and update functions from each other
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (6 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 07/12] drm/mgag200: Split several PLL functions by device type Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-05 12:45 ` [PATCH 09/12] drm/mgag200: Split PLL computation for G200SE Thomas Zimmermann
` (3 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 09/12] drm/mgag200: Split PLL computation for G200SE
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (7 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 08/12] drm/mgag200: Separate PLL compute and update functions from each other Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-05 12:45 ` [PATCH 10/12] drm/mgag200: Declare PLL clock constants static const Thomas Zimmermann
` (2 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 10/12] drm/mgag200: Declare PLL clock constants static const
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (8 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 09/12] drm/mgag200: Split PLL computation for G200SE Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-05 12:45 ` [PATCH 11/12] drm/mgag200: Introduce custom CRTC state Thomas Zimmermann
2021-07-05 12:45 ` [PATCH 12/12] drm/mgag200: Compute PLL values during atomic check Thomas Zimmermann
11 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 11/12] drm/mgag200: Introduce custom CRTC state
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (9 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 10/12] drm/mgag200: Declare PLL clock constants static const Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
2021-07-10 7:01 ` Sam Ravnborg
2021-07-05 12:45 ` [PATCH 12/12] drm/mgag200: Compute PLL values during atomic check Thomas Zimmermann
11 siblings, 1 reply; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 11/12] drm/mgag200: Introduce custom CRTC state
2021-07-05 12:45 ` [PATCH 11/12] drm/mgag200: Introduce custom CRTC state Thomas Zimmermann
@ 2021-07-10 7:01 ` Sam Ravnborg
0 siblings, 0 replies; 27+ messages in thread
From: Sam Ravnborg @ 2021-07-10 7:01 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: John.p.donnelly, dri-devel, airlied, emil.velikov
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 12/12] drm/mgag200: Compute PLL values during atomic check
2021-07-05 12:45 [PATCH 00/12] mgag200: Refactor PLL setup Thomas Zimmermann
` (10 preceding siblings ...)
2021-07-05 12:45 ` [PATCH 11/12] drm/mgag200: Introduce custom CRTC state Thomas Zimmermann
@ 2021-07-05 12:45 ` Thomas Zimmermann
11 siblings, 0 replies; 27+ messages in thread
From: Thomas Zimmermann @ 2021-07-05 12:45 UTC (permalink / raw)
To: daniel, airlied, sam, maarten.lankhorst, mripard, emil.velikov,
John.p.donnelly
Cc: Thomas Zimmermann, dri-devel
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
^ permalink raw reply related [flat|nested] 27+ messages in thread