Re: [PATCH v3 1/5] clk: Respect CLK_OPS_PARENT_ENABLE during recalc

[Stephen Boyd <sboyd@kernel.org>]

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()).

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.

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.