From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Date: Sat, 26 Apr 2014 01:59:40 +0000 Subject: Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core Message-Id: <535B130C.1000007@gmail.com> List-Id: References: <1398334403-26181-1-git-send-email-geert+renesas@glider.be> <1398334403-26181-4-git-send-email-geert+renesas@glider.be> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On 24.04.2014 15:11, Ulf Hansson wrote: > On 24 April 2014 12:13, Geert Uytterhoeven wrote: >> When adding a device from DT, check if its clocks are suitable for Runtime >> PM, and register them with the PM core. >> If Runtime PM is disabled, just enable the clock. >> >> This allows the PM core to automatically manage gate clocks of devices for >> Runtime PM. > > Normally I don't think it's a good idea to "automatically" manage > clocks from PM core or any other place but from the driver (and > possibly the subsystem). > > The reason is simply that we hide things that normally is supposed to > be handled by the driver. Typically a cross SOC driver should work > fine both with and without a pm_domain. It should also not rely on > CONFIG_PM_RUNTIME. > >> >> Signed-off-by: Geert Uytterhoeven >> --- >> drivers/of/Makefile | 1 + >> drivers/of/of_clk.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/of/platform.c | 3 ++ >> include/linux/of_clk.h | 18 +++++++++ >> 4 files changed, 125 insertions(+) >> create mode 100644 drivers/of/of_clk.c >> create mode 100644 include/linux/of_clk.h >> >> diff --git a/drivers/of/Makefile b/drivers/of/Makefile >> index ed9660adad77..49bcd413906f 100644 >> --- a/drivers/of/Makefile >> +++ b/drivers/of/Makefile >> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o >> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o >> obj-$(CONFIG_OF_MTD) += of_mtd.o >> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o >> +obj-$(CONFIG_COMMON_CLK) += of_clk.o >> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c >> new file mode 100644 >> index 000000000000..35f5e9f3dd42 >> --- /dev/null >> +++ b/drivers/of/of_clk.c >> @@ -0,0 +1,103 @@ >> +/* >> + * Copyright (C) 2014 Glider bvba >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> + >> +#ifdef CONFIG_PM_RUNTIME >> + >> +static int of_clk_pm_runtime_suspend(struct device *dev) >> +{ >> + int ret; >> + >> + ret = pm_generic_runtime_suspend(dev); >> + if (ret) >> + return ret; >> + >> + ret = pm_clk_suspend(dev); >> + if (ret) { >> + pm_generic_runtime_resume(dev); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int of_clk_pm_runtime_resume(struct device *dev) >> +{ >> + pm_clk_resume(dev); >> + return pm_generic_runtime_resume(dev); >> +} >> + >> +static struct dev_pm_domain of_clk_pm_domain = { >> + .ops = { >> + .runtime_suspend = of_clk_pm_runtime_suspend, >> + .runtime_resume = of_clk_pm_runtime_resume, >> + USE_PLATFORM_PM_SLEEP_OPS >> + }, >> +}; >> + >> +static int of_clk_register(struct device *dev, struct clk *clk) >> +{ >> + int error; >> + >> + if (!dev->pm_domain) { >> + error = pm_clk_create(dev); >> + if (error) >> + return error; >> + >> + dev->pm_domain = &of_clk_pm_domain; > > I am concerned about how this will work in conjunction with the > generic power domain. > > A device can't reside in more than one pm_domain; thus I think it > would be better to always use the generic power domain and not have a > specific one for clocks. Typically the genpd should invoke > pm_clk_resume|suspend from it's runtime PM callbacks. I'm not sure about this. A typical use case would be to gate clocks ASAP and then wait until device is idle long enough to consider turning off the power domain worthwhile. Also sometimes we may want to gate the clocks, but prevent power domain from being powered off to retain hardware state (e.g. because there is no way to read it and restore later). I believe, though, that for devices that are not inside a controllable power domain, this might be a good solution. Best regards, Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751249AbaDZB7r (ORCPT ); Fri, 25 Apr 2014 21:59:47 -0400 Received: from mail-ee0-f51.google.com ([74.125.83.51]:48255 "EHLO mail-ee0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750840AbaDZB7n (ORCPT ); Fri, 25 Apr 2014 21:59:43 -0400 Message-ID: <535B130C.1000007@gmail.com> Date: Sat, 26 Apr 2014 03:59:40 +0200 From: Tomasz Figa User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Ulf Hansson , Geert Uytterhoeven CC: Magnus Damm , Simon Horman , Laurent Pinchart , Ben Dooks , Felipe Balbi , Mike Turquette , "Rafael J. Wysocki" , linux-sh@vger.kernel.org, "linux-pm@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , linux-omap , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core References: <1398334403-26181-1-git-send-email-geert+renesas@glider.be> <1398334403-26181-4-git-send-email-geert+renesas@glider.be> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 24.04.2014 15:11, Ulf Hansson wrote: > On 24 April 2014 12:13, Geert Uytterhoeven wrote: >> When adding a device from DT, check if its clocks are suitable for Runtime >> PM, and register them with the PM core. >> If Runtime PM is disabled, just enable the clock. >> >> This allows the PM core to automatically manage gate clocks of devices for >> Runtime PM. > > Normally I don't think it's a good idea to "automatically" manage > clocks from PM core or any other place but from the driver (and > possibly the subsystem). > > The reason is simply that we hide things that normally is supposed to > be handled by the driver. Typically a cross SOC driver should work > fine both with and without a pm_domain. It should also not rely on > CONFIG_PM_RUNTIME. > >> >> Signed-off-by: Geert Uytterhoeven >> --- >> drivers/of/Makefile | 1 + >> drivers/of/of_clk.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/of/platform.c | 3 ++ >> include/linux/of_clk.h | 18 +++++++++ >> 4 files changed, 125 insertions(+) >> create mode 100644 drivers/of/of_clk.c >> create mode 100644 include/linux/of_clk.h >> >> diff --git a/drivers/of/Makefile b/drivers/of/Makefile >> index ed9660adad77..49bcd413906f 100644 >> --- a/drivers/of/Makefile >> +++ b/drivers/of/Makefile >> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o >> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o >> obj-$(CONFIG_OF_MTD) += of_mtd.o >> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o >> +obj-$(CONFIG_COMMON_CLK) += of_clk.o >> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c >> new file mode 100644 >> index 000000000000..35f5e9f3dd42 >> --- /dev/null >> +++ b/drivers/of/of_clk.c >> @@ -0,0 +1,103 @@ >> +/* >> + * Copyright (C) 2014 Glider bvba >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> + >> +#ifdef CONFIG_PM_RUNTIME >> + >> +static int of_clk_pm_runtime_suspend(struct device *dev) >> +{ >> + int ret; >> + >> + ret = pm_generic_runtime_suspend(dev); >> + if (ret) >> + return ret; >> + >> + ret = pm_clk_suspend(dev); >> + if (ret) { >> + pm_generic_runtime_resume(dev); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int of_clk_pm_runtime_resume(struct device *dev) >> +{ >> + pm_clk_resume(dev); >> + return pm_generic_runtime_resume(dev); >> +} >> + >> +static struct dev_pm_domain of_clk_pm_domain = { >> + .ops = { >> + .runtime_suspend = of_clk_pm_runtime_suspend, >> + .runtime_resume = of_clk_pm_runtime_resume, >> + USE_PLATFORM_PM_SLEEP_OPS >> + }, >> +}; >> + >> +static int of_clk_register(struct device *dev, struct clk *clk) >> +{ >> + int error; >> + >> + if (!dev->pm_domain) { >> + error = pm_clk_create(dev); >> + if (error) >> + return error; >> + >> + dev->pm_domain = &of_clk_pm_domain; > > I am concerned about how this will work in conjunction with the > generic power domain. > > A device can't reside in more than one pm_domain; thus I think it > would be better to always use the generic power domain and not have a > specific one for clocks. Typically the genpd should invoke > pm_clk_resume|suspend from it's runtime PM callbacks. I'm not sure about this. A typical use case would be to gate clocks ASAP and then wait until device is idle long enough to consider turning off the power domain worthwhile. Also sometimes we may want to gate the clocks, but prevent power domain from being powered off to retain hardware state (e.g. because there is no way to read it and restore later). I believe, though, that for devices that are not inside a controllable power domain, this might be a good solution. Best regards, Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core Date: Sat, 26 Apr 2014 03:59:40 +0200 Message-ID: <535B130C.1000007@gmail.com> References: <1398334403-26181-1-git-send-email-geert+renesas@glider.be> <1398334403-26181-4-git-send-email-geert+renesas@glider.be> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Ulf Hansson , Geert Uytterhoeven Cc: "devicetree@vger.kernel.org" , Mike Turquette , Simon Horman , linux-sh@vger.kernel.org, "Rafael J. Wysocki" , "linux-pm@vger.kernel.org" , Magnus Damm , "linux-kernel@vger.kernel.org" , Felipe Balbi , Ben Dooks , Laurent Pinchart , linux-omap , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org On 24.04.2014 15:11, Ulf Hansson wrote: > On 24 April 2014 12:13, Geert Uytterhoeven wrote: >> When adding a device from DT, check if its clocks are suitable for Runtime >> PM, and register them with the PM core. >> If Runtime PM is disabled, just enable the clock. >> >> This allows the PM core to automatically manage gate clocks of devices for >> Runtime PM. > > Normally I don't think it's a good idea to "automatically" manage > clocks from PM core or any other place but from the driver (and > possibly the subsystem). > > The reason is simply that we hide things that normally is supposed to > be handled by the driver. Typically a cross SOC driver should work > fine both with and without a pm_domain. It should also not rely on > CONFIG_PM_RUNTIME. > >> >> Signed-off-by: Geert Uytterhoeven >> --- >> drivers/of/Makefile | 1 + >> drivers/of/of_clk.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/of/platform.c | 3 ++ >> include/linux/of_clk.h | 18 +++++++++ >> 4 files changed, 125 insertions(+) >> create mode 100644 drivers/of/of_clk.c >> create mode 100644 include/linux/of_clk.h >> >> diff --git a/drivers/of/Makefile b/drivers/of/Makefile >> index ed9660adad77..49bcd413906f 100644 >> --- a/drivers/of/Makefile >> +++ b/drivers/of/Makefile >> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o >> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o >> obj-$(CONFIG_OF_MTD) += of_mtd.o >> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o >> +obj-$(CONFIG_COMMON_CLK) += of_clk.o >> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c >> new file mode 100644 >> index 000000000000..35f5e9f3dd42 >> --- /dev/null >> +++ b/drivers/of/of_clk.c >> @@ -0,0 +1,103 @@ >> +/* >> + * Copyright (C) 2014 Glider bvba >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> + >> +#ifdef CONFIG_PM_RUNTIME >> + >> +static int of_clk_pm_runtime_suspend(struct device *dev) >> +{ >> + int ret; >> + >> + ret = pm_generic_runtime_suspend(dev); >> + if (ret) >> + return ret; >> + >> + ret = pm_clk_suspend(dev); >> + if (ret) { >> + pm_generic_runtime_resume(dev); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int of_clk_pm_runtime_resume(struct device *dev) >> +{ >> + pm_clk_resume(dev); >> + return pm_generic_runtime_resume(dev); >> +} >> + >> +static struct dev_pm_domain of_clk_pm_domain = { >> + .ops = { >> + .runtime_suspend = of_clk_pm_runtime_suspend, >> + .runtime_resume = of_clk_pm_runtime_resume, >> + USE_PLATFORM_PM_SLEEP_OPS >> + }, >> +}; >> + >> +static int of_clk_register(struct device *dev, struct clk *clk) >> +{ >> + int error; >> + >> + if (!dev->pm_domain) { >> + error = pm_clk_create(dev); >> + if (error) >> + return error; >> + >> + dev->pm_domain = &of_clk_pm_domain; > > I am concerned about how this will work in conjunction with the > generic power domain. > > A device can't reside in more than one pm_domain; thus I think it > would be better to always use the generic power domain and not have a > specific one for clocks. Typically the genpd should invoke > pm_clk_resume|suspend from it's runtime PM callbacks. I'm not sure about this. A typical use case would be to gate clocks ASAP and then wait until device is idle long enough to consider turning off the power domain worthwhile. Also sometimes we may want to gate the clocks, but prevent power domain from being powered off to retain hardware state (e.g. because there is no way to read it and restore later). I believe, though, that for devices that are not inside a controllable power domain, this might be a good solution. Best regards, Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 From: tomasz.figa@gmail.com (Tomasz Figa) Date: Sat, 26 Apr 2014 03:59:40 +0200 Subject: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core In-Reply-To: References: <1398334403-26181-1-git-send-email-geert+renesas@glider.be> <1398334403-26181-4-git-send-email-geert+renesas@glider.be> Message-ID: <535B130C.1000007@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 24.04.2014 15:11, Ulf Hansson wrote: > On 24 April 2014 12:13, Geert Uytterhoeven wrote: >> When adding a device from DT, check if its clocks are suitable for Runtime >> PM, and register them with the PM core. >> If Runtime PM is disabled, just enable the clock. >> >> This allows the PM core to automatically manage gate clocks of devices for >> Runtime PM. > > Normally I don't think it's a good idea to "automatically" manage > clocks from PM core or any other place but from the driver (and > possibly the subsystem). > > The reason is simply that we hide things that normally is supposed to > be handled by the driver. Typically a cross SOC driver should work > fine both with and without a pm_domain. It should also not rely on > CONFIG_PM_RUNTIME. > >> >> Signed-off-by: Geert Uytterhoeven >> --- >> drivers/of/Makefile | 1 + >> drivers/of/of_clk.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/of/platform.c | 3 ++ >> include/linux/of_clk.h | 18 +++++++++ >> 4 files changed, 125 insertions(+) >> create mode 100644 drivers/of/of_clk.c >> create mode 100644 include/linux/of_clk.h >> >> diff --git a/drivers/of/Makefile b/drivers/of/Makefile >> index ed9660adad77..49bcd413906f 100644 >> --- a/drivers/of/Makefile >> +++ b/drivers/of/Makefile >> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o >> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o >> obj-$(CONFIG_OF_MTD) += of_mtd.o >> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o >> +obj-$(CONFIG_COMMON_CLK) += of_clk.o >> diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c >> new file mode 100644 >> index 000000000000..35f5e9f3dd42 >> --- /dev/null >> +++ b/drivers/of/of_clk.c >> @@ -0,0 +1,103 @@ >> +/* >> + * Copyright (C) 2014 Glider bvba >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> + >> +#ifdef CONFIG_PM_RUNTIME >> + >> +static int of_clk_pm_runtime_suspend(struct device *dev) >> +{ >> + int ret; >> + >> + ret = pm_generic_runtime_suspend(dev); >> + if (ret) >> + return ret; >> + >> + ret = pm_clk_suspend(dev); >> + if (ret) { >> + pm_generic_runtime_resume(dev); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int of_clk_pm_runtime_resume(struct device *dev) >> +{ >> + pm_clk_resume(dev); >> + return pm_generic_runtime_resume(dev); >> +} >> + >> +static struct dev_pm_domain of_clk_pm_domain = { >> + .ops = { >> + .runtime_suspend = of_clk_pm_runtime_suspend, >> + .runtime_resume = of_clk_pm_runtime_resume, >> + USE_PLATFORM_PM_SLEEP_OPS >> + }, >> +}; >> + >> +static int of_clk_register(struct device *dev, struct clk *clk) >> +{ >> + int error; >> + >> + if (!dev->pm_domain) { >> + error = pm_clk_create(dev); >> + if (error) >> + return error; >> + >> + dev->pm_domain = &of_clk_pm_domain; > > I am concerned about how this will work in conjunction with the > generic power domain. > > A device can't reside in more than one pm_domain; thus I think it > would be better to always use the generic power domain and not have a > specific one for clocks. Typically the genpd should invoke > pm_clk_resume|suspend from it's runtime PM callbacks. I'm not sure about this. A typical use case would be to gate clocks ASAP and then wait until device is idle long enough to consider turning off the power domain worthwhile. Also sometimes we may want to gate the clocks, but prevent power domain from being powered off to retain hardware state (e.g. because there is no way to read it and restore later). I believe, though, that for devices that are not inside a controllable power domain, this might be a good solution. Best regards, Tomasz