| Message ID | 20221117205411.11489-3-ftoth@exalondelft.nl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: core: defer probe on ulpi_read_id timeout | expand |
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
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
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
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; }
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(-)