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
