diff mbox series

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

Message ID 20220831104741.v6.3.I162c4be55f230cd439f0643f1624527bdc8a9831@changeid (mailing list archive)
State Awaiting Upstream, archived
Headers show
Series clk/qcom: Support gdsc collapse polling using 'reset' interface | expand

Commit Message

Akhil P Oommen Aug. 31, 2022, 5:18 a.m. UTC 
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(-)

Comments

Philipp Zabel Sept. 1, 2022, 10:28 a.m. UTC | #1 
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
Akhil P Oommen Sept. 1, 2022, 8:20 p.m. UTC | #2 
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
diff mbox series

Patch

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__ */