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

[Ferry Toth <fntoth@gmail.com>]

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