-----Original Message-----
From: Ulf Hansson [mailto:ulf.hansson at linaro.org]
Sent: Wednesday, October 31, 2018 11:56 PM

[...]

On 31 October 2018 at 02:45, A.s. Dong [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Ulf Hansson [mailto:ulf.hansson at linaro.org]
Sent: Wednesday, October 31, 2018 12:00 AM

[...]

quoted

On 30 October 2018 at 14:20, A.s. Dong [off-list ref] wrote:

quoted

[...]

quoted

}

quoted

+
+static struct imx_sc_pm_domain * imx_scu_add_pm_domain(struct
+device *dev, int idx,
+                     const struct imx_sc_pd_range *pd_ranges) {
+       struct imx_sc_pm_domain *sc_pd;
+       int ret;
+
+       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
+       if (!sc_pd)
+               return ERR_PTR(-ENOMEM);
+
+       sc_pd->rsrc = pd_ranges->rsrc + idx;
+       sc_pd->pd.power_off = imx_sc_pd_power_off;
+       sc_pd->pd.power_on = imx_sc_pd_power_on;

So, this means you are going to register one genpd per device (one
for each uart, pwm etc), but there is actually a better option.

What you seems to be needing is a way to "power on/off" devices,
rather than the PM domain. Right? The PM domain, is managed
internally

by the FW, no?

quoted

quoted

Yes, you're right.

quoted

In any case, it looks like what you should do is to implement the
->attach|detach_dev() callback for the genpd and use the regular
of_genpd_add_provider_simple(). From within the ->attach_dev(),
you should 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, you need to implement the ->stop() and ->start()
callbacks of genpd, which is where you "power on/off" devices,
rather than using the above
->power_on|off() callbacks.

Please have a look a drivers/soc/ti/ti_sci_pm_domains.c.
Apologize, I should have pointed you towards that reference
already in the earlier

version.

quoted

quoted

Appreciated for such detailed explanation. lt's a good suggestion
and I really like to switch to it.  However, after digged a bit
more, i found a few

blocking issues:

quoted

1. The .attach_dev() of power domain infrastructure still does not
support

multi domains case.

quoted

It looks like there's no way for .attach_dev() to understand which
sub 'power

domain'

quoted

the device is attaching for one global power domain provider like
ti_sci as

you suggested.

quoted

Secondly, the struct device *dev passed into is a virtual PD device.
It does not help for parsing the real device resource id.

So framework probably needs some extension to support multi power

domain cases.

Right, you are correct.

Let me think about this and see what I can come up with - unless you
already have something you want to propose?

I have thought we may need add two more params (real dev and pd index)
for
.attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
But I'm not sure if they are proper.

Right.

I don't think we need to change the definition of the callbacks.
However, from within the callbacks (or at least the ->attach_dev()) we need
access to the data you suggest above.

To make that data available, genpd needs to store it in the per device struct
generic_pm_domain_data, but of course before invoking the
->attach_dev() callback.

Yes, that's an option.
Currently gpd_data is allocated in genpd_add_device() which looks like to be
designed to attach device to the specified power domain passed in, so it's
unware of whether it's multi domains or not. Then we may still need fine
tune those functions in different abstract layers.

quoted

quoted

quoted

2. It also breaks most of current drivers as the driver probe
sequence behavior changed If removing .power_on() and .power_off()
callback and

use .start() and .stop() instead.

quoted

genpd_dev_pm_attach will only power up the domain and attach
device, but will not call .start() which relies on device runtime
pm. That means the device power is still not up before running
driver probe function. For SCU enabled platforms, all device
drivers accessing registers/clock without power domain enabled will
trigger a HW access error. That means we need fix most of drivers
probe sequence with proper

runtime pm. So I'm a bit wondering whether we should still keep the exist

way.

quoted

quoted

I see what you are trying to say, but I am not sure why you consider
this as they would break? How did they work in the first place?

Most drivers does not require power domains before. E.g. GPIO, I2C, PWM,

MMC.

quoted

Or even we need power domain(PUs/PCIE), they're enabled in
.power_on()/.power_off() which has been already up before probe.

quoted

Anyway, the problem you are talking about have so far been addressed
by making buses/drivers to call pm_runtime_enable() and
pm_runtime_get_sync() - before trying to probe their devices.

Yes, called them early before accessing clock and hw registers.

quoted

Do you have an idea of the amount of drivers that needs to fixed, in
regards to this?

So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc.
Suppose other need the same fix as we have not met this situation before.

Okay, I see.

quoted

quoted

I guess an option could be to, that via the ->attach_dev() callback
perform the same operations as also being done during ->start(). Of
course, you may need to keep a flag about this being done, so that
operations that need to be balanced with get/put or the like is
managed correctly, the next time
->start|stop() becomes called.

That might be a way to try. The follow seems may introduce a bit
complexities and driver needs to maintainer the runtime status along
with framework in order to keep the balance?

Yeah, it's likely not going to be very elegant, but should be rather self
contained in the PM domain driver and could serve as a way forward - while
working on fixing the drivers long term.

quoted

quoted

quoted

In summary, we probably may not be able to try ti_sci way before
fixing

above two issues.

quoted

Do you think I should wait for them to be fixed or if we could use
the current

way at first?

quoted

I might be a little intend to the second way as we now have a lot
upstreaming work pending on this. But please let me know if you
have a

different idea.

As this is also about deploying a DTB with a correctly modeled HW, I
think we need to fix the above issues.

DTB are the same with #power-domain-cells 1 (resource ID) in either case.
So DTB don't need change and driver could be optimized.

You are correct.

Although, how does $subject patch solves the case where a device may have
multiple PM domains attached to it? Or is left as a future improvement?

IIRC this patch does not have such issue for multi PM domains as we have
individual power domains for each resources. Devices will be attached to
each individual domains in __genpd_dev_pm_attach rather than the single
one global virtual SCU domain. Furthermore we're using domains .power_on()/
power_off() callback which means the power will be up before the probe.
(no the issue like when using .attach_dev()/.detach_dev() and run .start()/stop()
via runtime pm).

Actually I'm a bit wondering that if abstracting SCU domains into one
global virtual domain is certainly better than individual domains. For SCU based SoCs,
the power domain service is provided by SCU firmware, users don't have to care
too much about the HW internals. And according to SCU definition, each devices is
associated with a power domain, devices should treat it as a separate domain
and have to enable it before accessing HW. So one single global domain looks like
not exactly as SCU firmware defines. Not quite sure, but a bit feel like that....

Anyway, give us more time to fine tune it in the future might be a choice IMHO.

Thanks

Regards
Dong Aisheng

Kind regards
Uffe