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

[Stephen Boyd]

Quoting Stephen Boyd (2020-10-15 20:16:27)
> Quoting Douglas Anderson (2020-10-14 17:13:29)
> > From: Taniya Das <tdas@xxxxxxxxxxxxxx>
> > 
> > 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)