From: Rob Clark <robdclark@gmail.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
Cc: linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Konrad Dybcio <konrad.dybcio@somainline.org>,
marijn.suijten@somainline.org, martin.botka@somainline.org,
phone-devel@vger.kernel.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
Daniel Vetter <daniel@ffwll.ch>,
dri-devel <dri-devel@lists.freedesktop.org>,
freedreno <freedreno@lists.freedesktop.org>,
Abhinav Kumar <abhinavk@codeaurora.org>,
Douglas Anderson <dianders@chromium.org>
Subject: Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler
Date: Sun, 31 Jan 2021 11:50:29 -0800 [thread overview]
Message-ID: <CAF6AEGvDzdgDy7Znw6dQCV7Z=YxnF2_XsqkV+7BT+oY777TqHA@mail.gmail.com> (raw)
In-Reply-To: <20210109135112.147759-4-angelogioacchino.delregno@somainline.org>
On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> The VCO rate was being miscalculated due to a big overlook during
> the process of porting this driver from downstream to upstream:
> here we are really recalculating the rate of the VCO by reading
> the appropriate registers and returning a real frequency, while
> downstream the driver was doing something entirely different.
>
> In our case here, the recalculated rate was wrong, as it was then
> given back to the set_rate function, which was erroneously doing
> a division on the fractional value, based on the prescaler being
> either enabled or disabled: this was actually producing a bug for
> which the final VCO rate was being doubled, causing very obvious
> issues when trying to drive a DSI panel because the actual divider
> value was multiplied by two!
>
> To make things work properly, remove the multiplication of the
> reference clock by two from function dsi_pll_calc_dec_frac and
> account for the prescaler enablement in the vco_recalc_rate (if
> the prescaler is enabled, then the hardware will divide the rate
> by two).
>
> This will make the vco_recalc_rate function to pass the right
> frequency to the (clock framework) set_rate function when called,
> which will - in turn - program the right values in both the
> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> registers, finally making the PLL to output the right clock.
>
> Also, while at it, remove the prescaler TODO by also adding the
> possibility of disabling the prescaler on the PLL (it is in the
> PLL_ANALOG_CONTROLS_ONE register).
> Of course, both prescaler-ON and OFF cases were tested.
This somehow breaks things on sc7180 (display gets stuck at first
frame of splash screen). (This is a setup w/ an ti-sn65dsi86 dsi->eDP
bridge)
Also, something (I assume DSI related) that I was testing on
msm-next-staging seems to have effected the colors on the panel (ie.
they are more muted).. which seems to persist across reboots (ie. when
switching back to a good kernel), and interestingly if I reboot from a
good kernel I see part of the login prompt (or whatever was previously
on-screen) in the firmware ui screen !?! (so maybe somehow triggered
the display to think it is in PSR mode??)
Not sure if that is caused by these patches, but if I can figure out
how to get the panel back to normal I can bisect. I think for now
I'll drop this series. Possibly it could be a
two-wrongs-makes-a-right situation that had things working before, but
I think someone from qcom who knows the DSI IP should take a look.
BR,
-R
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
> drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> index 8b66e852eb36..5be562dfbf06 100644
> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
>
> pll_freq = pll->vco_current_rate;
>
> - if (config->disable_prescaler)
> - divider = fref;
> - else
> - divider = fref * 2;
> -
> + divider = fref;
> multiplier = 1 << config->frac_bits;
> dec_multiple = div_u64(pll_freq * multiplier, divider);
> dec = div_u64_rem(dec_multiple, multiplier, &frac);
> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
>
> static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
> {
> + struct dsi_pll_config *config = &pll->pll_configuration;
> void __iomem *base = pll->mmio;
> + u32 val = config->disable_prescaler ? 0x0 : 0x80;
>
> - pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> + pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
> pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
> pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
> pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
> frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
> 0x3) << 16);
>
> - /*
> - * TODO:
> - * 1. Assumes prescaler is disabled
> - */
> multiplier = 1 << config->frac_bits;
> - pll_freq = dec * (ref_clk * 2);
> - tmp64 = (ref_clk * 2 * frac);
> + pll_freq = dec * ref_clk;
> + tmp64 = ref_clk * frac;
> pll_freq += div_u64(tmp64, multiplier);
> -
> vco_rate = pll_freq;
>
> + if (config->disable_prescaler)
> + vco_rate = div_u64(vco_rate, 2);
> +
> DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
> pll_10nm->id, (unsigned long)vco_rate, dec, frac);
>
> --
> 2.29.2
>
next prev parent reply other threads:[~2021-01-31 20:03 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-09 13:51 [PATCH 0/5] Clock fixes for DSI 10nm PLL AngeloGioacchino Del Regno
2021-01-09 13:51 ` [PATCH 1/5] drm/msm/dsi_pll_10nm: Fix dividing the same numbers twice AngeloGioacchino Del Regno
2021-01-09 13:51 ` [PATCH 2/5] drm/msm/dsi_pll_10nm: Solve TODO for multiplier frac_bits assignment AngeloGioacchino Del Regno
2021-01-09 13:51 ` [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler AngeloGioacchino Del Regno
2021-01-31 19:50 ` Rob Clark [this message]
2021-02-01 10:11 ` AngeloGioacchino Del Regno
2021-02-01 15:47 ` Rob Clark
2021-02-01 17:05 ` Rob Clark
2021-02-01 17:18 ` Rob Clark
2021-02-01 17:31 ` Rob Clark
2021-02-02 14:32 ` AngeloGioacchino Del Regno
2021-02-02 18:45 ` Rob Clark
2021-02-02 18:46 ` AngeloGioacchino Del Regno
2021-02-02 19:08 ` Rob Clark
2021-02-02 21:35 ` Rob Clark
2021-01-09 13:51 ` [PATCH 4/5] drm/msm/dsi_pll_10nm: Fix variable usage for pll_lockdet_rate AngeloGioacchino Del Regno
2021-01-09 13:51 ` [PATCH 5/5] drm/msm/dsi_pll_10nm: Convert pr_err prints to DRM_DEV_ERROR AngeloGioacchino Del Regno
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAF6AEGvDzdgDy7Znw6dQCV7Z=YxnF2_XsqkV+7BT+oY777TqHA@mail.gmail.com' \
--to=robdclark@gmail.com \
--cc=abhinavk@codeaurora.org \
--cc=airlied@linux.ie \
--cc=angelogioacchino.delregno@somainline.org \
--cc=daniel@ffwll.ch \
--cc=dianders@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=konrad.dybcio@somainline.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=martin.botka@somainline.org \
--cc=phone-devel@vger.kernel.org \
--cc=sean@poorly.run \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).