Date:   Sat, 10 Jun 2023 19:54:47 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Stephan Gerhold <stephan@...hold.net>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Georgi Djakov <djakov@...nel.org>,
        Leo Yan <leo.yan@...aro.org>,
        Evan Green <evgreen@...omium.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 17/22] interconnect: qcom: icc-rpm: Control bus rpmcc
 from icc



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@...aro.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

Powered by blists - more mailing lists