Messages in this thread | | | Subject | Re: [PATCH 2/3] clk: WARN_ON about to disable a critical clock | From | Dirk Behme <> | Date | Mon, 3 Jul 2017 14:01:07 +0200 |
| |
On 03.07.2017 13:53, Lee Jones wrote: > On Tue, 27 Jun 2017, Dirk Behme wrote: > >> On 11.02.2016 01:43, Michael Turquette wrote: >>> Quoting Lee Jones (2016-01-18 06:28:50) >>>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> >>> Looks good to me. >>> >>> Regards, >>> Mike >>> >>>> --- >>>> drivers/clk/clk.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>>> index 835cb85..178b364 100644 >>>> --- a/drivers/clk/clk.c >>>> +++ b/drivers/clk/clk.c >>>> @@ -575,6 +575,9 @@ static void clk_core_unprepare(struct clk_core *core) >>>> if (WARN_ON(core->prepare_count == 0)) >>>> return; >>>> + if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL)) >>>> + return; >>>> + >>>> if (--core->prepare_count > 0) >>>> return; >>>> @@ -680,6 +683,9 @@ static void clk_core_disable(struct clk_core *core) >>>> if (WARN_ON(core->enable_count == 0)) >>>> return; >>>> + if (WARN_ON(core->enable_count == 1 && core->flags & CLK_IS_CRITICAL)) >>>> + return; >>>> + >>>> if (--core->enable_count > 0) >>>> return; >> >> >> I have a question regarding this patch, which is mainline meanwhile [1]: >> >> Having the following clock configuration: >> >> |--> child clk '1' (crit) >> clk source --> parent clk 'A' (crit) -->| >> |--> child clk '2' >> >> >> Clock '2' might be used, or not. It might be disabled or not. It doesn't >> matter. Clock '1' is not allowed to be disabled. Therefore its marked as >> critical. >> >> Parent clock 'A' is marked as critical because its not allowed to be >> disabled, even if the enable_count of all child clocks is 0. To avoid that >> by disabling parent clock 'A' the child clock '1' is disabled, too, whats >> not allowed as its marked as critical. >> >> >> Now, child clock '2' is used and enabled & disabled continuously by a (SPI) >> driver. What is ok. But: >> >> Disabling child clock '2' results in the attempt to disable parent clock >> 'A', too, which has correct enable_count 1 (from enabling the child '2'). >> What results >> >> a) in the WARN_ON output >> >> and >> >> b) enable_count of 'A' never decreases to 0. Being off by one after the >> WARN_ON >> >> >> It sounds like both is wrong for a configuration like above. > > Clock A still has one user, Clock 1. > > Why is that wrong?
Because clock 1 is not a (Linux kernel clock framework) used clock. Its enable count is 0. So from Linux kernel (clock framework) point of view clock 1 is unused.
The increase/decrease of enable count of parent clock A is only driven by the Linux kernel usage of clock 2.
Best regards
Dirk
>> Opinions or proposal how to fix/change this? >> >> >> Best regards >> >> Dirk >> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/clk.c?id=2e20fbf592621b2c2aeddd82e0fa3dad053cce03
| |