[PATCH 0/5] Clean-up for twl4030-usb

[PATCH 0/5] Clean-up for twl4030-usb

[Tony Lindgren]

Hi,

Here are few more patches for v3.18 to clean up twl4030-usb.

These are based on the two regression fixes I sent earlier:

[PATCH] usb: phy: twl4030-usb: Fix regressions to runtime PM on omaps
[PATCH] usb: phy: twl4030-usb: Fix lost interrupts after ID pin goes down

For testing with v3.17-rc2, you probably also want to revert
commit a9232076374334ca2bc2a448dfde96d38a54349a until that
hits the mainline tree. And you may also want to apply the
following patch if testing with PM:

[PATCH] mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

I've also pushed these patches with the optional patches
into a temporary testing branch at [1].

Regards,

Tony

[1] git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git omap-for-v3.17/testing-pending-usb

Tony Lindgren (5):
  usb: phy: twl4030-usb: Remove unused irq_enabled
  usb: phy: twl4030-usb: Simplify phy init to use runtime PM
  usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime
    PM calls
  usb: phy: twl4030-usb: Remove asleep and rely on runtime PM
  usb: phy: twl4030-usb: Use mutex instead of spinlock for protecting
    the data

 drivers/phy/phy-twl4030-usb.c     | 124 ++++++++++++++------------------------
 drivers/usb/phy/phy-twl6030-usb.c |   2 -
 2 files changed, 46 insertions(+), 80 deletions(-)

-- 
1.8.1.1

[PATCH 1/5] usb: phy: twl4030-usb: Remove unused irq_enabled

[Tony Lindgren]

It's not being used any longer.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/phy-twl4030-usb.c     | 2 --
 drivers/usb/phy/phy-twl6030-usb.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 9cd33a4..bc28ecc 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -164,7 +164,6 @@ struct twl4030_usb {
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
 	u8			asleep;
-	bool			irq_enabled;
 
 	struct delayed_work	id_workaround_work;
 };
@@ -755,7 +754,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	 * set_host() and/or set_peripheral() ... OTG_capable boards
 	 * need both handles, otherwise just one suffices.
 	 */
-	twl->irq_enabled = true;
 	status = devm_request_threaded_irq(twl->dev, twl->irq, NULL,
 			twl4030_usb_irq, IRQF_TRIGGER_FALLING |
 			IRQF_TRIGGER_RISING | IRQF_ONESHOT, "twl4030_usb", twl);
diff --git a/drivers/usb/phy/phy-twl6030-usb.c b/drivers/usb/phy/phy-twl6030-usb.c
index 04778cf..44ea082 100644
--- a/drivers/usb/phy/phy-twl6030-usb.c
+++ b/drivers/usb/phy/phy-twl6030-usb.c
@@ -104,7 +104,6 @@ struct twl6030_usb {
 	int			irq2;
 	enum omap_musb_vbus_id_status linkstat;
 	u8			asleep;
-	bool			irq_enabled;
 	bool			vbus_enable;
 	const char		*regulator;
 };
@@ -373,7 +372,6 @@ static int twl6030_usb_probe(struct platform_device *pdev)
 
 	INIT_WORK(&twl->set_vbus_work, otg_set_vbus_work);
 
-	twl->irq_enabled = true;
 	status = request_threaded_irq(twl->irq1, NULL, twl6030_usbotg_irq,
 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT,
 			"twl6030_usb", twl);
-- 
1.8.1.1

Re: [PATCH 1/5] usb: phy: twl4030-usb: Remove unused irq_enabled

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

Hi,

On Wed, Aug 27, 2014 at 04:28:07PM -0700, Tony Lindgren wrote:
> It's not being used any longer.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/phy/phy-twl4030-usb.c     | 2 --
>  drivers/usb/phy/phy-twl6030-usb.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 9cd33a4..bc28ecc 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -164,7 +164,6 @@ struct twl4030_usb {
>  	enum omap_musb_vbus_id_status linkstat;
>  	bool			vbus_supplied;
>  	u8			asleep;
> -	bool			irq_enabled;
>  
>  	struct delayed_work	id_workaround_work;
>  };
> @@ -755,7 +754,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>  	 * set_host() and/or set_peripheral() ... OTG_capable boards
>  	 * need both handles, otherwise just one suffices.
>  	 */
> -	twl->irq_enabled = true;
>  	status = devm_request_threaded_irq(twl->dev, twl->irq, NULL,
>  			twl4030_usb_irq, IRQF_TRIGGER_FALLING |
>  			IRQF_TRIGGER_RISING | IRQF_ONESHOT, "twl4030_usb", twl);

can you split this into two patches ? drivers/phy will be taken by
Kishon and drivers/usb/phy by me. another possibility is that I get an
Acked-by from Kishon and I can take $subject as is.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] usb: phy: twl4030-usb: Remove unused irq_enabled

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

Hi,

On Wed, Aug 27, 2014 at 04:28:07PM -0700, Tony Lindgren wrote:
> It's not being used any longer.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/phy/phy-twl4030-usb.c     | 2 --
>  drivers/usb/phy/phy-twl6030-usb.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 9cd33a4..bc28ecc 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -164,7 +164,6 @@ struct twl4030_usb {
>  	enum omap_musb_vbus_id_status linkstat;
>  	bool			vbus_supplied;
>  	u8			asleep;
> -	bool			irq_enabled;
>  
>  	struct delayed_work	id_workaround_work;
>  };
> @@ -755,7 +754,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>  	 * set_host() and/or set_peripheral() ... OTG_capable boards
>  	 * need both handles, otherwise just one suffices.
>  	 */
> -	twl->irq_enabled = true;
>  	status = devm_request_threaded_irq(twl->dev, twl->irq, NULL,
>  			twl4030_usb_irq, IRQF_TRIGGER_FALLING |
>  			IRQF_TRIGGER_RISING | IRQF_ONESHOT, "twl4030_usb", twl);

can you split this into two patches ? drivers/phy will be taken by
Kishon and drivers/usb/phy by me. another possibility is that I get an
Acked-by from Kishon and I can take $subject as is.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] usb: phy: twl4030-usb: Remove unused irq_enabled

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [140827 19:21]:
> Hi,
> 
> On Wed, Aug 27, 2014 at 04:28:07PM -0700, Tony Lindgren wrote:
> > It's not being used any longer.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/phy/phy-twl4030-usb.c     | 2 --
> >  drivers/usb/phy/phy-twl6030-usb.c | 2 --
> >  2 files changed, 4 deletions(-)
> > 
> > diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> > index 9cd33a4..bc28ecc 100644
> > --- a/drivers/phy/phy-twl4030-usb.c
> > +++ b/drivers/phy/phy-twl4030-usb.c
> > @@ -164,7 +164,6 @@ struct twl4030_usb {
> >  	enum omap_musb_vbus_id_status linkstat;
> >  	bool			vbus_supplied;
> >  	u8			asleep;
> > -	bool			irq_enabled;
> >  
> >  	struct delayed_work	id_workaround_work;
> >  };
> > @@ -755,7 +754,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
> >  	 * set_host() and/or set_peripheral() ... OTG_capable boards
> >  	 * need both handles, otherwise just one suffices.
> >  	 */
> > -	twl->irq_enabled = true;
> >  	status = devm_request_threaded_irq(twl->dev, twl->irq, NULL,
> >  			twl4030_usb_irq, IRQF_TRIGGER_FALLING |
> >  			IRQF_TRIGGER_RISING | IRQF_ONESHOT, "twl4030_usb", twl);
> 
> can you split this into two patches ? drivers/phy will be taken by
> Kishon and drivers/usb/phy by me. another possibility is that I get an
> Acked-by from Kishon and I can take $subject as is.

Oops yes sorry I already forgot I'm patching twl6030 too. Below
is the updated version of this patch for Kishon with just the
twl4030 changes. I will post the twl6030 changes separately.

Regards,

Tony

8< ---------------------
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 18 Aug 2014 07:54:16 -0700
Subject: [PATCH] usb: phy: twl4030-usb: Remove unused irq_enabled

It's not being used any longer.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -164,7 +164,6 @@ struct twl4030_usb {
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
 	u8			asleep;
-	bool			irq_enabled;
 
 	struct delayed_work	id_workaround_work;
 };
@@ -755,7 +754,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	 * set_host() and/or set_peripheral() ... OTG_capable boards
 	 * need both handles, otherwise just one suffices.
 	 */
-	twl->irq_enabled = true;
 	status = devm_request_threaded_irq(twl->dev, twl->irq, NULL,
 			twl4030_usb_irq, IRQF_TRIGGER_FALLING |
 			IRQF_TRIGGER_RISING | IRQF_ONESHOT, "twl4030_usb", twl);

Re: [PATCH 1/5] usb: phy: twl4030-usb: Remove unused irq_enabled

[Tony Lindgren]

* Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> [140827 19:21]:
> Hi,
> 
> On Wed, Aug 27, 2014 at 04:28:07PM -0700, Tony Lindgren wrote:
> > It's not being used any longer.
> > 
> > Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  drivers/phy/phy-twl4030-usb.c     | 2 --
> >  drivers/usb/phy/phy-twl6030-usb.c | 2 --
> >  2 files changed, 4 deletions(-)
> > 
> > diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> > index 9cd33a4..bc28ecc 100644
> > --- a/drivers/phy/phy-twl4030-usb.c
> > +++ b/drivers/phy/phy-twl4030-usb.c
> > @@ -164,7 +164,6 @@ struct twl4030_usb {
> >  	enum omap_musb_vbus_id_status linkstat;
> >  	bool			vbus_supplied;
> >  	u8			asleep;
> > -	bool			irq_enabled;
> >  
> >  	struct delayed_work	id_workaround_work;
> >  };
> > @@ -755,7 +754,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
> >  	 * set_host() and/or set_peripheral() ... OTG_capable boards
> >  	 * need both handles, otherwise just one suffices.
> >  	 */
> > -	twl->irq_enabled = true;
> >  	status = devm_request_threaded_irq(twl->dev, twl->irq, NULL,
> >  			twl4030_usb_irq, IRQF_TRIGGER_FALLING |
> >  			IRQF_TRIGGER_RISING | IRQF_ONESHOT, "twl4030_usb", twl);
> 
> can you split this into two patches ? drivers/phy will be taken by
> Kishon and drivers/usb/phy by me. another possibility is that I get an
> Acked-by from Kishon and I can take $subject as is.

Oops yes sorry I already forgot I'm patching twl6030 too. Below
is the updated version of this patch for Kishon with just the
twl4030 changes. I will post the twl6030 changes separately.

Regards,

Tony

8< ---------------------
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Mon, 18 Aug 2014 07:54:16 -0700
Subject: [PATCH] usb: phy: twl4030-usb: Remove unused irq_enabled

It's not being used any longer.

Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -164,7 +164,6 @@ struct twl4030_usb {
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
 	u8			asleep;
-	bool			irq_enabled;
 
 	struct delayed_work	id_workaround_work;
 };
@@ -755,7 +754,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	 * set_host() and/or set_peripheral() ... OTG_capable boards
 	 * need both handles, otherwise just one suffices.
 	 */
-	twl->irq_enabled = true;
 	status = devm_request_threaded_irq(twl->dev, twl->irq, NULL,
 			twl4030_usb_irq, IRQF_TRIGGER_FALLING |
 			IRQF_TRIGGER_RISING | IRQF_ONESHOT, "twl4030_usb", twl);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] usb: phy: twl4030-usb: Simplify phy init to use runtime PM

[Tony Lindgren]

We can now let the interrupt and delayed work do all that's
needed with runtime PM.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/phy-twl4030-usb.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index bc28ecc..a292db0 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -471,16 +471,8 @@ static int twl4030_phy_power_on(struct phy *phy)
 	twl4030_usb_set_mode(twl, twl->usb_mode);
 	if (twl->usb_mode == T2_USB_MODE_ULPI)
 		twl4030_i2c_access(twl, 0);
+	schedule_delayed_work(&twl->id_workaround_work, 0);
 
-	/*
-	 * XXX When VBUS gets driven after musb goes to A mode,
-	 * ID_PRES related interrupts no longer arrive, why?
-	 * Register itself is updated fine though, so we must poll.
-	 */
-	if (twl->linkstat == OMAP_MUSB_ID_GROUND) {
-		cancel_delayed_work(&twl->id_workaround_work);
-		schedule_delayed_work(&twl->id_workaround_work, HZ);
-	}
 	return 0;
 }
 
@@ -612,16 +604,9 @@ static void twl4030_id_workaround_work(struct work_struct *work)
 static int twl4030_phy_init(struct phy *phy)
 {
 	struct twl4030_usb *twl = phy_get_drvdata(phy);
-	enum omap_musb_vbus_id_status status;
 
 	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)
-		omap_musb_mailbox(twl->linkstat);
-
-	sysfs_notify(&twl->dev->kobj, NULL, "vbus");
+	schedule_delayed_work(&twl->id_workaround_work, 0);
 	pm_runtime_mark_last_busy(twl->dev);
 	pm_runtime_put_autosuspend(twl->dev);
 
@@ -698,6 +683,7 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	twl->dev		= &pdev->dev;
 	twl->irq		= platform_get_irq(pdev, 0);
 	twl->vbus_supplied	= false;
+	twl->linkstat		= -EINVAL;
 	twl->asleep		= 1;
 	twl->linkstat		= OMAP_MUSB_UNKNOWN;
 
-- 
1.8.1.1

[PATCH 3/5] usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime PM calls

[Tony Lindgren]

We don't need twl4030_phy_power() any longer now that we have
the runtime PM calls. Let's get rid of it as it's confusing.
No functional changes, just move the code and use res instead
of ret as we are not returning that value.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/phy-twl4030-usb.c | 72 +++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 41 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index a292db0..519cc90 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -383,45 +383,6 @@ static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
 	WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
 }
 
-static void twl4030_phy_power(struct twl4030_usb *twl, int on)
-{
-	int ret;
-
-	if (on) {
-		ret = regulator_enable(twl->usb3v1);
-		if (ret)
-			dev_err(twl->dev, "Failed to enable usb3v1\n");
-
-		ret = regulator_enable(twl->usb1v8);
-		if (ret)
-			dev_err(twl->dev, "Failed to enable usb1v8\n");
-
-		/*
-		 * Disabling usb3v1 regulator (= writing 0 to VUSB3V1_DEV_GRP
-		 * in twl4030) resets the VUSB_DEDICATED2 register. This reset
-		 * enables VUSB3V1_SLEEP bit that remaps usb3v1 ACTIVE state to
-		 * SLEEP. We work around this by clearing the bit after usv3v1
-		 * is re-activated. This ensures that VUSB3V1 is really active.
-		 */
-		twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0, VUSB_DEDICATED2);
-
-		ret = regulator_enable(twl->usb1v5);
-		if (ret)
-			dev_err(twl->dev, "Failed to enable usb1v5\n");
-
-		__twl4030_phy_power(twl, 1);
-		twl4030_usb_write(twl, PHY_CLK_CTRL,
-				  twl4030_usb_read(twl, PHY_CLK_CTRL) |
-					(PHY_CLK_CTRL_CLOCKGATING_EN |
-						PHY_CLK_CTRL_CLK32K_EN));
-	} else {
-		__twl4030_phy_power(twl, 0);
-		regulator_disable(twl->usb1v5);
-		regulator_disable(twl->usb1v8);
-		regulator_disable(twl->usb3v1);
-	}
-}
-
 static int twl4030_usb_runtime_suspend(struct device *dev)
 {
 	struct twl4030_usb *twl = dev_get_drvdata(dev);
@@ -430,7 +391,10 @@ static int twl4030_usb_runtime_suspend(struct device *dev)
 	if (twl->asleep)
 		return 0;
 
-	twl4030_phy_power(twl, 0);
+	__twl4030_phy_power(twl, 0);
+	regulator_disable(twl->usb1v5);
+	regulator_disable(twl->usb1v8);
+	regulator_disable(twl->usb3v1);
 	twl->asleep = 1;
 
 	return 0;
@@ -439,12 +403,38 @@ static int twl4030_usb_runtime_suspend(struct device *dev)
 static int twl4030_usb_runtime_resume(struct device *dev)
 {
 	struct twl4030_usb *twl = dev_get_drvdata(dev);
+	int res;
 
 	dev_dbg(twl->dev, "%s\n", __func__);
 	if (!twl->asleep)
 		return 0;
 
-	twl4030_phy_power(twl, 1);
+	res = regulator_enable(twl->usb3v1);
+	if (res)
+		dev_err(twl->dev, "Failed to enable usb3v1\n");
+
+	res = regulator_enable(twl->usb1v8);
+	if (res)
+		dev_err(twl->dev, "Failed to enable usb1v8\n");
+
+	/*
+	 * Disabling usb3v1 regulator (= writing 0 to VUSB3V1_DEV_GRP
+	 * in twl4030) resets the VUSB_DEDICATED2 register. This reset
+	 * enables VUSB3V1_SLEEP bit that remaps usb3v1 ACTIVE state to
+	 * SLEEP. We work around this by clearing the bit after usv3v1
+	 * is re-activated. This ensures that VUSB3V1 is really active.
+	 */
+	twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0, VUSB_DEDICATED2);
+
+	res = regulator_enable(twl->usb1v5);
+	if (res)
+		dev_err(twl->dev, "Failed to enable usb1v5\n");
+
+	__twl4030_phy_power(twl, 1);
+	twl4030_usb_write(twl, PHY_CLK_CTRL,
+			  twl4030_usb_read(twl, PHY_CLK_CTRL) |
+			  (PHY_CLK_CTRL_CLOCKGATING_EN |
+			   PHY_CLK_CTRL_CLK32K_EN));
 	twl->asleep = 0;
 
 	return 0;
-- 
1.8.1.1

Re: [PATCH 3/5] usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime PM calls

[Kishon Vijay Abraham I]

Hi Tony,

On Thursday 28 August 2014 04:58 AM, Tony Lindgren wrote:
> We don't need twl4030_phy_power() any longer now that we have
> the runtime PM calls. Let's get rid of it as it's confusing.
> No functional changes, just move the code and use res instead
> of ret as we are not returning that value.

Now that you are doing pm_runtime_get_sync in twl4030_phy_init, won't it power
on the phy even before initializing it (since runtime_resume will be invoked
even before doing phy_init)?

Even if pm_runtime_get_sync in not done in twl4030_phy_init, phy-core itself
does pm_runtime_get_sync in phy_init().

Thanks
Kishon

Re: [PATCH 3/5] usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime PM calls

[Kishon Vijay Abraham I]

Hi Tony,

On Thursday 28 August 2014 04:58 AM, Tony Lindgren wrote:
> We don't need twl4030_phy_power() any longer now that we have
> the runtime PM calls. Let's get rid of it as it's confusing.
> No functional changes, just move the code and use res instead
> of ret as we are not returning that value.

Now that you are doing pm_runtime_get_sync in twl4030_phy_init, won't it power
on the phy even before initializing it (since runtime_resume will be invoked
even before doing phy_init)?

Even if pm_runtime_get_sync in not done in twl4030_phy_init, phy-core itself
does pm_runtime_get_sync in phy_init().

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime PM calls

[Tony Lindgren]

* Kishon Vijay Abraham I <kishon@ti.com> [140904 06:51]:
> Hi Tony,
> 
> On Thursday 28 August 2014 04:58 AM, Tony Lindgren wrote:
> > We don't need twl4030_phy_power() any longer now that we have
> > the runtime PM calls. Let's get rid of it as it's confusing.
> > No functional changes, just move the code and use res instead
> > of ret as we are not returning that value.
> 
> Now that you are doing pm_runtime_get_sync in twl4030_phy_init, won't it power
> on the phy even before initializing it (since runtime_resume will be invoked
> even before doing phy_init)?

Yes. The logic being that it should not matter as we are not
configuring the phy until in twl4030_phy_power_on(). Maybe we
should add code to make sure the phy is deconfigured initially
though :)

In twl4030_phy_init() we just want to check the ID pin state to get
things right initially. In the twl4030-usb case the I2C chip is
always on, but let's try to get the runtime PM set up like any
standard Linux driver would do it to avoid confusion.

> Even if pm_runtime_get_sync in not done in twl4030_phy_init, phy-core itself
> does pm_runtime_get_sync in phy_init().

Hmm OK. Looking at that, looks like we don't neeed any of these
custom exports though:

$ git grep phy_pm_runtime | grep EXPORT_SYMBOL
drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get);
drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);

The reasons why I think we don't need the above at all is:

1. We already have a framework for that in Linux runtime PM and we
   can follow the standard Linux runtime PM calls and not proxy
   them in phy-core.c

2. We can allow idling the phy properly on the bus it's connected
   to, in this case I2C, even if USB driver is not loaded. We
   eventually should idle the phy even if usb_add_phy_dev()
   failed

3. There's no actual need for phy-core.c to proxy the runtime
   PM calls

So we can and should just let the phy drivers do their own runtime PM
as needed based on just the phy init, power_on and power_off calls.

Probably the same goes for the regulator_enable in phy-core.c. That
should be handled in the phy driver as phy-core is already unable
to handle it properly. For example, for phy-twl4030-usb.c we have
three regulators to deal with and the phy framework won't have any
idea how to deal with those.

And the phy-core won't be able to deal with the phy driver interrupts
anyways, so any attempt of doing finer grained runtime PM in phy-core
or handling of separate wake-up interrupts will be doomed to fail.

I think the changes above would simplify the phy handling quite
a bit, what do you think?

Regards,

Tony

Re: [PATCH 3/5] usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime PM calls

[Kishon Vijay Abraham I]

Hi Tony,

On Thursday 04 September 2014 10:37 PM, Tony Lindgren wrote:
> * Kishon Vijay Abraham I <kishon@ti.com> [140904 06:51]:
>> Hi Tony,
>>
>> On Thursday 28 August 2014 04:58 AM, Tony Lindgren wrote:
>>> We don't need twl4030_phy_power() any longer now that we have
>>> the runtime PM calls. Let's get rid of it as it's confusing.
>>> No functional changes, just move the code and use res instead
>>> of ret as we are not returning that value.
>>
>> Now that you are doing pm_runtime_get_sync in twl4030_phy_init, won't it power
>> on the phy even before initializing it (since runtime_resume will be invoked
>> even before doing phy_init)?
> 
> Yes. The logic being that it should not matter as we are not
> configuring the phy until in twl4030_phy_power_on(). Maybe we
> should add code to make sure the phy is deconfigured initially
> though :)
> 
> In twl4030_phy_init() we just want to check the ID pin state to get
> things right initially. In the twl4030-usb case the I2C chip is
> always on, but let's try to get the runtime PM set up like any
> standard Linux driver would do it to avoid confusion.
> 
>> Even if pm_runtime_get_sync in not done in twl4030_phy_init, phy-core itself
>> does pm_runtime_get_sync in phy_init().
> 
> Hmm OK. Looking at that, looks like we don't neeed any of these
> custom exports though:
> 
> $ git grep phy_pm_runtime | grep EXPORT_SYMBOL
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
> 
> The reasons why I think we don't need the above at all is:
> 
> 1. We already have a framework for that in Linux runtime PM and we
>    can follow the standard Linux runtime PM calls and not proxy
>    them in phy-core.c

The reason for adding these are for providing fine grained control of the PHY
by the controller drivers. In most cases the controller driver determines when
the PHY should be active or idle.
> 
> 2. We can allow idling the phy properly on the bus it's connected
>    to, in this case I2C, even if USB driver is not loaded. We
>    eventually should idle the phy even if usb_add_phy_dev()
>    failed

yes.. that's why we have ops like phy_power_on to tell when the PHY should be
active. So these PHYs can be idled in probe.
> 
> 3. There's no actual need for phy-core.c to proxy the runtime
>    PM calls
> 
> So we can and should just let the phy drivers do their own runtime PM
> as needed based on just the phy init, power_on and power_off calls.
> 
> Probably the same goes for the regulator_enable in phy-core.c. That
> should be handled in the phy driver as phy-core is already unable
> to handle it properly. For example, for phy-twl4030-usb.c we have
> three regulators to deal with and the phy framework won't have any
> idea how to deal with those.

hmm.. It was modelled for basic PHY drivers that have a single regulator (e.g.,
TI PIPE3 PHY). The idea is not to duplicate getting and enabling regulator in
each of the PHY drivers when it can be abstracted in phy-core.

Thanks
Kishon

Re: [PATCH 3/5] usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime PM calls

[Kishon Vijay Abraham I]

Hi Tony,

On Thursday 04 September 2014 10:37 PM, Tony Lindgren wrote:
> * Kishon Vijay Abraham I <kishon@ti.com> [140904 06:51]:
>> Hi Tony,
>>
>> On Thursday 28 August 2014 04:58 AM, Tony Lindgren wrote:
>>> We don't need twl4030_phy_power() any longer now that we have
>>> the runtime PM calls. Let's get rid of it as it's confusing.
>>> No functional changes, just move the code and use res instead
>>> of ret as we are not returning that value.
>>
>> Now that you are doing pm_runtime_get_sync in twl4030_phy_init, won't it power
>> on the phy even before initializing it (since runtime_resume will be invoked
>> even before doing phy_init)?
> 
> Yes. The logic being that it should not matter as we are not
> configuring the phy until in twl4030_phy_power_on(). Maybe we
> should add code to make sure the phy is deconfigured initially
> though :)
> 
> In twl4030_phy_init() we just want to check the ID pin state to get
> things right initially. In the twl4030-usb case the I2C chip is
> always on, but let's try to get the runtime PM set up like any
> standard Linux driver would do it to avoid confusion.
> 
>> Even if pm_runtime_get_sync in not done in twl4030_phy_init, phy-core itself
>> does pm_runtime_get_sync in phy_init().
> 
> Hmm OK. Looking at that, looks like we don't neeed any of these
> custom exports though:
> 
> $ git grep phy_pm_runtime | grep EXPORT_SYMBOL
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
> drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
> 
> The reasons why I think we don't need the above at all is:
> 
> 1. We already have a framework for that in Linux runtime PM and we
>    can follow the standard Linux runtime PM calls and not proxy
>    them in phy-core.c

The reason for adding these are for providing fine grained control of the PHY
by the controller drivers. In most cases the controller driver determines when
the PHY should be active or idle.
> 
> 2. We can allow idling the phy properly on the bus it's connected
>    to, in this case I2C, even if USB driver is not loaded. We
>    eventually should idle the phy even if usb_add_phy_dev()
>    failed

yes.. that's why we have ops like phy_power_on to tell when the PHY should be
active. So these PHYs can be idled in probe.
> 
> 3. There's no actual need for phy-core.c to proxy the runtime
>    PM calls
> 
> So we can and should just let the phy drivers do their own runtime PM
> as needed based on just the phy init, power_on and power_off calls.
> 
> Probably the same goes for the regulator_enable in phy-core.c. That
> should be handled in the phy driver as phy-core is already unable
> to handle it properly. For example, for phy-twl4030-usb.c we have
> three regulators to deal with and the phy framework won't have any
> idea how to deal with those.

hmm.. It was modelled for basic PHY drivers that have a single regulator (e.g.,
TI PIPE3 PHY). The idea is not to duplicate getting and enabling regulator in
each of the PHY drivers when it can be abstracted in phy-core.

Thanks
Kishon

Re: [PATCH 3/5] usb: phy: twl4030-usb: Move code from twl4030_phy_power to the runtime PM calls

[Tony Lindgren]

* Kishon Vijay Abraham I <kishon@ti.com> [140908 07:26]:
> Hi Tony,
> 
> On Thursday 04 September 2014 10:37 PM, Tony Lindgren wrote:
> > * Kishon Vijay Abraham I <kishon@ti.com> [140904 06:51]:
> >> Hi Tony,
> >>
> >> On Thursday 28 August 2014 04:58 AM, Tony Lindgren wrote:
> >>> We don't need twl4030_phy_power() any longer now that we have
> >>> the runtime PM calls. Let's get rid of it as it's confusing.
> >>> No functional changes, just move the code and use res instead
> >>> of ret as we are not returning that value.
> >>
> >> Now that you are doing pm_runtime_get_sync in twl4030_phy_init, won't it power
> >> on the phy even before initializing it (since runtime_resume will be invoked
> >> even before doing phy_init)?
> > 
> > Yes. The logic being that it should not matter as we are not
> > configuring the phy until in twl4030_phy_power_on(). Maybe we
> > should add code to make sure the phy is deconfigured initially
> > though :)
> > 
> > In twl4030_phy_init() we just want to check the ID pin state to get
> > things right initially. In the twl4030-usb case the I2C chip is
> > always on, but let's try to get the runtime PM set up like any
> > standard Linux driver would do it to avoid confusion.
> > 
> >> Even if pm_runtime_get_sync in not done in twl4030_phy_init, phy-core itself
> >> does pm_runtime_get_sync in phy_init().
> > 
> > Hmm OK. Looking at that, looks like we don't neeed any of these
> > custom exports though:
> > 
> > $ git grep phy_pm_runtime | grep EXPORT_SYMBOL
> > drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get);
> > drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
> > drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
> > drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
> > drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
> > drivers/phy/phy-core.c:EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
> > 
> > The reasons why I think we don't need the above at all is:
> > 
> > 1. We already have a framework for that in Linux runtime PM and we
> >    can follow the standard Linux runtime PM calls and not proxy
> >    them in phy-core.c
> 
> The reason for adding these are for providing fine grained control of the PHY
> by the controller drivers. In most cases the controller driver determines when
> the PHY should be active or idle.

Yeah but having the USB controller driver attempt to manage the
PHY did not work well. That all had to be ripped out of musb driver
in commits 30a70b026b4c and 8b2bc2c9351b.

I took a brief look at trying to fix musb + twl4030-usb runtime PM
so the USB controller driver would manage it. And that's probably
so far the only USB driver and PHY controller combo where we've had
runtime PM working in various forms in the mainline kernel.

Attempting to make the USB controller driver manage the runtime PM
for the PHY would make things unnecesarily complicated. The PHY can
sleep since it's on the I2C bus. So we'd have to implement some kind
of completion checking all over the place to attempt to keep things
in sync.

Meanwhile, having independent PHY drivers doing their own runtime
PM avoids all these issues. At minimum, it just means the PHY
driver needs to implement PHY init, power_on and power_off functions
like they already do. Then as needed, the PHY driver can implement
it's runtime PM calls.

I think what does make sense to do in the PHY framework is to keep
track of the PHY state in a generic way, and have a generic way of
telling the USB controller driver of the PHY state. That might allow
us eventually remove things like omap_musb_mailbox() calls.

And the custom exported functions above are unused AFAIK, so let's
just remove them.

> > 2. We can allow idling the phy properly on the bus it's connected
> >    to, in this case I2C, even if USB driver is not loaded. We
> >    eventually should idle the phy even if usb_add_phy_dev()
> >    failed
> 
> yes.. that's why we have ops like phy_power_on to tell when the PHY should be
> active. So these PHYs can be idled in probe.

Yeah phy_power_on et al are good, and needed. But the runtime PM
should be implemented in the actual PHY drivers because of the
reasons above.
 
> > 3. There's no actual need for phy-core.c to proxy the runtime
> >    PM calls
> > 
> > So we can and should just let the phy drivers do their own runtime PM
> > as needed based on just the phy init, power_on and power_off calls.
> > 
> > Probably the same goes for the regulator_enable in phy-core.c. That
> > should be handled in the phy driver as phy-core is already unable
> > to handle it properly. For example, for phy-twl4030-usb.c we have
> > three regulators to deal with and the phy framework won't have any
> > idea how to deal with those.
> 
> hmm.. It was modelled for basic PHY drivers that have a single regulator (e.g.,
> TI PIPE3 PHY). The idea is not to duplicate getting and enabling regulator in
> each of the PHY drivers when it can be abstracted in phy-core.

There's no need to for that AFAIK. That too can be implemented in the
PHY driver as part of runtime PM as needed.

Regards,

Tony

[PATCH 4/5] usb: phy: twl4030-usb: Remove asleep and rely on runtime PM

[Tony Lindgren]

There's no longer need for tracking the phy state in the driver
with asleep, we can now rely on runtime PM.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/phy-twl4030-usb.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 519cc90..24ff3c6 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -163,7 +163,6 @@ struct twl4030_usb {
 	int			irq;
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
-	u8			asleep;
 
 	struct delayed_work	id_workaround_work;
 };
@@ -388,14 +387,13 @@ static int twl4030_usb_runtime_suspend(struct device *dev)
 	struct twl4030_usb *twl = dev_get_drvdata(dev);
 
 	dev_dbg(twl->dev, "%s\n", __func__);
-	if (twl->asleep)
+	if (pm_runtime_suspended(dev))
 		return 0;
 
 	__twl4030_phy_power(twl, 0);
 	regulator_disable(twl->usb1v5);
 	regulator_disable(twl->usb1v8);
 	regulator_disable(twl->usb3v1);
-	twl->asleep = 1;
 
 	return 0;
 }
@@ -406,7 +404,7 @@ static int twl4030_usb_runtime_resume(struct device *dev)
 	int res;
 
 	dev_dbg(twl->dev, "%s\n", __func__);
-	if (!twl->asleep)
+	if (pm_runtime_active(dev))
 		return 0;
 
 	res = regulator_enable(twl->usb3v1);
@@ -435,7 +433,6 @@ static int twl4030_usb_runtime_resume(struct device *dev)
 			  twl4030_usb_read(twl, PHY_CLK_CTRL) |
 			  (PHY_CLK_CTRL_CLOCKGATING_EN |
 			   PHY_CLK_CTRL_CLK32K_EN));
-	twl->asleep = 0;
 
 	return 0;
 }
@@ -560,10 +557,10 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 		 */
 		if ((status == OMAP_MUSB_VBUS_VALID) ||
 		    (status == OMAP_MUSB_ID_GROUND)) {
-			if (twl->asleep)
+			if (pm_runtime_suspended(twl->dev))
 				pm_runtime_get_sync(twl->dev);
 		} else {
-			if (!twl->asleep) {
+			if (pm_runtime_active(twl->dev)) {
 				pm_runtime_mark_last_busy(twl->dev);
 				pm_runtime_put_autosuspend(twl->dev);
 			}
@@ -572,7 +569,7 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	}
 
 	/* don't schedule during sleep - irq works right then */
-	if (status == OMAP_MUSB_ID_GROUND && !twl->asleep) {
+	if (status == OMAP_MUSB_ID_GROUND && pm_runtime_active(twl->dev)) {
 		cancel_delayed_work(&twl->id_workaround_work);
 		schedule_delayed_work(&twl->id_workaround_work, HZ);
 	}
@@ -674,7 +671,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	twl->irq		= platform_get_irq(pdev, 0);
 	twl->vbus_supplied	= false;
 	twl->linkstat		= -EINVAL;
-	twl->asleep		= 1;
 	twl->linkstat		= OMAP_MUSB_UNKNOWN;
 
 	twl->phy.dev		= twl->dev;
-- 
1.8.1.1

[PATCH 5/5] usb: phy: twl4030-usb: Use mutex instead of spinlock for protecting the data

[Tony Lindgren]

We're using threaded irq on a I2C bus and we're sleeping in
twl4030_usb_irq() as it calls twl4030_usb_linkstat() which
calls the i2c functions. If we ever need to lock for longer
I2C transaction sequences a mutex will allow us to do that
easily.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/phy-twl4030-usb.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 24ff3c6..1e0e2d1 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -28,7 +28,6 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
-#include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/io.h>
 #include <linux/delay.h>
@@ -155,7 +154,7 @@ struct twl4030_usb {
 	struct regulator	*usb3v1;
 
 	/* for vbus reporting with irqs disabled */
-	spinlock_t		lock;
+	struct mutex		lock;
 
 	/* pin configuration */
 	enum twl4030_usb_mode	usb_mode;
@@ -516,13 +515,12 @@ static ssize_t twl4030_usb_vbus_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct twl4030_usb *twl = dev_get_drvdata(dev);
-	unsigned long flags;
 	int ret = -EINVAL;
 
-	spin_lock_irqsave(&twl->lock, flags);
+	mutex_lock(&twl->lock);
 	ret = sprintf(buf, "%s\n",
 			twl->vbus_supplied ? "on" : "off");
-	spin_unlock_irqrestore(&twl->lock, flags);
+	mutex_unlock(&twl->lock);
 
 	return ret;
 }
@@ -536,12 +534,12 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 
 	status = twl4030_usb_linkstat(twl);
 
-	spin_lock_irq(&twl->lock);
+	mutex_lock(&twl->lock);
 	if (status >= 0 && status != twl->linkstat) {
 		twl->linkstat = status;
 		status_changed = true;
 	}
-	spin_unlock_irq(&twl->lock);
+	mutex_unlock(&twl->lock);
 
 	if (status_changed) {
 		/* FIXME add a set_power() method so that B-devices can
@@ -695,8 +693,8 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	if (IS_ERR(phy_provider))
 		return PTR_ERR(phy_provider);
 
-	/* init spinlock for workqueue */
-	spin_lock_init(&twl->lock);
+	/* init mutex for workqueue */
+	mutex_init(&twl->lock);
 
 	INIT_DELAYED_WORK(&twl->id_workaround_work, twl4030_id_workaround_work);
 
-- 
1.8.1.1