On Sat, 29 Dec 2018 at 07:43, Aisheng Dong <aisheng.d...@nxp.com> wrote: > > > From: Ulf Hansson [mailto:ulf.hans...@linaro.org] > > Sent: Friday, December 28, 2018 11:37 PM > > > > On Thu, 27 Dec 2018 at 18:14, Aisheng Dong <aisheng.d...@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" <r...@rjwysocki.net> > > > Cc: Kevin Hilman <khil...@kernel.org> > > > Cc: Ulf Hansson <ulf.hans...@linaro.org> > > > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> > > > Signed-off-by: Dong Aisheng <aisheng.d...@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