Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752783AbaJYUEl (ORCPT ); Sat, 25 Oct 2014 16:04:41 -0400 Received: from mail-qg0-f50.google.com ([209.85.192.50]:53160 "EHLO mail-qg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752426AbaJYUEk (ORCPT ); Sat, 25 Oct 2014 16:04:40 -0400 MIME-Version: 1.0 In-Reply-To: <20141024163901.GD19933@dtor-ws> References: <1413809764-21995-3-git-send-email-grygorii.strashko@ti.com> <5446A065.9050308@gmail.com> <544793B5.6080601@ti.com> <544912AB.3050801@ti.com> <20141024163901.GD19933@dtor-ws> Date: Sat, 25 Oct 2014 12:45:23 +0200 Message-ID: Subject: Re: [PATCH v2 2/3] ARM: keystone: pm: switch to use generic pm domains From: Ulf Hansson To: Dmitry Torokhov Cc: Grygorii Strashko , Geert Uytterhoeven , 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 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 24 October 2014 18:39, Dmitry Torokhov wrote: > On Fri, Oct 24, 2014 at 11:53:05AM +0200, Ulf Hansson wrote: >> On 23 October 2014 16:37, Grygorii Strashko wrote: >> > Hi Ulf, >> > >> > On 10/23/2014 11:11 AM, Ulf Hansson wrote: >> >> 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(). >> > >> > Sorry, but some misunderstanding is here: >> > 1) If some code call pm_runtime_set_active() it has to ensure >> > that all PM resources switched to ON state. All! So, it will >> > be ok to call enable & get after that - these functions will only >> > adjust counters. >> >> Correct. >> >> This is also the key problem with your approach. You requires a >> pm_runtime_get_sync() to trigger the runtime PM resume callbacks to be >> invoked. That's a fragile design. > > Why is this fragile design? Having pm_runtime_get_sync() result in > resuming the device (and in turn the PM domain it is in) if device is > suspended is the proper behavior, no? It's fragile, because the device may very well be "runtime PM active" at the point when we invoke pm_runtime_get_sync(). Thus the runtime PM resume callback isn't invoked, which is a requirement for these cases to work. 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/