| Message ID | 20230110132202.956619-5-konrad.dybcio@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/10] interconnect: qcom: rpm: Don't set QoS params before non-zero bw is requested | expand |
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.. > + 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];
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]; >
On 10/01/2023 23:55, Konrad Dybcio wrote: > I use this field in the upcoming MSM8998 and SM6375 drivers, > which both require some part of this series to be merged. That's fine so. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
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) + 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];
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, 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(-)