Hi Bjorn,
On Tue, 21 Dec 2021 at 00:40, Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> 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@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?
> 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
