From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNWANTED_LANGUAGE_BODY, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7AB28C07E95 for ; Sat, 10 Jul 2021 07:06:52 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F1C3961166 for ; Sat, 10 Jul 2021 07:06:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F1C3961166 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ravnborg.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 08E686EB03; Sat, 10 Jul 2021 07:06:51 +0000 (UTC) Received: from mx2.smtp.larsendata.com (mx2.smtp.larsendata.com [91.221.196.228]) by gabe.freedesktop.org (Postfix) with ESMTPS id 911C66EB03 for ; Sat, 10 Jul 2021 07:06:49 +0000 (UTC) Received: from mail01.mxhotel.dk (mail01.mxhotel.dk [91.221.196.236]) by mx2.smtp.larsendata.com (Halon) with ESMTPS id 6821cd63-e14d-11eb-8d1a-0050568cd888; Sat, 10 Jul 2021 07:06:54 +0000 (UTC) Received: from ravnborg.org (80-162-45-141-cable.dk.customer.tdc.net [80.162.45.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: sam@ravnborg.org) by mail01.mxhotel.dk (Postfix) with ESMTPSA id 4CD73194B05; Sat, 10 Jul 2021 09:06:57 +0200 (CEST) Date: Sat, 10 Jul 2021 09:06:45 +0200 X-Report-Abuse-To: abuse@mxhotel.dk From: Sam Ravnborg To: Thomas Zimmermann Subject: Re: [PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values Message-ID: References: <20210705124515.27253-1-tzimmermann@suse.de> <20210705124515.27253-7-tzimmermann@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210705124515.27253-7-tzimmermann@suse.de> X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: John.p.donnelly@oracle.com, dri-devel@lists.freedesktop.org, airlied@redhat.com, emil.velikov@collabora.com Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 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