Received-SPF: pass (google.com: domain of linux-kernel+bounces-91894-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <0c2c039e-b49e-4172-9c7f-24061e3ac5c6@quicinc.com>
Date: Tue, 5 Mar 2024 13:47:05 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] usb: dwc3: qcom: Remove ACPI support from glue driver
To: Johan Hovold <johan@kernel.org>
CC: Bjorn Andersson <andersson@kernel.org>,
        Wesley Cheng
	<quic_wcheng@quicinc.com>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        "Greg
 Kroah-Hartman" <gregkh@linuxfoundation.org>,
        Thinh Nguyen
	<Thinh.Nguyen@synopsys.com>,
        <linux-arm-msm@vger.kernel.org>, <linux-usb@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_ppratap@quicinc.com>,
        <quic_jackp@quicinc.com>
References: <20240305042143.3455101-1-quic_kriskura@quicinc.com>
 <ZebFKlae0a-deBKl@hovoldconsulting.com>
Content-Language: en-US
From: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <ZebFKlae0a-deBKl@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 3/5/2024 12:39 PM, Johan Hovold wrote:
> On Tue, Mar 05, 2024 at 09:51:43AM +0530, Krishna Kurapati wrote:
>> Minimal ACPI support was added to the Qualcomm DWC3 glue driver in order to
>> enable USB on SDM850 and SC8180X compute platforms. The support is still
>> functional, but unnoticed regressions in other drivers indicates that no
>> one actually booting any of platforms dependent on this implementation.
>>
>> The functionality provides is the bare minimum and is not expected to aid
>> in the effort of bringing full ACPI support to the driver in the future.
>>
>> Remove the ACPI code from the Qualcomm DWC3 glue driver to aid in the
>> implementation of improvements that are actually used like multiport and
>> flattening device tree.
> 
> With a simple lookup function that returns the ACPI index based on name
> this shouldn't be required to add multiport support even if it may
> simplify it slightly. But IIRC it would help more with the devicetree
> binding rework.
>   

I agree to both your comments.
Actually both series are equally important to me. Adding a lookup 
function must ACPI index must help multiport code without this patch as 
well. But removing this is helping in multiport to write better code and 
definitely helps in flattening series.

>> Commit message by Bjorn Andersson.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/dwc3-qcom.c | 273 ++---------------------------------
>>   1 file changed, 11 insertions(+), 262 deletions(-)
> 
> You should update the Kconfig entry for USB_DWC3_QCOM as well and drop
> the ACPI dependency.

Missed it. Thanks for the catch. The following is the one right ?

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 5fc27b20df63..31078f3d41b8 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -131,7 +131,7 @@ config USB_DWC3_QCOM
         tristate "Qualcomm Platform"
         depends on ARCH_QCOM || COMPILE_TEST
         depends on EXTCON || !EXTCON
-       depends on (OF || ACPI)
+       depends on OF


> 
>>   static int dwc3_qcom_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node	*np = pdev->dev.of_node;
>>   	struct device		*dev = &pdev->dev;
>>   	struct dwc3_qcom	*qcom;
>>   	struct resource		*res, *parent_res = NULL;
> 
> You should drop parent_res as well.

Ahh, thanks for the catch.

> 
>> -	struct resource		local_res;
>>   	int			ret, i;
>>   	bool			ignore_pipe_clk;
>>   	bool			wakeup_source;
>> @@ -825,14 +659,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   	platform_set_drvdata(pdev, qcom);
>>   	qcom->dev = &pdev->dev;
>>   
>> -	if (has_acpi_companion(dev)) {
>> -		qcom->acpi_pdata = acpi_device_get_match_data(dev);
>> -		if (!qcom->acpi_pdata) {
>> -			dev_err(&pdev->dev, "no supporting ACPI device data\n");
>> -			return -EINVAL;
>> -		}
>> -	}
>> -
>>   	qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
>>   	if (IS_ERR(qcom->resets)) {
>>   		return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
>> @@ -860,41 +686,18 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   	}
>>   
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -
>> -	if (np) {
>> -		parent_res = res;
>> -	} else {
>> -		memcpy(&local_res, res, sizeof(struct resource));
>> -		parent_res = &local_res;
>> -
>> -		parent_res->start = res->start +
>> -			qcom->acpi_pdata->qscratch_base_offset;
>> -		parent_res->end = parent_res->start +
>> -			qcom->acpi_pdata->qscratch_base_size;
>> -
>> -		if (qcom->acpi_pdata->is_urs) {
>> -			qcom->urs_usb = dwc3_qcom_create_urs_usb_platdev(dev);
>> -			if (IS_ERR_OR_NULL(qcom->urs_usb)) {
>> -				dev_err(dev, "failed to create URS USB platdev\n");
>> -				if (!qcom->urs_usb)
>> -					ret = -ENODEV;
>> -				else
>> -					ret = PTR_ERR(qcom->urs_usb);
>> -				goto clk_disable;
>> -			}
>> -		}
>> -	}
>> +	parent_res = res;
>>   
>>   	qcom->qscratch_base = devm_ioremap_resource(dev, parent_res);
> 
> And just use res here.

ACK.

> 
>>   	if (IS_ERR(qcom->qscratch_base)) {
>>   		ret = PTR_ERR(qcom->qscratch_base);
>> -		goto free_urs;
>> +		goto clk_disable;
>> }
> 
> Looks good to me otherwise.

Thanks for the review. Wanted to reply to your comments on multiport 
series depending on how this patch goes in upstream.

Can I push out v2 or wait for a couple of days (as a standard practice 
before putting a new version) ?

Regards,
Krishna,