Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Fri, 3 Nov 2023 12:33:45 -0700
From:   Bjorn Andersson <quic_bjorande@quicinc.com>
To:     Atul Dhudase <quic_adhudase@quicinc.com>
CC:     <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <swboyd@chromium.org>,
        <isaacm@codeaurora.org>, <dianders@chromium.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] soc: qcom: llcc: Fix dis_cap_alloc and retain_on_pc
 configuration
Message-ID: <20231103193345.GY3553829@hu-bjorande-lv.qualcomm.com>
References: <20231103105712.1159213-1-quic_adhudase@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231103105712.1159213-1-quic_adhudase@quicinc.com>
Precedence: bulk

On Fri, Nov 03, 2023 at 04:27:12PM +0530, Atul Dhudase wrote:
> While programming dis_cap_alloc and retain_on_pc, set a bit
> corresponding to a specific SCID without disturbing the
> previously configured bits.
> 

As far as I can see, the only invocation of _qcom_llcc_cfg_program()
comes from qcom_llcc_cfg_program(), which is only called once, from
qcom_llcc_probe(), and here also seems to only be the single write to
these two registers.

This implies that "the previously configured bits" would be some unknown
configuration provided to us either from the bootloader or by reset of
the hardware. As such this changes the value of the two registers from
being known, to having 31 unknown bits.


I'm not saying that the change is wrong, but you're altering the
behavior of every platform except SDM845.
As such, I want the commit message to provide an actual problem
description, and mention the fact that you're changing the logic to
retain the state prior to Linux.

Regards,
Bjorn

> Fixes: c14e64b46944 ("soc: qcom: llcc: Support chipsets that can write to llcc")
> Signed-off-by: Atul Dhudase <quic_adhudase@quicinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 674abd0d6700..509d972c1bd9 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -941,15 +941,15 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config,
>  		u32 disable_cap_alloc, retain_pc;
> 
>  		disable_cap_alloc = config->dis_cap_alloc << config->slice_id;
> -		ret = regmap_write(drv_data->bcast_regmap,
> -				LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc);
> +		ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_SCID_DIS_CAP_ALLOC,
> +				BIT(config->slice_id), disable_cap_alloc);
>  		if (ret)
>  			return ret;
> 
>  		if (drv_data->version < LLCC_VERSION_4_1_0_0) {
>  			retain_pc = config->retain_on_pc << config->slice_id;
> -			ret = regmap_write(drv_data->bcast_regmap,
> -					LLCC_TRP_PCB_ACT, retain_pc);
> +			ret = regmap_update_bits(drv_data->bcast_regmap, LLCC_TRP_PCB_ACT,
> +					BIT(config->slice_id), retain_pc);
>  			if (ret)
>  				return ret;
>  		}
> --
> 2.25.1
>