> From: Ulf Hansson [mailto:ulf.hans...@linaro.org] > Sent: Friday, January 4, 2019 5:11 AM > On Wed, 2 Jan 2019 at 17:29, Aisheng Dong <aisheng.d...@nxp.com> wrote: > > > > > -----Original Message----- > > > From: Ulf Hansson [mailto:ulf.hans...@linaro.org] > > > Sent: Wednesday, January 2, 2019 6:49 PM > > > > > > 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? > > > > > > > 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