[PATCH] usb: chipidea: move ci_ulpi_init after the phy initialization

[PATCH] usb: chipidea: move ci_ulpi_init after the phy initialization

[Michael Grzeschik]

The function ci_usb_phy_init is already handling the
hw_phymode_configure path which is also only possible after we have
a valid phy. So we move the ci_ulpi_init after the phy initialization
to be really sure to be able to communicate with the ulpi phy.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/chipidea/core.c | 8 ++++----
 drivers/usb/chipidea/ulpi.c | 5 -----
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 835bf2428dc6e..bada13f704b62 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	ret = ci_ulpi_init(ci);
-	if (ret)
-		return ret;
-
 	if (ci->platdata->phy) {
 		ci->phy = ci->platdata->phy;
 	} else if (ci->platdata->usb_phy) {
@@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 		goto ulpi_exit;
 	}
 
+	ret = ci_ulpi_init(ci);
+	if (ret)
+		return ret;
+
 	ci->hw_bank.phys = res->start;
 
 	ci->irq = platform_get_irq(pdev, 0);
diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
index dfec07e8ae1d2..89fb51e2c3ded 100644
--- a/drivers/usb/chipidea/ulpi.c
+++ b/drivers/usb/chipidea/ulpi.c
@@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci)
 	if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
 		return 0;
 
-	/*
-	 * Set PORTSC correctly so we can read/write ULPI registers for
-	 * identification purposes
-	 */
-	hw_phymode_configure(ci);
 
 	ci->ulpi_ops.read = ci_ulpi_read;
 	ci->ulpi_ops.write = ci_ulpi_write;

---
base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848
change-id: 20240328-chipidea-phy-misc-b3f2bc814784

Best regards,
-- 
Michael Grzeschik <m.grzeschik@pengutronix.de>

Re: [PATCH] usb: chipidea: move ci_ulpi_init after the phy initialization

[Peter Chen]

On 24-04-02 08:23:43, Michael Grzeschik wrote:
> The function ci_usb_phy_init is already handling the
> hw_phymode_configure path which is also only possible after we have
> a valid phy. So we move the ci_ulpi_init after the phy initialization
> to be really sure to be able to communicate with the ulpi phy.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Acked-by: Peter Chen <peter.chen@kernel.org>

> ---
>  drivers/usb/chipidea/core.c | 8 ++++----
>  drivers/usb/chipidea/ulpi.c | 5 -----
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 835bf2428dc6e..bada13f704b62 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	ret = ci_ulpi_init(ci);
> -	if (ret)
> -		return ret;
> -
>  	if (ci->platdata->phy) {
>  		ci->phy = ci->platdata->phy;
>  	} else if (ci->platdata->usb_phy) {
> @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  		goto ulpi_exit;
>  	}
>  
> +	ret = ci_ulpi_init(ci);
> +	if (ret)
> +		return ret;
> +
>  	ci->hw_bank.phys = res->start;
>  
>  	ci->irq = platform_get_irq(pdev, 0);
> diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
> index dfec07e8ae1d2..89fb51e2c3ded 100644
> --- a/drivers/usb/chipidea/ulpi.c
> +++ b/drivers/usb/chipidea/ulpi.c
> @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci)
>  	if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
>  		return 0;
>  
> -	/*
> -	 * Set PORTSC correctly so we can read/write ULPI registers for
> -	 * identification purposes
> -	 */
> -	hw_phymode_configure(ci);
>  
>  	ci->ulpi_ops.read = ci_ulpi_read;
>  	ci->ulpi_ops.write = ci_ulpi_write;
> 
> ---
> base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848
> change-id: 20240328-chipidea-phy-misc-b3f2bc814784
> 
> Best regards,
> -- 
> Michael Grzeschik <m.grzeschik@pengutronix.de>
> 

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: chipidea: move ci_ulpi_init after the phy initialization

[Marek Szyprowski]

Dear All,

On 02.04.2024 08:23, Michael Grzeschik wrote:
> The function ci_usb_phy_init is already handling the
> hw_phymode_configure path which is also only possible after we have
> a valid phy. So we move the ci_ulpi_init after the phy initialization
> to be really sure to be able to communicate with the ulpi phy.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

This patch landed in linux-next some time ago as commit 22ffd399e6e7 
("usb: chipidea: move ci_ulpi_init after the phy initialization"). 
Unfortunately it breaks host USB operation on DragonBoard410c 
(arch/arm64/boot/dts/qcom/apq8016-sbc.dts). There is no error nor 
warning in the kernel log besides the information about deferred probe 
on the chipidea controller:

platform ci_hdrc.0: deferred probe pending: (reason unknown)

Tomorrow I will try to investigate which operation during driver probe 
triggers it. If there is anything else to check that might help fixing 
this issue, let me know.

> ---
>   drivers/usb/chipidea/core.c | 8 ++++----
>   drivers/usb/chipidea/ulpi.c | 5 -----
>   2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 835bf2428dc6e..bada13f704b62 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>   		return -ENODEV;
>   	}
>   
> -	ret = ci_ulpi_init(ci);
> -	if (ret)
> -		return ret;
> -
>   	if (ci->platdata->phy) {
>   		ci->phy = ci->platdata->phy;
>   	} else if (ci->platdata->usb_phy) {
> @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>   		goto ulpi_exit;
>   	}
>   
> +	ret = ci_ulpi_init(ci);
> +	if (ret)
> +		return ret;
> +
>   	ci->hw_bank.phys = res->start;
>   
>   	ci->irq = platform_get_irq(pdev, 0);
> diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
> index dfec07e8ae1d2..89fb51e2c3ded 100644
> --- a/drivers/usb/chipidea/ulpi.c
> +++ b/drivers/usb/chipidea/ulpi.c
> @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci)
>   	if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
>   		return 0;
>   
> -	/*
> -	 * Set PORTSC correctly so we can read/write ULPI registers for
> -	 * identification purposes
> -	 */
> -	hw_phymode_configure(ci);
>   
>   	ci->ulpi_ops.read = ci_ulpi_read;
>   	ci->ulpi_ops.write = ci_ulpi_write;
>
> ---
> base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848
> change-id: 20240328-chipidea-phy-misc-b3f2bc814784
>
> Best regards,

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: chipidea: move ci_ulpi_init after the phy initialization

[Alexandre Messier]

On 2024-04-25 15:40, Marek Szyprowski wrote:
> Dear All,
> 
> On 02.04.2024 08:23, Michael Grzeschik wrote:
>> The function ci_usb_phy_init is already handling the
>> hw_phymode_configure path which is also only possible after we have
>> a valid phy. So we move the ci_ulpi_init after the phy initialization
>> to be really sure to be able to communicate with the ulpi phy.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> This patch landed in linux-next some time ago as commit 22ffd399e6e7 
> ("usb: chipidea: move ci_ulpi_init after the phy initialization"). 
> Unfortunately it breaks host USB operation on DragonBoard410c 
> (arch/arm64/boot/dts/qcom/apq8016-sbc.dts). There is no error nor 
> warning in the kernel log besides the information about deferred probe 
> on the chipidea controller:
> 
> platform ci_hdrc.0: deferred probe pending: (reason unknown)
> 
> Tomorrow I will try to investigate which operation during driver probe 
> triggers it. If there is anything else to check that might help fixing 
> this issue, let me know.

I am also seeing this issue on a msm8974-based device.

There is also a report and analysis by Wouter Franken here:
https://lore.kernel.org/all/ecb8d3e8-d525-4a2d-a868-803202c16296@franken-peeters.be/

Reverting the commit does fix the issue for me.

Adding to regzbot for tracking:

#regzbot introduced: 22ffd399e6e7 ^

Thanks,
Alexandre

> 
>> ---
>>   drivers/usb/chipidea/core.c | 8 ++++----
>>   drivers/usb/chipidea/ulpi.c | 5 -----
>>   2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index 835bf2428dc6e..bada13f704b62 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>   		return -ENODEV;
>>   	}
>>   
>> -	ret = ci_ulpi_init(ci);
>> -	if (ret)
>> -		return ret;
>> -
>>   	if (ci->platdata->phy) {
>>   		ci->phy = ci->platdata->phy;
>>   	} else if (ci->platdata->usb_phy) {
>> @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>   		goto ulpi_exit;
>>   	}
>>   
>> +	ret = ci_ulpi_init(ci);
>> +	if (ret)
>> +		return ret;
>> +
>>   	ci->hw_bank.phys = res->start;
>>   
>>   	ci->irq = platform_get_irq(pdev, 0);
>> diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
>> index dfec07e8ae1d2..89fb51e2c3ded 100644
>> --- a/drivers/usb/chipidea/ulpi.c
>> +++ b/drivers/usb/chipidea/ulpi.c
>> @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci)
>>   	if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
>>   		return 0;
>>   
>> -	/*
>> -	 * Set PORTSC correctly so we can read/write ULPI registers for
>> -	 * identification purposes
>> -	 */
>> -	hw_phymode_configure(ci);
>>   
>>   	ci->ulpi_ops.read = ci_ulpi_read;
>>   	ci->ulpi_ops.write = ci_ulpi_write;
>>
>> ---
>> base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848
>> change-id: 20240328-chipidea-phy-misc-b3f2bc814784
>>
>> Best regards,
> 
> Best regards

Re: [PATCH] usb: chipidea: move ci_ulpi_init after the phy initialization

[Linux regression tracking (Thorsten Leemhuis)]

On 30.05.24 08:38, Alexandre Messier wrote:
> On 2024-04-25 15:40, Marek Szyprowski wrote:
>> On 02.04.2024 08:23, Michael Grzeschik wrote:
>>> The function ci_usb_phy_init is already handling the
>>> hw_phymode_configure path which is also only possible after we have
>>> a valid phy. So we move the ci_ulpi_init after the phy initialization
>>> to be really sure to be able to communicate with the ulpi phy.
>>>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> This patch landed in linux-next some time ago as commit 22ffd399e6e7 
>> ("usb: chipidea: move ci_ulpi_init after the phy initialization"). 
>> Unfortunately it breaks host USB operation on DragonBoard410c 
>> (arch/arm64/boot/dts/qcom/apq8016-sbc.dts). There is no error nor 
>> warning in the kernel log besides the information about deferred probe 
>> on the chipidea controller:
> 
> I am also seeing this issue on a msm8974-based device.
> 
> There is also a report and analysis by Wouter Franken here:
> https://lore.kernel.org/all/ecb8d3e8-d525-4a2d-a868-803202c16296@franken-peeters.be/

TWIMC, as this is easy to miss (I was extremely close to writing a
status inquiry instead of writing what I'm typing now): Greg applied
that revert two days ago and I assume he'll soon send it to Linus.

Ciao, Thorsten

#regzbot fix: 718d4a63c0a62d16af1d0425d515d7e76f35681e
#regzbot ignore-activity

Re: [PATCH] usb: chipidea: move ci_ulpi_init after the phy initialization

[Wouter Franken]

Hello all,

On 4/25/24 21:40, Marek Szyprowski wrote:
> Dear All,
>
> On 02.04.2024 08:23, Michael Grzeschik wrote:
>> The function ci_usb_phy_init is already handling the
>> hw_phymode_configure path which is also only possible after we have
>> a valid phy. So we move the ci_ulpi_init after the phy initialization
>> to be really sure to be able to communicate with the ulpi phy.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> This patch landed in linux-next some time ago as commit 22ffd399e6e7
> ("usb: chipidea: move ci_ulpi_init after the phy initialization").
> Unfortunately it breaks host USB operation on DragonBoard410c
> (arch/arm64/boot/dts/qcom/apq8016-sbc.dts). There is no error nor
> warning in the kernel log besides the information about deferred probe
> on the chipidea controller:
>
> platform ci_hdrc.0: deferred probe pending: (reason unknown)
>
> Tomorrow I will try to investigate which operation during driver probe
> triggers it. If there is anything else to check that might help fixing
> this issue, let me know.

I have the same problem on sony yuga 
(arch/arm/boot/dts/qcom/qcom-apq8064-sony-xperia-lagan-yuga.dts):
it breaks USB operation and shows the same message in the kernel log.

I have analyzed this issue and came to the following conclusion:

>> ---
>>    drivers/usb/chipidea/core.c | 8 ++++----
>>    drivers/usb/chipidea/ulpi.c | 5 -----
>>    2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index 835bf2428dc6e..bada13f704b62 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>    		return -ENODEV;
>>    	}
>>    
>> -	ret = ci_ulpi_init(ci);
>> -	if (ret)
>> -		return ret;
>> -

This ci_ulpi_init(...) call eventually calls probe(..) on the USB phy 
when the ulpi driver
registers all its child nodes. A couple lines later devm_phy_get(..) is 
called
to get the initialized phy device. Due to the move of the 
ci_ulpi_init(..) the phy
is not probed and the devm_phy_get(..) call fails with EPROBE_DEFER. 
This results
in a deadlock.

I don't understand the complete train of thought behind this patch to 
propose a fix
but hopefully my investigation can help to solve the issue.

>>    	if (ci->platdata->phy) {
>>    		ci->phy = ci->platdata->phy;
>>    	} else if (ci->platdata->usb_phy) {
>> @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>    		goto ulpi_exit;
>>    	}
>>    
>> +	ret = ci_ulpi_init(ci);
>> +	if (ret)
>> +		return ret;
>> +
>>    	ci->hw_bank.phys = res->start;
>>    
>>    	ci->irq = platform_get_irq(pdev, 0);
>> diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
>> index dfec07e8ae1d2..89fb51e2c3ded 100644
>> --- a/drivers/usb/chipidea/ulpi.c
>> +++ b/drivers/usb/chipidea/ulpi.c
>> @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci)
>>    	if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
>>    		return 0;
>>    
>> -	/*
>> -	 * Set PORTSC correctly so we can read/write ULPI registers for
>> -	 * identification purposes
>> -	 */
>> -	hw_phymode_configure(ci);
>>    
>>    	ci->ulpi_ops.read = ci_ulpi_read;
>>    	ci->ulpi_ops.write = ci_ulpi_write;
>>
>> ---
>> base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848
>> change-id: 20240328-chipidea-phy-misc-b3f2bc814784
>>
>> Best regards,
> Best regards

Best regards
--
Wouter Franken