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 ---
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
Hi Stephen,
On 08/06/2021 22:55, Stephen Boyd wrote:
Thank you for the lengthy explanation. May I suggest another solution: - Apply https://lore.kernel.org/linux-arm-msm/010101750064e17e-3db0087e-fc37-494d-aa...
- And make save_state for 7nm and 10nm cache vco freq (like 14nm does).
What do you think?
Quoting Dmitry Baryshkov (2021-06-08 14:41:21)
Maybe that can be done for the next merge window? I'd like to get the smallest possible patch in as a fix for this cycle given that the Fixes tag is a recent regression introduced during the most recent merge window.
I honestly have no idea what's going on with the clk driver in these files but from the clk framework perspective there are bigger problems than caching the vco freq properly. As I stated in the commit text above, calling set_rate from prepare is plain bad. That should stop.
From my quick glance, the patch you mention looks like another
workaround instead of a proper fix. Why would we need to save the registers at boot and then snap them back into place on enable? Maybe we shouldn't reset the phy after registering the clks? Instead register the clks after the phy is reset so recalc_rate can accurately calculate the frequency. I suppose that would break continuous splash screen though where you want the PLL to stay running the entire boot? But then issuing a reset would break that, wouldn't it? As you can see I'm pretty confused about how this is all supposed to work.
Note: my problem isn't about recovering what boot sets, it's mostly exposing incorrect usage of the clk framework in this driver because it relies on this chain of events:
1) recalc rate calculates something different than what is set via clk_set_rate()
2) clk_set_rate() is called with the different rate
3) clk_prepare() is called to actually enable the PLL and wait for it to start
If clk_prepare() was called before clk_set_rate(), which is totally valid, then it should similarly fail and think the rate is 0 and the PLL won't lock. Does implementing save_state fix that? If so, it seems like we have two pieces of code working around each other, maybe for suspend/resume purposes.
I admit this patch I'm proposing is another workaround, but at least it makes things work again without going off and adding a bunch of register save/restore logic.
On 09/06/2021 01:11, Stephen Boyd wrote:
Could you please spend few more words, on why calling the clock's set_rate() callback from the same clock's prepare callback is bad? I don't see how this would affect downstream clocks (as we do not change the frequency, we just set the registers).
The problem here is not about registration. PHY gets reset not just only on registration, it also might be powered off/reset later (e.g. when the DSI output is disabled for any reason). And during each of these resets we have to keep the PLL state. So keeping the state from the bootloaders seems also natural to me.
Yes, the continuous splash would be broken by resetting the PHY early.
Ah, we were safe here because the DSI driver first calls clk_set_rate, then clk_prepare_enable for the link clocks, which in turn makes VCO clock first receive the rate and then enable PLL.
I think we can not come with the better solution in the next day or two, we should merge your workaround. For now I'm trying to understand what are the alternatives and which of them can be better.
Also it's not about registers save/resore. We can add a call to recalc_rate to pll_save_state (as 14nm driver does).
Quoting Dmitry Baryshkov (2021-06-09 09:03:14)
The clk framework is caching things and we don't want clk providers to be calling into the clk framework again from within the clk ops. This recursion into the framework is why we have a nasty recursive aware lock in the clk framework that we're never going to get rid of if more and more code keeps recursing into the framework.
I think you're saying that the code is reusing the set rate clk op without going through the framework. That's mostly OK, as long as some proper locking is in place so that clk_prepare() can't call down into the clk op while clk_set_rate() is also calling down into the same clk op. Do we have to call the set rate code here on prepare so that the clk frequency can be restored? Maybe the name of the function threw me off.
Got it. This seems like another version of the half-baked save_context()/restore_context() clk ops. Maybe we should add some sort of save/restore a clk and all its children API that clk providers can call that calls the clk ops to save and restore and then puts things back into place. Then the clk framework will be aware of what's going on and be able to cache frequency and enable state, etc.
Yep.
The recalc_rate function can be called many times even when nothing has changed, similarly the determine_rate/round_rate callback can be called many times before the framework decides what it really wants to use. Please don't bolt on state saving logic to recalc_rate. I'd prefer that recalc_rate does one thing, calculate the frequency of the clk, and return it to the framework.
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
Stephen Boyd