Re: [PATCH v4 17/22] interconnect: qcom: icc-rpm: Control bus rpmcc from icc

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 13.06.2023 22:36, Stephan Gerhold wrote:
> On Tue, Jun 13, 2023 at 04:03:17PM +0200, Konrad Dybcio wrote:
>> The sole purpose of bus clocks that were previously registered with
>> rpmcc was to convey the aggregated bandwidth to RPM. There's no good
>> reason to keep them outside the interconnect framework, as it only
>> adds to the plentiful complexity.
>>
>> Add the required code to handle these clocks from within SMD RPM ICC.
>>
>> RPM-owned bus clocks are no longer considered a thing, but sadly we
>> have to allow for the existence of HLOS-owned bus clocks, as some
>> (mostly older) SoCs (ab)use these for bus scaling (e.g. MSM8998 and
>> &mmcc AHB_CLK_SRC).
>>
>> This in turn is trivially solved with a single *clk, which is filled
>> and used iff qp.bus_clk_desc is absent and we have a "bus" clock-names
>> entry in the DT node.
>>
>> This change should(tm) be fully compatible with all sorts of old
>> Device Trees as far as the interconnect functionality goes (modulo
>> abusing bus clock handles or wrongly using the qcom,icc.h binding,
>> but that's a mistake in and of itself).
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/interconnect/qcom/icc-rpm.c | 119 ++++++++++++++++++++----------------
>>  drivers/interconnect/qcom/icc-rpm.h |  13 ++--
>>  drivers/interconnect/qcom/msm8996.c |   1 -
>>  drivers/interconnect/qcom/sdm660.c  |   1 -
>>  4 files changed, 71 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index b8ecf9538ab9..f9d0ecba5631 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -49,7 +49,7 @@
>>  #define NOC_QOS_MODE_FIXED_VAL		0x0
>>  #define NOC_QOS_MODE_BYPASS_VAL		0x2
>>  
>> -#define ICC_BUS_CLK_MIN_RATE		19200000ULL
>> +#define ICC_BUS_CLK_MIN_RATE		19200ULL /* kHz */
>>  
>>  static int qcom_icc_set_qnoc_qos(struct icc_node *src)
>>  {
>> @@ -338,11 +338,10 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>>  	struct qcom_icc_node *src_qn = NULL, *dst_qn = NULL;
>>  	struct icc_provider *provider;
>>  	u64 sum_bw;
>> -	u64 rate;
>> +	u64 active_rate, sleep_rate;
>>  	u64 agg_avg[QCOM_ICC_NUM_BUCKETS], agg_peak[QCOM_ICC_NUM_BUCKETS];
>>  	u64 max_agg_avg;
>> -	int ret, i;
>> -	int bucket;
>> +	int ret;
>>  
>>  	src_qn = src->data;
>>  	if (dst)
>> @@ -364,49 +363,59 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>>  			return ret;
>>  	}
>>  
>> -	for (i = 0; i < qp->num_bus_clks; i++) {
>> -		/*
>> -		 * Use WAKE bucket for active clock, otherwise, use SLEEP bucket
>> -		 * for other clocks.  If a platform doesn't set interconnect
>> -		 * path tags, by default use sleep bucket for all clocks.
>> -		 *
>> -		 * Note, AMC bucket is not supported yet.
>> -		 */
>> -		if (!strcmp(qp->bus_clks[i].id, "bus_a"))
>> -			bucket = QCOM_ICC_BUCKET_WAKE;
>> -		else
>> -			bucket = QCOM_ICC_BUCKET_SLEEP;
>> -
>> -		rate = icc_units_to_bps(max(agg_avg[bucket], agg_peak[bucket]));
>> -		do_div(rate, src_qn->buswidth);
>> -		rate = min_t(u64, rate, LONG_MAX);
>> -
>> -		/*
>> -		 * Downstream checks whether the requested rate is zero, but it makes little sense
>> -		 * to vote for a value that's below the lower threshold, so let's not do so.
>> -		 */
>> -		if (bucket == QCOM_ICC_BUCKET_WAKE && qp->keep_alive)
>> -			rate = max(ICC_BUS_CLK_MIN_RATE, rate);
>> -
>> -		if (qp->bus_clk_rate[i] == rate)
>> -			continue;
>> -
>> -		ret = clk_set_rate(qp->bus_clks[i].clk, rate);
>> -		if (ret) {
>> -			pr_err("%s clk_set_rate error: %d\n",
>> -			       qp->bus_clks[i].id, ret);
>> +	/* Some providers don't have a bus clock to scale */
>> +	if (!qp->bus_clk_desc && !qp->bus_clk)
>> +		return 0;
>> +
>> +	/* Intentionally keep the rates in kHz as that's what RPM accepts */
>> +	active_rate = max(agg_avg[QCOM_SMD_RPM_ACTIVE_STATE],
>> +			  agg_peak[QCOM_SMD_RPM_ACTIVE_STATE]);
>> +	do_div(active_rate, src_qn->buswidth);
>> +
>> +	sleep_rate = max(agg_avg[QCOM_SMD_RPM_SLEEP_STATE],
>> +			 agg_peak[QCOM_SMD_RPM_SLEEP_STATE]);
>> +	do_div(sleep_rate, src_qn->buswidth);
>> +
>> +	/*
>> +	 * Downstream checks whether the requested rate is zero, but it makes little sense
>> +	 * to vote for a value that's below the lower threshold, so let's not do so.
>> +	 */
>> +	if (qp->keep_alive)
>> +		active_rate = max(ICC_BUS_CLK_MIN_RATE, active_rate);
>> +
>> +	/* Some providers have a non-RPM-owned bus clock - convert kHz->Hz for the CCF */
>> +	if (qp->bus_clk) {
>> +		active_rate = max_t(u64, active_rate, sleep_rate);
>> +		/* ARM32 caps clk_set_rate arg to u32.. Nothing we can do about that! */
>> +		active_rate = min_t(u64, 1000ULL * active_rate, ULONG_MAX);
>> +		return clk_set_rate(qp->bus_clk, active_rate);
>> +	}
>> +
>> +	/* RPM only accepts <=INT_MAX rates */
>> +	active_rate = min_t(u32, active_rate, INT_MAX);
>> +	sleep_rate = min_t(u32, sleep_rate, INT_MAX);
> 
> Realized this by coincidence while playing with the code changes for the
> comment below: This doesn't work as intended similar to the ARM32 cap
> above but it's broken even on ARM64:
> 
> Take this example:
> 
> 	u64 active_rate = 4294967296ULL;
> 	active_rate = min_t(u32, active_rate, INT_MAX);
> 
> This should result into active_rate = INT_MAX.
> But it actually results in rate = 0.
> 
> Why?
> 
> 	min_t(u32, rate, INT_MAX)
> 	= min((u32)rate, (u32)INT_MAX)
> 	= min((u32)4294967296ULL, (u32)INT_MAX)
> 	= min(0, INT_MAX)
> 	= 0
> 
> This needs to be min_t(u64 to work properly :)
Yikes, right!

> 
>> +
>> +	if (active_rate != qp->bus_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE]) {
>> +		ret = qcom_icc_rpm_set_bus_rate(qp->bus_clk_desc, active_rate, true);
>> +		if (ret)
>>  			return ret;
>> -		}
>> -		qp->bus_clk_rate[i] = rate;
>> +
>> +		/* Cache the rate after we've successfully commited it to RPM */
>> +		qp->bus_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE] = active_rate;
>> +	}
>> +
>> +	if (sleep_rate != qp->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE]) {
>> +		ret = qcom_icc_rpm_set_bus_rate(qp->bus_clk_desc, sleep_rate, false);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Cache the rate after we've successfully commited it to RPM */
>> +		qp->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE] = sleep_rate;
>>  	}
> 
> With my suggestion on the other patch to pass in the ctx/state number
> directly into qcom_icc_rpm_set_bus_rate() I wonder if it would be nicer
> to write as loop similar to the all others.
> 
> It's a bit shorter, although the line wrapping for the keep_alive is
> also more ugly. I leave it up to you :)
> 
> Not even compile tested this time!
I think it's more obvious when it's explicit for just two contexts.

Konrad
> 
> Thanks,
> Stephan
> 
> 	/*
> 	 * Downstream checks whether the requested rate is zero, but it makes little sense
> 	 * to vote for a value that's below the lower threshold, so let's not do so.
> 	 */
> 	if (qp->keep_alive)
> 		agg_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE]
> 			= max(ICC_BUS_CLK_MIN_RATE, agg_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE]);
> 
> 	/* Some providers have a non-RPM-owned bus clock - convert kHz->Hz for the CCF */
> 	if (qp->bus_clk) {
> 		rate = max_t(u64, agg_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE],
> 			     agg_clk_rate[QCOM_SMD_RPM_SLEEP_STATE]);
> 		/* ARM32 caps clk_set_rate arg to u32.. Nothing we can do about that! */
> 		reate = min_t(u64, 1000ULL * rate, ULONG_MAX);
> 		return clk_set_rate(qp->bus_clk, rate);
> 	}
> 
> 	for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) {
> 		/* RPM only accepts <=INT_MAX rates */
> 		rate = min_t(u64, active_rate, INT_MAX);
> 		if (rate == qp->bus_clk_rate[i])
> 			continue;
> 
> 		ret = qcom_icc_rpm_set_bus_rate(qp->bus_clk_desc, rate, i);
> 		if (ret)
> 			return ret;
> 
> 		/* Cache the rate after we've successfully commited it to RPM */
> 		qp->bus_clk_rate[i] = rate;
> 	}