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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
(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<[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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
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<[email protected]>
>>> ---
>>> 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
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<[email protected]>
> > > > ---
> > > > 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
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<[email protected]>
>>>>> ---
>>>>> 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
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<[email protected]>
> > > > > > ---
> > > > > > 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