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@xxxxxxxxxx>
> >> ---
> >> 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
