Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1564662 > unrolled thread

[PATCH v2] clk: qcom: gdsc: Fix handling of hw control enable/disable

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] clk: qcom: gdsc: Fix handling of hw control enable/disable Rajendra Nayak <rnayak@codeaurora.org> - 2017-01-23 05:30 +0100
    Re: [PATCH v2] clk: qcom: gdsc: Fix handling of hw control  enable/disable Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2017-01-26 22:50 +0100
    Re: [PATCH v2] clk: qcom: gdsc: Fix handling of hw control  enable/disable Stephen Boyd <sboyd@codeaurora.org> - 2017-01-27 01:10 +0100

#1564662 — [PATCH v2] clk: qcom: gdsc: Fix handling of hw control enable/disable

Once a gdsc is brought in and out of HW control, there is a
power down and up cycle which can take upto 1us. Polling on
the gdsc status immediately after the hw control enable/disable
can mislead software/firmware to belive the gdsc is already either on
or off, while its yet to complete the power cycle.
To avoid this add a 1us delay post a enable/disable of HW control
mode.

Also after the HW control mode is disabled, poll on the status to
check gdsc status reflects its 'on' before force disabling it
in software.

Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 58 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 288186c..a4f3580 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -63,11 +63,26 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
 	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
 }
 
+static int gdsc_poll_status(struct gdsc *sc, unsigned int reg, bool en)
+{
+	ktime_t start;
+
+	start = ktime_get();
+	do {
+		if (gdsc_is_enabled(sc, reg) == en)
+			return 0;
+	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
+
+	if (gdsc_is_enabled(sc, reg) == en)
+		return 0;
+
+	return -ETIMEDOUT;
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 {
 	int ret;
 	u32 val = en ? 0 : SW_COLLAPSE_MASK;
-	ktime_t start;
 	unsigned int status_reg = sc->gdscr;
 
 	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
@@ -100,16 +115,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 		udelay(1);
 	}
 
-	start = ktime_get();
-	do {
-		if (gdsc_is_enabled(sc, status_reg) == en)
-			return 0;
-	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
-
-	if (gdsc_is_enabled(sc, status_reg) == en)
-		return 0;
-
-	return -ETIMEDOUT;
+	return gdsc_poll_status(sc, status_reg, en);
 }
 
 static inline int gdsc_deassert_reset(struct gdsc *sc)
@@ -188,8 +194,20 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	udelay(1);
 
 	/* Turn on HW trigger mode if supported */
-	if (sc->flags & HW_CTRL)
-		return gdsc_hwctrl(sc, true);
+	if (sc->flags & HW_CTRL) {
+		ret = gdsc_hwctrl(sc, true);
+		if (ret)
+			return ret;
+		/*
+		 * Wait for the GDSC to go through a power down and
+		 * up cycle.  In case a firmware ends up polling status
+		 * bits for the gdsc, it might read an 'on' status before
+		 * the GDSC can finish the power cycle.
+		 * We wait 1us before returning to ensure the firmware
+		 * can't immediately poll the status bits.
+		 */
+		udelay(1);
+	}
 
 	return 0;
 }
@@ -204,9 +222,23 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 
 	/* Turn off HW trigger mode if supported */
 	if (sc->flags & HW_CTRL) {
+		unsigned int reg;
+
 		ret = gdsc_hwctrl(sc, false);
 		if (ret < 0)
 			return ret;
+		/*
+		 * Wait for the GDSC to go through a power down and
+		 * up cycle.  In case we end up polling status
+		 * bits for the gdsc before the power cycle is completed
+		 * it might read an 'on' status wrongly.
+		 */
+		udelay(1);
+
+		reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
+		ret = gdsc_poll_status(sc, reg, true);
+		if (ret)
+			return ret;
 	}
 
 	if (sc->pwrsts & PWRSTS_OFF)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[toc] | [next] | [standalone]

#1567734 — Re: [PATCH v2] clk: qcom: gdsc: Fix handling of hw control enable/disable

Hi,

Thanks for the patch!

On 23.01.2017 06:26, Rajendra Nayak wrote:
> Once a gdsc is brought in and out of HW control, there is a
> power down and up cycle which can take upto 1us. Polling on
> the gdsc status immediately after the hw control enable/disable
> can mislead software/firmware to belive the gdsc is already either on
> or off, while its yet to complete the power cycle.
> To avoid this add a 1us delay post a enable/disable of HW control
> mode.
>
> Also after the HW control mode is disabled, poll on the status to
> check gdsc status reflects its 'on' before force disabling it
> in software.
>
> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 58 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 13 deletions(-)

Reviewed and Tested-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

regards,
Stan

[toc] | [prev] | [next] | [standalone]

#1567827 — Re: [PATCH v2] clk: qcom: gdsc: Fix handling of hw control enable/disable

On 01/23, Rajendra Nayak wrote:
> Once a gdsc is brought in and out of HW control, there is a
> power down and up cycle which can take upto 1us. Polling on
> the gdsc status immediately after the hw control enable/disable
> can mislead software/firmware to belive the gdsc is already either on
> or off, while its yet to complete the power cycle.
> To avoid this add a 1us delay post a enable/disable of HW control
> mode.
> 
> Also after the HW control mode is disabled, poll on the status to
> check gdsc status reflects its 'on' before force disabling it
> in software.
> 
> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

Applied to clk-next + I added a fixes tag. I don't think any
driver is affected by this problem in mainline, hence the punt to
v4.11.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web