Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751586Ab1BDMG2 (ORCPT ); Fri, 4 Feb 2011 07:06:28 -0500 Received: from caramon.arm.linux.org.uk ([78.32.30.218]:38472 "EHLO caramon.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751098Ab1BDMG1 (ORCPT ); Fri, 4 Feb 2011 07:06:27 -0500 Date: Fri, 4 Feb 2011 12:05:25 +0000 From: Russell King - ARM Linux To: Jassi Brar Cc: Richard Zhao , Nicolas Pitre , Lorenzo Pieralisi , Vincent Guittot , linux-sh@vger.kernel.org, Ben Herrenschmidt , Sascha Hauer , Paul Mundt , Stephen Boyd , linux-kernel@vger.kernel.org, Dima Zavin , Saravana Kannan , Ben Dooks , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , Jeremy Kerr , linux-arm-kernel@lists.infradead.org Subject: Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare Message-ID: <20110204120525.GH14627@n2100.arm.linux.org.uk> References: <20110201143932.GK31216@n2100.arm.linux.org.uk> <20110201151846.GD1147@pengutronix.de> <20110201152458.GP31216@n2100.arm.linux.org.uk> <4D48741F.8060006@codeaurora.org> <20110201212409.GU31216@n2100.arm.linux.org.uk> <20110204095424.GB2347@richard-laptop> <20110204104832.GE14627@n2100.arm.linux.org.uk> <20110204111841.GG14627@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.19 (2009-01-05) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3468 Lines: 87 On Fri, Feb 04, 2011 at 08:51:15PM +0900, Jassi Brar wrote: > On Fri, Feb 4, 2011 at 8:18 PM, Russell King - ARM Linux > wrote: > > On Fri, Feb 04, 2011 at 08:04:03PM +0900, Jassi Brar wrote: > >> On Fri, Feb 4, 2011 at 7:48 PM, Russell King - ARM Linux > >> wrote: > >> > >> > int clk_enable(struct clk *clk) > >> > { > >> > ? ? ? ?unsigned long flags; > >> > ? ? ? ?int ret = 0; > >> > > >> > ? ? ? ?if (clk) { > >> > ? ? ? ? ? ? ? ?if (WARN_ON(!clk->prepare_count)) > >> > ? ? ? ? ? ? ? ? ? ? ? ?return -EINVAL; > >> > > >> > ? ? ? ? ? ? ? ?spin_lock_irqsave(&clk->lock, flags); > >> > ? ? ? ? ? ? ? ?if (clk->enable_count++ == 0) > >> > ? ? ? ? ? ? ? ? ? ? ? ?ret = clk->ops->enable(clk); > >> > ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&clk->lock, flags); > >> > ? ? ? ?} > >> > ? ? ? ?return ret; > >> > } > >> > > >> > is entirely sufficient to catch the case of a single-use clock not being > >> > prepared before clk_enable() is called. > >> > > >> > We're after detecting drivers missing calls to clk_prepare(), we're not > >> > after detecting concurrent calls to clk_prepare()/clk_unprepare(). > >> > >> I hope you mean 'making sure the clock is prepared before it's enabled > >> ' rather than > >> 'catching a driver that doesn't do clk_prepare before clk_enable'. > >> Because, the above implementation still doesn't catch a driver that > >> doesn't call clk_prepare > >> but simply uses a clock that happens to have been already prepare'd by > >> some other > >> driver or the platform. > > > > No, I mean what I said. > Then, how does that function catch a driver that, say, doesn't do clk_prepare > but share the clk with another already active driver? As per the code I just supplied! > Because you said - "We're after detecting drivers missing calls to > clk_prepare()" > > The point is, there is difference between detecting drivers that miss > the clk_prepare > and ensuring clk_prepare has been called before any call to > clk_enable. And making > that clear helps get rid of lots of confusion/misunderstanding. Uwe > seems to have > had similar confusions. As I said on the 1st February. > > The only way to do what you're asking is to attach a list of identifiers > > which have prepared a clock to the struct clk, where each identifier is > > unique to each driver instance. > I am not asking what you think. > In my second last post, I am rather asking the other way around - that > let us not worry > about drivers missing the clk_prepare and not try to catch those by the > new API. No. That means we have no way to flag a call to clk_enable on an unprepared clock, and will lead to unexplained system lockups. What I've been suggesting all along is the "best efforts" approach. I'm sorry you can't see that, but that's really not my problem. > > I think that's going completely over the top, and adds needless complexity > > to drivers, which now have to pass an instance specific cookie into every > > clk API call. > Exactly. > All we need is to ensure clk_prepare has been called atleast once before > any call to clk_enable. I described this fully in my reply to Stephen Boyd on 1st February, which is a parent to this sub-thread. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/