Message-ID: <85aae140-5c9b-4ff9-a522-549009f62601@amlogic.com>
Date: Fri, 8 Nov 2024 17:23:53 +0800
From: Chuan Liu <chuan.liu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Stephen Boyd <sboyd@...nel.org>,
 Neil Armstrong <neil.armstrong@...aro.org>,
 Kevin Hilman <khilman@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-amlogic@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH] clk: core: refine disable unused clocks


On 11/8/2024 4:38 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 08 Nov 2024 at 15:59, Chuan Liu <chuan.liu@...ogic.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@...libre.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

Powered by blists - more mailing lists