[0/2] musb-fixes for v4.9-rc2
diff mbox

Message ID 20161024180708.kpx6s2jb7dpg6xfx@atomide.com
State New
Headers show

Commit Message

Tony Lindgren Oct. 24, 2016, 6:07 p.m. UTC
Hi,

* Tony Lindgren <tony@atomide.com> [161021 00:18]:
> * Ladislav Michl <ladis@linux-mips.org> [161020 12:37]:
> > [  186.457519] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_bcon (90, <VBusValid), retry #3, port1 0008010c
> > 
> > And that's the end, since now it does not react on hub plug/unplug.
> > 
> > Also all that VBUS_ERROR conditions are strange as hub is powered separately
> > and power lines from phy are not used.
> 
> Hmm yeah. I'd like to be able to reproduce this. Can you email me
> your .config (again)? You have things in host mode with a powered
> hub plus few devices with no USB gadgets configured?

Well I found your earlier .config so presumably that did not change.
Below patch seems to do the trick for me, but I need to test more.

Care to test if it helps for you? Please test with v4.9-rc2 and the
following two fixes heading in Greg's usb-linus branch:

cacaaf80c3a6 ("usb: musb: Call pm_runtime from musb_gadget_queue")
d8e5f0eca1e8 ("usb: musb: Fix hardirq-safe hardirq-unsafe lock order error")

I'll send a proper patch if that works for you.

Regards,

Tony

8< ------------------------

Comments

Ladislav Michl Nov. 1, 2016, 9:13 p.m. UTC | #1
Hi,

On Mon, Oct 24, 2016 at 11:07:08AM -0700, Tony Lindgren wrote:
> Hi,
> 
> * Tony Lindgren <tony@atomide.com> [161021 00:18]:
> > * Ladislav Michl <ladis@linux-mips.org> [161020 12:37]:
> > > [  186.457519] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_bcon (90, <VBusValid), retry #3, port1 0008010c
> > > 
> > > And that's the end, since now it does not react on hub plug/unplug.
> > > 
> > > Also all that VBUS_ERROR conditions are strange as hub is powered separately
> > > and power lines from phy are not used.
> > 
> > Hmm yeah. I'd like to be able to reproduce this. Can you email me
> > your .config (again)? You have things in host mode with a powered
> > hub plus few devices with no USB gadgets configured?
> 
> Well I found your earlier .config so presumably that did not change.
> Below patch seems to do the trick for me, but I need to test more.
> 
> Care to test if it helps for you? Please test with v4.9-rc2 and the
> following two fixes heading in Greg's usb-linus branch:
> 
> cacaaf80c3a6 ("usb: musb: Call pm_runtime from musb_gadget_queue")
> d8e5f0eca1e8 ("usb: musb: Fix hardirq-safe hardirq-unsafe lock order error")

tested with v4.9-rc3 which have these included.

> I'll send a proper patch if that works for you.

Unfortunately it's still the same. Direct connection (without hub) remains
untested as there's not enough power to supply display:
usb 2-1: USB disconnect, device number 2
usb 2-1: new high-speed USB device number 3 using musb-hdrc
usb 2-1: New USB device found, idVendor=17e9, idProduct=0335
usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 2-1: Product: MIMO
usb 2-1: Manufacturer: DisplayLink
usb 2-1: SerialNumber: 1071007195
usb 2-1: rejected 1 configuration due to insufficient available bus power
usb 2-1: no configuration chosen from 1 choice

Regards,
	ladis

> Regards,
> 
> Tony
> 
> 8< ------------------------
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -459,8 +459,6 @@ static int twl4030_phy_power_off(struct phy *phy)
>  	struct twl4030_usb *twl = phy_get_drvdata(phy);
>  
>  	dev_dbg(twl->dev, "%s\n", __func__);
> -	pm_runtime_mark_last_busy(twl->dev);
> -	pm_runtime_put_autosuspend(twl->dev);
>  
>  	return 0;
>  }
> @@ -472,6 +470,8 @@ static int twl4030_phy_power_on(struct phy *phy)
>  	dev_dbg(twl->dev, "%s\n", __func__);
>  	pm_runtime_get_sync(twl->dev);
>  	schedule_delayed_work(&twl->id_workaround_work, HZ);
> +	pm_runtime_mark_last_busy(twl->dev);
> +	pm_runtime_put_autosuspend(twl->dev);
>  
>  	return 0;
>  }
> -- 
> 2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Nov. 3, 2016, 8:59 p.m. UTC | #2
* Ladislav Michl <ladis@linux-mips.org> [161101 14:14]:
> Hi,
> 
> On Mon, Oct 24, 2016 at 11:07:08AM -0700, Tony Lindgren wrote:
> > Hi,
> > 
> > * Tony Lindgren <tony@atomide.com> [161021 00:18]:
> > > * Ladislav Michl <ladis@linux-mips.org> [161020 12:37]:
> > > > [  186.457519] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_bcon (90, <VBusValid), retry #3, port1 0008010c
> > > > 
> > > > And that's the end, since now it does not react on hub plug/unplug.
> > > > 
> > > > Also all that VBUS_ERROR conditions are strange as hub is powered separately
> > > > and power lines from phy are not used.
> > > 
> > > Hmm yeah. I'd like to be able to reproduce this. Can you email me
> > > your .config (again)? You have things in host mode with a powered
> > > hub plus few devices with no USB gadgets configured?
> > 
> > Well I found your earlier .config so presumably that did not change.
> > Below patch seems to do the trick for me, but I need to test more.
> > 
> > Care to test if it helps for you? Please test with v4.9-rc2 and the
> > following two fixes heading in Greg's usb-linus branch:
> > 
> > cacaaf80c3a6 ("usb: musb: Call pm_runtime from musb_gadget_queue")
> > d8e5f0eca1e8 ("usb: musb: Fix hardirq-safe hardirq-unsafe lock order error")
> 
> tested with v4.9-rc3 which have these included.

OK thanks.

> > I'll send a proper patch if that works for you.
> 
> Unfortunately it's still the same. Direct connection (without hub) remains
> untested as there's not enough power to supply display:
> usb 2-1: USB disconnect, device number 2
> usb 2-1: new high-speed USB device number 3 using musb-hdrc
> usb 2-1: New USB device found, idVendor=17e9, idProduct=0335
> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> usb 2-1: Product: MIMO
> usb 2-1: Manufacturer: DisplayLink
> usb 2-1: SerialNumber: 1071007195
> usb 2-1: rejected 1 configuration due to insufficient available bus power
> usb 2-1: no configuration chosen from 1 choice

Hmm yeah playing with a hub connected devices don't always enumerate.
When that happens, I get this:

usb 1-1: reset high-speed USB device number 45 using musb-hdrc
usb 1-1: reset high-speed USB device number 45 using musb-hdrc
usb 1-1: reset high-speed USB device number 45 using musb-hdrc
usb 1-1: USB disconnect, device number 45
usb 1-1: new high-speed USB device number 47 using musb-hdrc
usb 1-1: new high-speed USB device number 48 using musb-hdrc
...

And that keeps on going until I reconnect the hub.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 4, 2016, 8:31 a.m. UTC | #3
Hi,

Tony Lindgren <tony@atomide.com> writes:
>> > * Tony Lindgren <tony@atomide.com> [161021 00:18]:
>> > > * Ladislav Michl <ladis@linux-mips.org> [161020 12:37]:
>> > > > [  186.457519] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_wait_bcon (90, <VBusValid), retry #3, port1 0008010c
>> > > > 
>> > > > And that's the end, since now it does not react on hub plug/unplug.
>> > > > 
>> > > > Also all that VBUS_ERROR conditions are strange as hub is powered separately
>> > > > and power lines from phy are not used.
>> > > 
>> > > Hmm yeah. I'd like to be able to reproduce this. Can you email me
>> > > your .config (again)? You have things in host mode with a powered
>> > > hub plus few devices with no USB gadgets configured?
>> > 
>> > Well I found your earlier .config so presumably that did not change.
>> > Below patch seems to do the trick for me, but I need to test more.
>> > 
>> > Care to test if it helps for you? Please test with v4.9-rc2 and the
>> > following two fixes heading in Greg's usb-linus branch:
>> > 
>> > cacaaf80c3a6 ("usb: musb: Call pm_runtime from musb_gadget_queue")
>> > d8e5f0eca1e8 ("usb: musb: Fix hardirq-safe hardirq-unsafe lock order error")
>> 
>> tested with v4.9-rc3 which have these included.
>
> OK thanks.
>
>> > I'll send a proper patch if that works for you.
>> 
>> Unfortunately it's still the same. Direct connection (without hub) remains
>> untested as there's not enough power to supply display:
>> usb 2-1: USB disconnect, device number 2
>> usb 2-1: new high-speed USB device number 3 using musb-hdrc
>> usb 2-1: New USB device found, idVendor=17e9, idProduct=0335
>> usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>> usb 2-1: Product: MIMO
>> usb 2-1: Manufacturer: DisplayLink
>> usb 2-1: SerialNumber: 1071007195
>> usb 2-1: rejected 1 configuration due to insufficient available bus power
>> usb 2-1: no configuration chosen from 1 choice
>
> Hmm yeah playing with a hub connected devices don't always enumerate.
> When that happens, I get this:
>
> usb 1-1: reset high-speed USB device number 45 using musb-hdrc
> usb 1-1: reset high-speed USB device number 45 using musb-hdrc
> usb 1-1: reset high-speed USB device number 45 using musb-hdrc
> usb 1-1: USB disconnect, device number 45
> usb 1-1: new high-speed USB device number 47 using musb-hdrc
> usb 1-1: new high-speed USB device number 48 using musb-hdrc
> ...
>
> And that keeps on going until I reconnect the hub.

Sounds like VBUS dropping to me. Remember, MUSB is really anal about
VBUS levels. If it drops enough for the PHY to report one of those 4
VBUS levels, then MUSB just gives up.

What we used to do back at Nokia was disable reporting of some of those
VBUS levels at the PHY driver.
Ladislav Michl Nov. 4, 2016, 9:46 a.m. UTC | #4
On Fri, Nov 04, 2016 at 10:31:38AM +0200, Felipe Balbi wrote:
[snip]
> Sounds like VBUS dropping to me. Remember, MUSB is really anal about
> VBUS levels. If it drops enough for the PHY to report one of those 4
> VBUS levels, then MUSB just gives up.
> 
> What we used to do back at Nokia was disable reporting of some of those
> VBUS levels at the PHY driver.

Well, I was getting these even without anything connected to musb:
[ 1526.594696] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000104
[  485.244781] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000104
...and with the hub connected:
[  562.803833] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000507
[   50.068664] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000507
And indeed, after these errors musb gives up.

However these are gone with Tony's last patch. Bisecting now, approx 6 iterations left.

Best regards,
	ladis
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ladislav Michl Nov. 4, 2016, 10:03 a.m. UTC | #5
On Fri, Nov 04, 2016 at 10:46:24AM +0100, Ladislav Michl wrote:
> On Fri, Nov 04, 2016 at 10:31:38AM +0200, Felipe Balbi wrote:
> [snip]
> > Sounds like VBUS dropping to me. Remember, MUSB is really anal about
> > VBUS levels. If it drops enough for the PHY to report one of those 4
> > VBUS levels, then MUSB just gives up.
> > 
> > What we used to do back at Nokia was disable reporting of some of those
> > VBUS levels at the PHY driver.
> 
> Well, I was getting these even without anything connected to musb:
> [ 1526.594696] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000104
> [  485.244781] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000104
> ...and with the hub connected:
> [  562.803833] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000507
> [   50.068664] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000507
> And indeed, after these errors musb gives up.
> 
> However these are gone with Tony's last patch. Bisecting now, approx 6 iterations left.

So here we are:

87326e858448c40e32f142c0b8dcc59d7b27c641 is the first bad commit
commit 87326e858448c40e32f142c0b8dcc59d7b27c641
Author: Tony Lindgren <tony@atomide.com>
Date:   Tue May 31 10:05:20 2016 -0500

    usb: musb: Remove extra PM runtime calls from 2430 glue layer

    With PM runtime behaving, these are all now unnecessary.
    Doing pm_runtime_get(musb->controller) will keep the parent
    glue layer also active.

    Signed-off-by: Tony Lindgren <tony@atomide.com>
    Signed-off-by: Bin Liu <b-liu@ti.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

:040000 040000 6b7553722d760982d40a3e211bca7595ff313c62 d007f683d33e61a9f0046661097849c976169670 M      drivers

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Nov. 4, 2016, 2:35 p.m. UTC | #6
* Ladislav Michl <ladis@linux-mips.org> [161104 02:47]:
> On Fri, Nov 04, 2016 at 10:31:38AM +0200, Felipe Balbi wrote:
> [snip]
> > Sounds like VBUS dropping to me. Remember, MUSB is really anal about
> > VBUS levels. If it drops enough for the PHY to report one of those 4
> > VBUS levels, then MUSB just gives up.
> > 
> > What we used to do back at Nokia was disable reporting of some of those
> > VBUS levels at the PHY driver.

Yeah that seems to be what Ladis is seeing.

> Well, I was getting these even without anything connected to musb:
> [ 1526.594696] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000104
> [  485.244781] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000104
> ...and with the hub connected:
> [  562.803833] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000507
> [   50.068664] musb-hdrc musb-hdrc.0.auto: VBUS_ERROR in a_idle (90, <VBusValid), retry #0, port1 00000507
> And indeed, after these errors musb gives up.
> 
> However these are gone with Tony's last patch. Bisecting now, approx 6 iterations left.

These were caused by devctl session bit taking few seconds to clear
so we have to poll that on disconnect until it clears.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -459,8 +459,6 @@  static int twl4030_phy_power_off(struct phy *phy)
 	struct twl4030_usb *twl = phy_get_drvdata(phy);
 
 	dev_dbg(twl->dev, "%s\n", __func__);
-	pm_runtime_mark_last_busy(twl->dev);
-	pm_runtime_put_autosuspend(twl->dev);
 
 	return 0;
 }
@@ -472,6 +470,8 @@  static int twl4030_phy_power_on(struct phy *phy)
 	dev_dbg(twl->dev, "%s\n", __func__);
 	pm_runtime_get_sync(twl->dev);
 	schedule_delayed_work(&twl->id_workaround_work, HZ);
+	pm_runtime_mark_last_busy(twl->dev);
+	pm_runtime_put_autosuspend(twl->dev);
 
 	return 0;
 }