Re: [RFC PATCH v2 2/3] clk: qcom: implement RCG2 'parked' clock support

[Stephen Boyd <sboyd@kernel.org>]

Quoting Stephen Boyd (2023-11-03 18:24:47)
> Quoting Dmitry Baryshkov (2023-10-03 18:23:07)
> > +
> > +       ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return FIELD_GET(CFG_SRC_SEL_MASK, cfg) != rcg->safe_src_index;
> > +}
> > +
> > +static int clk_rcg2_parked_init(struct clk_hw *hw)
> > +{
> > +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> > +       const struct freq_tbl *f = rcg->freq_tbl;
> > +
> > +       regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &rcg->parked_cfg);
> 
> I need this part today to fix a stuck clk problem I see on trogdor.lazor
> where during registration a call to clk_ops::get_parent() sees the clk
> isn't enabled at boot (because there isn't a clk_ops::is_enabled()
> function) so clk_rcg2_shared_get_parent() reads the parent from the
> 'parked_cfg' value, which is zero. If the hardware actually has non-zero
> for the parent then the framework will get the wrong parent, which is
> what happens on trogdor when the devmode screen is shown. The parent is
> the display PLL instead of XO. I haven't dug far enough to understand
> why disabling unused clks wedges the branch when we try to enable it
> again, but not disabling unused clks fixes the problem or reading the
> config register at registration to get the proper parent also fixes it.
> I guess the problem is that we're switching the RCG value when we
> shouldn't be doing that.
> 

I looked at this more today. It seems that I need to both read the
config register at init and also move over the rcg to the safe source at
init (i.e. park the clk at init). That's doable with a call to
clk_rcg2_shared_disable() for the clk_ops::init callback. Otherwise I
get a stuck clk warning.

Either

 disp_cc_mdss_mdp_clk status stuck at 'off'

or

 disp_cc_mdss_rot_clk status stuck at 'on'

When I don't park the rcg, the disp_cc_mdss_rot_clk gets stuck during
disabling of unused clks. I think I understand that problem. What
happens is disp_cc_mdss_mdp_clk_src and disp_cc_mdss_rot_clk_src are
both sourcing from disp_cc_pll0 at boot. Fixing the parent mapping makes
it so that enabling and then disabling disp_cc_mdss_ahb_clk causes
disp_cc_pll0 to be turned off when disp_cc_mdss_rot_clk_src is sourced
from it (and the branch disp_cc_mdss_rot_clk is enabled). If we park
both the rcgs at clk registration time we avoid this problem because the
PLL is disabled but nothing is actually a child clk. The act of reading
the config register and stashing that in the 'parked_cfg' only helps
avoid duplicate calls to change the rate, and doesn't help when we try
to repoint the clk at XO when the parent PLL is off.

The part I still don't understand is why reading the config register at
init and stashing that in 'parked_cfg' fixes the disp_cc_mdss_mdp_clk
stuck at off problem. I see that the branch clk is turned off and on
many times during boot and there aren't any warnings regardless of
stashing the config register. That means we should be moving the RCG to
XO source, between forcibly enabling and disabling the RCG, which
presumably would complain about being unable to update the config
register, but it doesn't. Only after late init does the clk fail to
enable, and the source is still XO at that time. Something else must be
happening that wedges the branch to the point that it can't be
recovered. But simply reporting the proper parent is enough for
disp_cc_mdss_mdp_clk.