On 1/29/23 04:28, Patrick Wildt wrote:
Am Sun, Jan 29, 2023 at 02:46:36AM +0100 schrieb Marek Vasut:
On 1/28/23 23:14, Patrick Wildt wrote:
The PCIe power domains are dependant on each other, which is why
the device tree makes both PCIe controllers reference the PCIe1
power domain, which then depends on the PCIe2 power domain.
Enabling the parent power domain used to be part of the driver,
but got partially lost in the rewrite. Add the enable call back
to be able to power up PCIe2.
Signed-off-by: Patrick Wildt <patr...@blueri.se>
---
drivers/power/domain/imx8m-power-domain.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/power/domain/imx8m-power-domain.c
b/drivers/power/domain/imx8m-power-domain.c
index 145f6ec0cd..d8e9ce3291 100644
--- a/drivers/power/domain/imx8m-power-domain.c
+++ b/drivers/power/domain/imx8m-power-domain.c
@@ -330,6 +330,9 @@ static int imx8m_power_domain_on(struct power_domain
*power_domain)
u32 pgc;
int ret;
+ if (pdata->has_pd)
+ power_domain_on(&pdata->pd);
+
if (pdata->clk.count) {
ret = clk_enable_bulk(&pdata->clk);
if (ret) {
One problem with this patch is that it does not turn the power domain back
OFF in imx8m_power_domain_off().
That's not true. That function already turns the power domain off, it
just never enables them in the first place. The enabling was lost when
the code was rewritten to not use smcc but the disabling is still there:
static int imx8m_power_domain_off(struct power_domain *power_domain)
{
(...)
if (pdata->has_pd)
power_domain_off(&pdata->pd);
return 0;
(...)
}
Doh, sorry, I did miss that one. Indeed, the imx power domain driver is
not symmetrical now.
What might be missing though is error handling to turn the power domain
back off when enabling clocks/power up fails. In case this needs to be
fixed elsewhere, then maybe we need to *remove* the call to power_domain_
off().
However, the driver should not have to care about that. It is the uclass job
to turn on any prerequisite power domains, which I believe happens in:
drivers/power/domain/power-domain-uclass.c
dev_power_domain_ctrl()
However, I suspect the code fails to recurse through more than one level of
power domains and therefore doesn't enable the power domains all the way up
the power domain tree. I also think this would be the fix -- recurse in
dev_power_domain_ctrl() if there are upstream domains to enable, enable them
in that recursive call, and then enable the current power domain using
power_domain_on() .
Can you take a closer look at the uclass and the way it enables (or fail to)
the upstream domains instead ?
I guess one issue is that I was calling power_domain_on() directly instead of
dev_power_domain_on() in my PCIe driver.
That sounds like a good idea.
And maybe I shouldn't even need to
have the PCIe driver call that one function anyway, maybe uclass is supposed
to turn on my PCIe controllers power domain automatically.
device_probe() does call dev_power_domain_on(), so that sounds like a
good idea too.
As you mentioned, there's no "parent handling" in dev_power_domain_ctrl().
Something like this could work, but I'm not sure that's better. Also I can't
judge if there are any other side-effects of this. Still feels a bit like a
layer violation
How so ?
, but maybe it's the right thing to do.
for (i = 0; i < count; i++) {
ret = power_domain_get_by_index(dev, &pd, i);
if (ret)
return ret;
if (on) {
ret = dev_power_domain_on(pd.dev);
if (!ret)
ret = power_domain_on(&pd);
} else {
ret = power_domain_off(&pd);
if (!ret)
ret = dev_power_domain_off(pd.dev);
}
}
I'll give this a go after some sleep.
I think this makes sense -- the domain should enable all its upstream
domains first, before enabling itself.
It might also make sense to remove the aforementioned power_domain_off()
from imx8m-power-domain.c once the dev_power_domain_ctrl() is fixed.
