Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031097AbaLLRkJ (ORCPT ); Fri, 12 Dec 2014 12:40:09 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:44302 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030596AbaLLRkG (ORCPT ); Fri, 12 Dec 2014 12:40:06 -0500 From: Laurent Pinchart To: Grygorii Strashko Cc: Grant Likely , Geert Uytterhoeven , ulf.hansson@linaro.org, Kevin Hilman , Mike Turquette , Tomasz Figa , Ben Dooks , Simon Horman , Magnus Damm , "Rafael J. Wysocki" , "linux-sh@vger.kernel.org" , Linux PM list , "devicetree@vger.kernel.org" , "linux-omap@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List Subject: Re: [RFC PATCH 2/2] of/clk: use "clkops-clocks" to specify clocks handled by clock_ops domain Date: Fri, 12 Dec 2014 19:40:52 +0200 Message-ID: <2061380.HdoWyg3PvY@avalon> User-Agent: KMail/4.12.5 (Linux/3.16.5-gentoo; KDE/4.12.5; x86_64; ; ) In-Reply-To: <53D8F24B.7010104@ti.com> References: <1398334403-26181-1-git-send-email-geert+renesas@glider.be> <2717555.fiA6A86F65@avalon> <53D8F24B.7010104@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Grygorii, I've found this mail deep inside my inbox :-) On Wednesday 30 July 2014 16:25:31 Grygorii Strashko wrote: > On 07/30/2014 03:06 AM, Laurent Pinchart wrote: > > On Monday 28 July 2014 23:52:34 Grant Likely wrote: > >> On Mon, Jul 28, 2014 at 11:47 AM, Grygorii Strashko wrote: > >>> On 07/28/2014 05:05 PM, Grant Likely wrote: > >>>> On Thu, 12 Jun 2014 19:53:43 +0300, Grygorii Strashko wrote: > >>>>> Use "clkops-clocks" property to specify clocks handled by > >>>>> clock_ops domain PM domain. Only clocks defined in "clkops-clocks" > >>>>> set of clocks will be handled by Runtime PM through clock_ops > >>>>> Pm domain. > >>>>> > >>>>> Signed-off-by: Grygorii Strashko > >>>>> --- > >>>>> > >>>>> drivers/of/of_clk.c | 7 ++----- > >>>>> 1 file changed, 2 insertions(+), 5 deletions(-) > >>>>> > >>>>> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c > >>>>> index 35f5e9f..5f9b90e 100644 > >>>>> --- a/drivers/of/of_clk.c > >>>>> +++ b/drivers/of/of_clk.c > >>>>> @@ -86,11 +86,8 @@ int of_clk_register_runtime_pm_clocks(struct > >>>>> device_node *np,>>> > >>>>> > >>>>> struct clk *clk; > >>>>> int error; > >>>>> > >>>>> - for (i = 0; (clk = of_clk_get(np, i)) && !IS_ERR(clk); i++) { > >>>>> - if (!clk_may_runtime_pm(clk)) { > >>>>> - clk_put(clk); > >>>>> - continue; > >>>>> - } > >>>>> + for (i = 0; (clk = of_clk_get_from_set(np, "clkops", i)) && > >>>>> + !IS_ERR(clk); i++) { > >>>> > >>>> This really looks like an ABI break to me. What happens to all the > >>>> existing platforms who don't have this new clkops-clocks in their > >>>> device tree? > >>> > >>> Agree. This patch as is will break such platforms. > >>> As possible solution for above problem - the NULL can be used as clock's > >>> prefix by default and platform code can configure new value of clock's > >>> prefix during initialization. > >>> In addition, to make this solution full the of_clk_get_by_name() will > >>> need to be modified too. > >>> > >>> But note pls, this is pure RFC patches which I did to find out the > >>> answer on questions: - What is better: maintain Runtime PM clocks > >>> configuration in DT or in code? > >> > >> In code. I don't think it is workable to embed runtime PM behaviour > >> into the DT bindings. I think there will be too much variance in what > >> hardware requires. We can create helpers to make this simpler, but I > >> don't think it is a good idea to set it up automatically without any > >> control from the driver itself. > >> > >>> - Where and when to call of_clk_register_runtime_pm_clocks()? > >>> > >>> Bus notifier/ platform core/ device drivers > >> > >> I would say in device drivers. > > > > I tend to agree with that. > > > > It will help here to take a step back and remember what the problem we're > > trying to solve is. > > > > At the root is clock management. Our system comprise many clocks, and they > > need to be handled. The Common Clock Framework nicely models the clocks, > > and offers an API for drivers to retrieve device clocks and control them. > > Drivers can thus implement clock management manually without much pain. > > > > A clock can be managed in roughly three different ways : > > > > - it can be enabled at probe time and disabled at remove time ; > > > > - it can be enabled right before the device leaves its idle state and > > disabled when the device goes back to idle ; or > > > > - it can be enabled and disabled in a more fine-grained, device-specific > > manner. > > > > The selected clock management granularity depends on constraints specific > > to the device and on how aggressive power saving needs to be. Enabling > > the clocks at probe time and disabling them at remove time is enough for > > most devices, but leads to a high power consumption. For that reason the > > second clock management scheme is often desired. > > > > Managing clocks manually in the driver is a valid option. However, when > > adding runtime PM to the equation, and realizing that the clocks need to > > be enabled in the runtime PM resume handler and disabled in the suspend > > handler, the clock management code starts looking very similar in most > > drivers. We're thus tempted to factorize it away from the drivers into a > > shared location. > > > > It's important to note at this point that the goal here is only to > > simplify drivers. Moving clock management code out of the drivers doesn't > > (unless I'm missing something) open the door to new possibilities, it just > > serves as a simplification. > > > > Now, as Grygorii mentioned, differences between how a given IP core is > > integrated in various SoCs can make clock management SoC-dependent. In the > > vast majority of cases (which is really what we need to target, given that > > our target is simplifying drivers) SoC integration can be described as a > > list of clocks that must be managed. That list can be common to all > > devices in a given SoC, or can be device-dependent as well. > > That's actually a problem - now we have static list of managed clocks > per-SoC and not per device. > > > Few locations can be used to express a per-device list of per-SoC clocks. > > We can have clocks lists in a per-SoC and per-device location, per-device > > clocks lists in an SoC-specific location, or per-SoC clocks lists in a > > device- specific location. > > > > The first option would require listing clocks to be managed by runtime PM > > in DT nodes, as proposed by this patch set. I don't think this is the > > best option, as that information is a mix of hardware description and > > software policy, with the hardware description part being already present > > in DT in the clocks property. > > I'm not fully agree here. The clock is "functional clock" If It's managed by > runtime PM. And all such clocks need to be enabled/disabled always when > device is powered on/off. So, from my point of view it's HW description and > it follows TRM. > > Other clocks are optional That's actually use-case dependent, some of them might be mandatory. > and only drivers should control them. > And question is how to define sets of such clocks in the best way? > > > The second option calls for storing the lists in SoC code under arch/. As > > we're trying to minimize the amount of SoC code there (and even remove SoC > > code completely when possible) I don't think that's a good option. > > > > The third option would require storing the clocks lists in device drivers. > > I believe this is our best option, as a trade-off between simplicity and > > versatility. Drivers that use runtime PM already need to enable it > > explicitly when probing devices. Passing a list of clock names to runtime > > PM at that point wouldn't complicate drivers much. When the clocks list > > isn't SoC- dependent it could be stored as static information. Otherwise > > it could be derived from DT (or any other source of hardware description) > > using C code, offering all the versatility we need. > > Ok. if I understand right, you propose smth like this: > 1) DT based solution: > > devA { > clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>; > rpm-clocks = <&clkpa>, <&clkcpgmac>; > - or - > clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>; > clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk"; > rpm-clocks = "clk_pa", "clk_cpgmac"; > } On a side note I believe the "rpm-clocks" name is too tied to the Linux implementation. A name similar to "functional-clocks" would be better. > in driver: > pm_runtime_enable(); > > |- of_clk_register_runtime_pm_clocks() > > - or - > of_clk_register_runtime_pm_clocks() > pm_runtime_enable(); I prefer the second option, as an explicit opt-in is less likely to cause regressions, and would also offer an easy way for drivers to opt-out. > 2) Static solution: > char *con_ids_davinci[] = > { "fck", "master", "slave", NULL }; > char *con_ids_keystone[] = > { "clk_pa", "clk_cpgmac" }; > > static struct of_device_id of_match[] = { > { .compatible = "ti,keystone", con_ids_keystone}, > { .compatible = "ti,davinci", con_ids_davinci}, > {}, > }; > > Personally, I like option 1 and, seems, it will not break ABI. Is option 2 really representative of most use cases ? The list of clock inputs to an IP core is a property of the IP core itself. How those inputs are connected in the SoC is a property of the SoC integration. The clocks references in DT can thus vary per-SoC, but the clock names should be pretty much constant for a given IP core. Thus, if we have a single list of clocks to manager for a given IP core, it shouldn't be difficult to pass that list to the of_clk_register_runtime_pm_clocks() function. > > The only drawback of this solution I can think of right now is that the > > runtime PM core couldn't manage device clocks before probing the device. > > Specifically device clocks couldn't be managed if no driver is loaded for > > that device. I somehow recall that someone raised this as being a > > problem, but I can't remember why. > > I can recollect only OMAP2+ SoCs where some abstraction called HW_MOD is > used during platform initialization to reset all devices and turn off > unused ones before probing the devices. But clock_ops are not used by > OMAP2+:) -- Regards, Laurent Pinchart -- 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/