On Wed, 2 Jan 2019 at 17:29, Aisheng Dong [off-list ref] wrote:

quoted

-----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 [off-list ref] wrote:

quoted

quoted

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 [off-list ref]
wrote:

quoted

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.

quoted

"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.

quoted

quoted

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."

quoted

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().

quoted

quoted

quoted

Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also
introduced to ease the driver operation.

Cc: "Rafael J. Wysocki" <redacted>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <redacted>
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].

quoted

After a bit more thinking, this is a less intrusive implementation
with the mininum impact on the exist function definitions and
calling

follows.

quoted

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().

quoted

quoted

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?

quoted

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!

quoted

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];
        ...
}

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel