[PATCH v3 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[PATCH v3 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Ferry Toth]

v3:
- Correct commit message (Greg)
- Add fixes (Greg)

v2:
- Split into separate commits (Thinh)
- Only defer probe on -ETIMEDOUT (Thinh)
- Loose curly brackets (Heikki)

Ferry Toth (2):
  usb: ulpi: defer ulpi_register on ulpi_read_id timeout
  usb: dwc3: core: defer probe on ulpi_read_id timeout

 drivers/usb/common/ulpi.c | 2 +-
 drivers/usb/dwc3/core.c   | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.37.2

[PATCH v3 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

[Ferry Toth]

Since commit 0f010171
Dual Role support on Intel Merrifield platform broke due to rearranging
the call to dwc3_get_extcon().

It appears to be caused by ulpi_read_id() on the first test write failing
with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via
DT when the test write fails and returns 0 in that case even if DT does not
provide the phy. As a result usb probe completes without phy.

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 drivers/usb/common/ulpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index d7c8461976ce..60e8174686a1 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi)
 	/* Test the interface */
 	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
 	if (ret < 0)
-		goto err;
+		return ret;
 
 	ret = ulpi_read(ulpi, ULPI_SCRATCH);
 	if (ret < 0)
-- 
2.37.2

Re: [PATCH v3 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 09:54:10PM +0100, Ferry Toth wrote:
> Since commit 0f010171
> Dual Role support on Intel Merrifield platform broke due to rearranging
> the call to dwc3_get_extcon().

Not sure why format is broken, you may add into your ~/.gitconfig

	[core]
		abbrev = 12

	[alias]
		one = show -s --pretty='format:%h (\"%s\")'

and run

	git one 0f010171

with the result

	0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")

> It appears to be caused by ulpi_read_id() on the first test write failing
> with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via
> DT when the test write fails and returns 0 in that case even if DT does not
> provide the phy. As a result usb probe completes without phy.


-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Ferry Toth]

Since commit 0f010171
Dual Role support on Intel Merrifield platform broke due to rearranging
the call to dwc3_get_extcon().

It appears to be caused by ulpi_read_id() masking the timeout on the first
test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset()
followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER.
On deferred probe ulpi_read_id() finally succeeded.

As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we
need to handle the error by calling dwc3_core_soft_reset() and request
-EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds.

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 drivers/usb/dwc3/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 648f1c570021..2779f17bffaf 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1106,8 +1106,13 @@ static int dwc3_core_init(struct dwc3 *dwc)
 
 	if (!dwc->ulpi_ready) {
 		ret = dwc3_core_ulpi_init(dwc);
-		if (ret)
+		if (ret) {
+			if (ret == -ETIMEDOUT) {
+				dwc3_core_soft_reset(dwc);
+				ret = -EPROBE_DEFER;
+			}
 			goto err0;
+		}
 		dwc->ulpi_ready = true;
 	}
 
-- 
2.37.2

Re: [PATCH v3 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Thinh Nguyen]

On Thu, Nov 17, 2022, Ferry Toth wrote:
> Since commit 0f010171

I don't your update as noted in the v3 change (ie. Greg's suggestions).

> Dual Role support on Intel Merrifield platform broke due to rearranging
> the call to dwc3_get_extcon().
> 
> It appears to be caused by ulpi_read_id() masking the timeout on the first
> test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset()
> followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER.
> On deferred probe ulpi_read_id() finally succeeded.
> 
> As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we
> need to handle the error by calling dwc3_core_soft_reset() and request
> -EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds.
> 
> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> ---
>  drivers/usb/dwc3/core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 648f1c570021..2779f17bffaf 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1106,8 +1106,13 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  
>  	if (!dwc->ulpi_ready) {
>  		ret = dwc3_core_ulpi_init(dwc);
> -		if (ret)
> +		if (ret) {
> +			if (ret == -ETIMEDOUT) {
> +				dwc3_core_soft_reset(dwc);

I'm not sure if you saw my previous response. I wanted to check to see
if we need to do soft-reset once before ulpi init as part of its
initialization.

I'm just curious why Andrey Smirnov test doesn't require this change. If
it's a quirk for this specific configuration, then the change here is
fine. If it's required for all ULPI setup, then we can unconditionally
do that for all ULPI setup during initialization.

> +				ret = -EPROBE_DEFER;
> +			}
>  			goto err0;
> +		}
>  		dwc->ulpi_ready = true;
>  	}
>  
> -- 
> 2.37.2
> 

Thanks,
Thinh

Re: [PATCH v3 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Ferry Toth]

On 18-11-2022 03:11, Thinh Nguyen wrote:
> On Thu, Nov 17, 2022, Ferry Toth wrote:
>> Since commit 0f010171
> I don't your update as noted in the v3 change (ie. Greg's suggestions).

Indeed. I seem to have sent in the wrong sha. Sorry about this.

>> Dual Role support on Intel Merrifield platform broke due to rearranging
>> the call to dwc3_get_extcon().
>>
>> It appears to be caused by ulpi_read_id() masking the timeout on the first
>> test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset()
>> followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER.
>> On deferred probe ulpi_read_id() finally succeeded.
>>
>> As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we
>> need to handle the error by calling dwc3_core_soft_reset() and request
>> -EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds.
>>
>> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
>> ---
>>   drivers/usb/dwc3/core.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 648f1c570021..2779f17bffaf 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1106,8 +1106,13 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   
>>   	if (!dwc->ulpi_ready) {
>>   		ret = dwc3_core_ulpi_init(dwc);
>> -		if (ret)
>> +		if (ret) {
>> +			if (ret == -ETIMEDOUT) {
>> +				dwc3_core_soft_reset(dwc);
> I'm not sure if you saw my previous response. I wanted to check to see
> if we need to do soft-reset once before ulpi init as part of its
> initialization.
I missed it but found it now. I will try your suggestion and answer.
> I'm just curious why Andrey Smirnov test doesn't require this change. If
> it's a quirk for this specific configuration, then the change here is
> fine. If it's required for all ULPI setup, then we can unconditionally
> do that for all ULPI setup during initialization.

Yes me too. I can reproduce that when I build kernel and rootfs with 
buildroot there is no issue. But as soon as I add ftrace / bootconfig 
the issue appears and then I can't analyze the flow. It's seems some 
kind of timing / race.

However, I have tried deferring without dwc3_core_soft_reset() (to 
verify if it is just a matter of time) and that doesn't work. dwc3 is 
deferred about 10x and then gives up without phy. So, it's not just the 
time and not just a matter of retrying the test write.

>> +				ret = -EPROBE_DEFER;
>> +			}
>>   			goto err0;
>> +		}
>>   		dwc->ulpi_ready = true;
>>   	}
>>   
>> -- 
>> 2.37.2
>>
> Thanks,
> Thinh

Re: [PATCH v3 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Thinh Nguyen]

On Fri, Nov 18, 2022, Ferry Toth wrote:
> 
> On 18-11-2022 03:11, Thinh Nguyen wrote:
> > On Thu, Nov 17, 2022, Ferry Toth wrote:
> > > Since commit 0f010171
> > I don't your update as noted in the v3 change (ie. Greg's suggestions).
> 
> Indeed. I seem to have sent in the wrong sha. Sorry about this.
> 
> > > Dual Role support on Intel Merrifield platform broke due to rearranging
> > > the call to dwc3_get_extcon().
> > > 
> > > It appears to be caused by ulpi_read_id() masking the timeout on the first
> > > test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset()
> > > followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER.
> > > On deferred probe ulpi_read_id() finally succeeded.
> > > 
> > > As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we
> > > need to handle the error by calling dwc3_core_soft_reset() and request
> > > -EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds.
> > > 
> > > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> > > ---
> > >   drivers/usb/dwc3/core.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 648f1c570021..2779f17bffaf 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1106,8 +1106,13 @@ static int dwc3_core_init(struct dwc3 *dwc)
> > >   	if (!dwc->ulpi_ready) {
> > >   		ret = dwc3_core_ulpi_init(dwc);
> > > -		if (ret)
> > > +		if (ret) {
> > > +			if (ret == -ETIMEDOUT) {
> > > +				dwc3_core_soft_reset(dwc);
> > I'm not sure if you saw my previous response. I wanted to check to see
> > if we need to do soft-reset once before ulpi init as part of its
> > initialization.
> I missed it but found it now. I will try your suggestion and answer.

I think it may not be needed now from your experiments noted below.

> > I'm just curious why Andrey Smirnov test doesn't require this change. If
> > it's a quirk for this specific configuration, then the change here is
> > fine. If it's required for all ULPI setup, then we can unconditionally
> > do that for all ULPI setup during initialization.
> 
> Yes me too. I can reproduce that when I build kernel and rootfs with
> buildroot there is no issue. But as soon as I add ftrace / bootconfig the
> issue appears and then I can't analyze the flow. It's seems some kind of
> timing / race.

I think this is very useful info that should go into the commit message.

> 
> However, I have tried deferring without dwc3_core_soft_reset() (to verify if
> it is just a matter of time) and that doesn't work. dwc3 is deferred about
> 10x and then gives up without phy. So, it's not just the time and not just a
> matter of retrying the test write.

Even though we can't root cause the problem, this fix should be fine.

> 
> > > +				ret = -EPROBE_DEFER;
> > > +			}
> > >   			goto err0;
> > > +		}
> > >   		dwc->ulpi_ready = true;
> > >   	}
> > > -- 
> > > 2.37.2
> > > 

Thanks,
Thinh

Re: [PATCH v3 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 09:54:09PM +0100, Ferry Toth wrote:
> v3:
> - Correct commit message (Greg)

> - Add fixes (Greg)

Not sure what this means. I believe Greg asked for Fixes: tags in the
patch(es).

-- 
With Best Regards,
Andy Shevchenko