drivers/clk/clk.c | 67 ++++++++++++++++++++++++++++++++++++-- drivers/clk/meson/a1-peripherals.c | 12 +++---- drivers/clk/meson/axg.c | 16 +++++---- drivers/clk/meson/c3-peripherals.c | 6 ++-- drivers/clk/meson/g12a.c | 18 ++++++---- drivers/clk/meson/gxbb.c | 18 ++++++---- drivers/clk/meson/s4-peripherals.c | 32 +++++++++--------- 7 files changed, 122 insertions(+), 47 deletions(-)
If CLK_OPS_PARENT_ENABLE is configured for clock,
clk_core_disable_unprepare() is called in clk_disable_unused_subtree().
Even clocks that are configured with CLK_IGNORE_UNUSED are disabled,
resulting in the failure of CLK_IGNORE_UNUSED.
To ensure that amlogic glitch free mux can switch clock channels
properly, add flag CLK_OPS_PARENT_ENABLE to glitch free mux. The issue
that CLK_OPS_PARENT_ENABLE in CCF causes CLK_IGNORE_UNUSED to fail is
also exposed.
glitch free mux channel switchover failure issue(Test vpu_clk on S4):
step 1:
$ cat /sys/kernel/debug/clk/vpu/clk_parent
vpu_0
$ cat /sys/kernel/debug/clk/vpu_0/clk_rate
399999994
$ cat /sys/kernel/debug/clk/vpu_1/clk_rate
666666656
$ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable
$ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
399987500 +/-12500Hz
step 2:
$ echo 0 > /sys/kernel/debug/clk/vpu/clk_prepare_enable
$ echo 1 > /sys/kernel/debug/clk/vpu/clk_parent
$ cat /sys/kernel/debug/clk/vpu/clk_parent
vpu_1
$ cat /sys/kernel/debug/clk/vpu/clk_rate
666666656
$ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable
$ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
0 +/-3125Hz
In step2, vpu_0 is disabled, and the vpu is not switched to vpu_1. At
this time, the vpu is still connected to vpu_0 and vpu_0 is disabled at
this time, resulting in the clk-measure not measuring the clock.
Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
Chuan Liu (2):
clk: Fix the CLK_IGNORE_UNUSED failure issue
clk: meson: Fix glitch free mux related issues
drivers/clk/clk.c | 67 ++++++++++++++++++++++++++++++++++++--
drivers/clk/meson/a1-peripherals.c | 12 +++----
drivers/clk/meson/axg.c | 16 +++++----
drivers/clk/meson/c3-peripherals.c | 6 ++--
drivers/clk/meson/g12a.c | 18 ++++++----
drivers/clk/meson/gxbb.c | 18 ++++++----
drivers/clk/meson/s4-peripherals.c | 32 +++++++++---------
7 files changed, 122 insertions(+), 47 deletions(-)
---
base-commit: e736da1956cf0880a02ec5023f3487eea7611b5f
change-id: 20240929-fix_glitch_free-290c88923c31
Best regards,
--
Chuan Liu <chuan.liu@amlogic.com>
On Sun 29 Sep 2024 at 14:10, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: > If CLK_OPS_PARENT_ENABLE is configured for clock, > clk_core_disable_unprepare() is called in clk_disable_unused_subtree(). > Even clocks that are configured with CLK_IGNORE_UNUSED are disabled, > resulting in the failure of CLK_IGNORE_UNUSED. > > To ensure that amlogic glitch free mux can switch clock channels > properly, add flag CLK_OPS_PARENT_ENABLE to glitch free mux. The issue > that CLK_OPS_PARENT_ENABLE in CCF causes CLK_IGNORE_UNUSED to fail is > also exposed. The problem is that you are mixing problems together and it makes things rather difficult to follow. There are 2 distinct problem, there should have been to distinct patchset, even if you reference the CCF one in the Amlogic change. CLK_IGNORE_UNUSED is no guarantee that a clock will stay on, no matter what happens in the clock tree. I explained that to you several times, and it is the very reason why you are being asked to justify each usage of the flag. Most of the time, using it is wrong. That being said, there seems to be problems with CLK_OPS_PARENT_ENABLE in clk_disable_unused_subtree(). As it is, I think * a clock with CLK_IGNORE_UNUSED and CLK_OPS_PARENT_ENABLE, would 'leak' an enable, essentially making the parent subtree critical. * All parents of a CLK_OPS_PARENT_ENABLE clock would have its CLK_IGNORE_UNUSED ignored as a result of the enable/disable sequence (note that in any other circumstance, enable/disable should indeed disable an CLK_IGNORE_UNUSED clock). * Parent of ignored clocks may still get disabled. I'll be sending a proposal to address these problems soon. > > glitch free mux channel switchover failure issue(Test vpu_clk on S4): > step 1: > $ cat /sys/kernel/debug/clk/vpu/clk_parent > vpu_0 > $ cat /sys/kernel/debug/clk/vpu_0/clk_rate > 399999994 > $ cat /sys/kernel/debug/clk/vpu_1/clk_rate > 666666656 > $ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable > $ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk > 399987500 +/-12500Hz > > step 2: > $ echo 0 > /sys/kernel/debug/clk/vpu/clk_prepare_enable > $ echo 1 > /sys/kernel/debug/clk/vpu/clk_parent > $ cat /sys/kernel/debug/clk/vpu/clk_parent > vpu_1 > $ cat /sys/kernel/debug/clk/vpu/clk_rate > 666666656 > $ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable > $ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk > 0 +/-3125Hz > > In step2, vpu_0 is disabled, and the vpu is not switched to vpu_1. At > this time, the vpu is still connected to vpu_0 and vpu_0 is disabled at > this time, resulting in the clk-measure not measuring the clock. > If keep the display on and managed at all time is critical, then may you should consider having the module managing it as built-in. You would not need CLK_IGNORE_USED at all because device would be present before clk_disable_unused() is executed. Past the late init, if no device actively use the clock, disabling it is the sane thing to do because nothing says it will ever be used. > Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> > --- > Chuan Liu (2): > clk: Fix the CLK_IGNORE_UNUSED failure issue > clk: meson: Fix glitch free mux related issues > > drivers/clk/clk.c | 67 ++++++++++++++++++++++++++++++++++++-- > drivers/clk/meson/a1-peripherals.c | 12 +++---- > drivers/clk/meson/axg.c | 16 +++++---- > drivers/clk/meson/c3-peripherals.c | 6 ++-- > drivers/clk/meson/g12a.c | 18 ++++++---- > drivers/clk/meson/gxbb.c | 18 ++++++---- > drivers/clk/meson/s4-peripherals.c | 32 +++++++++--------- > 7 files changed, 122 insertions(+), 47 deletions(-) > --- > base-commit: e736da1956cf0880a02ec5023f3487eea7611b5f > change-id: 20240929-fix_glitch_free-290c88923c31 > > Best regards, -- Jerome
As it as been pointed out numerous times, flagging a clock with
CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
on. The clock will get disabled if any enable/disable cycle happens on it
or its parent clocks.
Because enable/disable cycles will disable unused clocks,
clk_disable_unused() should not trigger such cycle. Doing so disregard
the flag if set for any parent clocks. This is problematic with
CLK_OPS_PARENT_ENABLE handling.
To solve this, and a couple other issues, pass the parent status to the
child while walking the subtree, and return whether child ignored disable,
or not.
* Knowing the parent status allows to safely disable clocks with
CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
that, while the clock is not gated it is effectively disabled. Turning on
the parents to sanitize the sitation would bring back our initial
problem, so just let it sanitize itself when the clock gets used.
* If a clock is not actively used (enabled_count == 0), does not have
CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
child ignored the disable, it should ignore the disable too. Doing so
avoids disabling what is feading the children. Let the flag trickle down
the tree. This has the added benefit to transfer the information to the
unprepare path, so we don't unprepare the parent of a clock that ignored
a disable.
* An enabled clock must be prepared in CCF but we can't rely solely on
counts at clk_disable_unused() stage. Make sure an enabled clock is
considered prepared too, even if does not implement the related callback.
Also make sure only disabled clocks get unprepared.
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
This is sent as an RFC to continue the discussion started by Chuan.
It is not meant to be applied as it is.
drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
1 file changed, 60 insertions(+), 32 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d02451f951cf..41c4504a41f1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
}
}
- /*
- * This could be called with the enable lock held, or from atomic
- * context. If the parent isn't enabled already, we can't do
- * anything here. We can also assume this clock isn't enabled.
- */
- if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
- if (!clk_core_is_enabled(core->parent)) {
- ret = false;
- goto done;
- }
-
ret = core->ops->is_enabled(core->hw);
done:
if (core->rpm_enabled)
@@ -1454,22 +1443,39 @@ 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 bool __init clk_unprepare_unused_subtree(struct clk_core *core,
+ bool parent_prepared)
{
struct clk_core *child;
+ bool prepared;
lockdep_assert_held(&prepare_lock);
+ /*
+ * Relying on count is not possible at this stage, so consider
+ * prepared an enabled clock, in case only .is_enabled() is
+ * implemented
+ */
+ if (parent_prepared)
+ prepared = (clk_core_is_prepared(core) ||
+ clk_core_is_enabled(core));
+ else
+ prepared = false;
+
hlist_for_each_entry(child, &core->children, child_node)
- clk_unprepare_unused_subtree(child);
+ if (clk_unprepare_unused_subtree(child, prepared) &&
+ prepared && !core->prepare_count)
+ core->flags |= CLK_IGNORE_UNUSED;
- if (core->prepare_count)
- return;
+ if (core->flags & CLK_IGNORE_UNUSED || core->prepare_count)
+ goto out;
- if (core->flags & CLK_IGNORE_UNUSED)
- return;
+ if (!parent_prepared && (core->flags & CLK_OPS_PARENT_ENABLE))
+ goto out;
- if (clk_core_is_prepared(core)) {
+ /* Do not unprepare an enabled clock */
+ if (clk_core_is_prepared(core) &&
+ !clk_core_is_enabled(core)) {
trace_clk_unprepare(core);
if (core->ops->unprepare_unused)
core->ops->unprepare_unused(core->hw);
@@ -1477,27 +1483,50 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
core->ops->unprepare(core->hw);
trace_clk_unprepare_complete(core);
}
+
+out:
+ return (core->flags & CLK_IGNORE_UNUSED) && prepared;
}
-static void __init clk_disable_unused_subtree(struct clk_core *core)
+static bool __init clk_disable_unused_subtree(struct clk_core *core,
+ bool parent_enabled)
{
struct clk_core *child;
unsigned long flags;
+ bool enabled;
lockdep_assert_held(&prepare_lock);
- hlist_for_each_entry(child, &core->children, child_node)
- clk_disable_unused_subtree(child);
+ flags = clk_enable_lock();
- if (core->flags & CLK_OPS_PARENT_ENABLE)
- clk_core_prepare_enable(core->parent);
+ /* Check if the clock is enabled from root to this clock */
+ if (parent_enabled)
+ enabled = clk_core_is_enabled(core);
+ else
+ enabled = false;
- flags = clk_enable_lock();
+ hlist_for_each_entry(child, &core->children, child_node)
+ /*
+ * If any child ignored disable, this clock should too,
+ * unless there is, valid reason for the clock to be enabled
+ */
+ if (clk_disable_unused_subtree(child, enabled) &&
+ enabled && !core->enable_count)
+ core->flags |= CLK_IGNORE_UNUSED;
- if (core->enable_count)
+ if (core->flags & CLK_IGNORE_UNUSED || core->enable_count)
goto unlock_out;
- if (core->flags & CLK_IGNORE_UNUSED)
+ /*
+ * If the parent is disabled but the gate is open, we should sanitize
+ * the situation. This will avoid an unexpected enable of the clock as
+ * soon as the parent is enabled, without control of CCF.
+ *
+ * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
+ * forcefully enabling a whole part of the subtree. Just let the
+ * situation resolve it self on the first enable of the clock
+ */
+ if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
goto unlock_out;
/*
@@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
unlock_out:
clk_enable_unlock(flags);
- if (core->flags & CLK_OPS_PARENT_ENABLE)
- clk_core_disable_unprepare(core->parent);
+ return (core->flags & CLK_IGNORE_UNUSED) && enabled;
}
static bool clk_ignore_unused __initdata;
@@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
clk_prepare_lock();
hlist_for_each_entry(core, &clk_root_list, child_node)
- clk_disable_unused_subtree(core);
+ clk_disable_unused_subtree(core, true);
hlist_for_each_entry(core, &clk_orphan_list, child_node)
- clk_disable_unused_subtree(core);
+ clk_disable_unused_subtree(core, true);
hlist_for_each_entry(core, &clk_root_list, child_node)
- clk_unprepare_unused_subtree(core);
+ clk_unprepare_unused_subtree(core, true);
hlist_for_each_entry(core, &clk_orphan_list, child_node)
- clk_unprepare_unused_subtree(core);
+ clk_unprepare_unused_subtree(core, true);
clk_prepare_unlock();
--
2.45.2hi Jerome:
Tranks for your REF. I looked at your patch and there are some
parts that I don't quite understand: The original intention of
CLK_OPS_PARENT_ENABLE was to solve the issue of "parents need enable
_during _gate/ungate, set rate and re-parent" when setting a clock. After setting
the clock, it can still be disabled. However, from what I see in your
patch, the handling logic seems more like "parents need _always _ gate
during clock gate period"?
On 10/4/2024 9:39 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> As it as been pointed out numerous times, flagging a clock with
> CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
> on. The clock will get disabled if any enable/disable cycle happens on it
> or its parent clocks.
>
> Because enable/disable cycles will disable unused clocks,
> clk_disable_unused() should not trigger such cycle. Doing so disregard
> the flag if set for any parent clocks. This is problematic with
> CLK_OPS_PARENT_ENABLE handling.
>
> To solve this, and a couple other issues, pass the parent status to the
> child while walking the subtree, and return whether child ignored disable,
> or not.
>
> * Knowing the parent status allows to safely disable clocks with
> CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
> that, while the clock is not gated it is effectively disabled. Turning on
> the parents to sanitize the sitation would bring back our initial
> problem, so just let it sanitize itself when the clock gets used.
>
> * If a clock is not actively used (enabled_count == 0), does not have
> CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
> child ignored the disable, it should ignore the disable too. Doing so
> avoids disabling what is feading the children. Let the flag trickle down
> the tree. This has the added benefit to transfer the information to the
> unprepare path, so we don't unprepare the parent of a clock that ignored
> a disable.
>
> * An enabled clock must be prepared in CCF but we can't rely solely on
> counts at clk_disable_unused() stage. Make sure an enabled clock is
> considered prepared too, even if does not implement the related callback.
> Also make sure only disabled clocks get unprepared.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>
> This is sent as an RFC to continue the discussion started by Chuan.
> It is not meant to be applied as it is.
>
>
> drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
> 1 file changed, 60 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d02451f951cf..41c4504a41f1 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
> }
> }
>
> - /*
> - * This could be called with the enable lock held, or from atomic
> - * context. If the parent isn't enabled already, we can't do
> - * anything here. We can also assume this clock isn't enabled.
> - */
> - if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
This judgment of CLK_OPS_PARENT_ENABLE seems redundant. According to
normal logic, if the parent is disabled, its children will also be
forced to disable. This seems unrelated to whether CLK_OPS_PARENT_ENABLE
is configured.😳
> - if (!clk_core_is_enabled(core->parent)) {
> - ret = false;
> - goto done;
> - }
> -
> ret = core->ops->is_enabled(core->hw);
> done:
> if (core->rpm_enabled)
> @@ -1454,22 +1443,39 @@ 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 bool __init clk_unprepare_unused_subtree(struct clk_core *core,
> + bool parent_prepared)
> {
> struct clk_core *child;
> + bool prepared;
>
> lockdep_assert_held(&prepare_lock);
>
> + /*
> + * Relying on count is not possible at this stage, so consider
> + * prepared an enabled clock, in case only .is_enabled() is
> + * implemented
> + */
> + if (parent_prepared)
> + prepared = (clk_core_is_prepared(core) ||
> + clk_core_is_enabled(core));
> + else
> + prepared = false;
> +
> hlist_for_each_entry(child, &core->children, child_node)
> - clk_unprepare_unused_subtree(child);
> + if (clk_unprepare_unused_subtree(child, prepared) &&
> + prepared && !core->prepare_count)
> + core->flags |= CLK_IGNORE_UNUSED;
>
> - if (core->prepare_count)
> - return;
> + if (core->flags & CLK_IGNORE_UNUSED || core->prepare_count)
> + goto out;
>
> - if (core->flags & CLK_IGNORE_UNUSED)
> - return;
> + if (!parent_prepared && (core->flags & CLK_OPS_PARENT_ENABLE))
> + goto out;
>
> - if (clk_core_is_prepared(core)) {
> + /* Do not unprepare an enabled clock */
> + if (clk_core_is_prepared(core) &&
> + !clk_core_is_enabled(core)) {
> trace_clk_unprepare(core);
> if (core->ops->unprepare_unused)
> core->ops->unprepare_unused(core->hw);
> @@ -1477,27 +1483,50 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
> core->ops->unprepare(core->hw);
> trace_clk_unprepare_complete(core);
> }
> +
> +out:
> + return (core->flags & CLK_IGNORE_UNUSED) && prepared;
> }
>
> -static void __init clk_disable_unused_subtree(struct clk_core *core)
> +static bool __init clk_disable_unused_subtree(struct clk_core *core,
> + bool parent_enabled)
> {
> struct clk_core *child;
> unsigned long flags;
> + bool enabled;
>
> lockdep_assert_held(&prepare_lock);
>
> - hlist_for_each_entry(child, &core->children, child_node)
> - clk_disable_unused_subtree(child);
> + flags = clk_enable_lock();
>
> - if (core->flags & CLK_OPS_PARENT_ENABLE)
> - clk_core_prepare_enable(core->parent);
> + /* Check if the clock is enabled from root to this clock */
> + if (parent_enabled)
> + enabled = clk_core_is_enabled(core);
> + else
> + enabled = false;
>
> - flags = clk_enable_lock();
> + hlist_for_each_entry(child, &core->children, child_node)
> + /*
> + * If any child ignored disable, this clock should too,
> + * unless there is, valid reason for the clock to be enabled
> + */
> + if (clk_disable_unused_subtree(child, enabled) &&
> + enabled && !core->enable_count)
> + core->flags |= CLK_IGNORE_UNUSED;
>
> - if (core->enable_count)
> + if (core->flags & CLK_IGNORE_UNUSED || core->enable_count)
> goto unlock_out;
>
> - if (core->flags & CLK_IGNORE_UNUSED)
> + /*
> + * If the parent is disabled but the gate is open, we should sanitize
> + * the situation. This will avoid an unexpected enable of the clock as
> + * soon as the parent is enabled, without control of CCF.
> + *
> + * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
> + * forcefully enabling a whole part of the subtree. Just let the
> + * situation resolve it self on the first enable of the clock
> + */
> + if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
> goto unlock_out;
>
> /*
> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>
> unlock_out:
> clk_enable_unlock(flags);
> - if (core->flags & CLK_OPS_PARENT_ENABLE)
> - clk_core_disable_unprepare(core->parent);
> + return (core->flags & CLK_IGNORE_UNUSED) && enabled;
> }
>
> static bool clk_ignore_unused __initdata;
> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
> clk_prepare_lock();
>
> hlist_for_each_entry(core, &clk_root_list, child_node)
> - clk_disable_unused_subtree(core);
> + clk_disable_unused_subtree(core, true);
>
> hlist_for_each_entry(core, &clk_orphan_list, child_node)
> - clk_disable_unused_subtree(core);
> + clk_disable_unused_subtree(core, true);
>
> hlist_for_each_entry(core, &clk_root_list, child_node)
> - clk_unprepare_unused_subtree(core);
> + clk_unprepare_unused_subtree(core, true);
>
> hlist_for_each_entry(core, &clk_orphan_list, child_node)
> - clk_unprepare_unused_subtree(core);
> + clk_unprepare_unused_subtree(core, true);
>
> clk_prepare_unlock();
>
> --
> 2.45.2
>
On Fri 08 Nov 2024 at 15:59, Chuan Liu <chuan.liu@amlogic.com> wrote:
> hi Jerome:
>
> Tranks for your REF. I looked at your patch and there are some parts
> that I don't quite understand: The original intention of
> CLK_OPS_PARENT_ENABLE was to solve the issue of "parents need enable
> _during _gate/ungate, set rate and re-parent" when setting a clock. After
> setting the clock, it can still be disabled. However, from what I see in
> your patch, the handling logic seems more like "parents need _always _ gate
> during clock gate period"?
As explained in the description, the problem with CLK_IGNORE_UNUSED and
CLK_OPS_PARENT_ENABLE is that you'll get cycle of enable/disable, which
will disable any parent clock that may have a been enabled and expected
to be ignored.
IOW, the CCF changes the state of the tree while inspecting it.
This change solves that.
>
> On 10/4/2024 9:39 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> As it as been pointed out numerous times, flagging a clock with
>> CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
>> on. The clock will get disabled if any enable/disable cycle happens on it
>> or its parent clocks.
>>
>> Because enable/disable cycles will disable unused clocks,
>> clk_disable_unused() should not trigger such cycle. Doing so disregard
>> the flag if set for any parent clocks. This is problematic with
>> CLK_OPS_PARENT_ENABLE handling.
>>
>> To solve this, and a couple other issues, pass the parent status to the
>> child while walking the subtree, and return whether child ignored disable,
>> or not.
>>
>> * Knowing the parent status allows to safely disable clocks with
>> CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
>> that, while the clock is not gated it is effectively disabled. Turning on
>> the parents to sanitize the sitation would bring back our initial
>> problem, so just let it sanitize itself when the clock gets used.
>>
>> * If a clock is not actively used (enabled_count == 0), does not have
>> CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
>> child ignored the disable, it should ignore the disable too. Doing so
>> avoids disabling what is feading the children. Let the flag trickle down
>> the tree. This has the added benefit to transfer the information to the
>> unprepare path, so we don't unprepare the parent of a clock that ignored
>> a disable.
>>
>> * An enabled clock must be prepared in CCF but we can't rely solely on
>> counts at clk_disable_unused() stage. Make sure an enabled clock is
>> considered prepared too, even if does not implement the related callback.
>> Also make sure only disabled clocks get unprepared.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>
>> This is sent as an RFC to continue the discussion started by Chuan.
>> It is not meant to be applied as it is.
>>
>>
>> drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 60 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index d02451f951cf..41c4504a41f1 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
>> }
>> }
>>
>> - /*
>> - * This could be called with the enable lock held, or from atomic
>> - * context. If the parent isn't enabled already, we can't do
>> - * anything here. We can also assume this clock isn't enabled.
>> - */
>> - if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
>
> This judgment of CLK_OPS_PARENT_ENABLE seems redundant. According to
> normal logic, if the parent is disabled, its children will also be
> forced to disable. This seems unrelated to whether CLK_OPS_PARENT_ENABLE
> is configured.😳
It's removed.
>
>> - if (!clk_core_is_enabled(core->parent)) {
>> - ret = false;
>> - goto done;
>> - }
>> -
>> ret = core->ops->is_enabled(core->hw);
>> done:
>> if (core->rpm_enabled)
>> @@ -1454,22 +1443,39 @@ 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 bool __init clk_unprepare_unused_subtree(struct clk_core *core,
>> + bool parent_prepared)
>> {
>> struct clk_core *child;
>> + bool prepared;
>>
>> lockdep_assert_held(&prepare_lock);
>>
>> + /*
>> + * Relying on count is not possible at this stage, so consider
>> + * prepared an enabled clock, in case only .is_enabled() is
>> + * implemented
>> + */
>> + if (parent_prepared)
>> + prepared = (clk_core_is_prepared(core) ||
>> + clk_core_is_enabled(core));
>> + else
>> + prepared = false;
>> +
>> hlist_for_each_entry(child, &core->children, child_node)
>> - clk_unprepare_unused_subtree(child);
>> + if (clk_unprepare_unused_subtree(child, prepared) &&
>> + prepared && !core->prepare_count)
>> + core->flags |= CLK_IGNORE_UNUSED;
>>
>> - if (core->prepare_count)
>> - return;
>> + if (core->flags & CLK_IGNORE_UNUSED || core->prepare_count)
>> + goto out;
>>
>> - if (core->flags & CLK_IGNORE_UNUSED)
>> - return;
>> + if (!parent_prepared && (core->flags & CLK_OPS_PARENT_ENABLE))
>> + goto out;
>>
>> - if (clk_core_is_prepared(core)) {
>> + /* Do not unprepare an enabled clock */
>> + if (clk_core_is_prepared(core) &&
>> + !clk_core_is_enabled(core)) {
>> trace_clk_unprepare(core);
>> if (core->ops->unprepare_unused)
>> core->ops->unprepare_unused(core->hw);
>> @@ -1477,27 +1483,50 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>> core->ops->unprepare(core->hw);
>> trace_clk_unprepare_complete(core);
>> }
>> +
>> +out:
>> + return (core->flags & CLK_IGNORE_UNUSED) && prepared;
>> }
>>
>> -static void __init clk_disable_unused_subtree(struct clk_core *core)
>> +static bool __init clk_disable_unused_subtree(struct clk_core *core,
>> + bool parent_enabled)
>> {
>> struct clk_core *child;
>> unsigned long flags;
>> + bool enabled;
>>
>> lockdep_assert_held(&prepare_lock);
>>
>> - hlist_for_each_entry(child, &core->children, child_node)
>> - clk_disable_unused_subtree(child);
>> + flags = clk_enable_lock();
>>
>> - if (core->flags & CLK_OPS_PARENT_ENABLE)
>> - clk_core_prepare_enable(core->parent);
>> + /* Check if the clock is enabled from root to this clock */
>> + if (parent_enabled)
>> + enabled = clk_core_is_enabled(core);
>> + else
>> + enabled = false;
>>
>> - flags = clk_enable_lock();
>> + hlist_for_each_entry(child, &core->children, child_node)
>> + /*
>> + * If any child ignored disable, this clock should too,
>> + * unless there is, valid reason for the clock to be enabled
>> + */
>> + if (clk_disable_unused_subtree(child, enabled) &&
>> + enabled && !core->enable_count)
>> + core->flags |= CLK_IGNORE_UNUSED;
>>
>> - if (core->enable_count)
>> + if (core->flags & CLK_IGNORE_UNUSED || core->enable_count)
>> goto unlock_out;
>>
>> - if (core->flags & CLK_IGNORE_UNUSED)
>> + /*
>> + * If the parent is disabled but the gate is open, we should sanitize
>> + * the situation. This will avoid an unexpected enable of the clock as
>> + * soon as the parent is enabled, without control of CCF.
>> + *
>> + * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>> + * forcefully enabling a whole part of the subtree. Just let the
>> + * situation resolve it self on the first enable of the clock
>> + */
>> + if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>> goto unlock_out;
>>
>> /*
>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>
>> unlock_out:
>> clk_enable_unlock(flags);
>> - if (core->flags & CLK_OPS_PARENT_ENABLE)
>> - clk_core_disable_unprepare(core->parent);
>> + return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>> }
>>
>> static bool clk_ignore_unused __initdata;
>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>> clk_prepare_lock();
>>
>> hlist_for_each_entry(core, &clk_root_list, child_node)
>> - clk_disable_unused_subtree(core);
>> + clk_disable_unused_subtree(core, true);
>>
>> hlist_for_each_entry(core, &clk_orphan_list, child_node)
>> - clk_disable_unused_subtree(core);
>> + clk_disable_unused_subtree(core, true);
>>
>> hlist_for_each_entry(core, &clk_root_list, child_node)
>> - clk_unprepare_unused_subtree(core);
>> + clk_unprepare_unused_subtree(core, true);
>>
>> hlist_for_each_entry(core, &clk_orphan_list, child_node)
>> - clk_unprepare_unused_subtree(core);
>> + clk_unprepare_unused_subtree(core, true);
>>
>> clk_prepare_unlock();
>>
>> --
>> 2.45.2
>>
--
Jerome
On 11/8/2024 4:38 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 08 Nov 2024 at 15:59, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>> hi Jerome:
>>
>> Tranks for your REF. I looked at your patch and there are some parts
>> that I don't quite understand: The original intention of
>> CLK_OPS_PARENT_ENABLE was to solve the issue of "parents need enable
>> _during _gate/ungate, set rate and re-parent" when setting a clock. After
>> setting the clock, it can still be disabled. However, from what I see in
>> your patch, the handling logic seems more like "parents need _always _ gate
>> during clock gate period"?
> As explained in the description, the problem with CLK_IGNORE_UNUSED and
> CLK_OPS_PARENT_ENABLE is that you'll get cycle of enable/disable, which
> will disable any parent clock that may have a been enabled and expected
> to be ignored.
>
> IOW, the CCF changes the state of the tree while inspecting it.
> This change solves that.
Ok, I understand your idea now... I have gotten myself tangled up.
>> On 10/4/2024 9:39 PM, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> As it as been pointed out numerous times, flagging a clock with
>>> CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
>>> on. The clock will get disabled if any enable/disable cycle happens on it
>>> or its parent clocks.
>>>
>>> Because enable/disable cycles will disable unused clocks,
>>> clk_disable_unused() should not trigger such cycle. Doing so disregard
>>> the flag if set for any parent clocks. This is problematic with
>>> CLK_OPS_PARENT_ENABLE handling.
>>>
>>> To solve this, and a couple other issues, pass the parent status to the
>>> child while walking the subtree, and return whether child ignored disable,
>>> or not.
>>>
>>> * Knowing the parent status allows to safely disable clocks with
>>> CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
>>> that, while the clock is not gated it is effectively disabled. Turning on
>>> the parents to sanitize the sitation would bring back our initial
>>> problem, so just let it sanitize itself when the clock gets used.
>>>
>>> * If a clock is not actively used (enabled_count == 0), does not have
>>> CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
>>> child ignored the disable, it should ignore the disable too. Doing so
>>> avoids disabling what is feading the children. Let the flag trickle down
>>> the tree. This has the added benefit to transfer the information to the
>>> unprepare path, so we don't unprepare the parent of a clock that ignored
>>> a disable.
>>>
>>> * An enabled clock must be prepared in CCF but we can't rely solely on
>>> counts at clk_disable_unused() stage. Make sure an enabled clock is
>>> considered prepared too, even if does not implement the related callback.
>>> Also make sure only disabled clocks get unprepared.
>>>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>
>>> This is sent as an RFC to continue the discussion started by Chuan.
>>> It is not meant to be applied as it is.
>>>
>>>
>>> drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
>>> 1 file changed, 60 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index d02451f951cf..41c4504a41f1 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
>>> }
>>> }
>>>
>>> - /*
>>> - * This could be called with the enable lock held, or from atomic
>>> - * context. If the parent isn't enabled already, we can't do
>>> - * anything here. We can also assume this clock isn't enabled.
>>> - */
>>> - if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
>> This judgment of CLK_OPS_PARENT_ENABLE seems redundant. According to
>> normal logic, if the parent is disabled, its children will also be
>> forced to disable. This seems unrelated to whether CLK_OPS_PARENT_ENABLE
>> is configured.😳
> It's removed.
Yes, I just want to express that judging CLK_OPS_PARENT_ENABLE seems unnecessary here.
>
>>> - if (!clk_core_is_enabled(core->parent)) {
>>> - ret = false;
>>> - goto done;
>>> - }
>>> -
>>> ret = core->ops->is_enabled(core->hw);
>>> done:
>>> if (core->rpm_enabled)
>>> @@ -1454,22 +1443,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
>>> clk_core_unprepare_lock(core);
>>> }
[ . . . ]
>>> - if (core->flags & CLK_IGNORE_UNUSED)
>>> + /*
>>> + * If the parent is disabled but the gate is open, we should sanitize
>>> + * the situation. This will avoid an unexpected enable of the clock as
>>> + * soon as the parent is enabled, without control of CCF.
>>> + *
>>> + * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>> + * forcefully enabling a whole part of the subtree. Just let the
>>> + * situation resolve it self on the first enable of the clock
>>> + */
>>> + if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
At first, I couldn't grasp the logic behind the 'return' here. Now it's
clear. This approach is equivalent to completely giving up on
handling clocks with CLK_OPS_PARENT_ENABLE feature in
clk_disable_unused_subtree().
>>> goto unlock_out;
>>>
>>> /*
>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>
>>> unlock_out:
>>> clk_enable_unlock(flags);
>>> - if (core->flags & CLK_OPS_PARENT_ENABLE)
>>> - clk_core_disable_unprepare(core->parent);
>>> + return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>> }
>>>
>>> static bool clk_ignore_unused __initdata;
>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>> clk_prepare_lock();
>>>
>>> hlist_for_each_entry(core, &clk_root_list, child_node)
>>> - clk_disable_unused_subtree(core);
>>> + clk_disable_unused_subtree(core, true);
>>>
>>> hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>> - clk_disable_unused_subtree(core);
>>> + clk_disable_unused_subtree(core, true);
>>>
>>> hlist_for_each_entry(core, &clk_root_list, child_node)
>>> - clk_unprepare_unused_subtree(core);
>>> + clk_unprepare_unused_subtree(core, true);
>>>
>>> hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>> - clk_unprepare_unused_subtree(core);
>>> + clk_unprepare_unused_subtree(core, true);
>>>
>>> clk_prepare_unlock();
>>>
>>> --
>>> 2.45.2
>>>
> --
> Jerome
On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote: >>>> - if (core->flags & CLK_IGNORE_UNUSED) >>>> + /* >>>> + * If the parent is disabled but the gate is open, we should sanitize >>>> + * the situation. This will avoid an unexpected enable of the clock as >>>> + * soon as the parent is enabled, without control of CCF. >>>> + * >>>> + * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without >>>> + * forcefully enabling a whole part of the subtree. Just let the >>>> + * situation resolve it self on the first enable of the clock >>>> + */ >>>> + if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE)) > > At first, I couldn't grasp the logic behind the 'return' here. Now it's > clear. This approach is equivalent to completely giving up on > handling clocks with CLK_OPS_PARENT_ENABLE feature in > clk_disable_unused_subtree(). > No. It's handled correctly as long as the tree is in coherent state. What is not done anymore is fixing up an inconsistent tree, by this I mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its own registers but has its parent disabled. In that particular case, clk_disable_unused_subtree() won't be turning on everything to properly disable that one clock. That is the root cause of the problem you reported initially. The clock is disabled anyway. Every other case are properly handled (at least I think). >>>> goto unlock_out; >>>> >>>> /* >>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core) >>>> >>>> unlock_out: >>>> clk_enable_unlock(flags); >>>> - if (core->flags & CLK_OPS_PARENT_ENABLE) >>>> - clk_core_disable_unprepare(core->parent); >>>> + return (core->flags & CLK_IGNORE_UNUSED) && enabled; >>>> } >>>> >>>> static bool clk_ignore_unused __initdata; >>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void) >>>> clk_prepare_lock(); >>>> >>>> hlist_for_each_entry(core, &clk_root_list, child_node) >>>> - clk_disable_unused_subtree(core); >>>> + clk_disable_unused_subtree(core, true); >>>> >>>> hlist_for_each_entry(core, &clk_orphan_list, child_node) >>>> - clk_disable_unused_subtree(core); >>>> + clk_disable_unused_subtree(core, true); >>>> >>>> hlist_for_each_entry(core, &clk_root_list, child_node) >>>> - clk_unprepare_unused_subtree(core); >>>> + clk_unprepare_unused_subtree(core, true); >>>> >>>> hlist_for_each_entry(core, &clk_orphan_list, child_node) >>>> - clk_unprepare_unused_subtree(core); >>>> + clk_unprepare_unused_subtree(core, true); >>>> >>>> clk_prepare_unlock(); >>>> >>>> -- >>>> 2.45.2 >>>> >> -- >> Jerome -- Jerome
On 11/8/2024 5:59 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>>>>> - if (core->flags & CLK_IGNORE_UNUSED)
>>>>> + /*
>>>>> + * If the parent is disabled but the gate is open, we should sanitize
>>>>> + * the situation. This will avoid an unexpected enable of the clock as
>>>>> + * soon as the parent is enabled, without control of CCF.
>>>>> + *
>>>>> + * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>>>> + * forcefully enabling a whole part of the subtree. Just let the
>>>>> + * situation resolve it self on the first enable of the clock
>>>>> + */
>>>>> + if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>> At first, I couldn't grasp the logic behind the 'return' here. Now it's
>> clear. This approach is equivalent to completely giving up on
>> handling clocks with CLK_OPS_PARENT_ENABLE feature in
>> clk_disable_unused_subtree().
>>
> No. It's handled correctly as long as the tree is in coherent state.
>
> What is not done anymore is fixing up an inconsistent tree, by this I
> mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its
> own registers but has its parent disabled.
>
> In that particular case, clk_disable_unused_subtree() won't be turning on
> everything to properly disable that one clock. That is the root cause of
> the problem you reported initially. The clock is disabled anyway.
>
> Every other case are properly handled (at least I think).
name en_sts flags
clk_a 1 CLK_IGNORE_UNUSED
clk_b 0 0
clk_c 1 CLK_OPS_PARENT_ENABLE
Based on the above case:
1. When 'clk_c' is configured with CLK_OPS_PARENT_ENABLE, disabling
'clk_c' requires enabling 'clk_b' first (disabling 'clk_c' before
disabling 'clk_b'). How can to ensure that during the period of
disabling 'clk_c', 'clk_b' remains enabled?
2. 'clk_c' is not configured with CLK_IGNORE_UNUSED, it should be
disabled later. However, here it goes to a 'goto' statement and then
return 'false', ultimately resulting in 'clk_c' not being disabled?
>>>>> goto unlock_out;
>>>>>
>>>>> /*
>>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>>>
>>>>> unlock_out:
>>>>> clk_enable_unlock(flags);
>>>>> - if (core->flags & CLK_OPS_PARENT_ENABLE)
>>>>> - clk_core_disable_unprepare(core->parent);
>>>>> + return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>>>> }
>>>>>
>>>>> static bool clk_ignore_unused __initdata;
>>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>>>> clk_prepare_lock();
>>>>>
>>>>> hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>> - clk_disable_unused_subtree(core);
>>>>> + clk_disable_unused_subtree(core, true);
>>>>>
>>>>> hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>> - clk_disable_unused_subtree(core);
>>>>> + clk_disable_unused_subtree(core, true);
>>>>>
>>>>> hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>> - clk_unprepare_unused_subtree(core);
>>>>> + clk_unprepare_unused_subtree(core, true);
>>>>>
>>>>> hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>> - clk_unprepare_unused_subtree(core);
>>>>> + clk_unprepare_unused_subtree(core, true);
>>>>>
>>>>> clk_prepare_unlock();
>>>>>
>>>>> --
>>>>> 2.45.2
>>>>>
>>> --
>>> Jerome
> --
> Jerome
On Fri 08 Nov 2024 at 19:49, Chuan Liu <chuan.liu@amlogic.com> wrote: > On 11/8/2024 5:59 PM, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote: >> >>>>>> - if (core->flags & CLK_IGNORE_UNUSED) >>>>>> + /* >>>>>> + * If the parent is disabled but the gate is open, we should sanitize >>>>>> + * the situation. This will avoid an unexpected enable of the clock as >>>>>> + * soon as the parent is enabled, without control of CCF. >>>>>> + * >>>>>> + * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without >>>>>> + * forcefully enabling a whole part of the subtree. Just let the >>>>>> + * situation resolve it self on the first enable of the clock >>>>>> + */ >>>>>> + if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE)) >>> At first, I couldn't grasp the logic behind the 'return' here. Now it's >>> clear. This approach is equivalent to completely giving up on >>> handling clocks with CLK_OPS_PARENT_ENABLE feature in >>> clk_disable_unused_subtree(). >>> >> No. It's handled correctly as long as the tree is in coherent state. >> >> What is not done anymore is fixing up an inconsistent tree, by this I >> mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its >> own registers but has its parent disabled. >> >> In that particular case, clk_disable_unused_subtree() won't be turning on >> everything to properly disable that one clock. That is the root cause of >> the problem you reported initially. The clock is disabled anyway. >> >> Every other case are properly handled (at least I think). > > name en_sts flags > clk_a 1 CLK_IGNORE_UNUSED > clk_b 0 0 > clk_c 1 CLK_OPS_PARENT_ENABLE > > Based on the above case: > 1. When 'clk_c' is configured with CLK_OPS_PARENT_ENABLE, disabling > 'clk_c' requires enabling 'clk_b' first (disabling 'clk_c' before > disabling 'clk_b'). How can to ensure that during the period of > disabling 'clk_c', 'clk_b' remains enabled? That's perfect example of incoherent state. How can 'clk_c' be enabled if its parent is disable. That makes no sense, so there is no point enabling a whole subtree for this IMO. > > 2. 'clk_c' is not configured with CLK_IGNORE_UNUSED, it should be > disabled later. However, here it goes to a 'goto' statement and then > return 'false', ultimately resulting in 'clk_c' not being disabled? We've discussed that 2 times already. This discussion is going in circles now. > >>>>>> goto unlock_out; >>>>>> >>>>>> /* >>>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core) >>>>>> >>>>>> unlock_out: >>>>>> clk_enable_unlock(flags); >>>>>> - if (core->flags & CLK_OPS_PARENT_ENABLE) >>>>>> - clk_core_disable_unprepare(core->parent); >>>>>> + return (core->flags & CLK_IGNORE_UNUSED) && enabled; >>>>>> } >>>>>> >>>>>> static bool clk_ignore_unused __initdata; >>>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void) >>>>>> clk_prepare_lock(); >>>>>> >>>>>> hlist_for_each_entry(core, &clk_root_list, child_node) >>>>>> - clk_disable_unused_subtree(core); >>>>>> + clk_disable_unused_subtree(core, true); >>>>>> >>>>>> hlist_for_each_entry(core, &clk_orphan_list, child_node) >>>>>> - clk_disable_unused_subtree(core); >>>>>> + clk_disable_unused_subtree(core, true); >>>>>> >>>>>> hlist_for_each_entry(core, &clk_root_list, child_node) >>>>>> - clk_unprepare_unused_subtree(core); >>>>>> + clk_unprepare_unused_subtree(core, true); >>>>>> >>>>>> hlist_for_each_entry(core, &clk_orphan_list, child_node) >>>>>> - clk_unprepare_unused_subtree(core); >>>>>> + clk_unprepare_unused_subtree(core, true); >>>>>> >>>>>> clk_prepare_unlock(); >>>>>> >>>>>> -- >>>>>> 2.45.2 >>>>>> >>>> -- >>>> Jerome >> -- >> Jerome -- Jerome
On 11/12/2024 4:36 PM, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On Fri 08 Nov 2024 at 19:49, Chuan Liu <chuan.liu@amlogic.com> wrote: > >> On 11/8/2024 5:59 PM, Jerome Brunet wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote: >>> >>>>>>> - if (core->flags & CLK_IGNORE_UNUSED) >>>>>>> + /* >>>>>>> + * If the parent is disabled but the gate is open, we should sanitize >>>>>>> + * the situation. This will avoid an unexpected enable of the clock as >>>>>>> + * soon as the parent is enabled, without control of CCF. >>>>>>> + * >>>>>>> + * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without >>>>>>> + * forcefully enabling a whole part of the subtree. Just let the >>>>>>> + * situation resolve it self on the first enable of the clock >>>>>>> + */ >>>>>>> + if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE)) >>>> At first, I couldn't grasp the logic behind the 'return' here. Now it's >>>> clear. This approach is equivalent to completely giving up on >>>> handling clocks with CLK_OPS_PARENT_ENABLE feature in >>>> clk_disable_unused_subtree(). Referring to the situation of 'clk_c' below, combined with your previous explanation: * Knowing the parent status allows to safely disable clocks with CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means that, while the clock is not gated it is effectively disabled. Turning on the parents to sanitize the sitation would bring back our initial problem, so just let it sanitize itself when the clock gets used. Do you mean 'clk_c' cases should be sanitized before clk_disable_unused() (such as during driver probe(), etc.)? Dropped in clk_disable_unused_subtree()? This is actually my biggest confusion.🙁 >>>> >>> No. It's handled correctly as long as the tree is in coherent state. >>> >>> What is not done anymore is fixing up an inconsistent tree, by this I >>> mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its >>> own registers but has its parent disabled. >>> >>> In that particular case, clk_disable_unused_subtree() won't be turning on >>> everything to properly disable that one clock. That is the root cause of >>> the problem you reported initially. The clock is disabled anyway. >>> >>> Every other case are properly handled (at least I think). >> name en_sts flags >> clk_a 1 CLK_IGNORE_UNUSED >> clk_b 0 0 >> clk_c 1 CLK_OPS_PARENT_ENABLE >> >> Based on the above case: >> 1. When 'clk_c' is configured with CLK_OPS_PARENT_ENABLE, disabling >> 'clk_c' requires enabling 'clk_b' first (disabling 'clk_c' before >> disabling 'clk_b'). How can to ensure that during the period of >> disabling 'clk_c', 'clk_b' remains enabled? > That's perfect example of incoherent state. > How can 'clk_c' be enabled if its parent is disable. That makes no > sense, so there is no point enabling a whole subtree for this IMO. > >> 2. 'clk_c' is not configured with CLK_IGNORE_UNUSED, it should be >> disabled later. However, here it goes to a 'goto' statement and then >> return 'false', ultimately resulting in 'clk_c' not being disabled? > We've discussed that 2 times already. This discussion is going in > circles now. > >>>>>>> goto unlock_out; >>>>>>> >>>>>>> /* >>>>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core) >>>>>>> >>>>>>> unlock_out: >>>>>>> clk_enable_unlock(flags); >>>>>>> - if (core->flags & CLK_OPS_PARENT_ENABLE) >>>>>>> - clk_core_disable_unprepare(core->parent); >>>>>>> + return (core->flags & CLK_IGNORE_UNUSED) && enabled; >>>>>>> } >>>>>>> >>>>>>> static bool clk_ignore_unused __initdata; >>>>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void) >>>>>>> clk_prepare_lock(); >>>>>>> >>>>>>> hlist_for_each_entry(core, &clk_root_list, child_node) >>>>>>> - clk_disable_unused_subtree(core); >>>>>>> + clk_disable_unused_subtree(core, true); >>>>>>> >>>>>>> hlist_for_each_entry(core, &clk_orphan_list, child_node) >>>>>>> - clk_disable_unused_subtree(core); >>>>>>> + clk_disable_unused_subtree(core, true); >>>>>>> >>>>>>> hlist_for_each_entry(core, &clk_root_list, child_node) >>>>>>> - clk_unprepare_unused_subtree(core); >>>>>>> + clk_unprepare_unused_subtree(core, true); >>>>>>> >>>>>>> hlist_for_each_entry(core, &clk_orphan_list, child_node) >>>>>>> - clk_unprepare_unused_subtree(core); >>>>>>> + clk_unprepare_unused_subtree(core, true); >>>>>>> >>>>>>> clk_prepare_unlock(); >>>>>>> >>>>>>> -- >>>>>>> 2.45.2 >>>>>>> >>>>> -- >>>>> Jerome >>> -- >>> Jerome > -- > Jerome
© 2016 - 2024 Red Hat, Inc.