Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

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>,
	Stephen Boyd <swboyd@chromium.org>
Subject: Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler
Date: Tue, 2 Feb 2021 13:35:59 -0800	[thread overview]
Message-ID: <CAF6AEGurHVQUhNBzkb8iVxHKrFf1sThyUC0mCDHBoEDnVOTk=w@mail.gmail.com> (raw)
In-Reply-To: <CAF6AEGtg2_CNTMMY4adSxiA8Z0=VSYFy7HaK3w_a2rhokmY2Zg@mail.gmail.com>

On Tue, Feb 2, 2021 at 11:08 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Feb 2, 2021 at 10:46 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
> >
> > Il 02/02/21 19:45, Rob Clark ha scritto:
> > > On Tue, Feb 2, 2021 at 6:32 AM AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@somainline.org> wrote:
> > >>
> > >> Il 01/02/21 18:31, Rob Clark ha scritto:
> > >>> fwiw, this is the clk_summary diff with and without this patch:
> > >>>
> > >>> ------------------
> > >>> 270,282c270,282
> > >>> <     dsi0_pll_out_div_clk              1        1        0
> > >>> 887039941          0     0  50000         Y
> > >>> <        dsi0_pll_post_out_div_clk       0        0        0
> > >>> 221759985          0     0  50000         Y
> > >>> <        dsi0_pll_bit_clk               2        2        0
> > >>> 887039941          0     0  50000         Y
> > >>> <           dsi0_pclk_mux               1        1        0
> > >>> 887039941          0     0  50000         Y
> > >>> <              dsi0_phy_pll_out_dsiclk       1        1        0
> > >>> 147839991          0     0  50000         Y
> > >>> <                 disp_cc_mdss_pclk0_clk_src       1        1        0
> > >>>     147839991          0     0  50000         Y
> > >>> <                    disp_cc_mdss_pclk0_clk       1        1        0
> > >>>    147839991          0     0  50000         Y
> > >>> <           dsi0_pll_by_2_bit_clk       0        0        0
> > >>> 443519970          0     0  50000         Y
> > >>> <           dsi0_phy_pll_out_byteclk       1        1        0
> > >>> 110879992          0     0  50000         Y
> > >>> <              disp_cc_mdss_byte0_clk_src       2        2        0
> > >>> 110879992          0     0  50000         Y
> > >>> <                 disp_cc_mdss_byte0_div_clk_src       1        1
> > >>>     0    55439996          0     0  50000         Y
> > >>> <                    disp_cc_mdss_byte0_intf_clk       1        1
> > >>>     0    55439996          0     0  50000         Y
> > >>> <                 disp_cc_mdss_byte0_clk       1        1        0
> > >>> 110879992          0     0  50000         Y
> > >>> ---
> > >>>>       dsi0_pll_out_div_clk              1        1        0   887039978          0     0  50000         Y
> > >>>>          dsi0_pll_post_out_div_clk       0        0        0   221759994          0     0  50000         Y
> > >>>>          dsi0_pll_bit_clk               2        2        0   887039978          0     0  50000         Y
> > >>>>             dsi0_pclk_mux               1        1        0   887039978          0     0  50000         Y
> > >>>>                dsi0_phy_pll_out_dsiclk       1        1        0   147839997          0     0  50000         Y
> > >>>>                   disp_cc_mdss_pclk0_clk_src       1        1        0   147839997          0     0  50000         Y
> > >>>>                      disp_cc_mdss_pclk0_clk       1        1        0   147839997          0     0  50000         Y
> > >>>>             dsi0_pll_by_2_bit_clk       0        0        0   443519989          0     0  50000         Y
> > >>>>             dsi0_phy_pll_out_byteclk       1        1        0   110879997          0     0  50000         Y
> > >>>>                disp_cc_mdss_byte0_clk_src       2        2        0   110879997          0     0  50000         Y
> > >>>>                   disp_cc_mdss_byte0_div_clk_src       1        1        0    55439999          0     0  50000         Y
> > >>>>                      disp_cc_mdss_byte0_intf_clk       1        1        0    55439999          0     0  50000         Y
> > >>>>                   disp_cc_mdss_byte0_clk       1        1        0   110879997          0     0  50000         Y
> > >>> ------------------
> > >>>
> > >>>
> > >>
> > >> This is almost exactly what I saw on my devices as well, you get a
> > >> difference of "just some Hz" (which can be totally ignored), because
> > >> of how the calculation is done now.
> > >>
> > >> Thing is, what you see as PIXEL and BYTE clocks *before* the change is
> > >> Linux thinking that your DSI is at that frequency, while the PLL will
> > >> output *half* the rate, which is exactly what the patch fixes.
> > >>
> > >> "Fun" story is: the Xperia XZ1 (8998) and XZ (8996) have got the same
> > >> display... by lowering the DSI rate on the MSM8996 phone by half, I
> > >> get the same *identical* issues as the 8998 one without this patch.
> > >> The clocks all match between one and another, because.. it's.. the same
> > >> display, after all.
> > >>
> > >> It is because of the aforementioned test that I have raised doubts about
> > >> the TI chip driver (or anything else really).. but then, anything is
> > >> possible.
> > >
> > > It does look like, *so far* the TI bridge chip is only used on qc
> > > platforms (according to grep'ing dts), so I suppose I can't rule out
> > > bugs which cancel each other out.  Although there are various other
> > > bridges used (for ex, the sdm845 rb3 board has some dsi->hdmi bridge)
> > >
> >
> > Argh...
> >
> > > I guess it would be useful if we could measure the clk somehow to
> > > confirm that it is running at the rate we think it is..
> > >
> >
> > I totally agree with you on this, I actually wanted to do it the proper
> > way, but then these clocks are really too high for my cheap oscilloscope
> > and I couldn't... :(
>
> Ok, there might actually be a way to do this.. there is a testclock
> utility (added sboyd who wrote it in CC):
>
> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/refs/heads/main/chipset-qc845/dev-util/testclock/files/testclock.py
>
> there is a similar thing for sc7180.. for other devices, I guess it
> would require some porting..
>
> Looking at disp_cc_mdss_byte0_clk and disp_cc_mdss_pclk0_clk on
> sc7180, the rates returned by testclock roughly match what is in
> clk_summary, ie. within rounding error
>

So Doug Anderson pointed out that we can actually read the DSI clock
from the bridge, if we put it in "automatic" mode, from him:

1. Boot up.
2. i2cget -f -y 2 0x2d 0x12 # reads calculated rate that we programmed
3. i2cset -f -y 2 0x2d 0x12 0 # switch to automatic mode
4. i2cget -f -y 2 0x2d 0x12 # reads measured rate; repeat this a bunch
and it should go up/down by 1 since measurement isn't perfect.

What I see without this patch:

localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cset -f -y 2 0x2d 0x12 0
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x59
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ #

And with this patch:

localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cset -f -y 2 0x2d 0x12 0
localhost ~ # i2cget -f -y 2 0x2d 0x12
0xb1
localhost ~ # i2cget -f -y 2 0x2d 0x12
0xb2
localhost ~ # i2cget -f -y 2 0x2d 0x12
0xb2
localhost ~ #

So this patch is doubling the rate

BR,
-R