| Message ID | 20230526-topic-smd_icc-v3-20-5fb7d39b874f@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restructure RPM SMD ICC | expand |
On Mon, Jun 12, 2023 at 08:24:37PM +0200, Konrad Dybcio wrote: > SMD RPM only provides two buckets, one each for the active-only and > active-sleep RPM contexts. Use the correct constant to allocate and > operate on them. > > This will make the qcom,icc.h header no longer work with this driver, > mostly because.. it was never meant to! The commit that introduced > bucket support to SMD RPM was trying to shove a square into a round > hole and it did not work out very well. That said, there are no > active users of SMD RPM ICC + qcom,icc.h, so that doesn't hurt. > > Fixes: dcbce7b0a79c ("interconnect: qcom: icc-rpm: Support multiple buckets") > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/interconnect/qcom/icc-rpm.c | 14 +++++++------- > drivers/interconnect/qcom/icc-rpm.h | 4 ++-- > 2 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c > index 5ffcf5ca8914..54a9999fe55d 100644 > --- a/drivers/interconnect/qcom/icc-rpm.c > +++ b/drivers/interconnect/qcom/icc-rpm.c > @@ -249,7 +249,7 @@ static void qcom_icc_pre_bw_aggregate(struct icc_node *node) > size_t i; > > qn = node->data; > - for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) { > + for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) { > qn->sum_avg[i] = 0; > qn->max_peak[i] = 0; > } > @@ -275,7 +275,7 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw, > if (!tag) > tag = QCOM_ICC_TAG_ALWAYS; We should replace this with the RPM variant. Also, can you check which header/file still includes qcom,icc.h? Don't think we should have it included at all for RPM so that referencing the wrong things cannot happen. Thanks, Stephan
On 12.06.2023 22:57, Stephan Gerhold wrote: > On Mon, Jun 12, 2023 at 08:24:37PM +0200, Konrad Dybcio wrote: >> SMD RPM only provides two buckets, one each for the active-only and >> active-sleep RPM contexts. Use the correct constant to allocate and >> operate on them. >> >> This will make the qcom,icc.h header no longer work with this driver, >> mostly because.. it was never meant to! The commit that introduced >> bucket support to SMD RPM was trying to shove a square into a round >> hole and it did not work out very well. That said, there are no >> active users of SMD RPM ICC + qcom,icc.h, so that doesn't hurt. >> >> Fixes: dcbce7b0a79c ("interconnect: qcom: icc-rpm: Support multiple buckets") >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/interconnect/qcom/icc-rpm.c | 14 +++++++------- >> drivers/interconnect/qcom/icc-rpm.h | 4 ++-- >> 2 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c >> index 5ffcf5ca8914..54a9999fe55d 100644 >> --- a/drivers/interconnect/qcom/icc-rpm.c >> +++ b/drivers/interconnect/qcom/icc-rpm.c >> @@ -249,7 +249,7 @@ static void qcom_icc_pre_bw_aggregate(struct icc_node *node) >> size_t i; >> >> qn = node->data; >> - for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) { >> + for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) { >> qn->sum_avg[i] = 0; >> qn->max_peak[i] = 0; >> } >> @@ -275,7 +275,7 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw, >> if (!tag) >> tag = QCOM_ICC_TAG_ALWAYS; > > We should replace this with the RPM variant. Also, can you check which > header/file still includes qcom,icc.h? Don't think we should have it > included at all for RPM so that referencing the wrong things cannot > happen. Nice catch Konrad > > Thanks, > Stephan
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c index 5ffcf5ca8914..54a9999fe55d 100644 --- a/drivers/interconnect/qcom/icc-rpm.c +++ b/drivers/interconnect/qcom/icc-rpm.c @@ -249,7 +249,7 @@ static void qcom_icc_pre_bw_aggregate(struct icc_node *node) size_t i; qn = node->data; - for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) { + for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) { qn->sum_avg[i] = 0; qn->max_peak[i] = 0; } @@ -275,7 +275,7 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw, if (!tag) tag = QCOM_ICC_TAG_ALWAYS; - for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) { + for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) { if (tag & BIT(i)) { qn->sum_avg[i] += avg_bw; qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw); @@ -300,11 +300,11 @@ 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]; + u64 sum_avg[QCOM_SMD_RPM_STATE_NUM]; int i; /* Initialise aggregate values */ - for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) { + for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) { agg_avg[i] = 0; agg_peak[i] = 0; } @@ -317,7 +317,7 @@ 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++) { + for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) { if (qn->channels) sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels); else @@ -328,7 +328,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider, } /* Find maximum values across all buckets */ - for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) + for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) *max_agg_avg = max_t(u64, *max_agg_avg, agg_avg[i]); } @@ -339,7 +339,7 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst) struct icc_provider *provider; u64 sum_bw; u64 active_rate, sleep_rate; - u64 agg_avg[QCOM_ICC_NUM_BUCKETS], agg_peak[QCOM_ICC_NUM_BUCKETS]; + u64 agg_avg[QCOM_SMD_RPM_STATE_NUM], agg_peak[QCOM_SMD_RPM_STATE_NUM]; u64 max_agg_avg; int ret; diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h index 11e7d503e4d0..ccc5541605ac 100644 --- a/drivers/interconnect/qcom/icc-rpm.h +++ b/drivers/interconnect/qcom/icc-rpm.h @@ -105,8 +105,8 @@ struct qcom_icc_node { u16 num_links; u16 channels; u16 buswidth; - u64 sum_avg[QCOM_ICC_NUM_BUCKETS]; - u64 max_peak[QCOM_ICC_NUM_BUCKETS]; + u64 sum_avg[QCOM_SMD_RPM_STATE_NUM]; + u64 max_peak[QCOM_SMD_RPM_STATE_NUM]; int mas_rpm_id; int slv_rpm_id; struct qcom_icc_qos qos;
SMD RPM only provides two buckets, one each for the active-only and active-sleep RPM contexts. Use the correct constant to allocate and operate on them. This will make the qcom,icc.h header no longer work with this driver, mostly because.. it was never meant to! The commit that introduced bucket support to SMD RPM was trying to shove a square into a round hole and it did not work out very well. That said, there are no active users of SMD RPM ICC + qcom,icc.h, so that doesn't hurt. Fixes: dcbce7b0a79c ("interconnect: qcom: icc-rpm: Support multiple buckets") Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/interconnect/qcom/icc-rpm.c | 14 +++++++------- drivers/interconnect/qcom/icc-rpm.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-)