[PATCHv3] usb: musb: Fix unbalanced platform_disable

[PATCHv3] 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. To do that in a safe way,
we need to call the glue specific *_musb_disable() directly in the
glue layer *_musb_init() as noted by Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>.

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").

Further, most of the glue layers already do a reset of the musb
controller in the *_musb_init(), so later on we may want to
consider removing the calls to *__musb_disable() where a reset is
already done.

Note that 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: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
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>
---

I've kept the 2430 glue layer related acks as those parts did
not change. For the glue layers doing things in *_musb_disable(),
I've tested da8xx, am35x and musb_dsps glue layers.

Changes since v2:

- Added glue layer calls to *_musb_disable() from *_musb_init()
  where needed as suggested by Bin

---
 drivers/phy/phy-twl4030-usb.c | 4 ++--
 drivers/usb/musb/am35x.c      | 1 +
 drivers/usb/musb/da8xx.c      | 1 +
 drivers/usb/musb/davinci.c    | 1 +
 drivers/usb/musb/musb_core.c  | 1 -
 drivers/usb/musb/musb_dsps.c  | 1 +
 drivers/usb/musb/sunxi.c      | 3 +++
 drivers/usb/musb/tusb6010.c   | 1 +
 8 files changed, 10 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/am35x.c b/drivers/usb/musb/am35x.c
--- a/drivers/usb/musb/am35x.c
+++ b/drivers/usb/musb/am35x.c
@@ -381,6 +381,7 @@ static int am35x_musb_init(struct musb *musb)
 
 	msleep(5);
 
+	am35x_musb_disable(musb);
 	musb->isr = am35x_musb_interrupt;
 
 	/* clear level interrupt */
diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -440,6 +440,7 @@ static int da8xx_musb_init(struct musb *musb)
 		 rev, __raw_readl(CFGCHIP2),
 		 musb_readb(reg_base, DA8XX_USB_CTRL_REG));
 
+	da8xx_musb_disable(musb);
 	musb->isr = da8xx_musb_interrupt;
 	return 0;
 fail:
diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
--- a/drivers/usb/musb/davinci.c
+++ b/drivers/usb/musb/davinci.c
@@ -432,6 +432,7 @@ static int davinci_musb_init(struct musb *musb)
 		revision, __raw_readl(USB_PHY_CTRL),
 		musb_readb(tibase, DAVINCI_USB_CTRL_REG));
 
+	davinci_musb_disable(musb);
 	musb->isr = davinci_musb_interrupt;
 	return 0;
 
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 74fc306..f7dc3df 100644
--- 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 */
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -441,6 +441,7 @@ static int dsps_musb_init(struct musb *musb)
 	/* Reset the musb */
 	musb_writel(reg_base, wrp->control, (1 << wrp->reset));
 
+	dsps_musb_disable(musb);
 	musb->isr = dsps_interrupt;
 
 	/* reset the otgdisable bit, needed for host mode to work */
diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -93,6 +93,8 @@ struct sunxi_glue {
 	struct notifier_block	host_nb;
 };
 
+static void sunxi_musb_disable(struct musb *musb);
+
 /* phy_power_on / off may sleep, so we use a workqueue  */
 static void sunxi_musb_work(struct work_struct *work)
 {
@@ -265,6 +267,7 @@ static int sunxi_musb_init(struct musb *musb)
 	if (ret)
 		goto error_unregister_notifier;
 
+	sunxi_musb_disable(musb);
 	musb->isr = sunxi_musb_interrupt;
 
 	/* Stop the musb-core from doing runtime pm (not supported on sunxi) */
diff --git a/drivers/usb/musb/tusb6010.c b/drivers/usb/musb/tusb6010.c
--- a/drivers/usb/musb/tusb6010.c
+++ b/drivers/usb/musb/tusb6010.c
@@ -1149,6 +1149,7 @@ static int tusb_musb_init(struct musb *musb)
 				ret);
 		goto done;
 	}
+	tusb_musb_disable(musb);
 	musb->isr = tusb_musb_interrupt;
 
 	musb->xceiv->set_power = tusb_draw_power;
-- 
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: [PATCHv3] usb: musb: Fix unbalanced platform_disable

[Hans de Goede]

Hi,

On 14-09-16 20:10, 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.

Yeah, the sunxi glue actually has:

static void sunxi_musb_enable(struct musb *musb)
{
         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

         glue->musb = musb;

         /* musb_core does not call us in a balanced manner */
         if (test_and_set_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags))
                 return;

         schedule_work(&glue->work);
}

static void sunxi_musb_disable(struct musb *musb)
{
         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

         clear_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags);
}

Note that since the glue struct is kzalloc-ed, this makes calling
sunxi_musb_disable() from sunxi_musb_init() a nop, so if this
needs a respin please drop the sunxi changes, they are not
necessary.

If there are no other comments I can live with this...

Regards,

Hans



>
> Instead, let's first fix the regression in a minimal way by removing
> the initial call to musb_platform_disable. To do that in a safe way,
> we need to call the glue specific *_musb_disable() directly in the
> glue layer *_musb_init() as noted by Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>.
>
> 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").
>
> Further, most of the glue layers already do a reset of the musb
> controller in the *_musb_init(), so later on we may want to
> consider removing the calls to *__musb_disable() where a reset is
> already done.
>
> Note that 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: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 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>
> ---
>
> I've kept the 2430 glue layer related acks as those parts did
> not change. For the glue layers doing things in *_musb_disable(),
> I've tested da8xx, am35x and musb_dsps glue layers.
>
> Changes since v2:
>
> - Added glue layer calls to *_musb_disable() from *_musb_init()
>   where needed as suggested by Bin
>
> ---
>  drivers/phy/phy-twl4030-usb.c | 4 ++--
>  drivers/usb/musb/am35x.c      | 1 +
>  drivers/usb/musb/da8xx.c      | 1 +
>  drivers/usb/musb/davinci.c    | 1 +
>  drivers/usb/musb/musb_core.c  | 1 -
>  drivers/usb/musb/musb_dsps.c  | 1 +
>  drivers/usb/musb/sunxi.c      | 3 +++
>  drivers/usb/musb/tusb6010.c   | 1 +
>  8 files changed, 10 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/am35x.c b/drivers/usb/musb/am35x.c
> --- a/drivers/usb/musb/am35x.c
> +++ b/drivers/usb/musb/am35x.c
> @@ -381,6 +381,7 @@ static int am35x_musb_init(struct musb *musb)
>
>  	msleep(5);
>
> +	am35x_musb_disable(musb);
>  	musb->isr = am35x_musb_interrupt;
>
>  	/* clear level interrupt */
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -440,6 +440,7 @@ static int da8xx_musb_init(struct musb *musb)
>  		 rev, __raw_readl(CFGCHIP2),
>  		 musb_readb(reg_base, DA8XX_USB_CTRL_REG));
>
> +	da8xx_musb_disable(musb);
>  	musb->isr = da8xx_musb_interrupt;
>  	return 0;
>  fail:
> diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
> --- a/drivers/usb/musb/davinci.c
> +++ b/drivers/usb/musb/davinci.c
> @@ -432,6 +432,7 @@ static int davinci_musb_init(struct musb *musb)
>  		revision, __raw_readl(USB_PHY_CTRL),
>  		musb_readb(tibase, DAVINCI_USB_CTRL_REG));
>
> +	davinci_musb_disable(musb);
>  	musb->isr = davinci_musb_interrupt;
>  	return 0;
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 74fc306..f7dc3df 100644
> --- 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 */
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -441,6 +441,7 @@ static int dsps_musb_init(struct musb *musb)
>  	/* Reset the musb */
>  	musb_writel(reg_base, wrp->control, (1 << wrp->reset));
>
> +	dsps_musb_disable(musb);
>  	musb->isr = dsps_interrupt;
>
>  	/* reset the otgdisable bit, needed for host mode to work */
> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> --- a/drivers/usb/musb/sunxi.c
> +++ b/drivers/usb/musb/sunxi.c
> @@ -93,6 +93,8 @@ struct sunxi_glue {
>  	struct notifier_block	host_nb;
>  };
>
> +static void sunxi_musb_disable(struct musb *musb);
> +
>  /* phy_power_on / off may sleep, so we use a workqueue  */
>  static void sunxi_musb_work(struct work_struct *work)
>  {
> @@ -265,6 +267,7 @@ static int sunxi_musb_init(struct musb *musb)
>  	if (ret)
>  		goto error_unregister_notifier;
>
> +	sunxi_musb_disable(musb);
>  	musb->isr = sunxi_musb_interrupt;
>
>  	/* Stop the musb-core from doing runtime pm (not supported on sunxi) */
> diff --git a/drivers/usb/musb/tusb6010.c b/drivers/usb/musb/tusb6010.c
> --- a/drivers/usb/musb/tusb6010.c
> +++ b/drivers/usb/musb/tusb6010.c
> @@ -1149,6 +1149,7 @@ static int tusb_musb_init(struct musb *musb)
>  				ret);
>  		goto done;
>  	}
> +	tusb_musb_disable(musb);
>  	musb->isr = tusb_musb_interrupt;
>
>  	musb->xceiv->set_power = tusb_draw_power;
>
--
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: [PATCHv3] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [160915 07:00]:
> Hi,
> 
> On 14-09-16 20:10, 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.
> 
> Yeah, the sunxi glue actually has:
> 
> static void sunxi_musb_enable(struct musb *musb)
> {
>         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
> 
>         glue->musb = musb;
> 
>         /* musb_core does not call us in a balanced manner */
>         if (test_and_set_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags))
>                 return;

Heh we're trying to get to the "balanced manner" point hopefully soon :)

>         schedule_work(&glue->work);
> }
> 
> static void sunxi_musb_disable(struct musb *musb)
> {
>         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
> 
>         clear_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags);
> }
> 
> Note that since the glue struct is kzalloc-ed, this makes calling
> sunxi_musb_disable() from sunxi_musb_init() a nop, so if this
> needs a respin please drop the sunxi changes, they are not
> necessary.

Hmm we have sunxi_musb_probe() do platform_device_register_full(),
then musb_probe() happens, and calls musb_init_controller().

In musb_init_controller() we do musb_platform_init() that finally
calls sunxi_musb_init().

So the sunxi glue is kzalloc'ed and initialized already in
sunxi_musb_probe(), and musb parts are already initialized in
musb_init_controller().

I don't quite follow what you mean how it's a nop.. Care to
clarify that a bit? Maybe you're thinking we're calling it from
sunxi_musb_probe() instead?

Anyways, calling sunxi_musb_disable() seems unnecessary from
sunxi_musb_init() because it does reset_control_deassert().
But that can be cleaned up later on if no other reasons for
changes for v4.8-rc cycle.

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: [PATCHv3] usb: musb: Fix unbalanced platform_disable

[Hans de Goede]

Hi,

On 15-09-16 16:43, Tony Lindgren wrote:
> * Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [160915 07:00]:
>> Hi,
>>
>> On 14-09-16 20:10, 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.
>>
>> Yeah, the sunxi glue actually has:
>>
>> static void sunxi_musb_enable(struct musb *musb)
>> {
>>         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
>>
>>         glue->musb = musb;
>>
>>         /* musb_core does not call us in a balanced manner */
>>         if (test_and_set_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags))
>>                 return;
>
> Heh we're trying to get to the "balanced manner" point hopefully soon :)
>
>>         schedule_work(&glue->work);
>> }
>>
>> static void sunxi_musb_disable(struct musb *musb)
>> {
>>         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
>>
>>         clear_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags);
>> }
>>
>> Note that since the glue struct is kzalloc-ed, this makes calling
>> sunxi_musb_disable() from sunxi_musb_init() a nop, so if this
>> needs a respin please drop the sunxi changes, they are not
>> necessary.
>
> Hmm we have sunxi_musb_probe() do platform_device_register_full(),
> then musb_probe() happens, and calls musb_init_controller().
>
> In musb_init_controller() we do musb_platform_init() that finally
> calls sunxi_musb_init().
>
> So the sunxi glue is kzalloc'ed and initialized already in
> sunxi_musb_probe(), and musb parts are already initialized in
> musb_init_controller().

Not really, musb_init_controller does little of interest before
calling musb_platform_init(), which is a function pointer
to sunxi_musb_init().

> I don't quite follow what you mean how it's a nop.. Care to
> clarify that a bit? Maybe you're thinking we're calling it from
> sunxi_musb_probe() instead?

Nope, I no the call chain, but nothing interesting happens between
sunxi_musb_probe() and sunxi_musb_init().

> Anyways, calling sunxi_musb_disable() seems unnecessary from
> sunxi_musb_init() because it does reset_control_deassert().
> But that can be cleaned up later on if no other reasons for
> changes for v4.8-rc cycle.

Ah is this patch for 4.8-rc, yeah then definitely keep it as
is, as I tried to explain in my previous mail, my remark
was only made in case you need to respin the patch for other
reasons, if not then it is fine as is.

Regards,

Hans
--
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: [PATCHv3] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [160915 09:59]:
> Hi,
> 
> On 15-09-16 16:43, Tony Lindgren wrote:
> > * Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [160915 07:00]:
> > > Hi,
> > > 
> > > On 14-09-16 20:10, 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.
> > > 
> > > Yeah, the sunxi glue actually has:
> > > 
> > > static void sunxi_musb_enable(struct musb *musb)
> > > {
> > >         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
> > > 
> > >         glue->musb = musb;
> > > 
> > >         /* musb_core does not call us in a balanced manner */
> > >         if (test_and_set_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags))
> > >                 return;
> > 
> > Heh we're trying to get to the "balanced manner" point hopefully soon :)
> > 
> > >         schedule_work(&glue->work);
> > > }
> > > 
> > > static void sunxi_musb_disable(struct musb *musb)
> > > {
> > >         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
> > > 
> > >         clear_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags);
> > > }
> > > 
> > > Note that since the glue struct is kzalloc-ed, this makes calling
> > > sunxi_musb_disable() from sunxi_musb_init() a nop, so if this
> > > needs a respin please drop the sunxi changes, they are not
> > > necessary.
> > 
> > Hmm we have sunxi_musb_probe() do platform_device_register_full(),
> > then musb_probe() happens, and calls musb_init_controller().
> > 
> > In musb_init_controller() we do musb_platform_init() that finally
> > calls sunxi_musb_init().
> > 
> > So the sunxi glue is kzalloc'ed and initialized already in
> > sunxi_musb_probe(), and musb parts are already initialized in
> > musb_init_controller().
> 
> Not really, musb_init_controller does little of interest before
> calling musb_platform_init(), which is a function pointer
> to sunxi_musb_init().
> 
> > I don't quite follow what you mean how it's a nop.. Care to
> > clarify that a bit? Maybe you're thinking we're calling it from
> > sunxi_musb_probe() instead?
> 
> Nope, I no the call chain, but nothing interesting happens between
> sunxi_musb_probe() and sunxi_musb_init().

OK thanks for confirming.

> > Anyways, calling sunxi_musb_disable() seems unnecessary from
> > sunxi_musb_init() because it does reset_control_deassert().
> > But that can be cleaned up later on if no other reasons for
> > changes for v4.8-rc cycle.
> 
> Ah is this patch for 4.8-rc, yeah then definitely keep it as
> is, as I tried to explain in my previous mail, my remark
> was only made in case you need to respin the patch for other
> reasons, if not then it is fine as is.

OK 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: [PATCHv3] usb: musb: Fix unbalanced platform_disable

[Bin Liu]

Hi,

On Wed, Sep 14, 2016 at 11:10:19AM -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. To do that in a safe way,
> we need to call the glue specific *_musb_disable() directly in the
> glue layer *_musb_init() as noted by Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>.
> 
> 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").
> 
> Further, most of the glue layers already do a reset of the musb
> controller in the *_musb_init(), so later on we may want to
> consider removing the calls to *__musb_disable() where a reset is
> already done.
> 
> Note that 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: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 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>
> ---

If Kishon wants to take it, here is my

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

Regards
-Bin.

> 
> I've kept the 2430 glue layer related acks as those parts did
> not change. For the glue layers doing things in *_musb_disable(),
> I've tested da8xx, am35x and musb_dsps glue layers.
> 
> Changes since v2:
> 
> - Added glue layer calls to *_musb_disable() from *_musb_init()
>   where needed as suggested by Bin
> 
> ---
>  drivers/phy/phy-twl4030-usb.c | 4 ++--
>  drivers/usb/musb/am35x.c      | 1 +
>  drivers/usb/musb/da8xx.c      | 1 +
>  drivers/usb/musb/davinci.c    | 1 +
>  drivers/usb/musb/musb_core.c  | 1 -
>  drivers/usb/musb/musb_dsps.c  | 1 +
>  drivers/usb/musb/sunxi.c      | 3 +++
>  drivers/usb/musb/tusb6010.c   | 1 +
>  8 files changed, 10 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/am35x.c b/drivers/usb/musb/am35x.c
> --- a/drivers/usb/musb/am35x.c
> +++ b/drivers/usb/musb/am35x.c
> @@ -381,6 +381,7 @@ static int am35x_musb_init(struct musb *musb)
>  
>  	msleep(5);
>  
> +	am35x_musb_disable(musb);
>  	musb->isr = am35x_musb_interrupt;
>  
>  	/* clear level interrupt */
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -440,6 +440,7 @@ static int da8xx_musb_init(struct musb *musb)
>  		 rev, __raw_readl(CFGCHIP2),
>  		 musb_readb(reg_base, DA8XX_USB_CTRL_REG));
>  
> +	da8xx_musb_disable(musb);
>  	musb->isr = da8xx_musb_interrupt;
>  	return 0;
>  fail:
> diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
> --- a/drivers/usb/musb/davinci.c
> +++ b/drivers/usb/musb/davinci.c
> @@ -432,6 +432,7 @@ static int davinci_musb_init(struct musb *musb)
>  		revision, __raw_readl(USB_PHY_CTRL),
>  		musb_readb(tibase, DAVINCI_USB_CTRL_REG));
>  
> +	davinci_musb_disable(musb);
>  	musb->isr = davinci_musb_interrupt;
>  	return 0;
>  
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 74fc306..f7dc3df 100644
> --- 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 */
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -441,6 +441,7 @@ static int dsps_musb_init(struct musb *musb)
>  	/* Reset the musb */
>  	musb_writel(reg_base, wrp->control, (1 << wrp->reset));
>  
> +	dsps_musb_disable(musb);
>  	musb->isr = dsps_interrupt;
>  
>  	/* reset the otgdisable bit, needed for host mode to work */
> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> --- a/drivers/usb/musb/sunxi.c
> +++ b/drivers/usb/musb/sunxi.c
> @@ -93,6 +93,8 @@ struct sunxi_glue {
>  	struct notifier_block	host_nb;
>  };
>  
> +static void sunxi_musb_disable(struct musb *musb);
> +
>  /* phy_power_on / off may sleep, so we use a workqueue  */
>  static void sunxi_musb_work(struct work_struct *work)
>  {
> @@ -265,6 +267,7 @@ static int sunxi_musb_init(struct musb *musb)
>  	if (ret)
>  		goto error_unregister_notifier;
>  
> +	sunxi_musb_disable(musb);
>  	musb->isr = sunxi_musb_interrupt;
>  
>  	/* Stop the musb-core from doing runtime pm (not supported on sunxi) */
> diff --git a/drivers/usb/musb/tusb6010.c b/drivers/usb/musb/tusb6010.c
> --- a/drivers/usb/musb/tusb6010.c
> +++ b/drivers/usb/musb/tusb6010.c
> @@ -1149,6 +1149,7 @@ static int tusb_musb_init(struct musb *musb)
>  				ret);
>  		goto done;
>  	}
> +	tusb_musb_disable(musb);
>  	musb->isr = tusb_musb_interrupt;
>  
>  	musb->xceiv->set_power = tusb_draw_power;
> -- 
> 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: [PATCHv3] usb: musb: Fix unbalanced platform_disable

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160916 07:04]:
> Hi,
> 
> On Wed, Sep 14, 2016 at 11:10:19AM -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. To do that in a safe way,
> > we need to call the glue specific *_musb_disable() directly in the
> > glue layer *_musb_init() as noted by Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>.
> > 
> > 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").
> > 
> > Further, most of the glue layers already do a reset of the musb
> > controller in the *_musb_init(), so later on we may want to
> > consider removing the calls to *__musb_disable() where a reset is
> > already done.
> > 
> > Note that 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: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 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>
> > ---
> 
> If Kishon wants to take it, here is my
> 
> Acked-by: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>

To recap, this seems to be now only partially needed with
commit d8e5f0eca1e8 ("usb: musb: Fix hardirq-safe hardirq-unsafe
lock order error") heading into v4.9-rc2.

The rest of the changes in the $subject patch we should probably
wait on until we have things working in a sane way with PM runtime.

I'll be sending the phy-twl4030-usb related changes separately.

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