[PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Andy Shevchenko]

On some platforms, like Intel Merrifield, the writing values during power on
may timeout:

   tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
   phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
   dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
   dwc3: probe of dwc3.0.auto failed with error -110

which effectively fails the probe of the USB controller.
Drop the check as it was before the culprit commit (see Fixes tag).

Fixes: 09a3512681b3 ("phy: ti: tusb1210: Improve ulpi_read()/_write() error checking")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/phy/ti/phy-tusb1210.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
index c3ab4b69ea68..669c13d6e402 100644
--- a/drivers/phy/ti/phy-tusb1210.c
+++ b/drivers/phy/ti/phy-tusb1210.c
@@ -105,8 +105,9 @@ static int tusb1210_power_on(struct phy *phy)
 	msleep(TUSB1210_RESET_TIME_MS);
 
 	/* Restore the optional eye diagram optimization value */
-	return tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2,
-				   tusb->vendor_specific2);
+	tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2, tusb->vendor_specific2);
+
+	return 0;
 }
 
 static int tusb1210_power_off(struct phy *phy)
-- 
2.35.1

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Ferry Toth]

Hi

Op 13-06-2022 om 18:08 schreef Andy Shevchenko:
> On some platforms, like Intel Merrifield, the writing values during power on
> may timeout:
>
>     tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
with the patch this error continues to appear in the logs
>     phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
>     dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
>     dwc3: probe of dwc3.0.auto failed with error -110
these errors are gone
> which effectively fails the probe of the USB controller.

and probe of dwc3 succeeds

Tested on Intel Edison (Merrifield)

> Drop the check as it was before the culprit commit (see Fixes tag).
>
> Fixes: 09a3512681b3 ("phy: ti: tusb1210: Improve ulpi_read()/_write() error checking")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/phy/ti/phy-tusb1210.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> index c3ab4b69ea68..669c13d6e402 100644
> --- a/drivers/phy/ti/phy-tusb1210.c
> +++ b/drivers/phy/ti/phy-tusb1210.c
> @@ -105,8 +105,9 @@ static int tusb1210_power_on(struct phy *phy)
>   	msleep(TUSB1210_RESET_TIME_MS);
>   
>   	/* Restore the optional eye diagram optimization value */
> -	return tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2,
> -				   tusb->vendor_specific2);
> +	tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2, tusb->vendor_specific2);
> +
> +	return 0;
>   }
>   
>   static int tusb1210_power_off(struct phy *phy)

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Ferry Toth]

Op 13-06-2022 om 22:40 schreef Ferry Toth:
> Hi
> 
> Op 13-06-2022 om 18:08 schreef Andy Shevchenko:
>> On some platforms, like Intel Merrifield, the writing values during 
>> power on
>> may timeout:
>>
>>     tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
> with the patch this error continues to appear in the logs
>>     phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
>>     dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
>>     dwc3: probe of dwc3.0.auto failed with error -110
> these errors are gone
>> which effectively fails the probe of the USB controller.
> 
> and probe of dwc3 succeeds
> 
> Tested on Intel Edison (Merrifield)

Tested-by: Ferry Toth <fntoth@gmail.com>

>> Drop the check as it was before the culprit commit (see Fixes tag).
>>
>> Fixes: 09a3512681b3 ("phy: ti: tusb1210: Improve ulpi_read()/_write() 
>> error checking")
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/phy/ti/phy-tusb1210.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-tusb1210.c 
>> b/drivers/phy/ti/phy-tusb1210.c
>> index c3ab4b69ea68..669c13d6e402 100644
>> --- a/drivers/phy/ti/phy-tusb1210.c
>> +++ b/drivers/phy/ti/phy-tusb1210.c
>> @@ -105,8 +105,9 @@ static int tusb1210_power_on(struct phy *phy)
>>       msleep(TUSB1210_RESET_TIME_MS);
>>       /* Restore the optional eye diagram optimization value */
>> -    return tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2,
>> -                   tusb->vendor_specific2);
>> +    tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2, 
>> tusb->vendor_specific2);
>> +
>> +    return 0;
>>   }
>>   static int tusb1210_power_off(struct phy *phy)

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Hans de Goede]

Hi,

On 6/13/22 18:08, Andy Shevchenko wrote:
> On some platforms, like Intel Merrifield, the writing values during power on
> may timeout:
> 
>    tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
>    phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
>    dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
>    dwc3: probe of dwc3.0.auto failed with error -110
> 
> which effectively fails the probe of the USB controller.
> Drop the check as it was before the culprit commit (see Fixes tag).
> 
> Fixes: 09a3512681b3 ("phy: ti: tusb1210: Improve ulpi_read()/_write() error checking")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Copy and pasting my reply about this in another thread to keep everyone up2date:

"""
In my experience with using the phy for charger-type detection on some
x86 android tablets which don't have any other way to do charger detection,
these errors indicate a real communication issue for reading/writing
phy registers. At the same time this usually does not seem to be a big
problem since the phy seems to work fine with its power-on defaults.

In case of Bay Trail these errors were related to 2 things:

1. Autosuspend of the phy-interface block in the dwc3, fixed by:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d7c93a903f33ff35aa0e6b5a8032eb9755b00826

But dwc3_pci_mrfld_properties[] already sets "snps,dis_u2_susphy_quirk",
so I guess it is not this.

2. There being no delay in tusb1210_power_on() between toggling the
reset IO and then trying to communicate with the phy, fixed in:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df37c99815d9e0775e67276d70c93cbc25f31c70

Maybe the:

#define TUSB1210_RESET_TIME_MS				30

Added by that commit needs to be a bit bigger for the possibly
older phy revision used on the merifield boards?

(note it is fine to just increase it a bit everywhere).
"""

IMHO it would be good to try and increase TUSB1210_RESET_TIME_MS (start with say 100
and then see if e.g. 50 also works). If increasing that does not work

I'm fine with going with this workaround patch to fix things.

Regards,

Hans



> ---
>  drivers/phy/ti/phy-tusb1210.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> index c3ab4b69ea68..669c13d6e402 100644
> --- a/drivers/phy/ti/phy-tusb1210.c
> +++ b/drivers/phy/ti/phy-tusb1210.c
> @@ -105,8 +105,9 @@ static int tusb1210_power_on(struct phy *phy)
>  	msleep(TUSB1210_RESET_TIME_MS);
>  
>  	/* Restore the optional eye diagram optimization value */
> -	return tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2,
> -				   tusb->vendor_specific2);
> +	tusb1210_ulpi_write(tusb, TUSB1210_VENDOR_SPECIFIC2, tusb->vendor_specific2);
> +
> +	return 0;
>  }
>  
>  static int tusb1210_power_off(struct phy *phy)

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Andy Shevchenko]

On Tue, Jun 14, 2022 at 01:23:21PM +0200, Hans de Goede wrote:
> On 6/13/22 18:08, Andy Shevchenko wrote:
> > On some platforms, like Intel Merrifield, the writing values during power on
> > may timeout:
> > 
> >    tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
> >    phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
> >    dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
> >    dwc3: probe of dwc3.0.auto failed with error -110
> > 
> > which effectively fails the probe of the USB controller.
> > Drop the check as it was before the culprit commit (see Fixes tag).
> > 
> > Fixes: 09a3512681b3 ("phy: ti: tusb1210: Improve ulpi_read()/_write() error checking")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Copy and pasting my reply about this in another thread to keep everyone up2date:

Thanks! My replies below.

> """
> In my experience with using the phy for charger-type detection on some
> x86 android tablets which don't have any other way to do charger detection,
> these errors indicate a real communication issue for reading/writing
> phy registers. At the same time this usually does not seem to be a big
> problem since the phy seems to work fine with its power-on defaults.
> 
> In case of Bay Trail these errors were related to 2 things:
> 
> 1. Autosuspend of the phy-interface block in the dwc3, fixed by:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d7c93a903f33ff35aa0e6b5a8032eb9755b00826
> 
> But dwc3_pci_mrfld_properties[] already sets "snps,dis_u2_susphy_quirk",
> so I guess it is not this.
> 
> 2. There being no delay in tusb1210_power_on() between toggling the
> reset IO and then trying to communicate with the phy, fixed in:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df37c99815d9e0775e67276d70c93cbc25f31c70
> 
> Maybe the:
> 
> #define TUSB1210_RESET_TIME_MS				30

Actually it's 50.

> Added by that commit needs to be a bit bigger for the possibly
> older phy revision used on the merifield boards?
> 
> (note it is fine to just increase it a bit everywhere).
> """
> 
> IMHO it would be good to try and increase TUSB1210_RESET_TIME_MS (start with say 100
> and then see if e.g. 50 also works). If increasing that does not work

No help

[   35.126397] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer reset
[   35.126418] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
[   35.126455] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
[   35.126465] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer reset found
[   35.126476] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer cs
[   35.126485] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
[   35.126538] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
[   35.126548] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer cs found
[   40.534107] tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80

(I put 5000 ms there to be sure)

> I'm fine with going with this workaround patch to fix things.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Hans de Goede]

Hi,

On 6/14/22 15:01, Andy Shevchenko wrote:
> On Tue, Jun 14, 2022 at 01:23:21PM +0200, Hans de Goede wrote:
>> On 6/13/22 18:08, Andy Shevchenko wrote:
>>> On some platforms, like Intel Merrifield, the writing values during power on
>>> may timeout:
>>>
>>>    tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
>>>    phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
>>>    dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
>>>    dwc3: probe of dwc3.0.auto failed with error -110
>>>
>>> which effectively fails the probe of the USB controller.
>>> Drop the check as it was before the culprit commit (see Fixes tag).
>>>
>>> Fixes: 09a3512681b3 ("phy: ti: tusb1210: Improve ulpi_read()/_write() error checking")
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> Copy and pasting my reply about this in another thread to keep everyone up2date:
> 
> Thanks! My replies below.
> 
>> """
>> In my experience with using the phy for charger-type detection on some
>> x86 android tablets which don't have any other way to do charger detection,
>> these errors indicate a real communication issue for reading/writing
>> phy registers. At the same time this usually does not seem to be a big
>> problem since the phy seems to work fine with its power-on defaults.
>>
>> In case of Bay Trail these errors were related to 2 things:
>>
>> 1. Autosuspend of the phy-interface block in the dwc3, fixed by:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d7c93a903f33ff35aa0e6b5a8032eb9755b00826
>>
>> But dwc3_pci_mrfld_properties[] already sets "snps,dis_u2_susphy_quirk",
>> so I guess it is not this.
>>
>> 2. There being no delay in tusb1210_power_on() between toggling the
>> reset IO and then trying to communicate with the phy, fixed in:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df37c99815d9e0775e67276d70c93cbc25f31c70
>>
>> Maybe the:
>>
>> #define TUSB1210_RESET_TIME_MS				30
> 
> Actually it's 50.
> 
>> Added by that commit needs to be a bit bigger for the possibly
>> older phy revision used on the merifield boards?
>>
>> (note it is fine to just increase it a bit everywhere).
>> """
>>
>> IMHO it would be good to try and increase TUSB1210_RESET_TIME_MS (start with say 100
>> and then see if e.g. 50 also works). If increasing that does not work
> 
> No help
> 
> [   35.126397] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer reset
> [   35.126418] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
> [   35.126455] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
> [   35.126465] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer reset found
> [   35.126476] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer cs
> [   35.126485] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
> [   35.126538] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
> [   35.126548] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer cs found
> [   40.534107] tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
> 
> (I put 5000 ms there to be sure)
> 
>> I'm fine with going with this workaround patch to fix things.

Ok, so I guess we should just apply this workaround patch to make
the error non fatal. Still would be good to dig a little deeper one
of these days and see what is going on here...

Regards,

Hans

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Andy Shevchenko]

On Tue, Jun 14, 2022 at 05:49:22PM +0200, Hans de Goede wrote:
> On 6/14/22 15:01, Andy Shevchenko wrote:
> > On Tue, Jun 14, 2022 at 01:23:21PM +0200, Hans de Goede wrote:
> >> On 6/13/22 18:08, Andy Shevchenko wrote:

...

> > [   35.126397] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer reset
> > [   35.126418] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
> > [   35.126455] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
> > [   35.126465] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer reset found
> > [   35.126476] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer cs
> > [   35.126485] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
> > [   35.126538] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
> > [   35.126548] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer cs found
> > [   40.534107] tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
> > 
> > (I put 5000 ms there to be sure)
> > 
> >> I'm fine with going with this workaround patch to fix things.
> 
> Ok, so I guess we should just apply this workaround patch to make
> the error non fatal. Still would be good to dig a little deeper one
> of these days and see what is going on here...

Can you give a formal tag?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Hans de Goede]

Hi,

On 6/15/22 13:51, Andy Shevchenko wrote:
> On Tue, Jun 14, 2022 at 05:49:22PM +0200, Hans de Goede wrote:
>> On 6/14/22 15:01, Andy Shevchenko wrote:
>>> On Tue, Jun 14, 2022 at 01:23:21PM +0200, Hans de Goede wrote:
>>>> On 6/13/22 18:08, Andy Shevchenko wrote:
> 
> ...
> 
>>> [   35.126397] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer reset
>>> [   35.126418] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
>>> [   35.126455] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
>>> [   35.126465] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer reset found
>>> [   35.126476] tusb1210 dwc3.0.auto.ulpi: GPIO lookup for consumer cs
>>> [   35.126485] tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup
>>> [   35.126538] tusb1210 dwc3.0.auto.ulpi: using lookup tables for GPIO lookup
>>> [   35.126548] tusb1210 dwc3.0.auto.ulpi: No GPIO consumer cs found
>>> [   40.534107] tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
>>>
>>> (I put 5000 ms there to be sure)
>>>
>>>> I'm fine with going with this workaround patch to fix things.
>>
>> Ok, so I guess we should just apply this workaround patch to make
>> the error non fatal. Still would be good to dig a little deeper one
>> of these days and see what is going on here...
> 
> Can you give a formal tag?

Sure:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH v1 1/1] phy: ti: tusb1210: Don't check for write errors when powering on

[Vinod Koul]

On 13-06-22, 19:08, Andy Shevchenko wrote:
> On some platforms, like Intel Merrifield, the writing values during power on
> may timeout:
> 
>    tusb1210 dwc3.0.auto.ulpi: error -110 writing val 0x41 to reg 0x80
>    phy phy-dwc3.0.auto.ulpi.0: phy poweron failed --> -110
>    dwc3 dwc3.0.auto: error -ETIMEDOUT: failed to initialize core
>    dwc3: probe of dwc3.0.auto failed with error -110
> 
> which effectively fails the probe of the USB controller.
> Drop the check as it was before the culprit commit (see Fixes tag).

Applied, thanks

-- 
~Vinod