Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
> >
> > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > domain. Currently we used a regulator to enable this domain on demand,
> > however this has some consequences, as genpd code is not reentrant.
> >
> > Teach Qualcomm clock controller code about setting up power domains and
> > using them for gdsc control.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> [...]
>
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 51ed640e527b..9401d01533c8 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> >                         continue;
> >                 scs[i]->regmap = regmap;
> >                 scs[i]->rcdev = rcdev;
> > +               scs[i]->pd.dev.parent = desc->dev;
> >                 ret = gdsc_init(scs[i]);
> >                 if (ret)
> >                         return ret;
> > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> >                         continue;
> >                 if (scs[i]->parent)
> >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
>
> So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> called for gdsc platform device from the platform bus', to try to
> attach the device to its corresponding PM domain.
>
> Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> shouldn't really try to attach a device to its PM domain, when its OF
> node (dev->of_node) contains a "#power-domain-cells" specifier. This
> is because it indicates that the device belongs to a genpd provider
> itself. In this case, a "power-domains" specifier tells that it has a
> parent domain.
>
> I will post a patch that fixes this asap.

I think there is nothing to fix here. The dispcc/videocc drivers
provide clocks in addition to the gdsc power domain. And provided
clocks would definitely benefit from having the dispcc device being
attached to the power domain which governs clock registers (MMCX in
our case). Thus I think it is perfectly valid to have:

rpmhpd device:
 - provides MMCX domain.

dispcc device:
 - is attached to the MMCX domain,
 - provides MDSS_GDSC
 - provides clocks

>
> > +                       pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> >         }
> >
> >         return of_genpd_add_provider_onecell(dev->of_node, data);
> > @@ -457,6 +460,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
> >                         continue;
> >                 if (scs[i]->parent)
> >                         pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
>
> Ditto.
>
> > +                       pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> >         }
> >         of_genpd_del_provider(dev->of_node);
> >  }
> > --
> > 2.30.2
> >
>
> Kind regards
> Uffe



-- 
With best wishes
Dmitry