| Message ID | 20220613160848.82746-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 9293ee00cc1c4a49325f7694563dd15591652897 |
| Headers | show |
| Series | [v1,1/1] phy: ti: tusb1210: Don't check for write errors when powering on | expand |
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)
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)
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)
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.
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
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?
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
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
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)
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(-)