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

From: Rob Clark
Date: Tue Feb 02 2021 - 16:37:04 EST

On Tue, Feb 2, 2021 at 11:08 AM Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> On Tue, Feb 2, 2021 at 10:46 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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