Received-SPF: pass (google.com: domain of linux-kernel+bounces-170238-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <2649871c-ad10-4642-acc0-a15b03b4f1dd@quicinc.com>
Date: Mon, 6 May 2024 10:40:13 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 2/5] soc: qcom: llcc: Add regmap for Broadcast_AND
 region
Content-Language: en-US
To: Konrad Dybcio <konrad.dybcio@linaro.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        "Rob Herring" <robh@kernel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Johan Hovold <johan+linaro@kernel.org>
CC: <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <kernel@quicinc.com>
References: <20240329-llcc-broadcast-and-v4-0-107c76fd8ceb@quicinc.com>
 <20240329-llcc-broadcast-and-v4-2-107c76fd8ceb@quicinc.com>
 <d6b0f9d2-a489-4c0e-9c77-0e3eab49d3cb@linaro.org>
 <13ccc36a-c3cc-469a-ae0b-71fd0d24bf63@quicinc.com>
 <3365d84a-1b1d-4b1d-8be5-6d8d1039e6b9@linaro.org>
From: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
In-Reply-To: <3365d84a-1b1d-4b1d-8be5-6d8d1039e6b9@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

On 4/10/2024 11:24 AM, Konrad Dybcio wrote:
> 
> 
> On 4/2/24 21:34, Unnathi Chalicheemala wrote:
>> On 3/30/2024 4:46 AM, Krzysztof Kozlowski wrote:
>>> On 29/03/2024 22:53, Unnathi Chalicheemala wrote:
>>>> Define new regmap structure for Broadcast_AND region and initialize
>>>> this regmap when HW block version is greater than 4.1, otherwise
>>>> initialize as a NULL pointer for backwards compatibility.
>>>>
>>>
>>>> +    struct regmap *regmap;
>>>>       u32 act_ctrl_reg;
>>>>       u32 act_clear_reg;
>>>>       u32 status_reg;
>>>> @@ -849,7 +850,8 @@ static int llcc_update_act_ctrl(u32 sid,
>>>>           return ret;
>>>>         if (drv_data->version >= LLCC_VERSION_4_1_0_0) {
>>>> -        ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
>>>> +        regmap = drv_data->bcast_and_regmap ?: drv_data->bcast_regmap;
>>>> +        ret = regmap_read_poll_timeout(regmap, status_reg,
>>>>                         slice_status, (slice_status & ACT_COMPLETE),
>>>>                         0, LLCC_STATUS_READ_DELAY);
>>>>           if (ret)
>>>> @@ -1284,6 +1286,16 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>>         drv_data->version = version;
>>>>   +    /* Applicable only when drv_data->version >= 4.1 */
>>>> +    drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base");
>>>> +    if (IS_ERR(drv_data->bcast_and_regmap)) {
>>>
>>> I am pretty sure this breaks all users. Can you please explain how do
>>> you maintain ABI and that IS_ERR() is applied only for version >= 4.1?
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> IS_ERR() check is done for all versions.
>> If new register isn't defined in DT(for version < 4.1) it simply sets bcast_and_regmap to NULL.
>> Otherwise, for version >= 4.1, it goes to err(in the case bcast_and_regmap isn't set properly).
> 
> b4 shazam <this series>
> 
> booting on 8250, I get:
> 
> [    2.794850] qcom-llcc 9200000.system-cache-controller: invalid resource (null)
> 
> which comes from lib/devres.c : __devm_ioremap_resource()
> 
> Now, this is gonna get you an angry Johan(+CC) response when he sees this land in
> the next release. Perhaps, this warning could either be removed from libdevres,
> or some sort of an _optional variant that doesn't print it could be introduced.
> 
> Konrad

Apologies for extremely late reply Konrad. Let me try to recap quickly.
The part you pointed out initializes a new regmap LLCC Boradcast AND region
which is available only SM8450 onward. This patch set is updating respective DTs
and driver code.

Regarding the resource error on booting, I had added this check in previous version
of patch set (https://lore.kernel.org/all/1ca4d384-9df4-4c00-a4c9-0c5ff491616e@linaro.org/)

@@ -1282,6 +1287,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 
 	drv_data->version = version;
 
+	if (drv_data->version >= LLCC_VERSION_4_1_0_0) {
+		drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base");
+		if (IS_ERR(drv_data->bcast_and_regmap)) {
+			ret = PTR_ERR(drv_data->bcast_and_regmap);
+			if (ret == -EINVAL)
+				drv_data->bcast_and_regmap = NULL;
+			else
+				goto err;
+		}
+	}
+

This check will make sure we call devm_ioremap_resource() only when LLCC Boradcast AND region
is defined in the devicetree and error is not shown in log.