| Message ID | 1393258967-4843-2-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 24, 2014 at 05:22:43PM +0100, Maxime Ripard wrote: > Right now, AHB is an indirect child clock of the CPU clock. If that happens to > change, since the CPU clock has no other consumers declared in Linux, it would > be shut down, which is not really a good idea. > > Prevent this by forcing it enabled. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/clk/sunxi/clk-sunxi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > index 23baad9..cedaf4b 100644 > --- a/drivers/clk/sunxi/clk-sunxi.c > +++ b/drivers/clk/sunxi/clk-sunxi.c > @@ -1301,6 +1301,14 @@ static void __init sunxi_clock_protect(void) > clk_prepare_enable(clk); > clk_put(clk); > } > + > + /* CPU clocks - sun6i */ > + clk = clk_get(NULL, "cpu"); > + if (!IS_ERR(clk)) { > + clk_prepare_enable(clk); > + clk_put(clk); > + } This is broken. I'm not sure what's difficult to grasp about the concept of "while a clock is in use, you should keep a reference to that clock". That implies that if you get a clock, and then enable it, you don't put the clock until you've disabled it. The only reason the core doesn't check for this kind of thing is that a clock may be shared, so it's entirely possible for a correctly written driver to have a clock which is still enabled at put time - but enabled by an entirely different driver. However, that's no excuse for this kind of sloppiness.
Hello Russell, El 24/02/14 13:30, Russell King - ARM Linux escribió: > On Mon, Feb 24, 2014 at 05:22:43PM +0100, Maxime Ripard wrote: >> Right now, AHB is an indirect child clock of the CPU clock. If that happens to >> change, since the CPU clock has no other consumers declared in Linux, it would >> be shut down, which is not really a good idea. >> >> Prevent this by forcing it enabled. >> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> --- >> drivers/clk/sunxi/clk-sunxi.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index 23baad9..cedaf4b 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -1301,6 +1301,14 @@ static void __init sunxi_clock_protect(void) >> clk_prepare_enable(clk); >> clk_put(clk); >> } >> + >> + /* CPU clocks - sun6i */ >> + clk = clk_get(NULL, "cpu"); >> + if (!IS_ERR(clk)) { >> + clk_prepare_enable(clk); >> + clk_put(clk); >> + } > > This is broken. I'm not sure what's difficult to grasp about the concept > of "while a clock is in use, you should keep a reference to that clock". > > That implies that if you get a clock, and then enable it, you don't > put the clock until you've disabled it. Why is this so? Can't a clock be left enabled while nobody has a reference to it? I have looked around in Documentation/ (rather quickly I must say) and have not found any explicit mention that it is required to keep a reference to the clock while it's enabled. I'd appreciate it if you could explain this a bit more verbosely or point me to the relevant documents. For what it's worth, I've seen this same pattern on enable/disable_clock() on drivers/base/power/clock_ops.c as well. Cheers, Emilio
Hi Emilio. On Mon, Feb 24, 2014 at 08:38:44PM -0300, Emilio López wrote: > Why is this so? Can't a clock be left enabled while nobody has a > reference to it? I have looked around in Documentation/ (rather quickly > I must say) and have not found any explicit mention that it is required > to keep a reference to the clock while it's enabled. I'd appreciate it > if you could explain this a bit more verbosely or point me to the > relevant documents. First up, if you have a requirement that a clock be enabled, then is it not unreasonable to ensure that the clock is referenced? Secondly, what if we have code which scans the clocks in the system, shutting down those leaf clocks which appear to be unreferenced? Thirdly, the API (as I designed it) says so: /** * clk_put - "free" the clock source * @clk: clock source * * Note: drivers must ensure that all clk_enable calls made on this * clock source are balanced by clk_disable calls prior to calling * this function. * * clk_put should not be called from within interrupt context. */ void clk_put(struct clk *clk); which has been there since the API was first created - it's part of the contract between drivers using the API and implementers creating something which conforms to the API - which today means CCF. The intention here is that while there are any users holding a clk_get() reference on a clock, the clock is assumed to be required for some device, and the struct clk may not be kfree'd, nor may its state be changed in an unpredictable way to those drivers holding a reference to it.
Hi Russell, El 24/02/14 21:01, Russell King - ARM Linux escribió: > Hi Emilio. > > On Mon, Feb 24, 2014 at 08:38:44PM -0300, Emilio López wrote: >> Why is this so? Can't a clock be left enabled while nobody has a >> reference to it? I have looked around in Documentation/ (rather quickly >> I must say) and have not found any explicit mention that it is required >> to keep a reference to the clock while it's enabled. I'd appreciate it >> if you could explain this a bit more verbosely or point me to the >> relevant documents. > > First up, if you have a requirement that a clock be enabled, then is it > not unreasonable to ensure that the clock is referenced? I was under the impression that the reference count was orthogonal to the clock status, but after getting that clarified, I can see your point. > Secondly, what if we have code which scans the clocks in the system, > shutting down those leaf clocks which appear to be unreferenced? Indeed, that would break things. > Thirdly, the API (as I designed it) says so: > > /** > * clk_put - "free" the clock source > * @clk: clock source > * > * Note: drivers must ensure that all clk_enable calls made on this > * clock source are balanced by clk_disable calls prior to calling > * this function. > * > * clk_put should not be called from within interrupt context. > */ > void clk_put(struct clk *clk); > > which has been there since the API was first created - it's part of the > contract between drivers using the API and implementers creating something > which conforms to the API - which today means CCF. That's enough of a reason on its own :) I should have checked clk.h > The intention here is that while there are any users holding a clk_get() > reference on a clock, the clock is assumed to be required for some > device, and the struct clk may not be kfree'd, nor may its state be > changed in an unpredictable way to those drivers holding a reference > to it. I understand now, thanks for the insight. I'll talk with Maxime and get this sorted out. As a side note, should drivers/base/power/clock_ops.c be fixed too? I have added Rafael to Cc. Cheers, Emilio
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index 23baad9..cedaf4b 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ b/drivers/clk/sunxi/clk-sunxi.c @@ -1301,6 +1301,14 @@ static void __init sunxi_clock_protect(void) clk_prepare_enable(clk); clk_put(clk); } + + /* CPU clocks - sun6i */ + clk = clk_get(NULL, "cpu"); + if (!IS_ERR(clk)) { + clk_prepare_enable(clk); + clk_put(clk); + } + } static void __init sunxi_init_clocks(void)
Right now, AHB is an indirect child clock of the CPU clock. If that happens to change, since the CPU clock has no other consumers declared in Linux, it would be shut down, which is not really a good idea. Prevent this by forcing it enabled. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/clk/sunxi/clk-sunxi.c | 8 ++++++++ 1 file changed, 8 insertions(+)