Re: [PATCH] clk: qcom: gcc-qcm2290: CLK_OPS_PARENT_ENABLE flag for rcg2 clocks

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 21 Dec 02:50 PST 2021, Loic Poulain wrote:

> Hi Bjorn,
> 
> On Tue, 21 Dec 2021 at 00:40, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Mon 20 Dec 01:54 PST 2021, Loic Poulain 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@linaro.org>
> > > ---
> > >  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...

Regards,
Bjorn

> > PS. Unfortunately these RCGs doesn't allow us to reliably implement
> > is_enabled() and as such clk_disable_unused() will skip parking the
> > unused clocks and continue to disable the PLLs feeding them (if they are
> > unused). I don't think this is a problem for the clocks you list here,
> > but we certainly have a problem with this in the dispcc. So this is
> > currently being discussed. For now it's expected that booting without
> > "clk_ignore_unused" is "broken".
> 
> Ok, understood thanks.
> 
> 
> Loic