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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Jul 2021 14:18:20 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Taniya Das <tdas@...eaurora.org>,
        Jonathan Marek <jonathan@...ek.ca>,
        Michael Turquette <mturquette@...libre.com>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "Bryan O'Donoghue" <bryan.odonoghue@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power
 domain support

On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
<dmitry.baryshkov@...aro.org> wrote:
>
> On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > <dmitry.baryshkov@...aro.org> 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@...aro.org>
> >
> > [...]
> >
> > > 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,

We don't need this, it's redundant and weird to me.

Also I am kind of worried that you will hit another new path in genpd,
causing locking issues etc, as it has not been designed to work like
this (a provider device and a child domain sharing the same "parent").

>  - provides MDSS_GDSC

It's perfectly fine that dispcc acts as a genpd provider. In this
case, the corresponding PM domain should be assigned as a child for
the parent MMCX domain. That should make this work, I think.

>  - provides clocks

That sounds reasonable as well.

>
> >
> > > +                       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