[PATCH] imx: power-domain: enable parent power domain

[Marek Vasut]

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 <patrick at 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.