From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs Date: Tue, 17 Mar 2015 04:01:57 +0100 Message-ID: <5508804.qPsCdhOgnd@vostro.rjw.lan> References: <1426261429-31883-1-git-send-email-ulf.hansson@linaro.org> <1566917.2Z45cAj17k@vostro.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7Bit Return-path: Received: from v094114.home.net.pl ([79.96.170.134]:52848 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751515AbbCQCiG (ORCPT ); Mon, 16 Mar 2015 22:38:06 -0400 In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Ulf Hansson Cc: Len Brown , Pavel Machek , Kevin Hilman , "linux-pm@vger.kernel.org" , Geert Uytterhoeven , Dmitry Torokhov , Russell King , Greg Kroah-Hartman , Mark Brown , Wolfram Sang , "linux-arm-kernel@lists.infradead.org" On Monday, March 16, 2015 10:26:46 AM Ulf Hansson wrote: > On 14 March 2015 at 02:31, Rafael J. Wysocki wrote: > > On Friday, March 13, 2015 04:43:41 PM Ulf Hansson wrote: > >> There may be more than one device in a PM domain which then will be > >> probed at different points in time. > >> > >> Depending on timing and runtime PM support, in for the device related > >> driver/subsystem, a PM domain may be advised to power off after a > >> successful probe sequence. > >> > >> A general requirement for a device within a PM domain, is that the > >> PM domain must stay powered during the probe sequence. To cope with > >> such requirement, let's add the dev_pm_domain_get|put() APIs. > >> > >> These APIs are intended to be invoked from subsystem-level code and the > >> calls between get/put needs to be balanced. > >> > >> dev_pm_domain_get(), tells the PM domain that it needs to increase a > >> usage count and to keep supplying power. dev_pm_domain_put(), does the > >> opposite. > >> > >> For a PM domain to support this feature, it must implement the optional > >> ->get|put() callbacks. > >> > >> Signed-off-by: Ulf Hansson > >> --- > >> drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++ > >> include/linux/pm.h | 2 ++ > >> include/linux/pm_domain.h | 4 ++++ > >> 3 files changed, 46 insertions(+) > >> > >> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > >> index f32b802..99225af 100644 > >> --- a/drivers/base/power/common.c > >> +++ b/drivers/base/power/common.c > >> @@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) > >> dev->pm_domain->detach(dev, power_off); > >> } > >> EXPORT_SYMBOL_GPL(dev_pm_domain_detach); > >> + > >> +/** > >> + * dev_pm_domain_get - Increase usage count to keep a PM domain powered. > >> + * @domain: The PM domain to operate on. > >> + * > >> + * This function will not by itself increase the usage count, that's up to each > >> + * PM domain implementation to support. Typically it should be invoked from > >> + * subsystem level code prior drivers starts probing. > >> + * > >> + * Do note, it's optional to implement the ->get() callback for a PM domain. > >> + * > >> + * Returns 0 on successfully increased usage count or negative error code. > >> + */ > >> +int dev_pm_domain_get(struct dev_pm_domain *domain) > >> +{ > >> + int ret = 0; > >> + > >> + if (domain && domain->get) > >> + ret = domain->get(domain); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(dev_pm_domain_get); > >> + > >> +/** > >> + * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off. > >> + * @domain: The PM domain to operate on. > >> + * > >> + * This function will not by itself decrease the usage count, that's up to each > >> + * PM domain implementation to support. Typically it should be invoked from > >> + * subsystem level code after drivers has finished probing. > >> + * > >> + * Do note, it's optional to implement the ->put() callback for a PM domain. > >> + */ > >> +void dev_pm_domain_put(struct dev_pm_domain *domain) > >> +{ > >> + if (domain && domain->put) > >> + domain->put(domain); > >> +} > >> +EXPORT_SYMBOL_GPL(dev_pm_domain_put); > >> diff --git a/include/linux/pm.h b/include/linux/pm.h > >> index e2f1be6..e62330b 100644 > >> --- a/include/linux/pm.h > >> +++ b/include/linux/pm.h > >> @@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev); > >> struct dev_pm_domain { > >> struct dev_pm_ops ops; > >> void (*detach)(struct device *dev, bool power_off); > >> + int (*get)(struct dev_pm_domain *domain); > >> + void (*put)(struct dev_pm_domain *domain); > > > > I don't like these names. They suggest that it's always going to be about > > reference counting which doesn't have to be the case in principle. > > I am happy to change, you don't happen to have a proposal? :-) > > For genpd we already have these related APIs: > > pm_genpd_poweron() > pm_genpd_name_poweron() > pm_genpd_poweroff_unused() > > Theoretically we should be able to replace these with > dev_pm_domain_get|put() or whatever we decide to name them to. > > > > > Also what about calling these from the driver core, ie. really_probe()? > > I like that! > > That also implies moving the calls to dev_pm_domain_attach|detach() > out of the buses and into the drivercore, since we need the PM domain > to be attached before calling dev_pm_domain_get|put(). I assume that's > what you also propose me to change, right? Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent. So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks. The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody. Rafael --- drivers/base/bus.c | 12 +++++++++++- drivers/base/dd.c | 9 +++++++++ include/linux/device.h | 3 +++ include/linux/pm.h | 2 ++ 4 files changed, 25 insertions(+), 1 deletion(-) Index: linux-pm/drivers/base/bus.c =================================================================== --- linux-pm.orig/drivers/base/bus.c +++ linux-pm/drivers/base/bus.c @@ -509,10 +509,15 @@ int bus_add_device(struct device *dev) int error = 0; if (bus) { + if (bus->pm_domain_init) { + error = bus->pm_domain_init(dev); + if (error) + goto out_put; + } pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev)); error = device_add_attrs(bus, dev); if (error) - goto out_put; + goto out_pm_domain; error = device_add_groups(dev, bus->dev_groups); if (error) goto out_groups; @@ -534,6 +539,9 @@ out_groups: device_remove_groups(dev, bus->dev_groups); out_id: device_remove_attrs(bus, dev); +out_pm_domain: + if (bus->pm_domain_exit) + bus->pm_domain_exit(dev); out_put: bus_put(dev->bus); return error; @@ -597,6 +605,8 @@ void bus_remove_device(struct device *de device_remove_groups(dev, dev->bus->dev_groups); if (klist_node_attached(&dev->p->knode_bus)) klist_del(&dev->p->knode_bus); + if (bus->pm_domain_exit) + bus->pm_domain_exit(dev); pr_debug("bus: '%s': remove device %s\n", dev->bus->name, dev_name(dev)); Index: linux-pm/include/linux/device.h =================================================================== --- linux-pm.orig/include/linux/device.h +++ linux-pm/include/linux/device.h @@ -123,6 +123,9 @@ struct bus_type { int (*suspend)(struct device *dev, pm_message_t state); int (*resume)(struct device *dev); + int (*pm_domain_init)(struct device *dev); + void (*pm_domain_exit)(struct device *dev); + const struct dev_pm_ops *pm; const struct iommu_ops *iommu_ops; Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struc struct dev_pm_domain { struct dev_pm_ops ops; void (*detach)(struct device *dev, bool power_off); + int (*pre_probe)(struct device *dev); + void (*post_probe)(struct device *dev); }; /* Index: linux-pm/drivers/base/dd.c =================================================================== --- linux-pm.orig/drivers/base/dd.c +++ linux-pm/drivers/base/dd.c @@ -298,6 +298,12 @@ static int really_probe(struct device *d goto probe_failed; } + if (dev->pm_domain && dev->pm_domain->pre_probe) { + ret = dev->pm_domain->pre_probe(dev); + if (ret) + goto probe_failed; + } + if (dev->bus->probe) { ret = dev->bus->probe(dev); if (ret) @@ -308,6 +314,9 @@ static int really_probe(struct device *d goto probe_failed; } + if (dev->pm_domain && dev->pm_domain->post_probe) + dev->pm_domain->post_probe(dev); + driver_bound(dev); ret = 1; pr_debug("bus: '%s': %s: bound device %s to driver %s\n", From mboxrd@z Thu Jan 1 00:00:00 1970 From: rjw@rjwysocki.net (Rafael J. Wysocki) Date: Tue, 17 Mar 2015 04:01:57 +0100 Subject: [PATCH 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs In-Reply-To: References: <1426261429-31883-1-git-send-email-ulf.hansson@linaro.org> <1566917.2Z45cAj17k@vostro.rjw.lan> Message-ID: <5508804.qPsCdhOgnd@vostro.rjw.lan> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Monday, March 16, 2015 10:26:46 AM Ulf Hansson wrote: > On 14 March 2015 at 02:31, Rafael J. Wysocki wrote: > > On Friday, March 13, 2015 04:43:41 PM Ulf Hansson wrote: > >> There may be more than one device in a PM domain which then will be > >> probed at different points in time. > >> > >> Depending on timing and runtime PM support, in for the device related > >> driver/subsystem, a PM domain may be advised to power off after a > >> successful probe sequence. > >> > >> A general requirement for a device within a PM domain, is that the > >> PM domain must stay powered during the probe sequence. To cope with > >> such requirement, let's add the dev_pm_domain_get|put() APIs. > >> > >> These APIs are intended to be invoked from subsystem-level code and the > >> calls between get/put needs to be balanced. > >> > >> dev_pm_domain_get(), tells the PM domain that it needs to increase a > >> usage count and to keep supplying power. dev_pm_domain_put(), does the > >> opposite. > >> > >> For a PM domain to support this feature, it must implement the optional > >> ->get|put() callbacks. > >> > >> Signed-off-by: Ulf Hansson > >> --- > >> drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++ > >> include/linux/pm.h | 2 ++ > >> include/linux/pm_domain.h | 4 ++++ > >> 3 files changed, 46 insertions(+) > >> > >> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > >> index f32b802..99225af 100644 > >> --- a/drivers/base/power/common.c > >> +++ b/drivers/base/power/common.c > >> @@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) > >> dev->pm_domain->detach(dev, power_off); > >> } > >> EXPORT_SYMBOL_GPL(dev_pm_domain_detach); > >> + > >> +/** > >> + * dev_pm_domain_get - Increase usage count to keep a PM domain powered. > >> + * @domain: The PM domain to operate on. > >> + * > >> + * This function will not by itself increase the usage count, that's up to each > >> + * PM domain implementation to support. Typically it should be invoked from > >> + * subsystem level code prior drivers starts probing. > >> + * > >> + * Do note, it's optional to implement the ->get() callback for a PM domain. > >> + * > >> + * Returns 0 on successfully increased usage count or negative error code. > >> + */ > >> +int dev_pm_domain_get(struct dev_pm_domain *domain) > >> +{ > >> + int ret = 0; > >> + > >> + if (domain && domain->get) > >> + ret = domain->get(domain); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(dev_pm_domain_get); > >> + > >> +/** > >> + * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off. > >> + * @domain: The PM domain to operate on. > >> + * > >> + * This function will not by itself decrease the usage count, that's up to each > >> + * PM domain implementation to support. Typically it should be invoked from > >> + * subsystem level code after drivers has finished probing. > >> + * > >> + * Do note, it's optional to implement the ->put() callback for a PM domain. > >> + */ > >> +void dev_pm_domain_put(struct dev_pm_domain *domain) > >> +{ > >> + if (domain && domain->put) > >> + domain->put(domain); > >> +} > >> +EXPORT_SYMBOL_GPL(dev_pm_domain_put); > >> diff --git a/include/linux/pm.h b/include/linux/pm.h > >> index e2f1be6..e62330b 100644 > >> --- a/include/linux/pm.h > >> +++ b/include/linux/pm.h > >> @@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev); > >> struct dev_pm_domain { > >> struct dev_pm_ops ops; > >> void (*detach)(struct device *dev, bool power_off); > >> + int (*get)(struct dev_pm_domain *domain); > >> + void (*put)(struct dev_pm_domain *domain); > > > > I don't like these names. They suggest that it's always going to be about > > reference counting which doesn't have to be the case in principle. > > I am happy to change, you don't happen to have a proposal? :-) > > For genpd we already have these related APIs: > > pm_genpd_poweron() > pm_genpd_name_poweron() > pm_genpd_poweroff_unused() > > Theoretically we should be able to replace these with > dev_pm_domain_get|put() or whatever we decide to name them to. > > > > > Also what about calling these from the driver core, ie. really_probe()? > > I like that! > > That also implies moving the calls to dev_pm_domain_attach|detach() > out of the buses and into the drivercore, since we need the PM domain > to be attached before calling dev_pm_domain_get|put(). I assume that's > what you also propose me to change, right? Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent. So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks. The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody. Rafael --- drivers/base/bus.c | 12 +++++++++++- drivers/base/dd.c | 9 +++++++++ include/linux/device.h | 3 +++ include/linux/pm.h | 2 ++ 4 files changed, 25 insertions(+), 1 deletion(-) Index: linux-pm/drivers/base/bus.c =================================================================== --- linux-pm.orig/drivers/base/bus.c +++ linux-pm/drivers/base/bus.c @@ -509,10 +509,15 @@ int bus_add_device(struct device *dev) int error = 0; if (bus) { + if (bus->pm_domain_init) { + error = bus->pm_domain_init(dev); + if (error) + goto out_put; + } pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev)); error = device_add_attrs(bus, dev); if (error) - goto out_put; + goto out_pm_domain; error = device_add_groups(dev, bus->dev_groups); if (error) goto out_groups; @@ -534,6 +539,9 @@ out_groups: device_remove_groups(dev, bus->dev_groups); out_id: device_remove_attrs(bus, dev); +out_pm_domain: + if (bus->pm_domain_exit) + bus->pm_domain_exit(dev); out_put: bus_put(dev->bus); return error; @@ -597,6 +605,8 @@ void bus_remove_device(struct device *de device_remove_groups(dev, dev->bus->dev_groups); if (klist_node_attached(&dev->p->knode_bus)) klist_del(&dev->p->knode_bus); + if (bus->pm_domain_exit) + bus->pm_domain_exit(dev); pr_debug("bus: '%s': remove device %s\n", dev->bus->name, dev_name(dev)); Index: linux-pm/include/linux/device.h =================================================================== --- linux-pm.orig/include/linux/device.h +++ linux-pm/include/linux/device.h @@ -123,6 +123,9 @@ struct bus_type { int (*suspend)(struct device *dev, pm_message_t state); int (*resume)(struct device *dev); + int (*pm_domain_init)(struct device *dev); + void (*pm_domain_exit)(struct device *dev); + const struct dev_pm_ops *pm; const struct iommu_ops *iommu_ops; Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struc struct dev_pm_domain { struct dev_pm_ops ops; void (*detach)(struct device *dev, bool power_off); + int (*pre_probe)(struct device *dev); + void (*post_probe)(struct device *dev); }; /* Index: linux-pm/drivers/base/dd.c =================================================================== --- linux-pm.orig/drivers/base/dd.c +++ linux-pm/drivers/base/dd.c @@ -298,6 +298,12 @@ static int really_probe(struct device *d goto probe_failed; } + if (dev->pm_domain && dev->pm_domain->pre_probe) { + ret = dev->pm_domain->pre_probe(dev); + if (ret) + goto probe_failed; + } + if (dev->bus->probe) { ret = dev->bus->probe(dev); if (ret) @@ -308,6 +314,9 @@ static int really_probe(struct device *d goto probe_failed; } + if (dev->pm_domain && dev->pm_domain->post_probe) + dev->pm_domain->post_probe(dev); + driver_bound(dev); ret = 1; pr_debug("bus: '%s': %s: bound device %s to driver %s\n",