[v2,17/22] interconnect: qcom: icc-rpm: Control bus rpmcc from icc
Commit Message
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(-)
Comments
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.
[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.
> [...]
> @@ -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.
> + 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.
>
> 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
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
On Sat, Jun 10, 2023 at 02:14:18PM +0200, Konrad Dybcio wrote:
> 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
>
Nooooo, "32-bit ought to be enough for anybody"! :P
> >
> > [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)
>
This is the same as
qp->bus_clk = devm_clk_get_optional(dev, "bus");
if (IS_ERR(qp->bus_clk))
return PTR_ERR(qp->bus_clk);
clk_get_optional is literally defined as
static inline struct clk *clk_get_optional(struct device *dev, const char *id)
{
struct clk *clk = clk_get(dev, id);
if (clk == ERR_PTR(-ENOENT))
return NULL;
return 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.
>
OK, that's fine for me (not sure how the DT maintainers would feel about
this though).
Thanks,
Stephan
On 10.06.2023 18:20, Stephan Gerhold wrote:
> On Sat, Jun 10, 2023 at 02:14:18PM +0200, Konrad Dybcio wrote:
>> 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
>>
>
> Nooooo, "32-bit ought to be enough for anybody"! :P
>
>
>>>
>>> [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)
>>
>
> This is the same as
>
> qp->bus_clk = devm_clk_get_optional(dev, "bus");
> if (IS_ERR(qp->bus_clk))
> return PTR_ERR(qp->bus_clk);
>
> clk_get_optional is literally defined as
>
> static inline struct clk *clk_get_optional(struct device *dev, const char *id)
> {
> struct clk *clk = clk_get(dev, id);
> if (clk == ERR_PTR(-ENOENT))
> return NULL;
> return clk;
> }
Touche. I'll use this.
Konrad
>
>>
>>>
>>>> [...]
>>>> @@ -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.
>>
>
> OK, that's fine for me (not sure how the DT maintainers would feel about
> this though).
>
> Thanks,
> Stephan
@@ -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,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));
+
+ /* 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;
- }
- 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;
+ qp->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE] = sleep_rate;
+
return 0;
}
-static const char * const bus_clocks[] = {
- "bus", "bus_a",
-};
-
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");
+ }
+
data = devm_kzalloc(dev, struct_size(data, nodes, num_nodes),
GFP_KERNEL);
if (!data)
@@ -457,10 +469,6 @@ int qnoc_probe(struct platform_device *pdev)
for (i = 0; i < cd_num; i++)
qp->intf_clks[i].id = cds[i];
- qp->num_bus_clks = desc->no_clk_scaling ? 0 : NUM_BUS_CLKS;
- for (i = 0; i < qp->num_bus_clks; i++)
- qp->bus_clks[i].id = bus_clocks[i];
-
qp->keep_alive = desc->keep_alive;
qp->type = desc->type;
qp->qos_offset = desc->qos_offset;
@@ -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;
+ if (qp->bus_clk) {
+ ret = clk_prepare_enable(qp->bus_clk);
+ if (ret)
+ return ret;
+ }
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);
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);
return 0;
}
@@ -36,32 +36,29 @@ struct rpm_clk_resource {
bool branch;
};
-#define NUM_BUS_CLKS 2
-
/**
* struct qcom_icc_provider - Qualcomm specific interconnect provider
* @provider: generic interconnect provider
- * @num_bus_clks: the total number of bus_clks clk_bulk_data entries (0 or 2)
* @num_intf_clks: the total number of intf_clks clk_bulk_data entries
* @type: the ICC provider type
* @regmap: regmap for QoS registers read/write access
* @bus_clk_rate: bus clock rate in Hz
- * @bus_clks: the clk_bulk_data table of bus clocks
+ * @bus_clk_desc: a pointer to a rpm_clk_resource description of bus clocks
* @intf_clks: a clk_bulk_data array of interface clocks
+ * @bus_clk: a pointer to a HLOS-owned bus clock
* @qos_offset: offset to QoS registers
* @keep_alive: whether to always keep a minimum vote on the bus clocks
* @is_on: whether the bus is powered on
*/
struct qcom_icc_provider {
struct icc_provider provider;
- int num_bus_clks;
int num_intf_clks;
enum qcom_icc_type type;
struct regmap *regmap;
int qos_offset;
- u64 bus_clk_rate[NUM_BUS_CLKS];
- struct clk_bulk_data bus_clks[NUM_BUS_CLKS];
+ u32 bus_clk_rate[QCOM_SMD_RPM_STATE_NUM];
const struct rpm_clk_resource *bus_clk_desc;
+ struct clk *bus_clk;
struct clk_bulk_data *intf_clks;
bool keep_alive;
bool is_on;
@@ -118,12 +115,10 @@ struct qcom_icc_node {
struct qcom_icc_desc {
struct qcom_icc_node * const *nodes;
size_t num_nodes;
- const char * const *bus_clocks;
const struct rpm_clk_resource *bus_clk_desc;
const char * const *intf_clocks;
size_t num_intf_clocks;
bool keep_alive;
- bool no_clk_scaling;
enum qcom_icc_type type;
const struct regmap_config *regmap_cfg;
int qos_offset;
@@ -1818,7 +1818,6 @@ static const struct qcom_icc_desc msm8996_a0noc = {
.num_nodes = ARRAY_SIZE(a0noc_nodes),
.intf_clocks = a0noc_intf_clocks,
.num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
- .no_clk_scaling = true,
.regmap_cfg = &msm8996_a0noc_regmap_config
};
@@ -1618,7 +1618,6 @@ static const struct qcom_icc_desc sdm660_gnoc = {
.nodes = sdm660_gnoc_nodes,
.num_nodes = ARRAY_SIZE(sdm660_gnoc_nodes),
.regmap_cfg = &sdm660_gnoc_regmap_config,
- .no_clk_scaling = true,
};
static struct qcom_icc_node * const sdm660_mnoc_nodes[] = {