diff mbox series

[v4,3/3] clk: qcom: lpasscc-sc7180: Re-configure the PLL in case lost

Message ID 20201014171259.v4.3.Id0cc5d859e2422082a29a7909658932c857f5a81@changeid (mailing list archive)
State Rejected, archived
Headers show
Series [v4,1/3] clk: qcom: lpasscc-sc7810: Use devm in probe | expand

Commit Message

Doug Anderson Oct. 15, 2020, 12:13 a.m. UTC 
From: Taniya Das <tdas@codeaurora.org>

In the case where the PLL configuration is lost, then the pm runtime
resume will reconfigure before usage.

Fixes: edab812d802d ("clk: qcom: lpass: Add support for LPASS clock controller for SC7180")
Signed-off-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v4:
- Put regmap names in "static const char *" globals.

Changes in v3:
- Now based on a series which disentangles the two clock devices.
- Use dev_get_regmap().
- Better comment about reading PLL_L_VAL.

Changes in v2:
- Don't needlessly have a 2nd copy of dev_pm_ops and jam it in.
- Check the return value of pm_clk_resume()
- l_val should be unsigned int.

 drivers/clk/qcom/lpasscorecc-sc7180.c | 28 ++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

Comments

Stephen Boyd Oct. 16, 2020, 3:16 a.m. UTC | #1 
Quoting Douglas Anderson (2020-10-14 17:13:29)
> From: Taniya Das <tdas@codeaurora.org>
> 
> In the case where the PLL configuration is lost, then the pm runtime
> resume will reconfigure before usage.

Taniya, this commit needs a lot more describing than one sentence. I see
that the PLL's L value is reset at boot, but only once. That seems to be
because the bootloader I have doesn't set bit 11 for the RETAIN_FF bit
on the lpass_core_hm_gdsc. Once the gdsc is turned off the first time,
the PLL settings are lost and the L val is reset to 0. That makes sense
because RETAIN_FF isn't set. This also means the other register writes
during probe are lost during the first suspend of the lpass core clk
controller. Then when the GDSC is turned on the next time for this clk
controller  being runtime resumed we will set the retain bit and then
configure the PLL again. BTW, I see that runtime PM is called for this
clk controller for all the clk operations. Maybe there should be some
auto suspend timeout so that we're not toggling the gdsc constantly?

I hacked up the GDSC code to set the bit at gdsc registration time and
it seems to fix the problem I'm seeing (i.e. that the PLL is stuck,
which should also be in the commit text here). When I try to set the bit
in the bootloader though my kernel won't boot. I guess something is
hanging the system if I enable the retain bit in the GDSC?

> 
> Fixes: edab812d802d ("clk: qcom: lpass: Add support for LPASS clock controller for SC7180")
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Stephen Boyd Oct. 17, 2020, 2:01 a.m. UTC | #2 
Quoting Stephen Boyd (2020-10-15 20:16:27)
> Quoting Douglas Anderson (2020-10-14 17:13:29)
> > From: Taniya Das <tdas@codeaurora.org>
> > 
> > In the case where the PLL configuration is lost, then the pm runtime
> > resume will reconfigure before usage.
> 
> Taniya, this commit needs a lot more describing than one sentence. I see
> that the PLL's L value is reset at boot, but only once. That seems to be
> because the bootloader I have doesn't set bit 11 for the RETAIN_FF bit
> on the lpass_core_hm_gdsc. Once the gdsc is turned off the first time,
> the PLL settings are lost and the L val is reset to 0. That makes sense
> because RETAIN_FF isn't set. This also means the other register writes
> during probe are lost during the first suspend of the lpass core clk
> controller. Then when the GDSC is turned on the next time for this clk
> controller  being runtime resumed we will set the retain bit and then
> configure the PLL again. BTW, I see that runtime PM is called for this
> clk controller for all the clk operations. Maybe there should be some
> auto suspend timeout so that we're not toggling the gdsc constantly?
> 
> I hacked up the GDSC code to set the bit at gdsc registration time and
> it seems to fix the problem I'm seeing (i.e. that the PLL is stuck,
> which should also be in the commit text here). When I try to set the bit
> in the bootloader though my kernel won't boot. I guess something is
> hanging the system if I enable the retain bit in the GDSC?
> 

After hacking on this for some time it looks like we can apply this
patch instead and things are good. The first two patches in this series
look mostly good to me minus some nitpicks so please resend.

---8<---
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 99834564bcc2..508c2901abfa 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -343,6 +343,14 @@ static int gdsc_init(struct gdsc *sc)
 	if ((sc->flags & VOTABLE) && on)
 		gdsc_enable(&sc->pd);
 
+	/*
+	 * Make sure the retain bit is set if the GDSC is already on, otherwise
+	 * we end up turning off the GDSC and destroying all the register
+	 * contents that we thought we were saving.
+	 */
+	if ((sc->flags & RETAIN_FF_ENABLE) && on)
+		gdsc_retain_ff_on(sc);
+
 	/* If ALWAYS_ON GDSCs are not ON, turn them ON */
 	if (sc->flags & ALWAYS_ON) {
 		if (!on)
Doug Anderson Oct. 17, 2020, 3:17 a.m. UTC | #3 
Hi,

On Fri, Oct 16, 2020 at 7:01 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Stephen Boyd (2020-10-15 20:16:27)
> > Quoting Douglas Anderson (2020-10-14 17:13:29)
> > > From: Taniya Das <tdas@codeaurora.org>
> > >
> > > In the case where the PLL configuration is lost, then the pm runtime
> > > resume will reconfigure before usage.
> >
> > Taniya, this commit needs a lot more describing than one sentence. I see
> > that the PLL's L value is reset at boot, but only once. That seems to be
> > because the bootloader I have doesn't set bit 11 for the RETAIN_FF bit
> > on the lpass_core_hm_gdsc. Once the gdsc is turned off the first time,
> > the PLL settings are lost and the L val is reset to 0. That makes sense
> > because RETAIN_FF isn't set. This also means the other register writes
> > during probe are lost during the first suspend of the lpass core clk
> > controller. Then when the GDSC is turned on the next time for this clk
> > controller  being runtime resumed we will set the retain bit and then
> > configure the PLL again. BTW, I see that runtime PM is called for this
> > clk controller for all the clk operations. Maybe there should be some
> > auto suspend timeout so that we're not toggling the gdsc constantly?
> >
> > I hacked up the GDSC code to set the bit at gdsc registration time and
> > it seems to fix the problem I'm seeing (i.e. that the PLL is stuck,
> > which should also be in the commit text here). When I try to set the bit
> > in the bootloader though my kernel won't boot. I guess something is
> > hanging the system if I enable the retain bit in the GDSC?
> >
>
> After hacking on this for some time it looks like we can apply this
> patch instead and things are good. The first two patches in this series
> look mostly good to me minus some nitpicks so please resend.

By this you mean the two newlines you mentioned on
<https://crrev.com/c/2473610>, right?  I think all the rest of your
comments were on patch #3 (this patch) which I think we're dropping.

I'm happy to repost a v5 of just patches #1 and #2 with the newlines
fixed next week, or I'm happy if you want to fix them when applying as
you alluded to on the Chrome OS gerrit.  Just let me know.  I just
want to make sure I'm not missing some other nits before I post the
v5.  ;-)

-Doug
Stephen Boyd Oct. 19, 2020, 10:38 p.m. UTC | #4 
Quoting Doug Anderson (2020-10-16 20:17:56)
> 
> I'm happy to repost a v5 of just patches #1 and #2 with the newlines
> fixed next week, or I'm happy if you want to fix them when applying as
> you alluded to on the Chrome OS gerrit. 

Please resend. Thanks!
diff mbox series

Patch

diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
index 4ed766ca08bb..d7586858760c 100644
--- a/drivers/clk/qcom/lpasscorecc-sc7180.c
+++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
@@ -21,6 +21,9 @@ 
 #include "common.h"
 #include "gdsc.h"
 
+static const char *lpass_audio_cc_regmap_name = "lpass_audio_cc";
+static const char *lpass_core_cc_regmap_name = "lpass_core_cc";
+
 enum {
 	P_BI_TCXO,
 	P_LPASS_LPAAUDIO_DIG_PLL_OUT_ODD,
@@ -388,6 +391,25 @@  static int lpass_create_pm_clks(struct platform_device *pdev)
 	return ret;
 }
 
+static int lpass_core_cc_pm_clk_resume(struct device *dev)
+{
+	struct regmap *regmap = dev_get_regmap(dev, lpass_core_cc_regmap_name);
+	unsigned int l_val;
+	int ret;
+
+	ret = pm_clk_resume(dev);
+	if (ret)
+		return ret;
+
+	/* If PLL_L_VAL was cleared then we should re-init the whole PLL */
+	regmap_read(regmap, 0x1004, &l_val);
+	if (!l_val)
+		clk_fabia_pll_configure(&lpass_lpaaudio_dig_pll, regmap,
+				&lpass_lpaaudio_dig_pll_config);
+
+	return 0;
+}
+
 static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
 {
 	const struct qcom_cc_desc *desc;
@@ -398,13 +420,13 @@  static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	lpass_core_cc_sc7180_regmap_config.name = "lpass_audio_cc";
+	lpass_core_cc_sc7180_regmap_config.name = lpass_audio_cc_regmap_name;
 	desc = &lpass_audio_hm_sc7180_desc;
 	ret = qcom_cc_probe_by_index(pdev, 1, desc);
 	if (ret)
 		return ret;
 
-	lpass_core_cc_sc7180_regmap_config.name = "lpass_core_cc";
+	lpass_core_cc_sc7180_regmap_config.name = lpass_core_cc_regmap_name;
 	regmap = qcom_cc_map(pdev, &lpass_core_cc_sc7180_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
@@ -457,7 +479,7 @@  static const struct of_device_id lpass_core_cc_sc7180_match_table[] = {
 MODULE_DEVICE_TABLE(of, lpass_core_cc_sc7180_match_table);
 
 static const struct dev_pm_ops lpass_core_cc_pm_ops = {
-	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
+	SET_RUNTIME_PM_OPS(pm_clk_suspend, lpass_core_cc_pm_clk_resume, NULL)
 };
 
 static struct platform_driver lpass_core_cc_sc7180_driver = {