Felipe,
On 30/05/16 14:34, Felipe Balbi wrote:
> now that we have re-factored dwc3_core_init() and
> dwc3_core_exit() we can use them for suspend/resume
> operations.
>
> This will help us avoid some common mistakes when
> patching code when we have duplicated pieces of code
> doing the same thing.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/dwc3/core.c | 59 +++++--------------------------------------------
> drivers/usb/dwc3/core.h | 3 ---
> 2 files changed, 5 insertions(+), 57 deletions(-)
This is the bad patch that breaks dwc3 on system suspend/resume.
Please see my comments below.
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index cbdefbb3d302..80e9affd3d77 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1113,33 +1113,19 @@ static int dwc3_remove(struct platform_device *pdev)
> static int dwc3_suspend(struct device *dev)
> {
> struct dwc3 *dwc = dev_get_drvdata(dev);
> - unsigned long flags;
> -
> - spin_lock_irqsave(&dwc->lock, flags);
Is it safe to call dwc3_gadget_suspend() or dwc3_core_exit without holding dwc->lock?
>
> switch (dwc->dr_mode) {
> case USB_DR_MODE_PERIPHERAL:
> case USB_DR_MODE_OTG:
> dwc3_gadget_suspend(dwc);
> - /* FALLTHROUGH */
> + break;
> case USB_DR_MODE_HOST:
> default:
> - dwc3_event_buffers_cleanup(dwc);
> + /* do nothing */
> break;
> }
>
> - dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL);
> - spin_unlock_irqrestore(&dwc->lock, flags);
> -
> - usb_phy_shutdown(dwc->usb3_phy);
> - usb_phy_shutdown(dwc->usb2_phy);
> - phy_exit(dwc->usb2_generic_phy);
> - phy_exit(dwc->usb3_generic_phy);
> -
> - usb_phy_set_suspend(dwc->usb2_phy, 1);
> - usb_phy_set_suspend(dwc->usb3_phy, 1);
> - WARN_ON(phy_power_off(dwc->usb2_generic_phy) < 0);
> - WARN_ON(phy_power_off(dwc->usb3_generic_phy) < 0);
> + dwc3_core_exit(dwc);
dwc3_core_exit() does a phy_power_off() as well which
changes the behaviour.
How can we power off the PHY if we want remote wakeup or
connect/disconnect to be detected? Also, this would break the USB link
to a suspended device.
>
> pinctrl_pm_select_sleep_state(dev);
>
> @@ -1149,36 +1135,14 @@ static int dwc3_suspend(struct device *dev)
> static int dwc3_resume(struct device *dev)
> {
> struct dwc3 *dwc = dev_get_drvdata(dev);
> - unsigned long flags;
> int ret;
>
> pinctrl_pm_select_default_state(dev);
>
> - usb_phy_set_suspend(dwc->usb2_phy, 0);
> - usb_phy_set_suspend(dwc->usb3_phy, 0);
> - ret = phy_power_on(dwc->usb2_generic_phy);
> - if (ret < 0)
> + ret = dwc3_core_init(dwc);
> + if (ret)
> return ret;
You've replaced just restoring gctl with a full core_init().
We will loose XHCI host controller state and the devices connected to it
will have to be re-enumerated.
So this cant be done IMO.
Why not just restore GCTL instead of doing a full core re-init?
>
> - ret = phy_power_on(dwc->usb3_generic_phy);
> - if (ret < 0)
> - goto err_usb2phy_power;
> -
> - usb_phy_init(dwc->usb3_phy);
> - usb_phy_init(dwc->usb2_phy);
> - ret = phy_init(dwc->usb2_generic_phy);
> - if (ret < 0)
> - goto err_usb3phy_power;
> -
> - ret = phy_init(dwc->usb3_generic_phy);
> - if (ret < 0)
> - goto err_usb2phy_init;
> -
> - spin_lock_irqsave(&dwc->lock, flags);
> -
> - dwc3_event_buffers_setup(dwc);
> - dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
> -
> switch (dwc->dr_mode) {
> case USB_DR_MODE_PERIPHERAL:
> case USB_DR_MODE_OTG:
> @@ -1190,24 +1154,11 @@ static int dwc3_resume(struct device *dev)
> break;
> }
>
> - spin_unlock_irqrestore(&dwc->lock, flags);
> -
> pm_runtime_disable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> return 0;
> -
> -err_usb2phy_init:
> - phy_exit(dwc->usb2_generic_phy);
> -
> -err_usb3phy_power:
> - phy_power_off(dwc->usb3_generic_phy);
> -
> -err_usb2phy_power:
> - phy_power_off(dwc->usb2_generic_phy);
> -
> - return ret;
> }
> #endif /* CONFIG_PM_SLEEP */
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index ce9596b1b1f4..58509cedb038 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -835,9 +835,6 @@ struct dwc3 {
>
> enum usb_dr_mode dr_mode;
>
> - /* used for suspend/resume */
> - u32 gctl;
> -
> u32 fladj;
> u32 nr_scratch;
> u32 u1u2;
>
--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
