On Fri, 9 Jul 2021 at 16:04, Bjorn Andersson [off-list ref] wrote:

On Fri 09 Jul 08:14 CDT 2021, Ulf Hansson wrote:

quoted

On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
[off-list ref] wrote:

quoted

On Fri, 9 Jul 2021 at 15:18, Ulf Hansson [off-list ref] wrote:

quoted

On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
[off-list ref] wrote:

quoted

On Fri, 9 Jul 2021 at 12:33, Ulf Hansson [off-list ref] wrote:

quoted

On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
[off-list ref] wrote:

quoted

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

[...]

quoted

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.

quoted

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.

quoted

This is exactly what the child/parent domain support in genpd is there
to help with.

quoted

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.

Kind regards
Uffe