Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Roger Quadros]

Hi Manu,

On 27/09/17 14:19, Manu Gautam wrote:
> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
> resume. While this works fine for gadget mode but in host
> mode there is not re-initialization of host stack. Also, resetting
> bus as part of bus_suspend/resume is not correct which could affect
> (or disconnect) connected devices.
> Fix this by not reinitializing core on suspend/resume in host mode
> for HOST only and OTG/drd configurations.
> 

All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
during dwc3_suspend() to have the lowest power state for our platforms.

After this patch, DWC3 controller and PHYs won't be turned off thus
preventing our platform from reaching low power levels.

So this is a regression for us (TI) in v4.15-rc.

Felipe, do you agree?

If yes I can send a patch which fixes the regression
and also makes USB host work after suspend/resume.


> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 03474d3..f75613f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -927,6 +927,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> +		dwc->current_dr_role = DWC3_GCTL_PRTCAP_DEVICE;
>  		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>  
>  		if (dwc->usb2_phy)
> @@ -942,6 +943,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_HOST:
> +		dwc->current_dr_role = DWC3_GCTL_PRTCAP_HOST;
>  		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>  
>  		if (dwc->usb2_phy)
> @@ -1293,21 +1295,19 @@ static int dwc3_suspend_common(struct dwc3 *dwc)
>  {
>  	unsigned long	flags;
>  
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
> +	switch (dwc->current_dr_role) {
> +	case DWC3_GCTL_PRTCAP_DEVICE:
>  		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_suspend(dwc);
>  		spin_unlock_irqrestore(&dwc->lock, flags);
> +		dwc3_core_exit(dwc);
>  		break;
> -	case USB_DR_MODE_HOST:
> +	case DWC3_GCTL_PRTCAP_HOST:
>  	default:
>  		/* do nothing */
>  		break;
>  	}
>  
> -	dwc3_core_exit(dwc);
> -
>  	return 0;
>  }
>  
> @@ -1316,18 +1316,17 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>  	unsigned long	flags;
>  	int		ret;
>  
> -	ret = dwc3_core_init(dwc);
> -	if (ret)
> -		return ret;
> +	switch (dwc->current_dr_role) {
> +	case DWC3_GCTL_PRTCAP_DEVICE:
> +		ret = dwc3_core_init(dwc);
> +		if (ret)
> +			return ret;
>  
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
>  		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_resume(dwc);
>  		spin_unlock_irqrestore(&dwc->lock, flags);
> -		/* FALLTHROUGH */
> -	case USB_DR_MODE_HOST:
> +		break;
> +	case DWC3_GCTL_PRTCAP_HOST:
>  	default:
>  		/* do nothing */
>  		break;
> @@ -1338,7 +1337,7 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>  
>  static int dwc3_runtime_checks(struct dwc3 *dwc)
>  {
> -	switch (dwc->dr_mode) {
> +	switch (dwc->current_dr_role) {
>  	case USB_DR_MODE_PERIPHERAL:
>  	case USB_DR_MODE_OTG:
>  		if (dwc->connected)
> @@ -1381,12 +1380,11 @@ static int dwc3_runtime_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
> +	switch (dwc->current_dr_role) {
> +	case DWC3_GCTL_PRTCAP_DEVICE:
>  		dwc3_gadget_process_pending_events(dwc);
>  		break;
> -	case USB_DR_MODE_HOST:
> +	case DWC3_GCTL_PRTCAP_HOST:
>  	default:
>  		/* do nothing */
>  		break;
> @@ -1402,13 +1400,12 @@ static int dwc3_runtime_idle(struct device *dev)
>  {
>  	struct dwc3     *dwc = dev_get_drvdata(dev);
>  
> -	switch (dwc->dr_mode) {
> -	case USB_DR_MODE_PERIPHERAL:
> -	case USB_DR_MODE_OTG:
> +	switch (dwc->current_dr_role) {
> +	case DWC3_GCTL_PRTCAP_DEVICE:
>  		if (dwc3_runtime_checks(dwc))
>  			return -EBUSY;
>  		break;
> -	case USB_DR_MODE_HOST:
> +	case DWC3_GCTL_PRTCAP_HOST:
>  	default:
>  		/* do nothing */
>  		break;
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> Hi Manu,
>
> On 27/09/17 14:19, Manu Gautam wrote:
>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>> resume. While this works fine for gadget mode but in host
>> mode there is not re-initialization of host stack. Also, resetting
>> bus as part of bus_suspend/resume is not correct which could affect
>> (or disconnect) connected devices.
>> Fix this by not reinitializing core on suspend/resume in host mode
>> for HOST only and OTG/drd configurations.
>> 
>
> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
> during dwc3_suspend() to have the lowest power state for our platforms.
>
> After this patch, DWC3 controller and PHYs won't be turned off thus
> preventing our platform from reaching low power levels.
>
> So this is a regression for us (TI) in v4.15-rc.
>
> Felipe, do you agree?
>
> If yes I can send a patch which fixes the regression
> and also makes USB host work after suspend/resume.

A power consumption regression is still a regression. But it's super
late in the -rc cycle, I think we need to get this merged after 4.16-rc1
and make sure to Cc stable.

Should be more than enough time to review and test patches.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Roger Quadros]

On 10/01/18 14:57, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
>> Hi Manu,
>>
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
> 
> A power consumption regression is still a regression. But it's super
> late in the -rc cycle, I think we need to get this merged after 4.16-rc1
> and make sure to Cc stable.
> 
> Should be more than enough time to review and test patches.
> 

Fine with me. 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
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: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Manu Gautam]

Hi,


On 1/10/2018 6:18 PM, Roger Quadros wrote:
> Hi Manu,
>
> On 27/09/17 14:19, Manu Gautam wrote:
>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>> resume. While this works fine for gadget mode but in host
>> mode there is not re-initialization of host stack. Also, resetting
>> bus as part of bus_suspend/resume is not correct which could affect
>> (or disconnect) connected devices.
>> Fix this by not reinitializing core on suspend/resume in host mode
>> for HOST only and OTG/drd configurations.
>>
> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
> during dwc3_suspend() to have the lowest power state for our platforms.
>
> After this patch, DWC3 controller and PHYs won't be turned off thus
> preventing our platform from reaching low power levels.
>
> So this is a regression for us (TI) in v4.15-rc.
>
> Felipe, do you agree?
>
> If yes I can send a patch which fixes the regression
> and also makes USB host work after suspend/resume.
>

I think it will be better to separate runtime_suspend and pm_suspend handling for
host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
suspend-resume should be ok but doing that for runtime suspend-resume is not
correct.
Let me know if that sounds ok, I can provide a patch for same instead of
reverting this which affects runtime PM with dwc3 host.
Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to
dr_mode as HOST.


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

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


Hi,

Manu Gautam <mgautam@codeaurora.org> writes:
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
>>
>
> I think it will be better to separate runtime_suspend and pm_suspend handling for
> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
> suspend-resume should be ok but doing that for runtime suspend-resume is not
> correct.

it sure is. It's part of hibernation-while-disconnected programming sequence

> Let me know if that sounds ok, I can provide a patch for same instead of
> reverting this which affects runtime PM with dwc3 host.

nope, that would break platforms using hibernation

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Manu Gautam]

Hi Felipe,


On 1/11/2018 1:44 PM, Felipe Balbi wrote:
> Hi,
>
> Manu Gautam <mgautam@codeaurora.org> writes:
>>> On 27/09/17 14:19, Manu Gautam wrote:
>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>>> resume. While this works fine for gadget mode but in host
>>>> mode there is not re-initialization of host stack. Also, resetting
>>>> bus as part of bus_suspend/resume is not correct which could affect
>>>> (or disconnect) connected devices.
>>>> Fix this by not reinitializing core on suspend/resume in host mode
>>>> for HOST only and OTG/drd configurations.
>>>>
>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>>> during dwc3_suspend() to have the lowest power state for our platforms.
>>>
>>> After this patch, DWC3 controller and PHYs won't be turned off thus
>>> preventing our platform from reaching low power levels.
>>>
>>> So this is a regression for us (TI) in v4.15-rc.
>>>
>>> Felipe, do you agree?
>>>
>>> If yes I can send a patch which fixes the regression
>>> and also makes USB host work after suspend/resume.
>>>
>> I think it will be better to separate runtime_suspend and pm_suspend handling for
>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>> correct.
> it sure is. It's part of hibernation-while-disconnected programming sequence
>
>> Let me know if that sounds ok, I can provide a patch for same instead of
>> reverting this which affects runtime PM with dwc3 host.
> nope, that would break platforms using hibernation

Please don't mind me asking this if it is very basic, I am probably missing something there
We should be able to distinguish between runtime_pm vs system_suspend/hibernation
and then process accordingly.
In host mode runtime suspend/resume could happen very often with device connected,
and resetting h/w on every runtime_resume might not be desired. And PHYs drivers
can also support runtime_suspend which would be preferred instead of shutting down
phy.


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

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


Hi,

Manu Gautam <mgautam@codeaurora.org> writes:
>>>> On 27/09/17 14:19, Manu Gautam wrote:
>>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>>>> resume. While this works fine for gadget mode but in host
>>>>> mode there is not re-initialization of host stack. Also, resetting
>>>>> bus as part of bus_suspend/resume is not correct which could affect
>>>>> (or disconnect) connected devices.
>>>>> Fix this by not reinitializing core on suspend/resume in host mode
>>>>> for HOST only and OTG/drd configurations.
>>>>>
>>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>>>> during dwc3_suspend() to have the lowest power state for our platforms.
>>>>
>>>> After this patch, DWC3 controller and PHYs won't be turned off thus
>>>> preventing our platform from reaching low power levels.
>>>>
>>>> So this is a regression for us (TI) in v4.15-rc.
>>>>
>>>> Felipe, do you agree?
>>>>
>>>> If yes I can send a patch which fixes the regression
>>>> and also makes USB host work after suspend/resume.
>>>>
>>> I think it will be better to separate runtime_suspend and pm_suspend handling for
>>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
>>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>>> correct.
>> it sure is. It's part of hibernation-while-disconnected programming sequence
>>
>>> Let me know if that sounds ok, I can provide a patch for same instead of
>>> reverting this which affects runtime PM with dwc3 host.
>> nope, that would break platforms using hibernation
>
> Please don't mind me asking this if it is very basic, I am probably
> missing something there
>
> We should be able to distinguish between runtime_pm vs
> system_suspend/hibernation and then process accordingly.

I'm not talking about Linux suspend to disk; I'm talking about Synopsys'
Hibernation feature (open up your databook and have a read ;-)

> In host mode runtime suspend/resume could happen very often with
> device connected, and resetting h/w on every runtime_resume might not
> be desired. And PHYs drivers can also support runtime_suspend which
> would be preferred instead of shutting down phy.

We don't do anything when dwc3 is working as a host, we simply assume if
we reach dwc3.ko, xhci has done its part. Here's what our
suspend_common looks like:

static int dwc3_suspend_common(struct dwc3 *dwc)
{
	unsigned long	flags;

	switch (dwc->current_dr_role) {
	case DWC3_GCTL_PRTCAP_DEVICE:
		spin_lock_irqsave(&dwc->lock, flags);
		dwc3_gadget_suspend(dwc);
		spin_unlock_irqrestore(&dwc->lock, flags);
		dwc3_core_exit(dwc);
		break;
	case DWC3_GCTL_PRTCAP_HOST:
	default:
		/* do nothing */
		break;
	}

	return 0;
}

We're not resetting anything, not tearing down anything. No idea why
you're saying that in host mode we're breaking things apart. If you have
out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
mainline is at fault.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Roger Quadros]

Felipe,

On 11/01/18 11:04, Felipe Balbi wrote:
> 
> Hi,
> 
> Manu Gautam <mgautam@codeaurora.org> writes:
>>>>> On 27/09/17 14:19, Manu Gautam wrote:
>>>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>>>>> resume. While this works fine for gadget mode but in host
>>>>>> mode there is not re-initialization of host stack. Also, resetting
>>>>>> bus as part of bus_suspend/resume is not correct which could affect
>>>>>> (or disconnect) connected devices.
>>>>>> Fix this by not reinitializing core on suspend/resume in host mode
>>>>>> for HOST only and OTG/drd configurations.
>>>>>>
>>>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>>>>> during dwc3_suspend() to have the lowest power state for our platforms.
>>>>>
>>>>> After this patch, DWC3 controller and PHYs won't be turned off thus
>>>>> preventing our platform from reaching low power levels.
>>>>>
>>>>> So this is a regression for us (TI) in v4.15-rc.
>>>>>
>>>>> Felipe, do you agree?
>>>>>
>>>>> If yes I can send a patch which fixes the regression
>>>>> and also makes USB host work after suspend/resume.
>>>>>
>>>> I think it will be better to separate runtime_suspend and pm_suspend handling for
>>>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
>>>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>>>> correct.
>>> it sure is. It's part of hibernation-while-disconnected programming sequence
>>>
>>>> Let me know if that sounds ok, I can provide a patch for same instead of
>>>> reverting this which affects runtime PM with dwc3 host.
>>> nope, that would break platforms using hibernation
>>
>> Please don't mind me asking this if it is very basic, I am probably
>> missing something there
>>
>> We should be able to distinguish between runtime_pm vs
>> system_suspend/hibernation and then process accordingly.
> 
> I'm not talking about Linux suspend to disk; I'm talking about Synopsys'
> Hibernation feature (open up your databook and have a read ;-)
> 
>> In host mode runtime suspend/resume could happen very often with
>> device connected, and resetting h/w on every runtime_resume might not
>> be desired. And PHYs drivers can also support runtime_suspend which
>> would be preferred instead of shutting down phy.
> 
> We don't do anything when dwc3 is working as a host, we simply assume if
> we reach dwc3.ko, xhci has done its part. Here's what our
> suspend_common looks like:
> 
> static int dwc3_suspend_common(struct dwc3 *dwc)
> {
> 	unsigned long	flags;
> 
> 	switch (dwc->current_dr_role) {
> 	case DWC3_GCTL_PRTCAP_DEVICE:
> 		spin_lock_irqsave(&dwc->lock, flags);
> 		dwc3_gadget_suspend(dwc);
> 		spin_unlock_irqrestore(&dwc->lock, flags);
> 		dwc3_core_exit(dwc);
> 		break;
> 	case DWC3_GCTL_PRTCAP_HOST:
> 	default:
> 		/* do nothing */
> 		break;
> 	}
> 
> 	return 0;
> }
> 
> We're not resetting anything, not tearing down anything. No idea why
> you're saying that in host mode we're breaking things apart. If you have
> out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
> mainline is at fault.
> 

This is the case after commit 689bf72c6e0d	("usb: dwc3: Don't reinitialize core during host bus-suspend/resume")
which is breaking low power for TI platforms in Host mode.

If we revert that commit we will be doing dwc3_core_exit() for host mode as well. Which is what we want for system suspend
but probably not for runtime suspend in host case.

This is why Manu wants to differentiate runtime vs system suspend.

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>> In host mode runtime suspend/resume could happen very often with
>>> device connected, and resetting h/w on every runtime_resume might not
>>> be desired. And PHYs drivers can also support runtime_suspend which
>>> would be preferred instead of shutting down phy.
>> 
>> We don't do anything when dwc3 is working as a host, we simply assume if
>> we reach dwc3.ko, xhci has done its part. Here's what our
>> suspend_common looks like:
>> 
>> static int dwc3_suspend_common(struct dwc3 *dwc)
>> {
>> 	unsigned long	flags;
>> 
>> 	switch (dwc->current_dr_role) {
>> 	case DWC3_GCTL_PRTCAP_DEVICE:
>> 		spin_lock_irqsave(&dwc->lock, flags);
>> 		dwc3_gadget_suspend(dwc);
>> 		spin_unlock_irqrestore(&dwc->lock, flags);
>> 		dwc3_core_exit(dwc);
>> 		break;
>> 	case DWC3_GCTL_PRTCAP_HOST:
>> 	default:
>> 		/* do nothing */
>> 		break;
>> 	}
>> 
>> 	return 0;
>> }
>> 
>> We're not resetting anything, not tearing down anything. No idea why
>> you're saying that in host mode we're breaking things apart. If you have
>> out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
>> mainline is at fault.
>> 
>
> This is the case after commit 689bf72c6e0d ("usb: dwc3: Don't
> reinitialize core during host bus-suspend/resume") which is breaking
> low power for TI platforms in Host mode.
>
> If we revert that commit we will be doing dwc3_core_exit() for host
> mode as well. Which is what we want for system suspend but probably
> not for runtime suspend in host case.
>
> This is why Manu wants to differentiate runtime vs system suspend.

We already differentiate them. Maybe the only thing we need is to *not*
call core_exit() in host-mode during runtime_suspend, but call it if
mode is device or during system sleep.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Roger Quadros]

Hi Manu,

On 11/01/18 03:41, Manu Gautam wrote:
> Hi,
> 
> 
> On 1/10/2018 6:18 PM, Roger Quadros wrote:
>> Hi Manu,
>>
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
>>
> 
> I think it will be better to separate runtime_suspend and pm_suspend handling for
> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
> suspend-resume should be ok but doing that for runtime suspend-resume is not
> correct.
> Let me know if that sounds ok, I can provide a patch for same instead of
> reverting this which affects runtime PM with dwc3 host.
> Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to
> dr_mode as HOST.
> 
> 

Are you going to provide a patch for this any time soon?

FYI, suspend/resume is broken on DRA7x with Dual-role while in host mode.

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Manu Gautam]

Hi Roger,


On 1/15/2018 9:10 PM, Roger Quadros wrote:
> Hi Manu,
[snip]
>> I think it will be better to separate runtime_suspend and pm_suspend handling for
>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>> correct.
>> Let me know if that sounds ok, I can provide a patch for same instead of
>> reverting this which affects runtime PM with dwc3 host.
>> Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to
>> dr_mode as HOST.
>>
>>
> Are you going to provide a patch for this any time soon?
>
> FYI, suspend/resume is broken on DRA7x with Dual-role while in host mode.
>

Posted following patch: https://patchwork.kernel.org/patch/10166077/
Let me know if this works for you.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project