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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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