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

[Ferry Toth]

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@xxxxxxxxxxxxxx>
> > > ---
> > >   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