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

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

On Mon Oct 28, 2024 at 11:52 PM CET, Stephen Boyd wrote:
> Quoting Théo Lebrun (2024-10-24 05:50:16)
> > 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>;
> >         };
>
> Why is this fixed factor clk registered from DT vs. from the driver that
> registers pll-per? Is it useful to describe it in DT because the factor
> can change? Where does it physically exist? In the SoC?

Those are internal SoC clocks, yes. No reason for them to change from
one board to another. Adding them from the driver makes the most sense,
I'll patch that up.

> > 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.
>
> Another solution is to register the fixed factor clk from the pll-per
> clk provider.
>
> And yet another solution is to return EPROBE_DEFER for orphaned clks. We
> have everything in place for that but we ran into trouble with consumers
> wanting to get orphaned clks in their probe or during assigned-clocks
> handling.

No suprise this kind of edge-case behavior can cause trouble.

Thanks,

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