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
