| Message ID | 20191118234229.54085-1-saravanak@google.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v1] clk: Keep boot clocks on for multiple consumers | expand |
On Mon, Nov 18, 2019 at 3:42 PM Saravana Kannan <saravanak@google.com> wrote: > > Clocks can 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 potentially crash > the device or cause poor user experience. > > This patch fixes this by explicitly enabling the boot clocks during > clock registration and then disabling them at late_initcall_sync(). This > gives all the consumers until late_initcall() to put their "votes" in to > keep any of the boot clocks on past late_initcall(). > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/clk/clk.c | 62 ++++++++++++++++++++++++++++++++++++ > include/linux/clk-provider.h | 1 + > 2 files changed, 63 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 1c677d7f7f53..a1b09c9f8845 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -72,6 +72,8 @@ struct clk_core { > unsigned long flags; > bool orphan; > bool rpm_enabled; > + bool state_held; > + bool boot_enabled; > unsigned int enable_count; > unsigned int prepare_count; > unsigned int protect_count; > @@ -1300,6 +1302,36 @@ static int clk_disable_unused(void) > } > late_initcall_sync(clk_disable_unused); > > +static void clk_unprepare_disable_subtree(struct clk_core *core) > +{ > + struct clk_core *child; > + > + lockdep_assert_held(&prepare_lock); > + > + hlist_for_each_entry(child, &core->children, child_node) > + clk_unprepare_disable_subtree(child); > + > + if (!core->state_held) > + return; > + > + clk_core_disable_unprepare(core); > +} > + > +static int clk_release_boot_state(void) > +{ > + struct clk_core *core; > + > + clk_prepare_lock(); > + > + hlist_for_each_entry(core, &clk_root_list, child_node) > + clk_unprepare_disable_subtree(core); > + > + clk_prepare_unlock(); > + > + return 0; > +} > +late_initcall_sync(clk_release_boot_state); > + > static int clk_core_determine_round_nolock(struct clk_core *core, > struct clk_rate_request *req) > { > @@ -1674,6 +1706,30 @@ static int clk_fetch_parent_index(struct clk_core *core, > return i; > } > > +static void clk_core_hold_state(struct clk_core *core) > +{ > + if (core->state_held || !core->boot_enabled || > + core->flags & CLK_DONT_HOLD_STATE) > + return; > + > + WARN(core->orphan, "%s: Can't hold state for orphan clk\n", core->name); > + > + core->state_held = !clk_core_prepare_enable(core); > +} > + > +static void __clk_core_update_orphan_hold_state(struct clk_core *core) > +{ > + struct clk_core *child; > + > + if (core->orphan) > + return; > + > + clk_core_hold_state(core); > + > + hlist_for_each_entry(child, &core->children, child_node) > + __clk_core_update_orphan_hold_state(child); > +} > + > /* > * Update the orphan status of @core and all its children. > */ > @@ -3374,6 +3430,8 @@ static int __clk_core_init(struct clk_core *core) > rate = 0; > core->rate = core->req_rate = rate; > > + core->boot_enabled = clk_core_is_enabled(core); > + > /* > * Enable CLK_IS_CRITICAL clocks so newly added critical clocks > * don't get accidentally disabled when walking the orphan tree and > @@ -3389,6 +3447,9 @@ static int __clk_core_init(struct clk_core *core) > clk_enable_unlock(flags); > } > > + if (!core->orphan) > + clk_core_hold_state(core); > + > /* > * walk the list of orphan clocks and reparent any that newly finds a > * parent. > @@ -3408,6 +3469,7 @@ static int __clk_core_init(struct clk_core *core) > __clk_set_parent_after(orphan, parent, NULL); > __clk_recalc_accuracies(orphan); > __clk_recalc_rates(orphan, 0); > + __clk_core_update_orphan_hold_state(orphan); > } > } > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 2fdfe8061363..f0e522ea793f 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -32,6 +32,7 @@ > #define CLK_OPS_PARENT_ENABLE BIT(12) > /* duty cycle call may be forwarded to the parent clock */ > #define CLK_DUTY_CYCLE_PARENT BIT(13) > +#define CLK_DONT_HOLD_STATE BIT(14) /* Don't hold state */ > > struct clk; > struct clk_hw; > -- > 2.24.0.432.g9d3f5f5b63-goog > Stephen, Nudge, nudge. Thoughts? -Saravana
Quoting Saravana Kannan (2019-11-18 15:42:28) > Clocks can 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. This has been a long standing issue with clk framework. Some clk providers work around it by marking clks like UART as special and keep them on whenever earlycon is present on the commandline. Mike tried to do something about this too[1] but that patchset only merged the critical clk part and not the handoff part. How does this compare with what Mike attempted many years ago? It may also be a good first step to merely detect that a clk is enabled at boot and to inform the framework that the clk is enabled and synchronize the state of the clk out of boot with the state that the framework is tracking. Essentially try to avoid the "when to turn it off" problem and fix the "what state is the clk in at boot" problem. We have clk providers that want to know this detail and currently workaround it by reading the state in their prepare/enable ops and bail out early, or read the enable state in their set_rate method and try to make the rate change go through. It's a mess. [1] https://lkml.kernel.org/r/1455225554-13267-1-git-send-email-mturquette@baylibre.com > > 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 potentially crash > the device or cause poor user experience. > > This patch fixes this by explicitly enabling the boot clocks during > clock registration and then disabling them at late_initcall_sync(). This > gives all the consumers until late_initcall() to put their "votes" in to > keep any of the boot clocks on past late_initcall(). How do we know some consumer won't "put in their votes" after late init? I suspect we should somehow scan the entire DT to determine what consumers exist and then match those consumers up with the clks that providers detect are on out of boot and then only remove the enable state of a clk once all consumers have gotten and enabled their clks. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- Please mention something about the new clk flag introduced and why it is introduced. Also talk about orphan clks and why they're special. > drivers/clk/clk.c | 62 ++++++++++++++++++++++++++++++++++++ > include/linux/clk-provider.h | 1 + > 2 files changed, 63 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 1c677d7f7f53..a1b09c9f8845 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1300,6 +1302,36 @@ static int clk_disable_unused(void) > } > late_initcall_sync(clk_disable_unused); > > +static void clk_unprepare_disable_subtree(struct clk_core *core) > +{ > + struct clk_core *child; > + > + lockdep_assert_held(&prepare_lock); > + > + hlist_for_each_entry(child, &core->children, child_node) > + clk_unprepare_disable_subtree(child); > + > + if (!core->state_held) > + return; > + > + clk_core_disable_unprepare(core); > +} > + > +static int clk_release_boot_state(void) > +{ > + struct clk_core *core; > + > + clk_prepare_lock(); > + > + hlist_for_each_entry(core, &clk_root_list, child_node) > + clk_unprepare_disable_subtree(core); > + > + clk_prepare_unlock(); > + > + return 0; > +} > +late_initcall_sync(clk_release_boot_state); > + > static int clk_core_determine_round_nolock(struct clk_core *core, > struct clk_rate_request *req) > { > @@ -1674,6 +1706,30 @@ static int clk_fetch_parent_index(struct clk_core *core, > return i; > } > > +static void clk_core_hold_state(struct clk_core *core) > +{ > + if (core->state_held || !core->boot_enabled || > + core->flags & CLK_DONT_HOLD_STATE) > + return; > + > + WARN(core->orphan, "%s: Can't hold state for orphan clk\n", core->name); Why are orphans special? > + > + core->state_held = !clk_core_prepare_enable(core); > +} > + > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 2fdfe8061363..f0e522ea793f 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -32,6 +32,7 @@ > #define CLK_OPS_PARENT_ENABLE BIT(12) > /* duty cycle call may be forwarded to the parent clock */ > #define CLK_DUTY_CYCLE_PARENT BIT(13) > +#define CLK_DONT_HOLD_STATE BIT(14) /* Don't hold state */ >
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 1c677d7f7f53..a1b09c9f8845 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -72,6 +72,8 @@ struct clk_core { unsigned long flags; bool orphan; bool rpm_enabled; + bool state_held; + bool boot_enabled; unsigned int enable_count; unsigned int prepare_count; unsigned int protect_count; @@ -1300,6 +1302,36 @@ static int clk_disable_unused(void) } late_initcall_sync(clk_disable_unused); +static void clk_unprepare_disable_subtree(struct clk_core *core) +{ + struct clk_core *child; + + lockdep_assert_held(&prepare_lock); + + hlist_for_each_entry(child, &core->children, child_node) + clk_unprepare_disable_subtree(child); + + if (!core->state_held) + return; + + clk_core_disable_unprepare(core); +} + +static int clk_release_boot_state(void) +{ + struct clk_core *core; + + clk_prepare_lock(); + + hlist_for_each_entry(core, &clk_root_list, child_node) + clk_unprepare_disable_subtree(core); + + clk_prepare_unlock(); + + return 0; +} +late_initcall_sync(clk_release_boot_state); + static int clk_core_determine_round_nolock(struct clk_core *core, struct clk_rate_request *req) { @@ -1674,6 +1706,30 @@ static int clk_fetch_parent_index(struct clk_core *core, return i; } +static void clk_core_hold_state(struct clk_core *core) +{ + if (core->state_held || !core->boot_enabled || + core->flags & CLK_DONT_HOLD_STATE) + return; + + WARN(core->orphan, "%s: Can't hold state for orphan clk\n", core->name); + + core->state_held = !clk_core_prepare_enable(core); +} + +static void __clk_core_update_orphan_hold_state(struct clk_core *core) +{ + struct clk_core *child; + + if (core->orphan) + return; + + clk_core_hold_state(core); + + hlist_for_each_entry(child, &core->children, child_node) + __clk_core_update_orphan_hold_state(child); +} + /* * Update the orphan status of @core and all its children. */ @@ -3374,6 +3430,8 @@ static int __clk_core_init(struct clk_core *core) rate = 0; core->rate = core->req_rate = rate; + core->boot_enabled = clk_core_is_enabled(core); + /* * Enable CLK_IS_CRITICAL clocks so newly added critical clocks * don't get accidentally disabled when walking the orphan tree and @@ -3389,6 +3447,9 @@ static int __clk_core_init(struct clk_core *core) clk_enable_unlock(flags); } + if (!core->orphan) + clk_core_hold_state(core); + /* * walk the list of orphan clocks and reparent any that newly finds a * parent. @@ -3408,6 +3469,7 @@ static int __clk_core_init(struct clk_core *core) __clk_set_parent_after(orphan, parent, NULL); __clk_recalc_accuracies(orphan); __clk_recalc_rates(orphan, 0); + __clk_core_update_orphan_hold_state(orphan); } } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 2fdfe8061363..f0e522ea793f 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -32,6 +32,7 @@ #define CLK_OPS_PARENT_ENABLE BIT(12) /* duty cycle call may be forwarded to the parent clock */ #define CLK_DUTY_CYCLE_PARENT BIT(13) +#define CLK_DONT_HOLD_STATE BIT(14) /* Don't hold state */ struct clk; struct clk_hw;
Clocks can 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 potentially crash the device or cause poor user experience. This patch fixes this by explicitly enabling the boot clocks during clock registration and then disabling them at late_initcall_sync(). This gives all the consumers until late_initcall() to put their "votes" in to keep any of the boot clocks on past late_initcall(). Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/clk/clk.c | 62 ++++++++++++++++++++++++++++++++++++ include/linux/clk-provider.h | 1 + 2 files changed, 63 insertions(+)