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@xxxxxxx> --- 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.1092838Apparently, 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:
OpenPGP_signature
Description: OpenPGP digital signature
