Re: [RFC] clk: Mark HW enabled clocks as enabled in core

[Abel Vesa <abel.vesa@nxp.com>]

On 21-01-27 11:47:20, Sascha Hauer wrote:
> On Wed, Jan 27, 2021 at 12:12:20PM +0200, Abel Vesa wrote:
> > On 21-01-26 15:30:17, Sascha Hauer wrote:
> > > On Tue, Jan 26, 2021 at 03:12:39PM +0200, Abel Vesa wrote:
> > > > On 21-01-26 12:51:05, Sascha Hauer wrote:
> > > > > On Tue, Jan 26, 2021 at 01:21:36PM +0200, Abel Vesa wrote:
> > > > > > Some clocks are already enabled in HW even before the kernel
> > > > > > starts to boot. So, in order to make sure that these clocks do not
> > > > > > get disabled when clk_disable_unused call is done or when
> > > > > > reparenting clocks, we enable them in core on clock registration.
> > > > > > Such a clock will have to be registered with CLK_IGNORE_UNUSED flag
> > > > > > and also needs to have the is_enabled ops implemented.
> > > > > > 
> > > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> > > > > > ---
> > > > > >  drivers/clk/clk.c | 11 ++++++++++-
> > > > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > > index 3d751ae5bc70..26d55851cfa5 100644
> > > > > > --- a/drivers/clk/clk.c
> > > > > > +++ b/drivers/clk/clk.c
> > > > > > @@ -3416,6 +3416,7 @@ static int __clk_core_init(struct clk_core *core)
> > > > > >  	int ret;
> > > > > >  	struct clk_core *parent;
> > > > > >  	unsigned long rate;
> > > > > > +	bool is_hw_enabled = false;
> > > > > >  	int phase;
> > > > > >  
> > > > > >  	if (!core)
> > > > > > @@ -3558,12 +3559,20 @@ static int __clk_core_init(struct clk_core *core)
> > > > > >  		rate = 0;
> > > > > >  	core->rate = core->req_rate = rate;
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * If the clock has the CLK_IGNORE_UNUSED flag set and it is already
> > > > > > +	 * enabled in HW, enable it in core too so it won't get accidentally
> > > > > > +	 * disabled when walking the orphan tree and reparenting clocks
> > > > > > +	 */
> > > > > > +	if (core->flags & CLK_IGNORE_UNUSED && core->ops->is_enabled)
> > > > > > +		is_hw_enabled = clk_core_is_enabled(core);
> > > > > > +
> > > > > >  	/*
> > > > > >  	 * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
> > > > > >  	 * don't get accidentally disabled when walking the orphan tree and
> > > > > >  	 * reparenting clocks
> > > > > >  	 */
> > > > > > -	if (core->flags & CLK_IS_CRITICAL) {
> > > > > > +	if (core->flags & CLK_IS_CRITICAL || is_hw_enabled) {
> > > > > >  		unsigned long flags;
> > > > > >  
> > > > > >  		ret = clk_core_prepare(core);
> > > > > 
> > > > > This means that a bootloader enabled clock with CLK_IGNORE_UNUSED flag
> > > > > can effectively never be disabled because the prepare/enable count is 1
> > > > > without any user. This is the behaviour we want to have with critical
> > > > > clocks, but I don't think this is desired for clocks with the
> > > > > CLK_IGNORE_UNUSED flag.
> > > > > 
> > > > 
> > > > Here is the way I see it. Critical clocks means the system can't work
> > > > without, so do not ever disable/unprepare. The "ignore unused" flag
> > > > tells the core to not do anything to this clock, even if it is unused.
> > > > For now, it just leaves the clock alone, but the flag could be used for
> > > > some other stuff in the future.
> > > > 
> > > > Now, the behavior is entirely different.
> > > > 
> > > > For the "critical" clock disable/unprepare, the core does nothing
> > > > (returns without calling the disable/unprepare ops).
> > > > 
> > > > As for the "ignore unused", the clock can be disabled later on,
> > > > which would decrement the prepare/enable counter.
> > > > The imx earlycon serial driver could implement a late initcall,
> > > > that takes the clocks from the devicetree uart node and disables
> > > > them. The user doesn't even count in this situation.
> > > > 
> > > > Plus, there is no other reason someone would use the CLK_IGNORE_UNUSED,
> > > > other than leaving a clock that is already enabled stay as is (at least,
> > > > not with the current implementation). So why not mark it as enabled in 
> > > > the core, if the HW says it is enabled ?
> > > 
> > > The CLK_IGNORE_UNUSED is there from the start of the clock framework, so
> > > there is no commit message that tells what it shall be used for. AFAIR
> > > the flag was thought for being used with clocks which should not be
> > > disabled, but had no driver initially that used them.
> > > Implementation of this flag was likely broken from the start as well,
> > > because in this situation:
> > > 
> > >       a
> > >      / \
> > >     b   c (CLK_IGNORE_UNUSED)
> > > 
> > > When clk b is enabled/disabled then the parent of clock c is disabled as
> > > well, so CLK_IGNORE_UNUSED doesn't help at all. In that sense your patch
> > > really improves things, because the above example would be fixed.
> > > 
> > > Anyway, CLK_IGNORE_UNUSED is excessively used in the kernel, we have
> > > over 1000 clocks that have this flag set. With your patch all of a
> > > sudden all these clocks won't be disabled anymore and all these clocks
> > > will require some fixup to finally disable them when desired. I don't
> > > think this is a good idea.
> > > 
> > 
> > OK, then, how about this ?
> > 
> > I can add a flag to the each imx gate clock type used for each of the
> > uart clocks in every platform driver, flag that would say something like this:
> > "Mark the clock as enabled in the core if the HW says it is already enabled."
> > This would happen right after registration of the clock and the flag will be
> > imx specific (not in the core).
> 
> How do you want to implement the "Mark the clock as enabled in the core"
> part without using the clk consumer API, which is the thing you want to
> avoid?
> 

Hmm, this is actually a problem, there are no clk_hw enable/prepare API.
You are right about this one.

> > 
> > There would be the following advantages:
> > - only the uart clock that was left enabled by the bootloader will be left
> > on after the clk_disable_unused call
> 
> Actually this would be a regression compared to current upstream. Right
> now the UART clock is kept on until the clk_disable_unused call and
> disabled in that call. This is what we want, because at that time the
> UART driver has taken over and takes care of the clocks itself.
> When after clk_disable_unused() there is still a reference on the UART
> clock we could never turn it off.

They could be turned off from the late initcall from imx serial.

> 
> > - fixing the situation where a common parent of the uart clocks is reparenting
> > or disabled by some other child clock.
> 
> That's not an issue currently.

Yes, as of now, it's not an issue because we're treating the uart clocks as a
special case by controlling them straight from the platform driver. But I would
like to get rid of this special case. Why ? Because, in the future, the clock
drivers would only use the API from clk-provider header. The clk.h should move
away for good from the clock drivers.

> 
> > - the platform drivers will not use the clk consumer API anymore
> 
> I'm not sure how you want to archieve this.
> 

Basically, all the clock drivers should move towards clk_hw APIi only.

> > - any future imx platform could be using that flag for uart clocks
> 
> No flags are necessary currently.
> 
> What's so wrong with the way it's currently implemented? Currently with
> the array of possible UART clocks it's really not nice, but with Adams
> patch which parses the clocks directly from the device node providing
> the stdout UART the arrays are gone and it looks ok to me.

Again, the ultimate goal here is to remove every clock consumer API from
all the imx clock drivers. The uart clocks are, at this point, that ugly
special case we can't seem to get rid of. The future platforms will use
the same approach if we don't deal with it. At this point, the imx6/7/8
platforms are not entirely clk_hw based API due to this uart clocks thing.
I intend to move the older imx to clk_hw API and that will leave us with
the uart clocks being the only exception from the rule, using clock consumer
API, in all the imx clock platform drivers.