* [PATCH v2 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout
@ 2022-11-10 21:11 Ferry Toth
2022-11-10 21:11 ` [PATCH v2 1/2] usb: ulpi: defer ulpi_register " Ferry Toth
2022-11-10 21:11 ` [PATCH v2 2/2] usb: dwc3: core: defer probe " Ferry Toth
0 siblings, 2 replies; 7+ messages in thread
From: Ferry Toth @ 2022-11-10 21:11 UTC (permalink / raw)
To: linux-usb, linux-kernel
Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson,
Liu Shixin, Ferry Toth, Andrey Smirnov, Andy Shevchenko,
Ferry Toth
v2:
- Split into separate commits (Thinh)
- One defer probe on -ETIMEDOUT (Thinh)
- Loose curly brackets (Heikki)
Ferry Toth (2):
usb: ulpi: defer ulpi_register on ulpi_read_id timeout
usb: dwc3: core: defer probe on ulpi_read_id timeout
drivers/usb/common/ulpi.c | 2 +-
drivers/usb/dwc3/core.c | 7 ++++++-
2 files changed, 7 insertions(+), 2 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout
2022-11-10 21:11 [PATCH v2 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth
@ 2022-11-10 21:11 ` Ferry Toth
2022-11-11 6:08 ` Greg Kroah-Hartman
2022-11-11 14:04 ` Ferry Toth
2022-11-10 21:11 ` [PATCH v2 2/2] usb: dwc3: core: defer probe " Ferry Toth
1 sibling, 2 replies; 7+ messages in thread
From: Ferry Toth @ 2022-11-10 21:11 UTC (permalink / raw)
To: linux-usb, linux-kernel
Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson,
Liu Shixin, Ferry Toth, 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. As a result usb probe completes without phy.
Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
drivers/usb/common/ulpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index d7c8461976ce..60e8174686a1 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi)
/* Test the interface */
ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
if (ret < 0)
- goto err;
+ return ret;
ret = ulpi_read(ulpi, ULPI_SCRATCH);
if (ret < 0)
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout
2022-11-10 21:11 ` [PATCH v2 1/2] usb: ulpi: defer ulpi_register " Ferry Toth
@ 2022-11-11 6:08 ` Greg Kroah-Hartman
2022-11-14 21:55 ` Ferry Toth
2022-11-11 14:04 ` Ferry Toth
1 sibling, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2022-11-11 6:08 UTC (permalink / raw)
To: Ferry Toth
Cc: linux-usb, linux-kernel, Heikki Krogerus, Thinh Nguyen,
Sean Anderson, Liu Shixin, Ferry Toth, Andrey Smirnov,
Andy Shevchenko
On Thu, Nov 10, 2022 at 10:11:31PM +0100, Ferry Toth wrote:
> Since commit 0f010171
> Dual Role support on Intel Merrifield platform broke due to rearranging
> the call to dwc3_get_extcon().
Please see the kernel documentation for how to refer to commits. This
should be written as:
Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if
extcon is present"), Dual role....
> 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. As a result usb probe completes without phy.
>
> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
What commit does this fix?
Should this also get a cc: stable?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout
2022-11-11 6:08 ` Greg Kroah-Hartman
@ 2022-11-14 21:55 ` Ferry Toth
0 siblings, 0 replies; 7+ messages in thread
From: Ferry Toth @ 2022-11-14 21:55 UTC (permalink / raw)
To: Greg Kroah-Hartman, Ferry Toth
Cc: linux-usb, linux-kernel, Heikki Krogerus, Thinh Nguyen,
Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko,
Stephen Boyd
Op 11-11-2022 om 07:08 schreef Greg Kroah-Hartman:
> On Thu, Nov 10, 2022 at 10:11:31PM +0100, Ferry Toth wrote:
>> Since commit 0f010171
>> Dual Role support on Intel Merrifield platform broke due to rearranging
>> the call to dwc3_get_extcon().
>
> Please see the kernel documentation for how to refer to commits. This
> should be written as:
>
> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if
> extcon is present"), Dual role....
Thanks I'll fix that in v3.
>> 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. As a result usb probe completes without phy.
>>
>> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
>
> What commit does this fix?
It's complicated, not sure how to explain this clearly:
ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") started to
hide -ETIMEDOUT by returning 0. That problem was hidden due to another
problem causing dwc3 to be deferred. But not properly, causing an
infinite probe loop. This was fixed for quite some time by an out of
tree patch. Now 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral
if extcon is present") accidentally fixes the probe loop, makes the out
tree patch obsolete, but exposes the initial problem.
In short this patch fixes ef6a7bcfb01c ("usb: ulpi: Support device
discovery via DT") by returning -ETIMEDOUT to its user, who should
handle it appropriately. In case of dwc3 probe it sets -EPROBE_DEFER and
bails out.
I'll add the short fixes: in v3.
> Should this also get a cc: stable?
I will add.
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout
2022-11-10 21:11 ` [PATCH v2 1/2] usb: ulpi: defer ulpi_register " Ferry Toth
2022-11-11 6:08 ` Greg Kroah-Hartman
@ 2022-11-11 14:04 ` Ferry Toth
2022-12-12 21:51 ` Stephen Boyd
1 sibling, 1 reply; 7+ messages in thread
From: Ferry Toth @ 2022-11-11 14:04 UTC (permalink / raw)
To: Ferry Toth, linux-usb, linux-kernel
Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson,
Liu Shixin, Andrey Smirnov, Andy Shevchenko, Stephen Boyd
+ Stephen Boyd
On 10-11-2022 22:11, 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. As a result usb probe completes without phy.
>
> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> ---
> drivers/usb/common/ulpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index d7c8461976ce..60e8174686a1 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi)
> /* Test the interface */
> ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> if (ret < 0)
> - goto err;
> + return ret;
>
> ret = ulpi_read(ulpi, ULPI_SCRATCH);
> if (ret < 0)
Would this affect others phys (like qcom HSIC)? I'm not sure if failing
the test write is a normal behavior.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout
2022-11-11 14:04 ` Ferry Toth
@ 2022-12-12 21:51 ` Stephen Boyd
0 siblings, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2022-12-12 21:51 UTC (permalink / raw)
To: Ferry Toth, Ferry Toth, linux-kernel, linux-usb
Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson,
Liu Shixin, Andrey Smirnov, Andy Shevchenko
Quoting Ferry Toth (2022-11-11 06:04:16)
> + Stephen Boyd
>
> On 10-11-2022 22:11, 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. As a result usb probe completes without phy.
> >
> > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> > ---
> > drivers/usb/common/ulpi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > index d7c8461976ce..60e8174686a1 100644
> > --- a/drivers/usb/common/ulpi.c
> > +++ b/drivers/usb/common/ulpi.c
> > @@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi)
> > /* Test the interface */
> > ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> > if (ret < 0)
> > - goto err;
> > + return ret;
> >
> > ret = ulpi_read(ulpi, ULPI_SCRATCH);
> > if (ret < 0)
>
> Would this affect others phys (like qcom HSIC)? I'm not sure if failing
> the test write is a normal behavior.
I don't think failing a test write is normal behavior. I don't have this
hardware on hand anymore though, so I can't help test it. Looks OK to me
though:
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout
2022-11-10 21:11 [PATCH v2 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth
2022-11-10 21:11 ` [PATCH v2 1/2] usb: ulpi: defer ulpi_register " Ferry Toth
@ 2022-11-10 21:11 ` Ferry Toth
1 sibling, 0 replies; 7+ messages in thread
From: Ferry Toth @ 2022-11-10 21:11 UTC (permalink / raw)
To: linux-usb, linux-kernel
Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson,
Liu Shixin, Ferry Toth, 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() 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);
+ ret = -EPROBE_DEFER;
+ }
goto err0;
+ }
dwc->ulpi_ready = true;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-12-12 21:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-10 21:11 [PATCH v2 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth
2022-11-10 21:11 ` [PATCH v2 1/2] usb: ulpi: defer ulpi_register " Ferry Toth
2022-11-11 6:08 ` Greg Kroah-Hartman
2022-11-14 21:55 ` Ferry Toth
2022-11-11 14:04 ` Ferry Toth
2022-12-12 21:51 ` Stephen Boyd
2022-11-10 21:11 ` [PATCH v2 2/2] usb: dwc3: core: defer probe " Ferry Toth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).