Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Ulf Hansson]

On Fri, 9 Jul 2021 at 15:47, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
>
> On Fri 09 Jul 08:22 CDT 2021, Ulf Hansson wrote:
>
> > On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > >
> > > Hi Ulf,
> > >
> > > Thanks for your patch!
> > >
> > > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > > According to the common power domain DT bindings, a power domain provider
> > > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > > a provider has a "power-domains" property, it means that it has a parent
> > > > domain.
> > >
> > > OK.
> > >
> > > > It has turned out that some OF nodes that represents a genpd provider may
> > > > also be compatible with a regular platform device. This leads to, during
> > > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> > >
> > > Why is that wrong?
> >
> > It may lead to that the struct device that corresponds to a genpd
> > provider may be attached to the parent domain. In other words, the
> > parent domain will not only be controlled by a child domain
> > (corresponding to the provider), but also through the provider's
> > struct device. As far as I can tell, this has never been the intent
> > for how things should work in genpd.
> >
> > So wrong or not, I guess it depends on what you expect to happen.
> >
> > Do you see an issue with changing this?
> >
>
> But this exactly what we have in the case of the "dispcc" in the
> Qualcomm platform that Dmitry is working on.
>
> The provider driver needs the parent power-domain to be powered in order
> to poke the registers and then it is the parent of the power-domains
> exposed.
>
> If I understand your proposed patch we'll have to manually attach the
> parent domain to the struct device of the controller with this patch?

Not even that would work after $subject patch, as it prevents
providers from being attached to a domain.

It sure sounds like you need to control power for the parent domain,
not only by registering a child domain to it.

>
> Is the Qualcomm case unique or will this change cut power do other genpd
> providers assuming the same?

I think the Qualcomm case is a bit unique or at least the first I
heard of. However, this change would affect all and of course we must
not break things.

>
>
>
> Worth mentioning as we discuss this is that we have another genpd
> provider, where I think the exposed genpds are parented by a few
> different (each one with a specific) parent domains. In this case we'd
> be forced to manually attach the genpd provider to the parent domain
> that it actually is powered by (as no automatic attachment happens when
> multiple domains are specified).

Yes, that's correct (assuming we don't apply $subject patch).

To sum up:

Rafael I am withdrawing the $subject patch, it seems like it may break
existing expectations of what will happen during attach.

Moreover, it may actually be beneficial to allow the attach to succeed
for the Qcom case, so let's leave this as is.

Kind regards
Uffe