From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH 1/5] clk: sun6i: Protect CPU clock Date: Mon, 24 Feb 2014 16:30:35 +0000 Message-ID: <20140224163034.GN21483@n2100.arm.linux.org.uk> References: <1393258967-4843-1-git-send-email-maxime.ripard@free-electrons.com> <1393258967-4843-2-git-send-email-maxime.ripard@free-electrons.com> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <1393258967-4843-2-git-send-email-maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Post: , List-Help: , List-Archive: List-Subscribe: , List-Unsubscribe: , Content-Disposition: inline To: Maxime Ripard Cc: Emilio Lopez , Dan Williams , Vinod Koul , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mike Turquette , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org 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 > --- > 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. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.