[PATCH v6 0/6] clk/qcom: Support gdsc collapse polling using 'reset' interface

[PATCH v6 0/6] clk/qcom: Support gdsc collapse polling using 'reset' interface

[Akhil P Oommen]

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 v6:
- No code changes in this version. Just captured the Acked-by tags

Changes in v5:
- Nit: Remove a duplicate blank line (Krzysztof)

Changes in v4:
- Update gpu dt-binding schema
- Typo fix in commit text

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 (6):
  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
  dt-bindings: drm/msm/gpu: Add optional resets
  arm64: dts: qcom: sc7280: Add Reset support for gpu

 .../devicetree/bindings/display/msm/gpu.yaml       |  6 +++++
 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 +++
 8 files changed, 83 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v6 1/6] dt-bindings: clk: qcom: Support gpu cx gdsc reset

[Akhil P Oommen]

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

Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

(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

[PATCH v6 2/6] clk: qcom: Allow custom reset ops

[Akhil P Oommen]

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 <quic_akhilpo@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

(no changes since v3)

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

[PATCH v6 3/6] clk: qcom: gdsc: Add a reset op to poll gdsc collapse

[Akhil P Oommen]

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

Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

(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

[PATCH v6 4/6] clk: qcom: gpucc-sc7280: Add cx collapse reset support

[Akhil P Oommen]

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

Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

(no changes since v3)

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

[PATCH v6 5/6] dt-bindings: drm/msm/gpu: Add optional resets

[Akhil P Oommen]

Add an optional reference to GPUCC reset which can be used to ensure cx
gdsc collapse during gpu recovery.

Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

(no changes since v5)

Changes in v5:
- Nit: Remove a duplicate blank line (Krzysztof)

Changes in v4:
- New patch in v4

 Documentation/devicetree/bindings/display/msm/gpu.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/gpu.yaml b/Documentation/devicetree/bindings/display/msm/gpu.yaml
index 3397bc3..408ed97 100644
--- a/Documentation/devicetree/bindings/display/msm/gpu.yaml
+++ b/Documentation/devicetree/bindings/display/msm/gpu.yaml
@@ -109,6 +109,12 @@ properties:
       For GMU attached devices a phandle to the GMU device that will
       control the power for the GPU.
 
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: cx_collapse
 
 required:
   - compatible
-- 
2.7.4

[PATCH v6 6/6] arm64: dts: qcom: sc7280: Add Reset support for gpu

[Akhil P Oommen]

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 <quic_akhilpo@quicinc.com>
---

(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

Re: [PATCH v6 2/6] clk: qcom: Allow custom reset ops

[Philipp Zabel]

Hi Akhil,

On Wed, Aug 31, 2022 at 10:48:23AM +0530, Akhil P Oommen wrote:
> 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 <quic_akhilpo@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> 
> (no changes since v3)
> 
> 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;
> +

It doesn't seem necessary to stack reset_ops -> qcom_reset_ops for what
you need here.
Just add an optional const struct reset_ops * to qcom_cc_desc and feed
that into qcom_cc_really_probe to replace &qcom_reset_ops.

[...]
> +struct qcom_reset_ops {
> +	int (*reset)(void *priv);
> +	int (*assert)(void *priv);
> +	int (*deassert)(void *priv);

Why add assert and deassert ops? There doesn't seem to be any user.

> +};
> +
>  struct qcom_reset_map {
>  	unsigned int reg;
>  	u8 bit;
> +	const struct qcom_reset_ops *ops;
> +	void *priv;

Adding per-reset ops + priv counters seems excessive to me.

Are you expecting different reset controls in the same reset controller
to have completely different ops at some point? If so, I would wonder
whether it wouldn't be better to split the reset controller in that
case. Either way, for the GDSC collapse workaround this does not seem
to be required at all.

regards
Philipp

Re: [PATCH v6 3/6] clk: qcom: gdsc: Add a reset op to poll gdsc collapse

[Philipp Zabel]

On Wed, Aug 31, 2022 at 10:48:24AM +0530, Akhil P Oommen wrote:
> Add a reset op compatible function to poll for gdsc collapse.
> 
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> 
> (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);

Could this loop be implemented with read_poll_timeout()?

>  	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);

Superficially, using this as a reset op seems like abuse of the reset
controller API. Calling reset_control_reset() on this in the GPU driver
will not trigger a reset signal on the GPU's "cx_collapse" reset input.

So at the very least, this patchset should contain an explanation why
this is a good idea regardless, and how this is almost a reset control.

I have read the linked discussion, and I'm not sure I understand all
of it, so please correct me if I'm wrong: There is some other way to
force the GDSC into a state that will eventually cause a GPU reset, and
this is just the remaining part to make sure that the workaround dance
is finished?

If so, it should be explained that this depends on something else to
actually indirectly trigger the reset, and where this happens.

regards
Philipp

Re: [PATCH v6 4/6] clk: qcom: gpucc-sc7280: Add cx collapse reset support

[Philipp Zabel]

On Wed, Aug 31, 2022 at 10:48:25AM +0530, Akhil P Oommen wrote:
> Allow a consumer driver to poll for cx gdsc collapse through Reset
> framework.
> 
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> 
> (no changes since v3)
> 
> 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,

This should be accompanied by a comment explaining the not-quite-reset
nature of this workaround, i.e. what is the prerequisite for this to
actually work as expected?

> +};
> +
> +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),

See my comment on patch 2. I think instead of adding a const struct
qcom_reset_ops * to gpucc_sc7280_resets, this should just add a const
struct reset_control * to gpu_cc_sc7280_desc.

regards
Philipp

Re: [PATCH v6 4/6] clk: qcom: gpucc-sc7280: Add cx collapse reset support

[Dmitry Baryshkov]

On 01/09/2022 13:34, Philipp Zabel wrote:
> On Wed, Aug 31, 2022 at 10:48:25AM +0530, Akhil P Oommen wrote:
>> Allow a consumer driver to poll for cx gdsc collapse through Reset
>> framework.
>>
>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>
>> (no changes since v3)
>>
>> 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,
> 
> This should be accompanied by a comment explaining the not-quite-reset
> nature of this workaround, i.e. what is the prerequisite for this to
> actually work as expected?
> 
>> +};
>> +
>> +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),
> 
> See my comment on patch 2. I think instead of adding a const struct
> qcom_reset_ops * to gpucc_sc7280_resets, this should just add a const
> struct reset_control * to gpu_cc_sc7280_desc.

While this will work for the sc7280, the platform that Akhil was 
developing, this will not work for other platforms (like sm8250), where 
the dispcc also provides traditional BCR resets.

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v6 4/6] clk: qcom: gpucc-sc7280: Add cx collapse reset support

[Akhil P Oommen]

On 9/1/2022 4:16 PM, Dmitry Baryshkov wrote:
> On 01/09/2022 13:34, Philipp Zabel wrote:
>> On Wed, Aug 31, 2022 at 10:48:25AM +0530, Akhil P Oommen wrote:
>>> Allow a consumer driver to poll for cx gdsc collapse through Reset
>>> framework.
>>>
>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>
>>> (no changes since v3)
>>>
>>> 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,
>>
>> This should be accompanied by a comment explaining the not-quite-reset
>> nature of this workaround, i.e. what is the prerequisite for this to
>> actually work as expected?
>>
>>> +};
>>> +
>>> +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),
>>
>> See my comment on patch 2. I think instead of adding a const struct
>> qcom_reset_ops * to gpucc_sc7280_resets, this should just add a const
>> struct reset_control * to gpu_cc_sc7280_desc.
>
> While this will work for the sc7280, the platform that Akhil was 
> developing, this will not work for other platforms (like sm8250), 
> where the dispcc also provides traditional BCR resets.
>
Like Dimtry mentioned, we should eventually implement this feature on 
all gpucc drivers and some of them already use the existing reset ops. 
The current implementation creates the least code churn and duplication's.

-Akhil

Re: [Freedreno] [PATCH v6 2/6] clk: qcom: Allow custom reset ops

[Akhil P Oommen]

On 9/1/2022 3:47 PM, Philipp Zabel wrote:
> Hi Akhil,
>
> On Wed, Aug 31, 2022 at 10:48:23AM +0530, Akhil P Oommen wrote:
>> 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 <quic_akhilpo@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>
>> (no changes since v3)
>>
>> 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;
>> +
> It doesn't seem necessary to stack reset_ops -> qcom_reset_ops for what
> you need here.
> Just add an optional const struct reset_ops * to qcom_cc_desc and feed
> that into qcom_cc_really_probe to replace &qcom_reset_ops.
>
> [...]
>> +struct qcom_reset_ops {
>> +	int (*reset)(void *priv);
>> +	int (*assert)(void *priv);
>> +	int (*deassert)(void *priv);
> Why add assert and deassert ops? There doesn't seem to be any user.
I had a more minimal implementation in v1. But this makes it more 
complete and make it less prone to trip up ourselves in future.
>
>> +};
>> +
>>   struct qcom_reset_map {
>>   	unsigned int reg;
>>   	u8 bit;
>> +	const struct qcom_reset_ops *ops;
>> +	void *priv;
> Adding per-reset ops + priv counters seems excessive to me.
>
> Are you expecting different reset controls in the same reset controller
> to have completely different ops at some point? If so, I would wonder
> whether it wouldn't be better to split the reset controller in that
> case. Either way, for the GDSC collapse workaround this does not seem
> to be required at all.
Yes, like I responded in patch 4, we need different ops for the same 
reset controller in some gpucc implementations.
For eg: 
https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/clk/qcom/gpucc-sdm660.c

-Akhil
>
> regards
> Philipp

Re: [PATCH v6 3/6] clk: qcom: gdsc: Add a reset op to poll gdsc collapse

[Akhil P Oommen]

On 9/1/2022 3:58 PM, Philipp Zabel wrote:
> On Wed, Aug 31, 2022 at 10:48:24AM +0530, Akhil P Oommen wrote:
>> Add a reset op compatible function to poll for gdsc collapse.
>>
>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>
>> (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);
> Could this loop be implemented with read_poll_timeout()?
I felt it is not worth the code churn. Currently, we hit this path only 
during GPU recovery which is a rare event.
>
>>   	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);
> Superficially, using this as a reset op seems like abuse of the reset
> controller API. Calling reset_control_reset() on this in the GPU driver
> will not trigger a reset signal on the GPU's "cx_collapse" reset input.
>
> So at the very least, this patchset should contain an explanation why
> this is a good idea regardless, and how this is almost a reset control.
>
> I have read the linked discussion, and I'm not sure I understand all
> of it, so please correct me if I'm wrong: There is some other way to
> force the GDSC into a state that will eventually cause a GPU reset, and
> this is just the remaining part to make sure that the workaround dance
> is finished?
>
> If so, it should be explained that this depends on something else to
> actually indirectly trigger the reset, and where this happens.

Let me clarify a bit. In Qcom gpu subsystem, power collapse is the only 
way to properly reset the cx domain. Power collapse is a bit complex 
here because multiple subsystems/drivers can keep a vote on the 
regulator which blocks power collapse. So we remove the vote from gpu 
driver and poll (with a reasonable timeout obviously) until everyone 
removes their vote and gdsc collapses.

I suppose generally a reset implementation would look like this:

reset {
         Step 1: Trigger a reset pulse
         Step 2: Wait/poll for reset to complete
}

We skip step1 because we don't have a way to force it during gpu reset. 
Instead of that, we just remove the gdsc vote from the gpu driver. So 
all that is left to do here is step 2.

Like you suggested, I think it would be better if we document this in 
patch 4.

Thanks for the review. Please let me know if you have any feedback.

-Akhil.
>
> regards
> Philipp