[PATCH 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs
Ulf Hansson
ulf.hansson at linaro.org
Tue Mar 17 02:27:03 PDT 2015
On 17 March 2015 at 04:01, Rafael J. Wysocki <rjw at rjwysocki.net> wrote:
> On Monday, March 16, 2015 10:26:46 AM Ulf Hansson wrote:
>> On 14 March 2015 at 02:31, Rafael J. Wysocki <rjw at rjwysocki.net> 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 <ulf.hansson at linaro.org>
>> >> ---
>> >> 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.
Okay!
>
> 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.
Overall, this seem like a very good idea!
>
> 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;
To make this work for bus_add_device(), we first need to change the
registration procedure for genpd DT providers. Currently that's done
when "PM domain drivers" invokes the __of_genpd_add_provider() API,
from SoC specific code and from different init levels.
We need to have the available gendp DT providers registered when the
->pm_domain_init() callback is invoked.
Besides the changes above, genpd also needs to deal with attached
devices, but which don't have a corresponding driver bound. I believe
we have some corner cases to fix around this as well.
As an intermediate step, how about adding the similar code as above
but into really_probe()? For the code in bus_remove_device() below, we
could add that into __device_release_driver()?
> @@ -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",
>
Kind regards
Uffe
More information about the linux-arm-kernel
mailing list