[PATCHv2] usb: musb: Fix unbalanced platform_disable

[PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.

As musb_platform_enable/disable are being called from various functions,
let's not attempt to make them paiered immediately. This would require
fixing all the callers like musb_remove.

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-l0cyMroinI0@public.gmane.org>
Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
for 2430 glue layer")
Reported-by: Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>
Acked-by: Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>
Reported-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---

Changes since v1: Updated description

This is needed as a regression fix for the v4.8-rc cycle please.

---
 drivers/phy/phy-twl4030-usb.c | 4 ++--
 drivers/usb/musb/musb_core.c  | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

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
@@ -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;
 }
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
--- 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 */
-- 
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Bin Liu]

Hi Tony,

On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> 
> As musb_platform_enable/disable are being called from various functions,
> let's not attempt to make them paiered immediately. This would require
> fixing all the callers like musb_remove.
> 
> 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

Many glue layers rely on musb_platform_diable to disable interrupts in
musb_init_controller() before registering ISR, is it safe to assume the
interrupts will be masked when musb is out-of-reset so that we don't
have to call musb_platform_disable() in musb_init_controller()?

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> Hi Tony,
> 
> On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > 
> > As musb_platform_enable/disable are being called from various functions,
> > let's not attempt to make them paiered immediately. This would require
> > fixing all the callers like musb_remove.
> > 
> > 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
> 
> Many glue layers rely on musb_platform_diable to disable interrupts in
> musb_init_controller() before registering ISR, is it safe to assume the
> interrupts will be masked when musb is out-of-reset so that we don't
> have to call musb_platform_disable() in musb_init_controller()?

It should be, we do request_irq only later on after this in musb_core.c.
And the glue layers don't do request_irq except for the separate DMA
interrupts in two cases.

And as the platform glue layer are the ones doing the probing, they
should initialize things into sane state :)

We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
request_irq. But I'm guessing there's no need to.. Do you have some
example in mind that should be tested?

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Bin Liu]

On Mon, Sep 12, 2016 at 10:19:40AM -0700, Tony Lindgren wrote:
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> > Hi Tony,
> > 
> > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > > 
> > > As musb_platform_enable/disable are being called from various functions,
> > > let's not attempt to make them paiered immediately. This would require
> > > fixing all the callers like musb_remove.
> > > 
> > > 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
> > 
> > Many glue layers rely on musb_platform_diable to disable interrupts in
> > musb_init_controller() before registering ISR, is it safe to assume the
> > interrupts will be masked when musb is out-of-reset so that we don't
> > have to call musb_platform_disable() in musb_init_controller()?
> 
> It should be, we do request_irq only later on after this in musb_core.c.
> And the glue layers don't do request_irq except for the separate DMA
> interrupts in two cases.

Yeah, the glue layer does not do request_irq, core does it after called
musb_platform_disable(), in which some glues mask interrupts.

If musb_init_controller() no longer calls musb_platform_disable(), it
kinda worries me. I have asked around, no one says that it is a safe
assumption that interrupt will be masked when out-of-reset, though it is
common sense.

This change should not break any glue layer which relies on
musb_platform_disable() to disable interrupts.

So Kishon, if you take this patch in your tree, here is my 

Signed-off-by: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>

Regards,
-Bin.

> 
> And as the platform glue layer are the ones doing the probing, they
> should initialize things into sane state :)
> 
> We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> request_irq. But I'm guessing there's no need to.. Do you have some
> example in mind that should be tested?
> 
> Regards,
> 
> Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160912 10:26]:
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> > Hi Tony,
> > 
> > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > > 
> > > As musb_platform_enable/disable are being called from various functions,
> > > let's not attempt to make them paiered immediately. This would require
> > > fixing all the callers like musb_remove.
> > > 
> > > 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
> > 
> > Many glue layers rely on musb_platform_diable to disable interrupts in
> > musb_init_controller() before registering ISR, is it safe to assume the
> > interrupts will be masked when musb is out-of-reset so that we don't
> > have to call musb_platform_disable() in musb_init_controller()?
> 
> It should be, we do request_irq only later on after this in musb_core.c.
> And the glue layers don't do request_irq except for the separate DMA
> interrupts in two cases.
> 
> And as the platform glue layer are the ones doing the probing, they
> should initialize things into sane state :)
> 
> We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> request_irq. But I'm guessing there's no need to.. Do you have some
> example in mind that should be tested?

Oh I see davinci_musb_disable and am35x_musb_disable reset devctl and
clear interrupts. I'll try to check on am3517 here today, don't have
any davinci boards configured here.

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Bin Liu]

On Mon, Sep 12, 2016 at 12:32:42PM -0500, Bin Liu wrote:
> On Mon, Sep 12, 2016 at 10:19:40AM -0700, Tony Lindgren wrote:
> > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> > > Hi Tony,
> > > 
> > > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > > 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > > > 
> > > > As musb_platform_enable/disable are being called from various functions,
> > > > let's not attempt to make them paiered immediately. This would require
> > > > fixing all the callers like musb_remove.
> > > > 
> > > > 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
> > > 
> > > Many glue layers rely on musb_platform_diable to disable interrupts in
> > > musb_init_controller() before registering ISR, is it safe to assume the
> > > interrupts will be masked when musb is out-of-reset so that we don't
> > > have to call musb_platform_disable() in musb_init_controller()?
> > 
> > It should be, we do request_irq only later on after this in musb_core.c.
> > And the glue layers don't do request_irq except for the separate DMA
> > interrupts in two cases.
> 
> Yeah, the glue layer does not do request_irq, core does it after called
> musb_platform_disable(), in which some glues mask interrupts.
> 
> If musb_init_controller() no longer calls musb_platform_disable(), it
> kinda worries me. I have asked around, no one says that it is a safe
> assumption that interrupt will be masked when out-of-reset, though it is
> common sense.
> 
> This change should not break any glue layer which relies on
> musb_platform_disable() to disable interrupts.
> 
> So Kishon, if you take this patch in your tree, here is my 
> 
> Signed-off-by: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>

Ahh, should be

Acked-by: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>

> 
> Regards,
> -Bin.
> 
> > 
> > And as the platform glue layer are the ones doing the probing, they
> > should initialize things into sane state :)
> > 
> > We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> > request_irq. But I'm guessing there's no need to.. Do you have some
> > example in mind that should be tested?
> > 
> > Regards,
> > 
> > Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Andreas Kemnade]

On Mon, 12 Sep 2016 10:34:08 -0700
Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:

> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160912 10:26]:
> > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> > > Hi Tony,
> > > 
> > > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > > 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > > > 
> > > > As musb_platform_enable/disable are being called from various
> > > > functions, let's not attempt to make them paiered immediately.
> > > > This would require fixing all the callers like musb_remove.
> > > > 
> > > > 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
> > > 
> > > Many glue layers rely on musb_platform_diable to disable
> > > interrupts in musb_init_controller() before registering ISR, is
> > > it safe to assume the interrupts will be masked when musb is
> > > out-of-reset so that we don't have to call
> > > musb_platform_disable() in musb_init_controller()?
> > 
> > It should be, we do request_irq only later on after this in
> > musb_core.c. And the glue layers don't do request_irq except for
> > the separate DMA interrupts in two cases.
> > 
> > And as the platform glue layer are the ones doing the probing, they
> > should initialize things into sane state :)
> > 
> > We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> > request_irq. But I'm guessing there's no need to.. Do you have some
> > example in mind that should be tested?
> 
> Oh I see davinci_musb_disable and am35x_musb_disable reset devctl and
> clear interrupts. I'll try to check on am3517 here today, don't have
> any davinci boards configured here.
> 
Hmm, then the question is: Couldn't the X_musb_disable simply be called
from X_probe if needed to be an the safe side?

Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160912 11:05]:
> On Mon, 12 Sep 2016 10:34:08 -0700
> Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160912 10:26]:
> > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> > > > Hi Tony,
> > > > 
> > > > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > > > 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > > > > 
> > > > > As musb_platform_enable/disable are being called from various
> > > > > functions, let's not attempt to make them paiered immediately.
> > > > > This would require fixing all the callers like musb_remove.
> > > > > 
> > > > > 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
> > > > 
> > > > Many glue layers rely on musb_platform_diable to disable
> > > > interrupts in musb_init_controller() before registering ISR, is
> > > > it safe to assume the interrupts will be masked when musb is
> > > > out-of-reset so that we don't have to call
> > > > musb_platform_disable() in musb_init_controller()?
> > > 
> > > It should be, we do request_irq only later on after this in
> > > musb_core.c. And the glue layers don't do request_irq except for
> > > the separate DMA interrupts in two cases.
> > > 
> > > And as the platform glue layer are the ones doing the probing, they
> > > should initialize things into sane state :)
> > > 
> > > We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> > > request_irq. But I'm guessing there's no need to.. Do you have some
> > > example in mind that should be tested?
> > 
> > Oh I see davinci_musb_disable and am35x_musb_disable reset devctl and
> > clear interrupts. I'll try to check on am3517 here today, don't have
> > any davinci boards configured here.
> > 
> Hmm, then the question is: Couldn't the X_musb_disable simply be called
> from X_probe if needed to be an the safe side?

Yes agreed, that's the sane way to deal with any potential issues
there.

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160912 11:13]:
> * Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160912 11:05]:
> > On Mon, 12 Sep 2016 10:34:08 -0700
> > Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > > 
> > > Oh I see davinci_musb_disable and am35x_musb_disable reset devctl and
> > > clear interrupts. I'll try to check on am3517 here today, don't have
> > > any davinci boards configured here.
> > > 
> > Hmm, then the question is: Couldn't the X_musb_disable simply be called
> > from X_probe if needed to be an the safe side?
> 
> Yes agreed, that's the sane way to deal with any potential issues
> there.

And looks like musb on am3517 has been broken for years, we don't
even have a PHY for it.

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Bin Liu]

On Mon, Sep 12, 2016 at 08:05:30PM +0200, Andreas Kemnade wrote:
> On Mon, 12 Sep 2016 10:34:08 -0700
> Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160912 10:26]:
> > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> > > > Hi Tony,
> > > > 
> > > > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > > > 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > > > > 
> > > > > As musb_platform_enable/disable are being called from various
> > > > > functions, let's not attempt to make them paiered immediately.
> > > > > This would require fixing all the callers like musb_remove.
> > > > > 
> > > > > 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
> > > > 
> > > > Many glue layers rely on musb_platform_diable to disable
> > > > interrupts in musb_init_controller() before registering ISR, is
> > > > it safe to assume the interrupts will be masked when musb is
> > > > out-of-reset so that we don't have to call
> > > > musb_platform_disable() in musb_init_controller()?
> > > 
> > > It should be, we do request_irq only later on after this in
> > > musb_core.c. And the glue layers don't do request_irq except for
> > > the separate DMA interrupts in two cases.
> > > 
> > > And as the platform glue layer are the ones doing the probing, they
> > > should initialize things into sane state :)
> > > 
> > > We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> > > request_irq. But I'm guessing there's no need to.. Do you have some
> > > example in mind that should be tested?
> > 
> > Oh I see davinci_musb_disable and am35x_musb_disable reset devctl and
> > clear interrupts. I'll try to check on am3517 here today, don't have
> > any davinci boards configured here.
> > 
> Hmm, then the question is: Couldn't the X_musb_disable simply be called
> from X_probe if needed to be an the safe side?

In general, we try not to do so if all possible. We want to put common
code in the core, not repleat them in glue layers.

In this specific case, we cannot do it. For example in dsps glue, the
musb reset is done in dsps_musb_init(), so no place in dsps_probe() to
call dsps_musb_disable().

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 11:36]:
> On Mon, Sep 12, 2016 at 08:05:30PM +0200, Andreas Kemnade wrote:
> > Hmm, then the question is: Couldn't the X_musb_disable simply be called
> > from X_probe if needed to be an the safe side?
> 
> In general, we try not to do so if all possible. We want to put common
> code in the core, not repleat them in glue layers.
> 
> In this specific case, we cannot do it. For example in dsps glue, the
> musb reset is done in dsps_musb_init(), so no place in dsps_probe() to
> call dsps_musb_disable().

OK yeah if we need to do something, then disabling the irq in
musb_core.c until we're done should be enough.

Anyways, I tested the $subject patch also with am35x code with the
following patch and no issues. So I've now tested with omap3,
omap4, am335x, and am35x.

There are also am35x musb device tree patches from last year by
Rolf Peukert <rolf.peukert-cSCHQGRfioA@public.gmane.org>:

https://patchwork.kernel.org/project/linux-omap/list/?submitter=144061

And we now have new drivers/phy/phy-da8xx-usb.c that might be the
same phy on am35x except with register bits moved around.

So maybe we'll have it working with device tree quite easily.
Meanwhile, we should probably apply the following patch so we
get things working again.

Regards,

Tony

8< --------------------------
diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
--- a/arch/arm/boot/dts/am3517.dtsi
+++ b/arch/arm/boot/dts/am3517.dtsi
@@ -90,5 +90,10 @@
 	status = "disabled";
 };
 
+/* Only on 34xx/36xx */
+&usb_otg_hs {
+	status = "disabled";
+};
+
 /include/ "am35xx-clocks.dtsi"
 /include/ "omap36xx-am35xx-omap3430es2plus-clocks.dtsi"
diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -12,6 +12,7 @@
 #include <linux/gpio.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/usb/musb.h>
 #include <linux/of_platform.h>
 #include <linux/ti_wilink_st.h>
 #include <linux/wl12xx.h>
@@ -38,6 +39,7 @@
 #include "omap-secure.h"
 #include "soc.h"
 #include "hsmmc.h"
+#include "usb.h"
 
 static struct omap_hsmmc_platform_data __maybe_unused mmc_pdata[2];
 
@@ -261,9 +263,39 @@ static void __init omap3_sbc_t3517_legacy_init(void)
 	omap3_sbc_t3517_wifi_init();
 }
 
+static struct omap_musb_board_data am3517_musb_board_data = {
+	.interface_type		= MUSB_INTERFACE_ULPI,
+	.mode			= MUSB_OTG,
+	.power			= 500,
+	.set_phy_power		= am35x_musb_phy_power,
+	.clear_irq		= am35x_musb_clear_irq,
+	.set_mode		= am35x_set_mode,
+	.reset			= am35x_musb_reset,
+};
+
+static __init void am3517_evm_musb_init(void)
+{
+	u32 devconf2;
+
+	/*
+	 * Set up USB clock/mode in the DEVCONF2 register.
+	 */
+	devconf2 = omap_ctrl_readl(AM35XX_CONTROL_DEVCONF2);
+
+	/* USB2.0 PHY reference clock is 13 MHz */
+	devconf2 &= ~(CONF2_REFFREQ | CONF2_OTGMODE | CONF2_PHY_GPIOMODE);
+	devconf2 |=  CONF2_REFFREQ_13MHZ | CONF2_SESENDEN | CONF2_VBDTCTEN
+		| CONF2_DATPOL;
+
+	omap_ctrl_writel(devconf2, AM35XX_CONTROL_DEVCONF2);
+
+	usb_musb_init(&am3517_musb_board_data);
+}
+
 static void __init am3517_evm_legacy_init(void)
 {
 	am35xx_emac_reset();
+	am3517_evm_musb_init();
 }
 
 static struct platform_device omap3_rom_rng_device = {
diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
--- a/arch/arm/mach-omap2/usb-musb.c
+++ b/arch/arm/mach-omap2/usb-musb.c
@@ -82,8 +82,13 @@ void __init usb_musb_init(struct omap_musb_board_data *musb_board_data)
 	musb_plat.mode = board_data->mode;
 	musb_plat.extvbus = board_data->extvbus;
 
-	oh_name = "usb_otg_hs";
-	name = "musb-omap2430";
+	if (soc_is_am35xx()) {
+		oh_name = "am35x_otg_hs";
+		name = "musb-am35x";
+	} else {
+		oh_name = "usb_otg_hs";
+		name = "musb-omap2430";
+	}
 
         oh = omap_hwmod_lookup(oh_name);
         if (WARN(!oh, "%s: could not find omap_hwmod for %s\n",
diff --git a/arch/arm/mach-omap2/usb.h b/arch/arm/mach-omap2/usb.h
--- a/arch/arm/mach-omap2/usb.h
+++ b/arch/arm/mach-omap2/usb.h
@@ -1,3 +1,6 @@
+#ifndef __ARCH_ARM_MACH_OMAP2_USB_H
+#define __ARCH_ARM_MACH_OMAP2_USB_H
+
 #include <linux/platform_data/usb-omap.h>
 
 /* AM35x */
@@ -22,37 +25,6 @@
 #define CONF2_OTGPWRDN		(1 << 2)
 #define CONF2_DATPOL		(1 << 1)
 
-/* TI81XX specific definitions */
-#define USBCTRL0	0x620
-#define USBSTAT0	0x624
-
-/* TI816X PHY controls bits */
-#define TI816X_USBPHY0_NORMAL_MODE	(1 << 0)
-#define TI816X_USBPHY_REFCLK_OSC	(1 << 8)
-
-/* TI814X PHY controls bits */
-#define USBPHY_CM_PWRDN		(1 << 0)
-#define USBPHY_OTG_PWRDN	(1 << 1)
-#define USBPHY_CHGDET_DIS	(1 << 2)
-#define USBPHY_CHGDET_RSTRT	(1 << 3)
-#define USBPHY_SRCONDM		(1 << 4)
-#define USBPHY_SINKONDP		(1 << 5)
-#define USBPHY_CHGISINK_EN	(1 << 6)
-#define USBPHY_CHGVSRC_EN	(1 << 7)
-#define USBPHY_DMPULLUP		(1 << 8)
-#define USBPHY_DPPULLUP		(1 << 9)
-#define USBPHY_CDET_EXTCTL	(1 << 10)
-#define USBPHY_GPIO_MODE	(1 << 12)
-#define USBPHY_DPOPBUFCTL	(1 << 13)
-#define USBPHY_DMOPBUFCTL	(1 << 14)
-#define USBPHY_DPINPUT		(1 << 15)
-#define USBPHY_DMINPUT		(1 << 16)
-#define USBPHY_DPGPIO_PD	(1 << 17)
-#define USBPHY_DMGPIO_PD	(1 << 18)
-#define USBPHY_OTGVDET_EN	(1 << 19)
-#define USBPHY_OTGSESSEND_EN	(1 << 20)
-#define USBPHY_DATA_POLARITY	(1 << 23)

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Bin Liu]

Hi,

On Mon, Sep 12, 2016 at 08:18:05PM -0700, Tony Lindgren wrote:
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 11:36]:
> > On Mon, Sep 12, 2016 at 08:05:30PM +0200, Andreas Kemnade wrote:
> > > Hmm, then the question is: Couldn't the X_musb_disable simply be called
> > > from X_probe if needed to be an the safe side?
> > 
> > In general, we try not to do so if all possible. We want to put common
> > code in the core, not repleat them in glue layers.
> > 
> > In this specific case, we cannot do it. For example in dsps glue, the
> > musb reset is done in dsps_musb_init(), so no place in dsps_probe() to
> > call dsps_musb_disable().
> 
> OK yeah if we need to do something, then disabling the irq in
> musb_core.c until we're done should be enough.

I don't think we can disable it in musb core directly. *_musb_disable()
in glue layers disables the musb wrapper's irq, not the core's irq.

But I guess we can let *_musb_init() directly calls *_musb_disable() in
the glue drivers right after musb reset. This would have to touch almost
all glue drivers, but trivial change. Thoughts?

> 
> Anyways, I tested the $subject patch also with am35x code with the
> following patch and no issues. So I've now tested with omap3,
> omap4, am335x, and am35x.
 
I don't expect your v2 breaks am35x either, as I believe its default of
out-of-reset already disables the irq. I just don't like that SW relies
on unguaranteed HW default state.

> There are also am35x musb device tree patches from last year by
> Rolf Peukert <rolf.peukert-cSCHQGRfioA@public.gmane.org>:
> 
> https://patchwork.kernel.org/project/linux-omap/list/?submitter=144061

Good to know this work.

> 
> And we now have new drivers/phy/phy-da8xx-usb.c that might be the
> same phy on am35x except with register bits moved around.
> 
> So maybe we'll have it working with device tree quite easily.
> Meanwhile, we should probably apply the following patch so we
> get things working again.

Yup.

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Laurent Pinchart]

Hi Bin,

On Tuesday 13 Sep 2016 09:14:48 Bin Liu wrote:
> On Mon, Sep 12, 2016 at 08:18:05PM -0700, Tony Lindgren wrote:
> > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 11:36]:
> > > On Mon, Sep 12, 2016 at 08:05:30PM +0200, Andreas Kemnade wrote:
> > > > Hmm, then the question is: Couldn't the X_musb_disable simply be
> > > > called
> > > > from X_probe if needed to be an the safe side?
> > > 
> > > In general, we try not to do so if all possible. We want to put common
> > > code in the core, not repleat them in glue layers.
> > > 
> > > In this specific case, we cannot do it. For example in dsps glue, the
> > > musb reset is done in dsps_musb_init(), so no place in dsps_probe() to
> > > call dsps_musb_disable().
> > 
> > OK yeah if we need to do something, then disabling the irq in
> > musb_core.c until we're done should be enough.
> 
> I don't think we can disable it in musb core directly. *_musb_disable()
> in glue layers disables the musb wrapper's irq, not the core's irq.
> 
> But I guess we can let *_musb_init() directly calls *_musb_disable() in
> the glue drivers right after musb reset. This would have to touch almost
> all glue drivers, but trivial change. Thoughts?

Is there any use case for starting a glue layer with interrupts enabled ? If 
not I agree that all glue layers should do so in *_musb_init(). Probably not 
by calling *_musb_disable() directly though, as that would reintroduce the 
unbalanced PHY power control problem.

> > Anyways, I tested the $subject patch also with am35x code with the
> > following patch and no issues. So I've now tested with omap3,
> > omap4, am335x, and am35x.
> 
> I don't expect your v2 breaks am35x either, as I believe its default of
> out-of-reset already disables the irq. I just don't like that SW relies
> on unguaranteed HW default state.
> 
> > There are also am35x musb device tree patches from last year by
> > Rolf Peukert <rolf.peukert-cSCHQGRfioA@public.gmane.org>:
> > 
> > https://patchwork.kernel.org/project/linux-omap/list/?submitter=144061
> 
> Good to know this work.
> 
> > And we now have new drivers/phy/phy-da8xx-usb.c that might be the
> > same phy on am35x except with register bits moved around.
> > 
> > So maybe we'll have it working with device tree quite easily.
> > Meanwhile, we should probably apply the following patch so we
> > get things working again.
> 
> Yup.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Bin Liu]

On Tue, Sep 13, 2016 at 05:32:23PM +0300, Laurent Pinchart wrote:
> Hi Bin,
> 
> On Tuesday 13 Sep 2016 09:14:48 Bin Liu wrote:
> > On Mon, Sep 12, 2016 at 08:18:05PM -0700, Tony Lindgren wrote:
> > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 11:36]:
> > > > On Mon, Sep 12, 2016 at 08:05:30PM +0200, Andreas Kemnade wrote:
> > > > > Hmm, then the question is: Couldn't the X_musb_disable simply be
> > > > > called
> > > > > from X_probe if needed to be an the safe side?
> > > > 
> > > > In general, we try not to do so if all possible. We want to put common
> > > > code in the core, not repleat them in glue layers.
> > > > 
> > > > In this specific case, we cannot do it. For example in dsps glue, the
> > > > musb reset is done in dsps_musb_init(), so no place in dsps_probe() to
> > > > call dsps_musb_disable().
> > > 
> > > OK yeah if we need to do something, then disabling the irq in
> > > musb_core.c until we're done should be enough.
> > 
> > I don't think we can disable it in musb core directly. *_musb_disable()
> > in glue layers disables the musb wrapper's irq, not the core's irq.
> > 
> > But I guess we can let *_musb_init() directly calls *_musb_disable() in
> > the glue drivers right after musb reset. This would have to touch almost
> > all glue drivers, but trivial change. Thoughts?
> 
> Is there any use case for starting a glue layer with interrupts enabled ? If 

I cannot think of any, at least in the current musb drivers.

> not I agree that all glue layers should do so in *_musb_init(). Probably not 
> by calling *_musb_disable() directly though, as that would reintroduce the 
> unbalanced PHY power control problem.

Not a problem, such change will be done in each glue driver, so we don't
do it for omap2430 glue.

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160913 07:40]:
> On Tue, Sep 13, 2016 at 05:32:23PM +0300, Laurent Pinchart wrote:
> > Hi Bin,
> > 
> > On Tuesday 13 Sep 2016 09:14:48 Bin Liu wrote:
> > > On Mon, Sep 12, 2016 at 08:18:05PM -0700, Tony Lindgren wrote:
> > > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 11:36]:
> > > > > On Mon, Sep 12, 2016 at 08:05:30PM +0200, Andreas Kemnade wrote:
> > > > > > Hmm, then the question is: Couldn't the X_musb_disable simply be
> > > > > > called
> > > > > > from X_probe if needed to be an the safe side?
> > > > > 
> > > > > In general, we try not to do so if all possible. We want to put common
> > > > > code in the core, not repleat them in glue layers.
> > > > > 
> > > > > In this specific case, we cannot do it. For example in dsps glue, the
> > > > > musb reset is done in dsps_musb_init(), so no place in dsps_probe() to
> > > > > call dsps_musb_disable().
> > > > 
> > > > OK yeah if we need to do something, then disabling the irq in
> > > > musb_core.c until we're done should be enough.
> > > 
> > > I don't think we can disable it in musb core directly. *_musb_disable()
> > > in glue layers disables the musb wrapper's irq, not the core's irq.
> > > 
> > > But I guess we can let *_musb_init() directly calls *_musb_disable() in
> > > the glue drivers right after musb reset. This would have to touch almost
> > > all glue drivers, but trivial change. Thoughts?
> > 
> > Is there any use case for starting a glue layer with interrupts enabled ? If 
> 
> I cannot think of any, at least in the current musb drivers.

No reason that I can think of :)

> > not I agree that all glue layers should do so in *_musb_init(). Probably not 
> > by calling *_musb_disable() directly though, as that would reintroduce the 
> > unbalanced PHY power control problem.
> 
> Not a problem, such change will be done in each glue driver, so we don't
> do it for omap2430 glue.

Makes sense. Then we can just move all of musb_generic_enable/disable
code into pm_runtime_resume/suspend in musb-core.c and make sure they
are paired.

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Laurent Pinchart]

Hi Tony,

On Monday 12 Sep 2016 08:39:49 Tony Lindgren wrote:
> 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-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> 
> As musb_platform_enable/disable are being called from various functions,
> let's not attempt to make them paiered immediately. This would require
> fixing all the callers like musb_remove.
> 
> 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-l0cyMroinI0@public.gmane.org>
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
> for 2430 glue layer")
> Reported-by: Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>
> Acked-by: Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>
> Reported-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

FYI, while this patch allows me to boot my Panda board with NFS over usbnet, 
it only works with cold boots. A warm reboot results in the following warning, 
and no ethernet traffic going through. The USB device is detected by the host 
though.

I'm not sure whether this is a regression introduced by commit a83e17d0f73b 
("usb: musb: Improve PM runtime and phy handling for 2430 glue layer") or an 
entirely different issue;

[    5.509918] ------------[ cut here ]------------
[    5.509918] WARNING: CPU: 0 PID: 1 at 
/home/laurent/src/kernel/omap4/linux-2.6/drivers/bus/omap_l3_noc.c:147 
l3_interrupt_handler+0x220/0x348
[    5.528137] 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Read): 
Data Access in User mode during Functional access
[    5.539825] Modules linked in:
[    5.539825] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc2-00806-
g37c4efacd6b9 #4
[    5.539825] Hardware name: Generic OMAP4 (Flattened Device Tree)
[    5.551361] [<c01101b0>] (unwind_backtrace) from [<c010c10c>] 
(show_stack+0x10/0x14)
[    5.565826] [<c010c10c>] (show_stack) from [<c048ca40>] 
(dump_stack+0xa8/0xe0)
[    5.565826] [<c048ca40>] (dump_stack) from [<c0137910>] (__warn+0xd8/0x104)
[    5.565826] [<c0137910>] (__warn) from [<c01379e4>] 
(warn_slowpath_fmt+0x38/0x48)
[    5.588623] [<c01379e4>] (warn_slowpath_fmt) from [<c04befd0>] 
(l3_interrupt_handler+0x220/0x348)
[    5.588623] [<c04befd0>] (l3_interrupt_handler) from [<c019ec7c>] 
(__handle_irq_event_percpu+0x98/0x3ec)
[    5.588623] [<c019ec7c>] (__handle_irq_event_percpu) from [<c019efec>] 
(handle_irq_event_percpu+0x1c/0x58)
[    5.588623] [<c019efec>] (handle_irq_event_percpu) from [<c019f060>] 
(handle_irq_event+0x38/0x5c)
[    5.627380] [<c019f060>] (handle_irq_event) from [<c01a24e0>] 
(handle_fasteoi_irq+0xcc/0x1a4)
[    5.627380] [<c01a24e0>] (handle_fasteoi_irq) from [<c019e490>] 
(generic_handle_irq+0x18/0x28)
[    5.636352] [<c019e490>] (generic_handle_irq) from [<c019e5a4>] 
(__handle_domain_irq+0x64/0xdc)
[    5.654541] [<c019e5a4>] (__handle_domain_irq) from [<c010152c>] 
(gic_handle_irq+0x48/0x9c)
[    5.654541] [<c010152c>] (gic_handle_irq) from [<c0872af0>] 
(__irq_svc+0x70/0x98)
[    5.654541] Exception stack(0xee8b7d18 to 0xee8b7d60)
[    5.676483] 7d00:                                                       
00000001 ee8b5328
[    5.676483] 7d20: 00000000 ee8b4d80 60000153 eee28010 eee28010 60000153 
00000002 c1603ae4
[    5.693664] 7d40: c0d029cc 0000016b c0f19314 ee8b7d68 c019268c c0872788 
20000153 ffffffff
[    5.693664] [<c0872af0>] (__irq_svc) from [<c0872788>] 
(_raw_spin_unlock_irqrestore+0x34/0x44)
[    5.711303] [<c0872788>] (_raw_spin_unlock_irqrestore) from [<c069b260>] 
(musb_gadget_queue+0x128/0x4ac)
[    5.711303] [<c069b260>] (musb_gadget_queue) from [<c06a9ae4>] 
(usb_ep_queue+0x38/0x1d4)
[    5.729766] [<c06a9ae4>] (usb_ep_queue) from [<c06aba40>] 
(rx_submit+0xc8/0x19c)
[    5.737548] [<c06aba40>] (rx_submit) from [<c06abb90>] (rx_fill+0x7c/0xa0)
[    5.737548] [<c06abb90>] (rx_fill) from [<c06abbdc>] (eth_start+0x28/0x48)
[    5.751983] [<c06abbdc>] (eth_start) from [<c06abe7c>] (eth_open+0x6c/0x7c)
[    5.751983] [<c06abe7c>] (eth_open) from [<c0778c2c>] 
(__dev_open+0x9c/0x104)
[    5.766815] [<c0778c2c>] (__dev_open) from [<c0778ea0>] 
(__dev_change_flags+0x88/0x150)
[    5.775238] [<c0778ea0>] (__dev_change_flags) from [<c0778f88>] 
(dev_change_flags+0x18/0x48)
[    5.775238] [<c0778f88>] (dev_change_flags) from [<c0c515d4>] 
(ip_auto_config+0x194/0x1148)
[    5.792907] [<c0c515d4>] (ip_auto_config) from [<c0101870>] 
(do_one_initcall+0x3c/0x174)
[    5.792907] [<c0101870>] (do_one_initcall) from [<c0c00eb0>] 
(kernel_init_freeable+0x204/0x2e0)
[    5.792907] [<c0c00eb0>] (kernel_init_freeable) from [<c086b8e4>] 
(kernel_init+0x8/0x118)
[    5.810577] [<c086b8e4>] (kernel_init) from [<c0108430>] 
(ret_from_fork+0x14/0x24)
[    5.819183] ---[ end trace e721ca4e3e3c1d62 ]---

> ---
> 
> Changes since v1: Updated description
> 
> This is needed as a regression fix for the v4.8-rc cycle please.
> 
> ---
>  drivers/phy/phy-twl4030-usb.c | 4 ++--
>  drivers/usb/musb/musb_core.c  | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> 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
> @@ -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;
>  }
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> --- 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 */

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160918 05:13]:
> 
> FYI, while this patch allows me to boot my Panda board with NFS over usbnet, 
> it only works with cold boots. A warm reboot results in the following warning, 
> and no ethernet traffic going through. The USB device is detected by the host 
> though.

Yeah I noticed too that we still have issues. For example doing rmmod of
omap2430 with gadget configured and connected will produce a hardirq-safe
hardirq-unsafe lock order error. That also happens with reboot with gadget
configured and connected.

> I'm not sure whether this is a regression introduced by commit a83e17d0f73b 
> ("usb: musb: Improve PM runtime and phy handling for 2430 glue layer") or an 
> entirely different issue;
...
> [    5.711303] [<c0872788>] (_raw_spin_unlock_irqrestore) from [<c069b260>] 
> (musb_gadget_queue+0x128/0x4ac)
> [    5.711303] [<c069b260>] (musb_gadget_queue) from [<c06a9ae4>] 
> (usb_ep_queue+0x38/0x1d4)
> [    5.729766] [<c06a9ae4>] (usb_ep_queue) from [<c06aba40>] 
> (rx_submit+0xc8/0x19c)
> [    5.737548] [<c06aba40>] (rx_submit) from [<c06abb90>] (rx_fill+0x7c/0xa0)
> [    5.737548] [<c06abb90>] (rx_fill) from [<c06abbdc>] (eth_start+0x28/0x48)
> [    5.751983] [<c06abbdc>] (eth_start) from [<c06abe7c>] (eth_open+0x6c/0x7c)
> [    5.751983] [<c06abe7c>] (eth_open) from [<c0778c2c>] 
> (__dev_open+0x9c/0x104)

This could be something else though. Care to email me your .config, maybe
this is related to legacy g_ether being built in?

Anyways, please also give the following patch a try. The reason why we
currently have no chance of getting musb_platform_enable/disable balanced
is because we need to try to set the musb devctl session bit in various
places and possibly retry too. So musb_start should really be called
musb_try_start_session() or something.

So I think the only sane thing to do at this point is to revert the changes
trying to enable/disable USB PHY from omap2430_musb_enable/disable. The other
fixes are OK too as they get us a bit closer to making the platform glue calls
balanced.

Regards,

Tony

8< ----------------------
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -337,6 +337,7 @@ static int omap2430_musb_init(struct musb *musb)
 	}
 	musb->isr = omap2430_musb_interrupt;
 	phy_init(musb->phy);
+	phy_power_on(musb->phy);
 
 	l = musb_readl(musb->mregs, OTG_INTERFSEL);
 
@@ -373,9 +374,6 @@ static void omap2430_musb_enable(struct musb *musb)
 	struct musb_hdrc_platform_data *pdata = dev_get_platdata(dev);
 	struct omap_musb_board_data *data = pdata->board_data;
 
-	if (!WARN_ON(!musb->phy))
-		phy_power_on(musb->phy);
-
 	omap2430_set_power(musb, true, glue->cable_connected);
 
 	switch (glue->status) {
@@ -413,9 +411,6 @@ 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);

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Andreas Kemnade]

On Sun, 18 Sep 2016 08:19:02 -0700
Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:

> * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160918 05:13]:
> > 
> > FYI, while this patch allows me to boot my Panda board with NFS
> > over usbnet, it only works with cold boots. A warm reboot results
> > in the following warning, and no ethernet traffic going through.
> > The USB device is detected by the host though.
> 
> Yeah I noticed too that we still have issues. For example doing rmmod
> of omap2430 with gadget configured and connected will produce a
> hardirq-safe hardirq-unsafe lock order error. That also happens with
> reboot with gadget configured and connected.
> 
hmm, well, there is a musb_platform_disable() in musb_remove() which is
simply superfluous...
Some days ago we had a locking problem with musb_start() and moved
it out of the locked area. Maybe we could do also something similar
here.
 
> > I'm not sure whether this is a regression introduced by commit
> > a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for
> > 2430 glue layer") or an entirely different issue;
> ...
> > [    5.711303] [<c0872788>] (_raw_spin_unlock_irqrestore) from
> > [<c069b260>] (musb_gadget_queue+0x128/0x4ac)
> > [    5.711303] [<c069b260>] (musb_gadget_queue) from [<c06a9ae4>] 
> > (usb_ep_queue+0x38/0x1d4)
> > [    5.729766] [<c06a9ae4>] (usb_ep_queue) from [<c06aba40>] 
> > (rx_submit+0xc8/0x19c)
> > [    5.737548] [<c06aba40>] (rx_submit) from [<c06abb90>]
> > (rx_fill+0x7c/0xa0) [    5.737548] [<c06abb90>] (rx_fill) from
> > [<c06abbdc>] (eth_start+0x28/0x48) [    5.751983] [<c06abbdc>]
> > (eth_start) from [<c06abe7c>] (eth_open+0x6c/0x7c) [    5.751983]
> > [<c06abe7c>] (eth_open) from [<c0778c2c>] (__dev_open+0x9c/0x104)
> 
> This could be something else though. Care to email me your .config,
> maybe this is related to legacy g_ether being built in?
> 
> Anyways, please also give the following patch a try. The reason why we
> currently have no chance of getting musb_platform_enable/disable
> balanced is because we need to try to set the musb devctl session bit
> in various places and possibly retry too. So musb_start should really
> be called musb_try_start_session() or something.
> 
> So I think the only sane thing to do at this point is to revert the
> changes trying to enable/disable USB PHY from
> omap2430_musb_enable/disable. The other fixes are OK too as they get
> us a bit closer to making the platform glue calls balanced.
>
or to balance it there (in a better way as done by my first patch).

Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160918 23:00]:
> On Sun, 18 Sep 2016 08:19:02 -0700
> Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160918 05:13]:
> > > 
> > > FYI, while this patch allows me to boot my Panda board with NFS
> > > over usbnet, it only works with cold boots. A warm reboot results
> > > in the following warning, and no ethernet traffic going through.
> > > The USB device is detected by the host though.
> > 
> > Yeah I noticed too that we still have issues. For example doing rmmod
> > of omap2430 with gadget configured and connected will produce a
> > hardirq-safe hardirq-unsafe lock order error. That also happens with
> > reboot with gadget configured and connected.
> > 
> hmm, well, there is a musb_platform_disable() in musb_remove() which is
> simply superfluous...
> Some days ago we had a locking problem with musb_start() and moved
> it out of the locked area. Maybe we could do also something similar
> here.

Well I don't think we can do that safely at this point because we
have unpaired calls to musb_start() and musb_stop(). So trying to
make musb_platform_enable/disable() paired right now will just lead
into mystery breakage in various use cases.

> > So I think the only sane thing to do at this point is to revert the
> > changes trying to enable/disable USB PHY from
> > omap2430_musb_enable/disable. The other fixes are OK too as they get
> > us a bit closer to making the platform glue calls balanced.
> >
> or to balance it there (in a better way as done by my first patch).

We can't do that until we have musb_start() and musb_stop() paired :)

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

Hi Tony,

On Sunday 18 Sep 2016 08:19:02 Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160918 05:13]:
> > FYI, while this patch allows me to boot my Panda board with NFS over
> > usbnet, it only works with cold boots. A warm reboot results in the
> > following warning, and no ethernet traffic going through. The USB device
> > is detected by the host though.
> 
> Yeah I noticed too that we still have issues. For example doing rmmod of
> omap2430 with gadget configured and connected will produce a hardirq-safe
> hardirq-unsafe lock order error. That also happens with reboot with gadget
> configured and connected.
> 
> > I'm not sure whether this is a regression introduced by commit
> > a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for 2430
> > glue layer") or an entirely different issue;
> 
> ...
> 
> > [    5.711303] [<c0872788>] (_raw_spin_unlock_irqrestore) from
> > [<c069b260>]
> > (musb_gadget_queue+0x128/0x4ac)
> > [    5.711303] [<c069b260>] (musb_gadget_queue) from [<c06a9ae4>]
> > (usb_ep_queue+0x38/0x1d4)
> > [    5.729766] [<c06a9ae4>] (usb_ep_queue) from [<c06aba40>]
> > (rx_submit+0xc8/0x19c)
> > [    5.737548] [<c06aba40>] (rx_submit) from [<c06abb90>]
> > (rx_fill+0x7c/0xa0) [    5.737548] [<c06abb90>] (rx_fill) from
> > [<c06abbdc>] (eth_start+0x28/0x48) [    5.751983] [<c06abbdc>]
> > (eth_start) from [<c06abe7c>] (eth_open+0x6c/0x7c) [    5.751983]
> > [<c06abe7c>] (eth_open) from [<c0778c2c>]
> > (__dev_open+0x9c/0x104)
> 
> This could be something else though. Care to email me your .config, maybe
> this is related to legacy g_ether being built in?

Sure, please find it attached. The legacy g_ether is indeed built-in, that's 
what I use to boot over nfsroot.

> Anyways, please also give the following patch a try.

I've tested the patch and if fixes the original problem. Warm reboots are 
still broken though.

> The reason why we
> currently have no chance of getting musb_platform_enable/disable balanced
> is because we need to try to set the musb devctl session bit in various
> places and possibly retry too. So musb_start should really be called
> musb_try_start_session() or something.
> 
> So I think the only sane thing to do at this point is to revert the changes
> trying to enable/disable USB PHY from omap2430_musb_enable/disable. The
> other fixes are OK too as they get us a bit closer to making the platform
> glue calls balanced.
> 
> Regards,
> 
> Tony
> 
> 8< ----------------------
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -337,6 +337,7 @@ static int omap2430_musb_init(struct musb *musb)
>  	}
>  	musb->isr = omap2430_musb_interrupt;
>  	phy_init(musb->phy);
> +	phy_power_on(musb->phy);
> 
>  	l = musb_readl(musb->mregs, OTG_INTERFSEL);
> 
> @@ -373,9 +374,6 @@ static void omap2430_musb_enable(struct musb *musb)
>  	struct musb_hdrc_platform_data *pdata = dev_get_platdata(dev);
>  	struct omap_musb_board_data *data = pdata->board_data;
> 
> -	if (!WARN_ON(!musb->phy))
> -		phy_power_on(musb->phy);
> -
>  	omap2430_set_power(musb, true, glue->cable_connected);
> 
>  	switch (glue->status) {
> @@ -413,9 +411,6 @@ 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->status != MUSB_UNKNOWN)
>  		omap_control_usb_set_mode(glue->control_otghs,
>  			USB_MODE_DISCONNECT);
> @@ -429,6 +424,7 @@ static int omap2430_musb_exit(struct musb *musb)
>  	struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
> 
>  	omap2430_low_level_exit(musb);
> +	phy_power_off(musb->phy);
>  	phy_exit(musb->phy);
>  	musb->phy = NULL;
>  	cancel_work_sync(&glue->omap_musb_mailbox_work);

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 25879 bytes --]

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 13:35]:
> On Sunday 18 Sep 2016 08:19:02 Tony Lindgren wrote:
> > > [    5.711303] [<c0872788>] (_raw_spin_unlock_irqrestore) from
> > > [<c069b260>]
> > > (musb_gadget_queue+0x128/0x4ac)
> > > [    5.711303] [<c069b260>] (musb_gadget_queue) from [<c06a9ae4>]
> > > (usb_ep_queue+0x38/0x1d4)
> > > [    5.729766] [<c06a9ae4>] (usb_ep_queue) from [<c06aba40>]
> > > (rx_submit+0xc8/0x19c)
> > > [    5.737548] [<c06aba40>] (rx_submit) from [<c06abb90>]
> > > (rx_fill+0x7c/0xa0) [    5.737548] [<c06abb90>] (rx_fill) from
> > > [<c06abbdc>] (eth_start+0x28/0x48) [    5.751983] [<c06abbdc>]
> > > (eth_start) from [<c06abe7c>] (eth_open+0x6c/0x7c) [    5.751983]
> > > [<c06abe7c>] (eth_open) from [<c0778c2c>]
> > > (__dev_open+0x9c/0x104)
> > 
> > This could be something else though. Care to email me your .config, maybe
> > this is related to legacy g_ether being built in?
> 
> Sure, please find it attached. The legacy g_ether is indeed built-in, that's 
> what I use to boot over nfsroot.

OK, I think g_ether may have issues in general..

I mostly test with configfs based gadgets and shell script as then I can
test load/configure/connect/disconnect/unconfigure/unload easily :)

> > Anyways, please also give the following patch a try.
> 
> I've tested the patch and if fixes the original problem. Warm reboots are 
> still broken though.

No luck here with your .config a try with my pandaboard es against v4.8-rc7
plus the patch I posted on Sunday. It reboots with no errors for me with
NFSroot. Do you do something other than just reboot?

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Andreas Kemnade]

On Mon, 19 Sep 2016 09:02:50 -0700
Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:

> * Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160918 23:00]:
> > On Sun, 18 Sep 2016 08:19:02 -0700
> > Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > 
> > > * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160918
> > > 05:13]:
> > > > 
> > > > FYI, while this patch allows me to boot my Panda board with NFS
> > > > over usbnet, it only works with cold boots. A warm reboot
> > > > results in the following warning, and no ethernet traffic going
> > > > through. The USB device is detected by the host though.
> > > 
> > > Yeah I noticed too that we still have issues. For example doing
> > > rmmod of omap2430 with gadget configured and connected will
> > > produce a hardirq-safe hardirq-unsafe lock order error. That also
> > > happens with reboot with gadget configured and connected.
> > > 
> > hmm, well, there is a musb_platform_disable() in musb_remove()
> > which is simply superfluous...
> > Some days ago we had a locking problem with musb_start() and moved
> > it out of the locked area. Maybe we could do also something similar
> > here.
> 
> Well I don't think we can do that safely at this point because we
> have unpaired calls to musb_start() and musb_stop(). So trying to
> make musb_platform_enable/disable() paired right now will just lead
> into mystery breakage in various use cases.
> 
I am primarily talking about 
doing things like the patch
      usb: musb: Fix locking errors for host only mode
	2c5575401e34de3d2f

did for musb_start(), for musb_stop() also. 


Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Laurent Pinchart]

Hi Tony,

On Monday 19 Sep 2016 15:41:50 Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 13:35]:
> > On Sunday 18 Sep 2016 08:19:02 Tony Lindgren wrote:
> >>> [    5.711303] [<c0872788>] (_raw_spin_unlock_irqrestore) from
> >>> [<c069b260>]
> >>> (musb_gadget_queue+0x128/0x4ac)
> >>> [    5.711303] [<c069b260>] (musb_gadget_queue) from [<c06a9ae4>]
> >>> (usb_ep_queue+0x38/0x1d4)
> >>> [    5.729766] [<c06a9ae4>] (usb_ep_queue) from [<c06aba40>]
> >>> (rx_submit+0xc8/0x19c)
> >>> [    5.737548] [<c06aba40>] (rx_submit) from [<c06abb90>]
> >>> (rx_fill+0x7c/0xa0) [    5.737548] [<c06abb90>] (rx_fill) from
> >>> [<c06abbdc>] (eth_start+0x28/0x48) [    5.751983] [<c06abbdc>]
> >>> (eth_start) from [<c06abe7c>] (eth_open+0x6c/0x7c) [    5.751983]
> >>> [<c06abe7c>] (eth_open) from [<c0778c2c>]
> >>> (__dev_open+0x9c/0x104)
> >> 
> >> This could be something else though. Care to email me your .config,
> >> maybe this is related to legacy g_ether being built in?
> > 
> > Sure, please find it attached. The legacy g_ether is indeed built-in,
> > that's what I use to boot over nfsroot.
> 
> OK, I think g_ether may have issues in general..
> 
> I mostly test with configfs based gadgets and shell script as then I can
> test load/configure/connect/disconnect/unconfigure/unload easily :)

g_ether is very convenient when using nfsroot, as it allows booting the system 
without an initramfs.

> >> Anyways, please also give the following patch a try.
> > 
> > I've tested the patch and if fixes the original problem. Warm reboots are
> > still broken though.
> 
> No luck here with your .config a try with my pandaboard es against v4.8-rc7
> plus the patch I posted on Sunday. It reboots with no errors for me with
> NFSroot. Do you do something other than just reboot?

No, I perform the following steps:

- Connect the panda board to the USB through USB (which powers the board on)
- Let the board boot over NFS
- Log in as root, run 'reboot'

The second boot produces the warning.

[    5.189025] ------------[ cut here ]------------
[    5.193450] WARNING: CPU: 0 PID: 1 at 
/home/laurent/src/kernel/omap4/linux-2.6/drivers/bus/omap_l3_noc.c:147 
l3_interrupt_handler+0x220/0x348
[    5.198059] 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Read): 
Data Access in User mode during Functional access
[    5.218933] Modules linked in:
[    5.218933] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc2-00816-
g0caf606bb84a #20
[    5.222167] Hardware name: Generic OMAP4 (Flattened Device Tree)
[    5.233612] [<c01101b0>] (unwind_backtrace) from [<c010c10c>] 
(show_stack+0x10/0x14)
[    5.233612] [<c010c10c>] (show_stack) from [<c048ca40>] 
(dump_stack+0xa8/0xe0)
[    5.233612] [<c048ca40>] (dump_stack) from [<c0137910>] (__warn+0xd8/0x104)
[    5.253662] [<c0137910>] (__warn) from [<c01379e4>] 
(warn_slowpath_fmt+0x38/0x48)
[    5.253662] [<c01379e4>] (warn_slowpath_fmt) from [<c04befd0>] 
(l3_interrupt_handler+0x220/0x348)
[    5.277191] [<c04befd0>] (l3_interrupt_handler) from [<c019ec7c>] 
(__handle_irq_event_percpu+0x98/0x3ec)
[    5.277191] [<c019ec7c>] (__handle_irq_event_percpu) from [<c019efec>] 
(handle_irq_event_percpu+0x1c/0x58)
[    5.293426] [<c019efec>] (handle_irq_event_percpu) from [<c019f060>] 
(handle_irq_event+0x38/0x5c)
[    5.306610] [<c019f060>] (handle_irq_event) from [<c01a24e0>] 
(handle_fasteoi_irq+0xcc/0x1a4)
[    5.306610] [<c01a24e0>] (handle_fasteoi_irq) from [<c019e490>] 
(generic_handle_irq+0x18/0x28)
[    5.315582] [<c019e490>] (generic_handle_irq) from [<c019e5a4>] 
(__handle_domain_irq+0x64/0xdc)
[    5.315582] [<c019e5a4>] (__handle_domain_irq) from [<c010152c>] 
(gic_handle_irq+0x48/0x9c)
[    5.315582] [<c010152c>] (gic_handle_irq) from [<c086f930>] 
(__irq_svc+0x70/0x98)
[    5.350402] Exception stack(0xee8b7d18 to 0xee8b7d60)
[    5.350402] 7d00:                                                       
00000001 ee8b5328
[    5.364288] 7d20: 00000000 ee8b4d80 60000153 eee54010 eee54010 60000153 
00000002 c1603ae4
[    5.364288] 7d40: c0d029cc 0000016b c0f19314 ee8b7d68 c019268c c086f5c0 
20000153 ffffffff
[    5.381469] [<c086f930>] (__irq_svc) from [<c086f5c0>] 
(_raw_spin_unlock_irqrestore+0x34/0x44)
[    5.381469] [<c086f5c0>] (_raw_spin_unlock_irqrestore) from [<c06980d8>] 
(musb_gadget_queue+0x128/0x4ac)
[    5.390533] [<c06980d8>] (musb_gadget_queue) from [<c06a6920>] 
(usb_ep_queue+0x38/0x1d4)
[    5.408996] [<c06a6920>] (usb_ep_queue) from [<c06a887c>] 
(rx_submit+0xc8/0x19c)
[    5.408996] [<c06a887c>] (rx_submit) from [<c06a89cc>] (rx_fill+0x7c/0xa0)
[    5.408996] [<c06a89cc>] (rx_fill) from [<c06a8a18>] (eth_start+0x28/0x48)
[    5.431213] [<c06a8a18>] (eth_start) from [<c06a8cb8>] (eth_open+0x6c/0x7c)
[    5.431213] [<c06a8cb8>] (eth_open) from [<c0775a68>] 
(__dev_open+0x9c/0x104)
[    5.446044] [<c0775a68>] (__dev_open) from [<c0775cdc>] 
(__dev_change_flags+0x88/0x150)
[    5.446044] [<c0775cdc>] (__dev_change_flags) from [<c0775dc4>] 
(dev_change_flags+0x18/0x48)
[    5.454467] [<c0775dc4>] (dev_change_flags) from [<c0c515c4>] 
(ip_auto_config+0x194/0x1148)
[    5.454467] [<c0c515c4>] (ip_auto_config) from [<c0101870>] 
(do_one_initcall+0x3c/0x174)
[    5.480621] [<c0101870>] (do_one_initcall) from [<c0c00eb0>] 
(kernel_init_freeable+0x204/0x2e0)
[    5.480621] [<c0c00eb0>] (kernel_init_freeable) from [<c0868720>] 
(kernel_init+0x8/0x118)
[    5.480621] [<c0868720>] (kernel_init) from [<c0108430>] 
(ret_from_fork+0x14/0x24)
[    5.506347] ---[ end trace 9a597c69856245a5 ]---

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160919 22:05]:
> On Mon, 19 Sep 2016 09:02:50 -0700
> Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > * Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160918 23:00]:
> > > On Sun, 18 Sep 2016 08:19:02 -0700
> > > Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > > 
> > > > * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160918
> > > > 05:13]:
> > > > > 
> > > > > FYI, while this patch allows me to boot my Panda board with NFS
> > > > > over usbnet, it only works with cold boots. A warm reboot
> > > > > results in the following warning, and no ethernet traffic going
> > > > > through. The USB device is detected by the host though.
> > > > 
> > > > Yeah I noticed too that we still have issues. For example doing
> > > > rmmod of omap2430 with gadget configured and connected will
> > > > produce a hardirq-safe hardirq-unsafe lock order error. That also
> > > > happens with reboot with gadget configured and connected.
> > > > 
> > > hmm, well, there is a musb_platform_disable() in musb_remove()
> > > which is simply superfluous...
> > > Some days ago we had a locking problem with musb_start() and moved
> > > it out of the locked area. Maybe we could do also something similar
> > > here.
> > 
> > Well I don't think we can do that safely at this point because we
> > have unpaired calls to musb_start() and musb_stop(). So trying to
> > make musb_platform_enable/disable() paired right now will just lead
> > into mystery breakage in various use cases.
> > 
> I am primarily talking about 
> doing things like the patch
>       usb: musb: Fix locking errors for host only mode
> 	2c5575401e34de3d2f
> 
> did for musb_start(), for musb_stop() also. 

Yeah I know, but that is really just trying to plug holes. The real
long term fix seems to to be something like:

1. Add musb_try_start_session/stop_session() that can be called
   multiple times unpaired and only tinker with the devctl register

2. Make musb_start/stop() paired for the hardware specific init

3. This allows making musb_platform_enable/disable() paired too

But for now, we still have issues in v4.8-rc cycle, so let's get
that fixed first.

Regards,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 23:36]:
> Hi Tony,
> 
> On Monday 19 Sep 2016 15:41:50 Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 13:35]:
> > > On Sunday 18 Sep 2016 08:19:02 Tony Lindgren wrote:
> > >>> [    5.711303] [<c0872788>] (_raw_spin_unlock_irqrestore) from
> > >>> [<c069b260>]
> > >>> (musb_gadget_queue+0x128/0x4ac)
> > >>> [    5.711303] [<c069b260>] (musb_gadget_queue) from [<c06a9ae4>]
> > >>> (usb_ep_queue+0x38/0x1d4)
> > >>> [    5.729766] [<c06a9ae4>] (usb_ep_queue) from [<c06aba40>]
> > >>> (rx_submit+0xc8/0x19c)
> > >>> [    5.737548] [<c06aba40>] (rx_submit) from [<c06abb90>]
> > >>> (rx_fill+0x7c/0xa0) [    5.737548] [<c06abb90>] (rx_fill) from
> > >>> [<c06abbdc>] (eth_start+0x28/0x48) [    5.751983] [<c06abbdc>]
> > >>> (eth_start) from [<c06abe7c>] (eth_open+0x6c/0x7c) [    5.751983]
> > >>> [<c06abe7c>] (eth_open) from [<c0778c2c>]
> > >>> (__dev_open+0x9c/0x104)
> > >> 
> > >> This could be something else though. Care to email me your .config,
> > >> maybe this is related to legacy g_ether being built in?
> > > 
> > > Sure, please find it attached. The legacy g_ether is indeed built-in,
> > > that's what I use to boot over nfsroot.
> > 
> > OK, I think g_ether may have issues in general..
> > 
> > I mostly test with configfs based gadgets and shell script as then I can
> > test load/configure/connect/disconnect/unconfigure/unload easily :)
> 
> g_ether is very convenient when using nfsroot, as it allows booting the system 
> without an initramfs.

Yeah no doubt about that.

> > >> Anyways, please also give the following patch a try.
> > > 
> > > I've tested the patch and if fixes the original problem. Warm reboots are
> > > still broken though.
> > 
> > No luck here with your .config a try with my pandaboard es against v4.8-rc7
> > plus the patch I posted on Sunday. It reboots with no errors for me with
> > NFSroot. Do you do something other than just reboot?
> 
> No, I perform the following steps:
> 
> - Connect the panda board to the USB through USB (which powers the board on)
> - Let the board boot over NFS
> - Log in as root, run 'reboot'
> 
> The second boot produces the warning.

Oh I was looking at the errors while shutting down things.. OK yeah I get
that too along with a bunch of DSS related warnings with your .config.
Probably I did not notice it earlier because of the DSS warnings. Will
take a look.

Thanks,

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160920 10:11]:
> * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 23:36]:
> > No, I perform the following steps:
> > 
> > - Connect the panda board to the USB through USB (which powers the board on)
> > - Let the board boot over NFS
> > - Log in as root, run 'reboot'
> > 
> > The second boot produces the warning.
> 
> Oh I was looking at the errors while shutting down things.. OK yeah I get
> that too along with a bunch of DSS related warnings with your .config.
> Probably I did not notice it earlier because of the DSS warnings. Will
> take a look.

The patch below fixes the issue for me, care to give it a try?

If that works for you I'll repost with a proper patch description.

Regards,

Tony

8< ------------------------
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1255,6 +1255,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 	map_dma_buffer(request, musb, musb_ep);
 
+	pm_runtime_get_sync(musb->controller);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */
@@ -1275,6 +1276,9 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 unlock:
 	spin_unlock_irqrestore(&musb->lock, lockflags);
+	pm_runtime_mark_last_busy(musb->controller);
+	pm_runtime_put_autosuspend(musb->controller);
+
 	return status;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160920 11:37]:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160920 10:11]:
> > * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 23:36]:
> > > No, I perform the following steps:
> > > 
> > > - Connect the panda board to the USB through USB (which powers the board on)
> > > - Let the board boot over NFS
> > > - Log in as root, run 'reboot'
> > > 
> > > The second boot produces the warning.
> > 
> > Oh I was looking at the errors while shutting down things.. OK yeah I get
> > that too along with a bunch of DSS related warnings with your .config.
> > Probably I did not notice it earlier because of the DSS warnings. Will
> > take a look.
> 
> The patch below fixes the issue for me, care to give it a try?
> 
> If that works for you I'll repost with a proper patch description.

Laurent, any news on when you may be able to test this one?

Regards,

Tony

> 8< ------------------------
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -1255,6 +1255,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
>  
>  	map_dma_buffer(request, musb, musb_ep);
>  
> +	pm_runtime_get_sync(musb->controller);
>  	spin_lock_irqsave(&musb->lock, lockflags);
>  
>  	/* don't queue if the ep is down */
> @@ -1275,6 +1276,9 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
>  
>  unlock:
>  	spin_unlock_irqrestore(&musb->lock, lockflags);
> +	pm_runtime_mark_last_busy(musb->controller);
> +	pm_runtime_put_autosuspend(musb->controller);
> +
>  	return status;
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Laurent Pinchart]

Hi Tony,

On Wednesday 28 Sep 2016 11:42:35 Tony Lindgren wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160920 11:37]:
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160920 10:11]:
> >> * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 23:36]:
> >>> No, I perform the following steps:
> >>> 
> >>> - Connect the panda board to the USB through USB (which powers the
> >>> board on) - Let the board boot over NFS
> >>> - Log in as root, run 'reboot'
> >>> 
> >>> The second boot produces the warning.
> >> 
> >> Oh I was looking at the errors while shutting down things.. OK yeah I
> >> get that too along with a bunch of DSS related warnings with your
> >> .config. Probably I did not notice it earlier because of the DSS
> >> warnings. Will take a look.
> > 
> > The patch below fixes the issue for me, care to give it a try?
> > 
> > If that works for you I'll repost with a proper patch description.
> 
> Laurent, any news on when you may be able to test this one?

I just did and your patch fixes the problem for me, thanks.

Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> > 8< ------------------------
> > diff --git a/drivers/usb/musb/musb_gadget.c
> > b/drivers/usb/musb/musb_gadget.c --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -1255,6 +1255,7 @@ static int musb_gadget_queue(struct usb_ep *ep,
> > struct usb_request *req,
> >  	map_dma_buffer(request, musb, musb_ep);
> > 
> > +	pm_runtime_get_sync(musb->controller);
> >  	spin_lock_irqsave(&musb->lock, lockflags);
> >  	
> >  	/* don't queue if the ep is down */
> > @@ -1275,6 +1276,9 @@ static int musb_gadget_queue(struct usb_ep *ep,
> > struct usb_request *req,> 
> >  unlock:
> >  	spin_unlock_irqrestore(&musb->lock, lockflags);
> > 
> > +	pm_runtime_mark_last_busy(musb->controller);
> > +	pm_runtime_put_autosuspend(musb->controller);
> > +
> >  	return status;
> >  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160929 02:38]:
> Hi Tony,
> 
> On Wednesday 28 Sep 2016 11:42:35 Tony Lindgren wrote:
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160920 11:37]:
> > > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160920 10:11]:
> > >> * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160919 23:36]:
> > >>> No, I perform the following steps:
> > >>> 
> > >>> - Connect the panda board to the USB through USB (which powers the
> > >>> board on) - Let the board boot over NFS
> > >>> - Log in as root, run 'reboot'
> > >>> 
> > >>> The second boot produces the warning.
> > >> 
> > >> Oh I was looking at the errors while shutting down things.. OK yeah I
> > >> get that too along with a bunch of DSS related warnings with your
> > >> .config. Probably I did not notice it earlier because of the DSS
> > >> warnings. Will take a look.
> > > 
> > > The patch below fixes the issue for me, care to give it a try?
> > > 
> > > If that works for you I'll repost with a proper patch description.
> > 
> > Laurent, any news on when you may be able to test this one?
> 
> I just did and your patch fixes the problem for me, thanks.
> 
> Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

OK thanks for testing, will send out the two pending fixes today.

Regards,

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