Re: [PATCH] usb: phy: twl4030-usb: Fix regressions to runtime PM on omaps

[Felipe Balbi]

On Thu, Aug 21, 2014 at 09:43:46AM -0700, Tony Lindgren wrote:
> Commit 30a70b026b4cd ("usb: musb: fix obex in g_nokia.ko causing kernel
> panic") attempted to fix runtime PM handling for PHYs that are on the
> I2C bus. Commit 3063a12be2b0 (usb: musb: fix PHY power on/off) then
> changed things around to enable of PHYs that rely on runtime PM.
> 
> These changes however broke idling of the PHY and causes at least
> 100 mW extra power consumption on omaps, which is a lot with
> the idle power consumption being below 10 mW range on many devices.
> 
> As calling phy_power_on/off from runtime PM calls in the USB
> causes complicated issues with I2C connected PHYs, let's just let
> the PHY do it's own runtime PM as needed. This leaves out the
> dependency between PHYs and USB controller drivers for runtime
> PM.
> 
> Let's fix the regression for twl4030-usb by adding minimal runtime
> PM support. This allows idling the PHY on disconnect.
> 
> Note that we are changing to use standard runtime PM handling
> for twl4030_phy_init() as that function just checks the state
> and does not initialize the PHY. The PHY won't get initialized
> until in twl4030_phy_power_on().
> 
> Fixes: 30a70b026b4cd ("usb: musb: fix obex in g_nokia.ko causing kernel panic")
> Fixes: 3063a12be2b0 ("usb: musb: fix PHY power on/off")
> Cc: stable@xxxxxxxxxxxxxxx # v3.15+
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> 
> ---
> 
> Kishon, this regression fix would be nice to get into the v3.17-rc
> series if no objections. If you don't have other fixes, I can also
> queue via arm-soc with proper acks.
> 
> It probably does not make sense to try to fix this without using
> runtime PM without complicating the code further.
> 
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -34,6 +34,7 @@
>  #include <linux/delay.h>
>  #include <linux/usb/otg.h>
>  #include <linux/phy/phy.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/usb/musb-omap.h>
>  #include <linux/usb/ulpi.h>
>  #include <linux/i2c/twl.h>
> @@ -422,37 +423,55 @@ static void twl4030_phy_power(struct twl4030_usb *twl, int on)
>  	}
>  }
>  
> -static int twl4030_phy_power_off(struct phy *phy)
> +static int twl4030_usb_runtime_suspend(struct device *dev)
>  {
> -	struct twl4030_usb *twl = phy_get_drvdata(phy);
> +	struct twl4030_usb *twl = dev_get_drvdata(dev);
>  
> +	dev_dbg(twl->dev, "%s\n", __func__);
>  	if (twl->asleep)
>  		return 0;
>  
>  	twl4030_phy_power(twl, 0);
>  	twl->asleep = 1;
> -	dev_dbg(twl->dev, "%s\n", __func__);
> +
>  	return 0;
>  }
>  
> -static void __twl4030_phy_power_on(struct twl4030_usb *twl)
> +static int twl4030_usb_runtime_resume(struct device *dev)
>  {
> +	struct twl4030_usb *twl = dev_get_drvdata(dev);
> +
> +	dev_dbg(twl->dev, "%s\n", __func__);
> +	if (!twl->asleep)
> +		return 0;
> +
>  	twl4030_phy_power(twl, 1);
> -	twl4030_i2c_access(twl, 1);
> -	twl4030_usb_set_mode(twl, twl->usb_mode);
> -	if (twl->usb_mode == T2_USB_MODE_ULPI)
> -		twl4030_i2c_access(twl, 0);
> +	twl->asleep = 0;
> +
> +	return 0;
> +}
> +
> +static int twl4030_phy_power_off(struct phy *phy)
> +{
> +	struct twl4030_usb *twl = phy_get_drvdata(phy);
> +
> +	dev_dbg(twl->dev, "%s\n", __func__);
> +	pm_runtime_mark_last_busy(twl->dev);
> +	pm_runtime_put_autosuspend(twl->dev);
> +
> +	return 0;
>  }
>  
>  static int twl4030_phy_power_on(struct phy *phy)
>  {
>  	struct twl4030_usb *twl = phy_get_drvdata(phy);
>  
> -	if (!twl->asleep)
> -		return 0;
> -	__twl4030_phy_power_on(twl);
> -	twl->asleep = 0;
>  	dev_dbg(twl->dev, "%s\n", __func__);
> +	pm_runtime_get_sync(twl->dev);
> +	twl4030_i2c_access(twl, 1);
> +	twl4030_usb_set_mode(twl, twl->usb_mode);
> +	if (twl->usb_mode == T2_USB_MODE_ULPI)
> +		twl4030_i2c_access(twl, 0);
>  
>  	/*
>  	 * XXX When VBUS gets driven after musb goes to A mode,
> @@ -558,6 +577,16 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>  		 * USB_LINK_VBUS state.  musb_hdrc won't care until it
>  		 * starts to handle softconnect right.
>  		 */
> +		if ((status == OMAP_MUSB_VBUS_VALID) ||
> +		    (status == OMAP_MUSB_ID_GROUND)) {
> +			if (twl->asleep)
> +				pm_runtime_get_sync(twl->dev);
> +		} else {
> +			if (!twl->asleep) {
> +				pm_runtime_mark_last_busy(twl->dev);
> +				pm_runtime_put_autosuspend(twl->dev);
> +			}
> +		}
>  		omap_musb_mailbox(status);
>  	}
>  	sysfs_notify(&twl->dev->kobj, NULL, "vbus");
> @@ -599,22 +628,17 @@ static int twl4030_phy_init(struct phy *phy)
>  	struct twl4030_usb *twl = phy_get_drvdata(phy);
>  	enum omap_musb_vbus_id_status status;
>  
> -	/*
> -	 * Start in sleep state, we'll get called through set_suspend()
> -	 * callback when musb is runtime resumed and it's time to start.
> -	 */
> -	__twl4030_phy_power(twl, 0);
> -	twl->asleep = 1;
> -
> +	pm_runtime_get_sync(twl->dev);
>  	status = twl4030_usb_linkstat(twl);
>  	twl->linkstat = status;
>  
> -	if (status == OMAP_MUSB_ID_GROUND || status == OMAP_MUSB_VBUS_VALID) {
> +	if (status == OMAP_MUSB_ID_GROUND || status == OMAP_MUSB_VBUS_VALID)
>  		omap_musb_mailbox(twl->linkstat);
> -		twl4030_phy_power_on(phy);
> -	}
>  
>  	sysfs_notify(&twl->dev->kobj, NULL, "vbus");
> +	pm_runtime_mark_last_busy(twl->dev);
> +	pm_runtime_put_autosuspend(twl->dev);
> +
>  	return 0;
>  }
>  
> @@ -650,6 +674,11 @@ static const struct phy_ops ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +static const struct dev_pm_ops twl4030_usb_pm_ops = {
> +	SET_RUNTIME_PM_OPS(twl4030_usb_runtime_suspend,
> +			   twl4030_usb_runtime_resume, NULL)
> +};
> +
>  static int twl4030_usb_probe(struct platform_device *pdev)
>  {
>  	struct twl4030_usb_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -726,6 +755,11 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>  
>  	ATOMIC_INIT_NOTIFIER_HEAD(&twl->phy.notifier);
>  
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	/* Our job is to use irqs and status from the power module
>  	 * to keep the transceiver disabled when nothing's connected.
>  	 *
> @@ -744,6 +778,9 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>  		return status;
>  	}
>  
> +	pm_runtime_mark_last_busy(&pdev->dev);
> +	pm_runtime_put(&pdev->dev);

don't you wanna do a put_autosuspend() here instead ?

Other than that

Acked-by: Felipe Balbi <balbi@xxxxxx>


-- 
balbi

Attachment: signature.asc
Description: Digital signature