* [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout @ 2022-11-09 22:17 Ferry Toth 2022-11-10 0:06 ` Thinh Nguyen 2022-11-10 13:06 ` Heikki Krogerus 0 siblings, 2 replies; 8+ messages in thread From: Ferry Toth @ 2022-11-09 22:17 UTC (permalink / raw) To: linux-usb, linux-kernel Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Stephen Boyd, Andrey Smirnov, Andy Shevchenko, Ferry Toth 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; + } 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); + ret = -EPROBE_DEFER; goto err0; + } dwc->ulpi_ready = true; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-11-09 22:17 [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout Ferry Toth @ 2022-11-10 0:06 ` Thinh Nguyen 2022-11-10 12:45 ` Ferry Toth 2022-11-10 13:06 ` Heikki Krogerus 1 sibling, 1 reply; 8+ messages in thread From: Thinh Nguyen @ 2022-11-10 0:06 UTC (permalink / raw) To: Ferry Toth Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Stephen Boyd, Andrey Smirnov, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-11-10 0:06 ` Thinh Nguyen @ 2022-11-10 12:45 ` Ferry Toth 2022-11-10 20:38 ` Ferry Toth 0 siblings, 1 reply; 8+ messages in thread From: Ferry Toth @ 2022-11-10 12:45 UTC (permalink / raw) To: Thinh Nguyen Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko (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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-11-10 12:45 ` Ferry Toth @ 2022-11-10 20:38 ` Ferry Toth 2022-11-11 1:31 ` Thinh Nguyen 0 siblings, 1 reply; 8+ messages in thread From: Ferry Toth @ 2022-11-10 20:38 UTC (permalink / raw) To: Thinh Nguyen Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-11-10 20:38 ` Ferry Toth @ 2022-11-11 1:31 ` Thinh Nguyen 2022-11-18 22:29 ` Ferry Toth 0 siblings, 1 reply; 8+ messages in thread From: Thinh Nguyen @ 2022-11-11 1:31 UTC (permalink / raw) To: Ferry Toth Cc: Thinh Nguyen, linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-11-11 1:31 ` Thinh Nguyen @ 2022-11-18 22:29 ` Ferry Toth 2022-11-18 23:55 ` Thinh Nguyen 0 siblings, 1 reply; 8+ messages in thread From: Ferry Toth @ 2022-11-18 22:29 UTC (permalink / raw) To: Thinh Nguyen Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-11-18 22:29 ` Ferry Toth @ 2022-11-18 23:55 ` Thinh Nguyen 0 siblings, 0 replies; 8+ messages in thread From: Thinh Nguyen @ 2022-11-18 23:55 UTC (permalink / raw) To: Ferry Toth Cc: Thinh Nguyen, linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-11-09 22:17 [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout Ferry Toth 2022-11-10 0:06 ` Thinh Nguyen @ 2022-11-10 13:06 ` Heikki Krogerus 1 sibling, 0 replies; 8+ messages in thread From: Heikki Krogerus @ 2022-11-10 13:06 UTC (permalink / raw) To: Ferry Toth Cc: linux-usb, linux-kernel, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Stephen Boyd, Andrey Smirnov, Andy Shevchenko 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, -- heikki ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-11-19 0:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-09 22:17 [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id timeout Ferry Toth 2022-11-10 0:06 ` Thinh Nguyen 2022-11-10 12:45 ` Ferry Toth 2022-11-10 20:38 ` Ferry Toth 2022-11-11 1:31 ` Thinh Nguyen 2022-11-18 22:29 ` Ferry Toth 2022-11-18 23:55 ` Thinh Nguyen 2022-11-10 13:06 ` Heikki Krogerus
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.