Messages in this thread Patch in this message | | | Subject | Re: [PATCH] drm/msm/dsi: Stash away calculated vco frequency on recalc | From | Dmitry Baryshkov <> | Date | Wed, 9 Jun 2021 00:41:21 +0300 |
| |
Hi Stephen,
On 08/06/2021 22:55, Stephen Boyd wrote: > A problem was reported on CoachZ devices where the display wouldn't come > up, or it would be distorted. It turns out that the PLL code here wasn't > getting called once dsi_pll_10nm_vco_recalc_rate() started returning the > same exact frequency, down to the Hz, that the bootloader was setting > instead of 0 when the clk was registered with the clk framework. > > After commit 001d8dc33875 ("drm/msm/dsi: remove temp data from global > pll structure") we use a hardcoded value for the parent clk frequency, > i.e. VCO_REF_CLK_RATE, and we also hardcode the value for FRAC_BITS, > instead of getting it from the config structure. This combination of > changes to the recalc function allows us to properly calculate the > frequency of the PLL regardless of whether or not the PLL has been > clk_prepare()d or clk_set_rate()d. That's a good improvement. > > Unfortunately, this means that now we won't call down into the PLL clk > driver when we call clk_set_rate() because the frequency calculated in > the framework matches the frequency that is set in hardware. If the rate > is the same as what we want it should be OK to not call the set_rate PLL > op. The real problem is that the prepare op in this driver uses a > private struct member to stash away the vco frequency so that it can > call the set_rate op directly during prepare. Once the set_rate op is > never called because recalc_rate told us the rate is the same, we don't > set this private struct member before the prepare op runs, so we try to > call the set_rate function directly with a frequency of 0. This > effectively kills the PLL and configures it for a rate that won't work. > Calling set_rate from prepare is really quite bad and will confuse any > downstream clks about what the rate actually is of their parent. Fixing > that will be a rather large change though so we leave that to later. > > For now, let's stash away the rate we calculate during recalc so that > the prepare op knows what frequency to set, instead of 0. This way > things keep working and the display can enable the PLL properly. In the > future, we should remove that code from the prepare op so that it > doesn't even try to call the set rate function. > > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Abhinav Kumar <abhinavk@codeaurora.org> > Fixes: 001d8dc33875 ("drm/msm/dsi: remove temp data from global pll structure") > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Thank you for the lengthy explanation. May I suggest another solution: - Apply https://lore.kernel.org/linux-arm-msm/010101750064e17e-3db0087e-fc37-494d-aac9-2c2b9b0a7c5b-000000@us-west-2.amazonses.com/
- And make save_state for 7nm and 10nm cache vco freq (like 14nm does).
What do you think?
> --- > > I didn't update the 14nm file as the caching logic looks different. But > between the 7nm and 10nm files it looks practically the same. > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 1 + > drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c > index 34bc93548fcf..657778889d35 100644 > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c > @@ -432,6 +432,7 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw, > pll_freq += div_u64(tmp64, multiplier); > > vco_rate = pll_freq; > + pll_10nm->vco_current_rate = vco_rate; > > DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x", > pll_10nm->phy->id, (unsigned long)vco_rate, dec, frac); > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > index e76ce40a12ab..6f96fbac8282 100644 > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > @@ -460,6 +460,7 @@ static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw, > pll_freq += div_u64(tmp64, multiplier); > > vco_rate = pll_freq; > + pll_7nm->vco_current_rate = vco_rate; > > DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x", > pll_7nm->phy->id, (unsigned long)vco_rate, dec, frac); > > base-commit: 8124c8a6b35386f73523d27eacb71b5364a68c4c >
-- With best wishes Dmitry
| |