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

[Marek Vasut]

On 1/29/23 11:47, Patrick Wildt wrote:
> On Sun, Jan 29, 2023 at 05:00:15AM +0100, Marek Vasut wrote:
>> 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.
> 
> Just tested, this diff works for me.  Should I send it as a patch to the
> list?

Yes please !

> One thing that worries me is that there's no refcounting to make sure
> that a parent that is used twice doesn't get disabled.  That might be
> something to follow-up on.

I agree. Could you try and add that too ? Separate patch is fine.

> diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
> index f6286c70c1..3b3caf6164 100644
> --- a/drivers/power/domain/power-domain-uclass.c
> +++ b/drivers/power/domain/power-domain-uclass.c
> @@ -138,10 +138,15 @@ static int dev_power_domain_ctrl(struct udevice *dev, bool on)
>   		ret = power_domain_get_by_index(dev, &pd, i);
>   		if (ret)
>   			return ret;
> -		if (on)
> -			ret = power_domain_on(&pd);
> -		else
> +		if (on) {
> +			ret = dev_power_domain_on(pd.dev);

You might want to add a fail path here:

if (ret)
   goto err_power_on;

...
err_poewr_on:
  ... un-do the power on ...

> +			if (!ret)
> +				ret = power_domain_on(&pd);
> +		} else {
>   			ret = power_domain_off(&pd);
> +			if (!ret)
> +				ret = dev_power_domain_off(pd.dev);
> +		}
>   	}
>   
>   	/*

[...]