On 12.06.2023 22:54, Stephan Gerhold wrote:
> On Mon, Jun 12, 2023 at 08:24:35PM +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, but that's a mistake in and of itself).
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>
> Would be nice to add a comment here already that you're breaking
> compatbility with the qcom,icc.h. It's a bit hidden otherwise.
Right, wouldn't hurt..
>
>> ---
>> drivers/interconnect/qcom/icc-rpm.c | 114 ++++++++++++++++++++----------------
>> drivers/interconnect/qcom/icc-rpm.h | 13 ++--
>> drivers/interconnect/qcom/msm8996.c | 1 -
>> drivers/interconnect/qcom/sdm660.c | 1 -
>> 4 files changed, 66 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index b8ecf9538ab9..5ffcf5ca8914 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,54 @@ 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);
>> +
>> + if ((active_rate != qp->bus_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE]) ||
>> + (sleep_rate != qp->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE])) {
>> + ret = qcom_icc_rpm_set_bus_rate(qp->bus_clk_desc,
>> + active_rate,
>> + sleep_rate);
>> + if (ret)
>> return ret;
>
> Hm, do we have to set both rates together in all cases? If cpufreq is
> quickly changing frequencies (and therefore active-only ICC bandwidths)
> it should be sufficient to make one call into RPM and leave the sleep
> rate as-is. Especially because you already cache the two rates
> separately.
>
> AFAICT downstream updates the contexts completely separately, so I don't
> think it updates both rates at once either. And actually even the old
> code before this patch didn't do that :D
Right, I can trivially solve that by making qcom_icc_rpm_set_bus_rate
accept an is_active parameter..
Konrad
>
> Thanks,
> Stephan
