Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932100AbaJWILM (ORCPT ); Thu, 23 Oct 2014 04:11:12 -0400 Received: from mail-qa0-f47.google.com ([209.85.216.47]:56058 "EHLO mail-qa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754283AbaJWILB convert rfc822-to-8bit (ORCPT ); Thu, 23 Oct 2014 04:11:01 -0400 MIME-Version: 1.0 In-Reply-To: References: <1413809764-21995-1-git-send-email-grygorii.strashko@ti.com> <1413809764-21995-3-git-send-email-grygorii.strashko@ti.com> <5446A065.9050308@gmail.com> <544793B5.6080601@ti.com> Date: Thu, 23 Oct 2014 10:11:00 +0200 Message-ID: Subject: Re: [PATCH v2 2/3] ARM: keystone: pm: switch to use generic pm domains From: Ulf Hansson To: Geert Uytterhoeven Cc: Grygorii Strashko , Santosh Shilimkar , ssantosh@kernel.org, "Rafael J. Wysocki" , Kevin Hilman , Geert Uytterhoeven , "linux-pm@vger.kernel.org" , Rob Herring , Grant Likely , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22 October 2014 17:44, Geert Uytterhoeven wrote: > On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson wrote: >> On 22 October 2014 17:09, Geert Uytterhoeven wrote: >>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson wrote: >>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev) >>>>>>> { >>>>>>> + struct clk *clk; >>>>>>> int ret; >>>>>>> + int i = 0; >>>>>>> >>>>>>> dev_dbg(dev, "%s\n", __func__); >>>>>>> >>>>>>> - ret = pm_generic_runtime_suspend(dev); >>>>>>> - if (ret) >>>>>>> - return ret; >>>>>>> - >>>>>>> - ret = pm_clk_suspend(dev); >>>>>>> + ret = pm_clk_create(dev); >>>>>>> if (ret) { >>>>>>> - pm_generic_runtime_resume(dev); >>>>>>> - return ret; >>>>>>> + dev_err(dev, "pm_clk_create failed %d\n", ret); >>>>>>> + return; >>>>>>> + }; >>>>>>> + >>>>>>> + while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) { >>>>>>> + ret = pm_clk_add_clk(dev, clk); >>>>>>> + if (ret) { >>>>>>> + dev_err(dev, "pm_clk_add_clk failed %d\n", ret); >>>>>>> + goto clk_err; >>>>>>> + }; >>>>>>> } >>>>>>> >>>>>>> - return 0; >>>>>>> + if (!IS_ENABLED(CONFIG_PM_RUNTIME)) { >>>>>> Can we not okkup two seperate callbacks instead of above check ? >>>>>> I don't like this CONFIG check here. Its slightly better version of >>>>>> ifdef in middle of the code. >>>>> >>>>> I've found more-less similar comment on patch >>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform" >>>>> https://lkml.org/lkml/2014/10/17/257 >>>>> >>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk() >>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME) >>>> >>>> I am wondering whether we actually should/could do this, no matter of >>>> CONFIG_PM_RUNTIME. >>>> >>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM >>>> clocks through pm_clk_suspend(), will be gated once the device becomes >>>> runtime PM suspended. Right? >>> >>> Doing it unconditionally means we'll have lots of unneeded clocks running >>> for a short while. > >> As long as the pm_clk_add() is being invoked from the ->attach_dev() >> callback, we are in the probe path. Certainly we would like to have >> clocks enabled while probing, don't you think? >> >> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will >> those be enabled? > > They will be enabled when the driver does > > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > in its .probe() method. No! This doesn't work for drivers which have used pm_runtime_set_active() prior pm_runtime_enable(). That should also be a common good practice for most drivers, otherwise they wouldn’t work unless CONFIG_PM_RUNTIME is enabled. Please have a look at the following patchset, which is fixing up one driver to behave better. http://marc.info/?l=linux-pm&m=141327095713390&w=2 Kind regards Uffe -- 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/