From: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Andy Gross <agross@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
Georgi Djakov <djakov@kernel.org>, Leo Yan <leo.yan@linaro.org>,
Evan Green <evgreen@chromium.org>,
Marijn Suijten <marijn.suijten@somainline.org>,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-clk@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v2 17/22] interconnect: qcom: icc-rpm: Control bus rpmcc from icc
Date: Sat, 10 Jun 2023 14:14:18 +0200 [thread overview]
Message-ID: <53a0cfa0-95e2-d466-efb0-e17094942652@linaro.org> (raw)
In-Reply-To: <ZIRlbi_BXyuUWs-8@gerhold.net>
On 10.06.2023 13:58, Stephan Gerhold wrote:
> On Fri, Jun 09, 2023 at 10:19:22PM +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@linaro.org>
>> ---
>> drivers/interconnect/qcom/icc-rpm.c | 116 +++++++++++++++++++-----------------
>> drivers/interconnect/qcom/icc-rpm.h | 13 ++--
>> drivers/interconnect/qcom/msm8996.c | 1 -
>> drivers/interconnect/qcom/sdm660.c | 1 -
>> 4 files changed, 66 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index b8ecf9538ab9..6d40815c5401 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> [...]
>> @@ -364,49 +363,50 @@ 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)
>> + return clk_set_rate(qp->bus_clk, 1000ULL * max(active_rate, sleep_rate));
>
> Something like the min_t(u64, rate, LONG_MAX)* that was there in the old
> code is still needed for the clk_set_rate(). The reason is that the rate
> parameter in clk_set_rate() is unsigned long (32-bit on ARM32) while you
> do the calculation in fixed u64. This can easily overflow and then the
> higher bits will just be cut off.
>
> Consider the following on ARM32:
>
> u64 rate = 1ULL << 32 = 4294967296ULL;
> clk_set_rate(clk, rate);
>
> This actually does clk_set_rate(clk, 0) because the upper 32-bit will
> just be truncated. So the min() is needed to ensure that we really set
> the highest possible.
>
> Also see commit a7d9436a6c85 ("interconnect: qcom: rpm: Prevent integer
> overflow in rate") [1].
>
> * I'm not sure why I used LONG_MAX instead of ULONG_MAX back then.
Ughh can we kill arm32 already? It only causes problems :P
Thanks for spotting this..
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a7d9436a6c85fcb8843c910fd323dcd7f839bf63
>
>> [...]
>> int qnoc_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -448,6 +448,18 @@ int qnoc_probe(struct platform_device *pdev)
>> if (!qp->intf_clks)
>> return -ENOMEM;
>>
>> + if (desc->bus_clk_desc) {
>> + qp->bus_clk_desc = devm_kzalloc(dev, sizeof(*qp->bus_clk_desc),
>> + GFP_KERNEL);
>> + if (!qp->bus_clk_desc)
>> + return -ENOMEM;
>> +
>> + qp->bus_clk_desc = desc->bus_clk_desc;
>> + } else if (!IS_ERR(devm_clk_get(dev, "bus"))) {
>> + /* Some older SoCs may have a single non-RPM-owned bus clock. */
>> + qp->bus_clk = devm_clk_get(dev, "bus");
>> + }
>
> Hm, looks like you're requesting the clock twice? devm_clk_get()
> allocates memory internally so that's not ideal. It would be better to
> call it just once and store the result. Or do you actually want
> devm_clk_get_optional(dev, "bus") maybe? The error handling is a bit
> weird here.
Hmm, right.. I think it should go something like this:
qp->bus_clk = devm_clk_get(dev, "bus");
if (IS_ERR(qp->bus_clk) && PTR_ERR(qp->bus_clk) == -ENOENT)
qp->bus_clk = NULL;
else if (IS_ERR(qp->bus_clk))
return PTR_ERR(qp->bus_clk)
>
>> [...]
>> @@ -490,13 +498,11 @@ int qnoc_probe(struct platform_device *pdev)
>> }
>>
>> regmap_done:
>> - ret = devm_clk_bulk_get(dev, qp->num_bus_clks, qp->bus_clks);
>> - if (ret)
>> - return ret;
>> -
>> - ret = clk_bulk_prepare_enable(qp->num_bus_clks, qp->bus_clks);
>> - if (ret)
>> - return ret;
>
> I guess we need dt-binding updates so we can drop the clocks from the
> device tree? They're not valid anymore after you remove them from
> clk-smd-rpm.c so it would be good to drop them from the DTs to avoid
> confusion.
I wanted to handle that separately as the bindings are technically still
correct.. There'll be a need for some deprecation though.
>
>> + if (qp->bus_clk) {
>> + ret = clk_prepare_enable(qp->bus_clk);
>> + if (ret)
>> + return ret;
>> + }
>
> The clk_* APIs have internal NULL checks for optional clocks so you
> could also omit the if statement and call it unconditionally.
Ack
Konrad
>
>>
>> ret = devm_clk_bulk_get(dev, qp->num_intf_clks, qp->intf_clks);
>> if (ret)
>> @@ -566,7 +572,8 @@ int qnoc_probe(struct platform_device *pdev)
>> icc_provider_deregister(provider);
>> err_remove_nodes:
>> icc_nodes_remove(provider);
>> - clk_bulk_disable_unprepare(qp->num_bus_clks, qp->bus_clks);
>> + if (qp->bus_clk)
>> + clk_disable_unprepare(qp->bus_clk);
>>
>
> Same here.
>
>> return ret;
>> }
>> @@ -578,7 +585,8 @@ int qnoc_remove(struct platform_device *pdev)
>>
>> icc_provider_deregister(&qp->provider);
>> icc_nodes_remove(&qp->provider);
>> - clk_bulk_disable_unprepare(qp->num_bus_clks, qp->bus_clks);
>> + if (qp->bus_clk)
>> + clk_disable_unprepare(qp->bus_clk);
>>
>
> And here.
>
> Thanks,
> Stephan
next prev parent reply other threads:[~2023-06-10 12:14 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-09 20:19 [PATCH v2 00/22] Restructure RPM SMD ICC Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 01/22] soc: qcom: smd-rpm: Add QCOM_SMD_RPM_STATE_NUM Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 02/22] soc: qcom: smd-rpm: Use tabs for defines Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 03/22] clk: qcom: smd-rpm: Move some RPM resources to the common header Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 04/22] clk: qcom: smd-rpm: Export clock scaling availability Konrad Dybcio
2023-06-10 11:35 ` Stephan Gerhold
2023-06-10 12:15 ` Konrad Dybcio
2023-06-10 18:53 ` Konrad Dybcio
2023-06-10 19:25 ` Stephan Gerhold
2023-06-10 19:39 ` Konrad Dybcio
2023-06-11 9:20 ` Stephan Gerhold
2023-06-12 12:51 ` Konrad Dybcio
2023-06-12 17:03 ` Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 05/22] interconnect: qcom: icc-rpm: Introduce keep_alive Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 06/22] interconnect: qcom: icc-rpm: Allow negative QoS offset Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 07/22] interconnect: qcom: Fold smd-rpm.h into icc-rpm.h Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 08/22] interconnect: qcom: smd-rpm: Add rpmcc handling skeleton code Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 09/22] interconnect: qcom: Add missing headers in icc-rpm.h Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 10/22] interconnect: qcom: Define RPM bus clocks Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 11/22] interconnect: qcom: sdm660: Hook up RPM bus clk definitions Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 12/22] interconnect: qcom: msm8996: " Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 13/22] interconnect: qcom: qcs404: " Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 14/22] interconnect: qcom: msm8939: " Konrad Dybcio
2023-06-10 12:02 ` Stephan Gerhold
2023-06-09 20:19 ` [PATCH v2 15/22] interconnect: qcom: msm8916: " Konrad Dybcio
2023-06-10 12:02 ` Stephan Gerhold
2023-06-09 20:19 ` [PATCH v2 16/22] interconnect: qcom: qcm2290: " Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 17/22] interconnect: qcom: icc-rpm: Control bus rpmcc from icc Konrad Dybcio
2023-06-10 11:58 ` Stephan Gerhold
2023-06-10 12:14 ` Konrad Dybcio [this message]
2023-06-10 16:20 ` Stephan Gerhold
2023-06-10 17:54 ` Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 18/22] clk: qcom: smd-rpm: Separate out interconnect bus clocks Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 19/22] interconnect: qcom: icc-rpm: Fix bucket number Konrad Dybcio
2023-06-10 17:46 ` Stephan Gerhold
2023-06-10 17:53 ` Konrad Dybcio
2023-06-09 20:19 ` [PATCH v2 20/22] interconnect: qcom: icc-rpm: Set bandwidth on both contexts Konrad Dybcio
2023-06-10 18:00 ` Stephan Gerhold
2023-06-10 18:28 ` Konrad Dybcio
2023-06-10 18:43 ` Stephan Gerhold
2023-06-09 20:19 ` [PATCH v2 21/22] interconnect: qcom: icc-rpm: Set correct bandwidth through RPM bw req Konrad Dybcio
2023-06-10 18:46 ` Stephan Gerhold
2023-06-09 20:19 ` [PATCH v2 22/22] interconnect: qcom: icc-rpm: Fix bandwidth calculations Konrad Dybcio
2023-06-10 19:06 ` Stephan Gerhold
2023-06-10 19:09 ` Konrad Dybcio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53a0cfa0-95e2-d466-efb0-e17094942652@linaro.org \
--to=konrad.dybcio@linaro.org \
--cc=agross@kernel.org \
--cc=andersson@kernel.org \
--cc=djakov@kernel.org \
--cc=evgreen@chromium.org \
--cc=leo.yan@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=mturquette@baylibre.com \
--cc=sboyd@kernel.org \
--cc=stephan@gerhold.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).