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