diff mbox series

[PATCH/RFC] clk: qcom: rpmh: Block system suspend if XO is enabled

Message ID 20220628201340.3981860-1-swboyd@chromium.org (mailing list archive)
State New
Headers show
Series [PATCH/RFC] clk: qcom: rpmh: Block system suspend if XO is enabled | expand

Commit Message

Stephen Boyd June 28, 2022, 8:13 p.m. UTC
Tracking down what RPMh resource is blocking XO shutdown in suspend is
hard. Largely because we need external debug tools to dump the RPMh
internal state to figure out what resource is enabled. Instead of doing
that, let's just block system wide suspend in the kernel if the RPMh XO
resource is enabled by something in the kernel. This will help us narrow
down XO shutdown failures to the XO clk, and not something else like an
interconnect or regulator RPMh resource.

I'm sending this as an RFC because it breaks suspend for me on Trogdor
boards. I found out that the XO resource is always enabled on these
devices because the audio driver leaves an audio clk always on. This
means that the XO resource must not be used to determine if XO shutdown
is achievable, or we're leaving power savings on the table.

Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Mike Tipton <quic_mdtipton@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Please don't apply. It will break suspend on Trogdor boards.

 drivers/clk/qcom/clk-rpmh.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56

Comments

Bjorn Andersson July 1, 2022, 4 a.m. UTC | #1
On Tue 28 Jun 15:13 CDT 2022, Stephen Boyd wrote:

> Tracking down what RPMh resource is blocking XO shutdown in suspend is
> hard. Largely because we need external debug tools to dump the RPMh
> internal state to figure out what resource is enabled. Instead of doing
> that, let's just block system wide suspend in the kernel if the RPMh XO
> resource is enabled by something in the kernel. This will help us narrow
> down XO shutdown failures to the XO clk, and not something else like an
> interconnect or regulator RPMh resource.
> 
> I'm sending this as an RFC because it breaks suspend for me on Trogdor
> boards. I found out that the XO resource is always enabled on these
> devices because the audio driver leaves an audio clk always on. This
> means that the XO resource must not be used to determine if XO shutdown
> is achievable, or we're leaving power savings on the table.
> 
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
> 
> Please don't apply. It will break suspend on Trogdor boards.
> 

This seems like a useful debug feature for people outside of Qualcomm,
so I assume you're saying that we shouldn't merge it until someone has
fixed the audio driver? Or did you post it just as a debug tool?

Regards,
Bjorn

>  drivers/clk/qcom/clk-rpmh.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index aed907982344..ba0e0e4b9cf2 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -70,6 +70,14 @@ struct clk_rpmh_desc {
>  
>  static DEFINE_MUTEX(rpmh_clk_lock);
>  
> +/* XO shutdown will fail if XO is enabled across suspend */
> +static int clk_rpmh_suspend(struct device *dev)
> +{
> +	struct clk_rpmh *xo = dev_get_drvdata(dev);
> +
> +	return xo && xo->state ? -EBUSY : 0;
> +}
> +
>  #define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
>  			  _res_en_offset, _res_on, _div)		\
>  	static struct clk_rpmh _platform##_##_name_active;		\
> @@ -690,6 +698,10 @@ static int clk_rpmh_probe(struct platform_device *pdev)
>  			dev_err(&pdev->dev, "failed to register %s\n", name);
>  			return ret;
>  		}
> +
> +		/* Stash CXO clk for XO shutdown tracking */
> +		if (i == RPMH_CXO_CLK)
> +			platform_set_drvdata(pdev, rpmh_clk);
>  	}
>  
>  	/* typecast to silence compiler warning */
> @@ -722,9 +734,12 @@ static const struct of_device_id clk_rpmh_match_table[] = {
>  };
>  MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);
>  
> +static SIMPLE_DEV_PM_OPS(clk_rpmh_pm_ops, clk_rpmh_suspend, NULL);
> +
>  static struct platform_driver clk_rpmh_driver = {
>  	.probe		= clk_rpmh_probe,
>  	.driver		= {
> +		.pm	= pm_ptr(&clk_rpmh_pm_ops),
>  		.name	= "clk-rpmh",
>  		.of_match_table = clk_rpmh_match_table,
>  	},
> 
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> -- 
> https://chromeos.dev
>
Stephen Boyd July 7, 2022, 12:02 a.m. UTC | #2
Quoting Bjorn Andersson (2022-06-30 21:00:11)
> On Tue 28 Jun 15:13 CDT 2022, Stephen Boyd wrote:
>
> > Tracking down what RPMh resource is blocking XO shutdown in suspend is
> > hard. Largely because we need external debug tools to dump the RPMh
> > internal state to figure out what resource is enabled. Instead of doing
> > that, let's just block system wide suspend in the kernel if the RPMh XO
> > resource is enabled by something in the kernel. This will help us narrow
> > down XO shutdown failures to the XO clk, and not something else like an
> > interconnect or regulator RPMh resource.
> >
> > I'm sending this as an RFC because it breaks suspend for me on Trogdor
> > boards. I found out that the XO resource is always enabled on these
> > devices because the audio driver leaves an audio clk always on. This
> > means that the XO resource must not be used to determine if XO shutdown
> > is achievable, or we're leaving power savings on the table.
> >
> > Cc: Taniya Das <quic_tdas@quicinc.com>
> > Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> > ---
> >
> > Please don't apply. It will break suspend on Trogdor boards.
> >
>
> This seems like a useful debug feature for people outside of Qualcomm,
> so I assume you're saying that we shouldn't merge it until someone has
> fixed the audio driver? Or did you post it just as a debug tool?
>

I mainly posted it to get a response from Qualcomm on what's going on. I
haven't tried to fix the audio driver so far, but I can certainly look
into it. The audio clk driver keeping XO on doesn't seem to matter for
power because we're hitting the deepest of sleeps on Trogdor boards.

I also realized that due to clk adoption logic we can't be certain that
this driver's suspend function will be called after other clk providers
that consume XO run their suspend functions. We can probably assume clk
consumers that aren't providers will probe after clk-rpmh, so this check
is generally safe because we don't have clk providers disabling clks
during their suspend functions. It's just not totally safe.

Maybe this is a good reason to add some sort of suspend op to clk_ops
and then call that during system wide suspend.
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
index aed907982344..ba0e0e4b9cf2 100644
--- a/drivers/clk/qcom/clk-rpmh.c
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -70,6 +70,14 @@  struct clk_rpmh_desc {
 
 static DEFINE_MUTEX(rpmh_clk_lock);
 
+/* XO shutdown will fail if XO is enabled across suspend */
+static int clk_rpmh_suspend(struct device *dev)
+{
+	struct clk_rpmh *xo = dev_get_drvdata(dev);
+
+	return xo && xo->state ? -EBUSY : 0;
+}
+
 #define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
 			  _res_en_offset, _res_on, _div)		\
 	static struct clk_rpmh _platform##_##_name_active;		\
@@ -690,6 +698,10 @@  static int clk_rpmh_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "failed to register %s\n", name);
 			return ret;
 		}
+
+		/* Stash CXO clk for XO shutdown tracking */
+		if (i == RPMH_CXO_CLK)
+			platform_set_drvdata(pdev, rpmh_clk);
 	}
 
 	/* typecast to silence compiler warning */
@@ -722,9 +734,12 @@  static const struct of_device_id clk_rpmh_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);
 
+static SIMPLE_DEV_PM_OPS(clk_rpmh_pm_ops, clk_rpmh_suspend, NULL);
+
 static struct platform_driver clk_rpmh_driver = {
 	.probe		= clk_rpmh_probe,
 	.driver		= {
+		.pm	= pm_ptr(&clk_rpmh_pm_ops),
 		.name	= "clk-rpmh",
 		.of_match_table = clk_rpmh_match_table,
 	},