[PATCH 04/12] drm/mgag200: Split PLL setup into compute and update functions
Thomas Zimmermann
tzimmermann at suse.de
Mon Jul 12 14:03:02 UTC 2021
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210712/774aab07/attachment.sig>
More information about the dri-devel
mailing list