[v3,1/2] clk: Add generic sync_state callback for disabling unused clocks

Commit Message

Abel Vesa Dec. 27, 2022, 8:45 p.m. UTC

There are unused clocks that need to remain untouched by clk_disable_unused,
and most likely could be disabled later on sync_state. So provide a generic
sync_state callback for the clock providers that register such clocks.
Then, use the same mechanism as clk_disable_unused from that generic
callback, but pass the device to make sure only the clocks belonging to
the current clock provider get disabled, if unused. Also, during the
default clk_disable_unused, if the driver that registered the clock has
the generic clk_sync_state_disable_unused callback set for sync_state,
skip disabling its clocks.

Signed-off-by: Abel Vesa <[e-mail]>
---

Changes since v2:
 * dropped the check for sync_state callback (clk_sync_state_disable_unused),
   like Dmitry suggested

 drivers/clk/clk.c            | 57 +++++++++++++++++++++++++++++-------
 include/linux/clk-provider.h |  1 +
 2 files changed, 47 insertions(+), 11 deletions(-)

Comments

Dmitry Baryshkov Jan. 5, 2023, 2:03 p.m. UTC | #1

On 27/12/2022 22:45, Abel Vesa wrote:
> There are unused clocks that need to remain untouched by clk_disable_unused,
> and most likely could be disabled later on sync_state. So provide a generic
> sync_state callback for the clock providers that register such clocks.
> Then, use the same mechanism as clk_disable_unused from that generic
> callback, but pass the device to make sure only the clocks belonging to
> the current clock provider get disabled, if unused. Also, during the
> default clk_disable_unused, if the driver that registered the clock has
> the generic clk_sync_state_disable_unused callback set for sync_state,
> skip disabling its clocks.
> 
> Signed-off-by: Abel Vesa <[e-mail]>
> ---
> 
> Changes since v2:
>   * dropped the check for sync_state callback (clk_sync_state_disable_unused),
>     like Dmitry suggested
> 
>   drivers/clk/clk.c            | 57 +++++++++++++++++++++++++++++-------
>   include/linux/clk-provider.h |  1 +
>   2 files changed, 47 insertions(+), 11 deletions(-)

Reviewed-by: Dmitry Baryshkov <[e-mail]>

Bjorn Andersson Jan. 6, 2023, 5:22 p.m. UTC | #2

On Tue, Dec 27, 2022 at 10:45:27PM +0200, Abel Vesa wrote:
> There are unused clocks that need to remain untouched by clk_disable_unused,
> and most likely could be disabled later on sync_state. So provide a generic
> sync_state callback for the clock providers that register such clocks.
> Then, use the same mechanism as clk_disable_unused from that generic
> callback, but pass the device to make sure only the clocks belonging to
> the current clock provider get disabled, if unused. Also, during the
> default clk_disable_unused, if the driver that registered the clock has
> the generic clk_sync_state_disable_unused callback set for sync_state,
> skip disabling its clocks.
> 
> Signed-off-by: Abel Vesa <[e-mail]>

Reviewed-by: Bjorn Andersson <[e-mail]>

Regards,
Bjorn

> ---
> 
> Changes since v2:
>  * dropped the check for sync_state callback (clk_sync_state_disable_unused),
>    like Dmitry suggested
> 
>  drivers/clk/clk.c            | 57 +++++++++++++++++++++++++++++-------
>  include/linux/clk-provider.h |  1 +
>  2 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e62552a75f08..ac7182903d88 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1302,14 +1302,26 @@ static void clk_core_disable_unprepare(struct clk_core *core)
>  	clk_core_unprepare_lock(core);
>  }
>  
> -static void __init clk_unprepare_unused_subtree(struct clk_core *core)
> +static void clk_unprepare_unused_subtree(struct clk_core *core,
> +						struct device *dev)
>  {
> +	bool from_sync_state = !!dev;
>  	struct clk_core *child;
>  
>  	lockdep_assert_held(&prepare_lock);
>  
>  	hlist_for_each_entry(child, &core->children, child_node)
> -		clk_unprepare_unused_subtree(child);
> +		clk_unprepare_unused_subtree(child, dev);
> +
> +	if (from_sync_state && core->dev != dev)
> +		return;
> +
> +	/*
> +	 * clock will be unprepared on sync_state,
> +	 * so leave as is for now
> +	 */
> +	if (!from_sync_state && dev_has_sync_state(core->dev))
> +		return;
>  
>  	if (core->prepare_count)
>  		return;
> @@ -1332,15 +1344,27 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>  	clk_pm_runtime_put(core);
>  }
>  
> -static void __init clk_disable_unused_subtree(struct clk_core *core)
> +static void clk_disable_unused_subtree(struct clk_core *core,
> +					struct device *dev)
>  {
> +	bool from_sync_state = !!dev;
>  	struct clk_core *child;
>  	unsigned long flags;
>  
>  	lockdep_assert_held(&prepare_lock);
>  
>  	hlist_for_each_entry(child, &core->children, child_node)
> -		clk_disable_unused_subtree(child);
> +		clk_disable_unused_subtree(child, dev);
> +
> +	if (from_sync_state && core->dev != dev)
> +		return;
> +
> +	/*
> +	 * clock will be disabled on sync_state,
> +	 * so leave as is for now
> +	 */
> +	if (!from_sync_state && dev_has_sync_state(core->dev))
> +		return;
>  
>  	if (core->flags & CLK_OPS_PARENT_ENABLE)
>  		clk_core_prepare_enable(core->parent);
> @@ -1378,7 +1402,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>  		clk_core_disable_unprepare(core->parent);
>  }
>  
> -static bool clk_ignore_unused __initdata;
> +static bool clk_ignore_unused;
>  static int __init clk_ignore_unused_setup(char *__unused)
>  {
>  	clk_ignore_unused = true;
> @@ -1386,35 +1410,46 @@ static int __init clk_ignore_unused_setup(char *__unused)
>  }
>  __setup("clk_ignore_unused", clk_ignore_unused_setup);
>  
> -static int __init clk_disable_unused(void)
> +static void __clk_disable_unused(struct device *dev)
>  {
>  	struct clk_core *core;
>  
>  	if (clk_ignore_unused) {
>  		pr_warn("clk: Not disabling unused clocks\n");
> -		return 0;
> +		return;
>  	}
>  
>  	clk_prepare_lock();
>  
>  	hlist_for_each_entry(core, &clk_root_list, child_node)
> -		clk_disable_unused_subtree(core);
> +		clk_disable_unused_subtree(core, dev);
>  
>  	hlist_for_each_entry(core, &clk_orphan_list, child_node)
> -		clk_disable_unused_subtree(core);
> +		clk_disable_unused_subtree(core, dev);
>  
>  	hlist_for_each_entry(core, &clk_root_list, child_node)
> -		clk_unprepare_unused_subtree(core);
> +		clk_unprepare_unused_subtree(core, dev);
>  
>  	hlist_for_each_entry(core, &clk_orphan_list, child_node)
> -		clk_unprepare_unused_subtree(core);
> +		clk_unprepare_unused_subtree(core, dev);
>  
>  	clk_prepare_unlock();
> +}
> +
> +static int __init clk_disable_unused(void)
> +{
> +	__clk_disable_unused(NULL);
>  
>  	return 0;
>  }
>  late_initcall_sync(clk_disable_unused);
>  
> +void clk_sync_state_disable_unused(struct device *dev)
> +{
> +	__clk_disable_unused(dev);
> +}
> +EXPORT_SYMBOL_GPL(clk_sync_state_disable_unused);
> +
>  static int clk_core_determine_round_nolock(struct clk_core *core,
>  					   struct clk_rate_request *req)
>  {
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 842e72a5348f..cf1adfeaf257 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
>  		void __iomem *reg, u8 shift, u8 width,
>  		u8 clk_divider_flags, const struct clk_div_table *table,
>  		spinlock_t *lock);
> +void clk_sync_state_disable_unused(struct device *dev);
>  /**
>   * clk_register_divider - register a divider clock with the clock framework
>   * @dev: device registering this clock
> -- 
> 2.34.1
>

Bjorn Andersson Jan. 10, 2023, 5:17 p.m. UTC | #3

On Tue, 27 Dec 2022 22:45:27 +0200, Abel Vesa wrote:
> There are unused clocks that need to remain untouched by clk_disable_unused,
> and most likely could be disabled later on sync_state. So provide a generic
> sync_state callback for the clock providers that register such clocks.
> Then, use the same mechanism as clk_disable_unused from that generic
> callback, but pass the device to make sure only the clocks belonging to
> the current clock provider get disabled, if unused. Also, during the
> default clk_disable_unused, if the driver that registered the clock has
> the generic clk_sync_state_disable_unused callback set for sync_state,
> skip disabling its clocks.
> 
> [...]

Applied, thanks!

[1/2] clk: Add generic sync_state callback for disabling unused clocks
      commit: 26b36df7516692292312063ca6fd19e73c06d4e7
[2/2] clk: qcom: sdm845: Use generic clk_sync_state_disable_unused callback
      commit: 99c0f7d35c4b204dd95ba50e155f32c99695b445

Best regards,

Stephen Boyd Feb. 18, 2023, 5:38 a.m. UTC | #4

Quoting Abel Vesa (2022-12-27 12:45:27)
> There are unused clocks that need to remain untouched by clk_disable_unused,
> and most likely could be disabled later on sync_state. So provide a generic
> sync_state callback for the clock providers that register such clocks.
> Then, use the same mechanism as clk_disable_unused from that generic
> callback, but pass the device to make sure only the clocks belonging to
> the current clock provider get disabled, if unused. Also, during the
> default clk_disable_unused, if the driver that registered the clock has
> the generic clk_sync_state_disable_unused callback set for sync_state,
> skip disabling its clocks.

How does that avoid disabling clks randomly in the clk tree? I'm
concerned about disabling an unused clk in the middle of the tree
because it doesn't have a driver using sync state, while the clk is the
parent of an unused clk that is backed by sync state.

   clk A -->  clk B 

clk A: No sync state
clk B: sync state

clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
from late init. Imagine clk A is the root of the tree.

	clk_disable_unused_subtree(clk_core A)
	  clk_disable_unused_subtree(clk_core B)
	    if (from_sync_state && core->dev != dev)
	      return;
	  ...
	  clk core A->ops->disable()

clk core B is off now?

Also sync_state seems broken right now. I saw mka mentioned that if you
have a device node enabled in your DT but never enable a driver for it
in the kernel we'll never get sync_state called. This is another
problem, but it concerns me that sync_state would make the unused clk
disabling happen at some random time or not at all.

Can the problem be approached more directly? If this is about fixing
continuous splash screen, then I wonder why we can't list out the clks
that we know are enabled by the bootloader in some new DT binding, e.g.:

	clock-controller {
		#clock-cells = <1>;
		boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
	};

Then mark those as "critical/don't turn off" all the way up the clk tree
when the clk driver probes by essentially incrementing the
prepare/enable count but not actually touching the hardware, and when
the clks are acquired by clk_get() for that device that's using them
from boot we make the first clk_prepare_enable() do nothing and not
increment the count at all. We can probably stick some flag into the
'struct clk' for this when we create the handle in clk_get() so that the
prepare and enable functions can special case and skip over.

The sync_state hook operates on a driver level, which is too large when
you consider that a single clk driver may register hundreds of clks that
are not related. We want to target a solution at the clk level so that
any damage from keeping on all the clks provided by the controller is
limited to just the drivers that aren't probed and ready to handle their
clks. If sync_state could be called whenever a clk consumer consumes a
clk it may work? Technically we already have that by the clk_hw_provider
function but there isn't enough information being passed there, like the
getting device.

> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 842e72a5348f..cf1adfeaf257 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
>                 void __iomem *reg, u8 shift, u8 width,
>                 u8 clk_divider_flags, const struct clk_div_table *table,
>                 spinlock_t *lock);
> +void clk_sync_state_disable_unused(struct device *dev);

This is a weird place to put this. Why not in the helper functions
section?

>  /**
>   * clk_register_divider - register a divider clock with the clock framework
>   * @dev: device registering this clock

Abel Vesa Feb. 20, 2023, 3:46 p.m. UTC | #5

On 23-02-17 21:38:22, Stephen Boyd wrote:
> Quoting Abel Vesa (2022-12-27 12:45:27)
> > There are unused clocks that need to remain untouched by clk_disable_unused,
> > and most likely could be disabled later on sync_state. So provide a generic
> > sync_state callback for the clock providers that register such clocks.
> > Then, use the same mechanism as clk_disable_unused from that generic
> > callback, but pass the device to make sure only the clocks belonging to
> > the current clock provider get disabled, if unused. Also, during the
> > default clk_disable_unused, if the driver that registered the clock has
> > the generic clk_sync_state_disable_unused callback set for sync_state,
> > skip disabling its clocks.
> 
> How does that avoid disabling clks randomly in the clk tree? I'm
> concerned about disabling an unused clk in the middle of the tree
> because it doesn't have a driver using sync state, while the clk is the
> parent of an unused clk that is backed by sync state.
> 
>    clk A -->  clk B 
> 
> clk A: No sync state
> clk B: sync state
> 
> clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> from late init. Imagine clk A is the root of the tree.
> 
> 	clk_disable_unused_subtree(clk_core A)
> 	  clk_disable_unused_subtree(clk_core B)
> 	    if (from_sync_state && core->dev != dev)
> 	      return;
> 	  ...
> 	  clk core A->ops->disable()
> 
> clk core B is off now?

Yes, that is correct. But the same thing is happening currently if the
clk_ignore_unused in not specified. At least with this new approach, we
get to leave unused clocks enabled either until sync_state is called or forever.
All the provider has to do is to implement a sync_state callback (or use
the generic one provided). So the provider of clk A would obviously need
a sync state callback registered.

> 
> Also sync_state seems broken right now. I saw mka mentioned that if you
> have a device node enabled in your DT but never enable a driver for it
> in the kernel we'll never get sync_state called. This is another
> problem, but it concerns me that sync_state would make the unused clk
> disabling happen at some random time or not at all.

Well, the fact that the sync state not being called because a driver for
a consumer device doesn't probe does not really mean it is broken. Just
because the consumer driver hasn't probed yet, doesn't mean it will
not probe later on.

That aside, rather than going with clk_ignore_unused all the time on
qcom platforms, at least in a perfect scenario (where sync state is
reached for all providers) the clocks get disabled.

> 
> Can the problem be approached more directly? If this is about fixing
> continuous splash screen, then I wonder why we can't list out the clks
> that we know are enabled by the bootloader in some new DT binding, e.g.:
> 
> 	clock-controller {
> 		#clock-cells = <1>;
> 		boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> 	};
> 
> Then mark those as "critical/don't turn off" all the way up the clk tree
> when the clk driver probes by essentially incrementing the
> prepare/enable count but not actually touching the hardware, and when
> the clks are acquired by clk_get() for that device that's using them
> from boot we make the first clk_prepare_enable() do nothing and not
> increment the count at all. We can probably stick some flag into the
> 'struct clk' for this when we create the handle in clk_get() so that the
> prepare and enable functions can special case and skip over.

Well, that means we need to play whack-a-mole by alsways adding such clocks to
devicetree.

> 
> The sync_state hook operates on a driver level, which is too large when
> you consider that a single clk driver may register hundreds of clks that
> are not related. We want to target a solution at the clk level so that
> any damage from keeping on all the clks provided by the controller is
> limited to just the drivers that aren't probed and ready to handle their
> clks. If sync_state could be called whenever a clk consumer consumes a
> clk it may work? Technically we already have that by the clk_hw_provider
> function but there isn't enough information being passed there, like the
> getting device.

Actually, from the multitude of clocks registered by one provider, the
ones already explicitely enabled (and obvisously their parents) by thier
consumer are safe. The only ones we need to worry about are the ones that
might be enabled by bootloader and need to remain on. With the sync state
approach, the latter mentioned clocks will either remain on indefinitely
or will be disabled on sync state. The provider driver is the only level
that has a registered sync state callback.

> 
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 842e72a5348f..cf1adfeaf257 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> >                 void __iomem *reg, u8 shift, u8 width,
> >                 u8 clk_divider_flags, const struct clk_div_table *table,
> >                 spinlock_t *lock);
> > +void clk_sync_state_disable_unused(struct device *dev);
> 
> This is a weird place to put this. Why not in the helper functions
> section?

Sure this can be moved.

> 
> >  /**
> >   * clk_register_divider - register a divider clock with the clock framework
> >   * @dev: device registering this clock

Bjorn Andersson Feb. 20, 2023, 4:21 p.m. UTC | #6

On Fri, Feb 17, 2023 at 09:38:22PM -0800, Stephen Boyd wrote:
> Quoting Abel Vesa (2022-12-27 12:45:27)
> > There are unused clocks that need to remain untouched by clk_disable_unused,
> > and most likely could be disabled later on sync_state. So provide a generic
> > sync_state callback for the clock providers that register such clocks.
> > Then, use the same mechanism as clk_disable_unused from that generic
> > callback, but pass the device to make sure only the clocks belonging to
> > the current clock provider get disabled, if unused. Also, during the
> > default clk_disable_unused, if the driver that registered the clock has
> > the generic clk_sync_state_disable_unused callback set for sync_state,
> > skip disabling its clocks.
> 
> How does that avoid disabling clks randomly in the clk tree? I'm
> concerned about disabling an unused clk in the middle of the tree
> because it doesn't have a driver using sync state, while the clk is the
> parent of an unused clk that is backed by sync state.
> 
>    clk A -->  clk B 
> 
> clk A: No sync state
> clk B: sync state
> 
> clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> from late init. Imagine clk A is the root of the tree.
> 
> 	clk_disable_unused_subtree(clk_core A)
> 	  clk_disable_unused_subtree(clk_core B)
> 	    if (from_sync_state && core->dev != dev)
> 	      return;
> 	  ...
> 	  clk core A->ops->disable()
> 
> clk core B is off now?
> 

I will have to give this some more thought. But this is exactly what we
have today; consider A being any builtin clock driver and B being any
clock driver built as modules, with relationship to A.

clk_disable_unused() will take down A without waiting for B, possibly
locking up parts of the clock hardware of B; or turning off the clocks
to IP blocks that rely on those clocks (e.g. display).

So my thought on this is that I don't think this patch negatively alter
the situation. But as it isn't recursive, this remains a problem that
needs to be fixed.

> Also sync_state seems broken right now. I saw mka mentioned that if you
> have a device node enabled in your DT but never enable a driver for it
> in the kernel we'll never get sync_state called. This is another
> problem, but it concerns me that sync_state would make the unused clk
> disabling happen at some random time or not at all.
> 

I don't think that sync_state is "broken".

There is no way to distinguish a driver not being built in, or a driver
being built as module but not yet loaded. The approach taken by
sync_state currently is optimistically speculative.

One alternative to this is found in the regulator framework, where we
have a 30 second timer triggering the late disable. The result of this
is that every time I end up in the ramdisk console because "root file
system can't be mounted", I have 25 second to figure out what the
problem is before the backlight goes out...

As such I do prefer the optimistic approach...

> Can the problem be approached more directly? If this is about fixing
> continuous splash screen, then I wonder why we can't list out the clks
> that we know are enabled by the bootloader in some new DT binding, e.g.:
> 
> 	clock-controller {
> 		#clock-cells = <1>;
> 		boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> 	};
> 

I was under the impression that we have ruled out this approach.

Presumably this list should not be a manually maintained list of display
clocks, and that means the bootloader would need to go in and  build
this list of all enabled clocks. I don't think this is practical.

> Then mark those as "critical/don't turn off" all the way up the clk tree
> when the clk driver probes by essentially incrementing the
> prepare/enable count but not actually touching the hardware, and when
> the clks are acquired by clk_get() for that device that's using them
> from boot we make the first clk_prepare_enable() do nothing and not
> increment the count at all. We can probably stick some flag into the
> 'struct clk' for this when we create the handle in clk_get() so that the
> prepare and enable functions can special case and skip over.
> 

The benefit of sync_state is that it kicks when the client drivers has
probed. As such, you can have e.g. the display driver clk_get(), then
probe defer on some other resource, and the clock state can remain
untouched.

> The sync_state hook operates on a driver level, which is too large when
> you consider that a single clk driver may register hundreds of clks that
> are not related. We want to target a solution at the clk level so that
> any damage from keeping on all the clks provided by the controller is
> limited to just the drivers that aren't probed and ready to handle their
> clks. If sync_state could be called whenever a clk consumer consumes a
> clk it may work? Technically we already have that by the clk_hw_provider
> function but there isn't enough information being passed there, like the
> getting device.
> 

The current solution already operates on all clocks of all drivers, that
happens to be probed at late_initcall(). This patch removes the
subordinate clause from this, allowing clock drivers and their clients
to be built as modules.

So while it still operates on all clocks of a driver, it moves that
point to a later stage, where that is more reasonable to do.



It would probably (haven't considered all aspects) if sync_state could
prune the tree gradually, disabling the branches that are fully probed.

But it wouldn't affect Matthias problem; e.g. if you forget to build the
venus driver, sync_state won't happen for that branch of the tree.
(Something that is arguably better than leaving all the clocks for that
driver enabled)

Regards,
Bjorn

> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 842e72a5348f..cf1adfeaf257 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> >                 void __iomem *reg, u8 shift, u8 width,
> >                 u8 clk_divider_flags, const struct clk_div_table *table,
> >                 spinlock_t *lock);
> > +void clk_sync_state_disable_unused(struct device *dev);
> 
> This is a weird place to put this. Why not in the helper functions
> section?
> 
> >  /**
> >   * clk_register_divider - register a divider clock with the clock framework
> >   * @dev: device registering this clock

Abel Vesa Feb. 20, 2023, 4:27 p.m. UTC | #7

On 23-02-20 17:46:36, Abel Vesa wrote:
> On 23-02-17 21:38:22, Stephen Boyd wrote:
> > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > and most likely could be disabled later on sync_state. So provide a generic
> > > sync_state callback for the clock providers that register such clocks.
> > > Then, use the same mechanism as clk_disable_unused from that generic
> > > callback, but pass the device to make sure only the clocks belonging to
> > > the current clock provider get disabled, if unused. Also, during the
> > > default clk_disable_unused, if the driver that registered the clock has
> > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > skip disabling its clocks.
> > 
> > How does that avoid disabling clks randomly in the clk tree? I'm
> > concerned about disabling an unused clk in the middle of the tree
> > because it doesn't have a driver using sync state, while the clk is the
> > parent of an unused clk that is backed by sync state.
> > 
> >    clk A -->  clk B 
> > 
> > clk A: No sync state
> > clk B: sync state
> > 
> > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > from late init. Imagine clk A is the root of the tree.
> > 
> > 	clk_disable_unused_subtree(clk_core A)
> > 	  clk_disable_unused_subtree(clk_core B)
> > 	    if (from_sync_state && core->dev != dev)
> > 	      return;
> > 	  ...
> > 	  clk core A->ops->disable()
> > 
> > clk core B is off now?
> 
> Yes, that is correct. But the same thing is happening currently if the
> clk_ignore_unused in not specified. At least with this new approach, we
> get to leave unused clocks enabled either until sync_state is called or forever.
> All the provider has to do is to implement a sync_state callback (or use
> the generic one provided). So the provider of clk A would obviously need
> a sync state callback registered.
> 
> > 
> > Also sync_state seems broken right now. I saw mka mentioned that if you
> > have a device node enabled in your DT but never enable a driver for it
> > in the kernel we'll never get sync_state called. This is another
> > problem, but it concerns me that sync_state would make the unused clk
> > disabling happen at some random time or not at all.
> 
> Well, the fact that the sync state not being called because a driver for
> a consumer device doesn't probe does not really mean it is broken. Just
> because the consumer driver hasn't probed yet, doesn't mean it will
> not probe later on.
> 

CC'ed Saravana

> That aside, rather than going with clk_ignore_unused all the time on
> qcom platforms, at least in a perfect scenario (where sync state is
> reached for all providers) the clocks get disabled.
> 
> > 
> > Can the problem be approached more directly? If this is about fixing
> > continuous splash screen, then I wonder why we can't list out the clks
> > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > 
> > 	clock-controller {
> > 		#clock-cells = <1>;
> > 		boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> > 	};
> > 
> > Then mark those as "critical/don't turn off" all the way up the clk tree
> > when the clk driver probes by essentially incrementing the
> > prepare/enable count but not actually touching the hardware, and when
> > the clks are acquired by clk_get() for that device that's using them
> > from boot we make the first clk_prepare_enable() do nothing and not
> > increment the count at all. We can probably stick some flag into the
> > 'struct clk' for this when we create the handle in clk_get() so that the
> > prepare and enable functions can special case and skip over.
> 
> Well, that means we need to play whack-a-mole by alsways adding such clocks to
> devicetree.
> 
> > 
> > The sync_state hook operates on a driver level, which is too large when
> > you consider that a single clk driver may register hundreds of clks that
> > are not related. We want to target a solution at the clk level so that
> > any damage from keeping on all the clks provided by the controller is
> > limited to just the drivers that aren't probed and ready to handle their
> > clks. If sync_state could be called whenever a clk consumer consumes a
> > clk it may work? Technically we already have that by the clk_hw_provider
> > function but there isn't enough information being passed there, like the
> > getting device.
> 
> Actually, from the multitude of clocks registered by one provider, the
> ones already explicitely enabled (and obvisously their parents) by thier
> consumer are safe. The only ones we need to worry about are the ones that
> might be enabled by bootloader and need to remain on. With the sync state
> approach, the latter mentioned clocks will either remain on indefinitely
> or will be disabled on sync state. The provider driver is the only level
> that has a registered sync state callback.
> 
> > 
> > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > index 842e72a5348f..cf1adfeaf257 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> > >                 void __iomem *reg, u8 shift, u8 width,
> > >                 u8 clk_divider_flags, const struct clk_div_table *table,
> > >                 spinlock_t *lock);
> > > +void clk_sync_state_disable_unused(struct device *dev);
> > 
> > This is a weird place to put this. Why not in the helper functions
> > section?
> 
> Sure this can be moved.
> 
> > 
> > >  /**
> > >   * clk_register_divider - register a divider clock with the clock framework
> > >   * @dev: device registering this clock

Bjorn Andersson Feb. 20, 2023, 4:33 p.m. UTC | #8

On Mon, Feb 20, 2023 at 05:46:36PM +0200, Abel Vesa wrote:
> On 23-02-17 21:38:22, Stephen Boyd wrote:
> > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > and most likely could be disabled later on sync_state. So provide a generic
> > > sync_state callback for the clock providers that register such clocks.
> > > Then, use the same mechanism as clk_disable_unused from that generic
> > > callback, but pass the device to make sure only the clocks belonging to
> > > the current clock provider get disabled, if unused. Also, during the
> > > default clk_disable_unused, if the driver that registered the clock has
> > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > skip disabling its clocks.
> > 
> > How does that avoid disabling clks randomly in the clk tree? I'm
> > concerned about disabling an unused clk in the middle of the tree
> > because it doesn't have a driver using sync state, while the clk is the
> > parent of an unused clk that is backed by sync state.
> > 
> >    clk A -->  clk B 
> > 
> > clk A: No sync state
> > clk B: sync state
> > 
> > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > from late init. Imagine clk A is the root of the tree.
> > 
> > 	clk_disable_unused_subtree(clk_core A)
> > 	  clk_disable_unused_subtree(clk_core B)
> > 	    if (from_sync_state && core->dev != dev)
> > 	      return;
> > 	  ...
> > 	  clk core A->ops->disable()
> > 
> > clk core B is off now?
> 
> Yes, that is correct. But the same thing is happening currently if the
> clk_ignore_unused in not specified. At least with this new approach, we
> get to leave unused clocks enabled either until sync_state is called or forever.
> All the provider has to do is to implement a sync_state callback (or use
> the generic one provided). So the provider of clk A would obviously need
> a sync state callback registered.
> 
> > 
> > Also sync_state seems broken right now. I saw mka mentioned that if you
> > have a device node enabled in your DT but never enable a driver for it
> > in the kernel we'll never get sync_state called. This is another
> > problem, but it concerns me that sync_state would make the unused clk
> > disabling happen at some random time or not at all.
> 
> Well, the fact that the sync state not being called because a driver for
> a consumer device doesn't probe does not really mean it is broken. Just
> because the consumer driver hasn't probed yet, doesn't mean it will
> not probe later on.
> 
> That aside, rather than going with clk_ignore_unused all the time on
> qcom platforms, at least in a perfect scenario (where sync state is
> reached for all providers) the clocks get disabled.
> 

Furthermore, the sync_state approach will cause clk_disable_unused() to
be invoked for clock drivers that probe after late_initcall() as well.

So not only can we boot without clk_ignore_unused, we will actually turn
off unused display clocks (perhaps all of them, for a headless device).

Regards,
Bjorn

Saravana Kannan Feb. 20, 2023, 5:51 p.m. UTC | #9

On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
>
> On 23-02-20 17:46:36, Abel Vesa wrote:
> > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > sync_state callback for the clock providers that register such clocks.
> > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > callback, but pass the device to make sure only the clocks belonging to
> > > > the current clock provider get disabled, if unused. Also, during the
> > > > default clk_disable_unused, if the driver that registered the clock has
> > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > skip disabling its clocks.

Hi Abel,

We have the day off today, so I'll respond more later. Also, please cc
me on all sync_state() related patches in the future.

I haven't taken a close look at your series yet, but at a glance it
seems incomplete.

Any reason you didn't just try to revive my series[1] or nudge me?
[1]- https://lore.kernel.org/lkml/[e-mail]/

At the least, I know [1] works on all Android devices (including
Qualcomm SoCs) released in the past 2-3 years or more. If [1] works
for you, I'd rather land that after addressing Stephen's comments
there (I remember them being fairly easy to address comments) instead
of whipping up a new series that's not as well used. I just got busy
with other things and addressing more fundamental fw_devlink TODOs
before getting back to this.

Hi Bjorn,

I see in another reply you've said:

Applied, thanks!

[1/2] clk: Add generic sync_state callback for disabling unused clocks
      commit: 26b36df7516692292312063ca6fd19e73c06d4e7
[2/2] clk: qcom: sdm845: Use generic clk_sync_state_disable_unused callback
      commit: 99c0f7d35c4b204dd95ba50e155f32c99695b445

Where exactly have you applied them? I hope you haven't applied the
clk.c changes to some tree that goes into 6.3.

-Saravana

> > >
> > > How does that avoid disabling clks randomly in the clk tree? I'm
> > > concerned about disabling an unused clk in the middle of the tree
> > > because it doesn't have a driver using sync state, while the clk is the
> > > parent of an unused clk that is backed by sync state.
> > >
> > >    clk A -->  clk B
> > >
> > > clk A: No sync state
> > > clk B: sync state
> > >
> > > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > > from late init. Imagine clk A is the root of the tree.
> > >
> > >     clk_disable_unused_subtree(clk_core A)
> > >       clk_disable_unused_subtree(clk_core B)
> > >         if (from_sync_state && core->dev != dev)
> > >           return;
> > >       ...
> > >       clk core A->ops->disable()
> > >
> > > clk core B is off now?
> >
> > Yes, that is correct. But the same thing is happening currently if the
> > clk_ignore_unused in not specified. At least with this new approach, we
> > get to leave unused clocks enabled either until sync_state is called or forever.
> > All the provider has to do is to implement a sync_state callback (or use
> > the generic one provided). So the provider of clk A would obviously need
> > a sync state callback registered.
> >
> > >
> > > Also sync_state seems broken right now. I saw mka mentioned that if you
> > > have a device node enabled in your DT but never enable a driver for it
> > > in the kernel we'll never get sync_state called. This is another
> > > problem, but it concerns me that sync_state would make the unused clk
> > > disabling happen at some random time or not at all.
> >
> > Well, the fact that the sync state not being called because a driver for
> > a consumer device doesn't probe does not really mean it is broken. Just
> > because the consumer driver hasn't probed yet, doesn't mean it will
> > not probe later on.
> >
>
> CC'ed Saravana
>
> > That aside, rather than going with clk_ignore_unused all the time on
> > qcom platforms, at least in a perfect scenario (where sync state is
> > reached for all providers) the clocks get disabled.
> >
> > >
> > > Can the problem be approached more directly? If this is about fixing
> > > continuous splash screen, then I wonder why we can't list out the clks
> > > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > >
> > >     clock-controller {
> > >             #clock-cells = <1>;
> > >             boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> > >     };
> > >
> > > Then mark those as "critical/don't turn off" all the way up the clk tree
> > > when the clk driver probes by essentially incrementing the
> > > prepare/enable count but not actually touching the hardware, and when
> > > the clks are acquired by clk_get() for that device that's using them
> > > from boot we make the first clk_prepare_enable() do nothing and not
> > > increment the count at all. We can probably stick some flag into the
> > > 'struct clk' for this when we create the handle in clk_get() so that the
> > > prepare and enable functions can special case and skip over.
> >
> > Well, that means we need to play whack-a-mole by alsways adding such clocks to
> > devicetree.
> >
> > >
> > > The sync_state hook operates on a driver level, which is too large when
> > > you consider that a single clk driver may register hundreds of clks that
> > > are not related. We want to target a solution at the clk level so that
> > > any damage from keeping on all the clks provided by the controller is
> > > limited to just the drivers that aren't probed and ready to handle their
> > > clks. If sync_state could be called whenever a clk consumer consumes a
> > > clk it may work? Technically we already have that by the clk_hw_provider
> > > function but there isn't enough information being passed there, like the
> > > getting device.
> >
> > Actually, from the multitude of clocks registered by one provider, the
> > ones already explicitely enabled (and obvisously their parents) by thier
> > consumer are safe. The only ones we need to worry about are the ones that
> > might be enabled by bootloader and need to remain on. With the sync state
> > approach, the latter mentioned clocks will either remain on indefinitely
> > or will be disabled on sync state. The provider driver is the only level
> > that has a registered sync state callback.
> >
> > >
> > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > > index 842e72a5348f..cf1adfeaf257 100644
> > > > --- a/include/linux/clk-provider.h
> > > > +++ b/include/linux/clk-provider.h
> > > > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> > > >                 void __iomem *reg, u8 shift, u8 width,
> > > >                 u8 clk_divider_flags, const struct clk_div_table *table,
> > > >                 spinlock_t *lock);
> > > > +void clk_sync_state_disable_unused(struct device *dev);
> > >
> > > This is a weird place to put this. Why not in the helper functions
> > > section?
> >
> > Sure this can be moved.
> >
> > >
> > > >  /**
> > > >   * clk_register_divider - register a divider clock with the clock framework
> > > >   * @dev: device registering this clock

Abel Vesa Feb. 20, 2023, 6:47 p.m. UTC | #10

On 23-02-20 09:51:55, Saravana Kannan wrote:
> On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
> >
> > On 23-02-20 17:46:36, Abel Vesa wrote:
> > > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > > sync_state callback for the clock providers that register such clocks.
> > > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > > callback, but pass the device to make sure only the clocks belonging to
> > > > > the current clock provider get disabled, if unused. Also, during the
> > > > > default clk_disable_unused, if the driver that registered the clock has
> > > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > > skip disabling its clocks.
> 
> Hi Abel,
> 
> We have the day off today, so I'll respond more later. Also, please cc
> me on all sync_state() related patches in the future.
> 

Sure thing.

> I haven't taken a close look at your series yet, but at a glance it
> seems incomplete.
> 
> Any reason you didn't just try to revive my series[1] or nudge me?
> [1]- https://lore.kernel.org/lkml/[e-mail]/

This patchset is heavily reworked and much more simpler as it relies
strictly on the sync_state being registered by the clock provider.

I saw your patchset a few months ago but then forgot about its
existence. That's also why I forgot to nudge you. Sorry about that.

> 
> At the least, I know [1] works on all Android devices (including
> Qualcomm SoCs) released in the past 2-3 years or more. If [1] works
> for you, I'd rather land that after addressing Stephen's comments
> there (I remember them being fairly easy to address comments) instead
> of whipping up a new series that's not as well used. I just got busy
> with other things and addressing more fundamental fw_devlink TODOs
> before getting back to this.
> 
> Hi Bjorn,
> 
> I see in another reply you've said:
> 
> Applied, thanks!
> 
> [1/2] clk: Add generic sync_state callback for disabling unused clocks
>       commit: 26b36df7516692292312063ca6fd19e73c06d4e7
> [2/2] clk: qcom: sdm845: Use generic clk_sync_state_disable_unused callback
>       commit: 99c0f7d35c4b204dd95ba50e155f32c99695b445
> 
> Where exactly have you applied them? I hope you haven't applied the
> clk.c changes to some tree that goes into 6.3.

I think it is already part of Bjorn's Qualcomm clocks pull request.

> 
> -Saravana
> 
> > > >
> > > > How does that avoid disabling clks randomly in the clk tree? I'm
> > > > concerned about disabling an unused clk in the middle of the tree
> > > > because it doesn't have a driver using sync state, while the clk is the
> > > > parent of an unused clk that is backed by sync state.
> > > >
> > > >    clk A -->  clk B
> > > >
> > > > clk A: No sync state
> > > > clk B: sync state
> > > >
> > > > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > > > from late init. Imagine clk A is the root of the tree.
> > > >
> > > >     clk_disable_unused_subtree(clk_core A)
> > > >       clk_disable_unused_subtree(clk_core B)
> > > >         if (from_sync_state && core->dev != dev)
> > > >           return;
> > > >       ...
> > > >       clk core A->ops->disable()
> > > >
> > > > clk core B is off now?
> > >
> > > Yes, that is correct. But the same thing is happening currently if the
> > > clk_ignore_unused in not specified. At least with this new approach, we
> > > get to leave unused clocks enabled either until sync_state is called or forever.
> > > All the provider has to do is to implement a sync_state callback (or use
> > > the generic one provided). So the provider of clk A would obviously need
> > > a sync state callback registered.
> > >
> > > >
> > > > Also sync_state seems broken right now. I saw mka mentioned that if you
> > > > have a device node enabled in your DT but never enable a driver for it
> > > > in the kernel we'll never get sync_state called. This is another
> > > > problem, but it concerns me that sync_state would make the unused clk
> > > > disabling happen at some random time or not at all.
> > >
> > > Well, the fact that the sync state not being called because a driver for
> > > a consumer device doesn't probe does not really mean it is broken. Just
> > > because the consumer driver hasn't probed yet, doesn't mean it will
> > > not probe later on.
> > >
> >
> > CC'ed Saravana
> >
> > > That aside, rather than going with clk_ignore_unused all the time on
> > > qcom platforms, at least in a perfect scenario (where sync state is
> > > reached for all providers) the clocks get disabled.
> > >
> > > >
> > > > Can the problem be approached more directly? If this is about fixing
> > > > continuous splash screen, then I wonder why we can't list out the clks
> > > > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > > >
> > > >     clock-controller {
> > > >             #clock-cells = <1>;
> > > >             boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> > > >     };
> > > >
> > > > Then mark those as "critical/don't turn off" all the way up the clk tree
> > > > when the clk driver probes by essentially incrementing the
> > > > prepare/enable count but not actually touching the hardware, and when
> > > > the clks are acquired by clk_get() for that device that's using them
> > > > from boot we make the first clk_prepare_enable() do nothing and not
> > > > increment the count at all. We can probably stick some flag into the
> > > > 'struct clk' for this when we create the handle in clk_get() so that the
> > > > prepare and enable functions can special case and skip over.
> > >
> > > Well, that means we need to play whack-a-mole by alsways adding such clocks to
> > > devicetree.
> > >
> > > >
> > > > The sync_state hook operates on a driver level, which is too large when
> > > > you consider that a single clk driver may register hundreds of clks that
> > > > are not related. We want to target a solution at the clk level so that
> > > > any damage from keeping on all the clks provided by the controller is
> > > > limited to just the drivers that aren't probed and ready to handle their
> > > > clks. If sync_state could be called whenever a clk consumer consumes a
> > > > clk it may work? Technically we already have that by the clk_hw_provider
> > > > function but there isn't enough information being passed there, like the
> > > > getting device.
> > >
> > > Actually, from the multitude of clocks registered by one provider, the
> > > ones already explicitely enabled (and obvisously their parents) by thier
> > > consumer are safe. The only ones we need to worry about are the ones that
> > > might be enabled by bootloader and need to remain on. With the sync state
> > > approach, the latter mentioned clocks will either remain on indefinitely
> > > or will be disabled on sync state. The provider driver is the only level
> > > that has a registered sync state callback.
> > >
> > > >
> > > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > > > index 842e72a5348f..cf1adfeaf257 100644
> > > > > --- a/include/linux/clk-provider.h
> > > > > +++ b/include/linux/clk-provider.h
> > > > > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> > > > >                 void __iomem *reg, u8 shift, u8 width,
> > > > >                 u8 clk_divider_flags, const struct clk_div_table *table,
> > > > >                 spinlock_t *lock);
> > > > > +void clk_sync_state_disable_unused(struct device *dev);
> > > >
> > > > This is a weird place to put this. Why not in the helper functions
> > > > section?
> > >
> > > Sure this can be moved.
> > >
> > > >
> > > > >  /**
> > > > >   * clk_register_divider - register a divider clock with the clock framework
> > > > >   * @dev: device registering this clock

Stephen Boyd Feb. 21, 2023, 6:44 p.m. UTC | #11

Quoting Abel Vesa (2023-02-20 07:46:36)
> On 23-02-17 21:38:22, Stephen Boyd wrote:
> > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > and most likely could be disabled later on sync_state. So provide a generic
> > > sync_state callback for the clock providers that register such clocks.
> > > Then, use the same mechanism as clk_disable_unused from that generic
> > > callback, but pass the device to make sure only the clocks belonging to
> > > the current clock provider get disabled, if unused. Also, during the
> > > default clk_disable_unused, if the driver that registered the clock has
> > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > skip disabling its clocks.
> > 
> > How does that avoid disabling clks randomly in the clk tree? I'm
> > concerned about disabling an unused clk in the middle of the tree
> > because it doesn't have a driver using sync state, while the clk is the
> > parent of an unused clk that is backed by sync state.
> > 
> >    clk A -->  clk B 
> > 
> > clk A: No sync state
> > clk B: sync state
> > 
> > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > from late init. Imagine clk A is the root of the tree.
> > 
> >       clk_disable_unused_subtree(clk_core A)
> >         clk_disable_unused_subtree(clk_core B)
> >           if (from_sync_state && core->dev != dev)
> >             return;
> >         ...
> >         clk core A->ops->disable()
> > 
> > clk core B is off now?
> 
> Yes, that is correct. But the same thing is happening currently if the
> clk_ignore_unused in not specified.

The existing code traverses the clk tree in depth-first order, disabling
clks from the leaves up to the root. This breaks that tree walk. It is
not the same thing.

> At least with this new approach, we
> get to leave unused clocks enabled either until sync_state is called or forever.
> All the provider has to do is to implement a sync_state callback (or use
> the generic one provided). So the provider of clk A would obviously need
> a sync state callback registered.

Sure.

> 
> > 
> > Also sync_state seems broken right now. I saw mka mentioned that if you
> > have a device node enabled in your DT but never enable a driver for it
> > in the kernel we'll never get sync_state called. This is another
> > problem, but it concerns me that sync_state would make the unused clk
> > disabling happen at some random time or not at all.
> 
> Well, the fact that the sync state not being called because a driver for
> a consumer device doesn't probe does not really mean it is broken. Just
> because the consumer driver hasn't probed yet, doesn't mean it will
> not probe later on.
> 
> That aside, rather than going with clk_ignore_unused all the time on
> qcom platforms, at least in a perfect scenario (where sync state is
> reached for all providers) the clocks get disabled.

The clks will get disabled in some random order though even if every clk
provider has sync_state.

> 
> > 
> > Can the problem be approached more directly? If this is about fixing
> > continuous splash screen, then I wonder why we can't list out the clks
> > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > 
> >       clock-controller {
> >               #clock-cells = <1>;
> >               boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> >       };
> > 
> > Then mark those as "critical/don't turn off" all the way up the clk tree
> > when the clk driver probes by essentially incrementing the
> > prepare/enable count but not actually touching the hardware, and when
> > the clks are acquired by clk_get() for that device that's using them
> > from boot we make the first clk_prepare_enable() do nothing and not
> > increment the count at all. We can probably stick some flag into the
> > 'struct clk' for this when we create the handle in clk_get() so that the
> > prepare and enable functions can special case and skip over.
> 
> Well, that means we need to play whack-a-mole by alsways adding such clocks to
> devicetree.

I don't think the bootloader is constantly changing. Either we want to
hand off the enable state to devices that are using them from boot, or
we don't. I doubt that is changing outside of bootloader development
time.

> 
> > 
> > The sync_state hook operates on a driver level, which is too large when
> > you consider that a single clk driver may register hundreds of clks that
> > are not related. We want to target a solution at the clk level so that
> > any damage from keeping on all the clks provided by the controller is
> > limited to just the drivers that aren't probed and ready to handle their
> > clks. If sync_state could be called whenever a clk consumer consumes a
> > clk it may work? Technically we already have that by the clk_hw_provider
> > function but there isn't enough information being passed there, like the
> > getting device.
> 
> Actually, from the multitude of clocks registered by one provider, the
> ones already explicitely enabled (and obvisously their parents) by thier
> consumer are safe. The only ones we need to worry about are the ones that
> might be enabled by bootloader and need to remain on. With the sync state
> approach, the latter mentioned clocks will either remain on indefinitely
> or will be disabled on sync state. The provider driver is the only level
> that has a registered sync state callback.
> 

The driver has sync_state callback, yes. I'm saying that it is too wide
of a scope to implement disabling unused clks via the sync_state
callback.

Stephen Boyd Feb. 21, 2023, 7:24 p.m. UTC | #12

Quoting Bjorn Andersson (2023-02-20 08:21:37)
> On Fri, Feb 17, 2023 at 09:38:22PM -0800, Stephen Boyd wrote:
> > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > and most likely could be disabled later on sync_state. So provide a generic
> > > sync_state callback for the clock providers that register such clocks.
> > > Then, use the same mechanism as clk_disable_unused from that generic
> > > callback, but pass the device to make sure only the clocks belonging to
> > > the current clock provider get disabled, if unused. Also, during the
> > > default clk_disable_unused, if the driver that registered the clock has
> > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > skip disabling its clocks.
> > 
> > How does that avoid disabling clks randomly in the clk tree? I'm
> > concerned about disabling an unused clk in the middle of the tree
> > because it doesn't have a driver using sync state, while the clk is the
> > parent of an unused clk that is backed by sync state.
> > 
> >    clk A -->  clk B 
> > 
> > clk A: No sync state
> > clk B: sync state
> > 
> > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > from late init. Imagine clk A is the root of the tree.
> > 
> >       clk_disable_unused_subtree(clk_core A)
> >         clk_disable_unused_subtree(clk_core B)
> >           if (from_sync_state && core->dev != dev)
> >             return;
> >         ...
> >         clk core A->ops->disable()
> > 
> > clk core B is off now?
> > 
> 
> I will have to give this some more thought. But this is exactly what we
> have today; consider A being any builtin clock driver and B being any
> clock driver built as modules, with relationship to A.
> 
> clk_disable_unused() will take down A without waiting for B, possibly
> locking up parts of the clock hardware of B; or turning off the clocks
> to IP blocks that rely on those clocks (e.g. display).

Oh, thanks for clarifying! Yes, the disabling of unused clks with
respect to modules is broken in the same way. This makes that brokenness
equally apply to builtin drivers, making the situation worse, not better.

> 
> So my thought on this is that I don't think this patch negatively alter
> the situation. But as it isn't recursive, this remains a problem that
> needs to be fixed.
> 
> > Also sync_state seems broken right now. I saw mka mentioned that if you
> > have a device node enabled in your DT but never enable a driver for it
> > in the kernel we'll never get sync_state called. This is another
> > problem, but it concerns me that sync_state would make the unused clk
> > disabling happen at some random time or not at all.
> > 
> 
> I don't think that sync_state is "broken".
> 
> There is no way to distinguish a driver not being built in, or a driver
> being built as module but not yet loaded. The approach taken by
> sync_state currently is optimistically speculative.
> 
> One alternative to this is found in the regulator framework, where we
> have a 30 second timer triggering the late disable. The result of this
> is that every time I end up in the ramdisk console because "root file
> system can't be mounted", I have 25 second to figure out what the
> problem is before the backlight goes out...
> 
> As such I do prefer the optimistic approach...
> 
> > Can the problem be approached more directly? If this is about fixing
> > continuous splash screen, then I wonder why we can't list out the clks
> > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > 
> >       clock-controller {
> >               #clock-cells = <1>;
> >               boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> >       };
> > 
> 
> I was under the impression that we have ruled out this approach.

Where did we rule it out? I suppose we already have this information if
we search the DT for 'clocks' properties and read the clk hardware at
boot to see if it is enabled. Putting these two things together gets us
all the enabled clks at boot that are consumed by device nodes. It
doesn't tell use about anything not consumed in DT, or handle overlays,
but if we have that problem we can probably figure something out later.

> 
> Presumably this list should not be a manually maintained list of display
> clocks, and that means the bootloader would need to go in and  build
> this list of all enabled clocks. I don't think this is practical.

Why does the bootloader need to do that? The devicetree author can list
out the clks that they want to keep on for the display driver until the
display driver can acquire them.

> 
> > Then mark those as "critical/don't turn off" all the way up the clk tree
> > when the clk driver probes by essentially incrementing the
> > prepare/enable count but not actually touching the hardware, and when
> > the clks are acquired by clk_get() for that device that's using them
> > from boot we make the first clk_prepare_enable() do nothing and not
> > increment the count at all. We can probably stick some flag into the
> > 'struct clk' for this when we create the handle in clk_get() so that the
> > prepare and enable functions can special case and skip over.
> > 
> 
> The benefit of sync_state is that it kicks when the client drivers has
> probed. As such, you can have e.g. the display driver clk_get(), then
> probe defer on some other resource, and the clock state can remain
> untouched.

Ok. I think this spitball design would do that still. It's not like we
would go and disable the clks that are handed to the display driver even
if it probe defers. The clk would be marked as enabled until the display
driver enables the clk, and then it wouldn't be disabled during late
init (or later) because the clk would be enabled either by the core or
by the display driver. The point where we transfer ownership of the
enable state is when the consumer calls clk_enable().

> 
> > The sync_state hook operates on a driver level, which is too large when
> > you consider that a single clk driver may register hundreds of clks that
> > are not related. We want to target a solution at the clk level so that
> > any damage from keeping on all the clks provided by the controller is
> > limited to just the drivers that aren't probed and ready to handle their
> > clks. If sync_state could be called whenever a clk consumer consumes a
> > clk it may work? Technically we already have that by the clk_hw_provider
> > function but there isn't enough information being passed there, like the
> > getting device.
> > 
> 
> The current solution already operates on all clocks of all drivers, that
> happens to be probed at late_initcall(). This patch removes the
> subordinate clause from this, allowing clock drivers and their clients
> to be built as modules.
> 
> So while it still operates on all clocks of a driver, it moves that
> point to a later stage, where that is more reasonable to do.
> 

When we have clk drivers that provide clks to many different device
drivers, they all have to probe for any unused clks to be disabled.

> 
> 
> It would probably (haven't considered all aspects) if sync_state could
> prune the tree gradually, disabling the branches that are fully probed.
> 
> But it wouldn't affect Matthias problem; e.g. if you forget to build the
> venus driver, sync_state won't happen for that branch of the tree.
> (Something that is arguably better than leaving all the clocks for that
> driver enabled)
> 

I didn't follow this part.

Saravana Kannan Feb. 21, 2023, 7:58 p.m. UTC | #13

On Mon, Feb 20, 2023 at 10:47 AM Abel Vesa <[e-mail]> wrote:
>
> On 23-02-20 09:51:55, Saravana Kannan wrote:
> > On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
> > >
> > > On 23-02-20 17:46:36, Abel Vesa wrote:
> > > > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > > > sync_state callback for the clock providers that register such clocks.
> > > > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > > > callback, but pass the device to make sure only the clocks belonging to
> > > > > > the current clock provider get disabled, if unused. Also, during the
> > > > > > default clk_disable_unused, if the driver that registered the clock has
> > > > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > > > skip disabling its clocks.
> >
> > Hi Abel,
> >
> > We have the day off today, so I'll respond more later. Also, please cc
> > me on all sync_state() related patches in the future.
> >
>
> Sure thing.
>
> > I haven't taken a close look at your series yet, but at a glance it
> > seems incomplete.
> >
> > Any reason you didn't just try to revive my series[1] or nudge me?
> > [1]- https://lore.kernel.org/lkml/[e-mail]/
>
> This patchset is heavily reworked and much more simpler as it relies
> strictly on the sync_state being registered by the clock provider.

It's simpler because it's not complete. It for sure doesn't handle
orphan-reparenting. It also doesn't make a lot of sense for only some
clock providers registering for sync_state(). If CC-A is feeding a
clock signal that's used as a root for clocks in CC-B, then what
happens if only CC-B implements sync_state() but CC-A doesn't. The
clocks from CC-B are still going to turn off when CC-A turns off its
PLL before CC-B registers.

Nack for this patch.

Also, unless there's a strong objection, let's go back to my patch
please. It's way more well tested and used across different SoCs than
this patch. Also, I'm pretty sure the orphan handling is needed for
qcom SoC's too.

-Saravana

>
> I saw your patchset a few months ago but then forgot about its
> existence. That's also why I forgot to nudge you. Sorry about that.
>
> >
> > At the least, I know [1] works on all Android devices (including
> > Qualcomm SoCs) released in the past 2-3 years or more. If [1] works
> > for you, I'd rather land that after addressing Stephen's comments
> > there (I remember them being fairly easy to address comments) instead
> > of whipping up a new series that's not as well used. I just got busy
> > with other things and addressing more fundamental fw_devlink TODOs
> > before getting back to this.
> >
> > Hi Bjorn,
> >
> > I see in another reply you've said:
> >
> > Applied, thanks!
> >
> > [1/2] clk: Add generic sync_state callback for disabling unused clocks
> >       commit: 26b36df7516692292312063ca6fd19e73c06d4e7
> > [2/2] clk: qcom: sdm845: Use generic clk_sync_state_disable_unused callback
> >       commit: 99c0f7d35c4b204dd95ba50e155f32c99695b445
> >
> > Where exactly have you applied them? I hope you haven't applied the
> > clk.c changes to some tree that goes into 6.3.
>
> I think it is already part of Bjorn's Qualcomm clocks pull request.
>
> >
> > -Saravana
> >
> > > > >
> > > > > How does that avoid disabling clks randomly in the clk tree? I'm
> > > > > concerned about disabling an unused clk in the middle of the tree
> > > > > because it doesn't have a driver using sync state, while the clk is the
> > > > > parent of an unused clk that is backed by sync state.
> > > > >
> > > > >    clk A -->  clk B
> > > > >
> > > > > clk A: No sync state
> > > > > clk B: sync state
> > > > >
> > > > > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > > > > from late init. Imagine clk A is the root of the tree.
> > > > >
> > > > >     clk_disable_unused_subtree(clk_core A)
> > > > >       clk_disable_unused_subtree(clk_core B)
> > > > >         if (from_sync_state && core->dev != dev)
> > > > >           return;
> > > > >       ...
> > > > >       clk core A->ops->disable()
> > > > >
> > > > > clk core B is off now?
> > > >
> > > > Yes, that is correct. But the same thing is happening currently if the
> > > > clk_ignore_unused in not specified. At least with this new approach, we
> > > > get to leave unused clocks enabled either until sync_state is called or forever.
> > > > All the provider has to do is to implement a sync_state callback (or use
> > > > the generic one provided). So the provider of clk A would obviously need
> > > > a sync state callback registered.
> > > >
> > > > >
> > > > > Also sync_state seems broken right now. I saw mka mentioned that if you
> > > > > have a device node enabled in your DT but never enable a driver for it
> > > > > in the kernel we'll never get sync_state called. This is another
> > > > > problem, but it concerns me that sync_state would make the unused clk
> > > > > disabling happen at some random time or not at all.
> > > >
> > > > Well, the fact that the sync state not being called because a driver for
> > > > a consumer device doesn't probe does not really mean it is broken. Just
> > > > because the consumer driver hasn't probed yet, doesn't mean it will
> > > > not probe later on.
> > > >
> > >
> > > CC'ed Saravana
> > >
> > > > That aside, rather than going with clk_ignore_unused all the time on
> > > > qcom platforms, at least in a perfect scenario (where sync state is
> > > > reached for all providers) the clocks get disabled.
> > > >
> > > > >
> > > > > Can the problem be approached more directly? If this is about fixing
> > > > > continuous splash screen, then I wonder why we can't list out the clks
> > > > > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > > > >
> > > > >     clock-controller {
> > > > >             #clock-cells = <1>;
> > > > >             boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> > > > >     };
> > > > >
> > > > > Then mark those as "critical/don't turn off" all the way up the clk tree
> > > > > when the clk driver probes by essentially incrementing the
> > > > > prepare/enable count but not actually touching the hardware, and when
> > > > > the clks are acquired by clk_get() for that device that's using them
> > > > > from boot we make the first clk_prepare_enable() do nothing and not
> > > > > increment the count at all. We can probably stick some flag into the
> > > > > 'struct clk' for this when we create the handle in clk_get() so that the
> > > > > prepare and enable functions can special case and skip over.
> > > >
> > > > Well, that means we need to play whack-a-mole by alsways adding such clocks to
> > > > devicetree.
> > > >
> > > > >
> > > > > The sync_state hook operates on a driver level, which is too large when
> > > > > you consider that a single clk driver may register hundreds of clks that
> > > > > are not related. We want to target a solution at the clk level so that
> > > > > any damage from keeping on all the clks provided by the controller is
> > > > > limited to just the drivers that aren't probed and ready to handle their
> > > > > clks. If sync_state could be called whenever a clk consumer consumes a
> > > > > clk it may work? Technically we already have that by the clk_hw_provider
> > > > > function but there isn't enough information being passed there, like the
> > > > > getting device.
> > > >
> > > > Actually, from the multitude of clocks registered by one provider, the
> > > > ones already explicitely enabled (and obvisously their parents) by thier
> > > > consumer are safe. The only ones we need to worry about are the ones that
> > > > might be enabled by bootloader and need to remain on. With the sync state
> > > > approach, the latter mentioned clocks will either remain on indefinitely
> > > > or will be disabled on sync state. The provider driver is the only level
> > > > that has a registered sync state callback.
> > > >
> > > > >
> > > > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > > > > index 842e72a5348f..cf1adfeaf257 100644
> > > > > > --- a/include/linux/clk-provider.h
> > > > > > +++ b/include/linux/clk-provider.h
> > > > > > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> > > > > >                 void __iomem *reg, u8 shift, u8 width,
> > > > > >                 u8 clk_divider_flags, const struct clk_div_table *table,
> > > > > >                 spinlock_t *lock);
> > > > > > +void clk_sync_state_disable_unused(struct device *dev);
> > > > >
> > > > > This is a weird place to put this. Why not in the helper functions
> > > > > section?
> > > >
> > > > Sure this can be moved.
> > > >
> > > > >
> > > > > >  /**
> > > > > >   * clk_register_divider - register a divider clock with the clock framework
> > > > > >   * @dev: device registering this clock

Abel Vesa Feb. 22, 2023, 7:57 a.m. UTC | #14

On 23-02-21 11:58:24, Saravana Kannan wrote:
> On Mon, Feb 20, 2023 at 10:47 AM Abel Vesa <[e-mail]> wrote:
> >
> > On 23-02-20 09:51:55, Saravana Kannan wrote:
> > > On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
> > > >
> > > > On 23-02-20 17:46:36, Abel Vesa wrote:
> > > > > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > > > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > > > > sync_state callback for the clock providers that register such clocks.
> > > > > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > > > > callback, but pass the device to make sure only the clocks belonging to
> > > > > > > the current clock provider get disabled, if unused. Also, during the
> > > > > > > default clk_disable_unused, if the driver that registered the clock has
> > > > > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > > > > skip disabling its clocks.
> > >
> > > Hi Abel,
> > >
> > > We have the day off today, so I'll respond more later. Also, please cc
> > > me on all sync_state() related patches in the future.
> > >
> >
> > Sure thing.
> >
> > > I haven't taken a close look at your series yet, but at a glance it
> > > seems incomplete.
> > >
> > > Any reason you didn't just try to revive my series[1] or nudge me?
> > > [1]- https://lore.kernel.org/lkml/[e-mail]/
> >
> > This patchset is heavily reworked and much more simpler as it relies
> > strictly on the sync_state being registered by the clock provider.
> 
> It's simpler because it's not complete. It for sure doesn't handle
> orphan-reparenting. It also doesn't make a lot of sense for only some
> clock providers registering for sync_state(). If CC-A is feeding a
> clock signal that's used as a root for clocks in CC-B, then what
> happens if only CC-B implements sync_state() but CC-A doesn't. The
> clocks from CC-B are still going to turn off when CC-A turns off its
> PLL before CC-B registers.
> 
> Nack for this patch.
> 
> Also, unless there's a strong objection, let's go back to my patch
> please. It's way more well tested and used across different SoCs than
> this patch. Also, I'm pretty sure the orphan handling is needed for
> qcom SoC's too.

Fine. Will wait for a respin of your patchset. Please CC me on it.

Bjorn please drop this patchset from your tree/pull request.

> 
> -Saravana
> 
> >
> > I saw your patchset a few months ago but then forgot about its
> > existence. That's also why I forgot to nudge you. Sorry about that.
> >
> > >
> > > At the least, I know [1] works on all Android devices (including
> > > Qualcomm SoCs) released in the past 2-3 years or more. If [1] works
> > > for you, I'd rather land that after addressing Stephen's comments
> > > there (I remember them being fairly easy to address comments) instead
> > > of whipping up a new series that's not as well used. I just got busy
> > > with other things and addressing more fundamental fw_devlink TODOs
> > > before getting back to this.
> > >
> > > Hi Bjorn,
> > >
> > > I see in another reply you've said:
> > >
> > > Applied, thanks!
> > >
> > > [1/2] clk: Add generic sync_state callback for disabling unused clocks
> > >       commit: 26b36df7516692292312063ca6fd19e73c06d4e7
> > > [2/2] clk: qcom: sdm845: Use generic clk_sync_state_disable_unused callback
> > >       commit: 99c0f7d35c4b204dd95ba50e155f32c99695b445
> > >
> > > Where exactly have you applied them? I hope you haven't applied the
> > > clk.c changes to some tree that goes into 6.3.
> >
> > I think it is already part of Bjorn's Qualcomm clocks pull request.
> >
> > >
> > > -Saravana
> > >
> > > > > >
> > > > > > How does that avoid disabling clks randomly in the clk tree? I'm
> > > > > > concerned about disabling an unused clk in the middle of the tree
> > > > > > because it doesn't have a driver using sync state, while the clk is the
> > > > > > parent of an unused clk that is backed by sync state.
> > > > > >
> > > > > >    clk A -->  clk B
> > > > > >
> > > > > > clk A: No sync state
> > > > > > clk B: sync state
> > > > > >
> > > > > > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > > > > > from late init. Imagine clk A is the root of the tree.
> > > > > >
> > > > > >     clk_disable_unused_subtree(clk_core A)
> > > > > >       clk_disable_unused_subtree(clk_core B)
> > > > > >         if (from_sync_state && core->dev != dev)
> > > > > >           return;
> > > > > >       ...
> > > > > >       clk core A->ops->disable()
> > > > > >
> > > > > > clk core B is off now?
> > > > >
> > > > > Yes, that is correct. But the same thing is happening currently if the
> > > > > clk_ignore_unused in not specified. At least with this new approach, we
> > > > > get to leave unused clocks enabled either until sync_state is called or forever.
> > > > > All the provider has to do is to implement a sync_state callback (or use
> > > > > the generic one provided). So the provider of clk A would obviously need
> > > > > a sync state callback registered.
> > > > >
> > > > > >
> > > > > > Also sync_state seems broken right now. I saw mka mentioned that if you
> > > > > > have a device node enabled in your DT but never enable a driver for it
> > > > > > in the kernel we'll never get sync_state called. This is another
> > > > > > problem, but it concerns me that sync_state would make the unused clk
> > > > > > disabling happen at some random time or not at all.
> > > > >
> > > > > Well, the fact that the sync state not being called because a driver for
> > > > > a consumer device doesn't probe does not really mean it is broken. Just
> > > > > because the consumer driver hasn't probed yet, doesn't mean it will
> > > > > not probe later on.
> > > > >
> > > >
> > > > CC'ed Saravana
> > > >
> > > > > That aside, rather than going with clk_ignore_unused all the time on
> > > > > qcom platforms, at least in a perfect scenario (where sync state is
> > > > > reached for all providers) the clocks get disabled.
> > > > >
> > > > > >
> > > > > > Can the problem be approached more directly? If this is about fixing
> > > > > > continuous splash screen, then I wonder why we can't list out the clks
> > > > > > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > > > > >
> > > > > >     clock-controller {
> > > > > >             #clock-cells = <1>;
> > > > > >             boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> > > > > >     };
> > > > > >
> > > > > > Then mark those as "critical/don't turn off" all the way up the clk tree
> > > > > > when the clk driver probes by essentially incrementing the
> > > > > > prepare/enable count but not actually touching the hardware, and when
> > > > > > the clks are acquired by clk_get() for that device that's using them
> > > > > > from boot we make the first clk_prepare_enable() do nothing and not
> > > > > > increment the count at all. We can probably stick some flag into the
> > > > > > 'struct clk' for this when we create the handle in clk_get() so that the
> > > > > > prepare and enable functions can special case and skip over.
> > > > >
> > > > > Well, that means we need to play whack-a-mole by alsways adding such clocks to
> > > > > devicetree.
> > > > >
> > > > > >
> > > > > > The sync_state hook operates on a driver level, which is too large when
> > > > > > you consider that a single clk driver may register hundreds of clks that
> > > > > > are not related. We want to target a solution at the clk level so that
> > > > > > any damage from keeping on all the clks provided by the controller is
> > > > > > limited to just the drivers that aren't probed and ready to handle their
> > > > > > clks. If sync_state could be called whenever a clk consumer consumes a
> > > > > > clk it may work? Technically we already have that by the clk_hw_provider
> > > > > > function but there isn't enough information being passed there, like the
> > > > > > getting device.
> > > > >
> > > > > Actually, from the multitude of clocks registered by one provider, the
> > > > > ones already explicitely enabled (and obvisously their parents) by thier
> > > > > consumer are safe. The only ones we need to worry about are the ones that
> > > > > might be enabled by bootloader and need to remain on. With the sync state
> > > > > approach, the latter mentioned clocks will either remain on indefinitely
> > > > > or will be disabled on sync state. The provider driver is the only level
> > > > > that has a registered sync state callback.
> > > > >
> > > > > >
> > > > > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > > > > > index 842e72a5348f..cf1adfeaf257 100644
> > > > > > > --- a/include/linux/clk-provider.h
> > > > > > > +++ b/include/linux/clk-provider.h
> > > > > > > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> > > > > > >                 void __iomem *reg, u8 shift, u8 width,
> > > > > > >                 u8 clk_divider_flags, const struct clk_div_table *table,
> > > > > > >                 spinlock_t *lock);
> > > > > > > +void clk_sync_state_disable_unused(struct device *dev);
> > > > > >
> > > > > > This is a weird place to put this. Why not in the helper functions
> > > > > > section?
> > > > >
> > > > > Sure this can be moved.
> > > > >
> > > > > >
> > > > > > >  /**
> > > > > > >   * clk_register_divider - register a divider clock with the clock framework
> > > > > > >   * @dev: device registering this clock

Bjorn Andersson Feb. 22, 2023, 3:34 p.m. UTC | #15

On Tue, Feb 21, 2023 at 11:24:36AM -0800, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2023-02-20 08:21:37)
> > On Fri, Feb 17, 2023 at 09:38:22PM -0800, Stephen Boyd wrote:
> > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > sync_state callback for the clock providers that register such clocks.
> > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > callback, but pass the device to make sure only the clocks belonging to
> > > > the current clock provider get disabled, if unused. Also, during the
> > > > default clk_disable_unused, if the driver that registered the clock has
> > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > skip disabling its clocks.
> > > 
> > > How does that avoid disabling clks randomly in the clk tree? I'm
> > > concerned about disabling an unused clk in the middle of the tree
> > > because it doesn't have a driver using sync state, while the clk is the
> > > parent of an unused clk that is backed by sync state.
> > > 
> > >    clk A -->  clk B 
> > > 
> > > clk A: No sync state
> > > clk B: sync state
> > > 
> > > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > > from late init. Imagine clk A is the root of the tree.
> > > 
> > >       clk_disable_unused_subtree(clk_core A)
> > >         clk_disable_unused_subtree(clk_core B)
> > >           if (from_sync_state && core->dev != dev)
> > >             return;
> > >         ...
> > >         clk core A->ops->disable()
> > > 
> > > clk core B is off now?
> > > 
> > 
> > I will have to give this some more thought. But this is exactly what we
> > have today; consider A being any builtin clock driver and B being any
> > clock driver built as modules, with relationship to A.
> > 
> > clk_disable_unused() will take down A without waiting for B, possibly
> > locking up parts of the clock hardware of B; or turning off the clocks
> > to IP blocks that rely on those clocks (e.g. display).
> 
> Oh, thanks for clarifying! Yes, the disabling of unused clks with
> respect to modules is broken in the same way. This makes that brokenness
> equally apply to builtin drivers, making the situation worse, not better.
> 

It does indeed improve the situation, because it allow us to have clock
drivers and consumers as modules without having their clocks disabled
prematurely (or by chance not be disabled ever).

But it does come at the cost of disabling unused clocks later, or if the
system doesn't probe all enabled devices, possibly never.

[..]
> > 
> > Presumably this list should not be a manually maintained list of display
> > clocks, and that means the bootloader would need to go in and  build
> > this list of all enabled clocks. I don't think this is practical.
> 
> Why does the bootloader need to do that? The devicetree author can list
> out the clks that they want to keep on for the display driver until the
> display driver can acquire them.
> 

I don't think maintaining this list is either necessary or the solution
to our problem. But if we need it, I don't think the list of clock which
happens to be needed for the author to boot his particular build of
Linux is sufficient "hardware description".

We need to solve the ordering issue, because we do want as many clock
drivers built as modules as possible, and anything shutting down
seemingly unused clocks at late_initcall() is causing problems, beyond
display.

> > 
> > > Then mark those as "critical/don't turn off" all the way up the clk tree
> > > when the clk driver probes by essentially incrementing the
> > > prepare/enable count but not actually touching the hardware, and when
> > > the clks are acquired by clk_get() for that device that's using them
> > > from boot we make the first clk_prepare_enable() do nothing and not
> > > increment the count at all. We can probably stick some flag into the
> > > 'struct clk' for this when we create the handle in clk_get() so that the
> > > prepare and enable functions can special case and skip over.
> > > 
> > 
> > The benefit of sync_state is that it kicks when the client drivers has
> > probed. As such, you can have e.g. the display driver clk_get(), then
> > probe defer on some other resource, and the clock state can remain
> > untouched.
> 
> Ok. I think this spitball design would do that still. It's not like we
> would go and disable the clks that are handed to the display driver even
> if it probe defers. The clk would be marked as enabled until the display
> driver enables the clk, and then it wouldn't be disabled during late
> init (or later) because the clk would be enabled either by the core or
> by the display driver. The point where we transfer ownership of the
> enable state is when the consumer calls clk_enable().
> 

You're right, and if a driver where to acquire a clock enable/disable it
and then probe defer, the sync_state mechanism wouldn't help us anyways.

But we need something that kicks in and disables unused clocks whenever
there will be no more consumers. Regardless if the list of resources to
do this with is defined my human, machine or derived from something.

> > 
> > > The sync_state hook operates on a driver level, which is too large when
> > > you consider that a single clk driver may register hundreds of clks that
> > > are not related. We want to target a solution at the clk level so that
> > > any damage from keeping on all the clks provided by the controller is
> > > limited to just the drivers that aren't probed and ready to handle their
> > > clks. If sync_state could be called whenever a clk consumer consumes a
> > > clk it may work? Technically we already have that by the clk_hw_provider
> > > function but there isn't enough information being passed there, like the
> > > getting device.
> > > 
> > 
> > The current solution already operates on all clocks of all drivers, that
> > happens to be probed at late_initcall(). This patch removes the
> > subordinate clause from this, allowing clock drivers and their clients
> > to be built as modules.
> > 
> > So while it still operates on all clocks of a driver, it moves that
> > point to a later stage, where that is more reasonable to do.
> > 
> 
> When we have clk drivers that provide clks to many different device
> drivers, they all have to probe for any unused clks to be disabled.
> 

I would prefer that we go to a mechanism where we disable all unused
clocks per provider, based on sync_state, and then from there try to
optimize that to disable subsets of those clocks a few seconds
earlier. Because upstream is broken by design right now.

I've reverted the applies patches, and sent a new pull request. Let's
try to make some progress on Saravana's proposal.

Regards,
Bjorn

Abel Vesa May 11, 2023, 12:58 p.m. UTC | #16

On 23-02-21 11:58:24, Saravana Kannan wrote:
> On Mon, Feb 20, 2023 at 10:47 AM Abel Vesa <[e-mail]> wrote:
> >
> > On 23-02-20 09:51:55, Saravana Kannan wrote:
> > > On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
> > > >
> > > > On 23-02-20 17:46:36, Abel Vesa wrote:
> > > > > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > > > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > > > > sync_state callback for the clock providers that register such clocks.
> > > > > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > > > > callback, but pass the device to make sure only the clocks belonging to
> > > > > > > the current clock provider get disabled, if unused. Also, during the
> > > > > > > default clk_disable_unused, if the driver that registered the clock has
> > > > > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > > > > skip disabling its clocks.
> > >
> > > Hi Abel,
> > >
> > > We have the day off today, so I'll respond more later. Also, please cc
> > > me on all sync_state() related patches in the future.
> > >
> >
> > Sure thing.
> >
> > > I haven't taken a close look at your series yet, but at a glance it
> > > seems incomplete.
> > >
> > > Any reason you didn't just try to revive my series[1] or nudge me?
> > > [1]- https://lore.kernel.org/lkml/[e-mail]/
> >
> > This patchset is heavily reworked and much more simpler as it relies
> > strictly on the sync_state being registered by the clock provider.
> 
> It's simpler because it's not complete. It for sure doesn't handle
> orphan-reparenting. It also doesn't make a lot of sense for only some
> clock providers registering for sync_state(). If CC-A is feeding a
> clock signal that's used as a root for clocks in CC-B, then what
> happens if only CC-B implements sync_state() but CC-A doesn't. The
> clocks from CC-B are still going to turn off when CC-A turns off its
> PLL before CC-B registers.

I gave your patchset a try and it breaks the uart for qcom platforms.
That is because your patchset enables the clock on __clk_core_init and
does not take into account the fact that 'boot enabled' clocks should be
left untouched. This also means the orphan-reparenting enabling should
be dropped as well.

As for the second part, related to providers that might not have a
registered sync_state(), your patchset sets the clock core generic
one. This is also wrong because it doesn't take into account the fact
that there might be providers that need to do their own stuff on
sync_state() and should do that by registering their own implementation
of it.

Therefore, I'll respin your patchset and use only the skipping of
disabling the unused clocks, but I'll drop all the enable on init and orphan
reparenting changes.

> 
> Nack for this patch.
> 
> Also, unless there's a strong objection, let's go back to my patch
> please. It's way more well tested and used across different SoCs than
> this patch. Also, I'm pretty sure the orphan handling is needed for
> qcom SoC's too.
> 
> -Saravana
> 
> >
> > I saw your patchset a few months ago but then forgot about its
> > existence. That's also why I forgot to nudge you. Sorry about that.
> >
> > >
> > > At the least, I know [1] works on all Android devices (including
> > > Qualcomm SoCs) released in the past 2-3 years or more. If [1] works
> > > for you, I'd rather land that after addressing Stephen's comments
> > > there (I remember them being fairly easy to address comments) instead
> > > of whipping up a new series that's not as well used. I just got busy
> > > with other things and addressing more fundamental fw_devlink TODOs
> > > before getting back to this.
> > >
> > > Hi Bjorn,
> > >
> > > I see in another reply you've said:
> > >
> > > Applied, thanks!
> > >
> > > [1/2] clk: Add generic sync_state callback for disabling unused clocks
> > >       commit: 26b36df7516692292312063ca6fd19e73c06d4e7
> > > [2/2] clk: qcom: sdm845: Use generic clk_sync_state_disable_unused callback
> > >       commit: 99c0f7d35c4b204dd95ba50e155f32c99695b445
> > >
> > > Where exactly have you applied them? I hope you haven't applied the
> > > clk.c changes to some tree that goes into 6.3.
> >
> > I think it is already part of Bjorn's Qualcomm clocks pull request.
> >
> > >
> > > -Saravana
> > >
> > > > > >
> > > > > > How does that avoid disabling clks randomly in the clk tree? I'm
> > > > > > concerned about disabling an unused clk in the middle of the tree
> > > > > > because it doesn't have a driver using sync state, while the clk is the
> > > > > > parent of an unused clk that is backed by sync state.
> > > > > >
> > > > > >    clk A -->  clk B
> > > > > >
> > > > > > clk A: No sync state
> > > > > > clk B: sync state
> > > > > >
> > > > > > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
> > > > > > from late init. Imagine clk A is the root of the tree.
> > > > > >
> > > > > >     clk_disable_unused_subtree(clk_core A)
> > > > > >       clk_disable_unused_subtree(clk_core B)
> > > > > >         if (from_sync_state && core->dev != dev)
> > > > > >           return;
> > > > > >       ...
> > > > > >       clk core A->ops->disable()
> > > > > >
> > > > > > clk core B is off now?
> > > > >
> > > > > Yes, that is correct. But the same thing is happening currently if the
> > > > > clk_ignore_unused in not specified. At least with this new approach, we
> > > > > get to leave unused clocks enabled either until sync_state is called or forever.
> > > > > All the provider has to do is to implement a sync_state callback (or use
> > > > > the generic one provided). So the provider of clk A would obviously need
> > > > > a sync state callback registered.
> > > > >
> > > > > >
> > > > > > Also sync_state seems broken right now. I saw mka mentioned that if you
> > > > > > have a device node enabled in your DT but never enable a driver for it
> > > > > > in the kernel we'll never get sync_state called. This is another
> > > > > > problem, but it concerns me that sync_state would make the unused clk
> > > > > > disabling happen at some random time or not at all.
> > > > >
> > > > > Well, the fact that the sync state not being called because a driver for
> > > > > a consumer device doesn't probe does not really mean it is broken. Just
> > > > > because the consumer driver hasn't probed yet, doesn't mean it will
> > > > > not probe later on.
> > > > >
> > > >
> > > > CC'ed Saravana
> > > >
> > > > > That aside, rather than going with clk_ignore_unused all the time on
> > > > > qcom platforms, at least in a perfect scenario (where sync state is
> > > > > reached for all providers) the clocks get disabled.
> > > > >
> > > > > >
> > > > > > Can the problem be approached more directly? If this is about fixing
> > > > > > continuous splash screen, then I wonder why we can't list out the clks
> > > > > > that we know are enabled by the bootloader in some new DT binding, e.g.:
> > > > > >
> > > > > >     clock-controller {
> > > > > >             #clock-cells = <1>;
> > > > > >             boot-handoff-clocks = <&consumer_device "clock cells for this clk provider">;
> > > > > >     };
> > > > > >
> > > > > > Then mark those as "critical/don't turn off" all the way up the clk tree
> > > > > > when the clk driver probes by essentially incrementing the
> > > > > > prepare/enable count but not actually touching the hardware, and when
> > > > > > the clks are acquired by clk_get() for that device that's using them
> > > > > > from boot we make the first clk_prepare_enable() do nothing and not
> > > > > > increment the count at all. We can probably stick some flag into the
> > > > > > 'struct clk' for this when we create the handle in clk_get() so that the
> > > > > > prepare and enable functions can special case and skip over.
> > > > >
> > > > > Well, that means we need to play whack-a-mole by alsways adding such clocks to
> > > > > devicetree.
> > > > >
> > > > > >
> > > > > > The sync_state hook operates on a driver level, which is too large when
> > > > > > you consider that a single clk driver may register hundreds of clks that
> > > > > > are not related. We want to target a solution at the clk level so that
> > > > > > any damage from keeping on all the clks provided by the controller is
> > > > > > limited to just the drivers that aren't probed and ready to handle their
> > > > > > clks. If sync_state could be called whenever a clk consumer consumes a
> > > > > > clk it may work? Technically we already have that by the clk_hw_provider
> > > > > > function but there isn't enough information being passed there, like the
> > > > > > getting device.
> > > > >
> > > > > Actually, from the multitude of clocks registered by one provider, the
> > > > > ones already explicitely enabled (and obvisously their parents) by thier
> > > > > consumer are safe. The only ones we need to worry about are the ones that
> > > > > might be enabled by bootloader and need to remain on. With the sync state
> > > > > approach, the latter mentioned clocks will either remain on indefinitely
> > > > > or will be disabled on sync state. The provider driver is the only level
> > > > > that has a registered sync state callback.
> > > > >
> > > > > >
> > > > > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > > > > > index 842e72a5348f..cf1adfeaf257 100644
> > > > > > > --- a/include/linux/clk-provider.h
> > > > > > > +++ b/include/linux/clk-provider.h
> > > > > > > @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
> > > > > > >                 void __iomem *reg, u8 shift, u8 width,
> > > > > > >                 u8 clk_divider_flags, const struct clk_div_table *table,
> > > > > > >                 spinlock_t *lock);
> > > > > > > +void clk_sync_state_disable_unused(struct device *dev);
> > > > > >
> > > > > > This is a weird place to put this. Why not in the helper functions
> > > > > > section?
> > > > >
> > > > > Sure this can be moved.
> > > > >
> > > > > >
> > > > > > >  /**
> > > > > > >   * clk_register_divider - register a divider clock with the clock framework
> > > > > > >   * @dev: device registering this clock

Saravana Kannan May 12, 2023, 12:46 a.m. UTC | #17

On Thu, May 11, 2023 at 5:58 AM Abel Vesa <[e-mail]> wrote:
>
> On 23-02-21 11:58:24, Saravana Kannan wrote:
> > On Mon, Feb 20, 2023 at 10:47 AM Abel Vesa <[e-mail]> wrote:
> > >
> > > On 23-02-20 09:51:55, Saravana Kannan wrote:
> > > > On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
> > > > >
> > > > > On 23-02-20 17:46:36, Abel Vesa wrote:
> > > > > > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > > > > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > > > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > > > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > > > > > sync_state callback for the clock providers that register such clocks.
> > > > > > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > > > > > callback, but pass the device to make sure only the clocks belonging to
> > > > > > > > the current clock provider get disabled, if unused. Also, during the
> > > > > > > > default clk_disable_unused, if the driver that registered the clock has
> > > > > > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > > > > > skip disabling its clocks.
> > > >
> > > > Hi Abel,
> > > >
> > > > We have the day off today, so I'll respond more later. Also, please cc
> > > > me on all sync_state() related patches in the future.
> > > >
> > >
> > > Sure thing.
> > >
> > > > I haven't taken a close look at your series yet, but at a glance it
> > > > seems incomplete.
> > > >
> > > > Any reason you didn't just try to revive my series[1] or nudge me?
> > > > [1]- https://lore.kernel.org/lkml/[e-mail]/
> > >
> > > This patchset is heavily reworked and much more simpler as it relies
> > > strictly on the sync_state being registered by the clock provider.
> >
> > It's simpler because it's not complete. It for sure doesn't handle
> > orphan-reparenting. It also doesn't make a lot of sense for only some
> > clock providers registering for sync_state(). If CC-A is feeding a
> > clock signal that's used as a root for clocks in CC-B, then what
> > happens if only CC-B implements sync_state() but CC-A doesn't. The
> > clocks from CC-B are still going to turn off when CC-A turns off its
> > PLL before CC-B registers.
>
> I gave your patchset a try and it breaks the uart for qcom platforms.
> That is because your patchset enables the clock on __clk_core_init and
> does not take into account the fact that 'boot enabled' clocks should be
> left untouched.

Those are probably just hacks when we didn't have sync_state(). But
sure, we can make sure existing drivers aren't broken if the flag is
set.

> This also means the orphan-reparenting enabling should
> be dropped as well.

No, maybe for boot enabled clocks, but not for all clocks in general.
You need this for sync_state() to work correctly for clocks left on at
boot but "boot enabled" isn't set.

> As for the second part, related to providers that might not have a
> registered sync_state(), your patchset sets the clock core generic
> one. This is also wrong because it doesn't take into account the fact
> that there might be providers that need to do their own stuff on
> sync_state() and should do that by registering their own implementation
> of it.

Right, in which case, they can set theirs or they get the default one.

> Therefore, I'll respin your patchset and use only the skipping of
> disabling the unused clocks, but I'll drop all the enable on init and orphan
> reparenting changes.

I think it'll result in a broken patch.

Sorry, I've been a bit busy with some other work and I haven't been
able to get to the clk_sync_state(). I'll try to rebase it soon and
send it out too.

-Saravana

Abel Vesa May 12, 2023, 10:30 a.m. UTC | #18

On 23-05-11 17:46:16, Saravana Kannan wrote:
> On Thu, May 11, 2023 at 5:58 AM Abel Vesa <[e-mail]> wrote:
> >
> > On 23-02-21 11:58:24, Saravana Kannan wrote:
> > > On Mon, Feb 20, 2023 at 10:47 AM Abel Vesa <[e-mail]> wrote:
> > > >
> > > > On 23-02-20 09:51:55, Saravana Kannan wrote:
> > > > > On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
> > > > > >
> > > > > > On 23-02-20 17:46:36, Abel Vesa wrote:
> > > > > > > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > > > > > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > > > > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > > > > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > > > > > > sync_state callback for the clock providers that register such clocks.
> > > > > > > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > > > > > > callback, but pass the device to make sure only the clocks belonging to
> > > > > > > > > the current clock provider get disabled, if unused. Also, during the
> > > > > > > > > default clk_disable_unused, if the driver that registered the clock has
> > > > > > > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > > > > > > skip disabling its clocks.
> > > > >
> > > > > Hi Abel,
> > > > >
> > > > > We have the day off today, so I'll respond more later. Also, please cc
> > > > > me on all sync_state() related patches in the future.
> > > > >
> > > >
> > > > Sure thing.
> > > >
> > > > > I haven't taken a close look at your series yet, but at a glance it
> > > > > seems incomplete.
> > > > >
> > > > > Any reason you didn't just try to revive my series[1] or nudge me?
> > > > > [1]- https://lore.kernel.org/lkml/[e-mail]/
> > > >
> > > > This patchset is heavily reworked and much more simpler as it relies
> > > > strictly on the sync_state being registered by the clock provider.
> > >
> > > It's simpler because it's not complete. It for sure doesn't handle
> > > orphan-reparenting. It also doesn't make a lot of sense for only some
> > > clock providers registering for sync_state(). If CC-A is feeding a
> > > clock signal that's used as a root for clocks in CC-B, then what
> > > happens if only CC-B implements sync_state() but CC-A doesn't. The
> > > clocks from CC-B are still going to turn off when CC-A turns off its
> > > PLL before CC-B registers.
> >
> > I gave your patchset a try and it breaks the uart for qcom platforms.
> > That is because your patchset enables the clock on __clk_core_init and
> > does not take into account the fact that 'boot enabled' clocks should be
> > left untouched.
> 
> Those are probably just hacks when we didn't have sync_state(). But
> sure, we can make sure existing drivers aren't broken if the flag is
> set.

I probably didn't make myself clear enough here. ANY clock that is
enabled (HW-wise) before the kernel boots should remain AS IS, that is, no writing
the enable bit, no reparenting, and so on. This rule applies to the clock itself
and for all of its parents. This is because, for some clocks, writing the
enable bit might lead to glitches. UART is just one example. So, please, do not
try enabling such clocks until the consumer driver does so.

> 
> > This also means the orphan-reparenting enabling should
> > be dropped as well.
> 
> No, maybe for boot enabled clocks, but not for all clocks in general.
> You need this for sync_state() to work correctly for clocks left on at
> boot but "boot enabled" isn't set.

I think you lost me here. What do you mean by 'this'? If you mean the
enabling of orphan clocks, then the rule above still applies. It
doesn't matter if the clock is an orphan one or not. It can be orphan
from linux point of view, but the actual parent (even if it is not
registered with the linux clock tree) might still be enabled. This means
the clock itself will be also enabled. And by enabling them when
registering, we can have glitches. Therefore, please, do not do this
either.

The registering of a boot enabled clock should not change/override/touch
the current state of it in any way!

Stephen, can you confirm this as well?

> 
> > As for the second part, related to providers that might not have a
> > registered sync_state(), your patchset sets the clock core generic
> > one. This is also wrong because it doesn't take into account the fact
> > that there might be providers that need to do their own stuff on
> > sync_state() and should do that by registering their own implementation
> > of it.
> 
> Right, in which case, they can set theirs or they get the default one.

I'm still not sure that defaulting to the clk_sync_state callback is a
good choice here. I have to think some more about what the impact is for
providers that do not have any sync_state callback registered currently.

> 
> > Therefore, I'll respin your patchset and use only the skipping of
> > disabling the unused clocks, but I'll drop all the enable on init and orphan
> > reparenting changes.
> 
> I think it'll result in a broken patch.

Yep, tried that and it doesn't work. What happened was that, because you
were enabling the 'boot enabled' clocks when registering them (on __clk_core_init),
the disabling from the sync state needs to be without dropping the enable/prepare
counts. This is why I think my patchset here is the best alternative he have
currently, as it does exactly what it is supposed to do, namingly, to leave
untouched the boot enabled clocks until sync state and then disabling
them with via clk_disable_unused_subtree which calls the disable and
unprepare ops without decrementing the prepare and enable counts.

> 
> Sorry, I've been a bit busy with some other work and I haven't been
> able to get to the clk_sync_state(). I'll try to rebase it soon and
> send it out too.

Well, I already did that and I described above why that won't help.

> 
> -Saravana

Saravana Kannan May 27, 2023, 12:13 a.m. UTC | #19

On Fri, May 12, 2023 at 3:31 AM Abel Vesa <[e-mail]> wrote:
>
> On 23-05-11 17:46:16, Saravana Kannan wrote:
> > On Thu, May 11, 2023 at 5:58 AM Abel Vesa <[e-mail]> wrote:
> > >
> > > On 23-02-21 11:58:24, Saravana Kannan wrote:
> > > > On Mon, Feb 20, 2023 at 10:47 AM Abel Vesa <[e-mail]> wrote:
> > > > >
> > > > > On 23-02-20 09:51:55, Saravana Kannan wrote:
> > > > > > On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <[e-mail]> wrote:
> > > > > > >
> > > > > > > On 23-02-20 17:46:36, Abel Vesa wrote:
> > > > > > > > On 23-02-17 21:38:22, Stephen Boyd wrote:
> > > > > > > > > Quoting Abel Vesa (2022-12-27 12:45:27)
> > > > > > > > > > There are unused clocks that need to remain untouched by clk_disable_unused,
> > > > > > > > > > and most likely could be disabled later on sync_state. So provide a generic
> > > > > > > > > > sync_state callback for the clock providers that register such clocks.
> > > > > > > > > > Then, use the same mechanism as clk_disable_unused from that generic
> > > > > > > > > > callback, but pass the device to make sure only the clocks belonging to
> > > > > > > > > > the current clock provider get disabled, if unused. Also, during the
> > > > > > > > > > default clk_disable_unused, if the driver that registered the clock has
> > > > > > > > > > the generic clk_sync_state_disable_unused callback set for sync_state,
> > > > > > > > > > skip disabling its clocks.
> > > > > >
> > > > > > Hi Abel,
> > > > > >
> > > > > > We have the day off today, so I'll respond more later. Also, please cc
> > > > > > me on all sync_state() related patches in the future.
> > > > > >
> > > > >
> > > > > Sure thing.
> > > > >
> > > > > > I haven't taken a close look at your series yet, but at a glance it
> > > > > > seems incomplete.
> > > > > >
> > > > > > Any reason you didn't just try to revive my series[1] or nudge me?
> > > > > > [1]- https://lore.kernel.org/lkml/[e-mail]/
> > > > >
> > > > > This patchset is heavily reworked and much more simpler as it relies
> > > > > strictly on the sync_state being registered by the clock provider.
> > > >
> > > > It's simpler because it's not complete. It for sure doesn't handle
> > > > orphan-reparenting. It also doesn't make a lot of sense for only some
> > > > clock providers registering for sync_state(). If CC-A is feeding a
> > > > clock signal that's used as a root for clocks in CC-B, then what
> > > > happens if only CC-B implements sync_state() but CC-A doesn't. The
> > > > clocks from CC-B are still going to turn off when CC-A turns off its
> > > > PLL before CC-B registers.
> > >
> > > I gave your patchset a try and it breaks the uart for qcom platforms.
> > > That is because your patchset enables the clock on __clk_core_init and
> > > does not take into account the fact that 'boot enabled' clocks should be
> > > left untouched.
> >
> > Those are probably just hacks when we didn't have sync_state(). But
> > sure, we can make sure existing drivers aren't broken if the flag is
> > set.
>
> I probably didn't make myself clear enough here. ANY clock that is
> enabled (HW-wise) before the kernel boots should remain AS IS, that is, no writing
> the enable bit, no reparenting, and so on. This rule applies to the clock itself
> and for all of its parents. This is because, for some clocks, writing the
> enable bit might lead to glitches. UART is just one example. So, please, do not
> try enabling such clocks until the consumer driver does so.
>
> >
> > > This also means the orphan-reparenting enabling should
> > > be dropped as well.
> >
> > No, maybe for boot enabled clocks, but not for all clocks in general.
> > You need this for sync_state() to work correctly for clocks left on at
> > boot but "boot enabled" isn't set.
>
> I think you lost me here. What do you mean by 'this'? If you mean the
> enabling of orphan clocks, then the rule above still applies. It
> doesn't matter if the clock is an orphan one or not. It can be orphan
> from linux point of view, but the actual parent (even if it is not
> registered with the linux clock tree) might still be enabled. This means
> the clock itself will be also enabled. And by enabling them when
> registering, we can have glitches. Therefore, please, do not do this
> either.
>
> The registering of a boot enabled clock should not change/override/touch
> the current state of it in any way!
>
> Stephen, can you confirm this as well?
>
> >
> > > As for the second part, related to providers that might not have a
> > > registered sync_state(), your patchset sets the clock core generic
> > > one. This is also wrong because it doesn't take into account the fact
> > > that there might be providers that need to do their own stuff on
> > > sync_state() and should do that by registering their own implementation
> > > of it.
> >
> > Right, in which case, they can set theirs or they get the default one.
>
> I'm still not sure that defaulting to the clk_sync_state callback is a
> good choice here. I have to think some more about what the impact is for
> providers that do not have any sync_state callback registered currently.
>
> >
> > > Therefore, I'll respin your patchset and use only the skipping of
> > > disabling the unused clocks, but I'll drop all the enable on init and orphan
> > > reparenting changes.
> >
> > I think it'll result in a broken patch.
>
> Yep, tried that and it doesn't work. What happened was that, because you
> were enabling the 'boot enabled' clocks when registering them (on __clk_core_init),
> the disabling from the sync state needs to be without dropping the enable/prepare
> counts. This is why I think my patchset here is the best alternative he have
> currently, as it does exactly what it is supposed to do, namingly, to leave
> untouched the boot enabled clocks until sync state and then disabling
> them with via clk_disable_unused_subtree which calls the disable and
> unprepare ops without decrementing the prepare and enable counts.
>
> >
> > Sorry, I've been a bit busy with some other work and I haven't been
> > able to get to the clk_sync_state(). I'll try to rebase it soon and
> > send it out too.
>
> Well, I already did that and I described above why that won't help.

The biggest disconnect is that you seem to think boot enabled clocks
should be untouched until sync_state() is called on them. But that's
not a valid assumption. Boot enabled clocks can have multiple
consumers and one of them might want to change the frequency before
the other one probes. That's perfectly valid. In some cases, we might
need to make sure the clock frequency doesn't go higher than the clock
frequency at boot (we can make that a flag). Actually, even if there's
only one consumer, that consumer might change the clock frequency at
probe -- since sync_state() only comes after the probe(), we need to
make sure we allow reparenting and frequency changes to boot enabled
clocks before sync_state() is called.

Also, consider this example.

PLL1 -> mux1 -> clock_gate1 -> consumer1.
PLL1 -> mux2 -> clock_gate2 -> consumer2.

If I don't do orphan handling/reparenting like I do so in my patch,
PLL1 could get turned off after consumer 1 probes depending on which
clock controller each of those blocks are on.

I'm pretty sure I actually identified this issue when I wrote my patch
by testing it on QC hardware. So this is not some "other" hardware
issue. This actually affects QC hardware too. Maybe you haven't
upstreamed all of your hardware drivers, but this is not some
imaginary scenario.

Your main problem seems to be that your hardware can't handle writing
1 to the enable bit of a clock that's already on. If that's the case,
protect against that inside your driver. I'm even okay with figuring
out a way to try and support that at a framework level. But to say you
don't need to reparenting or the orphan handling is definitely wrong.

-Saravana

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e62552a75f08..ac7182903d88 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1302,14 +1302,26 @@  static void clk_core_disable_unprepare(struct clk_core *core)
 	clk_core_unprepare_lock(core);
 }
 
-static void __init clk_unprepare_unused_subtree(struct clk_core *core)
+static void clk_unprepare_unused_subtree(struct clk_core *core,
+						struct device *dev)
 {
+	bool from_sync_state = !!dev;
 	struct clk_core *child;
 
 	lockdep_assert_held(&prepare_lock);
 
 	hlist_for_each_entry(child, &core->children, child_node)
-		clk_unprepare_unused_subtree(child);
+		clk_unprepare_unused_subtree(child, dev);
+
+	if (from_sync_state && core->dev != dev)
+		return;
+
+	/*
+	 * clock will be unprepared on sync_state,
+	 * so leave as is for now
+	 */
+	if (!from_sync_state && dev_has_sync_state(core->dev))
+		return;
 
 	if (core->prepare_count)
 		return;
@@ -1332,15 +1344,27 @@  static void __init clk_unprepare_unused_subtree(struct clk_core *core)
 	clk_pm_runtime_put(core);
 }
 
-static void __init clk_disable_unused_subtree(struct clk_core *core)
+static void clk_disable_unused_subtree(struct clk_core *core,
+					struct device *dev)
 {
+	bool from_sync_state = !!dev;
 	struct clk_core *child;
 	unsigned long flags;
 
 	lockdep_assert_held(&prepare_lock);
 
 	hlist_for_each_entry(child, &core->children, child_node)
-		clk_disable_unused_subtree(child);
+		clk_disable_unused_subtree(child, dev);
+
+	if (from_sync_state && core->dev != dev)
+		return;
+
+	/*
+	 * clock will be disabled on sync_state,
+	 * so leave as is for now
+	 */
+	if (!from_sync_state && dev_has_sync_state(core->dev))
+		return;
 
 	if (core->flags & CLK_OPS_PARENT_ENABLE)
 		clk_core_prepare_enable(core->parent);
@@ -1378,7 +1402,7 @@  static void __init clk_disable_unused_subtree(struct clk_core *core)
 		clk_core_disable_unprepare(core->parent);
 }
 
-static bool clk_ignore_unused __initdata;
+static bool clk_ignore_unused;
 static int __init clk_ignore_unused_setup(char *__unused)
 {
 	clk_ignore_unused = true;
@@ -1386,35 +1410,46 @@  static int __init clk_ignore_unused_setup(char *__unused)
 }
 __setup("clk_ignore_unused", clk_ignore_unused_setup);
 
-static int __init clk_disable_unused(void)
+static void __clk_disable_unused(struct device *dev)
 {
 	struct clk_core *core;
 
 	if (clk_ignore_unused) {
 		pr_warn("clk: Not disabling unused clocks\n");
-		return 0;
+		return;
 	}
 
 	clk_prepare_lock();
 
 	hlist_for_each_entry(core, &clk_root_list, child_node)
-		clk_disable_unused_subtree(core);
+		clk_disable_unused_subtree(core, dev);
 
 	hlist_for_each_entry(core, &clk_orphan_list, child_node)
-		clk_disable_unused_subtree(core);
+		clk_disable_unused_subtree(core, dev);
 
 	hlist_for_each_entry(core, &clk_root_list, child_node)
-		clk_unprepare_unused_subtree(core);
+		clk_unprepare_unused_subtree(core, dev);
 
 	hlist_for_each_entry(core, &clk_orphan_list, child_node)
-		clk_unprepare_unused_subtree(core);
+		clk_unprepare_unused_subtree(core, dev);
 
 	clk_prepare_unlock();
+}
+
+static int __init clk_disable_unused(void)
+{
+	__clk_disable_unused(NULL);
 
 	return 0;
 }
 late_initcall_sync(clk_disable_unused);
 
+void clk_sync_state_disable_unused(struct device *dev)
+{
+	__clk_disable_unused(dev);
+}
+EXPORT_SYMBOL_GPL(clk_sync_state_disable_unused);
+
 static int clk_core_determine_round_nolock(struct clk_core *core,
 					   struct clk_rate_request *req)
 {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 842e72a5348f..cf1adfeaf257 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -720,6 +720,7 @@  struct clk *clk_register_divider_table(struct device *dev, const char *name,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, const struct clk_div_table *table,
 		spinlock_t *lock);
+void clk_sync_state_disable_unused(struct device *dev);
 /**
  * clk_register_divider - register a divider clock with the clock framework
  * @dev: device registering this clock