Message-ID: <5200578.ejJDZkT8p0@workhorse>
Date: Wed, 28 Jan 2026 20:59:24 +0100
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Mark Brown <broonie@...nel.org>, Stephen Boyd <sboyd@...nel.org>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Michael Turquette <mturquette@...libre.com>,
 Dong Aisheng <aisheng.dong@....com>,
 Matthias Brugger <matthias.bgg@...il.com>,
 Yassine Oudjana <y.oudjana@...tonmail.com>,
 Laura Nao <laura.nao@...labora.com>,
 Nícolas F.  R.  A. Prado <nfraprado@...labora.com>,
 Chia-I Wu <olvaffe@...il.com>, Chen-Yu Tsai <wenst@...omium.org>,
 kernel@...labora.com, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v3 1/5] clk: Respect CLK_OPS_PARENT_ENABLE during recalc

On Wednesday, 28 January 2026 20:01:22 Central European Standard Time Stephen Boyd wrote:
> Quoting Nicolas Frattaroli (2026-01-28 09:04:55)
> > On Wednesday, 28 January 2026 15:47:08 Central European Standard Time Mark Brown wrote:
> > > 
> > > For the Avenger96 that says:
> > > 
> > > [    0.347816] __clk_core_init: enabling parent ck_hse for ck_per
> > > [    0.352230] __clk_core_init: disabling parent ck_hse for ck_per
> > > [    0.358207] __clk_core_init: enabling parent pll1_p for ck_mpu
> > > [    0.364005] __clk_core_init: disabling parent pll1_p for ck_mpu
> > > 
> > > https://lava.sirena.org.uk/scheduler/job/2412562#L563
> > > 
> > 
> > Okay, on this one, there may be a problem in the clock tree.
> > ck_mpu is marked CLK_IS_CRITICAL, but its parent, pll1_p, is not. Clock
> > core doesn't seem to check whether any children of a clock are marked as
> > critical before disabling it.
> > 
> > I'm not super familiar with the intended semantics of critical clocks.
> > If we need to manually mark all parents of critical clocks as critical
> > as well, then a (potentially partial) fix for the Avenger96 might be:
> > 
> 
> Marking parents critical hasn't been strictly necessary so far because
> we've been relying on the prepare/enable count on a critical child to
> keep the parent prepared/enabled.
> 
> > 
> > I just looked for CLK_IS_CRITICAL clocks in that file that have the
> > CLK_OPS_PARENT_ENABLE flag, and marked their PLL parents as critical
> > as well.
> > 
> > An alternate approach would be to skip the parent enable/disable pair
> > in the problematic patch in __clk_core_init for clocks that are marked
> > as critical, because if the parent wasn't on for a critical clock then
> > we wouldn't be in that function, we would be dead.
> 
> The parent may not be known yet in __clk_core_init() because we lazily
> find parents. Putting it another way, we can get to the recalc_rate()
> call in __clk_core_init() for a clk with CLK_OPS_PARENT_ENABLE when the
> parent pointer is NULL. This hasn't been a problem because when we adopt
> the orphan clk the rate is recalculated (see
> clk_core_reparent_orphans_nolock()).

In this instance, it doesn't seem like the parent pointer is null, since
it does print a parent clock name. I think the problem really is that the
parent gets disabled before the CLK_IS_CRITICAL check re-enables it.

> I don't see an easy way out of this problem because in general we need
> to know a clk with CLK_OPS_PARENT_ENABLE is parented enough to be able
> to read the registers when calling clk_ops like recalc_rate() (and
> probably get_parent() as well meaning that clk_op can't touch
> hardware?). Maybe the simplest approach is to skip any sort of hardware
> access when this flag is set during setup and reparenting.

The issue both SoCs seem to run into is that the parent gets disabled
when it shouldn't be, which I fixed in [1]. I don't think any platform
that already worked before this change will stop working with this fix
and the fix for the fix in [1], because if the parent is absent, then
the enable/disables are no-ops anyway. NULL clocks are explicitly
handled by the common clock framework.

> BTW, I don't know what this patch is actually fixing, so I'm going to
> revert it. When it is resubmitted please describe the problem you're
> seeing.

The justification got lost in the revisions of the series this was done
in. It's to deal with the MediaTek MT8196's MFGPLL clocks, where the
parent clock needs to be on before the clock registers of the child
clock can be touched in any way. I first modeled this as an RPM clock,
but Angelo didn't like this approach, and with some hackery I could sort
of see that the child clock indeed does not tick if the would-be RPM clock
is off, so it must be a parent clock instead.

The problem with making it a parent clock and adding this flag to it though
is that the flag is not respected in recalc_rate, so it reads from an
unclocked register.

If you want me to make it an RPM clock instead, I can do that, but then
we're leaking that the NXP i.MX8MP has a broken clock driver into the
device tree bindings of the MediaTek MT8196 PLLs, which seems like a hard
sell for the DT bindings maintainers.

So in conclusion, I'm fairly confident I've found a fix for the fix, and
I'm also very confident the problem doesn't go as deep as you fear. I'll
squash the fix for the fix into the fix when I resubmit the fix with a
fix for the i.MX, but that'll have to wait until after -rc1 I think.

Kind regards,
Nicolas Frattaroli

Link: https://lore.kernel.org/all/20260128-ops-parent-enable-fix-v1-1-ff39cc37f98d@collabora.com/ [1]

Powered by blists - more mailing lists