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