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

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);
+		}
 	}
 
 	/*