2022-08-18 20:23:50

by Akhil P Oommen

[permalink] [raw]
Subject: [PATCH v3 0/5] clk/qcom: Support gdsc collapse polling using 'reset' inteface


Some clients like adreno gpu driver would like to ensure that its gdsc
is collapsed at hardware during a gpu reset sequence. This is because it
has a votable gdsc which could be ON due to a vote from another subsystem
like tz, hyp etc or due to an internal hardware signal. To allow
this, gpucc driver can expose an interface to the client driver using
reset framework. Using this the client driver can trigger a polling within
the gdsc driver.

This series is rebased on top of linus's master branch.

Related discussion: https://patchwork.freedesktop.org/patch/493144/

Changes in v3:
- Use pointer to const for "struct qcom_reset_ops" in qcom_reset_map (Krzysztof)

Changes in v2:
- Return error when a particular custom reset op is not implemented. (Dmitry)

Akhil P Oommen (5):
dt-bindings: clk: qcom: Support gpu cx gdsc reset
clk: qcom: Allow custom reset ops
clk: qcom: gdsc: Add a reset op to poll gdsc collapse
clk: qcom: gpucc-sc7280: Add cx collapse reset support
arm64: dts: qcom: sc7280: Add Reset support for gpu

arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
drivers/clk/qcom/gdsc.h | 7 +++++++
drivers/clk/qcom/gpucc-sc7280.c | 10 ++++++++++
drivers/clk/qcom/reset.c | 27 +++++++++++++++++++++++++++
drivers/clk/qcom/reset.h | 8 ++++++++
include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
7 files changed, 77 insertions(+), 4 deletions(-)

--
2.7.4


2022-08-18 20:23:59

by Akhil P Oommen

[permalink] [raw]
Subject: [PATCH v3 2/5] clk: qcom: Allow custom reset ops

Allow soc specific clk drivers to specify a custom reset operation. We
will use this in an upcoming patch to allow gpucc driver to specify a
differet reset operation for cx_gdsc.

Signed-off-by: Akhil P Oommen <[email protected]>
---

Changes in v3:
- Use pointer to const for "struct qcom_reset_ops" in qcom_reset_map (Krzysztof)

Changes in v2:
- Return error when a particular custom reset op is not implemented. (Dmitry)

drivers/clk/qcom/reset.c | 27 +++++++++++++++++++++++++++
drivers/clk/qcom/reset.h | 8 ++++++++
2 files changed, 35 insertions(+)

diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c
index 819d194..b7ae4a3 100644
--- a/drivers/clk/qcom/reset.c
+++ b/drivers/clk/qcom/reset.c
@@ -13,6 +13,21 @@

static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id)
{
+ struct qcom_reset_controller *rst;
+ const struct qcom_reset_map *map;
+
+ rst = to_qcom_reset_controller(rcdev);
+ map = &rst->reset_map[id];
+
+ if (map->ops && map->ops->reset)
+ return map->ops->reset(map->priv);
+ /*
+ * If custom ops is implemented but just not this callback, return
+ * error
+ */
+ else if (map->ops)
+ return -EOPNOTSUPP;
+
rcdev->ops->assert(rcdev, id);
udelay(1);
rcdev->ops->deassert(rcdev, id);
@@ -28,6 +43,12 @@ qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)

rst = to_qcom_reset_controller(rcdev);
map = &rst->reset_map[id];
+
+ if (map->ops && map->ops->assert)
+ return map->ops->assert(map->priv);
+ else if (map->ops)
+ return -EOPNOTSUPP;
+
mask = BIT(map->bit);

return regmap_update_bits(rst->regmap, map->reg, mask, mask);
@@ -42,6 +63,12 @@ qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)

rst = to_qcom_reset_controller(rcdev);
map = &rst->reset_map[id];
+
+ if (map->ops && map->ops->deassert)
+ return map->ops->deassert(map->priv);
+ else if (map->ops)
+ return -EOPNOTSUPP;
+
mask = BIT(map->bit);

return regmap_update_bits(rst->regmap, map->reg, mask, 0);
diff --git a/drivers/clk/qcom/reset.h b/drivers/clk/qcom/reset.h
index 2a08b5e..f3147eb 100644
--- a/drivers/clk/qcom/reset.h
+++ b/drivers/clk/qcom/reset.h
@@ -8,9 +8,17 @@

#include <linux/reset-controller.h>

+struct qcom_reset_ops {
+ int (*reset)(void *priv);
+ int (*assert)(void *priv);
+ int (*deassert)(void *priv);
+};
+
struct qcom_reset_map {
unsigned int reg;
u8 bit;
+ const struct qcom_reset_ops *ops;
+ void *priv;
};

struct regmap;
--
2.7.4

2022-08-18 20:24:22

by Akhil P Oommen

[permalink] [raw]
Subject: [PATCH v3 5/5] arm64: dts: qcom: sc7280: Add Reset support for gpu

Add support for Reset using GPUCC driver for GPU. This helps to ensure
that GPU state is reset by making sure that CX head switch is collapsed.

Signed-off-by: Akhil P Oommen <[email protected]>
---

(no changes since v1)

arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index e66fc67..f5257d6 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2243,6 +2243,9 @@
nvmem-cells = <&gpu_speed_bin>;
nvmem-cell-names = "speed_bin";

+ resets = <&gpucc GPU_CX_COLLAPSE>;
+ reset-names = "cx_collapse";
+
gpu_opp_table: opp-table {
compatible = "operating-points-v2";

--
2.7.4

2022-08-18 20:38:22

by Akhil P Oommen

[permalink] [raw]
Subject: [PATCH v3 3/5] clk: qcom: gdsc: Add a reset op to poll gdsc collapse

Add a reset op compatible function to poll for gdsc collapse.

Signed-off-by: Akhil P Oommen <[email protected]>
---

(no changes since v2)

Changes in v2:
- Minor update to function prototype

drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
drivers/clk/qcom/gdsc.h | 7 +++++++
2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 44520ef..2d0f1d1 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -17,6 +17,7 @@
#include <linux/reset-controller.h>
#include <linux/slab.h>
#include "gdsc.h"
+#include "reset.h"

#define PWR_ON_MASK BIT(31)
#define EN_REST_WAIT_MASK GENMASK_ULL(23, 20)
@@ -116,7 +117,8 @@ 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, enum gdsc_status status)
+static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status,
+ s64 timeout_us, unsigned int interval_ms)
{
ktime_t start;

@@ -124,7 +126,9 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
do {
if (gdsc_check_status(sc, status))
return 0;
- } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
+ if (interval_ms)
+ msleep(interval_ms);
+ } while (ktime_us_delta(ktime_get(), start) < timeout_us);

if (gdsc_check_status(sc, status))
return 0;
@@ -172,7 +176,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
udelay(1);
}

- ret = gdsc_poll_status(sc, status);
+ ret = gdsc_poll_status(sc, status, TIMEOUT_US, 0);
WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");

if (!ret && status == GDSC_OFF && sc->rsupply) {
@@ -343,7 +347,7 @@ static int _gdsc_disable(struct gdsc *sc)
*/
udelay(1);

- ret = gdsc_poll_status(sc, GDSC_ON);
+ ret = gdsc_poll_status(sc, GDSC_ON, TIMEOUT_US, 0);
if (ret)
return ret;
}
@@ -565,3 +569,14 @@ int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
return 0;
}
EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
+
+int gdsc_wait_for_collapse(void *priv)
+{
+ struct gdsc *sc = priv;
+ int ret;
+
+ ret = gdsc_poll_status(sc, GDSC_OFF, 500000, 5);
+ WARN(ret, "%s status stuck at 'on'", sc->pd.name);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gdsc_wait_for_collapse);
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index ad313d7..d484bdb 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -12,6 +12,7 @@
struct regmap;
struct regulator;
struct reset_controller_dev;
+struct qcom_reset_map;

/**
* struct gdsc - Globally Distributed Switch Controller
@@ -79,6 +80,7 @@ int gdsc_register(struct gdsc_desc *desc, struct reset_controller_dev *,
struct regmap *);
void gdsc_unregister(struct gdsc_desc *desc);
int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain);
+int gdsc_wait_for_collapse(void *priv);
#else
static inline int gdsc_register(struct gdsc_desc *desc,
struct reset_controller_dev *rcdev,
@@ -88,5 +90,10 @@ static inline int gdsc_register(struct gdsc_desc *desc,
}

static inline void gdsc_unregister(struct gdsc_desc *desc) {};
+
+static int gdsc_wait_for_collapse(void *priv)
+{
+ return -ENOSYS;
+}
#endif /* CONFIG_QCOM_GDSC */
#endif /* __QCOM_GDSC_H__ */
--
2.7.4

2022-08-18 21:27:46

by Akhil P Oommen

[permalink] [raw]
Subject: [PATCH v3 4/5] clk: qcom: gpucc-sc7280: Add cx collapse reset support

Allow a consumer driver to poll for cx gdsc collapse through Reset
framework.

Signed-off-by: Akhil P Oommen <[email protected]>
---

Changes in v3:
- Convert 'struct qcom_reset_ops cx_gdsc_reset' to 'static const' (Krzysztof)

Changes in v2:
- Minor update to use the updated custom reset ops implementation

drivers/clk/qcom/gpucc-sc7280.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/clk/qcom/gpucc-sc7280.c b/drivers/clk/qcom/gpucc-sc7280.c
index 9a832f2..fece3f4 100644
--- a/drivers/clk/qcom/gpucc-sc7280.c
+++ b/drivers/clk/qcom/gpucc-sc7280.c
@@ -433,12 +433,22 @@ static const struct regmap_config gpu_cc_sc7280_regmap_config = {
.fast_io = true,
};

+static const struct qcom_reset_ops cx_gdsc_reset = {
+ .reset = gdsc_wait_for_collapse,
+};
+
+static const struct qcom_reset_map gpucc_sc7280_resets[] = {
+ [GPU_CX_COLLAPSE] = { .ops = &cx_gdsc_reset, .priv = &cx_gdsc },
+};
+
static const struct qcom_cc_desc gpu_cc_sc7280_desc = {
.config = &gpu_cc_sc7280_regmap_config,
.clks = gpu_cc_sc7280_clocks,
.num_clks = ARRAY_SIZE(gpu_cc_sc7280_clocks),
.gdscs = gpu_cc_sc7180_gdscs,
.num_gdscs = ARRAY_SIZE(gpu_cc_sc7180_gdscs),
+ .resets = gpucc_sc7280_resets,
+ .num_resets = ARRAY_SIZE(gpucc_sc7280_resets),
};

static const struct of_device_id gpu_cc_sc7280_match_table[] = {
--
2.7.4

2022-08-18 21:36:06

by Akhil P Oommen

[permalink] [raw]
Subject: [PATCH v3 1/5] dt-bindings: clk: qcom: Support gpu cx gdsc reset

Add necessary definitions in gpucc bindings to ensure gpu cx gdsc collapse
through 'reset' framework for SC7280.

Signed-off-by: Akhil P Oommen <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---

(no changes since v1)

include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/clock/qcom,gpucc-sc7280.h b/include/dt-bindings/clock/qcom,gpucc-sc7280.h
index 669b23b..843a31b 100644
--- a/include/dt-bindings/clock/qcom,gpucc-sc7280.h
+++ b/include/dt-bindings/clock/qcom,gpucc-sc7280.h
@@ -32,4 +32,7 @@
#define GPU_CC_CX_GDSC 0
#define GPU_CC_GX_GDSC 1

+/* GPU_CC reset IDs */
+#define GPU_CX_COLLAPSE 0
+
#endif
--
2.7.4

2022-08-19 07:17:00

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] arm64: dts: qcom: sc7280: Add Reset support for gpu

On 18/08/2022 23:18, Akhil P Oommen wrote:
> Add support for Reset using GPUCC driver for GPU. This helps to ensure
> that GPU state is reset by making sure that CX head switch is collapsed.
>
> Signed-off-by: Akhil P Oommen <[email protected]>
> ---
>
> (no changes since v1)
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index e66fc67..f5257d6 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2243,6 +2243,9 @@
> nvmem-cells = <&gpu_speed_bin>;
> nvmem-cell-names = "speed_bin";
>
> + resets = <&gpucc GPU_CX_COLLAPSE>;
> + reset-names = "cx_collapse";
> +

I think this is not allowed by bindings. Did you test your change with
dtbs_check?

Best regards,
Krzysztof

2022-08-19 17:41:34

by Akhil P Oommen

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] arm64: dts: qcom: sc7280: Add Reset support for gpu

On 8/19/2022 11:47 AM, Krzysztof Kozlowski wrote:
> On 18/08/2022 23:18, Akhil P Oommen wrote:
>> Add support for Reset using GPUCC driver for GPU. This helps to ensure
>> that GPU state is reset by making sure that CX head switch is collapsed.
>>
>> Signed-off-by: Akhil P Oommen <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index e66fc67..f5257d6 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -2243,6 +2243,9 @@
>> nvmem-cells = <&gpu_speed_bin>;
>> nvmem-cell-names = "speed_bin";
>>
>> + resets = <&gpucc GPU_CX_COLLAPSE>;
>> + reset-names = "cx_collapse";
>> +
> I think this is not allowed by bindings. Did you test your change with
> dtbs_check?
>
> Best regards,
> Krzysztof
My bad! Thanks for pointing this out. Fixed in v4 with patch 5/6.

-Akhil.

2022-09-27 17:38:54

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH v3 3/5] clk: qcom: gdsc: Add a reset op to poll gdsc collapse

On Fri, Aug 19, 2022 at 01:48:37AM +0530, Akhil P Oommen wrote:
> Add a reset op compatible function to poll for gdsc collapse.
>
> Signed-off-by: Akhil P Oommen <[email protected]>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Minor update to function prototype
>
> drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
> drivers/clk/qcom/gdsc.h | 7 +++++++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 44520ef..2d0f1d1 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -17,6 +17,7 @@
> #include <linux/reset-controller.h>
> #include <linux/slab.h>
> #include "gdsc.h"
> +#include "reset.h"
>
> #define PWR_ON_MASK BIT(31)
> #define EN_REST_WAIT_MASK GENMASK_ULL(23, 20)
> @@ -116,7 +117,8 @@ 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, enum gdsc_status status)
> +static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status,
> + s64 timeout_us, unsigned int interval_ms)
> {
> ktime_t start;
>
> @@ -124,7 +126,9 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
> do {
> if (gdsc_check_status(sc, status))
> return 0;
> - } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> + if (interval_ms)
> + msleep(interval_ms);

You effectively msleep(5) here, for which you shouldn't use msleep() -
or more likely, this only happens in exceptional circumstances, so a
longer interval_ms seems reasonable.

> + } while (ktime_us_delta(ktime_get(), start) < timeout_us);
>
> if (gdsc_check_status(sc, status))
> return 0;
> @@ -172,7 +176,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
> udelay(1);
> }
>
> - ret = gdsc_poll_status(sc, status);
> + ret = gdsc_poll_status(sc, status, TIMEOUT_US, 0);
> WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>
> if (!ret && status == GDSC_OFF && sc->rsupply) {
> @@ -343,7 +347,7 @@ static int _gdsc_disable(struct gdsc *sc)
> */
> udelay(1);
>
> - ret = gdsc_poll_status(sc, GDSC_ON);
> + ret = gdsc_poll_status(sc, GDSC_ON, TIMEOUT_US, 0);
> if (ret)
> return ret;
> }
> @@ -565,3 +569,14 @@ int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
> return 0;
> }
> EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
> +
> +int gdsc_wait_for_collapse(void *priv)
> +{
> + struct gdsc *sc = priv;
> + int ret;
> +
> + ret = gdsc_poll_status(sc, GDSC_OFF, 500000, 5);

So I presume the GPU driver will put() the GDSC and then issue a reset,
which will wait up to 5 seconds for the GDSC to be turned off.

So essentially, this logic is needed because we don't wait for VOTABLE
GDSCs to be turned off? And we have no way to do the put-with-wait for
this specific case.

I would like the commit message to capture this reasoning.

Thanks,
Bjorn

> + WARN(ret, "%s status stuck at 'on'", sc->pd.name);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(gdsc_wait_for_collapse);
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index ad313d7..d484bdb 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -12,6 +12,7 @@
> struct regmap;
> struct regulator;
> struct reset_controller_dev;
> +struct qcom_reset_map;
>
> /**
> * struct gdsc - Globally Distributed Switch Controller
> @@ -79,6 +80,7 @@ int gdsc_register(struct gdsc_desc *desc, struct reset_controller_dev *,
> struct regmap *);
> void gdsc_unregister(struct gdsc_desc *desc);
> int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain);
> +int gdsc_wait_for_collapse(void *priv);
> #else
> static inline int gdsc_register(struct gdsc_desc *desc,
> struct reset_controller_dev *rcdev,
> @@ -88,5 +90,10 @@ static inline int gdsc_register(struct gdsc_desc *desc,
> }
>
> static inline void gdsc_unregister(struct gdsc_desc *desc) {};
> +
> +static int gdsc_wait_for_collapse(void *priv)
> +{
> + return -ENOSYS;
> +}
> #endif /* CONFIG_QCOM_GDSC */
> #endif /* __QCOM_GDSC_H__ */
> --
> 2.7.4
>

2022-09-28 08:36:42

by Akhil P Oommen

[permalink] [raw]
Subject: Re: [PATCH v3 3/5] clk: qcom: gdsc: Add a reset op to poll gdsc collapse

On 9/27/2022 10:56 PM, Bjorn Andersson wrote:
> On Fri, Aug 19, 2022 at 01:48:37AM +0530, Akhil P Oommen wrote:
>> Add a reset op compatible function to poll for gdsc collapse.
>>
>> Signed-off-by: Akhil P Oommen <[email protected]>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - Minor update to function prototype
>>
>> drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
>> drivers/clk/qcom/gdsc.h | 7 +++++++
>> 2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index 44520ef..2d0f1d1 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -17,6 +17,7 @@
>> #include <linux/reset-controller.h>
>> #include <linux/slab.h>
>> #include "gdsc.h"
>> +#include "reset.h"
>>
>> #define PWR_ON_MASK BIT(31)
>> #define EN_REST_WAIT_MASK GENMASK_ULL(23, 20)
>> @@ -116,7 +117,8 @@ 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, enum gdsc_status status)
>> +static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status,
>> + s64 timeout_us, unsigned int interval_ms)
>> {
>> ktime_t start;
>>
>> @@ -124,7 +126,9 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
>> do {
>> if (gdsc_check_status(sc, status))
>> return 0;
>> - } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
>> + if (interval_ms)
>> + msleep(interval_ms);
> You effectively msleep(5) here, for which you shouldn't use msleep() -
> or more likely, this only happens in exceptional circumstances, so a
> longer interval_ms seems reasonable.
By reducing the overall polling time here, we can reduce any user
visible impact like missing frame/janks due to gpu hang/recovery. I kept
5ms here because in my local testing on sc7280 device I didn't see any
benefit beyond decreasing below 5ms. Msleep() here also helps to quickly
schedule other threads which holds pm_runtime refcount on cx_gdsc, which
indirectly helps to reduce overall polling time here significantly in my
testing.

>
>> + } while (ktime_us_delta(ktime_get(), start) < timeout_us);
>>
>> if (gdsc_check_status(sc, status))
>> return 0;
>> @@ -172,7 +176,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>> udelay(1);
>> }
>>
>> - ret = gdsc_poll_status(sc, status);
>> + ret = gdsc_poll_status(sc, status, TIMEOUT_US, 0);
>> WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>>
>> if (!ret && status == GDSC_OFF && sc->rsupply) {
>> @@ -343,7 +347,7 @@ static int _gdsc_disable(struct gdsc *sc)
>> */
>> udelay(1);
>>
>> - ret = gdsc_poll_status(sc, GDSC_ON);
>> + ret = gdsc_poll_status(sc, GDSC_ON, TIMEOUT_US, 0);
>> if (ret)
>> return ret;
>> }
>> @@ -565,3 +569,14 @@ int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
>> +
>> +int gdsc_wait_for_collapse(void *priv)
>> +{
>> + struct gdsc *sc = priv;
>> + int ret;
>> +
>> + ret = gdsc_poll_status(sc, GDSC_OFF, 500000, 5);
> So I presume the GPU driver will put() the GDSC and then issue a reset,
> which will wait up to 5 seconds for the GDSC to be turned off.
Not exactly. GPU driver will put() its GDSC vote and will wait for 500ms
to allow other clients to drop their vote and the cx_gdsc to finally
collapse at hw. There is no hw interface to 'reset' entire GPU
subsystem. We have to pull the plug on gdsc to reset it.
>
> So essentially, this logic is needed because we don't wait for VOTABLE
> GDSCs to be turned off? And we have no way to do the put-with-wait for
> this specific case.
>
> I would like the commit message to capture this reasoning.
Agree. Will post a new patchset once we have consensus on the rest of
the things here.

-Akhil.
>
> Thanks,
> Bjorn
>
>> + WARN(ret, "%s status stuck at 'on'", sc->pd.name);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(gdsc_wait_for_collapse);
>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>> index ad313d7..d484bdb 100644
>> --- a/drivers/clk/qcom/gdsc.h
>> +++ b/drivers/clk/qcom/gdsc.h
>> @@ -12,6 +12,7 @@
>> struct regmap;
>> struct regulator;
>> struct reset_controller_dev;
>> +struct qcom_reset_map;
>>
>> /**
>> * struct gdsc - Globally Distributed Switch Controller
>> @@ -79,6 +80,7 @@ int gdsc_register(struct gdsc_desc *desc, struct reset_controller_dev *,
>> struct regmap *);
>> void gdsc_unregister(struct gdsc_desc *desc);
>> int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain);
>> +int gdsc_wait_for_collapse(void *priv);
>> #else
>> static inline int gdsc_register(struct gdsc_desc *desc,
>> struct reset_controller_dev *rcdev,
>> @@ -88,5 +90,10 @@ static inline int gdsc_register(struct gdsc_desc *desc,
>> }
>>
>> static inline void gdsc_unregister(struct gdsc_desc *desc) {};
>> +
>> +static int gdsc_wait_for_collapse(void *priv)
>> +{
>> + return -ENOSYS;
>> +}
>> #endif /* CONFIG_QCOM_GDSC */
>> #endif /* __QCOM_GDSC_H__ */
>> --
>> 2.7.4
>>