Hi Bjorn,
On Wed, 22 Dec 2021 at 01:20, Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:
> > > > When a rcg2 clock migrates to a new parent, both the old and new
> > > > parent clocks must be enabled to complete the transition. This can
> > > > be automatically performed by the clock core when a clock is flagged
> > > > with CLK_OPS_PARENT_ENABLE.
> > > >
> > > > Without this, we may hit rate update failures:
> > > > gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
> > > > WARNING: CPU: 1 PID: 82 at drivers/clk/qcom/clk-rcg2.c:122 update_config+0xe0/0xf0
> > > >
> > > > Fixes: 496d1a13d405 ("clk: qcom: Add Global Clock Controller driver for QCM2290")
> > > > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
> > > > ---
> > > > drivers/clk/qcom/gcc-qcm2290.c | 20 ++++++++++++++++++++
> > > > 1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/qcom/gcc-qcm2290.c b/drivers/clk/qcom/gcc-qcm2290.c
> > > > index b6fa7b8..9e1d88e 100644
> > > > --- a/drivers/clk/qcom/gcc-qcm2290.c
> > > > +++ b/drivers/clk/qcom/gcc-qcm2290.c
> > > > @@ -674,6 +674,7 @@ static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
> > > > .name = "gcc_usb30_prim_mock_utmi_clk_src",
> > > > .parent_data = gcc_parents_0,
> > > > .num_parents = ARRAY_SIZE(gcc_parents_0),
> > > > + .flags = CLK_OPS_PARENT_ENABLE,
> > >
> > > This seems like a correct fix for the obvious problem that we might end
> > > up invoking clk_set_rate() and clk_set_parent() for these clocks while
> > > their (and thereby themselves - in a software sense) are disabled.
> > >
> > >
> > > However, clocks that downstream are marked "enable_safe_config", may in
> > > addition be enabled by the hardware, behind out back. As such we must
> > > ensure that they always have a valid configuration, we do this by
> > > "parking" them on CXO whenever we're going to disable their parent
> > > clocks.
> > >
> > > Upstream we handle this by using the clk_rcg2_shared_ops, which will do
> > > exactly this.
> >
> > Ok, thanks for the explanation, so we actually need both
> > clk_rcg2_shared_ops and CLK_OPS_PARENT_ENABLE, the former parking the
> > clock under always-on CXO (safe source), allowing hardware to toggle it
> > on its own. The latter allows safe parent switching by enabling the
> > new parent clock before update (and old parent, but we don't care
> > since we are actually parked on CXO) . Is that correct?
> >
>
> If a clock is parked and we get a request to change its rate, then the
> old parent doesn't matter (as you say). But as we are done with the
> set_rate the new parent will be turned off, as such as soon as we've
> changed the parent we must park the RCG again.
>
> So for parked shared_ops RCGs we simply remember the new configuration
> of the set_rate until it's time to enable the RCG again.
>
> As such I don't think that CLK_OPS_PARENT_ENABLE adds any value (for the
> shared_ops RCGs), only a bit of overhead.
>
>
> That said, if I read the code correctly don't think we properly handles
> a clk_set_parent() of a parked shared RCGs today...
Indeed, but most of the time, clk_rcg2_set_rate_and_parent is used,
and seems correctly implemented for shared case. But in case of e.g.
assigned-parent from dts, the clk_set_parent may indeed fail. So we
need a dedicated clk_rcg2_shared_set_parent function, maybe something
like:
+static int clk_rcg2_shared_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+ int ret;
+ u32 cfg = rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
+
+ /*
+ * In case clock is disabled, update the CFG and don't hit the
+ * update bit of CMD register, which will be done in next enable().
+ */
+ ret = regmap_update_bits(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg),
+ CFG_SRC_SEL_MASK, cfg);
+ if (ret || !__clk_is_enabled(hw->clk))
+ return ret;
+
+ return update_config(rcg);
+}
Regards,
Loic
