| Message ID | 1545930550-9906-1-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,1/1] PM / Domains: Add multi PM domains support for attach_dev | expand |
On Thu, 27 Dec 2018 at 18:14, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > Currently attach_dev() in power domain infrastructure still does > not support multi domains case as the struct device *dev passed > down from genpd_dev_pm_attach_by_id() is a virtual PD device, it > does not help for parsing the real device information from device > tree, e.g. Device/Power IDs, Clocks and it's unware of which real > power domain the device should attach. Thanks for working on this! I would appreciate if the changelog could clarify the problem a bit. Perhaps something along the lines of the below. "A genpd provider's ->attach_dev() callback may be invoked with a so called virtual device, which is created by genpd, at the point when a device is being attached to one of its corresponding multiple PM domains. In these cases, the genpd provider fails to look up any resource, by a clk_get() for example, for the virtual device in question. This is because, the virtual device that was created by genpd, does not have the virt_dev->of_node assigned." > > Extend the framework a bit to store the multi PM domains information > in per-device struct generic_pm_domain_data, then power domain driver > could retrieve it for necessary operations during attach_dev(). > > Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also > introduced to ease the driver operation. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > This patch is a follow-up work of the earlier discussion with Ulf Hansson > about the multi PM domains support for the attach_dev() function [1]. > After a bit more thinking, this is a less intrusive implementation with > the mininum impact on the exist function definitions and calling follows. > One known little drawback is that we have to use the device driver private > data (device.drvdata) to pass down the multi domains information in a > earlier time. However, as multi PD devices are created by domain framework, > this seems to be safe to use it in domain core code as device driver > is not likely going to use it. > Anyway, if any better ideas, please let me know. > > With the two new APIs, the using can be simply as: > static int xxx_attach_dev(struct generic_pm_domain *domain, > struct device *dev) > { > ... > if (genpd_is_mpd_device(dev)) { > mpd_data = dev_gpd_mpd_data(dev); > np = mpd_data->parent->of_node; > idx = mpd_data->index; > //dts parsing > ... > } > ... I think we can make this a lot less complicated. Just assign virt_dev->of_node = of_node_get(dev->of_node), somewhere in genpd_dev_pm_attach_by_id() and before calling __genpd_dev_pm_attach(). Doing that, would mean the genpd provider's ->attach_dev() callback, don't have to distinguish between virtual and non-virtual devices. Instead they should be able to look up resources in the same way as they did before. Kind regards Uffe > } > > [1] https://patchwork.kernel.org/patch/10658669/ > --- > drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 23 +++++++++++++++++++++++ > 2 files changed, 54 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 7f38a92..1aa0918 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1343,6 +1343,9 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev, > gpd_data->td.effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS; > gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; > > + if (genpd_is_mpd_device(dev)) > + gpd_data->mpd_data = dev_get_drvdata(dev); > + > spin_lock_irq(&dev->power.lock); > > if (dev->power.subsys_data->domain_data) { > @@ -2179,6 +2182,7 @@ EXPORT_SYMBOL_GPL(of_genpd_remove_last); > > static void genpd_release_dev(struct device *dev) > { > + kfree(dev->driver_data); > kfree(dev); > } > > @@ -2320,6 +2324,20 @@ int genpd_dev_pm_attach(struct device *dev) > EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); > > /** > + * genpd_is_mpd_device - Check if a device is associated with multi PM domains > + * @dev: Device to check. > + */ > + > +bool genpd_is_mpd_device(struct device *dev) > +{ > + if (!dev || (dev && !dev->bus)) > + return false; > + > + return dev->bus == &genpd_bus_type; > +}; > +EXPORT_SYMBOL_GPL(genpd_is_mpd_device); > + > +/** > * genpd_dev_pm_attach_by_id - Associate a device with one of its PM domains. > * @dev: The device used to lookup the PM domain. > * @index: The index of the PM domain. > @@ -2338,6 +2356,7 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); > struct device *genpd_dev_pm_attach_by_id(struct device *dev, > unsigned int index) > { > + struct pm_domain_mpd_data *mpd_data; > struct device *genpd_dev; > int num_domains; > int ret; > @@ -2366,6 +2385,18 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev, > return ERR_PTR(ret); > } > > + /* Allocate multi power domains data */ > + mpd_data = kzalloc(sizeof(*mpd_data), GFP_KERNEL); > + if (!mpd_data) { > + device_unregister(genpd_dev); > + return ERR_PTR(-ENOMEM); > + } > + > + mpd_data->parent = dev; > + mpd_data->index = index; > + > + dev_set_drvdata(genpd_dev, mpd_data); > + > /* Try to attach the device to the PM domain at the specified index. */ > ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index, false); > if (ret < 1) { > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 3b5d728..106d4e7 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -144,6 +144,11 @@ struct gpd_timing_data { > bool cached_suspend_ok; > }; > > +struct pm_domain_mpd_data { > + struct device *parent; > + unsigned int index; > +}; > + > struct pm_domain_data { > struct list_head list_node; > struct device *dev; > @@ -151,6 +156,7 @@ struct pm_domain_data { > > struct generic_pm_domain_data { > struct pm_domain_data base; > + struct pm_domain_mpd_data *mpd_data; > struct gpd_timing_data td; > struct notifier_block nb; > unsigned int performance_state; > @@ -262,10 +268,17 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev, > struct device_node *np); > > int genpd_dev_pm_attach(struct device *dev); > +bool genpd_is_mpd_device(struct device *dev); > struct device *genpd_dev_pm_attach_by_id(struct device *dev, > unsigned int index); > struct device *genpd_dev_pm_attach_by_name(struct device *dev, > char *name); > + > +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct device *dev) > +{ > + return dev_gpd_data(dev)->mpd_data; > +} > + > #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */ > static inline int of_genpd_add_provider_simple(struct device_node *np, > struct generic_pm_domain *genpd) > @@ -311,6 +324,11 @@ static inline int genpd_dev_pm_attach(struct device *dev) > return 0; > } > > +static bool genpd_is_mpd_device(struct device *dev) > +{ > + return false; > +} > + > static inline struct device *genpd_dev_pm_attach_by_id(struct device *dev, > unsigned int index) > { > @@ -323,6 +341,11 @@ static inline struct device *genpd_dev_pm_attach_by_name(struct device *dev, > return NULL; > } > > +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct device *dev) > +{ > + return ERR_PTR(-ENOSYS); > +} > + > static inline > struct generic_pm_domain *of_genpd_remove_last(struct device_node *np) > { > -- > 2.7.4 >
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Friday, December 28, 2018 11:37 PM > > On Thu, 27 Dec 2018 at 18:14, Aisheng Dong <aisheng.dong@nxp.com> > wrote: > > > > Currently attach_dev() in power domain infrastructure still does not > > support multi domains case as the struct device *dev passed down from > > genpd_dev_pm_attach_by_id() is a virtual PD device, it does not help > > for parsing the real device information from device tree, e.g. > > Device/Power IDs, Clocks and it's unware of which real power domain > > the device should attach. > > Thanks for working on this! > > I would appreciate if the changelog could clarify the problem a bit. > Perhaps something along the lines of the below. > Sounds good to me. I will add them in commit message. Thanks for the suggestion. > "A genpd provider's ->attach_dev() callback may be invoked with a so called > virtual device, which is created by genpd, at the point when a device is being > attached to one of its corresponding multiple PM domains. > > In these cases, the genpd provider fails to look up any resource, by a > clk_get() for example, for the virtual device in question. This is because, the > virtual device that was created by genpd, does not have the virt_dev->of_node > assigned." > > > > > Extend the framework a bit to store the multi PM domains information > > in per-device struct generic_pm_domain_data, then power domain driver > > could retrieve it for necessary operations during attach_dev(). > > > > Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also > > introduced to ease the driver operation. > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > Cc: Kevin Hilman <khilman@kernel.org> > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > This patch is a follow-up work of the earlier discussion with Ulf > > Hansson about the multi PM domains support for the attach_dev() function > [1]. > > After a bit more thinking, this is a less intrusive implementation > > with the mininum impact on the exist function definitions and calling > follows. > > One known little drawback is that we have to use the device driver > > private data (device.drvdata) to pass down the multi domains > > information in a earlier time. However, as multi PD devices are > > created by domain framework, this seems to be safe to use it in domain > > core code as device driver is not likely going to use it. > > Anyway, if any better ideas, please let me know. > > > > With the two new APIs, the using can be simply as: > > static int xxx_attach_dev(struct generic_pm_domain *domain, > > struct device *dev) { > > ... > > if (genpd_is_mpd_device(dev)) { > > mpd_data = dev_gpd_mpd_data(dev); > > np = mpd_data->parent->of_node; > > idx = mpd_data->index; > > //dts parsing > > ... > > } > > ... > > I think we can make this a lot less complicated. Just assign virt_dev->of_node = > of_node_get(dev->of_node), somewhere in > genpd_dev_pm_attach_by_id() and before calling __genpd_dev_pm_attach(). > > Doing that, would mean the genpd provider's ->attach_dev() callback, don't > have to distinguish between virtual and non-virtual devices. > Instead they should be able to look up resources in the same way as they did > before. > Yes, that seems like a smart way. But there's still a remain problem that how about the domain index information needed for attach_dev()? Regards Dong Aisheng > Kind regards > Uffe > > > } > > > > --- > > drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++++++ > > include/linux/pm_domain.h | 23 +++++++++++++++++++++++ > > 2 files changed, 54 insertions(+) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 7f38a92..1aa0918 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -1343,6 +1343,9 @@ static struct generic_pm_domain_data > *genpd_alloc_dev_data(struct device *dev, > > gpd_data->td.effective_constraint_ns = > PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS; > > gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; > > > > + if (genpd_is_mpd_device(dev)) > > + gpd_data->mpd_data = dev_get_drvdata(dev); > > + > > spin_lock_irq(&dev->power.lock); > > > > if (dev->power.subsys_data->domain_data) { @@ -2179,6 +2182,7 > > @@ EXPORT_SYMBOL_GPL(of_genpd_remove_last); > > > > static void genpd_release_dev(struct device *dev) { > > + kfree(dev->driver_data); > > kfree(dev); > > } > > > > @@ -2320,6 +2324,20 @@ int genpd_dev_pm_attach(struct device *dev) > > EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); > > > > /** > > + * genpd_is_mpd_device - Check if a device is associated with multi > > + PM domains > > + * @dev: Device to check. > > + */ > > + > > +bool genpd_is_mpd_device(struct device *dev) { > > + if (!dev || (dev && !dev->bus)) > > + return false; > > + > > + return dev->bus == &genpd_bus_type; }; > > +EXPORT_SYMBOL_GPL(genpd_is_mpd_device); > > + > > +/** > > * genpd_dev_pm_attach_by_id - Associate a device with one of its PM > domains. > > * @dev: The device used to lookup the PM domain. > > * @index: The index of the PM domain. > > @@ -2338,6 +2356,7 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); > > struct device *genpd_dev_pm_attach_by_id(struct device *dev, > > unsigned int index) { > > + struct pm_domain_mpd_data *mpd_data; > > struct device *genpd_dev; > > int num_domains; > > int ret; > > @@ -2366,6 +2385,18 @@ struct device > *genpd_dev_pm_attach_by_id(struct device *dev, > > return ERR_PTR(ret); > > } > > > > + /* Allocate multi power domains data */ > > + mpd_data = kzalloc(sizeof(*mpd_data), GFP_KERNEL); > > + if (!mpd_data) { > > + device_unregister(genpd_dev); > > + return ERR_PTR(-ENOMEM); > > + } > > + > > + mpd_data->parent = dev; > > + mpd_data->index = index; > > + > > + dev_set_drvdata(genpd_dev, mpd_data); > > + > > /* Try to attach the device to the PM domain at the specified index. > */ > > ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index, > false); > > if (ret < 1) { > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > > index 3b5d728..106d4e7 100644 > > --- a/include/linux/pm_domain.h > > +++ b/include/linux/pm_domain.h > > @@ -144,6 +144,11 @@ struct gpd_timing_data { > > bool cached_suspend_ok; > > }; > > > > +struct pm_domain_mpd_data { > > + struct device *parent; > > + unsigned int index; > > +}; > > + > > struct pm_domain_data { > > struct list_head list_node; > > struct device *dev; > > @@ -151,6 +156,7 @@ struct pm_domain_data { > > > > struct generic_pm_domain_data { > > struct pm_domain_data base; > > + struct pm_domain_mpd_data *mpd_data; > > struct gpd_timing_data td; > > struct notifier_block nb; > > unsigned int performance_state; @@ -262,10 +268,17 @@ > unsigned > > int of_genpd_opp_to_performance_state(struct device *dev, > > struct device_node *np); > > > > int genpd_dev_pm_attach(struct device *dev); > > +bool genpd_is_mpd_device(struct device *dev); > > struct device *genpd_dev_pm_attach_by_id(struct device *dev, > > unsigned int index); struct > > device *genpd_dev_pm_attach_by_name(struct device *dev, > > char *name); > > + > > +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct > > +device *dev) { > > + return dev_gpd_data(dev)->mpd_data; } > > + > > #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */ static inline int > > of_genpd_add_provider_simple(struct device_node *np, > > struct generic_pm_domain > > *genpd) @@ -311,6 +324,11 @@ static inline int > genpd_dev_pm_attach(struct device *dev) > > return 0; > > } > > > > +static bool genpd_is_mpd_device(struct device *dev) { > > + return false; > > +} > > + > > static inline struct device *genpd_dev_pm_attach_by_id(struct device *dev, > > unsigned > int > > index) { @@ -323,6 +341,11 @@ static inline struct device > > *genpd_dev_pm_attach_by_name(struct device *dev, > > return NULL; > > } > > > > +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct > > +device *dev) { > > + return ERR_PTR(-ENOSYS); > > +} > > + > > static inline > > struct generic_pm_domain *of_genpd_remove_last(struct device_node > > *np) { > > -- > > 2.7.4 > >
On Sat, 29 Dec 2018 at 07:43, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > > Sent: Friday, December 28, 2018 11:37 PM > > > > On Thu, 27 Dec 2018 at 18:14, Aisheng Dong <aisheng.dong@nxp.com> > > wrote: > > > > > > Currently attach_dev() in power domain infrastructure still does not > > > support multi domains case as the struct device *dev passed down from > > > genpd_dev_pm_attach_by_id() is a virtual PD device, it does not help > > > for parsing the real device information from device tree, e.g. > > > Device/Power IDs, Clocks and it's unware of which real power domain > > > the device should attach. > > > > Thanks for working on this! > > > > I would appreciate if the changelog could clarify the problem a bit. > > Perhaps something along the lines of the below. > > > > Sounds good to me. > I will add them in commit message. > Thanks for the suggestion. > > > "A genpd provider's ->attach_dev() callback may be invoked with a so called > > virtual device, which is created by genpd, at the point when a device is being > > attached to one of its corresponding multiple PM domains. > > > > In these cases, the genpd provider fails to look up any resource, by a > > clk_get() for example, for the virtual device in question. This is because, the > > virtual device that was created by genpd, does not have the virt_dev->of_node > > assigned." > > > > > > > > Extend the framework a bit to store the multi PM domains information > > > in per-device struct generic_pm_domain_data, then power domain driver > > > could retrieve it for necessary operations during attach_dev(). > > > > > > Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also > > > introduced to ease the driver operation. > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > Cc: Kevin Hilman <khilman@kernel.org> > > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > > --- > > > This patch is a follow-up work of the earlier discussion with Ulf > > > Hansson about the multi PM domains support for the attach_dev() function > > [1]. > > > After a bit more thinking, this is a less intrusive implementation > > > with the mininum impact on the exist function definitions and calling > > follows. > > > One known little drawback is that we have to use the device driver > > > private data (device.drvdata) to pass down the multi domains > > > information in a earlier time. However, as multi PD devices are > > > created by domain framework, this seems to be safe to use it in domain > > > core code as device driver is not likely going to use it. > > > Anyway, if any better ideas, please let me know. > > > > > > With the two new APIs, the using can be simply as: > > > static int xxx_attach_dev(struct generic_pm_domain *domain, > > > struct device *dev) { > > > ... > > > if (genpd_is_mpd_device(dev)) { > > > mpd_data = dev_gpd_mpd_data(dev); > > > np = mpd_data->parent->of_node; > > > idx = mpd_data->index; > > > //dts parsing > > > ... > > > } > > > ... > > > > I think we can make this a lot less complicated. Just assign virt_dev->of_node = > > of_node_get(dev->of_node), somewhere in > > genpd_dev_pm_attach_by_id() and before calling __genpd_dev_pm_attach(). > > > > Doing that, would mean the genpd provider's ->attach_dev() callback, don't > > have to distinguish between virtual and non-virtual devices. > > Instead they should be able to look up resources in the same way as they did > > before. > > > > Yes, that seems like a smart way. > But there's still a remain problem that how about the domain index information > needed for attach_dev()? > What do you mean by domain index? The ->attach_dev() is given both the device and the genpd in question as in-parameters. Could you store the domain index as part of your genpd struct? No? If you are talking about using the "power-domains" specifier from the DT node of the device, that should then work, similar to as been done in: drivers/soc/ti/ti_sci_pm_domains.c ti_sci_pd_attach_dev() You may also provide your own xlate callback for your genpd provider, like what is done in drivers/soc/tegra/powergate-bpmp. - if that helps!? Or am I missing something? [...] Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Wednesday, January 2, 2019 6:49 PM > > On Sat, 29 Dec 2018 at 07:43, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > > > Sent: Friday, December 28, 2018 11:37 PM > > > > > > On Thu, 27 Dec 2018 at 18:14, Aisheng Dong <aisheng.dong@nxp.com> > > > wrote: > > > > > > > > Currently attach_dev() in power domain infrastructure still does > > > > not support multi domains case as the struct device *dev passed > > > > down from > > > > genpd_dev_pm_attach_by_id() is a virtual PD device, it does not > > > > help for parsing the real device information from device tree, e.g. > > > > Device/Power IDs, Clocks and it's unware of which real power > > > > domain the device should attach. > > > > > > Thanks for working on this! > > > > > > I would appreciate if the changelog could clarify the problem a bit. > > > Perhaps something along the lines of the below. > > > > > > > Sounds good to me. > > I will add them in commit message. > > Thanks for the suggestion. > > > > > "A genpd provider's ->attach_dev() callback may be invoked with a so > > > called virtual device, which is created by genpd, at the point when > > > a device is being attached to one of its corresponding multiple PM > domains. > > > > > > In these cases, the genpd provider fails to look up any resource, by > > > a > > > clk_get() for example, for the virtual device in question. This is > > > because, the virtual device that was created by genpd, does not have > > > the virt_dev->of_node assigned." > > > > > > > > > > > Extend the framework a bit to store the multi PM domains > > > > information in per-device struct generic_pm_domain_data, then > > > > power domain driver could retrieve it for necessary operations during > attach_dev(). > > > > > > > > Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also > > > > introduced to ease the driver operation. > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > Cc: Kevin Hilman <khilman@kernel.org> > > > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > > > --- > > > > This patch is a follow-up work of the earlier discussion with Ulf > > > > Hansson about the multi PM domains support for the attach_dev() > > > > function > > > [1]. > > > > After a bit more thinking, this is a less intrusive implementation > > > > with the mininum impact on the exist function definitions and > > > > calling > > > follows. > > > > One known little drawback is that we have to use the device driver > > > > private data (device.drvdata) to pass down the multi domains > > > > information in a earlier time. However, as multi PD devices are > > > > created by domain framework, this seems to be safe to use it in > > > > domain core code as device driver is not likely going to use it. > > > > Anyway, if any better ideas, please let me know. > > > > > > > > With the two new APIs, the using can be simply as: > > > > static int xxx_attach_dev(struct generic_pm_domain *domain, > > > > struct device *dev) { > > > > ... > > > > if (genpd_is_mpd_device(dev)) { > > > > mpd_data = dev_gpd_mpd_data(dev); > > > > np = mpd_data->parent->of_node; > > > > idx = mpd_data->index; > > > > //dts parsing > > > > ... > > > > } > > > > ... > > > > > > I think we can make this a lot less complicated. Just assign > > > virt_dev->of_node = of_node_get(dev->of_node), somewhere in > > > genpd_dev_pm_attach_by_id() and before calling > __genpd_dev_pm_attach(). > > > > > > Doing that, would mean the genpd provider's ->attach_dev() callback, > > > don't have to distinguish between virtual and non-virtual devices. > > > Instead they should be able to look up resources in the same way as > > > they did before. > > > > > > > Yes, that seems like a smart way. > > But there's still a remain problem that how about the domain index > > information needed for attach_dev()? > > > > What do you mean by domain index? > As we're using multi power domains in Devicetree, attach_dev() needs to know which power domain the device is going to attach. e.g. sdhc1: mmc@5b010000 { compatible = "fsl,imx8qxp-usdhc", "fsl,imx7d-usdhc"; power-domains = <&pd IMX_SC_R_SDHC_0>, <&pd IMX_SC_R_SDHC_1>; // for test only ... }; Then attach_dev() can parse the correct "resource id" (e.g. IMX_SC_R_SDHC_1) from device tree And store it in per-device struct generic_pm_domain_data for later start()/stop() using. > The ->attach_dev() is given both the device and the genpd in question as > in-parameters. Could you store the domain index as part of your genpd struct? > No? > AFAICT no. With the only device and the genpd as in-parameters, the ->attach_dev() don't know which power domain to to parse from device tree. Thus no way to store it in genpd struct. > If you are talking about using the "power-domains" specifier from the DT node > of the device, that should then work, similar to as been done > in: > > drivers/soc/ti/ti_sci_pm_domains.c > ti_sci_pd_attach_dev() > TI actually does not use multi PM domains. So they can always parse the first power domains to get the correct "resource id" / power id.. wdt: wdt@02250000 { compatible = "ti,keystone-wdt", "ti,davinci-wdt"; power-domains = <&k2g_pds 0x22>; }; static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain, struct device *dev) { ... // the index is always 0 ret = of_parse_phandle_with_args(np, "power-domains", "#power-domain-cells", 0, &pd_args); idx = pd_args.args[0]; ... } > You may also provide your own xlate callback for your genpd provider, like > what is done in drivers/soc/tegra/powergate-bpmp. - if that helps!? > I'm afraid no. Per our earlier discussion, we're going to use a single global power domain (Tegra is not) and implement the ->attach|detach_dev() callback for the genpd and use the regular of_genpd_add_provider_simple(). From within the ->attach_dev(), we could get the OF node for the device that is being attached and then parse the power-domain cell containing the "resource id" and store that in the per device struct generic_pm_domain_data (we have void pointer there for storing these kind of things). Additionally, we need implement the ->stop() and ->start() callbacks of genpd, which is where we "power on/off" devices, rather than using the ->power_on|off() callbacks. Now the problem is current attach_dev() has no idea to parse the correct power domain containing the "resource id". That's why I stored it in per-device struct generic_pm_domain_data before calling attach_dev() in this patch implementation. Any ideas? Regards Dong Aisheng > Or am I missing something? > > [...] > > Kind regards > Uffe
On Wed, 2 Jan 2019 at 17:29, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > > -----Original Message----- > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > > Sent: Wednesday, January 2, 2019 6:49 PM > > > > On Sat, 29 Dec 2018 at 07:43, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > > > > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > > > > Sent: Friday, December 28, 2018 11:37 PM > > > > > > > > On Thu, 27 Dec 2018 at 18:14, Aisheng Dong <aisheng.dong@nxp.com> > > > > wrote: > > > > > > > > > > Currently attach_dev() in power domain infrastructure still does > > > > > not support multi domains case as the struct device *dev passed > > > > > down from > > > > > genpd_dev_pm_attach_by_id() is a virtual PD device, it does not > > > > > help for parsing the real device information from device tree, e.g. > > > > > Device/Power IDs, Clocks and it's unware of which real power > > > > > domain the device should attach. > > > > > > > > Thanks for working on this! > > > > > > > > I would appreciate if the changelog could clarify the problem a bit. > > > > Perhaps something along the lines of the below. > > > > > > > > > > Sounds good to me. > > > I will add them in commit message. > > > Thanks for the suggestion. > > > > > > > "A genpd provider's ->attach_dev() callback may be invoked with a so > > > > called virtual device, which is created by genpd, at the point when > > > > a device is being attached to one of its corresponding multiple PM > > domains. > > > > > > > > In these cases, the genpd provider fails to look up any resource, by > > > > a > > > > clk_get() for example, for the virtual device in question. This is > > > > because, the virtual device that was created by genpd, does not have > > > > the virt_dev->of_node assigned." > > > > > > > > > > > > > > Extend the framework a bit to store the multi PM domains > > > > > information in per-device struct generic_pm_domain_data, then > > > > > power domain driver could retrieve it for necessary operations during > > attach_dev(). > > > > > > > > > > Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also > > > > > introduced to ease the driver operation. > > > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > > Cc: Kevin Hilman <khilman@kernel.org> > > > > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > > > > --- > > > > > This patch is a follow-up work of the earlier discussion with Ulf > > > > > Hansson about the multi PM domains support for the attach_dev() > > > > > function > > > > [1]. > > > > > After a bit more thinking, this is a less intrusive implementation > > > > > with the mininum impact on the exist function definitions and > > > > > calling > > > > follows. > > > > > One known little drawback is that we have to use the device driver > > > > > private data (device.drvdata) to pass down the multi domains > > > > > information in a earlier time. However, as multi PD devices are > > > > > created by domain framework, this seems to be safe to use it in > > > > > domain core code as device driver is not likely going to use it. > > > > > Anyway, if any better ideas, please let me know. > > > > > > > > > > With the two new APIs, the using can be simply as: > > > > > static int xxx_attach_dev(struct generic_pm_domain *domain, > > > > > struct device *dev) { > > > > > ... > > > > > if (genpd_is_mpd_device(dev)) { > > > > > mpd_data = dev_gpd_mpd_data(dev); > > > > > np = mpd_data->parent->of_node; > > > > > idx = mpd_data->index; > > > > > //dts parsing > > > > > ... > > > > > } > > > > > ... > > > > > > > > I think we can make this a lot less complicated. Just assign > > > > virt_dev->of_node = of_node_get(dev->of_node), somewhere in > > > > genpd_dev_pm_attach_by_id() and before calling > > __genpd_dev_pm_attach(). > > > > > > > > Doing that, would mean the genpd provider's ->attach_dev() callback, > > > > don't have to distinguish between virtual and non-virtual devices. > > > > Instead they should be able to look up resources in the same way as > > > > they did before. > > > > > > > > > > Yes, that seems like a smart way. > > > But there's still a remain problem that how about the domain index > > > information needed for attach_dev()? > > > > > > > What do you mean by domain index? > > > > As we're using multi power domains in Devicetree, attach_dev() needs to > know which power domain the device is going to attach. > e.g. > sdhc1: mmc@5b010000 { > compatible = "fsl,imx8qxp-usdhc", "fsl,imx7d-usdhc"; > power-domains = <&pd IMX_SC_R_SDHC_0>, <&pd IMX_SC_R_SDHC_1>; // for test only > ... > }; > Then attach_dev() can parse the correct "resource id" (e.g. IMX_SC_R_SDHC_1) from device tree > And store it in per-device struct generic_pm_domain_data for later start()/stop() using. I see, thanks for clarifying. Seem like, we have two options to make this work. 1. Let genpd pre-store the index in a the per device generic_pm_domain_data while doing the attach and before calling the ->attach_dev() callback. This would make sense, if we consider this to be a common thing. 2. Provide the index as an additional new in-parameter to the ->attach_dev() callback. This requires a tree wide change as it means we need to update existing code using the ->attach_dev() callback. I would probably try 1) first to see how the code would look like and then fall back to 2). What do you think? > > > The ->attach_dev() is given both the device and the genpd in question as > > in-parameters. Could you store the domain index as part of your genpd struct? > > No? > > > > AFAICT no. > With the only device and the genpd as in-parameters, the ->attach_dev() don't know > which power domain to to parse from device tree. > Thus no way to store it in genpd struct. As stated, you are right! > > > If you are talking about using the "power-domains" specifier from the DT node > > of the device, that should then work, similar to as been done > > in: > > > > drivers/soc/ti/ti_sci_pm_domains.c > > ti_sci_pd_attach_dev() > > > > TI actually does not use multi PM domains. So they can always parse > the first power domains to get the correct "resource id" / power id.. > > wdt: wdt@02250000 { > compatible = "ti,keystone-wdt", "ti,davinci-wdt"; > power-domains = <&k2g_pds 0x22>; > }; > > static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain, > struct device *dev) > { > ... > // the index is always 0 > ret = of_parse_phandle_with_args(np, "power-domains", > "#power-domain-cells", 0, &pd_args); > idx = pd_args.args[0]; > ... > } > > > You may also provide your own xlate callback for your genpd provider, like > > what is done in drivers/soc/tegra/powergate-bpmp. - if that helps!? > > > > I'm afraid no. > Per our earlier discussion, we're going to use a single global power domain > (Tegra is not) and implement the ->attach|detach_dev() callback for the genpd > and use the regular of_genpd_add_provider_simple(). > > From within the ->attach_dev(), we could get the OF node for the device that is > being attached and then parse the power-domain cell containing the "resource id" > and store that in the per device struct generic_pm_domain_data (we have void pointer > there for storing these kind of things). > > Additionally, we need implement the ->stop() and ->start() callbacks of genpd, > which is where we "power on/off" devices, rather than using the > ->power_on|off() callbacks. > > Now the problem is current attach_dev() has no idea to parse the correct power domain > containing the "resource id". > That's why I stored it in per-device struct generic_pm_domain_data before calling > attach_dev() in this patch implementation. > > Any ideas? Again, thanks for clarifying! See my ideas above. Kind regards Uffe
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Friday, January 4, 2019 5:11 AM > On Wed, 2 Jan 2019 at 17:29, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > > > > -----Original Message----- > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > > > Sent: Wednesday, January 2, 2019 6:49 PM > > > > > > On Sat, 29 Dec 2018 at 07:43, Aisheng Dong <aisheng.dong@nxp.com> > wrote: > > > > > > > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > > > > > Sent: Friday, December 28, 2018 11:37 PM > > > > > > > > > > On Thu, 27 Dec 2018 at 18:14, Aisheng Dong > > > > > <aisheng.dong@nxp.com> > > > > > wrote: > > > > > > > > > > > > Currently attach_dev() in power domain infrastructure still > > > > > > does not support multi domains case as the struct device *dev > > > > > > passed down from > > > > > > genpd_dev_pm_attach_by_id() is a virtual PD device, it does > > > > > > not help for parsing the real device information from device tree, e.g. > > > > > > Device/Power IDs, Clocks and it's unware of which real power > > > > > > domain the device should attach. > > > > > > > > > > Thanks for working on this! > > > > > > > > > > I would appreciate if the changelog could clarify the problem a bit. > > > > > Perhaps something along the lines of the below. > > > > > > > > > > > > > Sounds good to me. > > > > I will add them in commit message. > > > > Thanks for the suggestion. > > > > > > > > > "A genpd provider's ->attach_dev() callback may be invoked with > > > > > a so called virtual device, which is created by genpd, at the > > > > > point when a device is being attached to one of its > > > > > corresponding multiple PM > > > domains. > > > > > > > > > > In these cases, the genpd provider fails to look up any > > > > > resource, by a > > > > > clk_get() for example, for the virtual device in question. This > > > > > is because, the virtual device that was created by genpd, does > > > > > not have the virt_dev->of_node assigned." > > > > > > > > > > > > > > > > > Extend the framework a bit to store the multi PM domains > > > > > > information in per-device struct generic_pm_domain_data, then > > > > > > power domain driver could retrieve it for necessary operations > > > > > > during > > > attach_dev(). > > > > > > > > > > > > Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are > > > > > > also introduced to ease the driver operation. > > > > > > > > > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > > > Cc: Kevin Hilman <khilman@kernel.org> > > > > > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > > > > > --- > > > > > > This patch is a follow-up work of the earlier discussion with > > > > > > Ulf Hansson about the multi PM domains support for the > > > > > > attach_dev() function > > > > > [1]. > > > > > > After a bit more thinking, this is a less intrusive > > > > > > implementation with the mininum impact on the exist function > > > > > > definitions and calling > > > > > follows. > > > > > > One known little drawback is that we have to use the device > > > > > > driver private data (device.drvdata) to pass down the multi > > > > > > domains information in a earlier time. However, as multi PD > > > > > > devices are created by domain framework, this seems to be safe > > > > > > to use it in domain core code as device driver is not likely going to use > it. > > > > > > Anyway, if any better ideas, please let me know. > > > > > > > > > > > > With the two new APIs, the using can be simply as: > > > > > > static int xxx_attach_dev(struct generic_pm_domain *domain, > > > > > > struct device *dev) { > > > > > > ... > > > > > > if (genpd_is_mpd_device(dev)) { > > > > > > mpd_data = dev_gpd_mpd_data(dev); > > > > > > np = mpd_data->parent->of_node; > > > > > > idx = mpd_data->index; > > > > > > //dts parsing > > > > > > ... > > > > > > } > > > > > > ... > > > > > > > > > > I think we can make this a lot less complicated. Just assign > > > > > virt_dev->of_node = of_node_get(dev->of_node), somewhere in > > > > > genpd_dev_pm_attach_by_id() and before calling > > > __genpd_dev_pm_attach(). > > > > > > > > > > Doing that, would mean the genpd provider's ->attach_dev() > > > > > callback, don't have to distinguish between virtual and non-virtual > devices. > > > > > Instead they should be able to look up resources in the same way > > > > > as they did before. > > > > > > > > > > > > > Yes, that seems like a smart way. > > > > But there's still a remain problem that how about the domain index > > > > information needed for attach_dev()? > > > > > > > > > > What do you mean by domain index? > > > > > > > As we're using multi power domains in Devicetree, attach_dev() needs > > to know which power domain the device is going to attach. > > e.g. > > sdhc1: mmc@5b010000 { > > compatible = "fsl,imx8qxp-usdhc", "fsl,imx7d-usdhc"; > > power-domains = <&pd IMX_SC_R_SDHC_0>, <&pd > IMX_SC_R_SDHC_1>; // for test only > > ... > > }; > > Then attach_dev() can parse the correct "resource id" (e.g. > > IMX_SC_R_SDHC_1) from device tree And store it in per-device struct > generic_pm_domain_data for later start()/stop() using. > > I see, thanks for clarifying. > > Seem like, we have two options to make this work. > > 1. Let genpd pre-store the index in a the per device generic_pm_domain_data > while doing the attach and before calling the > ->attach_dev() callback. This would make sense, if we consider this to > be a common thing. > > 2. Provide the index as an additional new in-parameter to the > ->attach_dev() callback. This requires a tree wide change as it means > we need to update existing code using the ->attach_dev() callback. > > I would probably try 1) first to see how the code would look like and then fall > back to 2). What do you think? > Yes, I agree with you. This patch is exactly doing 1. For your former suggestion: " Just assign virt_dev->of_node = of_node_get(dev->of_node), somewhere in genpd_dev_pm_attach_by_id() and before calling __genpd_dev_pm_attach(). Doing that, would mean the genpd provider's ->attach_dev() callback, don't have to distinguish between virtual and non-virtual devices" As this can only solve passing the real device node issue and we still need pre-store the index in a the per device generic_pm_domain_data and then distinguish if it's multi power domain virtual devices in attach_dev(), so I'm not sure if it's still quite necessary to do that way by assigning "virt_dev->of_node = of_node_get(dev->of_node) before attach". Probably after fully switch to option 2, then we can do it to fully drop the using of per device generic_pm_domain_data to make it transparent to attach_dev() users. So now the options may be: 1. Pre-store both device node and domain index in generic_pm_domain_data before Calling attach_dev(). This is exactly this patch does. 2. Pre-store only domain index in generic_pm_domain_data. For device node, assigning "virt_dev->of_node = of_node_get(dev->of_node) before attach. 3. Change attach_dev() to passing domain index and assigning "virt_dev->of_node = of_node_get(dev->of_node) before attach. Can you please tell what's your prefer? If you're okay with 1, can you please review if this patch is ok to you? Or we directly change to 3 which is fully transparent to users (users don't need to distinguish whether it's multi pd devices). e.g. saving genpd_is_mpd_device() and dev_gpd_mpd_data() in this patch. Regards Dong Aisheng > > > > > The ->attach_dev() is given both the device and the genpd in > > > question as in-parameters. Could you store the domain index as part of > your genpd struct? > > > No? > > > > > > > AFAICT no. > > With the only device and the genpd as in-parameters, the > > ->attach_dev() don't know which power domain to to parse from device tree. > > Thus no way to store it in genpd struct. > > As stated, you are right! > > > > > > If you are talking about using the "power-domains" specifier from > > > the DT node of the device, that should then work, similar to as been > > > done > > > in: > > > > > > drivers/soc/ti/ti_sci_pm_domains.c > > > ti_sci_pd_attach_dev() > > > > > > > TI actually does not use multi PM domains. So they can always parse > > the first power domains to get the correct "resource id" / power id.. > > > > wdt: wdt@02250000 { > > compatible = "ti,keystone-wdt", "ti,davinci-wdt"; > > power-domains = <&k2g_pds 0x22>; }; > > > > static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain, > > struct device *dev) { > > ... > > // the index is always 0 > > ret = of_parse_phandle_with_args(np, "power-domains", > > "#power-domain-cells", 0, > &pd_args); > > idx = pd_args.args[0]; > > ... > > } > > > > > You may also provide your own xlate callback for your genpd > > > provider, like what is done in drivers/soc/tegra/powergate-bpmp. - if that > helps!? > > > > > > > I'm afraid no. > > Per our earlier discussion, we're going to use a single global power > > domain (Tegra is not) and implement the ->attach|detach_dev() callback > > for the genpd and use the regular of_genpd_add_provider_simple(). > > > > From within the ->attach_dev(), we could get the OF node for the > > device that is being attached and then parse the power-domain cell > containing the "resource id" > > and store that in the per device struct generic_pm_domain_data (we > > have void pointer there for storing these kind of things). > > > > Additionally, we need implement the ->stop() and ->start() callbacks > > of genpd, which is where we "power on/off" devices, rather than using > > the > > ->power_on|off() callbacks. > > > > Now the problem is current attach_dev() has no idea to parse the > > correct power domain containing the "resource id". > > That's why I stored it in per-device struct generic_pm_domain_data > > before calling > > attach_dev() in this patch implementation. > > > > Any ideas? > > Again, thanks for clarifying! > > See my ideas above. > > Kind regards > Uffe
[...] > > > Then attach_dev() can parse the correct "resource id" (e.g. > > > IMX_SC_R_SDHC_1) from device tree And store it in per-device struct > > generic_pm_domain_data for later start()/stop() using. > > > > I see, thanks for clarifying. > > > > Seem like, we have two options to make this work. > > > > 1. Let genpd pre-store the index in a the per device generic_pm_domain_data > > while doing the attach and before calling the > > ->attach_dev() callback. This would make sense, if we consider this to > > be a common thing. > > > > 2. Provide the index as an additional new in-parameter to the > > ->attach_dev() callback. This requires a tree wide change as it means > > we need to update existing code using the ->attach_dev() callback. > > > > I would probably try 1) first to see how the code would look like and then fall > > back to 2). What do you think? > > > > Yes, I agree with you. > This patch is exactly doing 1. > > For your former suggestion: > " Just assign virt_dev->of_node = of_node_get(dev->of_node), somewhere in > genpd_dev_pm_attach_by_id() and before calling __genpd_dev_pm_attach(). > Doing that, would mean the genpd provider's ->attach_dev() callback, > don't have to distinguish between virtual and non-virtual devices" > > As this can only solve passing the real device node issue and we still need > pre-store the index in a the per device generic_pm_domain_data and then > distinguish if it's multi power domain virtual devices in attach_dev(), so > I'm not sure if it's still quite necessary to do that way by assigning > "virt_dev->of_node = of_node_get(dev->of_node) before attach". > Probably after fully switch to option 2, then we can do it to fully drop the using > of per device generic_pm_domain_data to make it transparent to attach_dev() > users. > > So now the options may be: > 1. Pre-store both device node and domain index in generic_pm_domain_data before > Calling attach_dev(). This is exactly this patch does. > 2. Pre-store only domain index in generic_pm_domain_data. For device node, assigning > "virt_dev->of_node = of_node_get(dev->of_node) before attach. > 3. Change attach_dev() to passing domain index and assigning > "virt_dev->of_node = of_node_get(dev->of_node) before attach. > > Can you please tell what's your prefer? Apologize for the delay, just realized that you were expecting a reply from me. To me is sounds like option 2) is the most feasible. Perhaps it's even a good idea to store the domain index in the struct generic_pm_domain_data, as it allows it then to be used at any point later in time, this not only from the ->attach_dev() callback as option 3) would mean. So according to my understandings, then your genpd backend driver's ->attach_dev() callback, would use dev_gpd_data() to find out the domain index for the device in question. Is that sufficient, or is there anything else that is needed? [...] Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 7f38a92..1aa0918 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1343,6 +1343,9 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev, gpd_data->td.effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS; gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; + if (genpd_is_mpd_device(dev)) + gpd_data->mpd_data = dev_get_drvdata(dev); + spin_lock_irq(&dev->power.lock); if (dev->power.subsys_data->domain_data) { @@ -2179,6 +2182,7 @@ EXPORT_SYMBOL_GPL(of_genpd_remove_last); static void genpd_release_dev(struct device *dev) { + kfree(dev->driver_data); kfree(dev); } @@ -2320,6 +2324,20 @@ int genpd_dev_pm_attach(struct device *dev) EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); /** + * genpd_is_mpd_device - Check if a device is associated with multi PM domains + * @dev: Device to check. + */ + +bool genpd_is_mpd_device(struct device *dev) +{ + if (!dev || (dev && !dev->bus)) + return false; + + return dev->bus == &genpd_bus_type; +}; +EXPORT_SYMBOL_GPL(genpd_is_mpd_device); + +/** * genpd_dev_pm_attach_by_id - Associate a device with one of its PM domains. * @dev: The device used to lookup the PM domain. * @index: The index of the PM domain. @@ -2338,6 +2356,7 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); struct device *genpd_dev_pm_attach_by_id(struct device *dev, unsigned int index) { + struct pm_domain_mpd_data *mpd_data; struct device *genpd_dev; int num_domains; int ret; @@ -2366,6 +2385,18 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev, return ERR_PTR(ret); } + /* Allocate multi power domains data */ + mpd_data = kzalloc(sizeof(*mpd_data), GFP_KERNEL); + if (!mpd_data) { + device_unregister(genpd_dev); + return ERR_PTR(-ENOMEM); + } + + mpd_data->parent = dev; + mpd_data->index = index; + + dev_set_drvdata(genpd_dev, mpd_data); + /* Try to attach the device to the PM domain at the specified index. */ ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index, false); if (ret < 1) { diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 3b5d728..106d4e7 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -144,6 +144,11 @@ struct gpd_timing_data { bool cached_suspend_ok; }; +struct pm_domain_mpd_data { + struct device *parent; + unsigned int index; +}; + struct pm_domain_data { struct list_head list_node; struct device *dev; @@ -151,6 +156,7 @@ struct pm_domain_data { struct generic_pm_domain_data { struct pm_domain_data base; + struct pm_domain_mpd_data *mpd_data; struct gpd_timing_data td; struct notifier_block nb; unsigned int performance_state; @@ -262,10 +268,17 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev, struct device_node *np); int genpd_dev_pm_attach(struct device *dev); +bool genpd_is_mpd_device(struct device *dev); struct device *genpd_dev_pm_attach_by_id(struct device *dev, unsigned int index); struct device *genpd_dev_pm_attach_by_name(struct device *dev, char *name); + +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct device *dev) +{ + return dev_gpd_data(dev)->mpd_data; +} + #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */ static inline int of_genpd_add_provider_simple(struct device_node *np, struct generic_pm_domain *genpd) @@ -311,6 +324,11 @@ static inline int genpd_dev_pm_attach(struct device *dev) return 0; } +static bool genpd_is_mpd_device(struct device *dev) +{ + return false; +} + static inline struct device *genpd_dev_pm_attach_by_id(struct device *dev, unsigned int index) { @@ -323,6 +341,11 @@ static inline struct device *genpd_dev_pm_attach_by_name(struct device *dev, return NULL; } +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct device *dev) +{ + return ERR_PTR(-ENOSYS); +} + static inline struct generic_pm_domain *of_genpd_remove_last(struct device_node *np) {
Currently attach_dev() in power domain infrastructure still does not support multi domains case as the struct device *dev passed down from genpd_dev_pm_attach_by_id() is a virtual PD device, it does not help for parsing the real device information from device tree, e.g. Device/Power IDs, Clocks and it's unware of which real power domain the device should attach. Extend the framework a bit to store the multi PM domains information in per-device struct generic_pm_domain_data, then power domain driver could retrieve it for necessary operations during attach_dev(). Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also introduced to ease the driver operation. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Kevin Hilman <khilman@kernel.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- This patch is a follow-up work of the earlier discussion with Ulf Hansson about the multi PM domains support for the attach_dev() function [1]. After a bit more thinking, this is a less intrusive implementation with the mininum impact on the exist function definitions and calling follows. One known little drawback is that we have to use the device driver private data (device.drvdata) to pass down the multi domains information in a earlier time. However, as multi PD devices are created by domain framework, this seems to be safe to use it in domain core code as device driver is not likely going to use it. Anyway, if any better ideas, please let me know. With the two new APIs, the using can be simply as: static int xxx_attach_dev(struct generic_pm_domain *domain, struct device *dev) { ... if (genpd_is_mpd_device(dev)) { mpd_data = dev_gpd_mpd_data(dev); np = mpd_data->parent->of_node; idx = mpd_data->index; //dts parsing ... } ... } [1] https://patchwork.kernel.org/patch/10658669/ --- drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++++++ include/linux/pm_domain.h | 23 +++++++++++++++++++++++ 2 files changed, 54 insertions(+)