Messages in this thread | | | From | Aisheng Dong <> | Subject | RE: [RFC PATCH 1/1] PM / Domains: Add multi PM domains support for attach_dev | Date | Wed, 2 Jan 2019 16:29:41 +0000 |
| |
> -----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
| |