Received-SPF: pass (google.com: domain of linux-kernel+bounces-90946-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 4 Mar 2024 08:40:09 -0800
From: Mike Tipton <quic_mdtipton@quicinc.com>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
CC: Bjorn Andersson <andersson@kernel.org>, Georgi Djakov <djakov@kernel.org>,
        Abel Vesa <abel.vesa@linaro.org>, Sibi Sankar <quic_sibis@quicinc.com>,
        Rajendra Nayak <quic_rjendra@quicinc.com>,
        Marijn Suijten
	<marijn.suijten@somainline.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-pm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] interconnect: qcom: x1e80100: Remove inexistent ACV_PERF
 BCM
Message-ID: <20240304164009.GB25492@hu-mdtipton-lv.qualcomm.com>
References: <20240302-topic-faux_bcm_x1e-v1-1-c40fab7c4bc5@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240302-topic-faux_bcm_x1e-v1-1-c40fab7c4bc5@linaro.org>
User-Agent: Mutt/1.9.4 (2018-02-28)

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