[v3,0/4] Allow genpd providers to power off domains on sync state

Message ID	20230327193829.3756640-1-abel.vesa@linaro.org (mailing list archive)
Headers	show Return-Path: <linux-clk-owner@vger.kernel.org> From: Abel Vesa <abel.vesa@linaro.org> To: "Rafael J. Wysocki" <rafael@kernel.org>, Kevin Hilman <khilman@kernel.org>, Ulf Hansson <ulf.hansson@linaro.org>, Pavel Machek <pavel@ucw.cz>, Len Brown <len.brown@intel.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Bjorn Andersson <andersson@kernel.org>, Andy Gross <agross@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Mike Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Saravana Kannan <saravanak@google.com> Cc: linux-pm@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, Doug Anderson <dianders@chromium.org>, Matthias Kaehlcke <mka@chromium.org> Subject: [PATCH v3 0/4] Allow genpd providers to power off domains on sync state Date: Mon, 27 Mar 2023 22:38:25 +0300 Message-Id: <20230327193829.3756640-1-abel.vesa@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Allow genpd providers to power off domains on sync state \| expand [v3,0/4] Allow genpd providers to power off domains on sync state [v3,1/4] PM: domains: Allow power off queuing from providers [v3,2/4] soc: qcom: rpmhpd: Do proper power off when state synced [v3,3/4] clk: qcom: gdsc: Avoid actual power off until sync state [v3,4/4] clk: qcom: Add sync state callback to all providers

Abel Vesa March 27, 2023, 7:38 p.m. UTC

There have been already a couple of tries to make the genpd "disable
unused" late initcall skip the powering off of domains that might be
needed until later on (i.e. until some consumer probes). The conclusion
was that the provider could return -EBUSY from the power_off callback
until the provider's sync state has been reached. This patch series tries
to provide a proof-of-concept that is working on Qualcomm platforms.

I've been doing extensive testing on SM8450, but I've also spinned this
on my X13s (SC8280XP). Both patches that add the sync state callback to
the SC8280XP and SM8450 are here to provide context. Once we agree on
the form, I intend to add the sync state callback to all gdsc providers.

Currently, some of the gdsc providers might not reach sync state due to
list of consumers not probing yet (or at all). The sync state can be
enforced by writing 1 to the state_synced sysfs attribute of the
provider, thanks to Saravana's commit [1] which has been already merged.

[1] https://lore.kernel.org/r/20230304005355.746421-3-saravanak@google.com

V2 (RFC) of this patchset was here:
https://lore.kernel.org/all/20230320134217.1685781-1-abel.vesa@linaro.org/

Changes since v2:
 * renamed genpd_queue_power_off_work to pm_genpd_queue_power_off and added
   comment about its purpose w.r.t. it being exported.
 * added the qcom_cc generic sync state callback to all providers that
   register GDSCs, instead of SM8450 and SC8280XP

Changes since v1:
 * Added the qcom_cc sync state callback which calls in turn the gdsc one
 * dropped extra semicolon from pm_domain.h

Abel Vesa (4):
  PM: domains: Allow power off queuing from providers
  soc: qcom: rpmhpd: Do proper power off when state synced
  clk: qcom: gdsc: Avoid actual power off until sync state
  clk: qcom: Add sync state callback to all providers

 drivers/base/power/domain.c            | 18 ++++++++++--------
 drivers/clk/qcom/apss-ipq6018.c        |  1 +
 drivers/clk/qcom/camcc-sc7180.c        |  1 +
 drivers/clk/qcom/camcc-sc7280.c        |  1 +
 drivers/clk/qcom/camcc-sdm845.c        |  1 +
 drivers/clk/qcom/camcc-sm6350.c        |  1 +
 drivers/clk/qcom/camcc-sm8250.c        |  1 +
 drivers/clk/qcom/camcc-sm8450.c        |  1 +
 drivers/clk/qcom/common.c              | 19 +++++++++++++++++++
 drivers/clk/qcom/common.h              |  2 ++
 drivers/clk/qcom/dispcc-qcm2290.c      |  1 +
 drivers/clk/qcom/dispcc-sc7180.c       |  1 +
 drivers/clk/qcom/dispcc-sc7280.c       |  1 +
 drivers/clk/qcom/dispcc-sc8280xp.c     |  1 +
 drivers/clk/qcom/dispcc-sdm845.c       |  1 +
 drivers/clk/qcom/dispcc-sm6115.c       |  1 +
 drivers/clk/qcom/dispcc-sm6125.c       |  1 +
 drivers/clk/qcom/dispcc-sm6350.c       |  1 +
 drivers/clk/qcom/dispcc-sm6375.c       |  1 +
 drivers/clk/qcom/dispcc-sm8250.c       |  1 +
 drivers/clk/qcom/dispcc-sm8450.c       |  1 +
 drivers/clk/qcom/dispcc-sm8550.c       |  1 +
 drivers/clk/qcom/gcc-apq8084.c         |  1 +
 drivers/clk/qcom/gcc-ipq806x.c         |  1 +
 drivers/clk/qcom/gcc-ipq8074.c         |  1 +
 drivers/clk/qcom/gcc-mdm9615.c         |  1 +
 drivers/clk/qcom/gcc-msm8660.c         |  1 +
 drivers/clk/qcom/gcc-msm8909.c         |  1 +
 drivers/clk/qcom/gcc-msm8916.c         |  1 +
 drivers/clk/qcom/gcc-msm8939.c         |  1 +
 drivers/clk/qcom/gcc-msm8953.c         |  1 +
 drivers/clk/qcom/gcc-msm8960.c         |  1 +
 drivers/clk/qcom/gcc-msm8974.c         |  1 +
 drivers/clk/qcom/gcc-msm8976.c         |  1 +
 drivers/clk/qcom/gcc-msm8994.c         |  1 +
 drivers/clk/qcom/gcc-msm8996.c         |  1 +
 drivers/clk/qcom/gcc-msm8998.c         |  1 +
 drivers/clk/qcom/gcc-qcm2290.c         |  1 +
 drivers/clk/qcom/gcc-qcs404.c          |  1 +
 drivers/clk/qcom/gcc-qdu1000.c         |  1 +
 drivers/clk/qcom/gcc-sa8775p.c         |  1 +
 drivers/clk/qcom/gcc-sc7180.c          |  1 +
 drivers/clk/qcom/gcc-sc7280.c          |  1 +
 drivers/clk/qcom/gcc-sc8180x.c         |  1 +
 drivers/clk/qcom/gcc-sc8280xp.c        |  1 +
 drivers/clk/qcom/gcc-sdm660.c          |  1 +
 drivers/clk/qcom/gcc-sdm845.c          |  1 +
 drivers/clk/qcom/gcc-sdx55.c           |  1 +
 drivers/clk/qcom/gcc-sdx65.c           |  1 +
 drivers/clk/qcom/gcc-sm6115.c          |  1 +
 drivers/clk/qcom/gcc-sm6125.c          |  1 +
 drivers/clk/qcom/gcc-sm6350.c          |  1 +
 drivers/clk/qcom/gcc-sm6375.c          |  1 +
 drivers/clk/qcom/gcc-sm7150.c          |  1 +
 drivers/clk/qcom/gcc-sm8150.c          |  1 +
 drivers/clk/qcom/gcc-sm8250.c          |  1 +
 drivers/clk/qcom/gcc-sm8350.c          |  1 +
 drivers/clk/qcom/gcc-sm8450.c          |  1 +
 drivers/clk/qcom/gcc-sm8550.c          |  1 +
 drivers/clk/qcom/gdsc.c                | 26 ++++++++++++++++++++++++++
 drivers/clk/qcom/gdsc.h                |  6 ++++++
 drivers/clk/qcom/gpucc-msm8998.c       |  1 +
 drivers/clk/qcom/gpucc-sc7180.c        |  1 +
 drivers/clk/qcom/gpucc-sc7280.c        |  1 +
 drivers/clk/qcom/gpucc-sc8280xp.c      |  1 +
 drivers/clk/qcom/gpucc-sdm660.c        |  1 +
 drivers/clk/qcom/gpucc-sdm845.c        |  1 +
 drivers/clk/qcom/gpucc-sm6115.c        |  1 +
 drivers/clk/qcom/gpucc-sm6125.c        |  1 +
 drivers/clk/qcom/gpucc-sm6350.c        |  1 +
 drivers/clk/qcom/gpucc-sm6375.c        |  1 +
 drivers/clk/qcom/gpucc-sm8150.c        |  1 +
 drivers/clk/qcom/gpucc-sm8250.c        |  1 +
 drivers/clk/qcom/gpucc-sm8350.c        |  1 +
 drivers/clk/qcom/lcc-ipq806x.c         |  1 +
 drivers/clk/qcom/lpassaudiocc-sc7280.c |  1 +
 drivers/clk/qcom/lpasscc-sc7280.c      |  1 +
 drivers/clk/qcom/lpasscorecc-sc7180.c  |  2 ++
 drivers/clk/qcom/lpasscorecc-sc7280.c  |  2 ++
 drivers/clk/qcom/mmcc-apq8084.c        |  1 +
 drivers/clk/qcom/mmcc-msm8974.c        |  1 +
 drivers/clk/qcom/mmcc-msm8994.c        |  1 +
 drivers/clk/qcom/mmcc-msm8996.c        |  1 +
 drivers/clk/qcom/mmcc-msm8998.c        |  1 +
 drivers/clk/qcom/mmcc-sdm660.c         |  1 +
 drivers/clk/qcom/videocc-sc7180.c      |  1 +
 drivers/clk/qcom/videocc-sc7280.c      |  1 +
 drivers/clk/qcom/videocc-sdm845.c      |  1 +
 drivers/clk/qcom/videocc-sm8150.c      |  1 +
 drivers/clk/qcom/videocc-sm8250.c      |  1 +
 drivers/soc/qcom/rpmhpd.c              | 19 +++++++------------
 include/linux/pm_domain.h              |  4 ++++
 92 files changed, 161 insertions(+), 20 deletions(-)

Saravana Kannan March 28, 2023, 12:17 a.m. UTC | #1

On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@linaro.org> wrote:
>
> There have been already a couple of tries to make the genpd "disable
> unused" late initcall skip the powering off of domains that might be
> needed until later on (i.e. until some consumer probes). The conclusion
> was that the provider could return -EBUSY from the power_off callback
> until the provider's sync state has been reached. This patch series tries
> to provide a proof-of-concept that is working on Qualcomm platforms.

I'm giving my thoughts in the cover letter instead of spreading it
around all the patches so that there's context between the comments.

1) Why can't all the logic in this patch series be implemented at the
framework level? And then allow the drivers to opt into this behavior
by setting the sync_state() callback.

That way, you can land it only for QC drivers by setting up
sync_state() callback only for QC drivers, but actually have the same
code function correctly for non-QC drivers too. And then once we have
this functionality working properly for QC drivers for one kernel
version (or two), we'll just have the framework set the device's
driver's sync_state() if it doesn't have one already.

2) sync_state() is not just about power on/off. It's also about the
power domain level. Can you handle that too please?

3) In your GDSC drivers, it's not clear to me if you are preventing
power off until sync_state() only for GDSCs that were already on at
boot. So if an off-at-boot GDSC gets turned on, and then you attempt
to turn it off before all its consumers have probed, it'll fail to
power it off even though that wasn't necessary?

4) The returning -EBUSY when a power off is attempted seems to be
quite wasteful. The framework will go through the whole sequence of
trying to power down, send the notifications and then fail and then
send the undo notifications. Combined with point (2) I think this can
be handled better at the aggregation level in the framework to avoid
even going that far into the power off sequence.

-Saravana

>
> I've been doing extensive testing on SM8450, but I've also spinned this
> on my X13s (SC8280XP). Both patches that add the sync state callback to
> the SC8280XP and SM8450 are here to provide context. Once we agree on
> the form, I intend to add the sync state callback to all gdsc providers.
>
> Currently, some of the gdsc providers might not reach sync state due to
> list of consumers not probing yet (or at all). The sync state can be
> enforced by writing 1 to the state_synced sysfs attribute of the
> provider, thanks to Saravana's commit [1] which has been already merged.
>
> [1] https://lore.kernel.org/r/20230304005355.746421-3-saravanak@google.com
>
> V2 (RFC) of this patchset was here:
> https://lore.kernel.org/all/20230320134217.1685781-1-abel.vesa@linaro.org/
>
> Changes since v2:
>  * renamed genpd_queue_power_off_work to pm_genpd_queue_power_off and added
>    comment about its purpose w.r.t. it being exported.
>  * added the qcom_cc generic sync state callback to all providers that
>    register GDSCs, instead of SM8450 and SC8280XP
>
> Changes since v1:
>  * Added the qcom_cc sync state callback which calls in turn the gdsc one
>  * dropped extra semicolon from pm_domain.h
>
> Abel Vesa (4):
>   PM: domains: Allow power off queuing from providers
>   soc: qcom: rpmhpd: Do proper power off when state synced
>   clk: qcom: gdsc: Avoid actual power off until sync state
>   clk: qcom: Add sync state callback to all providers
>
>  drivers/base/power/domain.c            | 18 ++++++++++--------
>  drivers/clk/qcom/apss-ipq6018.c        |  1 +
>  drivers/clk/qcom/camcc-sc7180.c        |  1 +
>  drivers/clk/qcom/camcc-sc7280.c        |  1 +
>  drivers/clk/qcom/camcc-sdm845.c        |  1 +
>  drivers/clk/qcom/camcc-sm6350.c        |  1 +
>  drivers/clk/qcom/camcc-sm8250.c        |  1 +
>  drivers/clk/qcom/camcc-sm8450.c        |  1 +
>  drivers/clk/qcom/common.c              | 19 +++++++++++++++++++
>  drivers/clk/qcom/common.h              |  2 ++
>  drivers/clk/qcom/dispcc-qcm2290.c      |  1 +
>  drivers/clk/qcom/dispcc-sc7180.c       |  1 +
>  drivers/clk/qcom/dispcc-sc7280.c       |  1 +
>  drivers/clk/qcom/dispcc-sc8280xp.c     |  1 +
>  drivers/clk/qcom/dispcc-sdm845.c       |  1 +
>  drivers/clk/qcom/dispcc-sm6115.c       |  1 +
>  drivers/clk/qcom/dispcc-sm6125.c       |  1 +
>  drivers/clk/qcom/dispcc-sm6350.c       |  1 +
>  drivers/clk/qcom/dispcc-sm6375.c       |  1 +
>  drivers/clk/qcom/dispcc-sm8250.c       |  1 +
>  drivers/clk/qcom/dispcc-sm8450.c       |  1 +
>  drivers/clk/qcom/dispcc-sm8550.c       |  1 +
>  drivers/clk/qcom/gcc-apq8084.c         |  1 +
>  drivers/clk/qcom/gcc-ipq806x.c         |  1 +
>  drivers/clk/qcom/gcc-ipq8074.c         |  1 +
>  drivers/clk/qcom/gcc-mdm9615.c         |  1 +
>  drivers/clk/qcom/gcc-msm8660.c         |  1 +
>  drivers/clk/qcom/gcc-msm8909.c         |  1 +
>  drivers/clk/qcom/gcc-msm8916.c         |  1 +
>  drivers/clk/qcom/gcc-msm8939.c         |  1 +
>  drivers/clk/qcom/gcc-msm8953.c         |  1 +
>  drivers/clk/qcom/gcc-msm8960.c         |  1 +
>  drivers/clk/qcom/gcc-msm8974.c         |  1 +
>  drivers/clk/qcom/gcc-msm8976.c         |  1 +
>  drivers/clk/qcom/gcc-msm8994.c         |  1 +
>  drivers/clk/qcom/gcc-msm8996.c         |  1 +
>  drivers/clk/qcom/gcc-msm8998.c         |  1 +
>  drivers/clk/qcom/gcc-qcm2290.c         |  1 +
>  drivers/clk/qcom/gcc-qcs404.c          |  1 +
>  drivers/clk/qcom/gcc-qdu1000.c         |  1 +
>  drivers/clk/qcom/gcc-sa8775p.c         |  1 +
>  drivers/clk/qcom/gcc-sc7180.c          |  1 +
>  drivers/clk/qcom/gcc-sc7280.c          |  1 +
>  drivers/clk/qcom/gcc-sc8180x.c         |  1 +
>  drivers/clk/qcom/gcc-sc8280xp.c        |  1 +
>  drivers/clk/qcom/gcc-sdm660.c          |  1 +
>  drivers/clk/qcom/gcc-sdm845.c          |  1 +
>  drivers/clk/qcom/gcc-sdx55.c           |  1 +
>  drivers/clk/qcom/gcc-sdx65.c           |  1 +
>  drivers/clk/qcom/gcc-sm6115.c          |  1 +
>  drivers/clk/qcom/gcc-sm6125.c          |  1 +
>  drivers/clk/qcom/gcc-sm6350.c          |  1 +
>  drivers/clk/qcom/gcc-sm6375.c          |  1 +
>  drivers/clk/qcom/gcc-sm7150.c          |  1 +
>  drivers/clk/qcom/gcc-sm8150.c          |  1 +
>  drivers/clk/qcom/gcc-sm8250.c          |  1 +
>  drivers/clk/qcom/gcc-sm8350.c          |  1 +
>  drivers/clk/qcom/gcc-sm8450.c          |  1 +
>  drivers/clk/qcom/gcc-sm8550.c          |  1 +
>  drivers/clk/qcom/gdsc.c                | 26 ++++++++++++++++++++++++++
>  drivers/clk/qcom/gdsc.h                |  6 ++++++
>  drivers/clk/qcom/gpucc-msm8998.c       |  1 +
>  drivers/clk/qcom/gpucc-sc7180.c        |  1 +
>  drivers/clk/qcom/gpucc-sc7280.c        |  1 +
>  drivers/clk/qcom/gpucc-sc8280xp.c      |  1 +
>  drivers/clk/qcom/gpucc-sdm660.c        |  1 +
>  drivers/clk/qcom/gpucc-sdm845.c        |  1 +
>  drivers/clk/qcom/gpucc-sm6115.c        |  1 +
>  drivers/clk/qcom/gpucc-sm6125.c        |  1 +
>  drivers/clk/qcom/gpucc-sm6350.c        |  1 +
>  drivers/clk/qcom/gpucc-sm6375.c        |  1 +
>  drivers/clk/qcom/gpucc-sm8150.c        |  1 +
>  drivers/clk/qcom/gpucc-sm8250.c        |  1 +
>  drivers/clk/qcom/gpucc-sm8350.c        |  1 +
>  drivers/clk/qcom/lcc-ipq806x.c         |  1 +
>  drivers/clk/qcom/lpassaudiocc-sc7280.c |  1 +
>  drivers/clk/qcom/lpasscc-sc7280.c      |  1 +
>  drivers/clk/qcom/lpasscorecc-sc7180.c  |  2 ++
>  drivers/clk/qcom/lpasscorecc-sc7280.c  |  2 ++
>  drivers/clk/qcom/mmcc-apq8084.c        |  1 +
>  drivers/clk/qcom/mmcc-msm8974.c        |  1 +
>  drivers/clk/qcom/mmcc-msm8994.c        |  1 +
>  drivers/clk/qcom/mmcc-msm8996.c        |  1 +
>  drivers/clk/qcom/mmcc-msm8998.c        |  1 +
>  drivers/clk/qcom/mmcc-sdm660.c         |  1 +
>  drivers/clk/qcom/videocc-sc7180.c      |  1 +
>  drivers/clk/qcom/videocc-sc7280.c      |  1 +
>  drivers/clk/qcom/videocc-sdm845.c      |  1 +
>  drivers/clk/qcom/videocc-sm8150.c      |  1 +
>  drivers/clk/qcom/videocc-sm8250.c      |  1 +
>  drivers/soc/qcom/rpmhpd.c              | 19 +++++++------------
>  include/linux/pm_domain.h              |  4 ++++
>  92 files changed, 161 insertions(+), 20 deletions(-)
>
> --
> 2.34.1
>

Abel Vesa March 30, 2023, 11:27 a.m. UTC | #2

On 23-03-27 17:17:28, Saravana Kannan wrote:
> On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > There have been already a couple of tries to make the genpd "disable
> > unused" late initcall skip the powering off of domains that might be
> > needed until later on (i.e. until some consumer probes). The conclusion
> > was that the provider could return -EBUSY from the power_off callback
> > until the provider's sync state has been reached. This patch series tries
> > to provide a proof-of-concept that is working on Qualcomm platforms.
> 
> I'm giving my thoughts in the cover letter instead of spreading it
> around all the patches so that there's context between the comments.
> 
> 1) Why can't all the logic in this patch series be implemented at the
> framework level? And then allow the drivers to opt into this behavior
> by setting the sync_state() callback.
> 
> That way, you can land it only for QC drivers by setting up
> sync_state() callback only for QC drivers, but actually have the same
> code function correctly for non-QC drivers too. And then once we have
> this functionality working properly for QC drivers for one kernel
> version (or two), we'll just have the framework set the device's
> driver's sync_state() if it doesn't have one already.

I think Ulf has already NACK'ed that approach here:
[1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com/

And suggested this new approach that this patch series proposes.
(Unless I missunderstood his point)

> 
> 2) sync_state() is not just about power on/off. It's also about the
> power domain level. Can you handle that too please?

Well, this patchset only tries to delay the disabling of unused power
domains until all consumers have had a chance to probe. So we use sync
state only to queue up a power-off request to make sure those unused
ones get disabled.

> 
> 3) In your GDSC drivers, it's not clear to me if you are preventing
> power off until sync_state() only for GDSCs that were already on at
> boot. So if an off-at-boot GDSC gets turned on, and then you attempt
> to turn it off before all its consumers have probed, it'll fail to
> power it off even though that wasn't necessary?

I think we can circumvent looking at a GDSC by knowing it there was ever
a power on request since boot. I'll try to come up with something in the
new version.

> 
> 4) The returning -EBUSY when a power off is attempted seems to be
> quite wasteful. The framework will go through the whole sequence of
> trying to power down, send the notifications and then fail and then
> send the undo notifications. Combined with point (2) I think this can
> be handled better at the aggregation level in the framework to avoid
> even going that far into the power off sequence.

Again, have a look at [1] (above).

Ulf, any thoughts on this 4th point?

> 
> -Saravana
> 
> >
> > I've been doing extensive testing on SM8450, but I've also spinned this
> > on my X13s (SC8280XP). Both patches that add the sync state callback to
> > the SC8280XP and SM8450 are here to provide context. Once we agree on
> > the form, I intend to add the sync state callback to all gdsc providers.
> >
> > Currently, some of the gdsc providers might not reach sync state due to
> > list of consumers not probing yet (or at all). The sync state can be
> > enforced by writing 1 to the state_synced sysfs attribute of the
> > provider, thanks to Saravana's commit [1] which has been already merged.
> >
> > [1] https://lore.kernel.org/r/20230304005355.746421-3-saravanak@google.com
> >
> > V2 (RFC) of this patchset was here:
> > https://lore.kernel.org/all/20230320134217.1685781-1-abel.vesa@linaro.org/
> >
> > Changes since v2:
> >  * renamed genpd_queue_power_off_work to pm_genpd_queue_power_off and added
> >    comment about its purpose w.r.t. it being exported.
> >  * added the qcom_cc generic sync state callback to all providers that
> >    register GDSCs, instead of SM8450 and SC8280XP
> >
> > Changes since v1:
> >  * Added the qcom_cc sync state callback which calls in turn the gdsc one
> >  * dropped extra semicolon from pm_domain.h
> >
> > Abel Vesa (4):
> >   PM: domains: Allow power off queuing from providers
> >   soc: qcom: rpmhpd: Do proper power off when state synced
> >   clk: qcom: gdsc: Avoid actual power off until sync state
> >   clk: qcom: Add sync state callback to all providers
> >
> >  drivers/base/power/domain.c            | 18 ++++++++++--------
> >  drivers/clk/qcom/apss-ipq6018.c        |  1 +
> >  drivers/clk/qcom/camcc-sc7180.c        |  1 +
> >  drivers/clk/qcom/camcc-sc7280.c        |  1 +
> >  drivers/clk/qcom/camcc-sdm845.c        |  1 +
> >  drivers/clk/qcom/camcc-sm6350.c        |  1 +
> >  drivers/clk/qcom/camcc-sm8250.c        |  1 +
> >  drivers/clk/qcom/camcc-sm8450.c        |  1 +
> >  drivers/clk/qcom/common.c              | 19 +++++++++++++++++++
> >  drivers/clk/qcom/common.h              |  2 ++
> >  drivers/clk/qcom/dispcc-qcm2290.c      |  1 +
> >  drivers/clk/qcom/dispcc-sc7180.c       |  1 +
> >  drivers/clk/qcom/dispcc-sc7280.c       |  1 +
> >  drivers/clk/qcom/dispcc-sc8280xp.c     |  1 +
> >  drivers/clk/qcom/dispcc-sdm845.c       |  1 +
> >  drivers/clk/qcom/dispcc-sm6115.c       |  1 +
> >  drivers/clk/qcom/dispcc-sm6125.c       |  1 +
> >  drivers/clk/qcom/dispcc-sm6350.c       |  1 +
> >  drivers/clk/qcom/dispcc-sm6375.c       |  1 +
> >  drivers/clk/qcom/dispcc-sm8250.c       |  1 +
> >  drivers/clk/qcom/dispcc-sm8450.c       |  1 +
> >  drivers/clk/qcom/dispcc-sm8550.c       |  1 +
> >  drivers/clk/qcom/gcc-apq8084.c         |  1 +
> >  drivers/clk/qcom/gcc-ipq806x.c         |  1 +
> >  drivers/clk/qcom/gcc-ipq8074.c         |  1 +
> >  drivers/clk/qcom/gcc-mdm9615.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8660.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8909.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8916.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8939.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8953.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8960.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8974.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8976.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8994.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8996.c         |  1 +
> >  drivers/clk/qcom/gcc-msm8998.c         |  1 +
> >  drivers/clk/qcom/gcc-qcm2290.c         |  1 +
> >  drivers/clk/qcom/gcc-qcs404.c          |  1 +
> >  drivers/clk/qcom/gcc-qdu1000.c         |  1 +
> >  drivers/clk/qcom/gcc-sa8775p.c         |  1 +
> >  drivers/clk/qcom/gcc-sc7180.c          |  1 +
> >  drivers/clk/qcom/gcc-sc7280.c          |  1 +
> >  drivers/clk/qcom/gcc-sc8180x.c         |  1 +
> >  drivers/clk/qcom/gcc-sc8280xp.c        |  1 +
> >  drivers/clk/qcom/gcc-sdm660.c          |  1 +
> >  drivers/clk/qcom/gcc-sdm845.c          |  1 +
> >  drivers/clk/qcom/gcc-sdx55.c           |  1 +
> >  drivers/clk/qcom/gcc-sdx65.c           |  1 +
> >  drivers/clk/qcom/gcc-sm6115.c          |  1 +
> >  drivers/clk/qcom/gcc-sm6125.c          |  1 +
> >  drivers/clk/qcom/gcc-sm6350.c          |  1 +
> >  drivers/clk/qcom/gcc-sm6375.c          |  1 +
> >  drivers/clk/qcom/gcc-sm7150.c          |  1 +
> >  drivers/clk/qcom/gcc-sm8150.c          |  1 +
> >  drivers/clk/qcom/gcc-sm8250.c          |  1 +
> >  drivers/clk/qcom/gcc-sm8350.c          |  1 +
> >  drivers/clk/qcom/gcc-sm8450.c          |  1 +
> >  drivers/clk/qcom/gcc-sm8550.c          |  1 +
> >  drivers/clk/qcom/gdsc.c                | 26 ++++++++++++++++++++++++++
> >  drivers/clk/qcom/gdsc.h                |  6 ++++++
> >  drivers/clk/qcom/gpucc-msm8998.c       |  1 +
> >  drivers/clk/qcom/gpucc-sc7180.c        |  1 +
> >  drivers/clk/qcom/gpucc-sc7280.c        |  1 +
> >  drivers/clk/qcom/gpucc-sc8280xp.c      |  1 +
> >  drivers/clk/qcom/gpucc-sdm660.c        |  1 +
> >  drivers/clk/qcom/gpucc-sdm845.c        |  1 +
> >  drivers/clk/qcom/gpucc-sm6115.c        |  1 +
> >  drivers/clk/qcom/gpucc-sm6125.c        |  1 +
> >  drivers/clk/qcom/gpucc-sm6350.c        |  1 +
> >  drivers/clk/qcom/gpucc-sm6375.c        |  1 +
> >  drivers/clk/qcom/gpucc-sm8150.c        |  1 +
> >  drivers/clk/qcom/gpucc-sm8250.c        |  1 +
> >  drivers/clk/qcom/gpucc-sm8350.c        |  1 +
> >  drivers/clk/qcom/lcc-ipq806x.c         |  1 +
> >  drivers/clk/qcom/lpassaudiocc-sc7280.c |  1 +
> >  drivers/clk/qcom/lpasscc-sc7280.c      |  1 +
> >  drivers/clk/qcom/lpasscorecc-sc7180.c  |  2 ++
> >  drivers/clk/qcom/lpasscorecc-sc7280.c  |  2 ++
> >  drivers/clk/qcom/mmcc-apq8084.c        |  1 +
> >  drivers/clk/qcom/mmcc-msm8974.c        |  1 +
> >  drivers/clk/qcom/mmcc-msm8994.c        |  1 +
> >  drivers/clk/qcom/mmcc-msm8996.c        |  1 +
> >  drivers/clk/qcom/mmcc-msm8998.c        |  1 +
> >  drivers/clk/qcom/mmcc-sdm660.c         |  1 +
> >  drivers/clk/qcom/videocc-sc7180.c      |  1 +
> >  drivers/clk/qcom/videocc-sc7280.c      |  1 +
> >  drivers/clk/qcom/videocc-sdm845.c      |  1 +
> >  drivers/clk/qcom/videocc-sm8150.c      |  1 +
> >  drivers/clk/qcom/videocc-sm8250.c      |  1 +
> >  drivers/soc/qcom/rpmhpd.c              | 19 +++++++------------
> >  include/linux/pm_domain.h              |  4 ++++
> >  92 files changed, 161 insertions(+), 20 deletions(-)
> >
> > --
> > 2.34.1
> >

Saravana Kannan March 30, 2023, 7:50 p.m. UTC | #3

On Thu, Mar 30, 2023 at 4:27 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 23-03-27 17:17:28, Saravana Kannan wrote:
> > On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > There have been already a couple of tries to make the genpd "disable
> > > unused" late initcall skip the powering off of domains that might be
> > > needed until later on (i.e. until some consumer probes). The conclusion
> > > was that the provider could return -EBUSY from the power_off callback
> > > until the provider's sync state has been reached. This patch series tries
> > > to provide a proof-of-concept that is working on Qualcomm platforms.
> >
> > I'm giving my thoughts in the cover letter instead of spreading it
> > around all the patches so that there's context between the comments.
> >
> > 1) Why can't all the logic in this patch series be implemented at the
> > framework level? And then allow the drivers to opt into this behavior
> > by setting the sync_state() callback.
> >
> > That way, you can land it only for QC drivers by setting up
> > sync_state() callback only for QC drivers, but actually have the same
> > code function correctly for non-QC drivers too. And then once we have
> > this functionality working properly for QC drivers for one kernel
> > version (or two), we'll just have the framework set the device's
> > driver's sync_state() if it doesn't have one already.
>
> I think Ulf has already NACK'ed that approach here:
> [1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com/

I would have NACK'ed that too because that's an incomplete fix. As I
said further below, the fix needs to be at the aggregation level where
you aggregate all the current consumer requests. In there, you need to
add in the "state at boot" input that gets cleared out after a
sync_state() call is received for that power domain.

> And suggested this new approach that this patch series proposes.
> (Unless I missunderstood his point)
>
> >
> > 2) sync_state() is not just about power on/off. It's also about the
> > power domain level. Can you handle that too please?
>
> Well, this patchset only tries to delay the disabling of unused power
> domains until all consumers have had a chance to probe. So we use sync
> state only to queue up a power-off request to make sure those unused
> ones get disabled.

Sure, but the design is completely unusable for a more complete
sync_state() behavior. I'm okay if you want to improve the
sync_state() behavior in layers, but don't do it in a way where the
current design will definitely not work for what you want to add in
the future.

> >
> > 3) In your GDSC drivers, it's not clear to me if you are preventing
> > power off until sync_state() only for GDSCs that were already on at
> > boot. So if an off-at-boot GDSC gets turned on, and then you attempt
> > to turn it off before all its consumers have probed, it'll fail to
> > power it off even though that wasn't necessary?
>
> I think we can circumvent looking at a GDSC by knowing it there was ever
> a power on request since boot. I'll try to come up with something in the
> new version.

Please no. There's nothing wrong with reading the GDSC values. Please
read them and don't turn on GDSC's that weren't on at boot.

Otherwise you are making it a hassle for the case where there is a
consumer without a driver for a GDSC that was off at boot. You are now
forcing the use of timeouts or writing to state_synced file. Those
should be absolute last resorts, but you are making that a requirement
with your current implementation. If you implement it correctly by
reading the GDSC register, things will "just work". And it's not even
hard to do.

NACK'ed until this is handled correctly.

>
> >
> > 4) The returning -EBUSY when a power off is attempted seems to be
> > quite wasteful. The framework will go through the whole sequence of
> > trying to power down, send the notifications and then fail and then
> > send the undo notifications. Combined with point (2) I think this can
> > be handled better at the aggregation level in the framework to avoid
> > even going that far into the power off sequence.
>
> Again, have a look at [1] (above).

See my reply above. If you do it properly at the framework level, this
can be done in a clean way and will work for all power domains.

-Saravana

>
> Ulf, any thoughts on this 4th point?
>
> >
> > -Saravana
> >
> > >
> > > I've been doing extensive testing on SM8450, but I've also spinned this
> > > on my X13s (SC8280XP). Both patches that add the sync state callback to
> > > the SC8280XP and SM8450 are here to provide context. Once we agree on
> > > the form, I intend to add the sync state callback to all gdsc providers.
> > >
> > > Currently, some of the gdsc providers might not reach sync state due to
> > > list of consumers not probing yet (or at all). The sync state can be
> > > enforced by writing 1 to the state_synced sysfs attribute of the
> > > provider, thanks to Saravana's commit [1] which has been already merged.
> > >
> > > [1] https://lore.kernel.org/r/20230304005355.746421-3-saravanak@google.com
> > >
> > > V2 (RFC) of this patchset was here:
> > > https://lore.kernel.org/all/20230320134217.1685781-1-abel.vesa@linaro.org/
> > >
> > > Changes since v2:
> > >  * renamed genpd_queue_power_off_work to pm_genpd_queue_power_off and added
> > >    comment about its purpose w.r.t. it being exported.
> > >  * added the qcom_cc generic sync state callback to all providers that
> > >    register GDSCs, instead of SM8450 and SC8280XP
> > >
> > > Changes since v1:
> > >  * Added the qcom_cc sync state callback which calls in turn the gdsc one
> > >  * dropped extra semicolon from pm_domain.h
> > >
> > > Abel Vesa (4):
> > >   PM: domains: Allow power off queuing from providers
> > >   soc: qcom: rpmhpd: Do proper power off when state synced
> > >   clk: qcom: gdsc: Avoid actual power off until sync state
> > >   clk: qcom: Add sync state callback to all providers
> > >
> > >  drivers/base/power/domain.c            | 18 ++++++++++--------
> > >  drivers/clk/qcom/apss-ipq6018.c        |  1 +
> > >  drivers/clk/qcom/camcc-sc7180.c        |  1 +
> > >  drivers/clk/qcom/camcc-sc7280.c        |  1 +
> > >  drivers/clk/qcom/camcc-sdm845.c        |  1 +
> > >  drivers/clk/qcom/camcc-sm6350.c        |  1 +
> > >  drivers/clk/qcom/camcc-sm8250.c        |  1 +
> > >  drivers/clk/qcom/camcc-sm8450.c        |  1 +
> > >  drivers/clk/qcom/common.c              | 19 +++++++++++++++++++
> > >  drivers/clk/qcom/common.h              |  2 ++
> > >  drivers/clk/qcom/dispcc-qcm2290.c      |  1 +
> > >  drivers/clk/qcom/dispcc-sc7180.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sc7280.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sc8280xp.c     |  1 +
> > >  drivers/clk/qcom/dispcc-sdm845.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sm6115.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sm6125.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sm6350.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sm6375.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sm8250.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sm8450.c       |  1 +
> > >  drivers/clk/qcom/dispcc-sm8550.c       |  1 +
> > >  drivers/clk/qcom/gcc-apq8084.c         |  1 +
> > >  drivers/clk/qcom/gcc-ipq806x.c         |  1 +
> > >  drivers/clk/qcom/gcc-ipq8074.c         |  1 +
> > >  drivers/clk/qcom/gcc-mdm9615.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8660.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8909.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8916.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8939.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8953.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8960.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8974.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8976.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8994.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8996.c         |  1 +
> > >  drivers/clk/qcom/gcc-msm8998.c         |  1 +
> > >  drivers/clk/qcom/gcc-qcm2290.c         |  1 +
> > >  drivers/clk/qcom/gcc-qcs404.c          |  1 +
> > >  drivers/clk/qcom/gcc-qdu1000.c         |  1 +
> > >  drivers/clk/qcom/gcc-sa8775p.c         |  1 +
> > >  drivers/clk/qcom/gcc-sc7180.c          |  1 +
> > >  drivers/clk/qcom/gcc-sc7280.c          |  1 +
> > >  drivers/clk/qcom/gcc-sc8180x.c         |  1 +
> > >  drivers/clk/qcom/gcc-sc8280xp.c        |  1 +
> > >  drivers/clk/qcom/gcc-sdm660.c          |  1 +
> > >  drivers/clk/qcom/gcc-sdm845.c          |  1 +
> > >  drivers/clk/qcom/gcc-sdx55.c           |  1 +
> > >  drivers/clk/qcom/gcc-sdx65.c           |  1 +
> > >  drivers/clk/qcom/gcc-sm6115.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm6125.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm6350.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm6375.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm7150.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm8150.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm8250.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm8350.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm8450.c          |  1 +
> > >  drivers/clk/qcom/gcc-sm8550.c          |  1 +
> > >  drivers/clk/qcom/gdsc.c                | 26 ++++++++++++++++++++++++++
> > >  drivers/clk/qcom/gdsc.h                |  6 ++++++
> > >  drivers/clk/qcom/gpucc-msm8998.c       |  1 +
> > >  drivers/clk/qcom/gpucc-sc7180.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sc7280.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sc8280xp.c      |  1 +
> > >  drivers/clk/qcom/gpucc-sdm660.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sdm845.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sm6115.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sm6125.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sm6350.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sm6375.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sm8150.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sm8250.c        |  1 +
> > >  drivers/clk/qcom/gpucc-sm8350.c        |  1 +
> > >  drivers/clk/qcom/lcc-ipq806x.c         |  1 +
> > >  drivers/clk/qcom/lpassaudiocc-sc7280.c |  1 +
> > >  drivers/clk/qcom/lpasscc-sc7280.c      |  1 +
> > >  drivers/clk/qcom/lpasscorecc-sc7180.c  |  2 ++
> > >  drivers/clk/qcom/lpasscorecc-sc7280.c  |  2 ++
> > >  drivers/clk/qcom/mmcc-apq8084.c        |  1 +
> > >  drivers/clk/qcom/mmcc-msm8974.c        |  1 +
> > >  drivers/clk/qcom/mmcc-msm8994.c        |  1 +
> > >  drivers/clk/qcom/mmcc-msm8996.c        |  1 +
> > >  drivers/clk/qcom/mmcc-msm8998.c        |  1 +
> > >  drivers/clk/qcom/mmcc-sdm660.c         |  1 +
> > >  drivers/clk/qcom/videocc-sc7180.c      |  1 +
> > >  drivers/clk/qcom/videocc-sc7280.c      |  1 +
> > >  drivers/clk/qcom/videocc-sdm845.c      |  1 +
> > >  drivers/clk/qcom/videocc-sm8150.c      |  1 +
> > >  drivers/clk/qcom/videocc-sm8250.c      |  1 +
> > >  drivers/soc/qcom/rpmhpd.c              | 19 +++++++------------
> > >  include/linux/pm_domain.h              |  4 ++++
> > >  92 files changed, 161 insertions(+), 20 deletions(-)
> > >
> > > --
> > > 2.34.1
> > >

Abel Vesa March 31, 2023, 4:59 a.m. UTC | #4

On 23-03-30 12:50:44, Saravana Kannan wrote:
> On Thu, Mar 30, 2023 at 4:27 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > On 23-03-27 17:17:28, Saravana Kannan wrote:
> > > On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > >
> > > > There have been already a couple of tries to make the genpd "disable
> > > > unused" late initcall skip the powering off of domains that might be
> > > > needed until later on (i.e. until some consumer probes). The conclusion
> > > > was that the provider could return -EBUSY from the power_off callback
> > > > until the provider's sync state has been reached. This patch series tries
> > > > to provide a proof-of-concept that is working on Qualcomm platforms.
> > >
> > > I'm giving my thoughts in the cover letter instead of spreading it
> > > around all the patches so that there's context between the comments.
> > >
> > > 1) Why can't all the logic in this patch series be implemented at the
> > > framework level? And then allow the drivers to opt into this behavior
> > > by setting the sync_state() callback.
> > >
> > > That way, you can land it only for QC drivers by setting up
> > > sync_state() callback only for QC drivers, but actually have the same
> > > code function correctly for non-QC drivers too. And then once we have
> > > this functionality working properly for QC drivers for one kernel
> > > version (or two), we'll just have the framework set the device's
> > > driver's sync_state() if it doesn't have one already.
> >
> > I think Ulf has already NACK'ed that approach here:
> > [1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com/
> 
> I would have NACK'ed that too because that's an incomplete fix. As I
> said further below, the fix needs to be at the aggregation level where
> you aggregate all the current consumer requests. In there, you need to
> add in the "state at boot" input that gets cleared out after a
> sync_state() call is received for that power domain.
> 

So, just to make sure I understand your point. You would rather have the
genpd_power_off check if 'state at boot' is 'on' and return busy and
then clear then, via a generic genpd sync state you would mark 'state at
boot' as 'off' and queue up a power off request for each PD from there.
And as for 'state at boot' it would check the enable bit through
provider.

Am I right so far?

> > And suggested this new approach that this patch series proposes.
> > (Unless I missunderstood his point)
> >
> > >
> > > 2) sync_state() is not just about power on/off. It's also about the
> > > power domain level. Can you handle that too please?
> >
> > Well, this patchset only tries to delay the disabling of unused power
> > domains until all consumers have had a chance to probe. So we use sync
> > state only to queue up a power-off request to make sure those unused
> > ones get disabled.
> 
> Sure, but the design is completely unusable for a more complete
> sync_state() behavior. I'm okay if you want to improve the
> sync_state() behavior in layers, but don't do it in a way where the
> current design will definitely not work for what you want to add in
> the future.

But you would still be OK with the qcom_cc sync state wrapper, I guess,
right? Your concern is only about the sync state callback being not
genpd generic one, AFAIU.

> 
> > >
> > > 3) In your GDSC drivers, it's not clear to me if you are preventing
> > > power off until sync_state() only for GDSCs that were already on at
> > > boot. So if an off-at-boot GDSC gets turned on, and then you attempt
> > > to turn it off before all its consumers have probed, it'll fail to
> > > power it off even though that wasn't necessary?
> >
> > I think we can circumvent looking at a GDSC by knowing it there was ever
> > a power on request since boot. I'll try to come up with something in the
> > new version.
> 
> Please no. There's nothing wrong with reading the GDSC values. Please
> read them and don't turn on GDSC's that weren't on at boot.

Sorry for the typos above, I basically said that for this concern of
yours, we can add the 'state at boot' thing you mentioned above by
looking at the GDSC (as in reading reg).

> 
> Otherwise you are making it a hassle for the case where there is a
> consumer without a driver for a GDSC that was off at boot. You are now
> forcing the use of timeouts or writing to state_synced file. Those
> should be absolute last resorts, but you are making that a requirement
> with your current implementation. If you implement it correctly by
> reading the GDSC register, things will "just work". And it's not even
> hard to do.
> 
> NACK'ed until this is handled correctly.
> 
> >
> > >
> > > 4) The returning -EBUSY when a power off is attempted seems to be
> > > quite wasteful. The framework will go through the whole sequence of
> > > trying to power down, send the notifications and then fail and then
> > > send the undo notifications. Combined with point (2) I think this can
> > > be handled better at the aggregation level in the framework to avoid
> > > even going that far into the power off sequence.
> >
> > Again, have a look at [1] (above).
> 
> See my reply above. If you do it properly at the framework level, this
> can be done in a clean way and will work for all power domains.
> 
> -Saravana
> 
> >
> > Ulf, any thoughts on this 4th point?
> >
> > >
> > > -Saravana
> > >
> > > >
> > > > I've been doing extensive testing on SM8450, but I've also spinned this
> > > > on my X13s (SC8280XP). Both patches that add the sync state callback to
> > > > the SC8280XP and SM8450 are here to provide context. Once we agree on
> > > > the form, I intend to add the sync state callback to all gdsc providers.
> > > >
> > > > Currently, some of the gdsc providers might not reach sync state due to
> > > > list of consumers not probing yet (or at all). The sync state can be
> > > > enforced by writing 1 to the state_synced sysfs attribute of the
> > > > provider, thanks to Saravana's commit [1] which has been already merged.
> > > >
> > > > [1] https://lore.kernel.org/r/20230304005355.746421-3-saravanak@google.com
> > > >
> > > > V2 (RFC) of this patchset was here:
> > > > https://lore.kernel.org/all/20230320134217.1685781-1-abel.vesa@linaro.org/
> > > >
> > > > Changes since v2:
> > > >  * renamed genpd_queue_power_off_work to pm_genpd_queue_power_off and added
> > > >    comment about its purpose w.r.t. it being exported.
> > > >  * added the qcom_cc generic sync state callback to all providers that
> > > >    register GDSCs, instead of SM8450 and SC8280XP
> > > >
> > > > Changes since v1:
> > > >  * Added the qcom_cc sync state callback which calls in turn the gdsc one
> > > >  * dropped extra semicolon from pm_domain.h
> > > >
> > > > Abel Vesa (4):
> > > >   PM: domains: Allow power off queuing from providers
> > > >   soc: qcom: rpmhpd: Do proper power off when state synced
> > > >   clk: qcom: gdsc: Avoid actual power off until sync state
> > > >   clk: qcom: Add sync state callback to all providers
> > > >
> > > >  drivers/base/power/domain.c            | 18 ++++++++++--------
> > > >  drivers/clk/qcom/apss-ipq6018.c        |  1 +
> > > >  drivers/clk/qcom/camcc-sc7180.c        |  1 +
> > > >  drivers/clk/qcom/camcc-sc7280.c        |  1 +
> > > >  drivers/clk/qcom/camcc-sdm845.c        |  1 +
> > > >  drivers/clk/qcom/camcc-sm6350.c        |  1 +
> > > >  drivers/clk/qcom/camcc-sm8250.c        |  1 +
> > > >  drivers/clk/qcom/camcc-sm8450.c        |  1 +
> > > >  drivers/clk/qcom/common.c              | 19 +++++++++++++++++++
> > > >  drivers/clk/qcom/common.h              |  2 ++
> > > >  drivers/clk/qcom/dispcc-qcm2290.c      |  1 +
> > > >  drivers/clk/qcom/dispcc-sc7180.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sc7280.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sc8280xp.c     |  1 +
> > > >  drivers/clk/qcom/dispcc-sdm845.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sm6115.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sm6125.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sm6350.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sm6375.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sm8250.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sm8450.c       |  1 +
> > > >  drivers/clk/qcom/dispcc-sm8550.c       |  1 +
> > > >  drivers/clk/qcom/gcc-apq8084.c         |  1 +
> > > >  drivers/clk/qcom/gcc-ipq806x.c         |  1 +
> > > >  drivers/clk/qcom/gcc-ipq8074.c         |  1 +
> > > >  drivers/clk/qcom/gcc-mdm9615.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8660.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8909.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8916.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8939.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8953.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8960.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8974.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8976.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8994.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8996.c         |  1 +
> > > >  drivers/clk/qcom/gcc-msm8998.c         |  1 +
> > > >  drivers/clk/qcom/gcc-qcm2290.c         |  1 +
> > > >  drivers/clk/qcom/gcc-qcs404.c          |  1 +
> > > >  drivers/clk/qcom/gcc-qdu1000.c         |  1 +
> > > >  drivers/clk/qcom/gcc-sa8775p.c         |  1 +
> > > >  drivers/clk/qcom/gcc-sc7180.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sc7280.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sc8180x.c         |  1 +
> > > >  drivers/clk/qcom/gcc-sc8280xp.c        |  1 +
> > > >  drivers/clk/qcom/gcc-sdm660.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sdm845.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sdx55.c           |  1 +
> > > >  drivers/clk/qcom/gcc-sdx65.c           |  1 +
> > > >  drivers/clk/qcom/gcc-sm6115.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm6125.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm6350.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm6375.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm7150.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm8150.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm8250.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm8350.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm8450.c          |  1 +
> > > >  drivers/clk/qcom/gcc-sm8550.c          |  1 +
> > > >  drivers/clk/qcom/gdsc.c                | 26 ++++++++++++++++++++++++++
> > > >  drivers/clk/qcom/gdsc.h                |  6 ++++++
> > > >  drivers/clk/qcom/gpucc-msm8998.c       |  1 +
> > > >  drivers/clk/qcom/gpucc-sc7180.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sc7280.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sc8280xp.c      |  1 +
> > > >  drivers/clk/qcom/gpucc-sdm660.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sdm845.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sm6115.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sm6125.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sm6350.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sm6375.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sm8150.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sm8250.c        |  1 +
> > > >  drivers/clk/qcom/gpucc-sm8350.c        |  1 +
> > > >  drivers/clk/qcom/lcc-ipq806x.c         |  1 +
> > > >  drivers/clk/qcom/lpassaudiocc-sc7280.c |  1 +
> > > >  drivers/clk/qcom/lpasscc-sc7280.c      |  1 +
> > > >  drivers/clk/qcom/lpasscorecc-sc7180.c  |  2 ++
> > > >  drivers/clk/qcom/lpasscorecc-sc7280.c  |  2 ++
> > > >  drivers/clk/qcom/mmcc-apq8084.c        |  1 +
> > > >  drivers/clk/qcom/mmcc-msm8974.c        |  1 +
> > > >  drivers/clk/qcom/mmcc-msm8994.c        |  1 +
> > > >  drivers/clk/qcom/mmcc-msm8996.c        |  1 +
> > > >  drivers/clk/qcom/mmcc-msm8998.c        |  1 +
> > > >  drivers/clk/qcom/mmcc-sdm660.c         |  1 +
> > > >  drivers/clk/qcom/videocc-sc7180.c      |  1 +
> > > >  drivers/clk/qcom/videocc-sc7280.c      |  1 +
> > > >  drivers/clk/qcom/videocc-sdm845.c      |  1 +
> > > >  drivers/clk/qcom/videocc-sm8150.c      |  1 +
> > > >  drivers/clk/qcom/videocc-sm8250.c      |  1 +
> > > >  drivers/soc/qcom/rpmhpd.c              | 19 +++++++------------
> > > >  include/linux/pm_domain.h              |  4 ++++
> > > >  92 files changed, 161 insertions(+), 20 deletions(-)
> > > >
> > > > --
> > > > 2.34.1
> > > >

Ulf Hansson April 5, 2023, 2:11 p.m. UTC | #5

Abel, Saravana,

On Fri, 31 Mar 2023 at 06:59, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 23-03-30 12:50:44, Saravana Kannan wrote:
> > On Thu, Mar 30, 2023 at 4:27 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > On 23-03-27 17:17:28, Saravana Kannan wrote:
> > > > On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > >
> > > > > There have been already a couple of tries to make the genpd "disable
> > > > > unused" late initcall skip the powering off of domains that might be
> > > > > needed until later on (i.e. until some consumer probes). The conclusion
> > > > > was that the provider could return -EBUSY from the power_off callback
> > > > > until the provider's sync state has been reached. This patch series tries
> > > > > to provide a proof-of-concept that is working on Qualcomm platforms.
> > > >
> > > > I'm giving my thoughts in the cover letter instead of spreading it
> > > > around all the patches so that there's context between the comments.
> > > >
> > > > 1) Why can't all the logic in this patch series be implemented at the
> > > > framework level? And then allow the drivers to opt into this behavior
> > > > by setting the sync_state() callback.
> > > >
> > > > That way, you can land it only for QC drivers by setting up
> > > > sync_state() callback only for QC drivers, but actually have the same
> > > > code function correctly for non-QC drivers too. And then once we have
> > > > this functionality working properly for QC drivers for one kernel
> > > > version (or two), we'll just have the framework set the device's
> > > > driver's sync_state() if it doesn't have one already.
> > >
> > > I think Ulf has already NACK'ed that approach here:
> > > [1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com/
> >
> > I would have NACK'ed that too because that's an incomplete fix. As I
> > said further below, the fix needs to be at the aggregation level where
> > you aggregate all the current consumer requests. In there, you need to
> > add in the "state at boot" input that gets cleared out after a
> > sync_state() call is received for that power domain.
> >
>
> So, just to make sure I understand your point. You would rather have the
> genpd_power_off check if 'state at boot' is 'on' and return busy and
> then clear then, via a generic genpd sync state you would mark 'state at
> boot' as 'off' and queue up a power off request for each PD from there.
> And as for 'state at boot' it would check the enable bit through
> provider.
>
> Am I right so far?

I am not sure I completely follow what you are suggesting here.

Although, let me point out that there is no requirement from the genpd
API point of view, that the provider needs to be a driver. This means
that the sync_state callback may not even be applicable for all genpd
providers.

In other words, it looks to me that we may need some new genpd helper
functions, no matter what. More importantly, it looks like we need an
opt-in behaviour, unless we can figure out a common way for genpd to
understand whether the sync_state thing is going to be applicable or
not. Maybe Saravana has some ideas around this?

Note that, I don't object to extending genpd to be more clever and to
share common code, of course. We could, for example, make
genpd_power_off() to bail out earlier, rather than calling the
->power_off() callback and waiting for it to return -EBUSY. Both of
you have pointed this out to me, in some of the earlier
replies/discussions too.

>
> > > And suggested this new approach that this patch series proposes.
> > > (Unless I missunderstood his point)
> > >
> > > >
> > > > 2) sync_state() is not just about power on/off. It's also about the
> > > > power domain level. Can you handle that too please?
> > >
> > > Well, this patchset only tries to delay the disabling of unused power
> > > domains until all consumers have had a chance to probe. So we use sync
> > > state only to queue up a power-off request to make sure those unused
> > > ones get disabled.
> >
> > Sure, but the design is completely unusable for a more complete
> > sync_state() behavior. I'm okay if you want to improve the
> > sync_state() behavior in layers, but don't do it in a way where the
> > current design will definitely not work for what you want to add in
> > the future.
>
> But you would still be OK with the qcom_cc sync state wrapper, I guess,
> right? Your concern is only about the sync state callback being not
> genpd generic one, AFAIU.
>
> >
> > > >
> > > > 3) In your GDSC drivers, it's not clear to me if you are preventing
> > > > power off until sync_state() only for GDSCs that were already on at
> > > > boot. So if an off-at-boot GDSC gets turned on, and then you attempt
> > > > to turn it off before all its consumers have probed, it'll fail to
> > > > power it off even though that wasn't necessary?
> > >
> > > I think we can circumvent looking at a GDSC by knowing it there was ever
> > > a power on request since boot. I'll try to come up with something in the
> > > new version.
> >
> > Please no. There's nothing wrong with reading the GDSC values. Please
> > read them and don't turn on GDSC's that weren't on at boot.
>
> Sorry for the typos above, I basically said that for this concern of
> yours, we can add the 'state at boot' thing you mentioned above by
> looking at the GDSC (as in reading reg).
>
> >
> > Otherwise you are making it a hassle for the case where there is a
> > consumer without a driver for a GDSC that was off at boot. You are now
> > forcing the use of timeouts or writing to state_synced file. Those
> > should be absolute last resorts, but you are making that a requirement
> > with your current implementation. If you implement it correctly by
> > reading the GDSC register, things will "just work". And it's not even
> > hard to do.
> >
> > NACK'ed until this is handled correctly.
> >
> > >
> > > >
> > > > 4) The returning -EBUSY when a power off is attempted seems to be
> > > > quite wasteful. The framework will go through the whole sequence of
> > > > trying to power down, send the notifications and then fail and then
> > > > send the undo notifications. Combined with point (2) I think this can
> > > > be handled better at the aggregation level in the framework to avoid
> > > > even going that far into the power off sequence.
> > >
> > > Again, have a look at [1] (above).
> >
> > See my reply above. If you do it properly at the framework level, this
> > can be done in a clean way and will work for all power domains.
> >
> > -Saravana
> >
> > >
> > > Ulf, any thoughts on this 4th point?

Please, see my reply above.

[...]

Kind regards
Uffe

Abel Vesa April 6, 2023, 9:26 a.m. UTC | #6

On 23-04-05 16:11:18, Ulf Hansson wrote:
> Abel, Saravana,
> 
> On Fri, 31 Mar 2023 at 06:59, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > On 23-03-30 12:50:44, Saravana Kannan wrote:
> > > On Thu, Mar 30, 2023 at 4:27 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > >
> > > > On 23-03-27 17:17:28, Saravana Kannan wrote:
> > > > > On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > > >
> > > > > > There have been already a couple of tries to make the genpd "disable
> > > > > > unused" late initcall skip the powering off of domains that might be
> > > > > > needed until later on (i.e. until some consumer probes). The conclusion
> > > > > > was that the provider could return -EBUSY from the power_off callback
> > > > > > until the provider's sync state has been reached. This patch series tries
> > > > > > to provide a proof-of-concept that is working on Qualcomm platforms.
> > > > >
> > > > > I'm giving my thoughts in the cover letter instead of spreading it
> > > > > around all the patches so that there's context between the comments.
> > > > >
> > > > > 1) Why can't all the logic in this patch series be implemented at the
> > > > > framework level? And then allow the drivers to opt into this behavior
> > > > > by setting the sync_state() callback.
> > > > >
> > > > > That way, you can land it only for QC drivers by setting up
> > > > > sync_state() callback only for QC drivers, but actually have the same
> > > > > code function correctly for non-QC drivers too. And then once we have
> > > > > this functionality working properly for QC drivers for one kernel
> > > > > version (or two), we'll just have the framework set the device's
> > > > > driver's sync_state() if it doesn't have one already.
> > > >
> > > > I think Ulf has already NACK'ed that approach here:
> > > > [1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com/
> > >
> > > I would have NACK'ed that too because that's an incomplete fix. As I
> > > said further below, the fix needs to be at the aggregation level where
> > > you aggregate all the current consumer requests. In there, you need to
> > > add in the "state at boot" input that gets cleared out after a
> > > sync_state() call is received for that power domain.
> > >
> >
> > So, just to make sure I understand your point. You would rather have the
> > genpd_power_off check if 'state at boot' is 'on' and return busy and
> > then clear then, via a generic genpd sync state you would mark 'state at
> > boot' as 'off' and queue up a power off request for each PD from there.
> > And as for 'state at boot' it would check the enable bit through
> > provider.
> >
> > Am I right so far?
> 
> I am not sure I completely follow what you are suggesting here.

Please have a look at this:
https://git.kernel.org/pub/scm/linux/kernel/git/abelvesa/linux.git/commit/?h=qcom/genpd/ignore_unused_until_sync_state&id=4f9e6140dfe77884012383f8ba2140cadb62ca4a

Keep in mind that is WIP for now. Once I have something, I'll post it on
mailing list. Right now, there is a missing piece mentioned in that
commit message.

> 
> Although, let me point out that there is no requirement from the genpd
> API point of view, that the provider needs to be a driver. This means
> that the sync_state callback may not even be applicable for all genpd
> providers.

Yes, I'm considering that case too.

> 
> In other words, it looks to me that we may need some new genpd helper
> functions, no matter what. More importantly, it looks like we need an
> opt-in behaviour, unless we can figure out a common way for genpd to
> understand whether the sync_state thing is going to be applicable or
> not. Maybe Saravana has some ideas around this?
> 
> Note that, I don't object to extending genpd to be more clever and to
> share common code, of course. We could, for example, make
> genpd_power_off() to bail out earlier, rather than calling the
> ->power_off() callback and waiting for it to return -EBUSY. Both of
> you have pointed this out to me, in some of the earlier
> replies/discussions too.

The above link basically does this. I hope this is what Saravana has in
mind as well.

> 
> >
> > > > And suggested this new approach that this patch series proposes.
> > > > (Unless I missunderstood his point)
> > > >
> > > > >
> > > > > 2) sync_state() is not just about power on/off. It's also about the
> > > > > power domain level. Can you handle that too please?
> > > >
> > > > Well, this patchset only tries to delay the disabling of unused power
> > > > domains until all consumers have had a chance to probe. So we use sync
> > > > state only to queue up a power-off request to make sure those unused
> > > > ones get disabled.
> > >
> > > Sure, but the design is completely unusable for a more complete
> > > sync_state() behavior. I'm okay if you want to improve the
> > > sync_state() behavior in layers, but don't do it in a way where the
> > > current design will definitely not work for what you want to add in
> > > the future.
> >
> > But you would still be OK with the qcom_cc sync state wrapper, I guess,
> > right? Your concern is only about the sync state callback being not
> > genpd generic one, AFAIU.
> >
> > >
> > > > >
> > > > > 3) In your GDSC drivers, it's not clear to me if you are preventing
> > > > > power off until sync_state() only for GDSCs that were already on at
> > > > > boot. So if an off-at-boot GDSC gets turned on, and then you attempt
> > > > > to turn it off before all its consumers have probed, it'll fail to
> > > > > power it off even though that wasn't necessary?
> > > >
> > > > I think we can circumvent looking at a GDSC by knowing it there was ever
> > > > a power on request since boot. I'll try to come up with something in the
> > > > new version.
> > >
> > > Please no. There's nothing wrong with reading the GDSC values. Please
> > > read them and don't turn on GDSC's that weren't on at boot.
> >
> > Sorry for the typos above, I basically said that for this concern of
> > yours, we can add the 'state at boot' thing you mentioned above by
> > looking at the GDSC (as in reading reg).
> >
> > >
> > > Otherwise you are making it a hassle for the case where there is a
> > > consumer without a driver for a GDSC that was off at boot. You are now
> > > forcing the use of timeouts or writing to state_synced file. Those
> > > should be absolute last resorts, but you are making that a requirement
> > > with your current implementation. If you implement it correctly by
> > > reading the GDSC register, things will "just work". And it's not even
> > > hard to do.
> > >
> > > NACK'ed until this is handled correctly.
> > >
> > > >
> > > > >
> > > > > 4) The returning -EBUSY when a power off is attempted seems to be
> > > > > quite wasteful. The framework will go through the whole sequence of
> > > > > trying to power down, send the notifications and then fail and then
> > > > > send the undo notifications. Combined with point (2) I think this can
> > > > > be handled better at the aggregation level in the framework to avoid
> > > > > even going that far into the power off sequence.
> > > >
> > > > Again, have a look at [1] (above).
> > >
> > > See my reply above. If you do it properly at the framework level, this
> > > can be done in a clean way and will work for all power domains.
> > >
> > > -Saravana
> > >
> > > >
> > > > Ulf, any thoughts on this 4th point?
> 
> Please, see my reply above.
> 
> [...]
> 
> Kind regards
> Uffe

Ulf Hansson April 6, 2023, 10:59 a.m. UTC | #7

On Thu, 6 Apr 2023 at 11:26, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 23-04-05 16:11:18, Ulf Hansson wrote:
> > Abel, Saravana,
> >
> > On Fri, 31 Mar 2023 at 06:59, Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > On 23-03-30 12:50:44, Saravana Kannan wrote:
> > > > On Thu, Mar 30, 2023 at 4:27 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > >
> > > > > On 23-03-27 17:17:28, Saravana Kannan wrote:
> > > > > > On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > > > >
> > > > > > > There have been already a couple of tries to make the genpd "disable
> > > > > > > unused" late initcall skip the powering off of domains that might be
> > > > > > > needed until later on (i.e. until some consumer probes). The conclusion
> > > > > > > was that the provider could return -EBUSY from the power_off callback
> > > > > > > until the provider's sync state has been reached. This patch series tries
> > > > > > > to provide a proof-of-concept that is working on Qualcomm platforms.
> > > > > >
> > > > > > I'm giving my thoughts in the cover letter instead of spreading it
> > > > > > around all the patches so that there's context between the comments.
> > > > > >
> > > > > > 1) Why can't all the logic in this patch series be implemented at the
> > > > > > framework level? And then allow the drivers to opt into this behavior
> > > > > > by setting the sync_state() callback.
> > > > > >
> > > > > > That way, you can land it only for QC drivers by setting up
> > > > > > sync_state() callback only for QC drivers, but actually have the same
> > > > > > code function correctly for non-QC drivers too. And then once we have
> > > > > > this functionality working properly for QC drivers for one kernel
> > > > > > version (or two), we'll just have the framework set the device's
> > > > > > driver's sync_state() if it doesn't have one already.
> > > > >
> > > > > I think Ulf has already NACK'ed that approach here:
> > > > > [1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com/
> > > >
> > > > I would have NACK'ed that too because that's an incomplete fix. As I
> > > > said further below, the fix needs to be at the aggregation level where
> > > > you aggregate all the current consumer requests. In there, you need to
> > > > add in the "state at boot" input that gets cleared out after a
> > > > sync_state() call is received for that power domain.
> > > >
> > >
> > > So, just to make sure I understand your point. You would rather have the
> > > genpd_power_off check if 'state at boot' is 'on' and return busy and
> > > then clear then, via a generic genpd sync state you would mark 'state at
> > > boot' as 'off' and queue up a power off request for each PD from there.
> > > And as for 'state at boot' it would check the enable bit through
> > > provider.
> > >
> > > Am I right so far?
> >
> > I am not sure I completely follow what you are suggesting here.
>
> Please have a look at this:
> https://git.kernel.org/pub/scm/linux/kernel/git/abelvesa/linux.git/commit/?h=qcom/genpd/ignore_unused_until_sync_state&id=4f9e6140dfe77884012383f8ba2140cadb62ca4a
>
> Keep in mind that is WIP for now. Once I have something, I'll post it on
> mailing list. Right now, there is a missing piece mentioned in that
> commit message.

I had a quick look and it seems rather promising, but I will have a
closer look when you post it.

>
> >
> > Although, let me point out that there is no requirement from the genpd
> > API point of view, that the provider needs to be a driver. This means
> > that the sync_state callback may not even be applicable for all genpd
> > providers.
>
> Yes, I'm considering that case too.

Good.

>
> >
> > In other words, it looks to me that we may need some new genpd helper
> > functions, no matter what. More importantly, it looks like we need an
> > opt-in behaviour, unless we can figure out a common way for genpd to
> > understand whether the sync_state thing is going to be applicable or
> > not. Maybe Saravana has some ideas around this?
> >
> > Note that, I don't object to extending genpd to be more clever and to
> > share common code, of course. We could, for example, make
> > genpd_power_off() to bail out earlier, rather than calling the
> > ->power_off() callback and waiting for it to return -EBUSY. Both of
> > you have pointed this out to me, in some of the earlier
> > replies/discussions too.
>
> The above link basically does this. I hope this is what Saravana has in
> mind as well.

Okay, let's see what Saravana thinks.

Maybe it's easier to post an RFC, based upon the above and continue
the discussion around that?

Kind regards
Uffe

[v3,0/4] Allow genpd providers to power off domains on sync state

Message

Comments