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(-)
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);
+
+ 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 */
+ for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
+ bcm->vote_x[i] = x;
+ bcm->vote_y[i] = y;
+ }
+
+out:
+ mutex_unlock(&voter->lock);
+}
+EXPORT_SYMBOL_GPL(qcom_icc_bcm_get_bw);
+
/**
* qcom_icc_bcm_voter_commit - generates and commits tcs cmds based on bcms
* @voter: voter that needs flushing
diff --git a/drivers/interconnect/qcom/bcm-voter.h b/drivers/interconnect/qcom/bcm-voter.h
index b4d36e349f3c..338cdc16653d 100644
--- a/drivers/interconnect/qcom/bcm-voter.h
+++ b/drivers/interconnect/qcom/bcm-voter.h
@@ -13,6 +13,8 @@
#include "icc-rpmh.h"
struct bcm_voter *of_bcm_voter_get(struct device *dev, const char *name);
+void qcom_icc_bcm_get_bw(struct bcm_voter *voter,
+ struct qcom_icc_bcm *bcm);
void qcom_icc_bcm_voter_add(struct bcm_voter *voter, struct qcom_icc_bcm *bcm);
int qcom_icc_bcm_voter_commit(struct bcm_voter *voter);
diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index 001404e91041..202bbb565de0 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -136,6 +136,61 @@ int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
}
EXPORT_SYMBOL_GPL(qcom_icc_set);
+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;
+
+ return 0;
+ }
+
+ for (i = 0; i < qn->num_bcms; ++i) {
+ 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);
+
+ avg_max = max(avg_max, x);
+ }
+
+ if (y) {
+ y *= bcm->aux_data.unit;
+ do_div(y, bcm->vote_scale);
+ y *= qn->buswidth;
+ do_div(y, bcm->aux_data.width);
+
+ peak_max = max(peak_max, y);
+ }
+ }
+ }
+
+ *avg = avg_max;
+ *peak = peak_max;
+
+ return 0;
+}
+
/**
* qcom_icc_bcm_init - populates bcm aux data and connect qnodes
* @bcm: bcm to be initialized
@@ -255,6 +310,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
provider = &qp->provider;
provider->dev = dev;
provider->set = qcom_icc_set;
+ provider->get_bw = qcom_icc_get_bw;
provider->pre_aggregate = qcom_icc_pre_aggregate;
provider->aggregate = qcom_icc_aggregate;
provider->xlate_extended = qcom_icc_xlate_extended;
@@ -272,8 +328,10 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
if (IS_ERR(qp->voter))
return PTR_ERR(qp->voter);
- for (i = 0; i < qp->num_bcms; i++)
+ for (i = 0; i < qp->num_bcms; i++) {
qcom_icc_bcm_init(qp->bcms[i], dev);
+ qcom_icc_bcm_get_bw(qp->voter, qp->bcms[i]);
+ }
for (i = 0; i < num_nodes; i++) {
qn = qnodes[i];
---
base-commit: c077667d2d33618e2053f79ec60300dae7a58e0c
change-id: 20251106-topic-sm8x50-icc-read-rpmh-eba461a452e7
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>On 11/6/25 6: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. > FWIW, I was able to finally test this on sdm845. Some nodes are indeed showing reasonable bandwidth values instead of the default INT_MAX. Tested-by: Georgi Djakov <djakov@kernel.org> #db845c > 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(-) >
On 1/13/26 6:53 PM, Georgi Djakov wrote:
> On 11/6/25 6: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.
>>
>
> FWIW, I was able to finally test this on sdm845. Some nodes are indeed
> showing reasonable bandwidth values instead of the default INT_MAX.
As I learnt here
https://lore.kernel.org/linux-arm-msm/1e7594dc-dca6-42e7-b478-b063e3325aff@oss.qualcomm.com/
rpmh_read() will only retrieve the currently active values, so as-is,
this hunk:
+ /* For boot-up, fill the AMC vote in all buckets */
+ for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
+ bcm->vote_x[i] = x;
+ bcm->vote_y[i] = y;
+ }
is lying about the state of wake/sleep buckets
this is ""fine"" today, as I don't see any "if (old_bw == new_bw)" checks
across the framework, but debugfs is going to report incorrect values and
if anyone decides to add the aforementioned check, it may introduce issues
where the values aren't commited to the hardware (because Linux is going
to believe they're already set)
Konrad
On 1/14/26 11:01, Konrad Dybcio wrote:
> On 1/13/26 6:53 PM, Georgi Djakov wrote:
>> On 11/6/25 6: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.
>>>
>>
>> FWIW, I was able to finally test this on sdm845. Some nodes are indeed
>> showing reasonable bandwidth values instead of the default INT_MAX.
>
> As I learnt here
>
> https://lore.kernel.org/linux-arm-msm/1e7594dc-dca6-42e7-b478-b063e3325aff@oss.qualcomm.com/
>
> rpmh_read() will only retrieve the currently active values, so as-is,
> this hunk:
>
> + /* For boot-up, fill the AMC vote in all buckets */
> + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
> + bcm->vote_x[i] = x;
> + bcm->vote_y[i] = y;
> + }
>
> is lying about the state of wake/sleep buckets
>
> this is ""fine"" today, as I don't see any "if (old_bw == new_bw)" checks
> across the framework, but debugfs is going to report incorrect values and
> if anyone decides to add the aforementioned check, it may introduce issues
> where the values aren't commited to the hardware (because Linux is going
> to believe they're already set)
This is only for the pre-sync-state phase, where we don't need the wake/sleep
values but the interconnect rpmh implementation needs them, and anyway they will
be replaced by proper values in sync_state
So this is an informed & assumed choice I did here. It's a small optimization
to avoid turning on _all_ interconnects at INT_MAX, and keep boot votes
up to sync_state.
Neil
>
> Konrad
On 1/14/26 11:07 AM, Neil Armstrong wrote:
> On 1/14/26 11:01, Konrad Dybcio wrote:
>> On 1/13/26 6:53 PM, Georgi Djakov wrote:
>>> On 11/6/25 6: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.
>>>>
>>>
>>> FWIW, I was able to finally test this on sdm845. Some nodes are indeed
>>> showing reasonable bandwidth values instead of the default INT_MAX.
>>
>> As I learnt here
>>
>> https://lore.kernel.org/linux-arm-msm/1e7594dc-dca6-42e7-b478-b063e3325aff@oss.qualcomm.com/
>>
>> rpmh_read() will only retrieve the currently active values, so as-is,
>> this hunk:
>>
>> + /* For boot-up, fill the AMC vote in all buckets */
>> + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>> + bcm->vote_x[i] = x;
>> + bcm->vote_y[i] = y;
>> + }
>>
>> is lying about the state of wake/sleep buckets
>>
>> this is ""fine"" today, as I don't see any "if (old_bw == new_bw)" checks
>> across the framework, but debugfs is going to report incorrect values and
>> if anyone decides to add the aforementioned check, it may introduce issues
>> where the values aren't commited to the hardware (because Linux is going
>> to believe they're already set)
>
> This is only for the pre-sync-state phase, where we don't need the wake/sleep
> values but the interconnect rpmh implementation needs them, and anyway they will
> be replaced by proper values in sync_state
I realize this may not be the most convincing argument, but consider
the case where sync_state can not be hit, for example with the Venus
driver that requests FW at probe time and errors out if it's absent
> So this is an informed & assumed choice I did here. It's a small optimization
> to avoid turning on _all_ interconnects at INT_MAX, and keep boot votes
> up to sync_state.
Another question is, whether that's a desired change - I could easily
see pinning buses to the maximum speed helping boot time KPIs, but
perhaps that could/should be configurable?
Konrad
On 1/14/26 11:31, Konrad Dybcio wrote:
> On 1/14/26 11:07 AM, Neil Armstrong wrote:
>> On 1/14/26 11:01, Konrad Dybcio wrote:
>>> On 1/13/26 6:53 PM, Georgi Djakov wrote:
>>>> On 11/6/25 6: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.
>>>>>
>>>>
>>>> FWIW, I was able to finally test this on sdm845. Some nodes are indeed
>>>> showing reasonable bandwidth values instead of the default INT_MAX.
>>>
>>> As I learnt here
>>>
>>> https://lore.kernel.org/linux-arm-msm/1e7594dc-dca6-42e7-b478-b063e3325aff@oss.qualcomm.com/
>>>
>>> rpmh_read() will only retrieve the currently active values, so as-is,
>>> this hunk:
>>>
>>> + /* For boot-up, fill the AMC vote in all buckets */
>>> + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>>> + bcm->vote_x[i] = x;
>>> + bcm->vote_y[i] = y;
>>> + }
>>>
>>> is lying about the state of wake/sleep buckets
>>>
>>> this is ""fine"" today, as I don't see any "if (old_bw == new_bw)" checks
>>> across the framework, but debugfs is going to report incorrect values and
>>> if anyone decides to add the aforementioned check, it may introduce issues
>>> where the values aren't commited to the hardware (because Linux is going
>>> to believe they're already set)
>>
>> This is only for the pre-sync-state phase, where we don't need the wake/sleep
>> values but the interconnect rpmh implementation needs them, and anyway they will
>> be replaced by proper values in sync_state
>
> I realize this may not be the most convincing argument, but consider
> the case where sync_state can not be hit, for example with the Venus
> driver that requests FW at probe time and errors out if it's absent
We're talking about initial states here, if a device votes for an interconnect
path, even before sync_state, the path will be voted with the requested bandwidth.
https://elixir.bootlin.com/linux/v6.18.5/source/drivers/interconnect/core.c#L295
Before this patch:
node->init_avg & node->init_peak are set to INT_MAX, so max(x, INT_MAX) always gives INT_MAX
After this patch:
node->init_avg & node->init_peak are from the boot, which can be 0. So we either vote
for the requested bandwidth, or floor the bandwidth set by the bootloader (could be a higher value).
>
>> So this is an informed & assumed choice I did here. It's a small optimization
>> to avoid turning on _all_ interconnects at INT_MAX, and keep boot votes
>> up to sync_state.
>
> Another question is, whether that's a desired change - I could easily
> see pinning buses to the maximum speed helping boot time KPIs, but
> perhaps that could/should be configurable?
It's all about the rest of the bussed endpoints, enabling _all_ endpoints
to INT_MAX could potentially lead to in fact reducing bandwidth for crucial
devices like UFS because we configure everything to the max bandwidth, and enable
unused busses (and associated clocks & power domains) for nothing.
The idea in this patch is to keep the votes from bootloader, keep the disabled
endpoints and vote with requested bandwidth for devices which are used in the boot process.
Neil
>
> Konrad
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
© 2016 - 2026 Red Hat, Inc.