LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] clk: qcom: gdsc: Ensure regulator init state matches GDSC state
@ 2021-07-21 22:40 Bjorn Andersson
  2021-07-31  8:02 ` Stephen Boyd
  2021-08-06  1:21 ` Stephen Boyd
  0 siblings, 2 replies; 3+ messages in thread
From: Bjorn Andersson @ 2021-07-21 22:40 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Vinod Koul, Mike Tipton, Taniya Das
  Cc: linux-arm-msm, linux-clk, linux-kernel, stable

As GDSCs are registered and found to be already enabled gdsc_init()
ensures that 1) the kernel state matches the hardware state, and 2)
votable GDSCs are properly enabled from this master as well.

But as the (optional) supply regulator is enabled deep into
gdsc_toggle_logic(), which is only executed for votable GDSCs the
kernel's state of the regulator might not match the hardware. The
regulator might be automatically turned off if no other users are
present or the next call to gdsc_disable() would cause an unbalanced
regulator_disable().

But as the votable case deals with an already enabled GDSC, most of
gdsc_enable() and gdsc_toggle_logic() can be skipped. Reducing it to
just clearing the SW_COLLAPSE_MASK and enabling hardware control allow
us to simply call regulator_enable() in both cases.

The enablement of hardware control seems to be an independent property
from the GDSC being enabled, so this is moved outside that conditional
segment.

Lastly, as the propagation of ALWAY_ON to GENPD_FLAG_ALWAYS_ON needs to
happen regardless of the initial state this is grouped together with the
other sc->pd updates at the end of the function.

Cc: stable@vger.kernel.org
Fixes: 37416e554961 ("clk: qcom: gdsc: Handle GDSC regulator supplies")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Refactored into if (on) else if (ALWAYS_ON) style
- Extracted relevant parts of gdsc_enable() to call under VOTABLE
- Turn on hwctrl if requested for non-votable gdscs

 drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 51ed640e527b..4ece326ea233 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -357,27 +357,43 @@ static int gdsc_init(struct gdsc *sc)
 	if (on < 0)
 		return on;
 
-	/*
-	 * Votable GDSCs can be ON due to Vote from other masters.
-	 * If a Votable GDSC is ON, make sure we have a Vote.
-	 */
-	if ((sc->flags & VOTABLE) && on)
-		gdsc_enable(&sc->pd);
+	if (on) {
+		/* The regulator must be on, sync the kernel state */
+		if (sc->rsupply) {
+			ret = regulator_enable(sc->rsupply);
+			if (ret < 0)
+				return ret;
+		}
 
-	/*
-	 * 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);
+		/*
+		 * Votable GDSCs can be ON due to Vote from other masters.
+		 * If a Votable GDSC is ON, make sure we have a Vote.
+		 */
+		if (sc->flags & VOTABLE) {
+			ret = regmap_update_bits(sc->regmap, sc->gdscr,
+						 SW_COLLAPSE_MASK, val);
+			if (ret)
+				return ret;
+		}
+
+		/* Turn on HW trigger mode if supported */
+		if (sc->flags & HW_CTRL) {
+			ret = gdsc_hwctrl(sc, true);
+			if (ret < 0)
+				return ret;
+		}
 
-	/* If ALWAYS_ON GDSCs are not ON, turn them ON */
-	if (sc->flags & ALWAYS_ON) {
-		if (!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)
+			gdsc_retain_ff_on(sc);
+	} else if (sc->flags & ALWAYS_ON) {
+		/* If ALWAYS_ON GDSCs are not ON, turn them ON */
+		gdsc_enable(&sc->pd);
 		on = true;
-		sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;
 	}
 
 	if (on || (sc->pwrsts & PWRSTS_RET))
@@ -385,6 +401,8 @@ static int gdsc_init(struct gdsc *sc)
 	else
 		gdsc_clear_mem_on(sc);
 
+	if (sc->flags & ALWAYS_ON)
+		sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;
 	if (!sc->pd.power_off)
 		sc->pd.power_off = gdsc_disable;
 	if (!sc->pd.power_on)
-- 
2.29.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] clk: qcom: gdsc: Ensure regulator init state matches GDSC state
  2021-07-21 22:40 [PATCH v2] clk: qcom: gdsc: Ensure regulator init state matches GDSC state Bjorn Andersson
@ 2021-07-31  8:02 ` Stephen Boyd
  2021-08-06  1:21 ` Stephen Boyd
  1 sibling, 0 replies; 3+ messages in thread
From: Stephen Boyd @ 2021-07-31  8:02 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Michael Turquette, Mike Tipton,
	Taniya Das, Vinod Koul
  Cc: linux-arm-msm, linux-clk, linux-kernel, stable

Quoting Bjorn Andersson (2021-07-21 15:40:56)
> As GDSCs are registered and found to be already enabled gdsc_init()
> ensures that 1) the kernel state matches the hardware state, and 2)
> votable GDSCs are properly enabled from this master as well.
> 
> But as the (optional) supply regulator is enabled deep into
> gdsc_toggle_logic(), which is only executed for votable GDSCs the
> kernel's state of the regulator might not match the hardware. The
> regulator might be automatically turned off if no other users are
> present or the next call to gdsc_disable() would cause an unbalanced
> regulator_disable().
> 
> But as the votable case deals with an already enabled GDSC, most of
> gdsc_enable() and gdsc_toggle_logic() can be skipped. Reducing it to
> just clearing the SW_COLLAPSE_MASK and enabling hardware control allow
> us to simply call regulator_enable() in both cases.
> 
> The enablement of hardware control seems to be an independent property
> from the GDSC being enabled, so this is moved outside that conditional
> segment.
> 
> Lastly, as the propagation of ALWAY_ON to GENPD_FLAG_ALWAYS_ON needs to
> happen regardless of the initial state this is grouped together with the
> other sc->pd updates at the end of the function.
> 
> Cc: stable@vger.kernel.org
> Fixes: 37416e554961 ("clk: qcom: gdsc: Handle GDSC regulator supplies")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

I was hoping someone from qcom would review it. I'll take a look in the
next couple of days and probably throw it on clk-fixes for the next rc.

-Stephen

> 
> Changes since v1:
> - Refactored into if (on) else if (ALWAYS_ON) style
> - Extracted relevant parts of gdsc_enable() to call under VOTABLE
> - Turn on hwctrl if requested for non-votable gdscs
> 
>  drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 51ed640e527b..4ece326ea233 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -357,27 +357,43 @@ static int gdsc_init(struct gdsc *sc)
>         if (on < 0)
>                 return on;
>  
> -       /*
> -        * Votable GDSCs can be ON due to Vote from other masters.
> -        * If a Votable GDSC is ON, make sure we have a Vote.
> -        */
> -       if ((sc->flags & VOTABLE) && on)
> -               gdsc_enable(&sc->pd);
> +       if (on) {
> +               /* The regulator must be on, sync the kernel state */
> +               if (sc->rsupply) {
> +                       ret = regulator_enable(sc->rsupply);
> +                       if (ret < 0)
> +                               return ret;
> +               }
>  
> -       /*
> -        * 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);
> +               /*
> +                * Votable GDSCs can be ON due to Vote from other masters.
> +                * If a Votable GDSC is ON, make sure we have a Vote.
> +                */
> +               if (sc->flags & VOTABLE) {
> +                       ret = regmap_update_bits(sc->regmap, sc->gdscr,
> +                                                SW_COLLAPSE_MASK, val);
> +                       if (ret)
> +                               return ret;
> +               }
> +
> +               /* Turn on HW trigger mode if supported */
> +               if (sc->flags & HW_CTRL) {
> +                       ret = gdsc_hwctrl(sc, true);
> +                       if (ret < 0)
> +                               return ret;
> +               }
>  
> -       /* If ALWAYS_ON GDSCs are not ON, turn them ON */
> -       if (sc->flags & ALWAYS_ON) {
> -               if (!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)
> +                       gdsc_retain_ff_on(sc);
> +       } else if (sc->flags & ALWAYS_ON) {
> +               /* If ALWAYS_ON GDSCs are not ON, turn them ON */
> +               gdsc_enable(&sc->pd);
>                 on = true;
> -               sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;
>         }
>  
>         if (on || (sc->pwrsts & PWRSTS_RET))
> @@ -385,6 +401,8 @@ static int gdsc_init(struct gdsc *sc)
>         else
>                 gdsc_clear_mem_on(sc);
>  
> +       if (sc->flags & ALWAYS_ON)
> +               sc->pd.flags |= GENPD_FLAG_ALWAYS_ON;
>         if (!sc->pd.power_off)
>                 sc->pd.power_off = gdsc_disable;
>         if (!sc->pd.power_on)
> -- 
> 2.29.2
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] clk: qcom: gdsc: Ensure regulator init state matches GDSC state
  2021-07-21 22:40 [PATCH v2] clk: qcom: gdsc: Ensure regulator init state matches GDSC state Bjorn Andersson
  2021-07-31  8:02 ` Stephen Boyd
@ 2021-08-06  1:21 ` Stephen Boyd
  1 sibling, 0 replies; 3+ messages in thread
From: Stephen Boyd @ 2021-08-06  1:21 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Michael Turquette, Mike Tipton,
	Taniya Das, Vinod Koul
  Cc: linux-arm-msm, linux-clk, linux-kernel, stable

Quoting Bjorn Andersson (2021-07-21 15:40:56)
> As GDSCs are registered and found to be already enabled gdsc_init()
> ensures that 1) the kernel state matches the hardware state, and 2)
> votable GDSCs are properly enabled from this master as well.
> 
> But as the (optional) supply regulator is enabled deep into
> gdsc_toggle_logic(), which is only executed for votable GDSCs the
> kernel's state of the regulator might not match the hardware. The
> regulator might be automatically turned off if no other users are
> present or the next call to gdsc_disable() would cause an unbalanced
> regulator_disable().
> 
> But as the votable case deals with an already enabled GDSC, most of
> gdsc_enable() and gdsc_toggle_logic() can be skipped. Reducing it to
> just clearing the SW_COLLAPSE_MASK and enabling hardware control allow
> us to simply call regulator_enable() in both cases.
> 
> The enablement of hardware control seems to be an independent property
> from the GDSC being enabled, so this is moved outside that conditional
> segment.
> 
> Lastly, as the propagation of ALWAY_ON to GENPD_FLAG_ALWAYS_ON needs to
> happen regardless of the initial state this is grouped together with the
> other sc->pd updates at the end of the function.
> 
> Cc: stable@vger.kernel.org
> Fixes: 37416e554961 ("clk: qcom: gdsc: Handle GDSC regulator supplies")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Applied to clk-fixes

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-06  1:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 22:40 [PATCH v2] clk: qcom: gdsc: Ensure regulator init state matches GDSC state Bjorn Andersson
2021-07-31  8:02 ` Stephen Boyd
2021-08-06  1:21 ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).