Re: [PATCH v1 2/2] clk: Add support for sync_state()
On Tue, Apr 6, 2021 at 8:45 PM 'Saravana Kannan' via kernel-team
wrote:
>
> Clocks can be turned on (by the hardware, bootloader, etc) upon a
> reset/boot of a hardware platform. These "boot clocks" could be clocking
> devices that are active before the kernel starts running. For example,
> clocks needed for the interconnects, UART console, display, CPUs, DDR,
> etc.
>
> When a boot clock is used by more than one consumer or multiple boot
> clocks share a parent clock, the boot clock (or the common parent) can
> be turned off when the first consumer probes. This can crash the device
> or cause poor user experience.
>
> Fix this by explicitly enabling the boot clocks during clock
> registration and then removing the enable vote when the clock provider
> device gets its sync_state() callback. Since sync_state() callback comes
> only when all the consumers of a device (not a specific clock) have
> probed, this ensures the boot clocks are kept on at least until all
> their consumers have had a chance to vote on them (in their respective
> probe functions).
>
> Also, if a clock provider is loaded as a module and it has some boot
> clocks, they get turned off only when a consumer explicitly turns them
> off. So clocks that are boot clocks and are unused never get turned off
> because the logic to turn off unused clocks has already run during
> late_initcall_sync(). Adding sync_state() support also makes sure these
> unused boot clocks are turned off once all the consumers have probed.
>
> Signed-off-by: Saravana Kannan
Hi Stephen,
Gentle reminder.
-Saravana
> ---
> drivers/clk/clk.c| 84 +++-
> include/linux/clk-provider.h | 1 +
> 2 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d6301a3351f2..cd07f4d1254c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -72,6 +72,8 @@ struct clk_core {
> unsigned long flags;
> boolorphan;
> boolrpm_enabled;
> + boolneed_sync;
> + boolboot_enabled;
> unsigned intenable_count;
> unsigned intprepare_count;
> unsigned intprotect_count;
> @@ -1215,6 +1217,15 @@ static void __init clk_unprepare_unused_subtree(struct
> clk_core *core)
> hlist_for_each_entry(child, &core->children, child_node)
> clk_unprepare_unused_subtree(child);
>
> + /*
> +* Orphan clocks might still not have their state held if one of their
> +* ancestors hasn't been registered yet. We don't want to turn off
> +* these orphan clocks now as they will be turned off later when their
> +* device gets a sync_state() call.
> +*/
> + if (dev_has_sync_state(core->dev))
> + return;
> +
> if (core->prepare_count)
> return;
>
> @@ -1246,6 +1257,15 @@ static void __init clk_disable_unused_subtree(struct
> clk_core *core)
> hlist_for_each_entry(child, &core->children, child_node)
> clk_disable_unused_subtree(child);
>
> + /*
> +* Orphan clocks might still not have their state held if one of their
> +* ancestors hasn't been registered yet. We don't want to turn off
> +* these orphan clocks now as they will be turned off later when their
> +* device gets a sync_state() call.
> +*/
> + if (dev_has_sync_state(core->dev))
> + return;
> +
> if (core->flags & CLK_OPS_PARENT_ENABLE)
> clk_core_prepare_enable(core->parent);
>
> @@ -1319,6 +1339,38 @@ static int __init clk_disable_unused(void)
> }
> late_initcall_sync(clk_disable_unused);
>
> +static void clk_unprepare_disable_dev_subtree(struct clk_core *core,
> + struct device *dev)
> +{
> + struct clk_core *child;
> +
> + lockdep_assert_held(&prepare_lock);
> +
> + hlist_for_each_entry(child, &core->children, child_node)
> + clk_unprepare_disable_dev_subtree(child, dev);
> +
> + if (core->dev != dev || !core->need_sync)
> + return;
> +
> + clk_core_disable_unprepare(core);
> +}
> +
> +void clk_sync_state(struct device *dev)
> +{
> + struct clk_core *core;
> +
> + clk_prepare_lock();
> +
> + hlist_for_each_entry(core, &clk_root_list, child_node)
> + clk_unprepare_disable_dev_subtree(core, dev);
> +
> + hlist_for_each_entry(core, &clk_orphan_list, child_node)
> + clk_unprepare_disable_dev_subtree(core, dev);
> +
> + clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_sync_state);
> +
> static int clk_core_determine_round_nolock(struct clk_core *core,
>struct clk_rate_request *req)
> {
> @@ -1725,6 +1777,30 @@ int clk_hw_get_parent_index(struct c
[PATCH v1 2/2] clk: Add support for sync_state()
Clocks can be turned on (by the hardware, bootloader, etc) upon a
reset/boot of a hardware platform. These "boot clocks" could be clocking
devices that are active before the kernel starts running. For example,
clocks needed for the interconnects, UART console, display, CPUs, DDR,
etc.
When a boot clock is used by more than one consumer or multiple boot
clocks share a parent clock, the boot clock (or the common parent) can
be turned off when the first consumer probes. This can crash the device
or cause poor user experience.
Fix this by explicitly enabling the boot clocks during clock
registration and then removing the enable vote when the clock provider
device gets its sync_state() callback. Since sync_state() callback comes
only when all the consumers of a device (not a specific clock) have
probed, this ensures the boot clocks are kept on at least until all
their consumers have had a chance to vote on them (in their respective
probe functions).
Also, if a clock provider is loaded as a module and it has some boot
clocks, they get turned off only when a consumer explicitly turns them
off. So clocks that are boot clocks and are unused never get turned off
because the logic to turn off unused clocks has already run during
late_initcall_sync(). Adding sync_state() support also makes sure these
unused boot clocks are turned off once all the consumers have probed.
Signed-off-by: Saravana Kannan
---
drivers/clk/clk.c| 84 +++-
include/linux/clk-provider.h | 1 +
2 files changed, 84 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d6301a3351f2..cd07f4d1254c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -72,6 +72,8 @@ struct clk_core {
unsigned long flags;
boolorphan;
boolrpm_enabled;
+ boolneed_sync;
+ boolboot_enabled;
unsigned intenable_count;
unsigned intprepare_count;
unsigned intprotect_count;
@@ -1215,6 +1217,15 @@ static void __init clk_unprepare_unused_subtree(struct
clk_core *core)
hlist_for_each_entry(child, &core->children, child_node)
clk_unprepare_unused_subtree(child);
+ /*
+* Orphan clocks might still not have their state held if one of their
+* ancestors hasn't been registered yet. We don't want to turn off
+* these orphan clocks now as they will be turned off later when their
+* device gets a sync_state() call.
+*/
+ if (dev_has_sync_state(core->dev))
+ return;
+
if (core->prepare_count)
return;
@@ -1246,6 +1257,15 @@ static void __init clk_disable_unused_subtree(struct
clk_core *core)
hlist_for_each_entry(child, &core->children, child_node)
clk_disable_unused_subtree(child);
+ /*
+* Orphan clocks might still not have their state held if one of their
+* ancestors hasn't been registered yet. We don't want to turn off
+* these orphan clocks now as they will be turned off later when their
+* device gets a sync_state() call.
+*/
+ if (dev_has_sync_state(core->dev))
+ return;
+
if (core->flags & CLK_OPS_PARENT_ENABLE)
clk_core_prepare_enable(core->parent);
@@ -1319,6 +1339,38 @@ static int __init clk_disable_unused(void)
}
late_initcall_sync(clk_disable_unused);
+static void clk_unprepare_disable_dev_subtree(struct clk_core *core,
+ struct device *dev)
+{
+ struct clk_core *child;
+
+ lockdep_assert_held(&prepare_lock);
+
+ hlist_for_each_entry(child, &core->children, child_node)
+ clk_unprepare_disable_dev_subtree(child, dev);
+
+ if (core->dev != dev || !core->need_sync)
+ return;
+
+ clk_core_disable_unprepare(core);
+}
+
+void clk_sync_state(struct device *dev)
+{
+ struct clk_core *core;
+
+ clk_prepare_lock();
+
+ hlist_for_each_entry(core, &clk_root_list, child_node)
+ clk_unprepare_disable_dev_subtree(core, dev);
+
+ hlist_for_each_entry(core, &clk_orphan_list, child_node)
+ clk_unprepare_disable_dev_subtree(core, dev);
+
+ clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_sync_state);
+
static int clk_core_determine_round_nolock(struct clk_core *core,
struct clk_rate_request *req)
{
@@ -1725,6 +1777,30 @@ int clk_hw_get_parent_index(struct clk_hw *hw)
}
EXPORT_SYMBOL_GPL(clk_hw_get_parent_index);
+static void clk_core_hold_state(struct clk_core *core)
+{
+ if (core->need_sync || !core->boot_enabled)
+ return;
+
+ if (core->orphan || !dev_has_sync_state(core->dev))
+ return;
+
+ core->need_sync = !clk_core_prepare_enable(core);
