| Message ID | 20170804160701.4d9f404d@bbrezillon (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Am Freitag, 4. August 2017, 16:07:01 CEST schrieb Boris Brezillon: > +Stephen, Mike and the linux-clk ML. > > On Fri, 4 Aug 2017 20:45:04 +0800 > "David.Wu" <david.wu@rock-chips.com> wrote: > > > Hi Boris & Heiko, > > > > 在 2016/3/31 4:03, Boris BREZILLON 写道: > > > + /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > > > + pwm_get_state(pc->chip.pwms, &pstate); > > > + if (!pstate.enabled) > > > + clk_disable(pc->clk); > > > > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 > > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It > > is true to close the pwm clock, and the pwm2 can't work during a while, > > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for > > their work. In fact, the pwm0 can not know the pwm2's status. > > > > So we need to get all the PWMs state in a public place where it's early > > than the PWM probe, if that's the way it is. Then keep the PWM clk > > enabled if theis is one PWM appears to be up and running. The place > > maybe in the clock core init, like adding pwm clock as critial one. > > > > Another way is that we don't enable pwm clock firstly at PWM probe, > > because whether or not the PWM state has been enabled in the Uboot, like > > other modules, our chip default PWM clock registers are enabled at the > > beginning, read the PWM registers directly to know the PWM state. Then > > if the PWM state is enabled, call the enable_clk(pc->clk) to add the > > clock count=1. If the PWM state is disabled, we do nothing. After all > > the PWMs are probed and all modules are probed, the clock core will gate > > the PWM clock if the clock count is 0, and keep clk enabled if the clock > > count is not 0. > > > > How do you feel about it? > > Ouch. That's indeed hard to solve in a clean way. I may have > something to suggest but I'm not sure clk maintainers will like it: what > if we make clk_disable() and clk_unprepare() just decrement the refcount > before the disable-unused-clks procedure has been executed (see > proposed patch below)? This way all clks that have been enabled by the > bootloader will stay in such state until all drivers have had a chance > to retain them (IOW, call clk_prepare()+clk_enable()). > > BTW, I think the problem you're describing here is not unique to PWM > devices, it's just that now, some PWM drivers are smart and try to keep > clks enabled to prevent glitches. Actually, Mike had patches that introduced so called "handoff" clocks [0]. Clocks that were handled as critical until some driver picked them up. It's not exactly the same as your change and still would require intervention from clock-drivers to mark clocks in such a way. So I really also like your approach, as it would make clock wiggling during early boot safe for everyone involved :-) . And both seem to cater to slightly different use-cases as well. Heiko [0] https://lwn.net/Articles/675658/ > --->8--- > From 47dcdc1bcc30b3ae1f76d33be824d2519a4dcca8 Mon Sep 17 00:00:00 2001 > From: Boris Brezillon <boris.brezillon@free-electrons.com> > Date: Fri, 4 Aug 2017 15:55:49 +0200 > Subject: [PATCH] clk: Keep clocks in their initial state until > clk_disable_unused() is called > > Some drivers are briefly preparing+enabling the clock in their > ->probe() hook and disable+unprepare them before leaving the function. > > This can be problem if a clock is shared between different devices, and > one of these devices is critical to the system. If this clock is > enabled/disabled by a non-critical device before the driver of the > critical one had a chance to enable+prepare it, there might be a short > period of time during which the critical device is not clocked. > > To solve this problem, we save the initial clock state (at registration > time) and prevent the clock from being disabled until kernel init is done > (which is when clk_disable_unused() is called). > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/clk/clk.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index fc58c52a26b4..3f61374a364b 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -58,6 +58,8 @@ struct clk_core { > struct clk_core *new_child; > unsigned long flags; > bool orphan; > + bool keep_enabled; > + bool keep_prepared; > unsigned int enable_count; > unsigned int prepare_count; > unsigned long min_rate; > @@ -486,7 +488,7 @@ static void clk_core_unprepare(struct clk_core *core) > > trace_clk_unprepare(core); > > - if (core->ops->unprepare) > + if (core->ops->unprepare && !core->keep_prepared) > core->ops->unprepare(core->hw); > > trace_clk_unprepare_complete(core); > @@ -602,7 +604,7 @@ static void clk_core_disable(struct clk_core *core) > > trace_clk_disable_rcuidle(core); > > - if (core->ops->disable) > + if (core->ops->disable && !core->keep_enabled) > core->ops->disable(core->hw); > > trace_clk_disable_complete_rcuidle(core); > @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_core *core) > hlist_for_each_entry(child, &core->children, child_node) > clk_unprepare_unused_subtree(child); > > + /* > + * Reset the ->keep_prepared flag so that subsequent calls to > + * clk_unprepare() on this clk actually unprepare it. > + */ > + core->keep_prepared = false; > + > if (core->prepare_count) > return; > > @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_core *core) > > flags = clk_enable_lock(); > > + /* > + * Reset the ->keep_enabled flag so that subsequent calls to > + * clk_disable() on this clk actually disable it. > + */ > + core->keep_enabled = false; > + > if (core->enable_count) > goto unlock_out; > > @@ -2446,6 +2460,17 @@ static int __clk_core_init(struct clk_core *core) > core->accuracy = 0; > > /* > + * We keep track of the initial clk status to keep clks in the state > + * they were left in by the bootloader until all drivers had a chance > + * to keep them prepared/enabled if they need to. > + */ > + if (core->ops->is_prepared && !clk_ignore_unused) > + core->keep_prepared = core->ops->is_prepared(core->hw); > + > + if (core->ops->is_enabled && !clk_ignore_unused) > + core->keep_enabled = core->ops->is_enabled(core->hw); > + > + /* > * Set clk's phase. > * Since a phase is by definition relative to its parent, just > * query the current clock phase, or just assume it's in phase. > >
Hi, On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote: >> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 >> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It >> > is true to close the pwm clock, and the pwm2 can't work during a while, >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for >> > their work. In fact, the pwm0 can not know the pwm2's status. >> > >> > So we need to get all the PWMs state in a public place where it's early >> > than the PWM probe, if that's the way it is. Then keep the PWM clk >> > enabled if theis is one PWM appears to be up and running. The place >> > maybe in the clock core init, like adding pwm clock as critial one. >> > >> > Another way is that we don't enable pwm clock firstly at PWM probe, >> > because whether or not the PWM state has been enabled in the Uboot, like >> > other modules, our chip default PWM clock registers are enabled at the >> > beginning, read the PWM registers directly to know the PWM state. Then >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the >> > clock count=1. If the PWM state is disabled, we do nothing. After all >> > the PWMs are probed and all modules are probed, the clock core will gate >> > the PWM clock if the clock count is 0, and keep clk enabled if the clock >> > count is not 0. >> > >> > How do you feel about it? >> >> Ouch. That's indeed hard to solve in a clean way. I may have >> something to suggest but I'm not sure clk maintainers will like it: what >> if we make clk_disable() and clk_unprepare() just decrement the refcount >> before the disable-unused-clks procedure has been executed (see >> proposed patch below)? This way all clks that have been enabled by the >> bootloader will stay in such state until all drivers have had a chance >> to retain them (IOW, call clk_prepare()+clk_enable()). >> >> BTW, I think the problem you're describing here is not unique to PWM >> devices, it's just that now, some PWM drivers are smart and try to keep >> clks enabled to prevent glitches. > > Actually, Mike had patches that introduced so called "handoff" clocks [0]. > Clocks that were handled as critical until some driver picked them up. > > It's not exactly the same as your change and still would require > intervention from clock-drivers to mark clocks in such a way. Right. As you're saying handoff isn't enough because in this case a driver _has_ picked up the clock. The whole issue is that it's a shared clock between the 4 PLLs. One driver already claimed the clock, enabled it, and disabled it. Really something would need to know that another driver in the future might want to also pick up the clock. > So I really also like your approach, as it would make clock wiggling > during early boot safe for everyone involved :-) . > > And both seem to cater to slightly different use-cases as well. One worry I have about not truly disabling any clocks at boot time is that it could break someone who relies on a clock being disabled. I'm not 100% sure that any of these would really affect someone, but... ...there is one example case I know of where you absolutely need a clock to stop on command (AKA it wouldn't be OK to defer till late init). That case is for SD Card Tuning. When you're doing a voltage switch the actual transition is keyed off the card clock stopping. That would break if there was a situation where clk_disable() didn't actually do what it was supposed to. This is a bit of a contrived case and probably isn't 100% relevant (I think dw_mmc, for instance, stops the card clock directly through the dw_mmc IP block and it's invisible to the common clock framework), but it illustrates the point that there could plausibly be cases where deferring a clk_disable() might be unwise. I suppose, though, that it would be possible to distinguish those two cases via a 2nd API call. AKA: clk_disable() -- disable the clock eventually clk_disable_sync() -- disable the clock but don't defer. Still won't actually disable if someone else explicitly holds a reference. --- If changing the API is bad, another thought for trying to solve this problem is that PWM regulators already need to be handled by special case SoC-specific code in a few different places (though, admittedly, much of this is firmware code) . Thus, maybe a not-too-ugly solution is to treat PWM as special case: In the Rockchip SoC clock driver, do a clk_get() of the PWM clock at early init time. In the Rockchip SoC clock driver, register to be notified for late initcall and do a clk_put() of the PWM clock The above acknowledges that: * The PWM clock is a bit special because it might be used by PWM regulators, thus it's very important not to glitch it at boot. * It's OK for the PWM clock to be on even if clients say it should be disabled (since there are 4 PWMs sharing this single clock this seems like a pretty safe bet). --- One last thought I had that everyone will probably hate (I kinda dislike it myself): you could introduce 4 "fake" PWM child clocks are just "divide by 1" clocks (aka they are no-ops). Thus: At pwm0 probe, we'll * enable the fake pwm0 clock, which will pass the enable up to the real pwm0 clock (which is already on). * disable the fake pwm0 clock, which will pass the disable up. Oops. ...but maybe this is an easier problem to solve? ...maybe we can say that parent clocks are special and we can defer this disable until late init. AKA: an explicit clk_enable() / clk_disable() still disables a clock but an enable/disable just because your child got enabled/disabled can be deferred. Like I said, this is probably too confusing and a bad idea... -Doug
On 08/04/2017 10:48 PM, Heiko Stuebner wrote: > Hi, > > Am Freitag, 4. August 2017, 16:07:01 CEST schrieb Boris Brezillon: >> +Stephen, Mike and the linux-clk ML. >> >> On Fri, 4 Aug 2017 20:45:04 +0800 >> "David.Wu" <david.wu@rock-chips.com> wrote: >> >>> Hi Boris & Heiko, >>> >>> 在 2016/3/31 4:03, Boris BREZILLON 写道: >>>> + /* Keep the PWM clk enabled if the PWM appears to be up and running. */ >>>> + pwm_get_state(pc->chip.pwms, &pstate); >>>> + if (!pstate.enabled) >>>> + clk_disable(pc->clk); >>> >>> We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 >>> is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It >>> is true to close the pwm clock, and the pwm2 can't work during a while, >>> until the pwm2 probe, because the pwm0 and pwm2 are the same clock for >>> their work. In fact, the pwm0 can not know the pwm2's status. >>> >>> So we need to get all the PWMs state in a public place where it's early >>> than the PWM probe, if that's the way it is. Then keep the PWM clk >>> enabled if theis is one PWM appears to be up and running. The place >>> maybe in the clock core init, like adding pwm clock as critial one. >>> >>> Another way is that we don't enable pwm clock firstly at PWM probe, >>> because whether or not the PWM state has been enabled in the Uboot, like >>> other modules, our chip default PWM clock registers are enabled at the >>> beginning, read the PWM registers directly to know the PWM state. Then >>> if the PWM state is enabled, call the enable_clk(pc->clk) to add the >>> clock count=1. If the PWM state is disabled, we do nothing. After all >>> the PWMs are probed and all modules are probed, the clock core will gate >>> the PWM clock if the clock count is 0, and keep clk enabled if the clock >>> count is not 0. >>> >>> How do you feel about it? >> >> Ouch. That's indeed hard to solve in a clean way. I may have >> something to suggest but I'm not sure clk maintainers will like it: what >> if we make clk_disable() and clk_unprepare() just decrement the refcount >> before the disable-unused-clks procedure has been executed (see >> proposed patch below)? This way all clks that have been enabled by the >> bootloader will stay in such state until all drivers have had a chance >> to retain them (IOW, call clk_prepare()+clk_enable()). >> >> BTW, I think the problem you're describing here is not unique to PWM >> devices, it's just that now, some PWM drivers are smart and try to keep >> clks enabled to prevent glitches. > > Actually, Mike had patches that introduced so called "handoff" clocks [0]. > Clocks that were handled as critical until some driver picked them up. > > It's not exactly the same as your change and still would require > intervention from clock-drivers to mark clocks in such a way. > > So I really also like your approach, as it would make clock wiggling > during early boot safe for everyone involved :-) . > > And both seem to cater to slightly different use-cases as well. > > > Heiko > > [0] https://lwn.net/Articles/675658/ Boris patch is better. If use the CLK_ENABLE_HAND_OFF flag for pwm clk, the pwm driver can't disable pwm clk by own.(Because the enable count >0) Boris patch is not merged into the master branch. Whether there is a plan to merge? > >> --->8--- >> From 47dcdc1bcc30b3ae1f76d33be824d2519a4dcca8 Mon Sep 17 00:00:00 2001 >> From: Boris Brezillon <boris.brezillon@free-electrons.com> >> Date: Fri, 4 Aug 2017 15:55:49 +0200 >> Subject: [PATCH] clk: Keep clocks in their initial state until >> clk_disable_unused() is called >> >> Some drivers are briefly preparing+enabling the clock in their >> ->probe() hook and disable+unprepare them before leaving the function. >> >> This can be problem if a clock is shared between different devices, and >> one of these devices is critical to the system. If this clock is >> enabled/disabled by a non-critical device before the driver of the >> critical one had a chance to enable+prepare it, there might be a short >> period of time during which the critical device is not clocked. >> >> To solve this problem, we save the initial clock state (at registration >> time) and prevent the clock from being disabled until kernel init is done >> (which is when clk_disable_unused() is called). >> >> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >> --- >> drivers/clk/clk.c | 29 +++++++++++++++++++++++++++-- >> 1 file changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index fc58c52a26b4..3f61374a364b 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -58,6 +58,8 @@ struct clk_core { >> struct clk_core *new_child; >> unsigned long flags; >> bool orphan; >> + bool keep_enabled; >> + bool keep_prepared; >> unsigned int enable_count; >> unsigned int prepare_count; >> unsigned long min_rate; >> @@ -486,7 +488,7 @@ static void clk_core_unprepare(struct clk_core *core) >> >> trace_clk_unprepare(core); >> >> - if (core->ops->unprepare) >> + if (core->ops->unprepare && !core->keep_prepared) >> core->ops->unprepare(core->hw); >> >> trace_clk_unprepare_complete(core); >> @@ -602,7 +604,7 @@ static void clk_core_disable(struct clk_core *core) >> >> trace_clk_disable_rcuidle(core); >> >> - if (core->ops->disable) >> + if (core->ops->disable && !core->keep_enabled) >> core->ops->disable(core->hw); >> >> trace_clk_disable_complete_rcuidle(core); >> @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_core *core) >> hlist_for_each_entry(child, &core->children, child_node) >> clk_unprepare_unused_subtree(child); >> >> + /* >> + * Reset the ->keep_prepared flag so that subsequent calls to >> + * clk_unprepare() on this clk actually unprepare it. >> + */ >> + core->keep_prepared = false; >> + >> if (core->prepare_count) >> return; >> >> @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_core *core) >> >> flags = clk_enable_lock(); >> >> + /* >> + * Reset the ->keep_enabled flag so that subsequent calls to >> + * clk_disable() on this clk actually disable it. >> + */ >> + core->keep_enabled = false; >> + >> if (core->enable_count) >> goto unlock_out; >> >> @@ -2446,6 +2460,17 @@ static int __clk_core_init(struct clk_core *core) >> core->accuracy = 0; >> >> /* >> + * We keep track of the initial clk status to keep clks in the state >> + * they were left in by the bootloader until all drivers had a chance >> + * to keep them prepared/enabled if they need to. >> + */ >> + if (core->ops->is_prepared && !clk_ignore_unused) >> + core->keep_prepared = core->ops->is_prepared(core->hw); >> + >> + if (core->ops->is_enabled && !clk_ignore_unused) >> + core->keep_enabled = core->ops->is_enabled(core->hw); >> + >> + /* >> * Set clk's phase. >> * Since a phase is by definition relative to its parent, just >> * query the current clock phase, or just assume it's in phase. >> >> > > > > >
Hi Doug, Sorry for the late reply. Le Fri, 4 Aug 2017 09:22:56 -0700, Doug Anderson <dianders@chromium.org> a écrit : > Hi, > > On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote: > >> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 > >> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It > >> > is true to close the pwm clock, and the pwm2 can't work during a while, > >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for > >> > their work. In fact, the pwm0 can not know the pwm2's status. > >> > > >> > So we need to get all the PWMs state in a public place where it's early > >> > than the PWM probe, if that's the way it is. Then keep the PWM clk > >> > enabled if theis is one PWM appears to be up and running. The place > >> > maybe in the clock core init, like adding pwm clock as critial one. > >> > > >> > Another way is that we don't enable pwm clock firstly at PWM probe, > >> > because whether or not the PWM state has been enabled in the Uboot, like > >> > other modules, our chip default PWM clock registers are enabled at the > >> > beginning, read the PWM registers directly to know the PWM state. Then > >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the > >> > clock count=1. If the PWM state is disabled, we do nothing. After all > >> > the PWMs are probed and all modules are probed, the clock core will gate > >> > the PWM clock if the clock count is 0, and keep clk enabled if the clock > >> > count is not 0. > >> > > >> > How do you feel about it? > >> > >> Ouch. That's indeed hard to solve in a clean way. I may have > >> something to suggest but I'm not sure clk maintainers will like it: what > >> if we make clk_disable() and clk_unprepare() just decrement the refcount > >> before the disable-unused-clks procedure has been executed (see > >> proposed patch below)? This way all clks that have been enabled by the > >> bootloader will stay in such state until all drivers have had a chance > >> to retain them (IOW, call clk_prepare()+clk_enable()). > >> > >> BTW, I think the problem you're describing here is not unique to PWM > >> devices, it's just that now, some PWM drivers are smart and try to keep > >> clks enabled to prevent glitches. > > > > Actually, Mike had patches that introduced so called "handoff" clocks [0]. > > Clocks that were handled as critical until some driver picked them up. > > > > It's not exactly the same as your change and still would require > > intervention from clock-drivers to mark clocks in such a way. > > Right. As you're saying handoff isn't enough because in this case a > driver _has_ picked up the clock. The whole issue is that it's a > shared clock between the 4 PLLs. One driver already claimed the > clock, enabled it, and disabled it. Really something would need to > know that another driver in the future might want to also pick up the > clock. > > > > So I really also like your approach, as it would make clock wiggling > > during early boot safe for everyone involved :-) . > > > > And both seem to cater to slightly different use-cases as well. > > One worry I have about not truly disabling any clocks at boot time is > that it could break someone who relies on a clock being disabled. I'm > not 100% sure that any of these would really affect someone, but... > > ...there is one example case I know of where you absolutely need a > clock to stop on command (AKA it wouldn't be OK to defer till late > init). That case is for SD Card Tuning. When you're doing a voltage > switch the actual transition is keyed off the card clock stopping. > That would break if there was a situation where clk_disable() didn't > actually do what it was supposed to. This is a bit of a contrived > case and probably isn't 100% relevant (I think dw_mmc, for instance, > stops the card clock directly through the dw_mmc IP block and it's > invisible to the common clock framework), but it illustrates the point > that there could plausibly be cases where deferring a clk_disable() > might be unwise. Right, I didn't consider this use case, but some drivers might rely on the fact that clk_disable() disables the clk right away instead of deferring it. > > I suppose, though, that it would be possible to distinguish those two > cases via a 2nd API call. AKA: > > clk_disable() -- disable the clock eventually > clk_disable_sync() -- disable the clock but don't defer. Still won't > actually disable if someone else explicitly holds a reference. Actually, I'd recommend keeping the existing behavior for clk_disable() and adding a new function called clk_disable_async() (or clk_disable_deferrable()). This way we do not break existing users that rely on clk_disable() synchronicity and force users that actually allow deferring clk gating to explicitly state it.
Hi, On Mon, Aug 21, 2017 at 8:39 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Doug, > > Sorry for the late reply. > > Le Fri, 4 Aug 2017 09:22:56 -0700, > Doug Anderson <dianders@chromium.org> a écrit : > >> Hi, >> >> On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote: >> >> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 >> >> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It >> >> > is true to close the pwm clock, and the pwm2 can't work during a while, >> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for >> >> > their work. In fact, the pwm0 can not know the pwm2's status. >> >> > >> >> > So we need to get all the PWMs state in a public place where it's early >> >> > than the PWM probe, if that's the way it is. Then keep the PWM clk >> >> > enabled if theis is one PWM appears to be up and running. The place >> >> > maybe in the clock core init, like adding pwm clock as critial one. >> >> > >> >> > Another way is that we don't enable pwm clock firstly at PWM probe, >> >> > because whether or not the PWM state has been enabled in the Uboot, like >> >> > other modules, our chip default PWM clock registers are enabled at the >> >> > beginning, read the PWM registers directly to know the PWM state. Then >> >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the >> >> > clock count=1. If the PWM state is disabled, we do nothing. After all >> >> > the PWMs are probed and all modules are probed, the clock core will gate >> >> > the PWM clock if the clock count is 0, and keep clk enabled if the clock >> >> > count is not 0. >> >> > >> >> > How do you feel about it? >> >> >> >> Ouch. That's indeed hard to solve in a clean way. I may have >> >> something to suggest but I'm not sure clk maintainers will like it: what >> >> if we make clk_disable() and clk_unprepare() just decrement the refcount >> >> before the disable-unused-clks procedure has been executed (see >> >> proposed patch below)? This way all clks that have been enabled by the >> >> bootloader will stay in such state until all drivers have had a chance >> >> to retain them (IOW, call clk_prepare()+clk_enable()). >> >> >> >> BTW, I think the problem you're describing here is not unique to PWM >> >> devices, it's just that now, some PWM drivers are smart and try to keep >> >> clks enabled to prevent glitches. >> > >> > Actually, Mike had patches that introduced so called "handoff" clocks [0]. >> > Clocks that were handled as critical until some driver picked them up. >> > >> > It's not exactly the same as your change and still would require >> > intervention from clock-drivers to mark clocks in such a way. >> >> Right. As you're saying handoff isn't enough because in this case a >> driver _has_ picked up the clock. The whole issue is that it's a >> shared clock between the 4 PLLs. One driver already claimed the >> clock, enabled it, and disabled it. Really something would need to >> know that another driver in the future might want to also pick up the >> clock. >> >> >> > So I really also like your approach, as it would make clock wiggling >> > during early boot safe for everyone involved :-) . >> > >> > And both seem to cater to slightly different use-cases as well. >> >> One worry I have about not truly disabling any clocks at boot time is >> that it could break someone who relies on a clock being disabled. I'm >> not 100% sure that any of these would really affect someone, but... >> >> ...there is one example case I know of where you absolutely need a >> clock to stop on command (AKA it wouldn't be OK to defer till late >> init). That case is for SD Card Tuning. When you're doing a voltage >> switch the actual transition is keyed off the card clock stopping. >> That would break if there was a situation where clk_disable() didn't >> actually do what it was supposed to. This is a bit of a contrived >> case and probably isn't 100% relevant (I think dw_mmc, for instance, >> stops the card clock directly through the dw_mmc IP block and it's >> invisible to the common clock framework), but it illustrates the point >> that there could plausibly be cases where deferring a clk_disable() >> might be unwise. > > Right, I didn't consider this use case, but some drivers might rely on > the fact that clk_disable() disables the clk right away instead of > deferring it. > >> >> I suppose, though, that it would be possible to distinguish those two >> cases via a 2nd API call. AKA: >> >> clk_disable() -- disable the clock eventually >> clk_disable_sync() -- disable the clock but don't defer. Still won't >> actually disable if someone else explicitly holds a reference. > > Actually, I'd recommend keeping the existing behavior for clk_disable() > and adding a new function called clk_disable_async() (or > clk_disable_deferrable()). This way we do not break existing users that > rely on clk_disable() synchronicity and force users that actually allow > deferring clk gating to explicitly state it. Yeah, there's a tradeoff. Your solution is definitely safer and causes less short term churn. ...but it's at the expense of some inconsistency between various similar APIs (some APIs the "default" is sync and some the "default" is async). Although I guess if there's one consistent thing about the kernel APIs it's that they're inconsistent. :-P Is anyone volunteering to do this? Do we want anyone with actual "approval" authority to chime in? -Doug
Le Mon, 21 Aug 2017 09:49:52 -0700, Doug Anderson <dianders@chromium.org> a écrit : > Hi, > > On Mon, Aug 21, 2017 at 8:39 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Hi Doug, > > > > Sorry for the late reply. > > > > Le Fri, 4 Aug 2017 09:22:56 -0700, > > Doug Anderson <dianders@chromium.org> a écrit : > > > >> Hi, > >> > >> On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote: > >> >> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 > >> >> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It > >> >> > is true to close the pwm clock, and the pwm2 can't work during a while, > >> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for > >> >> > their work. In fact, the pwm0 can not know the pwm2's status. > >> >> > > >> >> > So we need to get all the PWMs state in a public place where it's early > >> >> > than the PWM probe, if that's the way it is. Then keep the PWM clk > >> >> > enabled if theis is one PWM appears to be up and running. The place > >> >> > maybe in the clock core init, like adding pwm clock as critial one. > >> >> > > >> >> > Another way is that we don't enable pwm clock firstly at PWM probe, > >> >> > because whether or not the PWM state has been enabled in the Uboot, like > >> >> > other modules, our chip default PWM clock registers are enabled at the > >> >> > beginning, read the PWM registers directly to know the PWM state. Then > >> >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the > >> >> > clock count=1. If the PWM state is disabled, we do nothing. After all > >> >> > the PWMs are probed and all modules are probed, the clock core will gate > >> >> > the PWM clock if the clock count is 0, and keep clk enabled if the clock > >> >> > count is not 0. > >> >> > > >> >> > How do you feel about it? > >> >> > >> >> Ouch. That's indeed hard to solve in a clean way. I may have > >> >> something to suggest but I'm not sure clk maintainers will like it: what > >> >> if we make clk_disable() and clk_unprepare() just decrement the refcount > >> >> before the disable-unused-clks procedure has been executed (see > >> >> proposed patch below)? This way all clks that have been enabled by the > >> >> bootloader will stay in such state until all drivers have had a chance > >> >> to retain them (IOW, call clk_prepare()+clk_enable()). > >> >> > >> >> BTW, I think the problem you're describing here is not unique to PWM > >> >> devices, it's just that now, some PWM drivers are smart and try to keep > >> >> clks enabled to prevent glitches. > >> > > >> > Actually, Mike had patches that introduced so called "handoff" clocks [0]. > >> > Clocks that were handled as critical until some driver picked them up. > >> > > >> > It's not exactly the same as your change and still would require > >> > intervention from clock-drivers to mark clocks in such a way. > >> > >> Right. As you're saying handoff isn't enough because in this case a > >> driver _has_ picked up the clock. The whole issue is that it's a > >> shared clock between the 4 PLLs. One driver already claimed the > >> clock, enabled it, and disabled it. Really something would need to > >> know that another driver in the future might want to also pick up the > >> clock. > >> > >> > >> > So I really also like your approach, as it would make clock wiggling > >> > during early boot safe for everyone involved :-) . > >> > > >> > And both seem to cater to slightly different use-cases as well. > >> > >> One worry I have about not truly disabling any clocks at boot time is > >> that it could break someone who relies on a clock being disabled. I'm > >> not 100% sure that any of these would really affect someone, but... > >> > >> ...there is one example case I know of where you absolutely need a > >> clock to stop on command (AKA it wouldn't be OK to defer till late > >> init). That case is for SD Card Tuning. When you're doing a voltage > >> switch the actual transition is keyed off the card clock stopping. > >> That would break if there was a situation where clk_disable() didn't > >> actually do what it was supposed to. This is a bit of a contrived > >> case and probably isn't 100% relevant (I think dw_mmc, for instance, > >> stops the card clock directly through the dw_mmc IP block and it's > >> invisible to the common clock framework), but it illustrates the point > >> that there could plausibly be cases where deferring a clk_disable() > >> might be unwise. > > > > Right, I didn't consider this use case, but some drivers might rely on > > the fact that clk_disable() disables the clk right away instead of > > deferring it. > > > >> > >> I suppose, though, that it would be possible to distinguish those two > >> cases via a 2nd API call. AKA: > >> > >> clk_disable() -- disable the clock eventually > >> clk_disable_sync() -- disable the clock but don't defer. Still won't > >> actually disable if someone else explicitly holds a reference. > > > > Actually, I'd recommend keeping the existing behavior for clk_disable() > > and adding a new function called clk_disable_async() (or > > clk_disable_deferrable()). This way we do not break existing users that > > rely on clk_disable() synchronicity and force users that actually allow > > deferring clk gating to explicitly state it. > > Yeah, there's a tradeoff. Your solution is definitely safer and > causes less short term churn. ...but it's at the expense of some > inconsistency between various similar APIs (some APIs the "default" is > sync and some the "default" is async). Although I guess if there's > one consistent thing about the kernel APIs it's that they're > inconsistent. :-P Yep. > > Is anyone volunteering to do this? Looks like you found a volunteer: Elaine already posted my patch to the linux-clk ML :-). > Do we want anyone with actual > "approval" authority to chime in? Don't if that's what you meant by "approval" authority, but you'll need Mike or Stephen (or both) to validate this approach.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fc58c52a26b4..3f61374a364b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -58,6 +58,8 @@ struct clk_core { struct clk_core *new_child; unsigned long flags; bool orphan; + bool keep_enabled; + bool keep_prepared; unsigned int enable_count; unsigned int prepare_count; unsigned long min_rate; @@ -486,7 +488,7 @@ static void clk_core_unprepare(struct clk_core *core) trace_clk_unprepare(core); - if (core->ops->unprepare) + if (core->ops->unprepare && !core->keep_prepared) core->ops->unprepare(core->hw); trace_clk_unprepare_complete(core); @@ -602,7 +604,7 @@ static void clk_core_disable(struct clk_core *core) trace_clk_disable_rcuidle(core); - if (core->ops->disable) + if (core->ops->disable && !core->keep_enabled) core->ops->disable(core->hw); trace_clk_disable_complete_rcuidle(core); @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_core *core) hlist_for_each_entry(child, &core->children, child_node) clk_unprepare_unused_subtree(child); + /* + * Reset the ->keep_prepared flag so that subsequent calls to + * clk_unprepare() on this clk actually unprepare it. + */ + core->keep_prepared = false; + if (core->prepare_count) return; @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_core *core) flags = clk_enable_lock(); + /* + * Reset the ->keep_enabled flag so that subsequent calls to + * clk_disable() on this clk actually disable it. + */ + core->keep_enabled = false; + if (core->enable_count) goto unlock_out; @@ -2446,6 +2460,17 @@ static int __clk_core_init(struct clk_core *core) core->accuracy = 0; /* + * We keep track of the initial clk status to keep clks in the state + * they were left in by the bootloader until all drivers had a chance + * to keep them prepared/enabled if they need to. + */ + if (core->ops->is_prepared && !clk_ignore_unused) + core->keep_prepared = core->ops->is_prepared(core->hw); + + if (core->ops->is_enabled && !clk_ignore_unused) + core->keep_enabled = core->ops->is_enabled(core->hw); + + /* * Set clk's phase. * Since a phase is by definition relative to its parent, just * query the current clock phase, or just assume it's in phase.