On Fri, 9 Jul 2021 at 17:11, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: > > On Fri, 9 Jul 2021 at 15:22, Dmitry Baryshkov > <dmitry.baryshkov@xxxxxxxxxx> wrote: > > > > On Fri, 9 Jul 2021 at 16:14, Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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. > > > > > > 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. > > > > > > This is exactly what the child/parent domain support in genpd is there > > > to help with. > > > > This is done in this patchset. If we stop attaching dispcc to the MMCX > > genpd, I'll have to locate it in a different way, but the idea is > > implemented here. > > Right. Perhaps it's not such a bad idea after all as it gives you two things: > > 1) The handle to the MMCX PM domain, which makes sure it has been > registered too before dispcc gets probed. > 2) The possibility to control power for the MMCX PM domain via runtime > PM for the dispcc device. This seems useful for your use case. > > > > > > > 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. > > > > So, which device should I pass to clk_register to handle runtime PM > > for the provided clocks? dispcc, should I not? > > Right, anything but dispcc seems wrong. > > > Then if the dispcc is not attached, we will have to manually handle > > MMCX from dispcc's runtime pm callbacks. Correct? > > Yep - and we don't want that either. > > > > > Could you please be more specific, why is it so wrong to attach dispcc > > to the MMCX genpd? > > In the end it seems like I just needed to make my brain feel a little > more comfortable with the ideas that you put forward. > > It should work fine, I think! My apologies for all the noise. No problem, it is always better to have a discussion (and a conclusion) rather than not to have it. Thank you for your comments! -- With best wishes Dmitry
