Date:   Tue, 1 Nov 2022 20:29:20 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Bjorn Andersson <andersson@...nel.org>
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org,
        linux-clk@...r.kernel.org, patches@...ts.linux.dev,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        linux-arm-msm@...r.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Johan Hovold <johan+linaro@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Taniya Das <quic_tdas@...cinc.com>,
        Satya Priya <quic_c_skakit@...cinc.com>,
        Douglas Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>
Subject: Re: [PATCH] clk: qcom: gdsc: Remove direct runtime PM calls

Quoting Bjorn Andersson (2022-11-01 19:49:27)
> On Tue, Nov 01, 2022 at 04:34:21PM -0700, Stephen Boyd wrote:
> > We shouldn't be calling runtime PM APIs from within the genpd
> > enable/disable path for a couple reasons.
> [..][
> > Upon closer inspection, calling runtime PM APIs like this in the GDSC
> > driver doesn't make sense. It was intended to make sure the GDSC for the
> > clock controller providing other GDSCs was enabled, specifically the
> > MMCX GDSC for the display clk controller on SM8250 (sm8250-dispcc), so
> > that GDSC register accesses succeeded. That will already happen because
> > we make the 'dev->pm_domain' a parent domain of each GDSC we register in
> > gdsc_register() via pm_genpd_add_subdomain(). When any of these GDSCs
> > are accessed, we'll enable the parent domain (in this specific case
> > MMCX).
> >
>
> It's correct that adding the GDSCs as subdomains for the device's
> parent-domain will ensure that enabling a GDSC will propagate up and
> turn on the (typically) rpmhpd resource.
>
> But the purpose for the explicit calls was to ensure that the clock
> controller itself is accessible. It's been a while since I looked at
> this, but iirc letting MMCX to turn off would cause the register access
> during dispcc probing to fail - similar to how
> clk_pm_runtime_get()/put() ensures the clock registers are accessible.

The dispcc and videocc on sm8250 don't use pm_clk APIs. They do use
pm_runtime APIs during probe (i.e. pm_runtime_resume_and_get()). That
will enable the MMCX domain and keep it on. Then when the GDSCs are
registered it will create genpds for each GDSC and make them subdomains
of the 'dev->pm_domain' genpd for MMCX. If the GDSCs are enabled at
probe time they will increment the count on MMCX to put the count into
sync between MMCX and the GDSC provided.

The clk framework also has runtime PM calls throughout the code to make
sure the device is runtime resumed when it is accessed. Maybe the
problem is if probe defers and enough runtime puts are called to runtime
suspend the device thus disabling MMCX? Can MMCX really ever be disabled
or does disabling it act as a one way disable where you can never enable
it again?

Or maybe this is the problem where not all constraints are determined
yet but we're letting runtime PM put calls from the dispcc device shut
down the entire multimedia subsystem while other devices that are within
the same domain haven't probed and been able to sync their state but
they're actively accessing the bus (i.e. continuous splash screen). I
could see this problem being avoided by the pm_runtime_get() call in
gdsc registration keeping MMCX on forever because there isn't a matching
put anywhere.

>
> Perhaps I misunderstood something in the process, or lost track of the
> actual issues?

I dunno. It clearly is a problem to call runtime PM in the noirq phase
of system suspend though.

Powered by blists - more mailing lists