Re: [PATCH RFC RFT] interconnect: qcom: implement get_bw with rpmh_read

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 11/6/25 5:46 PM, Neil Armstrong wrote:
> Since we can actually read back the APPS rpmh interconnect
> BCM votes we can actually implement the get_bw() callback
> and provide a coherent average and peak bandwidth at probe time.
> 
> The benefits of that are:
> - keep disabled BCMs disabled
> - avoid voting unused BCMs to INT_MAX
> 
> If the interconnects are correctly described for a platform,
> all the required BCMs would be voted to the maximum bandwidth
> until sync_state is reached.
> 
> Since we only get the BCM vote, we need to redistribute
> the vote values to the associated nodes. The initial BCM
> votes are read back at probe time in order to be ready when
> the get_bw() is called when a node is added.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Depends on:
> https://lore.kernel.org/all/20251022-add-rpmh-read-support-v2-2-5c7a8e4df601@oss.qualcomm.com/
> ---
>  drivers/interconnect/qcom/bcm-voter.c | 36 +++++++++++++++++++++
>  drivers/interconnect/qcom/bcm-voter.h |  2 ++
>  drivers/interconnect/qcom/icc-rpmh.c  | 60 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
> index a2d437a05a11..9014bf20adad 100644
> --- a/drivers/interconnect/qcom/bcm-voter.c
> +++ b/drivers/interconnect/qcom/bcm-voter.c
> @@ -261,6 +261,42 @@ void qcom_icc_bcm_voter_add(struct bcm_voter *voter, struct qcom_icc_bcm *bcm)
>  }
>  EXPORT_SYMBOL_GPL(qcom_icc_bcm_voter_add);
>  
> +/**
> + * qcom_icc_bcm_get_bw - get current bcm vote
> + * @voter: voter used to query bcm
> + * @bcm: bcm to get current vote from
> + */
> +void qcom_icc_bcm_get_bw(struct bcm_voter *voter,
> +			 struct qcom_icc_bcm *bcm)
> +{
> +	struct tcs_cmd cmd = { .addr = bcm->addr };
> +	int ret, i;
> +	u64 x, y;
> +
> +	mutex_lock(&voter->lock);

guard(mutex)(&voter->lock) will let you drop the goto

> +
> +	rpmh_invalidate(voter->dev);
> +
> +	ret = rpmh_read(voter->dev, &cmd);
> +	if (ret) {
> +		pr_err("Error sending AMC RPMH requests (%d)\n", ret);
> +		goto out;
> +	}
> +
> +	x = FIELD_GET(BCM_TCS_CMD_VOTE_X_MASK, cmd.data);
> +	y = FIELD_GET(BCM_TCS_CMD_VOTE_Y_MASK, cmd.data);
> +
> +	/* For boot-up, fill the AMC vote in all buckets */

This isn't a good idea, I think we should be able to get information
from all buckets separately. I asked on the thread that introduced this
API. I'm assuming it was hardcoded to ACTIVE_ONLY because of its use
with the current state of the upstream regulator driver

> +static int qcom_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
> +{
> +	struct qcom_icc_node *qn = node->data;
> +	u32 avg_max = 0;
> +	u32 peak_max = 0;
> +	u64 x, y;
> +	int i;
> +
> +	if (!qn->num_bcms) {
> +		*avg = INT_MAX;
> +		*peak = INT_MAX;

Since this function returns an int, maybe you could alter the core to
check for an error and if -EOPNOTSUPP, fall back to these values (as
it does currently if !provider->get_bw)
> +
> +		return 0;
> +	}
> +
> +	for (i = 0; i < qn->num_bcms; ++i) {

odd pre-increment

> +		struct qcom_icc_bcm *bcm = qn->bcms[i];
> +
> +		/* Use AMC vote for boot-up */
> +		x = bcm->vote_x[QCOM_ICC_BUCKET_AMC];
> +		y = bcm->vote_y[QCOM_ICC_BUCKET_AMC];
> +
> +		/* Consider enable mask and convert to INT_MAX */
> +		if (bcm->enable_mask) {
> +			if (x & bcm->enable_mask)
> +				avg_max = INT_MAX;
> +			if (y & bcm->enable_mask)
> +				peak_max = INT_MAX;
> +		} else {
> +			if (x) {
> +				x *= bcm->aux_data.unit;
> +				do_div(x, bcm->vote_scale);
> +				x *= qn->buswidth * qn->channels;
> +				do_div(x, bcm->aux_data.width);

mult_frac()

Konrad