| Message ID | 5508804.qPsCdhOgnd@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17 March 2015 at 04:01, Rafael J. Wysocki <rjw@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@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@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
On Tue, Mar 17, 2015 at 03:45:55PM +0100, Rafael J. Wysocki wrote: > Well, I wouldn't really like to add new callbacks to struct bus_type for > intermediate steps, because that's guaranteed to lead to confusion. > > So I think the infrastructure is better added first and users may be > switched over to it gradually. > > I don't see any particular problems with moving the ACPI PM domain > attach/detach to bus_add/remove_device(), so that can be done as the first > step. As for genpd, it can implement the ->post_probe thing first and do > the rest in the bus type ->probe until the generic code is ready. At what point do you see the genpd binding taking place - remembering that this can fail with -EPROBE_DEFER when DT specifies a genpd, but the domain hasn't been registered yet? I'm guessing that needs to happen either in the bus type ->probe or the ->pre_probe callback. ->pre_probe sounds better as it can be a standard genpd function which bus types hook directly into that method.
On 17 March 2015 at 15:45, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, March 17, 2015 10:27:03 AM Ulf Hansson wrote: >> On 17 March 2015 at 04:01, Rafael J. Wysocki <rjw@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@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@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()? > > Well, I wouldn't really like to add new callbacks to struct bus_type for > intermediate steps, because that's guaranteed to lead to confusion. > > So I think the infrastructure is better added first and users may be > switched over to it gradually. > > I don't see any particular problems with moving the ACPI PM domain > attach/detach to bus_add/remove_device(), so that can be done as the first > step. As for genpd, it can implement the ->post_probe thing first and do > the rest in the bus type ->probe until the generic code is ready. Yes, that works. I will cook a new version of the patchset according to your suggestions. Kind regards Uffe
On Tuesday, March 17, 2015 10:27:03 AM Ulf Hansson wrote: > On 17 March 2015 at 04:01, Rafael J. Wysocki <rjw@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@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@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()? Well, I wouldn't really like to add new callbacks to struct bus_type for intermediate steps, because that's guaranteed to lead to confusion. So I think the infrastructure is better added first and users may be switched over to it gradually. I don't see any particular problems with moving the ACPI PM domain attach/detach to bus_add/remove_device(), so that can be done as the first step. As for genpd, it can implement the ->post_probe thing first and do the rest in the bus type ->probe until the generic code is ready.
On Tuesday, March 17, 2015 03:40:47 PM Ulf Hansson wrote: > On 17 March 2015 at 15:45, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, March 17, 2015 10:27:03 AM Ulf Hansson wrote: > >> On 17 March 2015 at 04:01, Rafael J. Wysocki <rjw@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@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@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()? > > > > Well, I wouldn't really like to add new callbacks to struct bus_type for > > intermediate steps, because that's guaranteed to lead to confusion. > > > > So I think the infrastructure is better added first and users may be > > switched over to it gradually. > > > > I don't see any particular problems with moving the ACPI PM domain > > attach/detach to bus_add/remove_device(), so that can be done as the first > > step. As for genpd, it can implement the ->post_probe thing first and do > > the rest in the bus type ->probe until the generic code is ready. > > Yes, that works. > > I will cook a new version of the patchset according to your suggestions. I'll send a cleaner version of my patch tomorrow (I actually would like to use different names for the new callbacks). Also I can do the ACPI part of the work, please let me know if you want me to.
On Tuesday, March 17, 2015 02:25:55 PM Russell King - ARM Linux wrote: > On Tue, Mar 17, 2015 at 03:45:55PM +0100, Rafael J. Wysocki wrote: > > Well, I wouldn't really like to add new callbacks to struct bus_type for > > intermediate steps, because that's guaranteed to lead to confusion. > > > > So I think the infrastructure is better added first and users may be > > switched over to it gradually. > > > > I don't see any particular problems with moving the ACPI PM domain > > attach/detach to bus_add/remove_device(), so that can be done as the first > > step. As for genpd, it can implement the ->post_probe thing first and do > > the rest in the bus type ->probe until the generic code is ready. > > At what point do you see the genpd binding taking place - remembering > that this can fail with -EPROBE_DEFER when DT specifies a genpd, but > the domain hasn't been registered yet? That -EPROBE_DEFER needs to be returned from the ->pre_probe callback I think. Otherwise bus types would need to duplicate that code. > I'm guessing that needs to happen either in the bus type ->probe or > the ->pre_probe callback. ->pre_probe sounds better as it can be > a standard genpd function which bus types hook directly into that > method. Right. Still, I'd like the bus type's (new) ->pm_domain_init (or whatever it will be eventually) to be used for populating the devices' ->pm_domain pointers. There are two reasons for that. First, those pointers only need to be populated once per device life cycle (and not during every driver probe/remove). Second, that will make limited power management of devices without drivers possible (at least in the ACPI case). Rafael
On 18 March 2015 at 02:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, March 17, 2015 03:40:47 PM Ulf Hansson wrote: >> On 17 March 2015 at 15:45, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > On Tuesday, March 17, 2015 10:27:03 AM Ulf Hansson wrote: >> >> On 17 March 2015 at 04:01, Rafael J. Wysocki <rjw@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@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@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()? >> > >> > Well, I wouldn't really like to add new callbacks to struct bus_type for >> > intermediate steps, because that's guaranteed to lead to confusion. >> > >> > So I think the infrastructure is better added first and users may be >> > switched over to it gradually. >> > >> > I don't see any particular problems with moving the ACPI PM domain >> > attach/detach to bus_add/remove_device(), so that can be done as the first >> > step. As for genpd, it can implement the ->post_probe thing first and do >> > the rest in the bus type ->probe until the generic code is ready. >> >> Yes, that works. >> >> I will cook a new version of the patchset according to your suggestions. > > I'll send a cleaner version of my patch tomorrow (I actually would like to > use different names for the new callbacks). Okay, great! > > Also I can do the ACPI part of the work, please let me know if you want me to. Appreciate your help! I wouldn't mind giving this a quick try. If I am way off, just nack the patches and then please help out implement it. Kind regards Uffe
On Wednesday, March 18, 2015 02:41:47 PM Ulf Hansson wrote: > On 18 March 2015 at 02:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, March 17, 2015 03:40:47 PM Ulf Hansson wrote: > >> On 17 March 2015 at 15:45, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > On Tuesday, March 17, 2015 10:27:03 AM Ulf Hansson wrote: > >> >> On 17 March 2015 at 04:01, Rafael J. Wysocki <rjw@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@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@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()? > >> > > >> > Well, I wouldn't really like to add new callbacks to struct bus_type for > >> > intermediate steps, because that's guaranteed to lead to confusion. > >> > > >> > So I think the infrastructure is better added first and users may be > >> > switched over to it gradually. > >> > > >> > I don't see any particular problems with moving the ACPI PM domain > >> > attach/detach to bus_add/remove_device(), so that can be done as the first > >> > step. As for genpd, it can implement the ->post_probe thing first and do > >> > the rest in the bus type ->probe until the generic code is ready. > >> > >> Yes, that works. > >> > >> I will cook a new version of the patchset according to your suggestions. > > > > I'll send a cleaner version of my patch tomorrow (I actually would like to > > use different names for the new callbacks). > > Okay, great! OK, sent this one (https://patchwork.kernel.org/patch/6040241/). > > Also I can do the ACPI part of the work, please let me know if you want me to. > > Appreciate your help! I wouldn't mind giving this a quick try. If I am > way off, just nack the patches and then please help out implement it. OK
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",