[v1,1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

  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. Due to the timeout being masked dwc3 probe continues by
calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which happens
to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally succeeds.

This patch changes ulpi_read_id() to return -ETIMEDOUT when it occurs and
catches the error in dwc3_core_init(). It handles the error by calling
dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On deferred
probe ulpi_read_id() again succeeds.

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 drivers/usb/common/ulpi.c | 5 +++--
 drivers/usb/dwc3/core.c   | 5 ++++-
 2 files changed, 7 insertions(+), 3 deletions(-)
  

Hi Ferry,

On Wed, Nov 09, 2022, Ferry Toth wrote:
> 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. Due to the timeout being masked dwc3 probe continues by
> calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which happens
> to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally succeeds.
> 
> This patch changes ulpi_read_id() to return -ETIMEDOUT when it occurs and
> catches the error in dwc3_core_init(). It handles the error by calling
> dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On deferred
> probe ulpi_read_id() again succeeds.
> 
> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> ---
>  drivers/usb/common/ulpi.c | 5 +++--
>  drivers/usb/dwc3/core.c   | 5 ++++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 

Can you split the dwc3 change and ulpi change to separate patches?

> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index d7c8461976ce..d8f22bc2f9d0 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
>  
>  	/* Test the interface */
>  	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> -	if (ret < 0)
> -		goto err;
> +	if (ret < 0) {
> +		return ret;
> +	}
>  
>  	ret = ulpi_read(ulpi, ULPI_SCRATCH);
>  	if (ret < 0)
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 648f1c570021..e293ef70039b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  
>  	if (!dwc->ulpi_ready) {
>  		ret = dwc3_core_ulpi_init(dwc);
> -		if (ret)
> +		if (ret) {
> +			dwc3_core_soft_reset(dwc);

We shouldn't need to do soft reset here. The controller shouldn't be at
a bad/incorrect state at this point to warrant a soft-reset. There will
be a soft-reset when it goes through the initialization again.

> +			ret = -EPROBE_DEFER;

We shouldn't automatically set every error status to correspond to
-EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
any other?).

>  			goto err0;
> +		}
>  		dwc->ulpi_ready = true;
>  	}
>  
> -- 
> 2.34.1
> 

Thanks,
Thinh
  

(sorry sent html with previous attempt)

On 10-11-2022 01:06, Thinh Nguyen wrote:
> Hi Ferry,
>
> On Wed, Nov 09, 2022, Ferry Toth wrote:
>> 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. Due to the timeout being masked dwc3 probe continues by
>> calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which happens
>> to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally succeeds.
>>
>> This patch changes ulpi_read_id() to return -ETIMEDOUT when it occurs and
>> catches the error in dwc3_core_init(). It handles the error by calling
>> dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On deferred
>> probe ulpi_read_id() again succeeds.
>>
>> Signed-off-by: Ferry Toth<ftoth@exalondelft.nl>
>> ---
>>   drivers/usb/common/ulpi.c | 5 +++--
>>   drivers/usb/dwc3/core.c   | 5 ++++-
>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>
> Can you split the dwc3 change and ulpi change to separate patches?

Thanks for your comments.

I will send v2

>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>> index d7c8461976ce..d8f22bc2f9d0 100644
>> --- a/drivers/usb/common/ulpi.c
>> +++ b/drivers/usb/common/ulpi.c
>> @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
>>   
>>   	/* Test the interface */
>>   	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
>> -	if (ret < 0)
>> -		goto err;
>> +	if (ret < 0) {
>> +		return ret;
>> +	}
>>   
>>   	ret = ulpi_read(ulpi, ULPI_SCRATCH);
>>   	if (ret < 0)
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 648f1c570021..e293ef70039b 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   
>>   	if (!dwc->ulpi_ready) {
>>   		ret = dwc3_core_ulpi_init(dwc);
>> -		if (ret)
>> +		if (ret) {
>> +			dwc3_core_soft_reset(dwc);
> We shouldn't need to do soft reset here. The controller shouldn't be at
> a bad/incorrect state at this point to warrant a soft-reset. There will
> be a soft-reset when it goes through the initialization again.

It doesn't go through the initialization again unless we set 
-EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it 
will goto err0 here, so skips dwc3_core_soft_reset.

Do you mean you prefer something like:

if (ret) {

     if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;

     else goto err0;

}

>> +			ret = -EPROBE_DEFER;
> We shouldn't automatically set every error status to correspond to
> -EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
> any other?).
Other could be -ENOMEM. I think no need to do any new handling for that.
>>   			goto err0;
>> +		}
>>   		dwc->ulpi_ready = true;
>>   	}
>>   
>> -- 
>> 2.34.1
>>
> Thanks,
> Thinh
  

On Wed, Nov 09, 2022 at 11:17:49PM +0100, Ferry Toth wrote:
> 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. Due to the timeout being masked dwc3 probe continues by
> calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which happens
> to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally succeeds.
> 
> This patch changes ulpi_read_id() to return -ETIMEDOUT when it occurs and
> catches the error in dwc3_core_init(). It handles the error by calling
> dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On deferred
> probe ulpi_read_id() again succeeds.
> 
> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> ---
>  drivers/usb/common/ulpi.c | 5 +++--
>  drivers/usb/dwc3/core.c   | 5 ++++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index d7c8461976ce..d8f22bc2f9d0 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
>  
>  	/* Test the interface */
>  	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> -	if (ret < 0)
> -		goto err;
> +	if (ret < 0) {
> +		return ret;
> +	}

No need for the curly brackets.

thanks,
  

Hi

Op 10-11-2022 om 13:45 schreef Ferry Toth:
> (sorry sent html with previous attempt)
> 
> On 10-11-2022 01:06, Thinh Nguyen wrote:
>> Hi Ferry,
>>
>> On Wed, Nov 09, 2022, Ferry Toth wrote:
>>> 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. Due to the timeout being masked dwc3 probe continues by
>>> calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which 
>>> happens
>>> to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally 
>>> succeeds.
>>>
>>> This patch changes ulpi_read_id() to return -ETIMEDOUT when it occurs 
>>> and
>>> catches the error in dwc3_core_init(). It handles the error by calling
>>> dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On 
>>> deferred
>>> probe ulpi_read_id() again succeeds.
>>>
>>> Signed-off-by: Ferry Toth<ftoth@exalondelft.nl>
>>> ---
>>>   drivers/usb/common/ulpi.c | 5 +++--
>>>   drivers/usb/dwc3/core.c   | 5 ++++-
>>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>>
>> Can you split the dwc3 change and ulpi change to separate patches?
> 
> Thanks for your comments.
> 
> I will send v2
> 
>>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>>> index d7c8461976ce..d8f22bc2f9d0 100644
>>> --- a/drivers/usb/common/ulpi.c
>>> +++ b/drivers/usb/common/ulpi.c
>>> @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
>>>       /* Test the interface */
>>>       ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
>>> -    if (ret < 0)
>>> -        goto err;
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>>       ret = ulpi_read(ulpi, ULPI_SCRATCH);
>>>       if (ret < 0)
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 648f1c570021..e293ef70039b 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>>       if (!dwc->ulpi_ready) {
>>>           ret = dwc3_core_ulpi_init(dwc);
>>> -        if (ret)
>>> +        if (ret) {
>>> +            dwc3_core_soft_reset(dwc);
>> We shouldn't need to do soft reset here. The controller shouldn't be at
>> a bad/incorrect state at this point to warrant a soft-reset. There will
>> be a soft-reset when it goes through the initialization again.
> 
> It doesn't go through the initialization again unless we set 
> -EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it 
> will goto err0 here, so skips dwc3_core_soft_reset.
> 
> Do you mean you prefer something like:
> 
> if (ret) {
> 
>      if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;
> 
>      else goto err0;
> 
> }

I just tested, and calling dwc3_core_soft_reset() proves to be necessary 
as we need to goto err0 directly after. Else ret is overwritten and 
-EPROBE_DEFER lost.

>>> +            ret = -EPROBE_DEFER;
>> We shouldn't automatically set every error status to correspond to
>> -EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
>> any other?).
> Other could be -ENOMEM. I think no need to do any new handling for that.
>>>               goto err0;
>>> +        }
>>>           dwc->ulpi_ready = true;
>>>       }
>>> -- 
>>> 2.34.1
>>>
>> Thanks,
>> Thinh
  

On Thu, Nov 10, 2022, Ferry Toth wrote:
> Hi
> 
> Op 10-11-2022 om 13:45 schreef Ferry Toth:
> > (sorry sent html with previous attempt)
> > 
> > On 10-11-2022 01:06, Thinh Nguyen wrote:
> > > Hi Ferry,
> > > 
> > > On Wed, Nov 09, 2022, Ferry Toth wrote:
> > > > 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. Due to the timeout being masked dwc3 probe continues by
> > > > calling dwc3_core_soft_reset() followed by dwc3_get_extcon()
> > > > which happens
> > > > to return -EPROBE_DEFER. On deferred probe ulpi_read_id()
> > > > finally succeeds.
> > > > 
> > > > This patch changes ulpi_read_id() to return -ETIMEDOUT when it
> > > > occurs and
> > > > catches the error in dwc3_core_init(). It handles the error by calling
> > > > dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On
> > > > deferred
> > > > probe ulpi_read_id() again succeeds.
> > > > 
> > > > Signed-off-by: Ferry Toth<ftoth@exalondelft.nl>
> > > > ---
> > > >   drivers/usb/common/ulpi.c | 5 +++--
> > > >   drivers/usb/dwc3/core.c   | 5 ++++-
> > > >   2 files changed, 7 insertions(+), 3 deletions(-)
> > > > 
> > > Can you split the dwc3 change and ulpi change to separate patches?
> > 
> > Thanks for your comments.
> > 
> > I will send v2
> > 
> > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > index d7c8461976ce..d8f22bc2f9d0 100644
> > > > --- a/drivers/usb/common/ulpi.c
> > > > +++ b/drivers/usb/common/ulpi.c
> > > > @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
> > > >       /* Test the interface */
> > > >       ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> > > > -    if (ret < 0)
> > > > -        goto err;
> > > > +    if (ret < 0) {
> > > > +        return ret;
> > > > +    }
> > > >       ret = ulpi_read(ulpi, ULPI_SCRATCH);
> > > >       if (ret < 0)
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 648f1c570021..e293ef70039b 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
> > > >       if (!dwc->ulpi_ready) {
> > > >           ret = dwc3_core_ulpi_init(dwc);
> > > > -        if (ret)
> > > > +        if (ret) {
> > > > +            dwc3_core_soft_reset(dwc);
> > > We shouldn't need to do soft reset here. The controller shouldn't be at
> > > a bad/incorrect state at this point to warrant a soft-reset. There will
> > > be a soft-reset when it goes through the initialization again.
> > 
> > It doesn't go through the initialization again unless we set
> > -EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it
> > will goto err0 here, so skips dwc3_core_soft_reset.
> > 
> > Do you mean you prefer something like:
> > 
> > if (ret) {
> > 
> >      if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;
> > 
> >      else goto err0;

Why "else"? But I saw you remove that in the new patch.

> > 
> > }
> 
> I just tested, and calling dwc3_core_soft_reset() proves to be necessary as
> we need to goto err0 directly after. Else ret is overwritten and
> -EPROBE_DEFER lost.

Looks like there's a strange dependency problem here.
 * The setup needs a soft-reset before ulpi registration
 * The ulpi registration needs to go before the phy initialization
 * The soft-reset should be called after the phy initialization

I can't explain the actual issue here, and we can't debug further
because to look into it further would require looking at internal
signals.

This soft-reset and -EPROBE_DEFER seems to be a workaround to this
dependency problem. Instead of using -EPROBE_DEFER, can you do this:

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2f0a9679686f..5a1aaf3741ec 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1097,6 +1097,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
                goto err0;
 
        if (!dwc->ulpi_ready) {
+               /* Add comment */
+               dwc3_core_soft_reset(dwc);
                ret = dwc3_core_ulpi_init(dwc);
                if (ret)
                        goto err0;


> 
> > > > +            ret = -EPROBE_DEFER;
> > > We shouldn't automatically set every error status to correspond to
> > > -EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
> > > any other?).
> > Other could be -ENOMEM. I think no need to do any new handling for that.

There's also -EINVAL and some others too. Regardless, if it's -ENOMEM,
then it should not return -EPROBE_DEFER.

> > > >               goto err0;
> > > > +        }
> > > >           dwc->ulpi_ready = true;
> > > >       }
> > > > 

Thanks,
Thinh
  

Hi,

Op 11-11-2022 om 02:31 schreef Thinh Nguyen:
> On Thu, Nov 10, 2022, Ferry Toth wrote:
>> Hi
>>
>> Op 10-11-2022 om 13:45 schreef Ferry Toth:
>>> (sorry sent html with previous attempt)
>>>
>>> On 10-11-2022 01:06, Thinh Nguyen wrote:
>>>> Hi Ferry,
>>>>
>>>> On Wed, Nov 09, 2022, Ferry Toth wrote:
>>>>> 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. Due to the timeout being masked dwc3 probe continues by
>>>>> calling dwc3_core_soft_reset() followed by dwc3_get_extcon()
>>>>> which happens
>>>>> to return -EPROBE_DEFER. On deferred probe ulpi_read_id()
>>>>> finally succeeds.
>>>>>
>>>>> This patch changes ulpi_read_id() to return -ETIMEDOUT when it
>>>>> occurs and
>>>>> catches the error in dwc3_core_init(). It handles the error by calling
>>>>> dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On
>>>>> deferred
>>>>> probe ulpi_read_id() again succeeds.
>>>>>
>>>>> Signed-off-by: Ferry Toth<ftoth@exalondelft.nl>
>>>>> ---
>>>>>    drivers/usb/common/ulpi.c | 5 +++--
>>>>>    drivers/usb/dwc3/core.c   | 5 ++++-
>>>>>    2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>> Can you split the dwc3 change and ulpi change to separate patches?
>>>
>>> Thanks for your comments.
>>>
>>> I will send v2
>>>
>>>>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>>>>> index d7c8461976ce..d8f22bc2f9d0 100644
>>>>> --- a/drivers/usb/common/ulpi.c
>>>>> +++ b/drivers/usb/common/ulpi.c
>>>>> @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
>>>>>        /* Test the interface */
>>>>>        ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
>>>>> -    if (ret < 0)
>>>>> -        goto err;
>>>>> +    if (ret < 0) {
>>>>> +        return ret;
>>>>> +    }
>>>>>        ret = ulpi_read(ulpi, ULPI_SCRATCH);
>>>>>        if (ret < 0)
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 648f1c570021..e293ef70039b 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>>>>        if (!dwc->ulpi_ready) {
>>>>>            ret = dwc3_core_ulpi_init(dwc);
>>>>> -        if (ret)
>>>>> +        if (ret) {
>>>>> +            dwc3_core_soft_reset(dwc);
>>>> We shouldn't need to do soft reset here. The controller shouldn't be at
>>>> a bad/incorrect state at this point to warrant a soft-reset. There will
>>>> be a soft-reset when it goes through the initialization again.
>>>
>>> It doesn't go through the initialization again unless we set
>>> -EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it
>>> will goto err0 here, so skips dwc3_core_soft_reset.
>>>
>>> Do you mean you prefer something like:
>>>
>>> if (ret) {
>>>
>>>       if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;
>>>
>>>       else goto err0;
> 
> Why "else"? But I saw you remove that in the new patch.
> 
>>>
>>> }
>>
>> I just tested, and calling dwc3_core_soft_reset() proves to be necessary as
>> we need to goto err0 directly after. Else ret is overwritten and
>> -EPROBE_DEFER lost.
> 
> Looks like there's a strange dependency problem here.
>   * The setup needs a soft-reset before ulpi registration
>   * The ulpi registration needs to go before the phy initialization
>   * The soft-reset should be called after the phy initialization
> 
> I can't explain the actual issue here, and we can't debug further
> because to look into it further would require looking at internal
> signals.
> 
> This soft-reset and -EPROBE_DEFER seems to be a workaround to this
> dependency problem. Instead of using -EPROBE_DEFER, can you do this:
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2f0a9679686f..5a1aaf3741ec 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1097,6 +1097,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
>                  goto err0;
>   
>          if (!dwc->ulpi_ready) {
> +               /* Add comment */
> +               dwc3_core_soft_reset(dwc);
>                  ret = dwc3_core_ulpi_init(dwc);
>                  if (ret)
>                          goto err0;
> 

This indeed fixes the issue as well. Here is the trace:

# tracer: function_graph
#
# CPU  DURATION                  FUNCTION CALLS
# |     |   |                     |   |   |   |
  0)               |  /* start_event: (dwc3_probe+0x0/0x1910) */
  0)   7.070 us    |  dwc3_clk_enable.part.0();
  0)   5.480 us    |  extcon_get_extcon_dev();
  0) + 10.230 us   |  dwc3_runtime_idle();
  0)               |  /* end_event: (platform_probe+0x3f/0xa0 <- 
dwc3_probe) */

** multiple defers while waiting for extcon

  0)               |  /* start_event: (dwc3_probe+0x0/0x1910) */
  0)   7.320 us    |  dwc3_clk_enable.part.0();
  0)   6.830 us    |  extcon_get_extcon_dev();
  0)               |  dwc3_core_init() {
  0) + 29.200 us   |    dwc3_core_soft_reset.part.0();
  0)               |    dwc3_ulpi_init() {
  0)               |      ulpi_register_interface() {
  0)               |        dwc3_ulpi_write() {
  0)   3.380 us    |          dwc3_ulpi_busyloop();

  ** without this patch this one times out after 10000us

  0)   7.710 us    |        }
  0)               |        dwc3_ulpi_read() {
  0)   3.060 us    |          dwc3_ulpi_busyloop();
  0)   7.210 us    |        }
  0)               |        dwc3_ulpi_read() {
  0)   2.830 us    |          dwc3_ulpi_busyloop();
  0)   6.690 us    |        }
  0)               |        dwc3_ulpi_read() {
  0)   2.880 us    |          dwc3_ulpi_busyloop();
  0)   6.670 us    |        }
  0)               |        dwc3_ulpi_read() {
  0)   2.940 us    |          dwc3_ulpi_busyloop();
  0)   6.690 us    |        }
  0)               |        dwc3_ulpi_read() {
  0)   2.870 us    |          dwc3_ulpi_busyloop();
  0)   6.620 us    |        }
  0) + 18.150 us   |        ulpi_uevent();
  0)   5.990 us    |        ulpi_match();
  0)               |        ulpi_probe() {
  0)               |          tusb1210_probe() {
  0)               |            ulpi_read() {
  0)               |              dwc3_ulpi_read() {
  0)   4.440 us    |                dwc3_ulpi_busyloop();
  0)   9.600 us    |              }
  0) + 15.770 us   |            }
  0)               |            ulpi_write() {
  0)               |              dwc3_ulpi_write() {
  0)   3.270 us    |                dwc3_ulpi_busyloop();
  0)   6.820 us    |              }
  0) + 11.020 us   |            }
  0) ! 407.540 us  |          }
  0) ! 416.980 us  |        }
  0)   9.800 us    |        ulpi_uevent();
  0) * 18604.00 us |      }
  0) * 18611.20 us |    }
  0) + 30.570 us   |    dwc3_core_soft_reset.part.0();
  0)               |    tusb1210_power_on() {
  1)               |  extcon_set_state_sync() {
  1)   5.330 us    |    extcon_set_state.part.0();
  1) + 90.550 us   |    extcon_sync.part.0();
  1) ! 113.670 us  |  }
  1) + 19.450 us   |  ulpi_uevent();
  0) + 13.640 us   |  ulpi_uevent();
  1) + 13.980 us   |  ulpi_uevent();
  0) + 15.960 us   |  ulpi_uevent();
  0)               |      ulpi_write() {
  0)               |        dwc3_ulpi_write() {
  0) * 10239.47 us |          dwc3_ulpi_busyloop();
  0) * 10250.57 us |        }
  0) * 10265.09 us |      }
  0) * 69518.95 us |    }
  0)   5.740 us    |    dwc3_event_buffers_setup();
  0) * 88241.02 us |  } /* dwc3_core_init */
  0) ! 104.900 us  |  dwc3_debugfs_init();
  0)               |  dwc3_drd_init() {
  0)   4.720 us    |    extcon_register_notifier();
  0)               |    extcon_get_state() {
  0)   2.640 us    |      extcon_get_state.part.0();
  0)   6.460 us    |    }
  0) + 14.460 us   |    dwc3_set_mode();
  0) + 43.300 us   |  }
  0)               |  /* end_event: (platform_probe+0x3f/0xa0 <- 
dwc3_probe) */

Maybe this is the preferred way to go if the dwc3_core_soft_reset() 
doesn't hurt other users?

>>
>>>>> +            ret = -EPROBE_DEFER;
>>>> We shouldn't automatically set every error status to correspond to
>>>> -EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
>>>> any other?).
>>> Other could be -ENOMEM. I think no need to do any new handling for that.
> 
> There's also -EINVAL and some others too. Regardless, if it's -ENOMEM,
> then it should not return -EPROBE_DEFER.
> 
>>>>>                goto err0;
>>>>> +        }
>>>>>            dwc->ulpi_ready = true;
>>>>>        }
>>>>>
> 
> Thanks,
> Thinh
  

On Fri, Nov 18, 2022, Ferry Toth wrote:
> Hi,
> 
> Op 11-11-2022 om 02:31 schreef Thinh Nguyen:
> > On Thu, Nov 10, 2022, Ferry Toth wrote:
> > > Hi
> > > 
> > > Op 10-11-2022 om 13:45 schreef Ferry Toth:
> > > > (sorry sent html with previous attempt)
> > > > 
> > > > On 10-11-2022 01:06, Thinh Nguyen wrote:
> > > > > Hi Ferry,
> > > > > 
> > > > > On Wed, Nov 09, 2022, Ferry Toth wrote:
> > > > > > 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. Due to the timeout being masked dwc3 probe continues by
> > > > > > calling dwc3_core_soft_reset() followed by dwc3_get_extcon()
> > > > > > which happens
> > > > > > to return -EPROBE_DEFER. On deferred probe ulpi_read_id()
> > > > > > finally succeeds.
> > > > > > 
> > > > > > This patch changes ulpi_read_id() to return -ETIMEDOUT when it
> > > > > > occurs and
> > > > > > catches the error in dwc3_core_init(). It handles the error by calling
> > > > > > dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On
> > > > > > deferred
> > > > > > probe ulpi_read_id() again succeeds.
> > > > > > 
> > > > > > Signed-off-by: Ferry Toth<ftoth@exalondelft.nl>
> > > > > > ---
> > > > > >    drivers/usb/common/ulpi.c | 5 +++--
> > > > > >    drivers/usb/dwc3/core.c   | 5 ++++-
> > > > > >    2 files changed, 7 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > Can you split the dwc3 change and ulpi change to separate patches?
> > > > 
> > > > Thanks for your comments.
> > > > 
> > > > I will send v2
> > > > 
> > > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > > > index d7c8461976ce..d8f22bc2f9d0 100644
> > > > > > --- a/drivers/usb/common/ulpi.c
> > > > > > +++ b/drivers/usb/common/ulpi.c
> > > > > > @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
> > > > > >        /* Test the interface */
> > > > > >        ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> > > > > > -    if (ret < 0)
> > > > > > -        goto err;
> > > > > > +    if (ret < 0) {
> > > > > > +        return ret;
> > > > > > +    }
> > > > > >        ret = ulpi_read(ulpi, ULPI_SCRATCH);
> > > > > >        if (ret < 0)
> > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > index 648f1c570021..e293ef70039b 100644
> > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
> > > > > >        if (!dwc->ulpi_ready) {
> > > > > >            ret = dwc3_core_ulpi_init(dwc);
> > > > > > -        if (ret)
> > > > > > +        if (ret) {
> > > > > > +            dwc3_core_soft_reset(dwc);
> > > > > We shouldn't need to do soft reset here. The controller shouldn't be at
> > > > > a bad/incorrect state at this point to warrant a soft-reset. There will
> > > > > be a soft-reset when it goes through the initialization again.
> > > > 
> > > > It doesn't go through the initialization again unless we set
> > > > -EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it
> > > > will goto err0 here, so skips dwc3_core_soft_reset.
> > > > 
> > > > Do you mean you prefer something like:
> > > > 
> > > > if (ret) {
> > > > 
> > > >       if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;
> > > > 
> > > >       else goto err0;
> > 
> > Why "else"? But I saw you remove that in the new patch.
> > 
> > > > 
> > > > }
> > > 
> > > I just tested, and calling dwc3_core_soft_reset() proves to be necessary as
> > > we need to goto err0 directly after. Else ret is overwritten and
> > > -EPROBE_DEFER lost.
> > 
> > Looks like there's a strange dependency problem here.
> >   * The setup needs a soft-reset before ulpi registration
> >   * The ulpi registration needs to go before the phy initialization
> >   * The soft-reset should be called after the phy initialization
> > 
> > I can't explain the actual issue here, and we can't debug further
> > because to look into it further would require looking at internal
> > signals.
> > 
> > This soft-reset and -EPROBE_DEFER seems to be a workaround to this
> > dependency problem. Instead of using -EPROBE_DEFER, can you do this:
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 2f0a9679686f..5a1aaf3741ec 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1097,6 +1097,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >                  goto err0;
> >          if (!dwc->ulpi_ready) {
> > +               /* Add comment */
> > +               dwc3_core_soft_reset(dwc);
> >                  ret = dwc3_core_ulpi_init(dwc);
> >                  if (ret)
> >                          goto err0;
> > 
> 
> This indeed fixes the issue as well. Here is the trace:

Thanks for the test!

> 
> # tracer: function_graph
> #
> # CPU  DURATION                  FUNCTION CALLS
> # |     |   |                     |   |   |   |
>  0)               |  /* start_event: (dwc3_probe+0x0/0x1910) */
>  0)   7.070 us    |  dwc3_clk_enable.part.0();
>  0)   5.480 us    |  extcon_get_extcon_dev();
>  0) + 10.230 us   |  dwc3_runtime_idle();
>  0)               |  /* end_event: (platform_probe+0x3f/0xa0 <- dwc3_probe)
> */
> 
> ** multiple defers while waiting for extcon
> 
>  0)               |  /* start_event: (dwc3_probe+0x0/0x1910) */
>  0)   7.320 us    |  dwc3_clk_enable.part.0();
>  0)   6.830 us    |  extcon_get_extcon_dev();
>  0)               |  dwc3_core_init() {
>  0) + 29.200 us   |    dwc3_core_soft_reset.part.0();
>  0)               |    dwc3_ulpi_init() {
>  0)               |      ulpi_register_interface() {
>  0)               |        dwc3_ulpi_write() {
>  0)   3.380 us    |          dwc3_ulpi_busyloop();
> 
>  ** without this patch this one times out after 10000us
> 
>  0)   7.710 us    |        }
>  0)               |        dwc3_ulpi_read() {
>  0)   3.060 us    |          dwc3_ulpi_busyloop();
>  0)   7.210 us    |        }
>  0)               |        dwc3_ulpi_read() {
>  0)   2.830 us    |          dwc3_ulpi_busyloop();
>  0)   6.690 us    |        }
>  0)               |        dwc3_ulpi_read() {
>  0)   2.880 us    |          dwc3_ulpi_busyloop();
>  0)   6.670 us    |        }
>  0)               |        dwc3_ulpi_read() {
>  0)   2.940 us    |          dwc3_ulpi_busyloop();
>  0)   6.690 us    |        }
>  0)               |        dwc3_ulpi_read() {
>  0)   2.870 us    |          dwc3_ulpi_busyloop();
>  0)   6.620 us    |        }
>  0) + 18.150 us   |        ulpi_uevent();
>  0)   5.990 us    |        ulpi_match();
>  0)               |        ulpi_probe() {
>  0)               |          tusb1210_probe() {
>  0)               |            ulpi_read() {
>  0)               |              dwc3_ulpi_read() {
>  0)   4.440 us    |                dwc3_ulpi_busyloop();
>  0)   9.600 us    |              }
>  0) + 15.770 us   |            }
>  0)               |            ulpi_write() {
>  0)               |              dwc3_ulpi_write() {
>  0)   3.270 us    |                dwc3_ulpi_busyloop();
>  0)   6.820 us    |              }
>  0) + 11.020 us   |            }
>  0) ! 407.540 us  |          }
>  0) ! 416.980 us  |        }
>  0)   9.800 us    |        ulpi_uevent();
>  0) * 18604.00 us |      }
>  0) * 18611.20 us |    }
>  0) + 30.570 us   |    dwc3_core_soft_reset.part.0();
>  0)               |    tusb1210_power_on() {
>  1)               |  extcon_set_state_sync() {
>  1)   5.330 us    |    extcon_set_state.part.0();
>  1) + 90.550 us   |    extcon_sync.part.0();
>  1) ! 113.670 us  |  }
>  1) + 19.450 us   |  ulpi_uevent();
>  0) + 13.640 us   |  ulpi_uevent();
>  1) + 13.980 us   |  ulpi_uevent();
>  0) + 15.960 us   |  ulpi_uevent();
>  0)               |      ulpi_write() {
>  0)               |        dwc3_ulpi_write() {
>  0) * 10239.47 us |          dwc3_ulpi_busyloop();
>  0) * 10250.57 us |        }
>  0) * 10265.09 us |      }
>  0) * 69518.95 us |    }
>  0)   5.740 us    |    dwc3_event_buffers_setup();
>  0) * 88241.02 us |  } /* dwc3_core_init */
>  0) ! 104.900 us  |  dwc3_debugfs_init();
>  0)               |  dwc3_drd_init() {
>  0)   4.720 us    |    extcon_register_notifier();
>  0)               |    extcon_get_state() {
>  0)   2.640 us    |      extcon_get_state.part.0();
>  0)   6.460 us    |    }
>  0) + 14.460 us   |    dwc3_set_mode();
>  0) + 43.300 us   |  }
>  0)               |  /* end_event: (platform_probe+0x3f/0xa0 <- dwc3_probe)
> */
> 
> Maybe this is the preferred way to go if the dwc3_core_soft_reset() doesn't
> hurt other users?
> 

The check you added seems to fit better for this behavior, which I'd
consider a quirk. We can revisit this change if the ulpi update doesn't
go through.

Thanks,
Thinh