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

[Dmitry Baryshkov]

On Fri, 9 Jul 2021 at 17:13, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Fri, 9 Jul 2021 at 16:04, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
> >
> > On Fri 09 Jul 08:14 CDT 2021, Ulf Hansson wrote:
> >
> > > On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
> > > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > > > > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > 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,
> > > > >
> > > > > 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").
> > > >
> > > > So, which domain should the dispcc device belong to? It's registers
> > > > are powered by the MMCX domain. I can not attach it to the child
> > > > (GDSC) domain either: in the case of videocc there are 4 child
> > > > domains.
> > >
> > > The dispcc device should *not* be attached to a PM domain.
> > >
> >
> > dispcc is powered by the MMCX power domain, so it needs to be on if you
> > want to touch the registers.
> >
> > I presume that for genpd this might not be a problem as long as all the
> > exposed power domains are parented by the genpd provider's parent, as
> > the core would turn the parent on before and turn off after performing
> > those operations. But without attaching to the domain we don't have
> > power to get through probe/registration.
> >
> > Further more, dispcc is also a clock driver and there's certainly
> > operations where the genpd framework won't save us.
> >
> > > Instead it should be registered as a genpd provider and the
> > > corresponding PM domains it provides, should be assigned as child
> > > domains to the MMCX domain.
> > >
> >
> > Right, this relationship is today missing and is what Dmitry needs to
> > add - so that the parent domains stays powered even when we're not
> > keeping the parent domain enabled to poke the dispcc.
> >
> > > This is exactly what the child/parent domain support in genpd is there
> > > to help with.
> > >
> > > > An alternative would be to request that all users of the provided
> > > > clocks power on one of the child domains. However this is also not
> > > > perfect. If some generic code (e.g. clock framework) calls into
> > > > provided clocks (e.g. because of assigned-clock-rates), this can
> > > > happen w/o proper power domain being powered up yet.
> > >
> > > Issues with power on/off synchronization during genpd initializations
> > > and genpd provider registration, certainly need to be fixed and I am
> > > happy to help. However, my point is that I think it's a bad idea to
> > > fix it through modelling the PM domain hierarchy in an incorrect way.
> > >
> >
> > This was my initial feeling to the patch as well and I think it might be
> > better to push the pm_runtime_get/put operations into gdsc.c - in
> > particular if you're saying that the general case is not for the genpd
> > provider itself to be powered by the specified parent domain.
> >
> > At least we could start by doing it manually in gdsc.c and possibly move
> > it into the framework if we're confident that it's a good idea.
>
> Yes, better to start making this Qcom specific, then we can take it from there.

I will re-add pm_runtime calls to gdsc.c and send a v3.

-- 
With best wishes
Dmitry