Re: [RFC PATCH] clk: core: refine disable unused clocks

[Chuan Liu <chuan.liu@amlogic.com>]

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"?

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
>

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic