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@xxxxxxxxxxxxxx> 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