[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5eb12197-330c-4f55-82f7-d13ea458ba43@amlogic.com>
Date: Fri, 8 Nov 2024 19:49:59 +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 5:59 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@...ogic.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
Powered by blists - more mailing lists