Reviving this old thread because this commit has lead to a couple bugs
now.
Quoting Ulf Hansson (2022-06-22 03:26:52)
> On Fri, 17 Jun 2022 at 21:58, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> >
> > Hi Bjorn and Dmitry,
> >
> > Yu reported a lockdep warning coming from the gdsc driver. It looks like
> > the runtime PM usage in gdsc.c is causing lockdep to see an AA deadlock
> > possibility with 'genpd->mlock'. I suspect this is because we have
> > commit 1b771839de05 ("clk: qcom: gdsc: enable optional power domain
> > support"), and that is now calling runtime PM code from within the genpd
> > code.
This commit has caused a deadlock at boot for Doug[1] and I see that the
camera driver on Google CoachZ and Wormdingler devices doesn't work
after resuming from suspend once this commit is applied. I'm leaning
towards sending a revert, because it seems to cause 3 issues while
removing the regulator hack that was in place to enable MMCX. This patch
is cleaning up the hack, but trading the hack for three more problems.
> I think genpd already has nested lock support, so the only
> > solution is to not use runtime PM from within genpd code and start
> > expressing genpd parent relationships in genpd itself?
>
> Not sure exactly what you mean here, but yes, expressing the
> parent/child domain relationship is always needed.
>
> Having gdsc_disable() to do runtime PM calls (gdsc_pm_runtime_put())
> seems awkward to me. Why is that needed, more exactly?
It seems like this is needed so that the gdsc_enable() and
gdsc_disable() calls can read/write the registers for the genpd, while
those registers live in some clk controller that needs either a
different clk (probably some AHB clk) or another genpd to be enabled. It
looks like for qcom,sm8250-dispcc it relies on MMCX gdsc (which is a
genpd). From a hardware view, the MDSS_GDSC provided by the display clk
controller is probably not a sub-domain of MMCX. Instead, we need to
have MMCX enabled so that we can access the registers for the MDSS GDSC.
My question is if it makes sense to simply describe that the GDSCs
provided by a device are sub-domains of whatever power domains are
listed in DT for that device? I think if we did that here for sm8250
dispcc, we wouldn't need to use runtime PM within the genpd code
assuming that the MMCX parent genpd is enabled before we attempt to
read/write the dispcc gdsc registers. Hopefully that is also done, i.e.
enabling parent domains before enabling child domains if the parent is
disabled.
Is this already being done with pm_genpd_add_subdomain() in
gdsc_register()? I see that we're attaching that to dispcc's struct
device::pm_domain, but I assume that is different from the MMCX genpd.
Maybe that is the problem here. Dmitry can you further describe the
problem being solved?
>
> > Or maybe genpd
> > needs to drop locks while calling down into gdsc_disable() and reacquire
> > them after that?
>
> Nope, that doesn't work. This internals of genpd relies on this
> behaviour, as it allows it to properly deal with power-on|off for
> parent/child domains, for example.
Ok.
[1] https://lore.kernel.org/r/20220922154354.2486595-1-dianders@xxxxxxxxxxxx
