Re: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

["André Draszik" <andre.draszik@linaro.org>]

On Fri, 2024-08-09 at 10:02 +0100, André Draszik wrote:
> Hi Peng,
> 
> On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > > +static int __init of_clk_drop_stdout_clocks(void) {
> > > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > > +		clk_put(of_clk_stdout_clks.clks[i]);
> > > +	}
> > > +
> > > +	kfree(of_clk_stdout_clks.clks);
> > > +
> > > +	/*
> > > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > > +	 * during further module loading, as we then wouldn't have a
> > > way to
> > > +	 * drop the references (and associated allocations) ever again.
> > > +	 */
> > > +	of_clk_stdout_clks.bump_refs = false;
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > 
> > If the uart driver is built as module, this might break earlycon.
> > Before uart driver loaded, clk disabled per my understanding.
> 
> You're right.
> 
> With this in mind, I'm not sure then if a generic solution is possible...
> 
> I guess it has to be duplicated into the platforms after all and platforms
> can enable this if they opt to disallow uart as module?
> 
> Any other suggestions?
> 
> > > +
> > >  /**
> > >   * struct of_clk_provider - Clock provider registration structure
> > >   * @link: Entry in global list of clock providers @@ -5031,6 +5156,8
> > > @@ int of_clk_add_provider(struct device_node *np,
> > > 
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > 
> > > +	of_clk_bump_stdout_clocks();
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> > > device_node *np,
> > > 
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > 
> > > +	of_clk_bump_stdout_clocks();
> > 
> > If clock driver is built as module,  the will make the
> > clocks will be always enabled, if my understanding is correct.
> 
> until late_initcall_sync(), at which point it'll be disabled before the uart
> driver has probed, yes :-(

Sorry, ignore that. If clock driver is built as module, the code to bump
the clocks is disabled by the time this code runs (due to setting the flag
as part of late_initcall_sync(of_clk_drop_stdout_clocks)), in other words
it will not bump the clocks at all in that case and behaviour is as before.

Did I miss something?

Cheers,
Andre'