Re: [PATCH v5 4/4] clk: eyeq: add driver

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Stephen, Conor, Krzysztof, Michael, Rob,

On Thu Oct 24, 2024 at 12:12 AM CEST, Stephen Boyd wrote:
> Quoting Théo Lebrun (2024-10-23 04:08:31)
> > On Thu Oct 17, 2024 at 8:48 PM CEST, Stephen Boyd wrote:
> > > Quoting Théo Lebrun (2024-10-07 06:49:19)
> > > > +/* Required early for UART. */
> > >
> > > I still don't get this. UART isn't an early device. It's only the
> > > interrupt controller and the timer that matter. Does MIPS do something
> > > special for UARTs?
> > 
> > Our hardware has a PL011. That is AMBA stuff; they get probed before
> > platform devices by of_platform_bus_create(). "pll-per" on EyeQ5 must
> > be available at that time.
> > 
> > In concrete terms, if we don't register pll-per on EyeQ5 at
> > of_clk_init(), we stare at void because the serial fails probing.
> > I haven't digged into why EPROBE_DEFER doesn't do its job. Anyway we
> > don't want our serial to stall for some time during our boot process.
> > 
>
> Ok thanks for the details. It sounds like there's a bug in there
> somewhere. Eventually this should be removed.
>
> Can you dump_stack() in clk_get() when the "pll-per" clk is claimed?
>
> I suspect of_clk_get_hw_from_clkspec() is seeing NULL if
> of_clk_hw_onecell_get() is being used and the clk_hw pointer isn't set
> yet. NULL is a valid clk and it will be returned to the consumer. You'll
> want to write a custom 'get' function for of_clk_add_hw_provider() that
> returns -EPROBE_DEFER for any clk that isn't registered early. Then the
> AMBA stuff should defer probe until the "full" clk provider is
> registered.

You encouraged me to keep digging.

The bug is elsewhere: we do get valid clocks from PL011. Both clk_get()
calls give proper pointers.

The issue is that we are using `compatible = "fixed-factor-clock"`
clocks in the middle, and those don't wait for their parents to be
active.

Simplified clock graph is: pll-per -> occ-periph.
pll-per is register by our driver. occ-periph looks like:

	occ_periph: occ-periph {
		compatible = "fixed-factor-clock";
		clocks = <&olb EQ5C_PLL_PER>;
		#clock-cells = <0>;
		clock-div = <16>;
		clock-mult = <1>;
	};

Sequence is:
 - eqc_early_init(): it registers a clock provider that will return
   EPROBE_DEFER for our pll-per.
 - _of_fixed_factor_clk_setup(): it registers occ-periph, even though
   its parent is EPROBE_DEFER. clk_core_populate_parent_map() runs all
   fine without complaining; logical as it doesn't query the clk_hw for
   its parent, it only stores indexes.
 - amba_get_enable_pclk(): it does a clk_get() which works because
   occ-periph exists.

Maybe __clk_register() should check the clk_hw for each parent: if any
is an EPROBE_DEFER then it should EPROBE_DEFER itself? That looks like
a rather big behavioral change.

The other solution is to keep as-is: provide all clocks consumed by
fixed-factor-clocks at of_clk_init() stage.

Hoping I provided enough info,
Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com