Messages in this thread Patch in this message | | | Subject | Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler | From | AngeloGioacchino Del Regno <> | Date | Mon, 1 Feb 2021 11:11:30 +0100 |
| |
Il 31/01/21 20:50, Rob Clark ha scritto: > 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) >
First frame of the splash means that something is "a bit" wrong... ...like the DSI clock is a little off.
I don't have such hardware, otherwise I would've tried... but what you describe is a bit strange. Is there any other older qcom platform using this chip? Any other non-qcom platform? Is the driver for the SN65DSI86 surely fine? Anyway, as you know, I would never propose untested patches nor partially working ones for any reason: I'm sorry that this happened.
In any case, just to be perfectly transparent, while being here waiting for review, this patch series got tested on more smartphones, even ones that I don't personally own, with different displays.
For your reference, here's a list (all MSM8998..): - OnePlus 5 (1920x1080) - F(x)Tec Pro 1 (2160x1080) - Sony Xperia XZ1 Compact (1280x720) - Sony Xperia XZ1 (1920x1080) - Sony Xperia XZ Premium (3840x2160)
> 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
So much "fun". This makes me think something about the PCC block doing the wrong thing (getting misconfigured).
> 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??) >
From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock cannot produce (logically, at least) this, so I say that it is very unlikely for this to be a consequence of the 10nm pll fixes...
...unless the bootloader is not configuring the DSI rates, but that's also veeeeery unlikely (it always does, or it always does not).
> 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. >
I would be happy if someone from Qualcomm takes a look: after all, there is no documentation and they're the only ones that can verify this kind of stuff. Please, Qualcomm.
Besides that, if there's anything I can help with to solve this riddle, I'm here for you.
Yours, -- Angelo
> 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 >>
| |