diff mbox

[2/3] usb: musb: Add a quirk to preserve MUSB_DEVCTL during suspend

Message ID 1480350381-26151-3-git-send-email-abailon@baylibre.com
State New
Headers show

Commit Message

Alexandre Bailon Nov. 28, 2016, 4:26 p.m. UTC 
On da8xx, VBUS is not maintained during suspend when musb is in host mode.
On resume, all the connected devices will be disconnected and then will
be enumerated again.
This happens because MUSB_DEVCTL is cleared during suspend.
Add a quirk to preserve MUSB_DEVCTL during a suspend.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

---
 drivers/usb/musb/musb_core.c | 13 +++++++------
 drivers/usb/musb/musb_core.h |  1 +
 2 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.7.3

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

Comments

Bin Liu Nov. 29, 2016, 4:16 p.m. UTC | #1 
Hi,

On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:
> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

> On resume, all the connected devices will be disconnected and then will

> be enumerated again.

> This happens because MUSB_DEVCTL is cleared during suspend.

> Add a quirk to preserve MUSB_DEVCTL during a suspend.


Will preserving MUSB_DEVCTL prevent the device getting disconnected?
This doesn't happen on am335x. The PHY is off during suspend, during
resume, PHY gets on, MUSB generates MUSB_CONNECT interrupt, then
re-enumeration happens.

Regards,
-Bin.

> 

> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

> ---

>  drivers/usb/musb/musb_core.c | 13 +++++++------

>  drivers/usb/musb/musb_core.h |  1 +

>  2 files changed, 8 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c

> index 9e22646..7e2cd98 100644

> --- a/drivers/usb/musb/musb_core.c

> +++ b/drivers/usb/musb/musb_core.c

> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)

>  

>  }

>  

> -static void musb_generic_disable(struct musb *musb)

> +static void musb_generic_disable(struct musb *musb, bool suspend)

>  {

>  	void __iomem	*mbase = musb->mregs;

>  

>  	musb_disable_interrupts(musb);

>  

>  	/* off */

> -	musb_writeb(mbase, MUSB_DEVCTL, 0);

> +	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))

> +		musb_writeb(mbase, MUSB_DEVCTL, 0);

>  }

>  

>  /*

> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)

>  {

>  	/* stop IRQs, timers, ... */

>  	musb_platform_disable(musb);

> -	musb_generic_disable(musb);

> +	musb_generic_disable(musb, false);

>  	musb_dbg(musb, "HDRC disabled");

>  

>  	/* FIXME

> @@ -2310,7 +2311,7 @@ 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);

> +	musb_generic_disable(musb, false);

>  

>  	/* Init IRQ workqueue before request_irq */

>  	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);

> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)

>  	musb_gadget_cleanup(musb);

>  	spin_lock_irqsave(&musb->lock, flags);

>  	musb_platform_disable(musb);

> -	musb_generic_disable(musb);

> +	musb_generic_disable(musb, false);

>  	spin_unlock_irqrestore(&musb->lock, flags);

>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

>  	pm_runtime_dont_use_autosuspend(musb->controller);

> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)

>  	unsigned long	flags;

>  

>  	musb_platform_disable(musb);

> -	musb_generic_disable(musb);

> +	musb_generic_disable(musb, true);

>  	WARN_ON(!list_empty(&musb->pending_list));

>  

>  	spin_lock_irqsave(&musb->lock, flags);

> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

> index a611e2f..22961ef 100644

> --- a/drivers/usb/musb/musb_core.h

> +++ b/drivers/usb/musb/musb_core.h

> @@ -172,6 +172,7 @@ struct musb_io;

>   */

>  struct musb_platform_ops {

>  

> +#define MUSB_PRESERVE_DEVCTL	BIT(7)

>  #define MUSB_DMA_UX500		BIT(6)

>  #define MUSB_DMA_CPPI41		BIT(5)

>  #define MUSB_DMA_CPPI		BIT(4)

> -- 

> 2.7.3

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Bailon Nov. 29, 2016, 5:41 p.m. UTC | #2 
On 11/29/2016 05:16 PM, Bin Liu wrote:
> Hi,

> 

> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:

>> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

>> On resume, all the connected devices will be disconnected and then will

>> be enumerated again.

>> This happens because MUSB_DEVCTL is cleared during suspend.

>> Add a quirk to preserve MUSB_DEVCTL during a suspend.

> 

> Will preserving MUSB_DEVCTL prevent the device getting disconnected?

Yes. Preserving MUSB_DEVCTL keeps VBUS on though the PHY is off during
suspend.
Note that in device mode, the disconnect still happen but I think it's
the expected behavior.
> This doesn't happen on am335x. The PHY is off during suspend, during

> resume, PHY gets on, MUSB generates MUSB_CONNECT interrupt, then

> re-enumeration happens.

I haven't been able to try on the BBB. I don't know if my config is
incorrect or if some patches are missing but I was not able to
suspend it.

Regards,
Alexandre
> 

> Regards,

> -Bin.

> 

>>

>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

>> ---

>>  drivers/usb/musb/musb_core.c | 13 +++++++------

>>  drivers/usb/musb/musb_core.h |  1 +

>>  2 files changed, 8 insertions(+), 6 deletions(-)

>>

>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c

>> index 9e22646..7e2cd98 100644

>> --- a/drivers/usb/musb/musb_core.c

>> +++ b/drivers/usb/musb/musb_core.c

>> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)

>>  

>>  }

>>  

>> -static void musb_generic_disable(struct musb *musb)

>> +static void musb_generic_disable(struct musb *musb, bool suspend)

>>  {

>>  	void __iomem	*mbase = musb->mregs;

>>  

>>  	musb_disable_interrupts(musb);

>>  

>>  	/* off */

>> -	musb_writeb(mbase, MUSB_DEVCTL, 0);

>> +	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))

>> +		musb_writeb(mbase, MUSB_DEVCTL, 0);

>>  }

>>  

>>  /*

>> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)

>>  {

>>  	/* stop IRQs, timers, ... */

>>  	musb_platform_disable(musb);

>> -	musb_generic_disable(musb);

>> +	musb_generic_disable(musb, false);

>>  	musb_dbg(musb, "HDRC disabled");

>>  

>>  	/* FIXME

>> @@ -2310,7 +2311,7 @@ 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);

>> +	musb_generic_disable(musb, false);

>>  

>>  	/* Init IRQ workqueue before request_irq */

>>  	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);

>> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)

>>  	musb_gadget_cleanup(musb);

>>  	spin_lock_irqsave(&musb->lock, flags);

>>  	musb_platform_disable(musb);

>> -	musb_generic_disable(musb);

>> +	musb_generic_disable(musb, false);

>>  	spin_unlock_irqrestore(&musb->lock, flags);

>>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

>>  	pm_runtime_dont_use_autosuspend(musb->controller);

>> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)

>>  	unsigned long	flags;

>>  

>>  	musb_platform_disable(musb);

>> -	musb_generic_disable(musb);

>> +	musb_generic_disable(musb, true);

>>  	WARN_ON(!list_empty(&musb->pending_list));

>>  

>>  	spin_lock_irqsave(&musb->lock, flags);

>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

>> index a611e2f..22961ef 100644

>> --- a/drivers/usb/musb/musb_core.h

>> +++ b/drivers/usb/musb/musb_core.h

>> @@ -172,6 +172,7 @@ struct musb_io;

>>   */

>>  struct musb_platform_ops {

>>  

>> +#define MUSB_PRESERVE_DEVCTL	BIT(7)

>>  #define MUSB_DMA_UX500		BIT(6)

>>  #define MUSB_DMA_CPPI41		BIT(5)

>>  #define MUSB_DMA_CPPI		BIT(4)

>> -- 

>> 2.7.3

>>


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bin Liu Nov. 29, 2016, 5:56 p.m. UTC | #3 
On Tue, Nov 29, 2016 at 06:41:04PM +0100, Alexandre Bailon wrote:
> On 11/29/2016 05:16 PM, Bin Liu wrote:

> > Hi,

> > 

> > On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:

> >> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

> >> On resume, all the connected devices will be disconnected and then will

> >> be enumerated again.

> >> This happens because MUSB_DEVCTL is cleared during suspend.

> >> Add a quirk to preserve MUSB_DEVCTL during a suspend.

> > 

> > Will preserving MUSB_DEVCTL prevent the device getting disconnected?

> Yes. Preserving MUSB_DEVCTL keeps VBUS on though the PHY is off during

> suspend.


VBUS will be on, but does it prevent disconnecting the usb device on
resume? I don't have a DA8xx to test, but I doubt it, since the PHY is
off.

> Note that in device mode, the disconnect still happen but I think it's

> the expected behavior.


Right, the PHY off disables the pullup.

> > This doesn't happen on am335x. The PHY is off during suspend, during

> > resume, PHY gets on, MUSB generates MUSB_CONNECT interrupt, then

> > re-enumeration happens.

> I haven't been able to try on the BBB. I don't know if my config is

> incorrect or if some patches are missing but I was not able to

> suspend it.


I heard the mainline kernel currently is broken on am335x PM, so
suspend/resume doesn't work. I had to test it on TI kernel.

Regards,
-Bin.

> 

> Regards,

> Alexandre

> > 

> > Regards,

> > -Bin.

> > 

> >>

> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

> >> ---

> >>  drivers/usb/musb/musb_core.c | 13 +++++++------

> >>  drivers/usb/musb/musb_core.h |  1 +

> >>  2 files changed, 8 insertions(+), 6 deletions(-)

> >>

> >> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c

> >> index 9e22646..7e2cd98 100644

> >> --- a/drivers/usb/musb/musb_core.c

> >> +++ b/drivers/usb/musb/musb_core.c

> >> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)

> >>  

> >>  }

> >>  

> >> -static void musb_generic_disable(struct musb *musb)

> >> +static void musb_generic_disable(struct musb *musb, bool suspend)

> >>  {

> >>  	void __iomem	*mbase = musb->mregs;

> >>  

> >>  	musb_disable_interrupts(musb);

> >>  

> >>  	/* off */

> >> -	musb_writeb(mbase, MUSB_DEVCTL, 0);

> >> +	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))

> >> +		musb_writeb(mbase, MUSB_DEVCTL, 0);

> >>  }

> >>  

> >>  /*

> >> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)

> >>  {

> >>  	/* stop IRQs, timers, ... */

> >>  	musb_platform_disable(musb);

> >> -	musb_generic_disable(musb);

> >> +	musb_generic_disable(musb, false);

> >>  	musb_dbg(musb, "HDRC disabled");

> >>  

> >>  	/* FIXME

> >> @@ -2310,7 +2311,7 @@ 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);

> >> +	musb_generic_disable(musb, false);

> >>  

> >>  	/* Init IRQ workqueue before request_irq */

> >>  	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);

> >> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)

> >>  	musb_gadget_cleanup(musb);

> >>  	spin_lock_irqsave(&musb->lock, flags);

> >>  	musb_platform_disable(musb);

> >> -	musb_generic_disable(musb);

> >> +	musb_generic_disable(musb, false);

> >>  	spin_unlock_irqrestore(&musb->lock, flags);

> >>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

> >>  	pm_runtime_dont_use_autosuspend(musb->controller);

> >> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)

> >>  	unsigned long	flags;

> >>  

> >>  	musb_platform_disable(musb);

> >> -	musb_generic_disable(musb);

> >> +	musb_generic_disable(musb, true);

> >>  	WARN_ON(!list_empty(&musb->pending_list));

> >>  

> >>  	spin_lock_irqsave(&musb->lock, flags);

> >> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

> >> index a611e2f..22961ef 100644

> >> --- a/drivers/usb/musb/musb_core.h

> >> +++ b/drivers/usb/musb/musb_core.h

> >> @@ -172,6 +172,7 @@ struct musb_io;

> >>   */

> >>  struct musb_platform_ops {

> >>  

> >> +#define MUSB_PRESERVE_DEVCTL	BIT(7)

> >>  #define MUSB_DMA_UX500		BIT(6)

> >>  #define MUSB_DMA_CPPI41		BIT(5)

> >>  #define MUSB_DMA_CPPI		BIT(4)

> >> -- 

> >> 2.7.3

> >>

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Bailon Dec. 2, 2016, 2:23 p.m. UTC | #4 
On 11/29/2016 06:56 PM, Bin Liu wrote:
> On Tue, Nov 29, 2016 at 06:41:04PM +0100, Alexandre Bailon wrote:

>> On 11/29/2016 05:16 PM, Bin Liu wrote:

>>> Hi,

>>>

>>> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:

>>>> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

>>>> On resume, all the connected devices will be disconnected and then will

>>>> be enumerated again.

>>>> This happens because MUSB_DEVCTL is cleared during suspend.

>>>> Add a quirk to preserve MUSB_DEVCTL during a suspend.

>>>

>>> Will preserving MUSB_DEVCTL prevent the device getting disconnected?

>> Yes. Preserving MUSB_DEVCTL keeps VBUS on though the PHY is off during

>> suspend.

> 

> VBUS will be on, but does it prevent disconnecting the usb device on

> resume? I don't have a DA8xx to test, but I doubt it, since the PHY is

> off.

Actually, the PHY is not really off.
The name is probably confusing but the PHY off only put the PHY in the
lowest power state (chapter 10.11.1 of omapl138 TRM).
I have tried with the TI kernel and it was able to suspend and resume
the omapl138-lcdk without without getting a disconnect.
That why I wrote this series.

Regards,
Alexandre
> 

>> Note that in device mode, the disconnect still happen but I think it's

>> the expected behavior.

> 

> Right, the PHY off disables the pullup.

> 

>>> This doesn't happen on am335x. The PHY is off during suspend, during

>>> resume, PHY gets on, MUSB generates MUSB_CONNECT interrupt, then

>>> re-enumeration happens.

>> I haven't been able to try on the BBB. I don't know if my config is

>> incorrect or if some patches are missing but I was not able to

>> suspend it.

> 

> I heard the mainline kernel currently is broken on am335x PM, so

> suspend/resume doesn't work. I had to test it on TI kernel.

> 

> Regards,

> -Bin.

> 

>>

>> Regards,

>> Alexandre

>>>

>>> Regards,

>>> -Bin.

>>>

>>>>

>>>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

>>>> ---

>>>>  drivers/usb/musb/musb_core.c | 13 +++++++------

>>>>  drivers/usb/musb/musb_core.h |  1 +

>>>>  2 files changed, 8 insertions(+), 6 deletions(-)

>>>>

>>>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c

>>>> index 9e22646..7e2cd98 100644

>>>> --- a/drivers/usb/musb/musb_core.c

>>>> +++ b/drivers/usb/musb/musb_core.c

>>>> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)

>>>>  

>>>>  }

>>>>  

>>>> -static void musb_generic_disable(struct musb *musb)

>>>> +static void musb_generic_disable(struct musb *musb, bool suspend)

>>>>  {

>>>>  	void __iomem	*mbase = musb->mregs;

>>>>  

>>>>  	musb_disable_interrupts(musb);

>>>>  

>>>>  	/* off */

>>>> -	musb_writeb(mbase, MUSB_DEVCTL, 0);

>>>> +	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))

>>>> +		musb_writeb(mbase, MUSB_DEVCTL, 0);

>>>>  }

>>>>  

>>>>  /*

>>>> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)

>>>>  {

>>>>  	/* stop IRQs, timers, ... */

>>>>  	musb_platform_disable(musb);

>>>> -	musb_generic_disable(musb);

>>>> +	musb_generic_disable(musb, false);

>>>>  	musb_dbg(musb, "HDRC disabled");

>>>>  

>>>>  	/* FIXME

>>>> @@ -2310,7 +2311,7 @@ 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);

>>>> +	musb_generic_disable(musb, false);

>>>>  

>>>>  	/* Init IRQ workqueue before request_irq */

>>>>  	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);

>>>> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)

>>>>  	musb_gadget_cleanup(musb);

>>>>  	spin_lock_irqsave(&musb->lock, flags);

>>>>  	musb_platform_disable(musb);

>>>> -	musb_generic_disable(musb);

>>>> +	musb_generic_disable(musb, false);

>>>>  	spin_unlock_irqrestore(&musb->lock, flags);

>>>>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

>>>>  	pm_runtime_dont_use_autosuspend(musb->controller);

>>>> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)

>>>>  	unsigned long	flags;

>>>>  

>>>>  	musb_platform_disable(musb);

>>>> -	musb_generic_disable(musb);

>>>> +	musb_generic_disable(musb, true);

>>>>  	WARN_ON(!list_empty(&musb->pending_list));

>>>>  

>>>>  	spin_lock_irqsave(&musb->lock, flags);

>>>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

>>>> index a611e2f..22961ef 100644

>>>> --- a/drivers/usb/musb/musb_core.h

>>>> +++ b/drivers/usb/musb/musb_core.h

>>>> @@ -172,6 +172,7 @@ struct musb_io;

>>>>   */

>>>>  struct musb_platform_ops {

>>>>  

>>>> +#define MUSB_PRESERVE_DEVCTL	BIT(7)

>>>>  #define MUSB_DMA_UX500		BIT(6)

>>>>  #define MUSB_DMA_CPPI41		BIT(5)

>>>>  #define MUSB_DMA_CPPI		BIT(4)

>>>> -- 

>>>> 2.7.3

>>>>

>>


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bin Liu Dec. 5, 2016, 4:47 p.m. UTC | #5 
On Fri, Dec 02, 2016 at 03:23:50PM +0100, Alexandre Bailon wrote:
> On 11/29/2016 06:56 PM, Bin Liu wrote:

> > On Tue, Nov 29, 2016 at 06:41:04PM +0100, Alexandre Bailon wrote:

> >> On 11/29/2016 05:16 PM, Bin Liu wrote:

> >>> Hi,

> >>>

> >>> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:

> >>>> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

> >>>> On resume, all the connected devices will be disconnected and then will

> >>>> be enumerated again.

> >>>> This happens because MUSB_DEVCTL is cleared during suspend.

> >>>> Add a quirk to preserve MUSB_DEVCTL during a suspend.

> >>>

> >>> Will preserving MUSB_DEVCTL prevent the device getting disconnected?

> >> Yes. Preserving MUSB_DEVCTL keeps VBUS on though the PHY is off during

> >> suspend.

> > 

> > VBUS will be on, but does it prevent disconnecting the usb device on

> > resume? I don't have a DA8xx to test, but I doubt it, since the PHY is

> > off.

> Actually, the PHY is not really off.


I guess the PHY should be off when the system is suspended for an
aggressive power saving.

Sekhar, any comments?

Regards,
-Bin.

> The name is probably confusing but the PHY off only put the PHY in the

> lowest power state (chapter 10.11.1 of omapl138 TRM).

> I have tried with the TI kernel and it was able to suspend and resume

> the omapl138-lcdk without without getting a disconnect.

> That why I wrote this series.

> 

> Regards,

> Alexandre

> > 

> >> Note that in device mode, the disconnect still happen but I think it's

> >> the expected behavior.

> > 

> > Right, the PHY off disables the pullup.

> > 

> >>> This doesn't happen on am335x. The PHY is off during suspend, during

> >>> resume, PHY gets on, MUSB generates MUSB_CONNECT interrupt, then

> >>> re-enumeration happens.

> >> I haven't been able to try on the BBB. I don't know if my config is

> >> incorrect or if some patches are missing but I was not able to

> >> suspend it.

> > 

> > I heard the mainline kernel currently is broken on am335x PM, so

> > suspend/resume doesn't work. I had to test it on TI kernel.

> > 

> > Regards,

> > -Bin.

> > 

> >>

> >> Regards,

> >> Alexandre

> >>>

> >>> Regards,

> >>> -Bin.

> >>>

> >>>>

> >>>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

> >>>> ---

> >>>>  drivers/usb/musb/musb_core.c | 13 +++++++------

> >>>>  drivers/usb/musb/musb_core.h |  1 +

> >>>>  2 files changed, 8 insertions(+), 6 deletions(-)

> >>>>

> >>>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c

> >>>> index 9e22646..7e2cd98 100644

> >>>> --- a/drivers/usb/musb/musb_core.c

> >>>> +++ b/drivers/usb/musb/musb_core.c

> >>>> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)

> >>>>  

> >>>>  }

> >>>>  

> >>>> -static void musb_generic_disable(struct musb *musb)

> >>>> +static void musb_generic_disable(struct musb *musb, bool suspend)

> >>>>  {

> >>>>  	void __iomem	*mbase = musb->mregs;

> >>>>  

> >>>>  	musb_disable_interrupts(musb);

> >>>>  

> >>>>  	/* off */

> >>>> -	musb_writeb(mbase, MUSB_DEVCTL, 0);

> >>>> +	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))

> >>>> +		musb_writeb(mbase, MUSB_DEVCTL, 0);

> >>>>  }

> >>>>  

> >>>>  /*

> >>>> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)

> >>>>  {

> >>>>  	/* stop IRQs, timers, ... */

> >>>>  	musb_platform_disable(musb);

> >>>> -	musb_generic_disable(musb);

> >>>> +	musb_generic_disable(musb, false);

> >>>>  	musb_dbg(musb, "HDRC disabled");

> >>>>  

> >>>>  	/* FIXME

> >>>> @@ -2310,7 +2311,7 @@ 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);

> >>>> +	musb_generic_disable(musb, false);

> >>>>  

> >>>>  	/* Init IRQ workqueue before request_irq */

> >>>>  	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);

> >>>> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)

> >>>>  	musb_gadget_cleanup(musb);

> >>>>  	spin_lock_irqsave(&musb->lock, flags);

> >>>>  	musb_platform_disable(musb);

> >>>> -	musb_generic_disable(musb);

> >>>> +	musb_generic_disable(musb, false);

> >>>>  	spin_unlock_irqrestore(&musb->lock, flags);

> >>>>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

> >>>>  	pm_runtime_dont_use_autosuspend(musb->controller);

> >>>> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)

> >>>>  	unsigned long	flags;

> >>>>  

> >>>>  	musb_platform_disable(musb);

> >>>> -	musb_generic_disable(musb);

> >>>> +	musb_generic_disable(musb, true);

> >>>>  	WARN_ON(!list_empty(&musb->pending_list));

> >>>>  

> >>>>  	spin_lock_irqsave(&musb->lock, flags);

> >>>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

> >>>> index a611e2f..22961ef 100644

> >>>> --- a/drivers/usb/musb/musb_core.h

> >>>> +++ b/drivers/usb/musb/musb_core.h

> >>>> @@ -172,6 +172,7 @@ struct musb_io;

> >>>>   */

> >>>>  struct musb_platform_ops {

> >>>>  

> >>>> +#define MUSB_PRESERVE_DEVCTL	BIT(7)

> >>>>  #define MUSB_DMA_UX500		BIT(6)

> >>>>  #define MUSB_DMA_CPPI41		BIT(5)

> >>>>  #define MUSB_DMA_CPPI		BIT(4)

> >>>> -- 

> >>>> 2.7.3

> >>>>

> >>

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sekhar Nori Dec. 6, 2016, 12:59 p.m. UTC | #6 
On Monday 05 December 2016 10:17 PM, Bin Liu wrote:
> On Fri, Dec 02, 2016 at 03:23:50PM +0100, Alexandre Bailon wrote:

>> On 11/29/2016 06:56 PM, Bin Liu wrote:

>>> On Tue, Nov 29, 2016 at 06:41:04PM +0100, Alexandre Bailon wrote:

>>>> On 11/29/2016 05:16 PM, Bin Liu wrote:

>>>>> Hi,

>>>>>

>>>>> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:

>>>>>> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

>>>>>> On resume, all the connected devices will be disconnected and then will

>>>>>> be enumerated again.

>>>>>> This happens because MUSB_DEVCTL is cleared during suspend.

>>>>>> Add a quirk to preserve MUSB_DEVCTL during a suspend.

>>>>>

>>>>> Will preserving MUSB_DEVCTL prevent the device getting disconnected?

>>>> Yes. Preserving MUSB_DEVCTL keeps VBUS on though the PHY is off during

>>>> suspend.

>>>

>>> VBUS will be on, but does it prevent disconnecting the usb device on

>>> resume? I don't have a DA8xx to test, but I doubt it, since the PHY is

>>> off.

>> Actually, the PHY is not really off.

> 

> I guess the PHY should be off when the system is suspended for an

> aggressive power saving.

> 

> Sekhar, any comments?


No, nothing from my side. I haven't really played with this side of
DA850 at all.

For your reference, this is what chapter 10.11.1 referenced by Alexandre
says:

"
The USB modules can be clock gated using the PSC; however, this does not
power down/clock gate the PHY logic. You can put the USB2.0 PHY and OTG
module in the lowest power state, when not in use, by writing to the
USB0PHYPWDN and the USB0OTGPWRDN bits in the Chip Configuration 2
Register (CFGCHIP2) in the System Configuration (SYSCFG) Module chapter.
"

It is little bit ambiguous on if USB0PHYPWDN and USB0OTGPWRDN do really
power down the PHY.

I would just program the bits suggested by the manual and assume that
the hardware reaches the safest low power state it can reach.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bin Liu Dec. 6, 2016, 2:47 p.m. UTC | #7 
On Tue, Dec 06, 2016 at 06:29:31PM +0530, Sekhar Nori wrote:
> On Monday 05 December 2016 10:17 PM, Bin Liu wrote:

> > On Fri, Dec 02, 2016 at 03:23:50PM +0100, Alexandre Bailon wrote:

> >> On 11/29/2016 06:56 PM, Bin Liu wrote:

> >>> On Tue, Nov 29, 2016 at 06:41:04PM +0100, Alexandre Bailon wrote:

> >>>> On 11/29/2016 05:16 PM, Bin Liu wrote:

> >>>>> Hi,

> >>>>>

> >>>>> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:

> >>>>>> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

> >>>>>> On resume, all the connected devices will be disconnected and then will

> >>>>>> be enumerated again.

> >>>>>> This happens because MUSB_DEVCTL is cleared during suspend.

> >>>>>> Add a quirk to preserve MUSB_DEVCTL during a suspend.

> >>>>>

> >>>>> Will preserving MUSB_DEVCTL prevent the device getting disconnected?

> >>>> Yes. Preserving MUSB_DEVCTL keeps VBUS on though the PHY is off during

> >>>> suspend.

> >>>

> >>> VBUS will be on, but does it prevent disconnecting the usb device on

> >>> resume? I don't have a DA8xx to test, but I doubt it, since the PHY is

> >>> off.

> >> Actually, the PHY is not really off.

> > 

> > I guess the PHY should be off when the system is suspended for an

> > aggressive power saving.

> > 

> > Sekhar, any comments?

> 

> No, nothing from my side. I haven't really played with this side of

> DA850 at all.


Ok, I wanted to know the power requirement in system suspend - do we
want to power off the PHY to save power as much as possible, or leave
the PHY on to not disconnect enumerated devices.

> 

> For your reference, this is what chapter 10.11.1 referenced by Alexandre

> says:

> 

> "

> The USB modules can be clock gated using the PSC; however, this does not

> power down/clock gate the PHY logic. You can put the USB2.0 PHY and OTG

> module in the lowest power state, when not in use, by writing to the

> USB0PHYPWDN and the USB0OTGPWRDN bits in the Chip Configuration 2

> Register (CFGCHIP2) in the System Configuration (SYSCFG) Module chapter.

> "


It sounds like the PHY cannot be turned off on DA8xx?

> 

> It is little bit ambiguous on if USB0PHYPWDN and USB0OTGPWRDN do really

> power down the PHY.

> 

> I would just program the bits suggested by the manual and assume that

> the hardware reaches the safest low power state it can reach.

> 

> Thanks,

> Sekhar


Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bin Liu Dec. 16, 2016, 2:36 a.m. UTC | #8 
On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:
> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

> On resume, all the connected devices will be disconnected and then will

> be enumerated again.

> This happens because MUSB_DEVCTL is cleared during suspend.

> Add a quirk to preserve MUSB_DEVCTL during a suspend.

> 

> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

> ---

>  drivers/usb/musb/musb_core.c | 13 +++++++------

>  drivers/usb/musb/musb_core.h |  1 +

>  2 files changed, 8 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c

> index 9e22646..7e2cd98 100644

> --- a/drivers/usb/musb/musb_core.c

> +++ b/drivers/usb/musb/musb_core.c

> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)

>  

>  }

>  

> -static void musb_generic_disable(struct musb *musb)

> +static void musb_generic_disable(struct musb *musb, bool suspend)


I don't think I like it, so made a change [1] to remove this function,
since it only has two lines of code.

>  {

>  	void __iomem	*mbase = musb->mregs;

>  

>  	musb_disable_interrupts(musb);

>  

>  	/* off */

> -	musb_writeb(mbase, MUSB_DEVCTL, 0);

> +	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))

> +		musb_writeb(mbase, MUSB_DEVCTL, 0);


So now you can move this quirk check into musb_suspend(),

>  }

>  

>  /*

> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)

>  {

>  	/* stop IRQs, timers, ... */

>  	musb_platform_disable(musb);

> -	musb_generic_disable(musb);

> +	musb_generic_disable(musb, false);

>  	musb_dbg(musb, "HDRC disabled");

>  

>  	/* FIXME

> @@ -2310,7 +2311,7 @@ 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);

> +	musb_generic_disable(musb, false);

>  

>  	/* Init IRQ workqueue before request_irq */

>  	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);

> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)

>  	musb_gadget_cleanup(musb);

>  	spin_lock_irqsave(&musb->lock, flags);

>  	musb_platform_disable(musb);

> -	musb_generic_disable(musb);

> +	musb_generic_disable(musb, false);

>  	spin_unlock_irqrestore(&musb->lock, flags);

>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

>  	pm_runtime_dont_use_autosuspend(musb->controller);

> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)

>  	unsigned long	flags;

>  

>  	musb_platform_disable(musb);

> -	musb_generic_disable(musb);

> +	musb_generic_disable(musb, true);

>  	WARN_ON(!list_empty(&musb->pending_list));


and all the changes above are no longer needed. Can you please revise
this patch based on [1]?

>  

>  	spin_lock_irqsave(&musb->lock, flags);

> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

> index a611e2f..22961ef 100644

> --- a/drivers/usb/musb/musb_core.h

> +++ b/drivers/usb/musb/musb_core.h

> @@ -172,6 +172,7 @@ struct musb_io;

>   */

>  struct musb_platform_ops {

>  

> +#define MUSB_PRESERVE_DEVCTL	BIT(7)


Does it miss a TAB before BIT to align with follows?

>  #define MUSB_DMA_UX500		BIT(6)

>  #define MUSB_DMA_CPPI41		BIT(5)

>  #define MUSB_DMA_CPPI		BIT(4)

> -- 

> 2.7.3

>


[1] http://www.spinics.net/lists/linux-usb/msg150856.html

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Bailon Dec. 16, 2016, 10:33 a.m. UTC | #9 
On 12/16/2016 03:36 AM, Bin Liu wrote:
> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:

>> On da8xx, VBUS is not maintained during suspend when musb is in host mode.

>> On resume, all the connected devices will be disconnected and then will

>> be enumerated again.

>> This happens because MUSB_DEVCTL is cleared during suspend.

>> Add a quirk to preserve MUSB_DEVCTL during a suspend.

>>

>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

>> ---

>>  drivers/usb/musb/musb_core.c | 13 +++++++------

>>  drivers/usb/musb/musb_core.h |  1 +

>>  2 files changed, 8 insertions(+), 6 deletions(-)

>>

>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c

>> index 9e22646..7e2cd98 100644

>> --- a/drivers/usb/musb/musb_core.c

>> +++ b/drivers/usb/musb/musb_core.c

>> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)

>>  

>>  }

>>  

>> -static void musb_generic_disable(struct musb *musb)

>> +static void musb_generic_disable(struct musb *musb, bool suspend)

> 

> I don't think I like it, so made a change [1] to remove this function,

> since it only has two lines of code.

> 

>>  {

>>  	void __iomem	*mbase = musb->mregs;

>>  

>>  	musb_disable_interrupts(musb);

>>  

>>  	/* off */

>> -	musb_writeb(mbase, MUSB_DEVCTL, 0);

>> +	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))

>> +		musb_writeb(mbase, MUSB_DEVCTL, 0);

> 

> So now you can move this quirk check into musb_suspend(),

> 

>>  }

>>  

>>  /*

>> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)

>>  {

>>  	/* stop IRQs, timers, ... */

>>  	musb_platform_disable(musb);

>> -	musb_generic_disable(musb);

>> +	musb_generic_disable(musb, false);

>>  	musb_dbg(musb, "HDRC disabled");

>>  

>>  	/* FIXME

>> @@ -2310,7 +2311,7 @@ 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);

>> +	musb_generic_disable(musb, false);

>>  

>>  	/* Init IRQ workqueue before request_irq */

>>  	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);

>> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)

>>  	musb_gadget_cleanup(musb);

>>  	spin_lock_irqsave(&musb->lock, flags);

>>  	musb_platform_disable(musb);

>> -	musb_generic_disable(musb);

>> +	musb_generic_disable(musb, false);

>>  	spin_unlock_irqrestore(&musb->lock, flags);

>>  	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

>>  	pm_runtime_dont_use_autosuspend(musb->controller);

>> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)

>>  	unsigned long	flags;

>>  

>>  	musb_platform_disable(musb);

>> -	musb_generic_disable(musb);

>> +	musb_generic_disable(musb, true);

>>  	WARN_ON(!list_empty(&musb->pending_list));

> 

> and all the changes above are no longer needed. Can you please revise

> this patch based on [1]?

> 

>>  

>>  	spin_lock_irqsave(&musb->lock, flags);

>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

>> index a611e2f..22961ef 100644

>> --- a/drivers/usb/musb/musb_core.h

>> +++ b/drivers/usb/musb/musb_core.h

>> @@ -172,6 +172,7 @@ struct musb_io;

>>   */

>>  struct musb_platform_ops {

>>  

>> +#define MUSB_PRESERVE_DEVCTL	BIT(7)

> 

> Does it miss a TAB before BIT to align with follows?

I have checked and I don't think there is any missing TAB.

Regards,
Alexandre
> 

>>  #define MUSB_DMA_UX500		BIT(6)

>>  #define MUSB_DMA_CPPI41		BIT(5)

>>  #define MUSB_DMA_CPPI		BIT(4)

>> -- 

>> 2.7.3

>>

> 

> [1] http://www.spinics.net/lists/linux-usb/msg150856.html

> 

> Regards,

> -Bin.

> 


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

Patch

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 9e22646..7e2cd98 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1040,14 +1040,15 @@  static void musb_enable_interrupts(struct musb *musb)
 
 }
 
-static void musb_generic_disable(struct musb *musb)
+static void musb_generic_disable(struct musb *musb, bool suspend)
 {
 	void __iomem	*mbase = musb->mregs;
 
 	musb_disable_interrupts(musb);
 
 	/* off */
-	musb_writeb(mbase, MUSB_DEVCTL, 0);
+	if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))
+		musb_writeb(mbase, MUSB_DEVCTL, 0);
 }
 
 /*
@@ -1106,7 +1107,7 @@  void musb_stop(struct musb *musb)
 {
 	/* stop IRQs, timers, ... */
 	musb_platform_disable(musb);
-	musb_generic_disable(musb);
+	musb_generic_disable(musb, false);
 	musb_dbg(musb, "HDRC disabled");
 
 	/* FIXME
@@ -2310,7 +2311,7 @@  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);
+	musb_generic_disable(musb, false);
 
 	/* Init IRQ workqueue before request_irq */
 	INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);
@@ -2486,7 +2487,7 @@  static int musb_remove(struct platform_device *pdev)
 	musb_gadget_cleanup(musb);
 	spin_lock_irqsave(&musb->lock, flags);
 	musb_platform_disable(musb);
-	musb_generic_disable(musb);
+	musb_generic_disable(musb, false);
 	spin_unlock_irqrestore(&musb->lock, flags);
 	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 	pm_runtime_dont_use_autosuspend(musb->controller);
@@ -2663,7 +2664,7 @@  static int musb_suspend(struct device *dev)
 	unsigned long	flags;
 
 	musb_platform_disable(musb);
-	musb_generic_disable(musb);
+	musb_generic_disable(musb, true);
 	WARN_ON(!list_empty(&musb->pending_list));
 
 	spin_lock_irqsave(&musb->lock, flags);
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index a611e2f..22961ef 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -172,6 +172,7 @@  struct musb_io;
  */
 struct musb_platform_ops {
 
+#define MUSB_PRESERVE_DEVCTL	BIT(7)
 #define MUSB_DMA_UX500		BIT(6)
 #define MUSB_DMA_CPPI41		BIT(5)
 #define MUSB_DMA_CPPI		BIT(4)