diff mbox series

interconnect: qcom: x1e80100: Remove inexistent ACV_PERF BCM

Message ID 20240302-topic-faux_bcm_x1e-v1-1-c40fab7c4bc5@linaro.org (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series interconnect: qcom: x1e80100: Remove inexistent ACV_PERF BCM | expand

Commit Message

Konrad Dybcio March 2, 2024, 2:22 a.m. UTC 
Booting the kernel on X1E results in a message like:

[    2.561524] qnoc-x1e80100 interconnect-0: ACV_PERF could not find RPMh address

And indeed, taking a look at cmd-db, no such BCM exists. Remove it.

Fixes: 9f196772841e ("interconnect: qcom: Add X1E80100 interconnect provider driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/x1e80100.c | 26 --------------------------
 1 file changed, 26 deletions(-)


---
base-commit: 1870cdc0e8dee32e3c221704a2977898ba4c10e8
change-id: 20240302-topic-faux_bcm_x1e-8639adf9d010

Best regards,

Comments

Mike Tipton March 4, 2024, 4:40 p.m. UTC | #1 
On Sat, Mar 02, 2024 at 03:22:49AM +0100, Konrad Dybcio wrote:
> Booting the kernel on X1E results in a message like:
> 
> [    2.561524] qnoc-x1e80100 interconnect-0: ACV_PERF could not find RPMh address
> 
> And indeed, taking a look at cmd-db, no such BCM exists. Remove it.
> 
> Fixes: 9f196772841e ("interconnect: qcom: Add X1E80100 interconnect provider driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Mike Tipton <quic_mdtipton@quicinc.com>

For some background, ACV "perf mode" does exist, but not as a separate
BCM. It's controlled by a separate bit in the ACV mask. By default, the
ACV node only sets the bit indicating the HLOS voter. It doesn't assert
the perf mode bit.

Enabling perf mode toggles different trade-offs within the DDR subsystem
for slightly improved performance at the expense of slightly higher
power. There are limited use cases of this downstream, where we expose
control over this bit to clients through icc_set_tag(). It primarily
improves certain latency sensitive benchmarks, AFAIK. I don't think it
has much impact on real world use cases.

This is true for many other targets as well, not just x1e80100.

Voting for perf-mode is entirely optional and in most cases also
entirely unnecessary. So, removing this broken way to control it without
adding the proper control is totally fine.

I have a local series to add the proper support, but haven't posted it
yet. There aren't any users for it upstream yet, nor am I aware of any
near term plans to add them. So, it would be unused for a little while,
at least. That said, anybody could use it to set that tag on their BW
votes on the off-chance it improves performance and they don't care
about the power trade-offs.

I could post the series soon if there's interest.

> ---
>  drivers/interconnect/qcom/x1e80100.c | 26 --------------------------
>  1 file changed, 26 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/x1e80100.c b/drivers/interconnect/qcom/x1e80100.c
> index 99824675ee3f..654abb9ce08e 100644
> --- a/drivers/interconnect/qcom/x1e80100.c
> +++ b/drivers/interconnect/qcom/x1e80100.c
> @@ -116,15 +116,6 @@ static struct qcom_icc_node xm_sdc2 = {
>  	.links = { X1E80100_SLAVE_A2NOC_SNOC },
>  };
>  
> -static struct qcom_icc_node ddr_perf_mode_master = {
> -	.name = "ddr_perf_mode_master",
> -	.id = X1E80100_MASTER_DDR_PERF_MODE,
> -	.channels = 1,
> -	.buswidth = 4,
> -	.num_links = 1,
> -	.links = { X1E80100_SLAVE_DDR_PERF_MODE },
> -};
> -
>  static struct qcom_icc_node qup0_core_master = {
>  	.name = "qup0_core_master",
>  	.id = X1E80100_MASTER_QUP_CORE_0,
> @@ -688,14 +679,6 @@ static struct qcom_icc_node qns_a2noc_snoc = {
>  	.links = { X1E80100_MASTER_A2NOC_SNOC },
>  };
>  
> -static struct qcom_icc_node ddr_perf_mode_slave = {
> -	.name = "ddr_perf_mode_slave",
> -	.id = X1E80100_SLAVE_DDR_PERF_MODE,
> -	.channels = 1,
> -	.buswidth = 4,
> -	.num_links = 0,
> -};
> -
>  static struct qcom_icc_node qup0_core_slave = {
>  	.name = "qup0_core_slave",
>  	.id = X1E80100_SLAVE_QUP_CORE_0,
> @@ -1377,12 +1360,6 @@ static struct qcom_icc_bcm bcm_acv = {
>  	.nodes = { &ebi },
>  };
>  
> -static struct qcom_icc_bcm bcm_acv_perf = {
> -	.name = "ACV_PERF",
> -	.num_nodes = 1,
> -	.nodes = { &ddr_perf_mode_slave },
> -};
> -
>  static struct qcom_icc_bcm bcm_ce0 = {
>  	.name = "CE0",
>  	.num_nodes = 1,
> @@ -1583,18 +1560,15 @@ static const struct qcom_icc_desc x1e80100_aggre2_noc = {
>  };
>  
>  static struct qcom_icc_bcm * const clk_virt_bcms[] = {
> -	&bcm_acv_perf,
>  	&bcm_qup0,
>  	&bcm_qup1,
>  	&bcm_qup2,
>  };
>  
>  static struct qcom_icc_node * const clk_virt_nodes[] = {
> -	[MASTER_DDR_PERF_MODE] = &ddr_perf_mode_master,
>  	[MASTER_QUP_CORE_0] = &qup0_core_master,
>  	[MASTER_QUP_CORE_1] = &qup1_core_master,
>  	[MASTER_QUP_CORE_2] = &qup2_core_master,
> -	[SLAVE_DDR_PERF_MODE] = &ddr_perf_mode_slave,
>  	[SLAVE_QUP_CORE_0] = &qup0_core_slave,
>  	[SLAVE_QUP_CORE_1] = &qup1_core_slave,
>  	[SLAVE_QUP_CORE_2] = &qup2_core_slave,
> 
> ---
> base-commit: 1870cdc0e8dee32e3c221704a2977898ba4c10e8
> change-id: 20240302-topic-faux_bcm_x1e-8639adf9d010
> 
> Best regards,
> -- 
> Konrad Dybcio <konrad.dybcio@linaro.org>
>
Konrad Dybcio March 4, 2024, 5:36 p.m. UTC | #2 
On 3/4/24 17:40, Mike Tipton wrote:
> On Sat, Mar 02, 2024 at 03:22:49AM +0100, Konrad Dybcio wrote:
>> Booting the kernel on X1E results in a message like:
>>
>> [    2.561524] qnoc-x1e80100 interconnect-0: ACV_PERF could not find RPMh address
>>
>> And indeed, taking a look at cmd-db, no such BCM exists. Remove it.
>>
>> Fixes: 9f196772841e ("interconnect: qcom: Add X1E80100 interconnect provider driver")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Reviewed-by: Mike Tipton <quic_mdtipton@quicinc.com>
> 
> For some background, ACV "perf mode" does exist, but not as a separate
> BCM. It's controlled by a separate bit in the ACV mask. By default, the
> ACV node only sets the bit indicating the HLOS voter. It doesn't assert
> the perf mode bit.
> 
> Enabling perf mode toggles different trade-offs within the DDR subsystem
> for slightly improved performance at the expense of slightly higher
> power. There are limited use cases of this downstream, where we expose
> control over this bit to clients through icc_set_tag(). It primarily
> improves certain latency sensitive benchmarks, AFAIK. I don't think it
> has much impact on real world use cases.
> 
> This is true for many other targets as well, not just x1e80100.
> 
> Voting for perf-mode is entirely optional and in most cases also
> entirely unnecessary. So, removing this broken way to control it without
> adding the proper control is totally fine.
> 
> I have a local series to add the proper support, but haven't posted it
> yet. There aren't any users for it upstream yet, nor am I aware of any
> near term plans to add them. So, it would be unused for a little while,
> at least. That said, anybody could use it to set that tag on their BW
> votes on the off-chance it improves performance and they don't care
> about the power trade-offs.
> 
> I could post the series soon if there's interest.

I think adding a sysfs entry for toggling this could be very interesting.

Userspace could toggle this based on "power profile"-style settings.

Konrad
diff mbox series

Patch

diff --git a/drivers/interconnect/qcom/x1e80100.c b/drivers/interconnect/qcom/x1e80100.c
index 99824675ee3f..654abb9ce08e 100644
--- a/drivers/interconnect/qcom/x1e80100.c
+++ b/drivers/interconnect/qcom/x1e80100.c
@@ -116,15 +116,6 @@  static struct qcom_icc_node xm_sdc2 = {
 	.links = { X1E80100_SLAVE_A2NOC_SNOC },
 };
 
-static struct qcom_icc_node ddr_perf_mode_master = {
-	.name = "ddr_perf_mode_master",
-	.id = X1E80100_MASTER_DDR_PERF_MODE,
-	.channels = 1,
-	.buswidth = 4,
-	.num_links = 1,
-	.links = { X1E80100_SLAVE_DDR_PERF_MODE },
-};
-
 static struct qcom_icc_node qup0_core_master = {
 	.name = "qup0_core_master",
 	.id = X1E80100_MASTER_QUP_CORE_0,
@@ -688,14 +679,6 @@  static struct qcom_icc_node qns_a2noc_snoc = {
 	.links = { X1E80100_MASTER_A2NOC_SNOC },
 };
 
-static struct qcom_icc_node ddr_perf_mode_slave = {
-	.name = "ddr_perf_mode_slave",
-	.id = X1E80100_SLAVE_DDR_PERF_MODE,
-	.channels = 1,
-	.buswidth = 4,
-	.num_links = 0,
-};
-
 static struct qcom_icc_node qup0_core_slave = {
 	.name = "qup0_core_slave",
 	.id = X1E80100_SLAVE_QUP_CORE_0,
@@ -1377,12 +1360,6 @@  static struct qcom_icc_bcm bcm_acv = {
 	.nodes = { &ebi },
 };
 
-static struct qcom_icc_bcm bcm_acv_perf = {
-	.name = "ACV_PERF",
-	.num_nodes = 1,
-	.nodes = { &ddr_perf_mode_slave },
-};
-
 static struct qcom_icc_bcm bcm_ce0 = {
 	.name = "CE0",
 	.num_nodes = 1,
@@ -1583,18 +1560,15 @@  static const struct qcom_icc_desc x1e80100_aggre2_noc = {
 };
 
 static struct qcom_icc_bcm * const clk_virt_bcms[] = {
-	&bcm_acv_perf,
 	&bcm_qup0,
 	&bcm_qup1,
 	&bcm_qup2,
 };
 
 static struct qcom_icc_node * const clk_virt_nodes[] = {
-	[MASTER_DDR_PERF_MODE] = &ddr_perf_mode_master,
 	[MASTER_QUP_CORE_0] = &qup0_core_master,
 	[MASTER_QUP_CORE_1] = &qup1_core_master,
 	[MASTER_QUP_CORE_2] = &qup2_core_master,
-	[SLAVE_DDR_PERF_MODE] = &ddr_perf_mode_slave,
 	[SLAVE_QUP_CORE_0] = &qup0_core_slave,
 	[SLAVE_QUP_CORE_1] = &qup1_core_slave,
 	[SLAVE_QUP_CORE_2] = &qup2_core_slave,