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

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