| Message ID | 20230526-topic-smd_icc-v4-0-5ba82b6fbba2@linaro.org |
|---|---|
| Headers | show |
| Series | Restructure RPM SMD ICC | expand |
On Tue, Jun 13, 2023 at 04:03:19PM +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> Reviewed-by: Stephan Gerhold <stephan@gerhold.net> > --- > drivers/interconnect/qcom/icc-rpm.c | 16 ++++++++-------- > drivers/interconnect/qcom/icc-rpm.h | 6 +++--- > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c > index f9d0ecba5631..01e14b8ced64 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; > } > @@ -273,9 +273,9 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw, > qn = node->data; > > if (!tag) > - tag = QCOM_ICC_TAG_ALWAYS; > + tag = RPM_ALWAYS_TAG; > > - 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 a4588a3aebf3..fe928ab6b878 100644 > --- a/drivers/interconnect/qcom/icc-rpm.h > +++ b/drivers/interconnect/qcom/icc-rpm.h > @@ -10,7 +10,7 @@ > #include <linux/platform_device.h> > #include <linux/soc/qcom/smd-rpm.h> > > -#include <dt-bindings/interconnect/qcom,icc.h> > +#include <dt-bindings/interconnect/qcom,rpm-icc.h> > > #define RPM_BUS_MASTER_REQ 0x73616d62 > #define RPM_BUS_SLAVE_REQ 0x766c7362 > @@ -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; > > -- > 2.41.0 >
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 :) > + > + 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! 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; }
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; > }
This series reshuffles things around, moving the management of SMD RPM bus clocks to the interconnect framework where they belong. This helps us solve a couple of issues: 1. We can work towards unused clk cleanup of RPMCC without worrying about it killing some NoC bus, resulting in the SoC dying. Deasserting actually unused RPM clocks (among other things) will let us achieve "true SoC-wide power collapse states", also known as VDD_LOW and VDD_MIN. 2. We no longer have to keep tons of quirky bus clock ifs in the icc driver. You either have a RPM clock and call "rpm set rate" or you have a single non-RPM clock (like AHB_CLK_SRC) or you don't have any. 3. There's less overhead - instead of going through layers and layers of the CCF, ratesetting comes down to calling max() and sending a single RPM message. ICC is very very dynamic so that's a big plus. The clocks still need to be vaguely described in the clk-smd-rpm driver, as it gives them an initial kickoff, before actually telling RPM to enable DVFS scaling. After RPM receives that command, all clocks that have not been assigned a rate are considered unused and are shut down in hardware, leading to the same issue as described in point 1. We can consider marking them __initconst in the future, but this series is very fat even without that.. Apart from that, it squashes a couple of bugs that really need fixing.. --- MERGING STRATEGY --- If Stephen and Georgi agree, it would be best to take all of this through the qcom tree, as it touches on heavily intertwined components and introduces compile-time dependencies between icc and clk drivers. Tested on SM6375 (OOT), MSM8998 (OOT), MSM8996. MSM8974 conversion to common code and modernization will be handled separately. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Changes in v4: - Only set clk rate on a context if necessary - Mention qcom,icc.h is not the correct header in "Control bus rpmcc form icc" - Fix the bindings (BIT vs 1<<) - Fix one more wrong use of qcom,icc.h in "Fix bucket numer" and uninclude it - Drop "Allow negative QoS offset" (will be handled separately) - Export icc clocks descriptions to unbreak =m builds - Pick up tags - Link to v3: https://lore.kernel.org/r/20230526-topic-smd_icc-v3-0-5fb7d39b874f@linaro.org Changes in v3: - Use devm_clk_get_optional and only get() the clock once - Drop unnecessary NULL-checks for qp->bus_clk - Handle ARM32 CCF limitations, add an explicit comment about them - Use Stephan's alternative rpmcc readiness check - Fix one more wrong usage of QCOM_ICC_NUM_BUCKETS in icc-rpm.h - Introduce new dt-bindings for icc rpm tags - Mention the rpm tags situation in the commit message of "Fix bucket number" - Pick up tags - Link to v2: https://lore.kernel.org/r/20230526-topic-smd_icc-v2-0-e5934b07d813@linaro.org Changes in v2: - Sort entries properly in "Add missing headers in icc-rpm.h" - Fix the check for no clocks on a given provider - Replace "Divide clk rate by src node bus width" with a proper fix - Add "Set correct bandwidth through RPM bw req" - Split "Add QCOM_SMD_RPM_STATE_NUM" into 2 logical changes - Move "Separate out interconnect bus clocks" a bit later in the series - Link to v1: https://lore.kernel.org/r/20230526-topic-smd_icc-v1-0-1bf8e6663c4e@linaro.org --- Konrad Dybcio (21): dt-bindings: interconnect: Add Qcom RPM ICC bindings soc: qcom: smd-rpm: Add QCOM_SMD_RPM_STATE_NUM soc: qcom: smd-rpm: Use tabs for defines clk: qcom: smd-rpm: Move some RPM resources to the common header interconnect: qcom: icc-rpm: Introduce keep_alive interconnect: qcom: Fold smd-rpm.h into icc-rpm.h interconnect: qcom: smd-rpm: Add rpmcc handling skeleton code interconnect: qcom: Add missing headers in icc-rpm.h interconnect: qcom: Define RPM bus clocks interconnect: qcom: sdm660: Hook up RPM bus clk definitions interconnect: qcom: msm8996: Hook up RPM bus clk definitions interconnect: qcom: qcs404: Hook up RPM bus clk definitions interconnect: qcom: msm8939: Hook up RPM bus clk definitions interconnect: qcom: msm8916: Hook up RPM bus clk definitions interconnect: qcom: qcm2290: Hook up RPM bus clk definitions interconnect: qcom: icc-rpm: Control bus rpmcc from icc clk: qcom: smd-rpm: Separate out interconnect bus clocks interconnect: qcom: icc-rpm: Fix bucket number interconnect: qcom: icc-rpm: Set bandwidth on both contexts interconnect: qcom: icc-rpm: Set correct bandwidth through RPM bw req interconnect: qcom: icc-rpm: Fix bandwidth calculations Stephan Gerhold (1): soc: qcom: smd-rpm: Move icc_smd_rpm registration to clk-smd-rpm drivers/clk/qcom/clk-smd-rpm.c | 312 +++++++++++------------- drivers/interconnect/qcom/Makefile | 2 +- drivers/interconnect/qcom/icc-rpm-clocks.c | 77 ++++++ drivers/interconnect/qcom/icc-rpm.c | 217 ++++++++-------- drivers/interconnect/qcom/icc-rpm.h | 55 ++++- drivers/interconnect/qcom/msm8916.c | 4 +- drivers/interconnect/qcom/msm8939.c | 5 +- drivers/interconnect/qcom/msm8974.c | 2 +- drivers/interconnect/qcom/msm8996.c | 9 +- drivers/interconnect/qcom/qcm2290.c | 7 +- drivers/interconnect/qcom/qcs404.c | 4 +- drivers/interconnect/qcom/sdm660.c | 7 +- drivers/interconnect/qcom/smd-rpm.c | 24 +- drivers/interconnect/qcom/smd-rpm.h | 15 -- drivers/soc/qcom/smd-rpm.c | 17 +- include/dt-bindings/interconnect/qcom,rpm-icc.h | 13 + include/linux/soc/qcom/smd-rpm.h | 20 +- 17 files changed, 453 insertions(+), 337 deletions(-) --- base-commit: 1f6ce8392d6ff486af5ca96df9ded5882c4b6977 change-id: 20230526-topic-smd_icc-b8213948a5ed Best regards,