Re: [PATCH v2 04/10] interconnect: qcom: rpm: Add support for specifying channel num

From: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
	linux-arm-msm@vger.kernel.org, andersson@kernel.org,
	agross@kernel.org, krzysztof.kozlowski@linaro.org
Cc: marijn.suijten@somainline.org, Georgi Djakov <djakov@kernel.org>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 04/10] interconnect: qcom: rpm: Add support for specifying channel num
Date: Wed, 11 Jan 2023 00:55:47 +0100	[thread overview]
Message-ID: <bdff84b5-0531-909e-43ed-5cc1eda64f17@linaro.org> (raw)
In-Reply-To: <4a30931b-ef94-df2f-2e89-1028bf9510ce@linaro.org>

On 11.01.2023 00:44, Bryan O'Donoghue wrote:
> On 10/01/2023 13:21, Konrad Dybcio wrote:
>> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
>> one channel. This should be taken into account in bandwidth calcualtion,
> calculation
> 
>> as we're supposed to feed msmbus with the per-channel bandwidth. Add
>> support for specifying that and use it during bandwidth aggregation.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>>   drivers/interconnect/qcom/icc-rpm.h | 2 ++
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index 0516b74abdc7..3207b4c99d04 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -336,6 +336,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>   {
>>       struct icc_node *node;
>>       struct qcom_icc_node *qn;
>> +    u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>       int i;
>>         /* Initialise aggregate values */
>> @@ -353,7 +354,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>       list_for_each_entry(node, &provider->nodes, node_list) {
>>           qn = node->data;
>>           for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>> -            agg_avg[i] += qn->sum_avg[i];
>> +            if (qn->channels)
> 
> when do you actually populate channels ?
> 
> I had a quick scan of your series, I didn't see it..
I use this field in the upcoming MSM8998 and SM6375 drivers,
which both require some part of this series to be merged.

If I'm not mistaken, this is essentially what downstream
calls qcom,agg-ports. 8996 should also use it, but I think
I'll add that in a separate series.

Other SoCs that I can see have a non-1 value here in various
downstream trees I have on my PC that don't necessarily have
interconnect drivers at the moment:

msm8976
sdm660
mdm9607
msm8953/sdm429
qcs405
msm8952

and a whole bunch of RPMh SoCs that already take care of this.

Konrad

> 
>> +                sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
>> +            else
>> +                sum_avg[i] = qn->sum_avg[i];
>> +            agg_avg[i] += sum_avg[i];
>>               agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
>>           }
>>       }
>> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
>> index 3762648f9d47..eb51680f890d 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.h
>> +++ b/drivers/interconnect/qcom/icc-rpm.h
>> @@ -66,6 +66,7 @@ struct qcom_icc_qos {
>>    * @id: a unique node identifier
>>    * @links: an array of nodes where we can go next while traversing
>>    * @num_links: the total number of @links
>> + * @channels: number of channels at this node (e.g. DDR channels)
>>    * @buswidth: width of the interconnect between a node and the bus (bytes)
>>    * @sum_avg: current sum aggregate value of all avg bw requests
>>    * @max_peak: current max aggregate value of all peak bw requests
>> @@ -78,6 +79,7 @@ struct qcom_icc_node {
>>       u16 id;
>>       const u16 *links;
>>       u16 num_links;
>> +    u16 channels;
>>       u16 buswidth;
>>       u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>       u64 max_peak[QCOM_ICC_NUM_BUCKETS];
> 