| Message ID | 1526639490-12167-10-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PM / Domains: Add support for multi PM domains per device | expand |
On 18/05/18 11:31, Ulf Hansson wrote: > The existing dev_pm_domain_attach() function, allows a single PM domain to > be attached per device. To be able to support devices that are partitioned > across multiple PM domains, let's introduce a new interface, > dev_pm_domain_attach_by_id(). > > The dev_pm_domain_attach_by_id() returns a new allocated struct device with > the corresponding attached PM domain. This enables for example a driver to > operate on the new device from a power management point of view. The driver > may then also benefit from using the received device, to set up so called > device-links towards its original device. Depending on the situation, these > links may then be dynamically changed. > > The new interface is typically called by drivers during their probe phase, > in case they manages devices which uses multiple PM domains. If that is the > case, the driver also becomes responsible of managing the detaching of the > PM domains, which typically should be done at the remove phase. Detaching > is done by calling the existing dev_pm_domain_detach() function and for > each of the received devices from dev_pm_domain_attach_by_id(). > > Note, currently its only genpd that supports multiple PM domains per > device, but dev_pm_domain_attach_by_id() can easily by extended to cover > other PM domain types, if/when needed. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++- > include/linux/pm_domain.h | 7 +++++++ > 2 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > index 7ae62b6..d3db974 100644 > --- a/drivers/base/power/common.c > +++ b/drivers/base/power/common.c > @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool power_on) > EXPORT_SYMBOL_GPL(dev_pm_domain_attach); > > /** > + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains. Isn't this more of a 'get'? > + * @index: The index of the PM domain. > + * @dev: Device to attach. Isn't this just the device associated with the PM domain we are getting? > + * > + * As @dev may only be attached to a single PM domain, the backend PM domain > + * provider should create a virtual device to attach instead. As attachment > + * succeeds, the ->detach() callback in the struct dev_pm_domain should be > + * assigned by the corresponding backend attach function. > + * > + * This function should typically be invoked from drivers during probe phase. > + * Especially for those that manages devices which requires power management > + * through more than one PM domain. > + * > + * Callers must ensure proper synchronization of this function with power > + * management callbacks. > + * > + * Returns the virtual attached device in case successfully attached PM domain, > + * NULL in case @dev don't need a PM domain, else a PTR_ERR(). Should this be 'NULL in the case where the @dev already has a power-domain'? > + */ > +struct device *dev_pm_domain_attach_by_id(struct device *dev, > + unsigned int index) > +{ > + if (dev->pm_domain) I wonder if this is worthy of a ... if (WARN_ON(dev->pm_domain)) > + return NULL; Don't we consider this an error case? I wonder why not return PTR_ERR here as well? This would be consistent with dev_pm_domain_attach(). Cheers Jon -- nvpublic
On 24 May 2018 at 17:48, Jon Hunter <jonathanh@nvidia.com> wrote: > > On 18/05/18 11:31, Ulf Hansson wrote: >> >> The existing dev_pm_domain_attach() function, allows a single PM domain to >> be attached per device. To be able to support devices that are partitioned >> across multiple PM domains, let's introduce a new interface, >> dev_pm_domain_attach_by_id(). >> >> The dev_pm_domain_attach_by_id() returns a new allocated struct device >> with >> the corresponding attached PM domain. This enables for example a driver to >> operate on the new device from a power management point of view. The >> driver >> may then also benefit from using the received device, to set up so called >> device-links towards its original device. Depending on the situation, >> these >> links may then be dynamically changed. >> >> The new interface is typically called by drivers during their probe phase, >> in case they manages devices which uses multiple PM domains. If that is >> the >> case, the driver also becomes responsible of managing the detaching of the >> PM domains, which typically should be done at the remove phase. Detaching >> is done by calling the existing dev_pm_domain_detach() function and for >> each of the received devices from dev_pm_domain_attach_by_id(). >> >> Note, currently its only genpd that supports multiple PM domains per >> device, but dev_pm_domain_attach_by_id() can easily by extended to cover >> other PM domain types, if/when needed. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++- >> include/linux/pm_domain.h | 7 +++++++ >> 2 files changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c >> index 7ae62b6..d3db974 100644 >> --- a/drivers/base/power/common.c >> +++ b/drivers/base/power/common.c >> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool >> power_on) >> EXPORT_SYMBOL_GPL(dev_pm_domain_attach); >> /** >> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains. > > > Isn't this more of a 'get'? I don't think so, at least according to the common understanding of how we use get and put APIs. For example, clk_get() returns a cookie to a clk, which you then can do a hole bunch of different clk specific operations on. This is different, there are no specific PM domain operations the caller can or should do. Instead the idea is to keep drivers more or less transparent, still using runtime PM as before. The only care the caller need to take is to use device links, which BTW isn't a PM domain specific thing. > >> + * @index: The index of the PM domain. >> + * @dev: Device to attach. > > > Isn't this just the device associated with the PM domain we are getting? Correct, but please note, as stated above, I don't think it's a good idea to return a special PM domain cookie, because we don't want the caller to run special PM domain operations. > >> + * >> + * As @dev may only be attached to a single PM domain, the backend PM >> domain >> + * provider should create a virtual device to attach instead. As >> attachment >> + * succeeds, the ->detach() callback in the struct dev_pm_domain should >> be >> + * assigned by the corresponding backend attach function. >> + * >> + * This function should typically be invoked from drivers during probe >> phase. >> + * Especially for those that manages devices which requires power >> management >> + * through more than one PM domain. >> + * >> + * Callers must ensure proper synchronization of this function with power >> + * management callbacks. >> + * >> + * Returns the virtual attached device in case successfully attached PM >> domain, >> + * NULL in case @dev don't need a PM domain, else a PTR_ERR(). > > > Should this be 'NULL in the case where the @dev already has a power-domain'? Yes, I think so. The intent is to be consistent with how dev_pm_domain_attach() works and according to the latest changes I did for it. It's queued for 4.18, please have a look in Rafael's tree and you will see. > >> + */ >> +struct device *dev_pm_domain_attach_by_id(struct device *dev, >> + unsigned int index) >> +{ >> + if (dev->pm_domain) > > > I wonder if this is worthy of a ... > > if (WARN_ON(dev->pm_domain)) > >> + return NULL; > > > Don't we consider this an error case? I wonder why not return PTR_ERR here > as well? This would be consistent with dev_pm_domain_attach(). Please see above comment. Kind regards Uffe
On 24/05/18 22:11, Ulf Hansson wrote: > On 24 May 2018 at 17:48, Jon Hunter <jonathanh@nvidia.com> wrote: >> >> On 18/05/18 11:31, Ulf Hansson wrote: >>> >>> The existing dev_pm_domain_attach() function, allows a single PM domain to >>> be attached per device. To be able to support devices that are partitioned >>> across multiple PM domains, let's introduce a new interface, >>> dev_pm_domain_attach_by_id(). >>> >>> The dev_pm_domain_attach_by_id() returns a new allocated struct device >>> with >>> the corresponding attached PM domain. This enables for example a driver to >>> operate on the new device from a power management point of view. The >>> driver >>> may then also benefit from using the received device, to set up so called >>> device-links towards its original device. Depending on the situation, >>> these >>> links may then be dynamically changed. >>> >>> The new interface is typically called by drivers during their probe phase, >>> in case they manages devices which uses multiple PM domains. If that is >>> the >>> case, the driver also becomes responsible of managing the detaching of the >>> PM domains, which typically should be done at the remove phase. Detaching >>> is done by calling the existing dev_pm_domain_detach() function and for >>> each of the received devices from dev_pm_domain_attach_by_id(). >>> >>> Note, currently its only genpd that supports multiple PM domains per >>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover >>> other PM domain types, if/when needed. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++- >>> include/linux/pm_domain.h | 7 +++++++ >>> 2 files changed, 39 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c >>> index 7ae62b6..d3db974 100644 >>> --- a/drivers/base/power/common.c >>> +++ b/drivers/base/power/common.c >>> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool >>> power_on) >>> EXPORT_SYMBOL_GPL(dev_pm_domain_attach); >>> /** >>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains. >> >> >> Isn't this more of a 'get'? > > I don't think so, at least according to the common understanding of > how we use get and put APIs. > > For example, clk_get() returns a cookie to a clk, which you then can > do a hole bunch of different clk specific operations on. > > This is different, there are no specific PM domain operations the > caller can or should do. Instead the idea is to keep drivers more or > less transparent, still using runtime PM as before. The only care the > caller need to take is to use device links, which BTW isn't a PM > domain specific thing. Yes, but a user can still call pm_runtime_get/put with the device returned if they wish to, right? >> >>> + * @index: The index of the PM domain. >>> + * @dev: Device to attach. >> >> >> Isn't this just the device associated with the PM domain we are getting? > > Correct, but please note, as stated above, I don't think it's a good > idea to return a special PM domain cookie, because we don't want the > caller to run special PM domain operations. > >> >>> + * >>> + * As @dev may only be attached to a single PM domain, the backend PM >>> domain >>> + * provider should create a virtual device to attach instead. As >>> attachment >>> + * succeeds, the ->detach() callback in the struct dev_pm_domain should >>> be >>> + * assigned by the corresponding backend attach function. >>> + * >>> + * This function should typically be invoked from drivers during probe >>> phase. >>> + * Especially for those that manages devices which requires power >>> management >>> + * through more than one PM domain. >>> + * >>> + * Callers must ensure proper synchronization of this function with power >>> + * management callbacks. >>> + * >>> + * Returns the virtual attached device in case successfully attached PM >>> domain, >>> + * NULL in case @dev don't need a PM domain, else a PTR_ERR(). >> >> >> Should this be 'NULL in the case where the @dev already has a power-domain'? > > Yes, I think so. The intent is to be consistent with how > dev_pm_domain_attach() works and according to the latest changes I did > for it. It's queued for 4.18, please have a look in Rafael's tree and > you will see. Ah, I see your change now. >> >>> + */ >>> +struct device *dev_pm_domain_attach_by_id(struct device *dev, >>> + unsigned int index) >>> +{ >>> + if (dev->pm_domain) >> >> >> I wonder if this is worthy of a ... >> >> if (WARN_ON(dev->pm_domain)) >> >>> + return NULL; >> >> >> Don't we consider this an error case? I wonder why not return PTR_ERR here >> as well? This would be consistent with dev_pm_domain_attach(). > > Please see above comment. Right, but this case still seems like an error. My understanding is that only drivers will use this API directly and it will not be used by the device driver core (unlike dev_pm_domain_attach), so if anyone calls this attempting to attach another PM domain when one is already attached, they are doing something wrong. Cheers Jon -- nvpublic
On 25/05/18 11:45, Ulf Hansson wrote: ... >> Right, but this case still seems like an error. My understanding is that >> only drivers will use this API directly and it will not be used by the >> device driver core (unlike dev_pm_domain_attach), so if anyone calls this >> attempting to attach another PM domain when one is already attached, they >> are doing something wrong. > > [...] > > You may be right! > > What I was thinking of is whether multiple PM domains may be optional > in some cases, but instead a PM domain have already been attached by > dev_pm_domain_attach(), prior the driver starts to probe. > > Then, assuming we return an error for this case, that means the caller > then need to check the dev->pm_domain pointer, prior calling > dev_pm_domain_attach_by_id(). Wouldn't it? Perhaps that is more clear > though? IMO the driver should know whether is needs multiple power-domains or not and if it needs multiple then it should just call dev_pm_domain_attach_by_id() N times without needing to checking dev->pm_domain first. If it fails then either the PM domain core did something wrong or power-domains are missing from DT, but either way there is an error, so let it fail. Cheers Jon -- nvpublic
On 25 May 2018 at 13:07, Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 25/05/18 11:45, Ulf Hansson wrote: > > ... > >>> Right, but this case still seems like an error. My understanding is that >>> only drivers will use this API directly and it will not be used by the >>> device driver core (unlike dev_pm_domain_attach), so if anyone calls this >>> attempting to attach another PM domain when one is already attached, they >>> are doing something wrong. >> >> >> [...] >> >> You may be right! >> >> What I was thinking of is whether multiple PM domains may be optional >> in some cases, but instead a PM domain have already been attached by >> dev_pm_domain_attach(), prior the driver starts to probe. >> >> Then, assuming we return an error for this case, that means the caller >> then need to check the dev->pm_domain pointer, prior calling >> dev_pm_domain_attach_by_id(). Wouldn't it? Perhaps that is more clear >> though? > > > IMO the driver should know whether is needs multiple power-domains or not > and if it needs multiple then it should just call > dev_pm_domain_attach_by_id() N times without needing to checking > dev->pm_domain first. If it fails then either the PM domain core did > something wrong or power-domains are missing from DT, but either way there > is an error, so let it fail. Right, sounds reasonable! Kind regards Uffe
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c index 7ae62b6..d3db974 100644 --- a/drivers/base/power/common.c +++ b/drivers/base/power/common.c @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool power_on) EXPORT_SYMBOL_GPL(dev_pm_domain_attach); /** + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains. + * @index: The index of the PM domain. + * @dev: Device to attach. + * + * As @dev may only be attached to a single PM domain, the backend PM domain + * provider should create a virtual device to attach instead. As attachment + * succeeds, the ->detach() callback in the struct dev_pm_domain should be + * assigned by the corresponding backend attach function. + * + * This function should typically be invoked from drivers during probe phase. + * Especially for those that manages devices which requires power management + * through more than one PM domain. + * + * Callers must ensure proper synchronization of this function with power + * management callbacks. + * + * Returns the virtual attached device in case successfully attached PM domain, + * NULL in case @dev don't need a PM domain, else a PTR_ERR(). + */ +struct device *dev_pm_domain_attach_by_id(struct device *dev, + unsigned int index) +{ + if (dev->pm_domain) + return NULL; + + return genpd_dev_pm_attach_by_id(dev, index); +} +EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_id); + +/** * dev_pm_domain_detach - Detach a device from its PM domain. * @dev: Device to detach. * @power_off: Used to indicate whether we should power off the device. * * This functions will reverse the actions from dev_pm_domain_attach() and thus * try to detach the @dev from its PM domain. Typically it should be invoked - * from subsystem level code during the remove phase. + * during the remove phase, either from subsystem level code or from drivers in + * case attaching was done through dev_pm_domain_attach_by_id. * * Callers must ensure proper synchronization of this function with power * management callbacks. diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 82458e8..493ce67 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -299,6 +299,8 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np) #ifdef CONFIG_PM int dev_pm_domain_attach(struct device *dev, bool power_on); +struct device *dev_pm_domain_attach_by_id(struct device *dev, + unsigned int index); void dev_pm_domain_detach(struct device *dev, bool power_off); void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd); #else @@ -306,6 +308,11 @@ static inline int dev_pm_domain_attach(struct device *dev, bool power_on) { return 0; } +static inline struct device *dev_pm_domain_attach_by_id(struct device *dev, + unsigned int index); +{ + return NULL; +} static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {} static inline void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd) {}
The existing dev_pm_domain_attach() function, allows a single PM domain to be attached per device. To be able to support devices that are partitioned across multiple PM domains, let's introduce a new interface, dev_pm_domain_attach_by_id(). The dev_pm_domain_attach_by_id() returns a new allocated struct device with the corresponding attached PM domain. This enables for example a driver to operate on the new device from a power management point of view. The driver may then also benefit from using the received device, to set up so called device-links towards its original device. Depending on the situation, these links may then be dynamically changed. The new interface is typically called by drivers during their probe phase, in case they manages devices which uses multiple PM domains. If that is the case, the driver also becomes responsible of managing the detaching of the PM domains, which typically should be done at the remove phase. Detaching is done by calling the existing dev_pm_domain_detach() function and for each of the received devices from dev_pm_domain_attach_by_id(). Note, currently its only genpd that supports multiple PM domains per device, but dev_pm_domain_attach_by_id() can easily by extended to cover other PM domain types, if/when needed. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++- include/linux/pm_domain.h | 7 +++++++ 2 files changed, 39 insertions(+), 1 deletion(-) -- 2.7.4