| Message ID | 1471340976-5379-10-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote: > It is possible that a device has more than one provider of PM domains > and to support the removal of a PM domain by provider, it is necessary > to store a reference to the provider in the PM domain structure. > Therefore, store a reference to the firmware node handle in the PM > domain structure and populate it when providers (only device-tree based > providers are currently supported by PM domains) are registered. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/base/power/domain.c | 18 ++++++++++++++---- > include/linux/pm_domain.h | 1 + > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 72b973539205..0bc145e8e902 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1539,6 +1539,8 @@ int of_genpd_add_provider_simple(struct device_node *np, > return -EINVAL; > } > > + genpd->provider = &np->fwnode; > + > ret = genpd_add_provider(np, genpd_xlate_simple, genpd); I guess you want to reset genpd->provider = NULL, when genpd_add_provider() fails!? Perhaps better to assign genpd->provider when you know genpd_add_provider() has succeeded. > > mutex_unlock(&gpd_list_lock); > @@ -1564,10 +1566,10 @@ int of_genpd_add_provider_onecell(struct device_node *np, > mutex_lock(&gpd_list_lock); > > for (i = 0; i < data->num_domains; i++) { > - if (!pm_genpd_present(data->domains[i])) { > - mutex_unlock(&gpd_list_lock); > - return -EINVAL; > - } > + if (!pm_genpd_present(data->domains[i])) > + goto error; > + > + data->domains[i]->provider = &np->fwnode; > } > > ret = genpd_add_provider(np, genpd_xlate_onecell, data); > @@ -1575,6 +1577,14 @@ int of_genpd_add_provider_onecell(struct device_node *np, > mutex_unlock(&gpd_list_lock); > > return ret; > + > +error: > + while (i--) > + data->domains[i]->provider = NULL; > + > + mutex_unlock(&gpd_list_lock); > + > + return -EINVAL; > } > EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell); > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index fbdc5c4588ef..4aa285e44eb0 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -51,6 +51,7 @@ struct generic_pm_domain { > struct mutex lock; > struct dev_power_governor *gov; > struct work_struct power_off_work; > + struct fwnode_handle *provider; /* Identity of the domain provider */ > const char *name; > atomic_t sd_count; /* Number of subdomains with power "on" */ > enum gpd_status status; /* Current state of the domain */ > -- > 2.1.4 > I also think you should extend this change, to also make the of_genpd_del_provider() API to reset the genpd->provider = NULL. Otherwise you can't track when a provider is removed. Kind regards Uffe
On 08/09/16 12:56, Ulf Hansson wrote: > On 16 August 2016 at 11:49, Jon Hunter <jonathanh@nvidia.com> wrote: >> It is possible that a device has more than one provider of PM domains >> and to support the removal of a PM domain by provider, it is necessary >> to store a reference to the provider in the PM domain structure. >> Therefore, store a reference to the firmware node handle in the PM >> domain structure and populate it when providers (only device-tree based >> providers are currently supported by PM domains) are registered. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/base/power/domain.c | 18 ++++++++++++++---- >> include/linux/pm_domain.h | 1 + >> 2 files changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index 72b973539205..0bc145e8e902 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -1539,6 +1539,8 @@ int of_genpd_add_provider_simple(struct device_node *np, >> return -EINVAL; >> } >> >> + genpd->provider = &np->fwnode; >> + >> ret = genpd_add_provider(np, genpd_xlate_simple, genpd); > > I guess you want to reset genpd->provider = NULL, when > genpd_add_provider() fails!? Yes, will fix. > Perhaps better to assign genpd->provider when you know > genpd_add_provider() has succeeded. Yes seems sane! >> >> mutex_unlock(&gpd_list_lock); >> @@ -1564,10 +1566,10 @@ int of_genpd_add_provider_onecell(struct device_node *np, >> mutex_lock(&gpd_list_lock); >> >> for (i = 0; i < data->num_domains; i++) { >> - if (!pm_genpd_present(data->domains[i])) { >> - mutex_unlock(&gpd_list_lock); >> - return -EINVAL; >> - } >> + if (!pm_genpd_present(data->domains[i])) >> + goto error; >> + >> + data->domains[i]->provider = &np->fwnode; >> } >> >> ret = genpd_add_provider(np, genpd_xlate_onecell, data); >> @@ -1575,6 +1577,14 @@ int of_genpd_add_provider_onecell(struct device_node *np, >> mutex_unlock(&gpd_list_lock); >> >> return ret; >> + >> +error: >> + while (i--) >> + data->domains[i]->provider = NULL; >> + >> + mutex_unlock(&gpd_list_lock); >> + >> + return -EINVAL; >> } >> EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell); >> >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index fbdc5c4588ef..4aa285e44eb0 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -51,6 +51,7 @@ struct generic_pm_domain { >> struct mutex lock; >> struct dev_power_governor *gov; >> struct work_struct power_off_work; >> + struct fwnode_handle *provider; /* Identity of the domain provider */ >> const char *name; >> atomic_t sd_count; /* Number of subdomains with power "on" */ >> enum gpd_status status; /* Current state of the domain */ >> -- >> 2.1.4 >> > > I also think you should extend this change, to also make the > of_genpd_del_provider() API to reset the genpd->provider = NULL. > Otherwise you can't track when a provider is removed. Unfortunately that is not going to work. The function of_genpd_remove_tail() (patch #10) uses the ->provider member to remove the last domain for the given provider and of_genpd_del_provider() must be called before hand. Cheers Jon
[...] >> >> I also think you should extend this change, to also make the >> of_genpd_del_provider() API to reset the genpd->provider = NULL. >> Otherwise you can't track when a provider is removed. > > Unfortunately that is not going to work. The function > of_genpd_remove_tail() (patch #10) uses the ->provider member to remove > the last domain for the given provider and of_genpd_del_provider() must > be called before hand. Yes, of course - you are right! Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 72b973539205..0bc145e8e902 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1539,6 +1539,8 @@ int of_genpd_add_provider_simple(struct device_node *np, return -EINVAL; } + genpd->provider = &np->fwnode; + ret = genpd_add_provider(np, genpd_xlate_simple, genpd); mutex_unlock(&gpd_list_lock); @@ -1564,10 +1566,10 @@ int of_genpd_add_provider_onecell(struct device_node *np, mutex_lock(&gpd_list_lock); for (i = 0; i < data->num_domains; i++) { - if (!pm_genpd_present(data->domains[i])) { - mutex_unlock(&gpd_list_lock); - return -EINVAL; - } + if (!pm_genpd_present(data->domains[i])) + goto error; + + data->domains[i]->provider = &np->fwnode; } ret = genpd_add_provider(np, genpd_xlate_onecell, data); @@ -1575,6 +1577,14 @@ int of_genpd_add_provider_onecell(struct device_node *np, mutex_unlock(&gpd_list_lock); return ret; + +error: + while (i--) + data->domains[i]->provider = NULL; + + mutex_unlock(&gpd_list_lock); + + return -EINVAL; } EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell); diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index fbdc5c4588ef..4aa285e44eb0 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -51,6 +51,7 @@ struct generic_pm_domain { struct mutex lock; struct dev_power_governor *gov; struct work_struct power_off_work; + struct fwnode_handle *provider; /* Identity of the domain provider */ const char *name; atomic_t sd_count; /* Number of subdomains with power "on" */ enum gpd_status status; /* Current state of the domain */
It is possible that a device has more than one provider of PM domains and to support the removal of a PM domain by provider, it is necessary to store a reference to the provider in the PM domain structure. Therefore, store a reference to the firmware node handle in the PM domain structure and populate it when providers (only device-tree based providers are currently supported by PM domains) are registered. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/base/power/domain.c | 18 ++++++++++++++---- include/linux/pm_domain.h | 1 + 2 files changed, 15 insertions(+), 4 deletions(-)