From patchwork Wed Oct 22 22:46:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Torokhov X-Patchwork-Id: 5137481 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id AE4649F349 for ; Wed, 22 Oct 2014 22:49:10 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 43A9820253 for ; Wed, 22 Oct 2014 22:49:09 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B29CF2024F for ; Wed, 22 Oct 2014 22:49:07 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Xh4g3-0001iF-7B; Wed, 22 Oct 2014 22:46:47 +0000 Received: from mail-pa0-x233.google.com ([2607:f8b0:400e:c03::233]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Xh4fx-0001Zy-Vj for linux-arm-kernel@lists.infradead.org; Wed, 22 Oct 2014 22:46:43 +0000 Received: by mail-pa0-f51.google.com with SMTP id lj1so4600298pab.38 for ; Wed, 22 Oct 2014 15:46:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=hsSS4A2kQxwF7yi6Nx0SKagAX198R9342guNdHrMocQ=; b=c2gp+ljVfMw0CoS7J5xUdhVLrxfNsxHI6otW3KRYfHT1YrAH63ZDQam1TfDss9BoMP V6DpLg3GyCA9DA1nQN2sYqWRUyRCcy1P99fTALPQjY34NM9l/jgar6LnSpvJC0QZl6lL zHpxMTMDRDkHc23LQgilcpDzPVVYTHxJFc0ZS0FOk3PFvADGyI13GpPEQXqTMvvR097q X841Qoluh+rUxQH+wA46cVL7B6GZEplSQyKieApK1kJzyhIhwEOqUvGzQjFLFz0SAlto Wdi1F0M6jT0a9NNGaQSJKqqCW01QsTxtc/45p7sM+aCqamZxjSUOJvNlWFghODvLY191 DWvg== X-Received: by 10.68.211.6 with SMTP id my6mr801954pbc.125.1414017979950; Wed, 22 Oct 2014 15:46:19 -0700 (PDT) Received: from dtor-ws ([2620:0:1000:1301:21ee:7553:df8f:2bf2]) by mx.google.com with ESMTPSA id pc8sm72707pbc.10.2014.10.22.15.46.16 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 22 Oct 2014 15:46:17 -0700 (PDT) Date: Wed, 22 Oct 2014 15:46:14 -0700 From: Dmitry Torokhov To: Grygorii Strashko Subject: Re: [PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk() Message-ID: <20141022224614.GD4250@dtor-ws> References: <1413809764-21995-1-git-send-email-grygorii.strashko@ti.com> <1413809764-21995-2-git-send-email-grygorii.strashko@ti.com> <20141022173856.GB28104@dtor-ws> <5447FF51.5000401@ti.com> <20141022201409.GB4250@dtor-ws> <20141022211631.GC4250@dtor-ws> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20141022211631.GC4250@dtor-ws> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20141022_154642_103297_2116EA2E X-CRM114-Status: GOOD ( 39.85 ) X-Spam-Score: -0.8 (/) Cc: devicetree@vger.kernel.org, ulf.hansson@linaro.org, khilman@linaro.org, Geert Uytterhoeven , linux-pm@vger.kernel.org, "Rafael J. Wysocki" , linux-kernel@vger.kernel.org, grant.likely@secretlab.ca, Rob Herring , ssantosh@kernel.org, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Oct 22, 2014 at 02:16:31PM -0700, Dmitry Torokhov wrote: > On Wed, Oct 22, 2014 at 01:14:09PM -0700, Dmitry Torokhov wrote: > > On Wed, Oct 22, 2014 at 10:02:41PM +0300, Grygorii Strashko wrote: > > > On 10/22/2014 08:38 PM, Dmitry Torokhov wrote: > > > >On Mon, Oct 20, 2014 at 03:56:02PM +0300, Grygorii Strashko wrote: > > > >>From: Geert Uytterhoeven > > > >> > > > >>The existing pm_clk_add() allows to pass a clock by con_id. However, > > > >>when referring to a specific clock from DT, no con_id is available. > > > >> > > > >>Add pm_clk_add_clk(), which allows to specify the struct clk * directly. > > > >> > > > >>Reviewed-by: Kevin Hilman > > > >>Signed-off-by: Geert Uytterhoeven > > > >>Signed-off-by: Grygorii Strashko > > > >>--- > > > >> > > > >> Pay attantion pls, that there is another series of patches > > > >> which have been posted already and which depends from this patch > > > >> "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support" > > > >> https://lkml.org/lkml/2014/10/20/105 > > > >> > > > >> drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++---------- > > > >> include/linux/pm_clock.h | 8 ++++++++ > > > >> 2 files changed, 39 insertions(+), 10 deletions(-) > > > >> > > > >>diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c > > > >>index 7836930..f14b767 100644 > > > >>--- a/drivers/base/power/clock_ops.c > > > >>+++ b/drivers/base/power/clock_ops.c > > > >>@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk) > > > >> */ > > > >> static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce) > > > >> { > > > >>- ce->clk = clk_get(dev, ce->con_id); > > > >>+ if (!ce->clk) > > > >>+ ce->clk = clk_get(dev, ce->con_id); > > > >> if (IS_ERR(ce->clk)) { > > > >> ce->status = PCE_STATUS_ERROR; > > > >> } else { > > > >>@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce) > > > >> } > > > >> } > > > >> > > > >>-/** > > > >>- * pm_clk_add - Start using a device clock for power management. > > > >>- * @dev: Device whose clock is going to be used for power management. > > > >>- * @con_id: Connection ID of the clock. > > > >>- * > > > >>- * Add the clock represented by @con_id to the list of clocks used for > > > >>- * the power management of @dev. > > > >>- */ > > > >>-int pm_clk_add(struct device *dev, const char *con_id) > > > >>+static int __pm_clk_add(struct device *dev, const char *con_id, > > > >>+ struct clk *clk) > > > >> { > > > >> struct pm_subsys_data *psd = dev_to_psd(dev); > > > >> struct pm_clock_entry *ce; > > > >>@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id) > > > >> kfree(ce); > > > >> return -ENOMEM; > > > >> } > > > >>+ } else { > > > >>+ ce->clk = clk; > > Shouldn't this be > > ce->clk = __clk_get(clk); > > to account for clk_put() in __pm_clk_remove()? > > > > >> } > > > >> > > > >> pm_clk_acquire(dev, ce); > > > >>@@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id) > > > >> } > > > >> > > > >> /** > > > >>+ * pm_clk_add - Start using a device clock for power management. > > > >>+ * @dev: Device whose clock is going to be used for power management. > > > >>+ * @con_id: Connection ID of the clock. > > > >>+ * > > > >>+ * Add the clock represented by @con_id to the list of clocks used for > > > >>+ * the power management of @dev. > > > >>+ */ > > > >>+int pm_clk_add(struct device *dev, const char *con_id) > > > >>+{ > > > >>+ return __pm_clk_add(dev, con_id, NULL); > > > > > > > >Bikeshedding: why do we need __pm_clk_add() and not simply have > > > >"canonical" pm_clk_add_clk() and then do: > > > > > > > >int pm_clk_add(struct device *dev, const char *con_id) > > > >{ > > > > struct clk *clk; > > > > > > > > clk = clk_get(dev, con_id); > > > > ... > > > > return pm_clk_add_clk(dev, clk); > > > >} > > > > > > Hm. I did fast look at code and: > > > 1) agree - there is a lot of thing which can be optimized ;) > > > 2) in my strong opinion, this patch is the fastest and simplest > > > way to introduce new API (take a look on pm_clock_entry->con_id > > > management code) and It is exactly what we need as of now. > > > > Yeah, I guess. We are lucky we do not crash when we are tryign to print > > NULL strings (see pm_clk_acquire). > > > > BTW, what is the point of doing pm_clk_add(dev, NULL)? We add clock > > entry with status PCE_STATUS_ERROR and then have to handle it > > everywhere? Can we just return -EINVAL if someone triies to pass NULL > > ass con_id? > > Umm, no, ignore me here, I misread clk_get(dev, NULL) - it won't return > error. Still, do why do we need to keep clock entry if clk_get() fails > for some reason? OK, so what if we do something like the patch below? Thanks. diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index f14b767..840e133 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -12,21 +12,21 @@ #include #include #include +#include +#include #include #include #ifdef CONFIG_PM enum pce_status { - PCE_STATUS_NONE = 0, - PCE_STATUS_ACQUIRED, + PCE_STATUS_ACQUIRED = 0, + PCE_STATUS_PREPARED, PCE_STATUS_ENABLED, - PCE_STATUS_ERROR, }; struct pm_clock_entry { struct list_head node; - char *con_id; struct clk *clk; enum pce_status status; }; @@ -47,25 +47,13 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk) } /** - * pm_clk_acquire - Acquire a device clock. - * @dev: Device whose clock is to be acquired. - * @ce: PM clock entry corresponding to the clock. + * pm_clk_add_clk - Start using a device clock for power management. + * @dev: Device whose clock is going to be used for power management. + * @clk: Clock pointer + * + * Add the clock to the list of clocks used for the power management of @dev. */ -static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce) -{ - if (!ce->clk) - ce->clk = clk_get(dev, ce->con_id); - if (IS_ERR(ce->clk)) { - ce->status = PCE_STATUS_ERROR; - } else { - clk_prepare(ce->clk); - ce->status = PCE_STATUS_ACQUIRED; - dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id); - } -} - -static int __pm_clk_add(struct device *dev, const char *con_id, - struct clk *clk) +int pm_clk_add_clk(struct device *dev, struct clk *clk) { struct pm_subsys_data *psd = dev_to_psd(dev); struct pm_clock_entry *ce; @@ -79,23 +67,19 @@ static int __pm_clk_add(struct device *dev, const char *con_id, return -ENOMEM; } - if (con_id) { - ce->con_id = kstrdup(con_id, GFP_KERNEL); - if (!ce->con_id) { - dev_err(dev, - "Not enough memory for clock connection ID.\n"); - kfree(ce); - return -ENOMEM; - } - } else { - ce->clk = clk; - } + __clk_get(clk); - pm_clk_acquire(dev, ce); + clk_prepare(clk); + + ce->status = PCE_STATUS_PREPARED; + ce->clk = clk; spin_lock_irq(&psd->lock); list_add_tail(&ce->node, &psd->clock_list); spin_unlock_irq(&psd->lock); + + dev_dbg(dev, "Clock %s managed by runtime PM.\n", __clk_get_name(clk)); + return 0; } @@ -109,19 +93,23 @@ static int __pm_clk_add(struct device *dev, const char *con_id, */ int pm_clk_add(struct device *dev, const char *con_id) { - return __pm_clk_add(dev, con_id, NULL); -} + struct clk *clk; + int retval; -/** - * pm_clk_add_clk - Start using a device clock for power management. - * @dev: Device whose clock is going to be used for power management. - * @clk: Clock pointer - * - * Add the clock to the list of clocks used for the power management of @dev. - */ -int pm_clk_add_clk(struct device *dev, struct clk *clk) -{ - return __pm_clk_add(dev, NULL, clk); + clk = clk_get(dev, con_id); + if (IS_ERR(clk)) { + retval = PTR_ERR(clk); + dev_err(dev, "Failed to locate lock (con_id %s): %d\n", + con_id, retval); + return retval; + } + + retval = pm_clk_add_clk(dev, clk); + + /* pm_clk_add_clk takes its own reference to clk */ + clk_put(clk); + + return retval; } /** @@ -133,32 +121,30 @@ static void __pm_clk_remove(struct pm_clock_entry *ce) if (!ce) return; - if (ce->status < PCE_STATUS_ERROR) { - if (ce->status == PCE_STATUS_ENABLED) - clk_disable(ce->clk); + if (ce->status == PCE_STATUS_ENABLED) + clk_disable(ce->clk); - if (ce->status >= PCE_STATUS_ACQUIRED) { - clk_unprepare(ce->clk); - clk_put(ce->clk); - } + if (ce->status >= PCE_STATUS_ACQUIRED) { + clk_unprepare(ce->clk); + clk_put(ce->clk); } - kfree(ce->con_id); kfree(ce); } /** * pm_clk_remove - Stop using a device clock for power management. * @dev: Device whose clock should not be used for PM any more. - * @con_id: Connection ID of the clock. + * @clk: Clock pointer * - * Remove the clock represented by @con_id from the list of clocks used for - * the power management of @dev. + * Remove the clock from the list of clocks used for the power + * management of @dev. */ -void pm_clk_remove(struct device *dev, const char *con_id) + +void pm_clk_remove_clk(struct device *dev, struct clk *clk) { struct pm_subsys_data *psd = dev_to_psd(dev); - struct pm_clock_entry *ce; + struct pm_clock_entry *ce, *matching_ce = NULL; if (!psd) return; @@ -166,22 +152,35 @@ void pm_clk_remove(struct device *dev, const char *con_id) spin_lock_irq(&psd->lock); list_for_each_entry(ce, &psd->clock_list, node) { - if (!con_id && !ce->con_id) - goto remove; - else if (!con_id || !ce->con_id) - continue; - else if (!strcmp(con_id, ce->con_id)) - goto remove; + if (ce->clk == clk) { + matching_ce = ce; + list_del(&ce->node); + break; + } } spin_unlock_irq(&psd->lock); - return; - remove: - list_del(&ce->node); - spin_unlock_irq(&psd->lock); + __pm_clk_remove(matching_ce); +} + +/** + * pm_clk_remove - Stop using a device clock for power management. + * @dev: Device whose clock should not be used for PM any more. + * @con_id: Connection ID of the clock. + * + * Remove the clock represented by @con_id from the list of clocks used for + * the power management of @dev. + */ +void pm_clk_remove(struct device *dev, const char *con_id) +{ + struct clk *clk; - __pm_clk_remove(ce); + clk = clk_get(dev, con_id); + if (!IS_ERR(clk)) { + pm_clk_remove_clk(dev, clk); + clk_put(clk); + } } /** @@ -266,10 +265,9 @@ int pm_clk_suspend(struct device *dev) spin_lock_irqsave(&psd->lock, flags); list_for_each_entry_reverse(ce, &psd->clock_list, node) { - if (ce->status < PCE_STATUS_ERROR) { - if (ce->status == PCE_STATUS_ENABLED) - clk_disable(ce->clk); - ce->status = PCE_STATUS_ACQUIRED; + if (ce->status == PCE_STATUS_ENABLED) { + clk_disable(ce->clk); + ce->status = PCE_STATUS_PREPARED; } } @@ -297,11 +295,9 @@ int pm_clk_resume(struct device *dev) spin_lock_irqsave(&psd->lock, flags); list_for_each_entry(ce, &psd->clock_list, node) { - if (ce->status < PCE_STATUS_ERROR) { - ret = __pm_clk_enable(dev, ce->clk); - if (!ret) - ce->status = PCE_STATUS_ENABLED; - } + ret = __pm_clk_enable(dev, ce->clk); + if (!ret) + ce->status = PCE_STATUS_ENABLED; } spin_unlock_irqrestore(&psd->lock, flags); @@ -390,10 +386,9 @@ int pm_clk_suspend(struct device *dev) spin_lock_irqsave(&psd->lock, flags); list_for_each_entry_reverse(ce, &psd->clock_list, node) { - if (ce->status < PCE_STATUS_ERROR) { - if (ce->status == PCE_STATUS_ENABLED) - clk_disable(ce->clk); - ce->status = PCE_STATUS_ACQUIRED; + if (ce->status == PCE_STATUS_ENABLED) { + clk_disable(ce->clk); + ce->status = PCE_STATUS_PREPARED; } } @@ -422,11 +417,9 @@ int pm_clk_resume(struct device *dev) spin_lock_irqsave(&psd->lock, flags); list_for_each_entry(ce, &psd->clock_list, node) { - if (ce->status < PCE_STATUS_ERROR) { - ret = __pm_clk_enable(dev, ce->clk); - if (!ret) - ce->status = PCE_STATUS_ENABLED; - } + ret = __pm_clk_enable(dev, ce->clk); + if (!ret) + ce->status = PCE_STATUS_ENABLED; } spin_unlock_irqrestore(&psd->lock, flags);