[PATCH v2 0/3] Update reset and poll logic for GDSCs

[PATCH v2 0/3] Update reset and poll logic for GDSCs

[Taniya Das]

 [v2]
  * Addressed review comments given in v1 series

This series implements the below logic for the GDSCs

 1. logic to reset the AON logic before or assert/deassert the block
   control reset removing the clamp io for few GDSCs on SDM845 SoC.
 2. It also introduces the requirement to poll for higher timeout values
   for few of the GDSCs.
 3. There is a new poll register for the GDSCs on SDM845 SoCs which needs
   to be polled for the correct hardware status of the GDSCs.

Amit Nischal (3):
  clk: qcom: gdsc: Add support to reset AON and block reset logic
  clk: qcom: gdsc: Add support to poll for higher timeout value
  clk: qcom: gdsc: Add support to poll CFG register to check GDSC state

 drivers/clk/qcom/gdsc.c | 63 +++++++++++++++++++++++++++++++++++++++++++------
 drivers/clk/qcom/gdsc.h |  5 +++-
 2 files changed, 60 insertions(+), 8 deletions(-)

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[PATCH v2 1/3] clk: qcom: gdsc: Add support to reset AON and block reset logic

[Taniya Das]

From: Amit Nischal <anischal@codeaurora.org>

For some of the gdsc power domains, there could be need to reset the
AON logic or assert/deassert the block control reset before removing
the clamp_io. Add support for the same by introducing new flags
SW_RESET and AON_RESET. Both SW reset and AON reset requires to be
asserted for at least 1us before being de-asserted.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Amit Nischal <anischal@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h |  4 +++-
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a4f3580..266fefa 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015, 2017-2018, The Linux Foundation. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and
@@ -31,6 +31,7 @@
 #define HW_CONTROL_MASK		BIT(1)
 #define SW_COLLAPSE_MASK	BIT(0)
 #define GMEM_CLAMP_IO_MASK	BIT(0)
+#define GMEM_RESET_MASK		BIT(4)

 /* Wait 2^n CXO cycles between all states. Here, n=2 (4 cycles). */
 #define EN_REST_WAIT_VAL	(0x2 << 20)
@@ -166,6 +167,14 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
 			   GMEM_CLAMP_IO_MASK, 1);
 }

+static inline void gdsc_assert_reset_aon(struct gdsc *sc)
+{
+	regmap_update_bits(sc->regmap, sc->clamp_io_ctrl,
+			   GMEM_RESET_MASK, 1);
+	udelay(1);
+	regmap_update_bits(sc->regmap, sc->clamp_io_ctrl,
+			   GMEM_RESET_MASK, 0);
+}
 static int gdsc_enable(struct generic_pm_domain *domain)
 {
 	struct gdsc *sc = domain_to_gdsc(domain);
@@ -174,8 +183,17 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_deassert_reset(sc);

-	if (sc->flags & CLAMP_IO)
+	if (sc->flags & SW_RESET) {
+		gdsc_assert_reset(sc);
+		udelay(1);
+		gdsc_deassert_reset(sc);
+	}
+
+	if (sc->flags & CLAMP_IO) {
+		if (sc->flags & AON_RESET)
+			gdsc_assert_reset_aon(sc);
 		gdsc_deassert_clamp_io(sc);
+	}

 	ret = gdsc_toggle_logic(sc, true);
 	if (ret)
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 3964834..9279278 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015, 2017-2018, The Linux Foundation. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and
@@ -53,6 +53,8 @@ struct gdsc {
 #define VOTABLE		BIT(0)
 #define CLAMP_IO	BIT(1)
 #define HW_CTRL		BIT(2)
+#define SW_RESET	BIT(3)
+#define AON_RESET	BIT(4)
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[PATCH v2 2/3] clk: qcom: gdsc: Add support to poll for higher timeout value

[Taniya Das]

From: Amit Nischal <anischal@codeaurora.org>

For some gdscs, it might take longer time up to 500us for updating their
status. Update the timeout value for all GDSC polling status.

Signed-off-by: Amit Nischal <anischal@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 266fefa..cb61c15 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -41,7 +41,7 @@
 #define RETAIN_MEM		BIT(14)
 #define RETAIN_PERIPH		BIT(13)

-#define TIMEOUT_US		100
+#define TIMEOUT_US		500

 #define domain_to_gdsc(domain) container_of(domain, struct gdsc, pd)

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[PATCH v2 3/3] clk: qcom: gdsc: Add support to poll CFG register to check GDSC state

[Taniya Das]

From: Amit Nischal <anischal@codeaurora.org>

The default behavior of the GDSC enable/disable sequence is to
poll the status bits of either the actual GDSCR or the
corresponding HW_CTRL registers.

On targets which have support for a CFG_GDSCR register, the
status bits might not show the correct state of the GDSC,
especially in the disable sequence, where the status bit
will be cleared even before the core is completely power
collapsed. On targets with this issue, poll the power on/off
bits in the CFG_GDSCR register instead to correctly determine
the GDSC state.

Signed-off-by: Amit Nischal <anischal@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 39 +++++++++++++++++++++++++++++++++++----
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index cb61c15..2dda2d5 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -33,6 +33,11 @@
 #define GMEM_CLAMP_IO_MASK	BIT(0)
 #define GMEM_RESET_MASK		BIT(4)

+/* CFG_GDSCR */
+#define GDSC_POWER_UP_COMPLETE		BIT(16)
+#define GDSC_POWER_DOWN_COMPLETE	BIT(15)
+#define CFG_GDSCR_OFFSET		0x4
+
 /* Wait 2^n CXO cycles between all states. Here, n=2 (4 cycles). */
 #define EN_REST_WAIT_VAL	(0x2 << 20)
 #define EN_FEW_WAIT_VAL		(0x8 << 16)
@@ -64,18 +69,43 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
 	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
 }

+static int gdsc_is_enabled_by_poll_cfg_reg(struct gdsc *sc, bool en)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_read(sc->regmap, sc->gdscr + CFG_GDSCR_OFFSET, &val);
+	if (ret)
+		return ret;
+
+	if (en)
+		return !!(val & GDSC_POWER_UP_COMPLETE);
+
+	return !(val & GDSC_POWER_DOWN_COMPLETE);
+}
+
 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;
+		if (sc->flags & POLL_CFG_GDSCR) {
+			if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en)
+				return 0;
+		} else {
+			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;
+	if (sc->flags & POLL_CFG_GDSCR) {
+		if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en)
+			return 0;
+	} else {
+		if (gdsc_is_enabled(sc, reg) == en)
+			return 0;
+	}

 	return -ETIMEDOUT;
 }
@@ -254,6 +284,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 		udelay(1);

 		reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
+
 		ret = gdsc_poll_status(sc, reg, true);
 		if (ret)
 			return ret;
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 9279278..0f992e8 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -55,6 +55,7 @@ struct gdsc {
 #define HW_CTRL		BIT(2)
 #define SW_RESET	BIT(3)
 #define AON_RESET	BIT(4)
+#define POLL_CFG_GDSCR  BIT(5)
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v2 1/3] clk: qcom: gdsc: Add support to reset AON and block reset logic

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:44)
> From: Amit Nischal <anischal@codeaurora.org>
> 
> For some of the gdsc power domains, there could be need to reset the
> AON logic or assert/deassert the block control reset before removing
> the clamp_io. Add support for the same by introducing new flags
> SW_RESET and AON_RESET. Both SW reset and AON reset requires to be
> asserted for at least 1us before being de-asserted.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> ---

Applied to clk-next

Re: [PATCH v2 1/3] clk: qcom: gdsc: Add support to reset AON and block reset logic

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:44)
> From: Amit Nischal <anischal@codeaurora.org>
> 
> For some of the gdsc power domains, there could be need to reset the
> AON logic or assert/deassert the block control reset before removing
> the clamp_io. Add support for the same by introducing new flags
> SW_RESET and AON_RESET. Both SW reset and AON reset requires to be
> asserted for at least 1us before being de-asserted.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> ---

Applied to clk-next

Re: [PATCH v2 1/3] clk: qcom: gdsc: Add support to reset AON and block reset logic

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:44)
> From: Amit Nischal <anischal@codeaurora.org>
> =

> For some of the gdsc power domains, there could be need to reset the
> AON logic or assert/deassert the block control reset before removing
> the clamp_io. Add support for the same by introducing new flags
> SW_RESET and AON_RESET. Both SW reset and AON reset requires to be
> asserted for at least 1us before being de-asserted.
> =

> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> ---

Applied to clk-next

Re: [PATCH v2 2/3] clk: qcom: gdsc: Add support to poll for higher timeout value

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:45)
> From: Amit Nischal <anischal@codeaurora.org>
> 
> For some gdscs, it might take longer time up to 500us for updating their
> status. Update the timeout value for all GDSC polling status.
> 
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---

Applied to clk-next

Re: [PATCH v2 2/3] clk: qcom: gdsc: Add support to poll for higher timeout value

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:45)
> From: Amit Nischal <anischal@codeaurora.org>
> 
> For some gdscs, it might take longer time up to 500us for updating their
> status. Update the timeout value for all GDSC polling status.
> 
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---

Applied to clk-next

Re: [PATCH v2 2/3] clk: qcom: gdsc: Add support to poll for higher timeout value

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:45)
> From: Amit Nischal <anischal@codeaurora.org>
> =

> For some gdscs, it might take longer time up to 500us for updating their
> status. Update the timeout value for all GDSC polling status.
> =

> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---

Applied to clk-next

Re: [PATCH v2 3/3] clk: qcom: gdsc: Add support to poll CFG register to check GDSC state

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:46)
> @@ -64,18 +69,43 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
>         return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
>  }
> 
> +static int gdsc_is_enabled_by_poll_cfg_reg(struct gdsc *sc, bool en)
> +{
> +       u32 val;
> +       int ret;
> +
> +       ret = regmap_read(sc->regmap, sc->gdscr + CFG_GDSCR_OFFSET, &val);
> +       if (ret)
> +               return ret;
> +
> +       if (en)
> +               return !!(val & GDSC_POWER_UP_COMPLETE);
> +
> +       return !(val & GDSC_POWER_DOWN_COMPLETE);
> +}
> +
>  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;
> +               if (sc->flags & POLL_CFG_GDSCR) {
> +                       if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en)
> +                               return 0;
> +               } else {
> +                       if (gdsc_is_enabled(sc, reg) == en)
> +                               return 0;

Ok, I thought you would bury the is_enabled_by_poll_cfg_reg() stuff into
gdsc_is_enabled() directly. Because that function is used in one more
place than here, in gdsc_init(), to figure out if a GDSC is on at boot
time. The sc->gds_hw_ctrl check should be able to get buried into
gdsc_is_enabled() as well so that the function doesn't have to take a
register at all.

> +               }
>         } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> 
> -       if (gdsc_is_enabled(sc, reg) == en)
> -               return 0;
> +       if (sc->flags & POLL_CFG_GDSCR) {
> +               if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en)
> +                       return 0;
> +       } else {
> +               if (gdsc_is_enabled(sc, reg) == en)
> +                       return 0;
> +       }

And then this duplicate diff wouldn't happen either.

> 
>         return -ETIMEDOUT;
>  }
> @@ -254,6 +284,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>                 udelay(1);
> 
>                 reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
> +

This change shouldn't be here?

>                 ret = gdsc_poll_status(sc, reg, true);
>                 if (ret)
>                         return ret;

Re: [PATCH v2 3/3] clk: qcom: gdsc: Add support to poll CFG register to check GDSC state

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:46)
> @@ -64,18 +69,43 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
>         return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
>  }
> 
> +static int gdsc_is_enabled_by_poll_cfg_reg(struct gdsc *sc, bool en)
> +{
> +       u32 val;
> +       int ret;
> +
> +       ret = regmap_read(sc->regmap, sc->gdscr + CFG_GDSCR_OFFSET, &val);
> +       if (ret)
> +               return ret;
> +
> +       if (en)
> +               return !!(val & GDSC_POWER_UP_COMPLETE);
> +
> +       return !(val & GDSC_POWER_DOWN_COMPLETE);
> +}
> +
>  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;
> +               if (sc->flags & POLL_CFG_GDSCR) {
> +                       if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en)
> +                               return 0;
> +               } else {
> +                       if (gdsc_is_enabled(sc, reg) == en)
> +                               return 0;

Ok, I thought you would bury the is_enabled_by_poll_cfg_reg() stuff into
gdsc_is_enabled() directly. Because that function is used in one more
place than here, in gdsc_init(), to figure out if a GDSC is on at boot
time. The sc->gds_hw_ctrl check should be able to get buried into
gdsc_is_enabled() as well so that the function doesn't have to take a
register at all.

> +               }
>         } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> 
> -       if (gdsc_is_enabled(sc, reg) == en)
> -               return 0;
> +       if (sc->flags & POLL_CFG_GDSCR) {
> +               if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en)
> +                       return 0;
> +       } else {
> +               if (gdsc_is_enabled(sc, reg) == en)
> +                       return 0;
> +       }

And then this duplicate diff wouldn't happen either.

> 
>         return -ETIMEDOUT;
>  }
> @@ -254,6 +284,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>                 udelay(1);
> 
>                 reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
> +

This change shouldn't be here?

>                 ret = gdsc_poll_status(sc, reg, true);
>                 if (ret)
>                         return ret;

Re: [PATCH v2 3/3] clk: qcom: gdsc: Add support to poll CFG register to check GDSC state

[Stephen Boyd]

Quoting Taniya Das (2018-04-09 01:41:46)
> @@ -64,18 +69,43 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
>         return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK,=
 val);
>  }
> =

> +static int gdsc_is_enabled_by_poll_cfg_reg(struct gdsc *sc, bool en)
> +{
> +       u32 val;
> +       int ret;
> +
> +       ret =3D regmap_read(sc->regmap, sc->gdscr + CFG_GDSCR_OFFSET, &va=
l);
> +       if (ret)
> +               return ret;
> +
> +       if (en)
> +               return !!(val & GDSC_POWER_UP_COMPLETE);
> +
> +       return !(val & GDSC_POWER_DOWN_COMPLETE);
> +}
> +
>  static int gdsc_poll_status(struct gdsc *sc, unsigned int reg, bool en)
>  {
>         ktime_t start;
> =

>         start =3D ktime_get();
>         do {
> -               if (gdsc_is_enabled(sc, reg) =3D=3D en)
> -                       return 0;
> +               if (sc->flags & POLL_CFG_GDSCR) {
> +                       if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) =3D=
=3D en)
> +                               return 0;
> +               } else {
> +                       if (gdsc_is_enabled(sc, reg) =3D=3D en)
> +                               return 0;

Ok, I thought you would bury the is_enabled_by_poll_cfg_reg() stuff into
gdsc_is_enabled() directly. Because that function is used in one more
place than here, in gdsc_init(), to figure out if a GDSC is on at boot
time. The sc->gds_hw_ctrl check should be able to get buried into
gdsc_is_enabled() as well so that the function doesn't have to take a
register at all.

> +               }
>         } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> =

> -       if (gdsc_is_enabled(sc, reg) =3D=3D en)
> -               return 0;
> +       if (sc->flags & POLL_CFG_GDSCR) {
> +               if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) =3D=3D en)
> +                       return 0;
> +       } else {
> +               if (gdsc_is_enabled(sc, reg) =3D=3D en)
> +                       return 0;
> +       }

And then this duplicate diff wouldn't happen either.

> =

>         return -ETIMEDOUT;
>  }
> @@ -254,6 +284,7 @@ static int gdsc_disable(struct generic_pm_domain *dom=
ain)
>                 udelay(1);
> =

>                 reg =3D sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
> +

This change shouldn't be here?

>                 ret =3D gdsc_poll_status(sc, reg, true);
>                 if (ret)
>                         return ret;