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

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

[Patrick Wildt]

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@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) {
-- 
2.39.1

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

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

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 ?

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

[Patrick Wildt]

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@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;
	(...)
}

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

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

Cheers,
Patrick

Re: [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@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.

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

[Patrick Wildt]

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@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?

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.

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);
+			if (!ret)
+				ret = power_domain_on(&pd);
+		} else {
 			ret = power_domain_off(&pd);
+			if (!ret)
+				ret = dev_power_domain_off(pd.dev);
+		}
 	}
 
 	/*

Re: [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@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);
> +		}
>   	}
>   
>   	/*

[...]