Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1480271 > unrolled thread

Re: [v2] musb: omap2430: do not assume balanced enable()/disable()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-09-09 21:30 +0200
    Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Tony Lindgren <tony@atomide.com> - 2016-09-09 22:10 +0200
      Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-09-09 22:30 +0200
        Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Andreas Kemnade <andreas@kemnade.info> - 2016-09-09 22:50 +0200
          Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Tony Lindgren <tony@atomide.com> - 2016-09-09 23:00 +0200
        Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Tony Lindgren <tony@atomide.com> - 2016-09-09 23:00 +0200
          Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Andreas Kemnade <andreas@kemnade.info> - 2016-09-09 23:30 +0200
            Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Tony Lindgren <tony@atomide.com> - 2016-09-09 23:40 +0200
              Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Tony Lindgren <tony@atomide.com> - 2016-09-10 01:50 +0200
                Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Andreas Kemnade <andreas@kemnade.info> - 2016-09-10 13:30 +0200
                  Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Tony Lindgren <tony@atomide.com> - 2016-09-10 15:10 +0200
                    Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-09-11 11:10 +0200
                      Re: [v2] musb: omap2430: do not assume balanced enable()/disable() Tony Lindgren <tony@atomide.com> - 2016-09-12 16:40 +0200

#1480271 — Re: [v2] musb: omap2430: do not assume balanced enable()/disable()

Hi Andreas,

Thank you for the patch.

On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> The code assumes that omap2430_musb_enable() and
> omap2430_musb_disable() are called in a balanced way.
> That fact is broken by the fact that musb_init_controller() calls
> musb_platform_disable() to switch from unknown state to off state
> on initialisation.
> 
> That means that phy_power_off() is called first so that
> phy->power_count gets -1 and the phy is not enabled on phy_power_on().
> So when usb gadget is started the phy is not powered on.
> Depending on the phy used that caused various problems.
> Besides of causing usb problems, that can also have side effects.
> 
> In the case of using the phy_twl4030, that prevents also charging
> the battery via usb (using twl4030_charger) and so makes further
> kernel debugging hard.
> The problem was seen with 4.7 on an openphoenux gta04. It has a DM3730
> SoC and a TPS65950 companion.  phy->power never became 1
> and so the usb did get powered on.
> 
> The patch prevents phy_power_off() from being called when
> it is already off.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

This fixes USB gadget operation on the Panda board.

Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for 2430 
glue layer")
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> changes in v2:
> improved commit message
> 
>  drivers/usb/musb/omap2430.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 0b4cec9..c1a2b7b 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -413,9 +413,10 @@ static void omap2430_musb_disable(struct musb *musb)
>  	struct device *dev = musb->controller;
>  	struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
> 
> -	if (!WARN_ON(!musb->phy))
> -		phy_power_off(musb->phy);
> -
> +	if (glue->enabled) {
> +		if (!WARN_ON(!musb->phy))
> +			phy_power_off(musb->phy);
> +	}
>  	if (glue->status != MUSB_UNKNOWN)
>  		omap_control_usb_set_mode(glue->control_otghs,
>  			USB_MODE_DISCONNECT);

-- 
Regards,

Laurent Pinchart

[toc] | [next] | [standalone]

#1480304

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 12:27]:
> Hi Andreas,
> 
> Thank you for the patch.
> 
> On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> > The code assumes that omap2430_musb_enable() and
> > omap2430_musb_disable() are called in a balanced way.
> > That fact is broken by the fact that musb_init_controller() calls
> > musb_platform_disable() to switch from unknown state to off state
> > on initialisation.
> > 
> > That means that phy_power_off() is called first so that
> > phy->power_count gets -1 and the phy is not enabled on phy_power_on().
> > So when usb gadget is started the phy is not powered on.
> > Depending on the phy used that caused various problems.
> > Besides of causing usb problems, that can also have side effects.
> > 
> > In the case of using the phy_twl4030, that prevents also charging
> > the battery via usb (using twl4030_charger) and so makes further
> > kernel debugging hard.
> > The problem was seen with 4.7 on an openphoenux gta04. It has a DM3730
> > SoC and a TPS65950 companion.  phy->power never became 1
> > and so the usb did get powered on.
> > 
> > The patch prevents phy_power_off() from being called when
> > it is already off.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> 
> This fixes USB gadget operation on the Panda board.
> 
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for 2430 
> glue layer")
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This patch has a side effect of fixing the issue by breaking PM
runtime, not a good fix as discussed.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1480333

Hi Tony,

On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 12:27]:
> > On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> >> The code assumes that omap2430_musb_enable() and
> >> omap2430_musb_disable() are called in a balanced way.
> >> That fact is broken by the fact that musb_init_controller() calls
> >> musb_platform_disable() to switch from unknown state to off state
> >> on initialisation.
> >> 
> >> That means that phy_power_off() is called first so that
> >> phy->power_count gets -1 and the phy is not enabled on phy_power_on().
> >> So when usb gadget is started the phy is not powered on.
> >> Depending on the phy used that caused various problems.
> >> Besides of causing usb problems, that can also have side effects.
> >> 
> >> In the case of using the phy_twl4030, that prevents also charging
> >> the battery via usb (using twl4030_charger) and so makes further
> >> kernel debugging hard.
> >> The problem was seen with 4.7 on an openphoenux gta04. It has a DM3730
> >> SoC and a TPS65950 companion.  phy->power never became 1
> >> and so the usb did get powered on.
> >> 
> >> The patch prevents phy_power_off() from being called when
> >> it is already off.
> >> 
> >> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > 
> > This fixes USB gadget operation on the Panda board.
> > 
> > Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for
> > 2430 glue layer")
> > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> This patch has a side effect of fixing the issue by breaking PM
> runtime, not a good fix as discussed.

How exactly is it worse breaking runtime PM than breaking USB gadget 
completely ? :-)

The issue here is that the .disable() platform operation is called by musb 
with the PHY already powered off, leading to the PHY power reference count 
becoming negative. The next call to the .enable() operation restores the 
reference count to 0 without enabling the PHY.

Feel free to send me a better fix and I will test it.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1480344

On Fri, 09 Sep 2016 23:21:50 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Tony,
> 
> On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909
> > 12:27]:
> > > On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> > >> The code assumes that omap2430_musb_enable() and
> > >> omap2430_musb_disable() are called in a balanced way.
> > >> That fact is broken by the fact that musb_init_controller() calls
> > >> musb_platform_disable() to switch from unknown state to off state
> > >> on initialisation.
> > >> 
> > >> That means that phy_power_off() is called first so that
> > >> phy->power_count gets -1 and the phy is not enabled on
> > >> phy_power_on(). So when usb gadget is started the phy is not
> > >> powered on. Depending on the phy used that caused various
> > >> problems. Besides of causing usb problems, that can also have
> > >> side effects.
> > >> 
> > >> In the case of using the phy_twl4030, that prevents also charging
> > >> the battery via usb (using twl4030_charger) and so makes further
> > >> kernel debugging hard.
> > >> The problem was seen with 4.7 on an openphoenux gta04. It has a
> > >> DM3730 SoC and a TPS65950 companion.  phy->power never became 1
> > >> and so the usb did get powered on.
> > >> 
> > >> The patch prevents phy_power_off() from being called when
> > >> it is already off.
> > >> 
> > >> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > 
> > > This fixes USB gadget operation on the Panda board.
> > > 
> > > Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy
> > > handling for 2430 glue layer")
> > > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > This patch has a side effect of fixing the issue by breaking PM
> > runtime, not a good fix as discussed.
> 
> How exactly is it worse breaking runtime PM than breaking USB gadget 
> completely ? :-)
> 
Does it still break with my phy-twl4030 fixes? At least on gta04,
they fix real problems and hide the musb problem I tried to fix with
this patch.
https://patchwork.kernel.org/patch/9292097/
https://patchwork.kernel.org/patch/9298447/

> The issue here is that the .disable() platform operation is called by
> musb with the PHY already powered off, leading to the PHY power
> reference count becoming negative. The next call to the .enable()
> operation restores the reference count to 0 without enabling the PHY.
> 
> Feel free to send me a better fix and I will test it.
> 
The patch has to be reworked on top of the patch series:
Implement PM runtime for musb-core based on session bit

Regards,
Andreas Kemnade

[toc] | [prev] | [next] | [standalone]

#1480349

* Andreas Kemnade <andreas@kemnade.info> [160909 13:40]:
> On Fri, 09 Sep 2016 23:21:50 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > 
> > How exactly is it worse breaking runtime PM than breaking USB gadget 
> > completely ? :-)
> > 
> Does it still break with my phy-twl4030 fixes? At least on gta04,
> they fix real problems and hide the musb problem I tried to fix with
> this patch.
> https://patchwork.kernel.org/patch/9292097/
> https://patchwork.kernel.org/patch/9298447/

Andreas, it's a different USB PHY on pandaboard, that's using
phy-twl6030-usb.c. Probably similar issue.

> > The issue here is that the .disable() platform operation is called by
> > musb with the PHY already powered off, leading to the PHY power
> > reference count becoming negative. The next call to the .enable()
> > operation restores the reference count to 0 without enabling the PHY.
> > 
> > Feel free to send me a better fix and I will test it.
> > 
> The patch has to be reworked on top of the patch series:
> Implement PM runtime for musb-core based on session bit

Yeah that leaves out all most of the trickery with the glue
specific PM runtime tinkering so tracking down any remaining
unbalanced calls should be easier :)

But that's for v4.9, let's see what's the minimal fix for v4.8.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1480351

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 13:21]:
> On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> > This patch has a side effect of fixing the issue by breaking PM
> > runtime, not a good fix as discussed.
> 
> How exactly is it worse breaking runtime PM than breaking USB gadget 
> completely ? :-)

Yeah sorry to break it, I obviously did not test it on all platforms :(
I'm mostly using omap3 with the 2430 glue layer and am335x for the
dsps glue layer and did not know that omap4 is broken. I guess I've
recently just used the EHCI ports on panda.

> The issue here is that the .disable() platform operation is called by musb 
> with the PHY already powered off, leading to the PHY power reference count 
> becoming negative. The next call to the .enable() operation restores the 
> reference count to 0 without enabling the PHY.

Well for the phy-twl4030-usb.c, AFAIK the right fix is to fix the PHY
driver as done in "[PATCH v2] phy-twl4030-usb: initialize charging-related
stuff via pm_runtime". I suspect something similar is happening here
also with the omap4 legacy phy.

> Feel free to send me a better fix and I will test it.

Yeah will do, hang on.

Tony

[toc] | [prev] | [next] | [standalone]

#1480362

On Fri, 9 Sep 2016 13:51:04 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 13:21]:
> > On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> > > This patch has a side effect of fixing the issue by breaking PM
> > > runtime, not a good fix as discussed.
> > 
> > How exactly is it worse breaking runtime PM than breaking USB
> > gadget completely ? :-)
> 
> Yeah sorry to break it, I obviously did not test it on all
> platforms :( I'm mostly using omap3 with the 2430 glue layer and
> am335x for the dsps glue layer and did not know that omap4 is broken.
> I guess I've recently just used the EHCI ports on panda.
> 
> > The issue here is that the .disable() platform operation is called
> > by musb with the PHY already powered off, leading to the PHY power
> > reference count becoming negative. The next call to the .enable()
> > operation restores the reference count to 0 without enabling the
> > PHY.
> 
> Well for the phy-twl4030-usb.c, AFAIK the right fix is to fix the PHY
> driver as done in "[PATCH v2] phy-twl4030-usb: initialize
> charging-related stuff via pm_runtime". I suspect something similar
> is happening here also with the omap4 legacy phy.
> 
No, the fix is for making charging work independant of musb.
Gadget is working because charging is enabled and enables all parts in
the phy needed for it. And you can charge without musb (only musb_hdrc
for the mailbox but not the omap2430 glue module).

We have two independant things:
1. phy-twl4030-usb (and perhaps others) do not enable
  the phy enough to allow charging on pm_runtime_get().
  That is fixed by my phy-related patches.

2. phy_power_off/on() in called in an unbalanced way if
   it is called behind musb_platform_enable()/disable()
   as it happens in omap2430.c. Two ways to fix it:
   a) prevent phy_power_off()/on() to be called in
      an unbalanced way in omap240.c
   b) prevent musb_platform_enable()
	      musb_platform_disable() to be called in an
	      unbalanced way by fixing musb_core.c

Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
have gadget working for the most common usecases. (not using
twl4030-charger would not work yet)
But in the longer term 2. has to be fixed too.

Regards,
Andreas Kemnade

[toc] | [prev] | [next] | [standalone]

#1480367

* Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> On Fri, 9 Sep 2016 13:51:04 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> > Well for the phy-twl4030-usb.c, AFAIK the right fix is to fix the PHY
> > driver as done in "[PATCH v2] phy-twl4030-usb: initialize
> > charging-related stuff via pm_runtime". I suspect something similar
> > is happening here also with the omap4 legacy phy.
> > 
> No, the fix is for making charging work independant of musb.
> Gadget is working because charging is enabled and enables all parts in
> the phy needed for it. And you can charge without musb (only musb_hdrc
> for the mailbox but not the omap2430 glue module).

Oh right.

> We have two independant things:
> 1. phy-twl4030-usb (and perhaps others) do not enable
>   the phy enough to allow charging on pm_runtime_get().
>   That is fixed by my phy-related patches.

OK

> 2. phy_power_off/on() in called in an unbalanced way if
>    it is called behind musb_platform_enable()/disable()
>    as it happens in omap2430.c. Two ways to fix it:
>    a) prevent phy_power_off()/on() to be called in
>       an unbalanced way in omap240.c
>    b) prevent musb_platform_enable()
> 	      musb_platform_disable() to be called in an
> 	      unbalanced way by fixing musb_core.c
> 
> Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> have gadget working for the most common usecases. (not using
> twl4030-charger would not work yet)
> But in the longer term 2. has to be fixed too.

Sounds like option 2b here is the real fix.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1480438

* Tony Lindgren <tony@atomide.com> [160909 14:33]:
> * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > We have two independant things:
> > 1. phy-twl4030-usb (and perhaps others) do not enable
> >   the phy enough to allow charging on pm_runtime_get().
> >   That is fixed by my phy-related patches.
> 
> OK
>
> > 2. phy_power_off/on() in called in an unbalanced way if
> >    it is called behind musb_platform_enable()/disable()
> >    as it happens in omap2430.c. Two ways to fix it:
> >    a) prevent phy_power_off()/on() to be called in
> >       an unbalanced way in omap240.c
> >    b) prevent musb_platform_enable()
> > 	      musb_platform_disable() to be called in an
> > 	      unbalanced way by fixing musb_core.c
> > 
> > Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > have gadget working for the most common usecases. (not using
> > twl4030-charger would not work yet)
> > But in the longer term 2. has to be fixed too.
> 
> Sounds like option 2b here is the real fix.

And doing full option 2b would be intrusive because of musb_stop
also calling musb_platform_disable. Here's a suggested fix for
v4.8-rc cycle.

Seems to work for me for omap3 torpedo using phy-twl4030-usb,
omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
black using dsps glue. Also PM runtime works on omap3.

This patch causes a slight merge conlict with Andrea's patches,
as listed in #1 above, but we should probably merge this first
as a fix. That is assuming it does not cause side effects to
Andrea's phy-twl4030-usb charger test case.

Can you guys please test? If things work I'll resend the
patch with proper tested-bys and acks.

Regards,

Tony

8< --------------------------
From: Tony Lindgren <tony@atomide.com>
Date: Fri, 9 Sep 2016 15:04:53 -0700
Subject: [PATCH] usb: musb: Fix unbalanced platform_disable

Commit a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
for 2430 glue layer") moved PHY enable/disable calls to happen from
omap2430_musb_enable/disable(). That broke enumeration for several
devices as PM runtime in the PHY will never enable it.

The root cause of the problem is unpaired calls from musb_core.c to
musb_platform_enable/disable in musb_core.c as reported by
Andreas Kemnade <andreas@kemnade.info>.

As musb_platform_enable/disable are being called from various functions,
let's not attempt to make them paiered immediately. This would require
fixing also musb_stop as it currently calls musb_platform_disable.

Instead, let's first fix the regression in a minimal way by removing
the initial call to musb_platform_disable.

AFAIK the initial musb_platform_disable call has always been just an
attempted workaround for the 2430 glue layer announcing itself too
early before the gadgets are configured. And that issue finally
got fixed with commit a118df07f5b1 ("usb: musb: Don't set d+ high
before enable for 2430 glue layer").

We now also need to fix the twl4030-phy accordingly making it's
PM runtime call only needed in twl4030_phy_power_on and have it
autosuspend. The cable state will keep the phy active when connected.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Reported-by: Andreas Kemnade <andreas@kemnade.info>
Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
for 2430 glue layer")
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -447,8 +447,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;
 }
@@ -465,6 +463,8 @@ static int twl4030_phy_power_on(struct phy *phy)
 		twl4030_i2c_access(twl, 0);
 	twl->linkstat = MUSB_UNKNOWN;
 	schedule_delayed_work(&twl->id_workaround_work, HZ);
+	pm_runtime_mark_last_busy(twl->dev);
+	pm_runtime_put_autosuspend(twl->dev);
 
 	return 0;
 }
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2142,7 +2142,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	/* be sure interrupts are disabled before connecting ISR */
-	musb_platform_disable(musb);
 	musb_generic_disable(musb);
 
 	/* Init IRQ workqueue before request_irq */

[toc] | [prev] | [next] | [standalone]

#1480605

On Fri, 9 Sep 2016 16:40:15 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> > * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > > We have two independant things:
> > > 1. phy-twl4030-usb (and perhaps others) do not enable
> > >   the phy enough to allow charging on pm_runtime_get().
> > >   That is fixed by my phy-related patches.
> > 
> > OK
> >
> > > 2. phy_power_off/on() in called in an unbalanced way if
> > >    it is called behind musb_platform_enable()/disable()
> > >    as it happens in omap2430.c. Two ways to fix it:
> > >    a) prevent phy_power_off()/on() to be called in
> > >       an unbalanced way in omap240.c
> > >    b) prevent musb_platform_enable()
> > > 	      musb_platform_disable() to be called in an
> > > 	      unbalanced way by fixing musb_core.c
> > > 
> > > Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > > have gadget working for the most common usecases. (not using
> > > twl4030-charger would not work yet)
> > > But in the longer term 2. has to be fixed too.
> > 
> > Sounds like option 2b here is the real fix.
> 
> And doing full option 2b would be intrusive because of musb_stop
> also calling musb_platform_disable. Here's a suggested fix for
> v4.8-rc cycle.
> 
musb_platform_disable() in musb_stop() seems to be balanced.

from my list in an earlier mail:
musb_platform_disable() in musb_remove() called upon module removal

To my analysis this is odd because musb_stop() is also called indirectly
upon removal.
But topic that can be left for later.

> Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> black using dsps glue. Also PM runtime works on omap3.
> 
> This patch causes a slight merge conlict with Andrea's patches,
> as listed in #1 above, but we should probably merge this first
> as a fix. That is assuming it does not cause side effects to
> Andrea's phy-twl4030-usb charger test case.
> 
Hmm, if the patch will gender-change me, then a clear NAK ;-)

> Can you guys please test? If things work I'll resend the
> patch with proper tested-bys and acks.
> 
I tested the patch without any other musb/phy-fixes.
No regressions. It fixes Gadget to be usable. Charging seems
not to be totally stable. I will check my phy-patches
on top of that again. 
PM runtime probably still desires some work on it.
But I give a clear Ack for merging this one first.

Regards,
Andreas Kemnade

> Regards,
> 
> Tony
> 
> 8< --------------------------
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 9 Sep 2016 15:04:53 -0700
> Subject: [PATCH] usb: musb: Fix unbalanced platform_disable
> 
> Commit a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
> for 2430 glue layer") moved PHY enable/disable calls to happen from
> omap2430_musb_enable/disable(). That broke enumeration for several
> devices as PM runtime in the PHY will never enable it.
> 
> The root cause of the problem is unpaired calls from musb_core.c to
> musb_platform_enable/disable in musb_core.c as reported by
> Andreas Kemnade <andreas@kemnade.info>.
> 
> As musb_platform_enable/disable are being called from various
> functions, let's not attempt to make them paiered immediately. This
> would require fixing also musb_stop as it currently calls
> musb_platform_disable.
> 
> Instead, let's first fix the regression in a minimal way by removing
> the initial call to musb_platform_disable.
> 
> AFAIK the initial musb_platform_disable call has always been just an
> attempted workaround for the 2430 glue layer announcing itself too
> early before the gadgets are configured. And that issue finally
> got fixed with commit a118df07f5b1 ("usb: musb: Don't set d+ high
> before enable for 2430 glue layer").
> 
> We now also need to fix the twl4030-phy accordingly making it's
> PM runtime call only needed in twl4030_phy_power_on and have it
> autosuspend. The cable state will keep the phy active when connected.
> 
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Reported-by: Andreas Kemnade <andreas@kemnade.info>
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
> for 2430 glue layer")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -447,8 +447,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;
>  }
> @@ -465,6 +463,8 @@ static int twl4030_phy_power_on(struct phy *phy)
>  		twl4030_i2c_access(twl, 0);
>  	twl->linkstat = MUSB_UNKNOWN;
>  	schedule_delayed_work(&twl->id_workaround_work, HZ);
> +	pm_runtime_mark_last_busy(twl->dev);
> +	pm_runtime_put_autosuspend(twl->dev);
>  
>  	return 0;
>  }
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2142,7 +2142,6 @@ musb_init_controller(struct device *dev, int
> nIrq, void __iomem *ctrl) }
>  
>  	/* be sure interrupts are disabled before connecting ISR */
> -	musb_platform_disable(musb);
>  	musb_generic_disable(musb);
>  
>  	/* Init IRQ workqueue before request_irq */
> 
> 

[toc] | [prev] | [next] | [standalone]

#1480622

* Andreas Kemnade <andreas@kemnade.info> [160910 04:27]:
> On Fri, 9 Sep 2016 16:40:15 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> > > * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > > > We have two independant things:
> > > > 1. phy-twl4030-usb (and perhaps others) do not enable
> > > >   the phy enough to allow charging on pm_runtime_get().
> > > >   That is fixed by my phy-related patches.
> > > 
> > > OK
> > >
> > > > 2. phy_power_off/on() in called in an unbalanced way if
> > > >    it is called behind musb_platform_enable()/disable()
> > > >    as it happens in omap2430.c. Two ways to fix it:
> > > >    a) prevent phy_power_off()/on() to be called in
> > > >       an unbalanced way in omap240.c
> > > >    b) prevent musb_platform_enable()
> > > > 	      musb_platform_disable() to be called in an
> > > > 	      unbalanced way by fixing musb_core.c
> > > > 
> > > > Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > > > have gadget working for the most common usecases. (not using
> > > > twl4030-charger would not work yet)
> > > > But in the longer term 2. has to be fixed too.
> > > 
> > > Sounds like option 2b here is the real fix.
> > 
> > And doing full option 2b would be intrusive because of musb_stop
> > also calling musb_platform_disable. Here's a suggested fix for
> > v4.8-rc cycle.
> > 
> musb_platform_disable() in musb_stop() seems to be balanced.
> 
> from my list in an earlier mail:
> musb_platform_disable() in musb_remove() called upon module removal
> 
> To my analysis this is odd because musb_stop() is also called indirectly
> upon removal.
> But topic that can be left for later.

OK will update the patch description. You probaby should attempt
to do a patch to make the calls paired as you already know what
needs to be done there..

> > Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> > omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> > black using dsps glue. Also PM runtime works on omap3.
> > 
> > This patch causes a slight merge conlict with Andrea's patches,
> > as listed in #1 above, but we should probably merge this first
> > as a fix. That is assuming it does not cause side effects to
> > Andrea's phy-twl4030-usb charger test case.
> > 
> Hmm, if the patch will gender-change me, then a clear NAK ;-)

Sorry typo there, s/Andrea/Andreas/, no need for other radical
changes :)

> > Can you guys please test? If things work I'll resend the
> > patch with proper tested-bys and acks.
> > 
> I tested the patch without any other musb/phy-fixes.
> No regressions. It fixes Gadget to be usable. Charging seems
> not to be totally stable. I will check my phy-patches
> on top of that again. 
> PM runtime probably still desires some work on it.
> But I give a clear Ack for merging this one first.

OK good to hear & thanks for testing. Let's see what Laurent
says.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1480756

Hi Tony,

On Saturday 10 Sep 2016 06:07:50 Tony Lindgren wrote:
> * Andreas Kemnade <andreas@kemnade.info> [160910 04:27]:
> > On Fri, 9 Sep 2016 16:40:15 -0700 Tony Lindgren wrote:
> >> * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> >>> * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> >>>> We have two independant things:
> >>>> 1. phy-twl4030-usb (and perhaps others) do not enable
> >>>> 
> >>>>   the phy enough to allow charging on pm_runtime_get().
> >>>>   That is fixed by my phy-related patches.
> >>> 
> >>> OK
> >>> 
> >>>> 2. phy_power_off/on() in called in an unbalanced way if
> >>>>    it is called behind musb_platform_enable()/disable()
> >>>>    as it happens in omap2430.c. Two ways to fix it:
> >>>>    a) prevent phy_power_off()/on() to be called in
> >>>>       an unbalanced way in omap240.c
> >>>>    
> >>>>    b) prevent musb_platform_enable()
> >>>> 	      musb_platform_disable() to be called in an
> >>>> 	      unbalanced way by fixing musb_core.c
> >>>> 
> >>>> Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> >>>> have gadget working for the most common usecases. (not using
> >>>> twl4030-charger would not work yet)
> >>>> But in the longer term 2. has to be fixed too.
> >>> 
> >>> Sounds like option 2b here is the real fix.
> >> 
> >> And doing full option 2b would be intrusive because of musb_stop
> >> also calling musb_platform_disable. Here's a suggested fix for
> >> v4.8-rc cycle.
> > 
> > musb_platform_disable() in musb_stop() seems to be balanced.
> > 
> > from my list in an earlier mail:
> > musb_platform_disable() in musb_remove() called upon module removal
> > 
> > To my analysis this is odd because musb_stop() is also called indirectly
> > upon removal.
> > But topic that can be left for later.
> 
> OK will update the patch description. You probaby should attempt
> to do a patch to make the calls paired as you already know what
> needs to be done there..
> 
> >> Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> >> omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> >> black using dsps glue. Also PM runtime works on omap3.
> >> 
> >> This patch causes a slight merge conlict with Andrea's patches,
> >> as listed in #1 above, but we should probably merge this first
> >> as a fix. That is assuming it does not cause side effects to
> >> Andrea's phy-twl4030-usb charger test case.
> > 
> > Hmm, if the patch will gender-change me, then a clear NAK ;-)
> 
> Sorry typo there, s/Andrea/Andreas/, no need for other radical
> changes :)
> 
> >> Can you guys please test? If things work I'll resend the
> >> patch with proper tested-bys and acks.
> > 
> > I tested the patch without any other musb/phy-fixes.
> > No regressions. It fixes Gadget to be usable. Charging seems
> > not to be totally stable. I will check my phy-patches
> > on top of that again.
> > PM runtime probably still desires some work on it.
> > But I give a clear Ack for merging this one first.
> 
> OK good to hear & thanks for testing. Let's see what Laurent
> says.

I say

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you for the fix.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1481359

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160911 02:06]:
> Hi Tony,
> 
> On Saturday 10 Sep 2016 06:07:50 Tony Lindgren wrote:
> > * Andreas Kemnade <andreas@kemnade.info> [160910 04:27]:
> > > On Fri, 9 Sep 2016 16:40:15 -0700 Tony Lindgren wrote:
> > >> * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> > >>> * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > >>>> We have two independant things:
> > >>>> 1. phy-twl4030-usb (and perhaps others) do not enable
> > >>>> 
> > >>>>   the phy enough to allow charging on pm_runtime_get().
> > >>>>   That is fixed by my phy-related patches.
> > >>> 
> > >>> OK
> > >>> 
> > >>>> 2. phy_power_off/on() in called in an unbalanced way if
> > >>>>    it is called behind musb_platform_enable()/disable()
> > >>>>    as it happens in omap2430.c. Two ways to fix it:
> > >>>>    a) prevent phy_power_off()/on() to be called in
> > >>>>       an unbalanced way in omap240.c
> > >>>>    
> > >>>>    b) prevent musb_platform_enable()
> > >>>> 	      musb_platform_disable() to be called in an
> > >>>> 	      unbalanced way by fixing musb_core.c
> > >>>> 
> > >>>> Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > >>>> have gadget working for the most common usecases. (not using
> > >>>> twl4030-charger would not work yet)
> > >>>> But in the longer term 2. has to be fixed too.
> > >>> 
> > >>> Sounds like option 2b here is the real fix.
> > >> 
> > >> And doing full option 2b would be intrusive because of musb_stop
> > >> also calling musb_platform_disable. Here's a suggested fix for
> > >> v4.8-rc cycle.
> > > 
> > > musb_platform_disable() in musb_stop() seems to be balanced.
> > > 
> > > from my list in an earlier mail:
> > > musb_platform_disable() in musb_remove() called upon module removal
> > > 
> > > To my analysis this is odd because musb_stop() is also called indirectly
> > > upon removal.
> > > But topic that can be left for later.
> > 
> > OK will update the patch description. You probaby should attempt
> > to do a patch to make the calls paired as you already know what
> > needs to be done there..
> > 
> > >> Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> > >> omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> > >> black using dsps glue. Also PM runtime works on omap3.
> > >> 
> > >> This patch causes a slight merge conlict with Andrea's patches,
> > >> as listed in #1 above, but we should probably merge this first
> > >> as a fix. That is assuming it does not cause side effects to
> > >> Andrea's phy-twl4030-usb charger test case.
> > > 
> > > Hmm, if the patch will gender-change me, then a clear NAK ;-)
> > 
> > Sorry typo there, s/Andrea/Andreas/, no need for other radical
> > changes :)
> > 
> > >> Can you guys please test? If things work I'll resend the
> > >> patch with proper tested-bys and acks.
> > > 
> > > I tested the patch without any other musb/phy-fixes.
> > > No regressions. It fixes Gadget to be usable. Charging seems
> > > not to be totally stable. I will check my phy-patches
> > > on top of that again.
> > > PM runtime probably still desires some work on it.
> > > But I give a clear Ack for merging this one first.
> > 
> > OK good to hear & thanks for testing. Let's see what Laurent
> > says.
> 
> I say
> 
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thank you for the fix.

OK great, will send a proper patch with acks shortly.

Thanks,

Tony

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web