Re: [PATCH V2 2/6] usb: core: Add ability to skip phy exit on suspend and init on resume

[Chunfeng Yun]

hi,

On Wed, 2018-10-17 at 18:29 -0400, Al Cooper wrote:
> Add the ability to skip calling the PHY's exit routine on suspend
> and the PHY's init routine on resume. This is to handle a USB PHY
> that should have it's power_off function called on suspend but cannot
> have it's exit function called because on exit it will disable the
> PHY to the point where register accesses to the Host Controllers
> using the PHY will be disabled and the host drivers will crash.
> 
> This is enabled with the HCD flag "suspend_without_phy_exit" which
> can be set from any HCD driver.
> 
> Signed-off-by: Al Cooper <alcooperx@xxxxxxxxx>
> ---
>  drivers/usb/core/hcd.c  |  8 ++++----
>  drivers/usb/core/phy.c  | 18 ++++++++++++------
>  drivers/usb/core/phy.h  |  9 ++++++---
>  include/linux/usb/hcd.h |  3 +++
>  4 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 1c21955fe7c0..e67e4d6b3d21 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2263,7 +2263,7 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		hcd->state = HC_STATE_SUSPENDED;
>  
>  		if (!PMSG_IS_AUTO(msg))
> -			usb_phy_roothub_suspend(hcd->self.sysdev,
> +			usb_phy_roothub_suspend(hcd,
>  						hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
> @@ -2304,7 +2304,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_resume(hcd->self.sysdev,
> +		status = usb_phy_roothub_resume(hcd,
>  						hcd->phy_roothub);
>  		if (status)
>  			return status;
> @@ -2347,7 +2347,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> @@ -2744,7 +2744,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	struct usb_device *rhdev;
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd);
>  		if (IS_ERR(hcd->phy_roothub))
>  			return PTR_ERR(hcd->phy_roothub);
>  
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 9879767452a2..b54db39eee4c 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -45,10 +45,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd)
>  {
>  	struct usb_phy_roothub *phy_roothub;
>  	int i, num_phys, err;
> +	struct device *dev = hcd->self.sysdev;
>  
>  	if (!IS_ENABLED(CONFIG_GENERIC_PHY))
>  		return NULL;
> @@ -161,26 +162,30 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
>  
> -int usb_phy_roothub_suspend(struct device *controller_dev,
> +int usb_phy_roothub_suspend(struct usb_hcd *hcd,
>  			    struct usb_phy_roothub *phy_roothub)
>  {
> +	struct device *controller_dev = hcd->self.sysdev;
> +
>  	usb_phy_roothub_power_off(phy_roothub);
>  
>  	/* keep the PHYs initialized so the device can wake up the system */
> -	if (device_may_wakeup(controller_dev))
> +	if (device_may_wakeup(controller_dev) || hcd->suspend_without_phy_exit)
>  		return 0;
>  
>  	return usb_phy_roothub_exit(phy_roothub);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
>  
> -int usb_phy_roothub_resume(struct device *controller_dev,
> +int usb_phy_roothub_resume(struct usb_hcd *hcd,
>  			   struct usb_phy_roothub *phy_roothub)
>  {
> +	struct device *controller_dev = hcd->self.sysdev;
>  	int err;
>  
>  	/* if the device can't wake up the system _exit was called */
> -	if (!device_may_wakeup(controller_dev)) {
> +	if (!device_may_wakeup(controller_dev) &&
> +	    !hcd->suspend_without_phy_exit) {
>  		err = usb_phy_roothub_init(phy_roothub);
>  		if (err)
>  			return err;
> @@ -189,7 +194,8 @@ int usb_phy_roothub_resume(struct device *controller_dev,
>  	err = usb_phy_roothub_power_on(phy_roothub);
>  
>  	/* undo _init if _power_on failed */
> -	if (err && !device_may_wakeup(controller_dev))
> +	if (err && !device_may_wakeup(controller_dev)
> +	    && !hcd->suspend_without_phy_exit)
>  		usb_phy_roothub_exit(phy_roothub);
>  
>  	return err;
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 88a3c037e9df..34293e11a917 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -5,13 +5,16 @@
>   * Copyright (C) 2018 Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
>   */
>  
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
>  #ifndef __USB_CORE_PHY_H_
>  #define __USB_CORE_PHY_H_
>  
>  struct device;
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd);
>  
>  int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
> @@ -19,9 +22,9 @@
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
>  
> -int usb_phy_roothub_suspend(struct device *controller_dev,
> +int usb_phy_roothub_suspend(struct usb_hcd *hcd,
>  			    struct usb_phy_roothub *phy_roothub);
> -int usb_phy_roothub_resume(struct device *controller_dev,
> +int usb_phy_roothub_resume(struct usb_hcd *hcd,
>  			   struct usb_phy_roothub *phy_roothub);
>  
>  #endif /* __USB_CORE_PHY_H_ */
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 97e2ddec18b1..87a104055b5e 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -157,6 +157,9 @@ struct usb_hcd {
>  	 */
>  	unsigned		skip_phy_initialization:1;
>  
> +	/* Some phys don't want the phy's exit/init called on suspend/resume */
> +	unsigned		suspend_without_phy_exit:1;
As suggested before, you can skip phy's exit/init during suspend/resume
by enabling wakeup of hcd, so needn't add a new variable for it.

> +
>  	/* The next flag is a stopgap, to be removed when all the HCDs
>  	 * support the new root-hub polling mechanism. */
>  	unsigned		uses_new_polling:1;