diff mbox series

[v2,20/22] interconnect: qcom: icc-rpm: Set bandwidth on both contexts

Message ID 20230526-topic-smd_icc-v2-20-e5934b07d813@linaro.org
State New
Headers
Series Restructure RPM SMD ICC |

Commit Message

Konrad Dybcio June 9, 2023, 8:19 p.m. UTC 
  Up until now, for some reason we've only been setting bandwidth values
on the active-only context. That pretty much meant that RPM could lift
all votes when entering sleep mode. Or never sleep at all.

That in turn could potentially break things like USB wakeup, as the
connection between APSS and SNoC/PNoC would simply be dead.

Set the values appropriately.

Fixes: 30c8fa3ec61a ("interconnect: qcom: Add MSM8916 interconnect provider driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 54 +++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

Comments

Stephan Gerhold June 10, 2023, 6 p.m. UTC | #1 
On Fri, Jun 09, 2023 at 10:19:25PM +0200, Konrad Dybcio wrote:
> Up until now, for some reason we've only been setting bandwidth values
> on the active-only context. That pretty much meant that RPM could lift
> all votes when entering sleep mode. Or never sleep at all.
> 
> That in turn could potentially break things like USB wakeup, as the
> connection between APSS and SNoC/PNoC would simply be dead.
> 

Nitpick: Apparently an "active" vote is applied during both active+sleep
until the first "sleep" vote is sent. It's documented only for
regulators [1] but I would expect the same applies to the bandwidths.
This means actual breakage shouldn't have been possible.

The patch itself is still the right thing to do to have the sleep state
correct during deep cpuidle/suspend.

[1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/blob/LA.BR.1.2.9.c26-04700-8x09.0/drivers/regulator/rpm-smd-regulator.c#L199-209

> Set the values appropriately.
> 
> Fixes: 30c8fa3ec61a ("interconnect: qcom: Add MSM8916 interconnect provider driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 54 +++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 3ac47b818afe..ac719013077e 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -205,34 +205,39 @@ static int qcom_icc_qos_set(struct icc_node *node)
>  	}
>  }
>  
> -static int qcom_icc_rpm_set(struct qcom_icc_node *qn, u64 sum_bw)
> +static int qcom_icc_rpm_set(struct qcom_icc_node *qn, u64 *bw)
>  {
> -	int ret = 0;
> +	int ret, rpm_ctx = 0;
> +	u64 bw_bps;
>  
>  	if (qn->qos.ap_owned)
>  		return 0;
>  
> -	if (qn->mas_rpm_id != -1) {
> -		ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
> -					    RPM_BUS_MASTER_REQ,
> -					    qn->mas_rpm_id,
> -					    sum_bw);
> -		if (ret) {
> -			pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
> -			       qn->mas_rpm_id, ret);
> -			return ret;
> +	for (rpm_ctx = 0; rpm_ctx < QCOM_SMD_RPM_STATE_NUM; rpm_ctx++) {
> +		bw_bps = icc_units_to_bps(bw[rpm_ctx]);
> +
> +		if (qn->mas_rpm_id != -1) {
> +			ret = qcom_icc_rpm_smd_send(rpm_ctx,
> +						    RPM_BUS_MASTER_REQ,
> +						    qn->mas_rpm_id,
> +						    bw_bps);
> +			if (ret) {
> +				pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
> +				qn->mas_rpm_id, ret);
> +				return ret;
> +			}
>  		}
> -	}
>  
> -	if (qn->slv_rpm_id != -1) {
> -		ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
> -					    RPM_BUS_SLAVE_REQ,
> -					    qn->slv_rpm_id,
> -					    sum_bw);
> -		if (ret) {
> -			pr_err("qcom_icc_rpm_smd_send slv %d error %d\n",
> -			       qn->slv_rpm_id, ret);
> -			return ret;
> +		if (qn->slv_rpm_id != -1) {
> +			ret = qcom_icc_rpm_smd_send(rpm_ctx,
> +						    RPM_BUS_SLAVE_REQ,
> +						    qn->slv_rpm_id,
> +						    bw_bps);
> +			if (ret) {
> +				pr_err("qcom_icc_rpm_smd_send slv %d error %d\n",
> +				qn->slv_rpm_id, ret);
> +				return ret;
> +			}
>  		}
>  	}
>  
> @@ -337,7 +342,6 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>  	struct qcom_icc_provider *qp;
>  	struct qcom_icc_node *src_qn = NULL, *dst_qn = NULL;
>  	struct icc_provider *provider;
> -	u64 sum_bw;
>  	u64 active_rate, sleep_rate;
>  	u64 agg_avg[QCOM_SMD_RPM_STATE_NUM], agg_peak[QCOM_SMD_RPM_STATE_NUM];
>  	u64 max_agg_avg;
> @@ -351,14 +355,12 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>  
>  	qcom_icc_bus_aggregate(provider, agg_avg, agg_peak, &max_agg_avg);
>  
> -	sum_bw = icc_units_to_bps(max_agg_avg);
> -
> -	ret = qcom_icc_rpm_set(src_qn, sum_bw);
> +	ret = qcom_icc_rpm_set(src_qn, agg_avg);
>  	if (ret)
>  		return ret;
>  
>  	if (dst_qn) {
> -		ret = qcom_icc_rpm_set(dst_qn, sum_bw);
> +		ret = qcom_icc_rpm_set(dst_qn, agg_avg);
>  		if (ret)
>  			return ret;
>  	}
> 
> -- 
> 2.41.0
>
Konrad Dybcio June 10, 2023, 6:28 p.m. UTC | #2 
On 10.06.2023 20:00, Stephan Gerhold wrote:
> On Fri, Jun 09, 2023 at 10:19:25PM +0200, Konrad Dybcio wrote:
>> Up until now, for some reason we've only been setting bandwidth values
>> on the active-only context. That pretty much meant that RPM could lift
>> all votes when entering sleep mode. Or never sleep at all.
>>
>> That in turn could potentially break things like USB wakeup, as the
>> connection between APSS and SNoC/PNoC would simply be dead.
>>
> 
> Nitpick: Apparently an "active" vote is applied during both active+sleep
> until the first "sleep" vote is sent. It's documented only for
> regulators [1] but I would expect the same applies to the bandwidths.
> This means actual breakage shouldn't have been possible.
..unless some part of the boot chain voted for the sleep set!

I'm not sure whether the regulator comment also holds for bw, but I
also don't really have a great way to check it.. Would you want me to
alter this commit message somehow?

Konrad
> 
> The patch itself is still the right thing to do to have the sleep state
> correct during deep cpuidle/suspend.
> 
> [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/blob/LA.BR.1.2.9.c26-04700-8x09.0/drivers/regulator/rpm-smd-regulator.c#L199-209
> 
>> Set the values appropriately.
>>
>> Fixes: 30c8fa3ec61a ("interconnect: qcom: Add MSM8916 interconnect provider driver")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/interconnect/qcom/icc-rpm.c | 54 +++++++++++++++++++------------------
>>  1 file changed, 28 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index 3ac47b818afe..ac719013077e 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -205,34 +205,39 @@ static int qcom_icc_qos_set(struct icc_node *node)
>>  	}
>>  }
>>  
>> -static int qcom_icc_rpm_set(struct qcom_icc_node *qn, u64 sum_bw)
>> +static int qcom_icc_rpm_set(struct qcom_icc_node *qn, u64 *bw)
>>  {
>> -	int ret = 0;
>> +	int ret, rpm_ctx = 0;
>> +	u64 bw_bps;
>>  
>>  	if (qn->qos.ap_owned)
>>  		return 0;
>>  
>> -	if (qn->mas_rpm_id != -1) {
>> -		ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
>> -					    RPM_BUS_MASTER_REQ,
>> -					    qn->mas_rpm_id,
>> -					    sum_bw);
>> -		if (ret) {
>> -			pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
>> -			       qn->mas_rpm_id, ret);
>> -			return ret;
>> +	for (rpm_ctx = 0; rpm_ctx < QCOM_SMD_RPM_STATE_NUM; rpm_ctx++) {
>> +		bw_bps = icc_units_to_bps(bw[rpm_ctx]);
>> +
>> +		if (qn->mas_rpm_id != -1) {
>> +			ret = qcom_icc_rpm_smd_send(rpm_ctx,
>> +						    RPM_BUS_MASTER_REQ,
>> +						    qn->mas_rpm_id,
>> +						    bw_bps);
>> +			if (ret) {
>> +				pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
>> +				qn->mas_rpm_id, ret);
>> +				return ret;
>> +			}
>>  		}
>> -	}
>>  
>> -	if (qn->slv_rpm_id != -1) {
>> -		ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
>> -					    RPM_BUS_SLAVE_REQ,
>> -					    qn->slv_rpm_id,
>> -					    sum_bw);
>> -		if (ret) {
>> -			pr_err("qcom_icc_rpm_smd_send slv %d error %d\n",
>> -			       qn->slv_rpm_id, ret);
>> -			return ret;
>> +		if (qn->slv_rpm_id != -1) {
>> +			ret = qcom_icc_rpm_smd_send(rpm_ctx,
>> +						    RPM_BUS_SLAVE_REQ,
>> +						    qn->slv_rpm_id,
>> +						    bw_bps);
>> +			if (ret) {
>> +				pr_err("qcom_icc_rpm_smd_send slv %d error %d\n",
>> +				qn->slv_rpm_id, ret);
>> +				return ret;
>> +			}
>>  		}
>>  	}
>>  
>> @@ -337,7 +342,6 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>>  	struct qcom_icc_provider *qp;
>>  	struct qcom_icc_node *src_qn = NULL, *dst_qn = NULL;
>>  	struct icc_provider *provider;
>> -	u64 sum_bw;
>>  	u64 active_rate, sleep_rate;
>>  	u64 agg_avg[QCOM_SMD_RPM_STATE_NUM], agg_peak[QCOM_SMD_RPM_STATE_NUM];
>>  	u64 max_agg_avg;
>> @@ -351,14 +355,12 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>>  
>>  	qcom_icc_bus_aggregate(provider, agg_avg, agg_peak, &max_agg_avg);
>>  
>> -	sum_bw = icc_units_to_bps(max_agg_avg);
>> -
>> -	ret = qcom_icc_rpm_set(src_qn, sum_bw);
>> +	ret = qcom_icc_rpm_set(src_qn, agg_avg);
>>  	if (ret)
>>  		return ret;
>>  
>>  	if (dst_qn) {
>> -		ret = qcom_icc_rpm_set(dst_qn, sum_bw);
>> +		ret = qcom_icc_rpm_set(dst_qn, agg_avg);
>>  		if (ret)
>>  			return ret;
>>  	}
>>
>> -- 
>> 2.41.0
>>
Stephan Gerhold June 10, 2023, 6:43 p.m. UTC | #3 
On Sat, Jun 10, 2023 at 08:28:22PM +0200, Konrad Dybcio wrote:
> 
> 
> On 10.06.2023 20:00, Stephan Gerhold wrote:
> > On Fri, Jun 09, 2023 at 10:19:25PM +0200, Konrad Dybcio wrote:
> >> Up until now, for some reason we've only been setting bandwidth values
> >> on the active-only context. That pretty much meant that RPM could lift
> >> all votes when entering sleep mode. Or never sleep at all.
> >>
> >> That in turn could potentially break things like USB wakeup, as the
> >> connection between APSS and SNoC/PNoC would simply be dead.
> >>
> > 
> > Nitpick: Apparently an "active" vote is applied during both active+sleep
> > until the first "sleep" vote is sent. It's documented only for
> > regulators [1] but I would expect the same applies to the bandwidths.
> > This means actual breakage shouldn't have been possible.
> ..unless some part of the boot chain voted for the sleep set!
> 
> I'm not sure whether the regulator comment also holds for bw, but I
> also don't really have a great way to check it.. Would you want me to
> alter this commit message somehow?
> 

Hm. Well, on a second look you used "could" instead of "definitely does"
everywhere in your commit message. There is a indeed a slight chance
so feel free to just keep it as-is. :D

Thanks,
Stephan
diff mbox series

Patch

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 3ac47b818afe..ac719013077e 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -205,34 +205,39 @@  static int qcom_icc_qos_set(struct icc_node *node)
 	}
 }
 
-static int qcom_icc_rpm_set(struct qcom_icc_node *qn, u64 sum_bw)
+static int qcom_icc_rpm_set(struct qcom_icc_node *qn, u64 *bw)
 {
-	int ret = 0;
+	int ret, rpm_ctx = 0;
+	u64 bw_bps;
 
 	if (qn->qos.ap_owned)
 		return 0;
 
-	if (qn->mas_rpm_id != -1) {
-		ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
-					    RPM_BUS_MASTER_REQ,
-					    qn->mas_rpm_id,
-					    sum_bw);
-		if (ret) {
-			pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
-			       qn->mas_rpm_id, ret);
-			return ret;
+	for (rpm_ctx = 0; rpm_ctx < QCOM_SMD_RPM_STATE_NUM; rpm_ctx++) {
+		bw_bps = icc_units_to_bps(bw[rpm_ctx]);
+
+		if (qn->mas_rpm_id != -1) {
+			ret = qcom_icc_rpm_smd_send(rpm_ctx,
+						    RPM_BUS_MASTER_REQ,
+						    qn->mas_rpm_id,
+						    bw_bps);
+			if (ret) {
+				pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
+				qn->mas_rpm_id, ret);
+				return ret;
+			}
 		}
-	}
 
-	if (qn->slv_rpm_id != -1) {
-		ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
-					    RPM_BUS_SLAVE_REQ,
-					    qn->slv_rpm_id,
-					    sum_bw);
-		if (ret) {
-			pr_err("qcom_icc_rpm_smd_send slv %d error %d\n",
-			       qn->slv_rpm_id, ret);
-			return ret;
+		if (qn->slv_rpm_id != -1) {
+			ret = qcom_icc_rpm_smd_send(rpm_ctx,
+						    RPM_BUS_SLAVE_REQ,
+						    qn->slv_rpm_id,
+						    bw_bps);
+			if (ret) {
+				pr_err("qcom_icc_rpm_smd_send slv %d error %d\n",
+				qn->slv_rpm_id, ret);
+				return ret;
+			}
 		}
 	}
 
@@ -337,7 +342,6 @@  static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 	struct qcom_icc_provider *qp;
 	struct qcom_icc_node *src_qn = NULL, *dst_qn = NULL;
 	struct icc_provider *provider;
-	u64 sum_bw;
 	u64 active_rate, sleep_rate;
 	u64 agg_avg[QCOM_SMD_RPM_STATE_NUM], agg_peak[QCOM_SMD_RPM_STATE_NUM];
 	u64 max_agg_avg;
@@ -351,14 +355,12 @@  static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 
 	qcom_icc_bus_aggregate(provider, agg_avg, agg_peak, &max_agg_avg);
 
-	sum_bw = icc_units_to_bps(max_agg_avg);
-
-	ret = qcom_icc_rpm_set(src_qn, sum_bw);
+	ret = qcom_icc_rpm_set(src_qn, agg_avg);
 	if (ret)
 		return ret;
 
 	if (dst_qn) {
-		ret = qcom_icc_rpm_set(dst_qn, sum_bw);
+		ret = qcom_icc_rpm_set(dst_qn, agg_avg);
 		if (ret)
 			return ret;
 	}