| Message ID | 1352417516-15213-1-git-send-email-mturquette@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 8 Nov 2012, Mike Turquette wrote: > The OMAP port to the common clk framework[1] resulted in spurious WARNs > while disable unused clocks. This is due to _clkdm_clk_hwmod_disable > catching clkdm->usecount's with a value of zero. Even less desirable it > would not allow the clkdm_clk_disable function pointer to get called due > to an early return of -ERANGE. > > This patch adds a check for such a corner case by skipping the WARN and > early return in the event that clkdm->usecount and clk->enable_usecount > are both zero. Presumably this could only happen during the check for > unused clocks at boot-time. > > [1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824 > > Signed-off-by: Mike Turquette <mturquette@ti.com> I don't think this is going to work, as it currently stands. The code will just bypass the warning and the error return. The clockdomain usecount still will be decremented, which is going to cause problems since the usecount will be inaccurate. - Paul
Quoting Paul Walmsley (2012-11-08 16:58:21) > On Thu, 8 Nov 2012, Mike Turquette wrote: > > > The OMAP port to the common clk framework[1] resulted in spurious WARNs > > while disable unused clocks. This is due to _clkdm_clk_hwmod_disable > > catching clkdm->usecount's with a value of zero. Even less desirable it > > would not allow the clkdm_clk_disable function pointer to get called due > > to an early return of -ERANGE. > > > > This patch adds a check for such a corner case by skipping the WARN and > > early return in the event that clkdm->usecount and clk->enable_usecount > > are both zero. Presumably this could only happen during the check for > > unused clocks at boot-time. > > > > [1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824 > > > > Signed-off-by: Mike Turquette <mturquette@ti.com> > > I don't think this is going to work, as it currently stands. The code > will just bypass the warning and the error return. The clockdomain > usecount still will be decremented, which is going to cause problems since > the usecount will be inaccurate. > You're right. In my rush I glossed over the clkdm decrement part. In light of the suspend/resume issues I'm not sure this approach is really valid. I think getting to the bottom of those issues will give the final word. Regards, Mike > > - Paul
On Thu, 8 Nov 2012, Mike Turquette wrote: > You're right. In my rush I glossed over the clkdm decrement part. In > light of the suspend/resume issues I'm not sure this approach is really > valid. I think getting to the bottom of those issues will give the > final word. What do you think about something like this? It's still under test and review here, but seems to avoid the warnings on 3530ES3 Beagle at least. The usage of __clk_get_enable_count() in this code still seems like a hack to me. It would be better for the CCF to call a different clk_hw_ops function pointer for the disable-unused-clocks case. But if you agree, and plan to fix this, or have some other cleaner fix in mind for the near future, then something like this seems reasonable for the short term to me. What do you think? - Paul
diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c index 64e5046..b0c0ce6 100644 --- a/arch/arm/mach-omap2/clockdomain.c +++ b/arch/arm/mach-omap2/clockdomain.c @@ -947,16 +947,22 @@ static int _clkdm_clk_hwmod_enable(struct clockdomain *clkdm) return 0; } -static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm) +static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm, + struct clk *clk) { unsigned long flags; + int clk_enable_count = 1; if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_disable) return -EINVAL; spin_lock_irqsave(&clkdm->lock, flags); - if (atomic_read(&clkdm->usecount) == 0) { + /* corner case: disabling unused clocks */ + if (clk) + clk_enable_count = __clk_get_enable_count(clk); + + if (atomic_read(&clkdm->usecount) == 0 && clk_enable_count) { spin_unlock_irqrestore(&clkdm->lock, flags); WARN_ON(1); /* underflow */ return -ERANGE; @@ -1026,7 +1032,7 @@ int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk) if (!clk) return -EINVAL; - return _clkdm_clk_hwmod_disable(clkdm); + return _clkdm_clk_hwmod_disable(clkdm, clk); } /** @@ -1089,6 +1095,6 @@ int clkdm_hwmod_disable(struct clockdomain *clkdm, struct omap_hwmod *oh) if (!oh) return -EINVAL; - return _clkdm_clk_hwmod_disable(clkdm); + return _clkdm_clk_hwmod_disable(clkdm, NULL); }
The OMAP port to the common clk framework[1] resulted in spurious WARNs while disable unused clocks. This is due to _clkdm_clk_hwmod_disable catching clkdm->usecount's with a value of zero. Even less desirable it would not allow the clkdm_clk_disable function pointer to get called due to an early return of -ERANGE. This patch adds a check for such a corner case by skipping the WARN and early return in the event that clkdm->usecount and clk->enable_usecount are both zero. Presumably this could only happen during the check for unused clocks at boot-time. [1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824 Signed-off-by: Mike Turquette <mturquette@ti.com> --- arch/arm/mach-omap2/clockdomain.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)