[PATCH] USB: otg: twl4030: fix phy initialization

[PATCH] USB: otg: twl4030: fix phy initialization

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

Commit 461c317705eca5cac09a360f488715927fd0a927(into 2.6.36-v3)
is put forward to power down phy if no usb cable is connected,
but does introduce the two issues below:

1), phy is not into work state if usb cable is connected
with PC during poweron, so musb device mode is not usable
in such case, follows the reasons:
	-twl4030_phy_resume is not called, so
		regulators are not enabled
		i2c access are not enabled
		usb mode not configurated

2), The kernel warings[1] of regulators 'unbalanced disables'
is caused if poweron without usb cable connected
with PC or b-device.

This patch fixes the two issues above:
	-power down phy only if no usb cable is connected with PC
and b-device
	-do phy initialization(via __twl4030_phy_resume) if usb cable
is connected with PC(vbus event) or another b-device(ID event) in
twl4030_usb_probe.

This patch is verified OK on Beagle board either connected with
usb cable or not when poweron.

[1]. warnings of 'unbalanced disables' of regulators.
[root@OMAP3EVM /]# dmesg
------------[ cut here ]------------
WARNING: at drivers/regulator/core.c:1357 _regulator_disable+0x38/0x128()
unbalanced disables for VUSB1V8
Modules linked in:
Backtrace:
[<c0030c48>] (dump_backtrace+0x0/0x110) from [<c034f5a8>] (dump_stack+0x18/0x1c)
 r7:c78179d8 r6:c01ed6b8 r5:c0410822 r4:0000054d
[<c034f590>] (dump_stack+0x0/0x1c) from [<c0057da8>] (warn_slowpath_common+0x54/0x6c)
[<c0057d54>] (warn_slowpath_common+0x0/0x6c) from [<c0057e64>] (warn_slowpath_fmt+0x38/0x40)
 r9:00000000 r8:00000000 r7:c78e6608 r6:00000000 r5:fffffffb
r4:c78e6c00
[<c0057e2c>] (warn_slowpath_fmt+0x0/0x40) from [<c01ed6b8>] (_regulator_disable+0x38/0x128)
 r3:c0410e53 r2:c0410ad5
[<c01ed680>] (_regulator_disable+0x0/0x128) from [<c01ed87c>] (regulator_disable+0x24/0x38)
 r7:c78e6608 r6:00000000 r5:c78e6c40 r4:c78e6c00
[<c01ed858>] (regulator_disable+0x0/0x38) from [<c02382dc>] (twl4030_phy_power+0x15c/0x17c)
 r5:c78595c0 r4:00000000
[<c0238180>] (twl4030_phy_power+0x0/0x17c) from [<c023831c>] (twl4030_phy_suspend+0x20/0x2c)
 r6:00000000 r5:c78595c0 r4:c78595c0
[<c02382fc>] (twl4030_phy_suspend+0x0/0x2c) from [<c0238638>] (twl4030_usb_irq+0x11c/0x16c)
 r5:c78595c0 r4:00000040
[<c023851c>] (twl4030_usb_irq+0x0/0x16c) from [<c034ec18>] (twl4030_usb_probe+0x2c4/0x32c)
 r6:00000000 r5:00000000 r4:c78595c0
[<c034e954>] (twl4030_usb_probe+0x0/0x32c) from [<c02152a0>] (platform_drv_probe+0x20/0x24)
 r7:00000000 r6:c047d49c r5:c78e6608 r4:c047d49c
[<c0215280>] (platform_drv_probe+0x0/0x24) from [<c0214244>] (driver_probe_device+0xd0/0x190)
[<c0214174>] (driver_probe_device+0x0/0x190) from [<c02143d4>] (__device_attach+0x44/0x48)
 r7:00000000 r6:c78e6608 r5:c78e6608 r4:c047d49c
[<c0214390>] (__device_attach+0x0/0x48) from [<c0213694>] (bus_for_each_drv+0x50/0x90)
 r5:c0214390 r4:00000000
[<c0213644>] (bus_for_each_drv+0x0/0x90) from [<c0214474>] (device_attach+0x70/0x94)
 r6:c78e663c r5:c78e6608 r4:c78e6608
[<c0214404>] (device_attach+0x0/0x94) from [<c02134fc>] (bus_probe_device+0x2c/0x48)
 r7:00000000 r6:00000002 r5:c78e6608 r4:c78e6600
[<c02134d0>] (bus_probe_device+0x0/0x48) from [<c0211e48>] (device_add+0x340/0x4b4)
[<c0211b08>] (device_add+0x0/0x4b4) from [<c021597c>] (platform_device_add+0x110/0x16c)
[<c021586c>] (platform_device_add+0x0/0x16c) from [<c0220cb0>] (add_numbered_child+0xd8/0x118)
 r7:00000000 r6:c045f15c r5:c78e6600 r4:00000000
[<c0220bd8>] (add_numbered_child+0x0/0x118) from [<c001c618>] (twl_probe+0x3a4/0x72c)
[<c001c274>] (twl_probe+0x0/0x72c) from [<c02601ac>] (i2c_device_probe+0x7c/0xa4)
[<c0260130>] (i2c_device_probe+0x0/0xa4) from [<c0214244>] (driver_probe_device+0xd0/0x190)
 r5:c7856e20 r4:c047c860
[<c0214174>] (driver_probe_device+0x0/0x190) from [<c02143d4>] (__device_attach+0x44/0x48)
 r7:c7856e04 r6:c7856e20 r5:c7856e20 r4:c047c860
[<c0214390>] (__device_attach+0x0/0x48) from [<c0213694>] (bus_for_each_drv+0x50/0x90)
 r5:c0214390 r4:00000000
[<c0213644>] (bus_for_each_drv+0x0/0x90) from [<c0214474>] (device_attach+0x70/0x94)
 r6:c7856e54 r5:c7856e20 r4:c7856e20
[<c0214404>] (device_attach+0x0/0x94) from [<c02134fc>] (bus_probe_device+0x2c/0x48)
 r7:c7856e04 r6:c78fd048 r5:c7856e20 r4:c7856e20
[<c02134d0>] (bus_probe_device+0x0/0x48) from [<c0211e48>] (device_add+0x340/0x4b4)
[<c0211b08>] (device_add+0x0/0x4b4) from [<c0211fd8>] (device_register+0x1c/0x20)
[<c0211fbc>] (device_register+0x0/0x20) from [<c0260aa8>] (i2c_new_device+0xec/0x150)
 r5:c7856e00 r4:c7856e20
[<c02609bc>] (i2c_new_device+0x0/0x150) from [<c0260dc0>] (i2c_register_adapter+0xa0/0x1c4)
 r7:00000000 r6:c78fd078 r5:c78fd048 r4:c781d5c0
[<c0260d20>] (i2c_register_adapter+0x0/0x1c4) from [<c0260f80>] (i2c_add_numbered_adapter+0x9c/0xb4)
 r7:00000a28 r6:c04600a8 r5:c78fd048 r4:00000000
[<c0260ee4>] (i2c_add_numbered_adapter+0x0/0xb4) from [<c034efa4>] (omap_i2c_probe+0x324/0x3e8)
 r5:00000000 r4:c78fd000
[<c034ec80>] (omap_i2c_probe+0x0/0x3e8) from [<c02152a0>] (platform_drv_probe+0x20/0x24)
[<c0215280>] (platform_drv_probe+0x0/0x24) from [<c0214244>] (driver_probe_device+0xd0/0x190)
[<c0214174>] (driver_probe_device+0x0/0x190) from [<c021436c>] (__driver_attach+0x68/0x8c)
 r7:c78b2140 r6:c047e214 r5:c04600e4 r4:c04600b0
[<c0214304>] (__driver_attach+0x0/0x8c) from [<c021399c>] (bus_for_each_dev+0x50/0x84)
 r7:c78b2140 r6:c047e214 r5:c0214304 r4:00000000
[<c021394c>] (bus_for_each_dev+0x0/0x84) from [<c0214068>] (driver_attach+0x20/0x28)
 r6:c047e214 r5:c047e214 r4:c00270d0
[<c0214048>] (driver_attach+0x0/0x28) from [<c0213274>] (bus_add_driver+0xa8/0x228)
[<c02131cc>] (bus_add_driver+0x0/0x228) from [<c02146a4>] (driver_register+0xb0/0x13c)
[<c02145f4>] (driver_register+0x0/0x13c) from [<c0215744>] (platform_driver_register+0x4c/0x60)
 r9:00000000 r8:c001f688 r7:00000013 r6:c005b6fc r5:c00083dc
r4:c00270d0
[<c02156f8>] (platform_driver_register+0x0/0x60) from [<c001f69c>] (omap_i2c_init_driver+0x14/0x1c)
[<c001f688>] (omap_i2c_init_driver+0x0/0x1c) from [<c002c460>] (do_one_initcall+0xd0/0x1a4)
[<c002c390>] (do_one_initcall+0x0/0x1a4) from [<c0008478>] (kernel_init+0x9c/0x154)
[<c00083dc>] (kernel_init+0x0/0x154) from [<c005b6fc>] (do_exit+0x0/0x688)
 r5:c00083dc r4:00000000
---[ end trace 1b75b31a2719ed1d ]---

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Felipe Balbi <felipe.balbi@nokia.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
 drivers/usb/otg/twl4030-usb.c |   72 ++++++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index 05aaac1..df6381f 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -347,11 +347,22 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 	}
 }
 
-static void twl4030_phy_power(struct twl4030_usb *twl, int on)
+static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
 {
+	u8 old_pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
 	u8 pwr;
 
-	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
+	if (on)
+		pwr = old_pwr & ~PHY_PWR_PHYPWD;
+	else
+		pwr = old_pwr | PHY_PWR_PHYPWD;
+
+	if (pwr != old_pwr)
+		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
+}
+
+static void twl4030_phy_power(struct twl4030_usb *twl, int on)
+{
 	if (on) {
 		regulator_enable(twl->usb3v1);
 		regulator_enable(twl->usb1v8);
@@ -365,15 +376,13 @@ static void twl4030_phy_power(struct twl4030_usb *twl, int on)
 		twl_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0,
 							VUSB_DEDICATED2);
 		regulator_enable(twl->usb1v5);
-		pwr &= ~PHY_PWR_PHYPWD;
-		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
+		__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  {
-		pwr |= PHY_PWR_PHYPWD;
-		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
+		__twl4030_phy_power(twl, 0);
 		regulator_disable(twl->usb1v5);
 		regulator_disable(twl->usb1v8);
 		regulator_disable(twl->usb3v1);
@@ -387,19 +396,25 @@ static void twl4030_phy_suspend(struct twl4030_usb *twl, int controller_off)
 
 	twl4030_phy_power(twl, 0);
 	twl->asleep = 1;
+	dev_dbg(twl->dev, "%s\n", __func__);
 }
 
-static void twl4030_phy_resume(struct twl4030_usb *twl)
+static void __twl4030_phy_resume(struct twl4030_usb *twl)
 {
-	if (!twl->asleep)
-		return;
-
 	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);
+}
+
+static void twl4030_phy_resume(struct twl4030_usb *twl)
+{
+	if (!twl->asleep)
+		return;
+	__twl4030_phy_resume(twl);
 	twl->asleep = 0;
+	dev_dbg(twl->dev, "%s\n", __func__);
 }
 
 static int twl4030_usb_ldo_init(struct twl4030_usb *twl)
@@ -502,6 +517,26 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	return IRQ_HANDLED;
 }
 
+static void twl4030_usb_phy_init(struct twl4030_usb *twl)
+{
+	int status;
+
+	status = twl4030_usb_linkstat(twl);
+	if (status >= 0) {
+		if (status == USB_EVENT_NONE) {
+			__twl4030_phy_power(twl, 0);
+			twl->asleep = 1;
+		} else {
+			__twl4030_phy_resume(twl);
+			twl->asleep = 0;
+		}
+
+		blocking_notifier_call_chain(&twl->otg.notifier, status,
+				twl->otg.gadget);
+	}
+	sysfs_notify(&twl->dev->kobj, NULL, "vbus");
+}
+
 static int twl4030_set_suspend(struct otg_transceiver *x, int suspend)
 {
 	struct twl4030_usb *twl = xceiv_to_twl(x);
@@ -550,7 +585,6 @@ static int __devinit twl4030_usb_probe(struct platform_device *pdev)
 	struct twl4030_usb_data *pdata = pdev->dev.platform_data;
 	struct twl4030_usb	*twl;
 	int			status, err;
-	u8			pwr;
 
 	if (!pdata) {
 		dev_dbg(&pdev->dev, "platform_data not available\n");
@@ -569,10 +603,7 @@ static int __devinit twl4030_usb_probe(struct platform_device *pdev)
 	twl->otg.set_peripheral	= twl4030_set_peripheral;
 	twl->otg.set_suspend	= twl4030_set_suspend;
 	twl->usb_mode		= pdata->usb_mode;
-
-	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
-
-	twl->asleep		= (pwr & PHY_PWR_PHYPWD);
+	twl->asleep = 1;
 
 	/* init spinlock for workqueue */
 	spin_lock_init(&twl->lock);
@@ -610,15 +641,10 @@ static int __devinit twl4030_usb_probe(struct platform_device *pdev)
 		return status;
 	}
 
-	/* The IRQ handler just handles changes from the previous states
-	 * of the ID and VBUS pins ... in probe() we must initialize that
-	 * previous state.  The easy way:  fake an IRQ.
-	 *
-	 * REVISIT:  a real IRQ might have happened already, if PREEMPT is
-	 * enabled.  Else the IRQ may not yet be configured or enabled,
-	 * because of scheduling delays.
+	/* Power down phy or make it work according to
+	 * current link state.
 	 */
-	twl4030_usb_irq(twl->irq, twl);
+	twl4030_usb_phy_init(twl);
 
 	dev_info(&pdev->dev, "Initialized TWL4030 USB module\n");
 	return 0;
-- 
1.6.2.5

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Felipe Balbi]

Hi,

On Thu,  2 Sep 2010 23:58:18 +0800, tom.leiming@gmail.com wrote:
> From: Ming Lei <tom.leiming@gmail.com>
> 
> Commit 461c317705eca5cac09a360f488715927fd0a927(into 2.6.36-v3)
> is put forward to power down phy if no usb cable is connected,
> but does introduce the two issues below:
> 
> 1), phy is not into work state if usb cable is connected
> with PC during poweron, so musb device mode is not usable
> in such case, follows the reasons:

I'm pretty sure I verified both cases.

> 	-twl4030_phy_resume is not called, so
> 		regulators are not enabled
> 		i2c access are not enabled
> 		usb mode not configurated
> 
> 2), The kernel warings[1] of regulators 'unbalanced disables'
> is caused if poweron without usb cable connected
> with PC or b-device.
> 
> This patch fixes the two issues above:
> 	-power down phy only if no usb cable is connected with PC
> and b-device
> 	-do phy initialization(via __twl4030_phy_resume) if usb cable
> is connected with PC(vbus event) or another b-device(ID event) in
> twl4030_usb_probe.
> 
> This patch is verified OK on Beagle board either connected with
> usb cable or not when poweron.

I kinda doubt it, would have to test it myself, but I'm without
enough gear to test it again.

> [1]. warnings of 'unbalanced disables' of regulators.
> [root@OMAP3EVM /]# dmesg
> ------------[ cut here ]------------
> WARNING: at drivers/regulator/core.c:1357
_regulator_disable+0x38/0x128()

this should not have been caused by that patch, though.


> Cc: Felipe Balbi <felipe.balbi@nokia.com>

this email doesn't exist anymore... I'm yet to subscribe the new one,
for now keep this one in Cc, sorry for that.

> diff --git a/drivers/usb/otg/twl4030-usb.c
b/drivers/usb/otg/twl4030-usb.c
> index 05aaac1..df6381f 100644
> --- a/drivers/usb/otg/twl4030-usb.c
> +++ b/drivers/usb/otg/twl4030-usb.c
> @@ -347,11 +347,22 @@ static void twl4030_i2c_access(struct twl4030_usb
> *twl, int on)
>  	}
>  }
>  
> -static void twl4030_phy_power(struct twl4030_usb *twl, int on)
> +static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
>  {
> +	u8 old_pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
>  	u8 pwr;
>  
> -	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
> +	if (on)
> +		pwr = old_pwr & ~PHY_PWR_PHYPWD;
> +	else
> +		pwr = old_pwr | PHY_PWR_PHYPWD;
> +
> +	if (pwr != old_pwr)
> +		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);

writing 1 if register is one won't cause any problems, you're just
wasting a variable.

> +static void twl4030_phy_power(struct twl4030_usb *twl, int on)
> +{
>  	if (on) {
>  		regulator_enable(twl->usb3v1);
>  		regulator_enable(twl->usb1v8);
> @@ -365,15 +376,13 @@ static void twl4030_phy_power(struct twl4030_usb
> *twl, int on)
>  		twl_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0,
>  							VUSB_DEDICATED2);
>  		regulator_enable(twl->usb1v5);
> -		pwr &= ~PHY_PWR_PHYPWD;
> -		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
> +		__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  {
> -		pwr |= PHY_PWR_PHYPWD;
> -		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
> +		__twl4030_phy_power(twl, 0);
>  		regulator_disable(twl->usb1v5);
>  		regulator_disable(twl->usb1v8);
>  		regulator_disable(twl->usb3v1);
> @@ -387,19 +396,25 @@ static void twl4030_phy_suspend(struct twl4030_usb
> *twl, int controller_off)
>  
>  	twl4030_phy_power(twl, 0);
>  	twl->asleep = 1;
> +	dev_dbg(twl->dev, "%s\n", __func__);

this is noise.

>  }
>  
> -static void twl4030_phy_resume(struct twl4030_usb *twl)
> +static void __twl4030_phy_resume(struct twl4030_usb *twl)
>  {
> -	if (!twl->asleep)
> -		return;
> -
>  	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);
> +}
> +
> +static void twl4030_phy_resume(struct twl4030_usb *twl)
> +{
> +	if (!twl->asleep)
> +		return;
> +	__twl4030_phy_resume(twl);
>  	twl->asleep = 0;
> +	dev_dbg(twl->dev, "%s\n", __func__);

this is noise.

> @@ -502,6 +517,26 @@ static irqreturn_t twl4030_usb_irq(int irq, void
> *_twl)
>  	return IRQ_HANDLED;
>  }
>  
> +static void twl4030_usb_phy_init(struct twl4030_usb *twl)
> +{
> +	int status;
> +
> +	status = twl4030_usb_linkstat(twl);
> +	if (status >= 0) {
> +		if (status == USB_EVENT_NONE) {
> +			__twl4030_phy_power(twl, 0);
> +			twl->asleep = 1;
> +		} else {
> +			__twl4030_phy_resume(twl);

this might break people who are using charger detection, although I
would have to revisit some code to be sure.

> +			twl->asleep = 0;
> +		}
> +
> +		blocking_notifier_call_chain(&twl->otg.notifier, status,
> +				twl->otg.gadget);
> +	}
> +	sysfs_notify(&twl->dev->kobj, NULL, "vbus");

thís should only be called from IRQ handler and is duplicating
code.

> @@ -550,7 +585,6 @@ static int __devinit twl4030_usb_probe(struct
> platform_device *pdev)
>  	struct twl4030_usb_data *pdata = pdev->dev.platform_data;
>  	struct twl4030_usb	*twl;
>  	int			status, err;
> -	u8			pwr;
>  
>  	if (!pdata) {
>  		dev_dbg(&pdev->dev, "platform_data not available\n");
> @@ -569,10 +603,7 @@ static int __devinit twl4030_usb_probe(struct
> platform_device *pdev)
>  	twl->otg.set_peripheral	= twl4030_set_peripheral;
>  	twl->otg.set_suspend	= twl4030_set_suspend;
>  	twl->usb_mode		= pdata->usb_mode;
> -
> -	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
> -
> -	twl->asleep		= (pwr & PHY_PWR_PHYPWD);
> +	twl->asleep = 1;

and now you're lying to the driver again. What happens if
bootloader has put transceiver to sleep ??

> @@ -610,15 +641,10 @@ static int __devinit twl4030_usb_probe(struct
> platform_device *pdev)
>  		return status;
>  	}
>  
> -	/* The IRQ handler just handles changes from the previous states
> -	 * of the ID and VBUS pins ... in probe() we must initialize that
> -	 * previous state.  The easy way:  fake an IRQ.
> -	 *
> -	 * REVISIT:  a real IRQ might have happened already, if PREEMPT is
> -	 * enabled.  Else the IRQ may not yet be configured or enabled,
> -	 * because of scheduling delays.
> +	/* Power down phy or make it work according to
> +	 * current link state.

if you're changing the comment you might as well fix the comment style.

>  	 */
> -	twl4030_usb_irq(twl->irq, twl);
> +	twl4030_usb_phy_init(twl);

now I see, this doesn't care about that flag, so why even setting
it above ??

-- 
balbi

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Felipe Balbi]

Hi,

On Thu,  2 Sep 2010 23:58:18 +0800, tom.leiming@gmail.com wrote:
> From: Ming Lei <tom.leiming@gmail.com>
> 
> Commit 461c317705eca5cac09a360f488715927fd0a927(into 2.6.36-v3)
> is put forward to power down phy if no usb cable is connected,
> but does introduce the two issues below:
> 
> 1), phy is not into work state if usb cable is connected
> with PC during poweron, so musb device mode is not usable
> in such case, follows the reasons:

I'm pretty sure I verified both cases.

> 	-twl4030_phy_resume is not called, so
> 		regulators are not enabled
> 		i2c access are not enabled
> 		usb mode not configurated
> 
> 2), The kernel warings[1] of regulators 'unbalanced disables'
> is caused if poweron without usb cable connected
> with PC or b-device.
> 
> This patch fixes the two issues above:
> 	-power down phy only if no usb cable is connected with PC
> and b-device
> 	-do phy initialization(via __twl4030_phy_resume) if usb cable
> is connected with PC(vbus event) or another b-device(ID event) in
> twl4030_usb_probe.
> 
> This patch is verified OK on Beagle board either connected with
> usb cable or not when poweron.

I kinda doubt it, would have to test it myself, but I'm without
enough gear to test it again.

> [1]. warnings of 'unbalanced disables' of regulators.
> [root@OMAP3EVM /]# dmesg
> ------------[ cut here ]------------
> WARNING: at drivers/regulator/core.c:1357
_regulator_disable+0x38/0x128()

this should not have been caused by that patch, though.


> Cc: Felipe Balbi <felipe.balbi@nokia.com>

this email doesn't exist anymore... I'm yet to subscribe the new one,
for now keep this one in Cc, sorry for that.

> diff --git a/drivers/usb/otg/twl4030-usb.c
b/drivers/usb/otg/twl4030-usb.c
> index 05aaac1..df6381f 100644
> --- a/drivers/usb/otg/twl4030-usb.c
> +++ b/drivers/usb/otg/twl4030-usb.c
> @@ -347,11 +347,22 @@ static void twl4030_i2c_access(struct twl4030_usb
> *twl, int on)
>  	}
>  }
>  
> -static void twl4030_phy_power(struct twl4030_usb *twl, int on)
> +static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
>  {
> +	u8 old_pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
>  	u8 pwr;
>  
> -	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
> +	if (on)
> +		pwr = old_pwr & ~PHY_PWR_PHYPWD;
> +	else
> +		pwr = old_pwr | PHY_PWR_PHYPWD;
> +
> +	if (pwr != old_pwr)
> +		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);

writing 1 if register is one won't cause any problems, you're just
wasting a variable.

> +static void twl4030_phy_power(struct twl4030_usb *twl, int on)
> +{
>  	if (on) {
>  		regulator_enable(twl->usb3v1);
>  		regulator_enable(twl->usb1v8);
> @@ -365,15 +376,13 @@ static void twl4030_phy_power(struct twl4030_usb
> *twl, int on)
>  		twl_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0,
>  							VUSB_DEDICATED2);
>  		regulator_enable(twl->usb1v5);
> -		pwr &= ~PHY_PWR_PHYPWD;
> -		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
> +		__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  {
> -		pwr |= PHY_PWR_PHYPWD;
> -		WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
> +		__twl4030_phy_power(twl, 0);
>  		regulator_disable(twl->usb1v5);
>  		regulator_disable(twl->usb1v8);
>  		regulator_disable(twl->usb3v1);
> @@ -387,19 +396,25 @@ static void twl4030_phy_suspend(struct twl4030_usb
> *twl, int controller_off)
>  
>  	twl4030_phy_power(twl, 0);
>  	twl->asleep = 1;
> +	dev_dbg(twl->dev, "%s\n", __func__);

this is noise.

>  }
>  
> -static void twl4030_phy_resume(struct twl4030_usb *twl)
> +static void __twl4030_phy_resume(struct twl4030_usb *twl)
>  {
> -	if (!twl->asleep)
> -		return;
> -
>  	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);
> +}
> +
> +static void twl4030_phy_resume(struct twl4030_usb *twl)
> +{
> +	if (!twl->asleep)
> +		return;
> +	__twl4030_phy_resume(twl);
>  	twl->asleep = 0;
> +	dev_dbg(twl->dev, "%s\n", __func__);

this is noise.

> @@ -502,6 +517,26 @@ static irqreturn_t twl4030_usb_irq(int irq, void
> *_twl)
>  	return IRQ_HANDLED;
>  }
>  
> +static void twl4030_usb_phy_init(struct twl4030_usb *twl)
> +{
> +	int status;
> +
> +	status = twl4030_usb_linkstat(twl);
> +	if (status >= 0) {
> +		if (status == USB_EVENT_NONE) {
> +			__twl4030_phy_power(twl, 0);
> +			twl->asleep = 1;
> +		} else {
> +			__twl4030_phy_resume(twl);

this might break people who are using charger detection, although I
would have to revisit some code to be sure.

> +			twl->asleep = 0;
> +		}
> +
> +		blocking_notifier_call_chain(&twl->otg.notifier, status,
> +				twl->otg.gadget);
> +	}
> +	sysfs_notify(&twl->dev->kobj, NULL, "vbus");

thís should only be called from IRQ handler and is duplicating
code.

> @@ -550,7 +585,6 @@ static int __devinit twl4030_usb_probe(struct
> platform_device *pdev)
>  	struct twl4030_usb_data *pdata = pdev->dev.platform_data;
>  	struct twl4030_usb	*twl;
>  	int			status, err;
> -	u8			pwr;
>  
>  	if (!pdata) {
>  		dev_dbg(&pdev->dev, "platform_data not available\n");
> @@ -569,10 +603,7 @@ static int __devinit twl4030_usb_probe(struct
> platform_device *pdev)
>  	twl->otg.set_peripheral	= twl4030_set_peripheral;
>  	twl->otg.set_suspend	= twl4030_set_suspend;
>  	twl->usb_mode		= pdata->usb_mode;
> -
> -	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
> -
> -	twl->asleep		= (pwr & PHY_PWR_PHYPWD);
> +	twl->asleep = 1;

and now you're lying to the driver again. What happens if
bootloader has put transceiver to sleep ??

> @@ -610,15 +641,10 @@ static int __devinit twl4030_usb_probe(struct
> platform_device *pdev)
>  		return status;
>  	}
>  
> -	/* The IRQ handler just handles changes from the previous states
> -	 * of the ID and VBUS pins ... in probe() we must initialize that
> -	 * previous state.  The easy way:  fake an IRQ.
> -	 *
> -	 * REVISIT:  a real IRQ might have happened already, if PREEMPT is
> -	 * enabled.  Else the IRQ may not yet be configured or enabled,
> -	 * because of scheduling delays.
> +	/* Power down phy or make it work according to
> +	 * current link state.

if you're changing the comment you might as well fix the comment style.

>  	 */
> -	twl4030_usb_irq(twl->irq, twl);
> +	twl4030_usb_phy_init(twl);

now I see, this doesn't care about that flag, so why even setting
it above ??

-- 
balbi

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Ming Lei]

2010/9/3 Felipe Balbi <me@felipebalbi.com>:
> Hi,

thanks for your comments.

>
> On Thu,  2 Sep 2010 23:58:18 +0800, tom.leiming@gmail.com wrote:
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> Commit 461c317705eca5cac09a360f488715927fd0a927(into 2.6.36-v3)
>> is put forward to power down phy if no usb cable is connected,
>> but does introduce the two issues below:
>>
>> 1), phy is not into work state if usb cable is connected
>> with PC during poweron, so musb device mode is not usable
>> in such case, follows the reasons:
>
> I'm pretty sure I verified both cases.

Could you verify the beagle board in such case? If you commit is
reverted, no issue #1 for me.

>
>>       -twl4030_phy_resume is not called, so
>>               regulators are not enabled
>>               i2c access are not enabled
>>               usb mode not configurated
>>
>> 2), The kernel warings[1] of regulators 'unbalanced disables'
>> is caused if poweron without usb cable connected
>> with PC or b-device.
>>
>> This patch fixes the two issues above:
>>       -power down phy only if no usb cable is connected with PC
>> and b-device
>>       -do phy initialization(via __twl4030_phy_resume) if usb cable
>> is connected with PC(vbus event) or another b-device(ID event) in
>> twl4030_usb_probe.
>>
>> This patch is verified OK on Beagle board either connected with
>> usb cable or not when poweron.
>
> I kinda doubt it, would have to test it myself, but I'm without
> enough gear to test it again.

See my analysis below.

>
>> [1]. warnings of 'unbalanced disables' of regulators.
>> [root@OMAP3EVM /]# dmesg
>> ------------[ cut here ]------------
>> WARNING: at drivers/regulator/core.c:1357
> _regulator_disable+0x38/0x128()
>
> this should not have been caused by that patch, though.

It is surely caused by usb twl4030 otg driver if otg phy is not power down
in bootloader.

See my analysis  below.

>
>
>> Cc: Felipe Balbi <felipe.balbi@nokia.com>
>
> this email doesn't exist anymore... I'm yet to subscribe the new one,
> for now keep this one in Cc, sorry for that.
>
>> diff --git a/drivers/usb/otg/twl4030-usb.c
> b/drivers/usb/otg/twl4030-usb.c
>> index 05aaac1..df6381f 100644
>> --- a/drivers/usb/otg/twl4030-usb.c
>> +++ b/drivers/usb/otg/twl4030-usb.c
>> @@ -347,11 +347,22 @@ static void twl4030_i2c_access(struct twl4030_usb
>> *twl, int on)
>>       }
>>  }
>>
>> -static void twl4030_phy_power(struct twl4030_usb *twl, int on)
>> +static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
>>  {
>> +     u8 old_pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
>>       u8 pwr;
>>
>> -     pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
>> +     if (on)
>> +             pwr = old_pwr & ~PHY_PWR_PHYPWD;
>> +     else
>> +             pwr = old_pwr | PHY_PWR_PHYPWD;
>> +
>> +     if (pwr != old_pwr)
>> +             WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
>
> writing 1 if register is one won't cause any problems, you're just
> wasting a variable.
>
>> +static void twl4030_phy_power(struct twl4030_usb *twl, int on)
>> +{
>>       if (on) {
>>               regulator_enable(twl->usb3v1);
>>               regulator_enable(twl->usb1v8);
>> @@ -365,15 +376,13 @@ static void twl4030_phy_power(struct twl4030_usb
>> *twl, int on)
>>               twl_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0,
>>                                                       VUSB_DEDICATED2);
>>               regulator_enable(twl->usb1v5);
>> -             pwr &= ~PHY_PWR_PHYPWD;
>> -             WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
>> +             __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  {
>> -             pwr |= PHY_PWR_PHYPWD;
>> -             WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
>> +             __twl4030_phy_power(twl, 0);
>>               regulator_disable(twl->usb1v5);
>>               regulator_disable(twl->usb1v8);
>>               regulator_disable(twl->usb3v1);
>> @@ -387,19 +396,25 @@ static void twl4030_phy_suspend(struct twl4030_usb
>> *twl, int controller_off)
>>
>>       twl4030_phy_power(twl, 0);
>>       twl->asleep = 1;
>> +     dev_dbg(twl->dev, "%s\n", __func__);
>
> this is noise.

It is useful to troubleshoot the two issues above,  and I can
remove it if you doesn't like it.

>
>>  }
>>
>> -static void twl4030_phy_resume(struct twl4030_usb *twl)
>> +static void __twl4030_phy_resume(struct twl4030_usb *twl)
>>  {
>> -     if (!twl->asleep)
>> -             return;
>> -
>>       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);
>> +}
>> +
>> +static void twl4030_phy_resume(struct twl4030_usb *twl)
>> +{
>> +     if (!twl->asleep)
>> +             return;
>> +     __twl4030_phy_resume(twl);
>>       twl->asleep = 0;
>> +     dev_dbg(twl->dev, "%s\n", __func__);
>
> this is noise.

see above.

>> @@ -502,6 +517,26 @@ static irqreturn_t twl4030_usb_irq(int irq, void
>> *_twl)
>>       return IRQ_HANDLED;
>>  }
>>
>> +static void twl4030_usb_phy_init(struct twl4030_usb *twl)
>> +{
>> +     int status;
>> +
>> +     status = twl4030_usb_linkstat(twl);
>> +     if (status >= 0) {
>> +             if (status == USB_EVENT_NONE) {
>> +                     __twl4030_phy_power(twl, 0);
>> +                     twl->asleep = 1;
>> +             } else {
>> +                     __twl4030_phy_resume(twl);
>
> this might break people who are using charger detection, although I
> would have to revisit some code to be sure.

twl4030_usb_phy_init is only called once from .probe, and I don't think
break chager detection, which in principle is same with vbus detect, right?

>
>> +                     twl->asleep = 0;
>> +             }
>> +
>> +             blocking_notifier_call_chain(&twl->otg.notifier, status,
>> +                             twl->otg.gadget);
>> +     }
>> +     sysfs_notify(&twl->dev->kobj, NULL, "vbus");
>
> thís should only be called from IRQ handler and is duplicating
> code.

For the 1st case, something is not same.

>
>> @@ -550,7 +585,6 @@ static int __devinit twl4030_usb_probe(struct
>> platform_device *pdev)
>>       struct twl4030_usb_data *pdata = pdev->dev.platform_data;
>>       struct twl4030_usb      *twl;
>>       int                     status, err;
>> -     u8                      pwr;
>>
>>       if (!pdata) {
>>               dev_dbg(&pdev->dev, "platform_data not available\n");
>> @@ -569,10 +603,7 @@ static int __devinit twl4030_usb_probe(struct
>> platform_device *pdev)
>>       twl->otg.set_peripheral = twl4030_set_peripheral;
>>       twl->otg.set_suspend    = twl4030_set_suspend;
>>       twl->usb_mode           = pdata->usb_mode;
>> -
>> -     pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
>> -
>> -     twl->asleep             = (pwr & PHY_PWR_PHYPWD);
>> +     twl->asleep = 1;
>
> and now you're lying to the driver again. What happens if
> bootloader has put transceiver to sleep ??

If bootloader has put transceiver to sleep, all is OK and the two
issues above will not happen. The patch is to fix the two issues if
bootloader did not power down phy.

issue #1:
         -twl->asleep is set as zero in .probe since bootloader has not
powerdown phy
         -EVENT_VBUS returned from twl4030_usb_linkstat since usb cable
is connected with PC
         -twl4030_phy_resume is called but does nothing since
twl->asleep is zero
         -the following are not called to initialize otg phy:
                  twl4030_phy_power / twl4030_i2c_access / twl4030_usb_set_mode
         -so musb device mode does not work

issue #2:
         -twl->asleep is set as zero in .probe since bootloader has
not powerdown phy
         -EVENT_NONE returned from twl4030_usb_linkstat since no usb cable is
connected with board
         -twl4030_phy_suspend is called
         -twl4030_phy_power is called since twl->asleep is zero
         -regulator_disable is called for power down case
         -so cause the kernel warning since no regulator_enable is called before

>
>> @@ -610,15 +641,10 @@ static int __devinit twl4030_usb_probe(struct
>> platform_device *pdev)
>>               return status;
>>       }
>>
>> -     /* The IRQ handler just handles changes from the previous states
>> -      * of the ID and VBUS pins ... in probe() we must initialize that
>> -      * previous state.  The easy way:  fake an IRQ.
>> -      *
>> -      * REVISIT:  a real IRQ might have happened already, if PREEMPT is
>> -      * enabled.  Else the IRQ may not yet be configured or enabled,
>> -      * because of scheduling delays.
>> +     /* Power down phy or make it work according to
>> +      * current link state.
>
> if you're changing the comment you might as well fix the comment style.
>
>>        */
>> -     twl4030_usb_irq(twl->irq, twl);
>> +     twl4030_usb_phy_init(twl);
>
> now I see, this doesn't care about that flag, so why even setting
> it above ??

You mean .asleep? If so, the flag is set to be consistent
with the actual link state.

-- 
Lei Ming

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Ming Lei]

Felipe,

Considered bit PHYPWD of PHY_PWR_CTRL is zero at default,
say, OTG phy is not put into power down after system poweron,
so 2.6.36-rc3 will make musb device mode broken if bootloader
doesn't touch  PHYPWD of PHY_PWR_CTRL.

Could you ack the patch if no further objections or give other
suggestions?

thanks,

-- 
Lei Ming

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Ming Lei]

Felipe,

Considered bit PHYPWD of PHY_PWR_CTRL is zero at default,
say, OTG phy is not put into power down after system poweron,
so 2.6.36-rc3 will make musb device mode broken if bootloader
doesn't touch  PHYPWD of PHY_PWR_CTRL.

Could you ack the patch if no further objections or give other
suggestions?

thanks,

-- 
Lei Ming
--
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] USB: otg: twl4030: fix phy initialization

[Felipe Balbi]

Hi,

On Sat, 4 Sep 2010 23:36:39 +0800, Ming Lei <tom.leiming@gmail.com> wrote:
> Considered bit PHYPWD of PHY_PWR_CTRL is zero at default,
> say, OTG phy is not put into power down after system poweron,
> so 2.6.36-rc3 will make musb device mode broken if bootloader
> doesn't touch  PHYPWD of PHY_PWR_CTRL.
> 
> Could you ack the patch if no further objections or give other
> suggestions?

I don't see why that patch should cause problems with device
mode. Fact is that transceiver is only "asleep" when PHYPWD
bit is true and without setting that flag correctly we were
leaving phy's LDOs powered on for nothing when we were
booting without a USB cable connected.

If you don't explain exactly what is the problem you see
(besides the WARN) it's to help any further, specially now
that I'm 100% without HW to test against. I'm also in a
big hurry packing a buch of stuff for moving to another
appartment, so if there isn't enough information, I can't
be of any help.

Still, I would NACK you patch because I don't think it's
approaching the correct problem. If you only see that WARN,
you could start by chaging the code so that:

if (!regulator_is_enabled(twl->usb3v1)
    regulator_enable(twl->usb3v1);

and similarly for the regulator_disable() call, then check
whether there are any other problems. If there are, we will
take it from there.

All in all, I doubt that patch would cause break anything
since it's just checking the initial state from the
transceiver itself to set a flag in the driver structure,
that's all.

-- 
balbi

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Felipe Balbi]

Hi,

On Sat, 4 Sep 2010 23:36:39 +0800, Ming Lei <tom.leiming@gmail.com> wrote:
> Considered bit PHYPWD of PHY_PWR_CTRL is zero at default,
> say, OTG phy is not put into power down after system poweron,
> so 2.6.36-rc3 will make musb device mode broken if bootloader
> doesn't touch  PHYPWD of PHY_PWR_CTRL.
> 
> Could you ack the patch if no further objections or give other
> suggestions?

I don't see why that patch should cause problems with device
mode. Fact is that transceiver is only "asleep" when PHYPWD
bit is true and without setting that flag correctly we were
leaving phy's LDOs powered on for nothing when we were
booting without a USB cable connected.

If you don't explain exactly what is the problem you see
(besides the WARN) it's to help any further, specially now
that I'm 100% without HW to test against. I'm also in a
big hurry packing a buch of stuff for moving to another
appartment, so if there isn't enough information, I can't
be of any help.

Still, I would NACK you patch because I don't think it's
approaching the correct problem. If you only see that WARN,
you could start by chaging the code so that:

if (!regulator_is_enabled(twl->usb3v1)
    regulator_enable(twl->usb3v1);

and similarly for the regulator_disable() call, then check
whether there are any other problems. If there are, we will
take it from there.

All in all, I doubt that patch would cause break anything
since it's just checking the initial state from the
transceiver itself to set a flag in the driver structure,
that's all.

-- 
balbi

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Ming Lei]

2010/9/5 Felipe Balbi <me@felipebalbi.com>:
> Hi,
>
> On Sat, 4 Sep 2010 23:36:39 +0800, Ming Lei <tom.leiming@gmail.com> wrote:
>> Considered bit PHYPWD of PHY_PWR_CTRL is zero at default,
>> say, OTG phy is not put into power down after system poweron,
>> so 2.6.36-rc3 will make musb device mode broken if bootloader
>> doesn't touch  PHYPWD of PHY_PWR_CTRL.
>>
>> Could you ack the patch if no further objections or give other
>> suggestions?
>
> I don't see why that patch should cause problems with device
> mode. Fact is that transceiver is only "asleep" when PHYPWD
> bit is true and without setting that flag correctly we were
> leaving phy's LDOs powered on for nothing when we were
> booting without a USB cable connected.
>
> If you don't explain exactly what is the problem you see
> (besides the WARN) it's to help any further, specially now

Oh, I already explain the cause in detail in this thread.

          http://marc.info/?l=linux-kernel&m=128347462130590&w=2

No problem, do it again, if you have any questions about the explanation
please feel free to let me know.

issue #1:(musb device not work with cable connected with PC)
        -twl->asleep is set as zero in .probe since bootloader has not
powerdown phy
        -EVENT_VBUS returned from twl4030_usb_linkstat since usb cable
is connected with PC
        -twl4030_phy_resume is called but does nothing since
twl->asleep is zero
        -the following are not called to initialize otg phy:
                 twl4030_phy_power / twl4030_i2c_access / twl4030_usb_set_mode
        -so musb device mode does not work

issue #2:(regulator 'unbalanced disables' warnings )
        -twl->asleep is set as zero in .probe since bootloader has
not powerdown phy
        -EVENT_NONE returned from twl4030_usb_linkstat since no usb cable is
connected with board
        -twl4030_phy_suspend is called
        -twl4030_phy_power is called since twl->asleep is zero
        -regulator_disable is called for power down case
        -so cause the kernel warning since no regulator_enable is called before

> that I'm 100% without HW to test against. I'm also in a
> big hurry packing a buch of stuff for moving to another
> appartment, so if there isn't enough information, I can't
> be of any help.

Sorry for disturbing you, hope you have a happy moving, :-)

>
> Still, I would NACK you patch because I don't think it's
> approaching the correct problem. If you only see that WARN,
> you could start by chaging the code so that:
>
> if (!regulator_is_enabled(twl->usb3v1)
>    regulator_enable(twl->usb3v1);

Yes, it is another fix for the issue #2, just different way for
the same problem, right?

>
> and similarly for the regulator_disable() call, then check
> whether there are any other problems. If there are, we will
> take it from there.
>
> All in all, I doubt that patch would cause break anything
> since it's just checking the initial state from the
> transceiver itself to set a flag in the driver structure,
> that's all.

No, .asleep flag is set according to the current link state,
instead of the initial state of transceiver. I don't think the patch
will cause break anything. If you think it will, please describe the
break in detail.

thanks,

-- 
Lei Ming

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Ming Lei]

2010/9/5 Felipe Balbi <me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org>:
> Hi,
>
> On Sat, 4 Sep 2010 23:36:39 +0800, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Considered bit PHYPWD of PHY_PWR_CTRL is zero at default,
>> say, OTG phy is not put into power down after system poweron,
>> so 2.6.36-rc3 will make musb device mode broken if bootloader
>> doesn't touch  PHYPWD of PHY_PWR_CTRL.
>>
>> Could you ack the patch if no further objections or give other
>> suggestions?
>
> I don't see why that patch should cause problems with device
> mode. Fact is that transceiver is only "asleep" when PHYPWD
> bit is true and without setting that flag correctly we were
> leaving phy's LDOs powered on for nothing when we were
> booting without a USB cable connected.
>
> If you don't explain exactly what is the problem you see
> (besides the WARN) it's to help any further, specially now

Oh, I already explain the cause in detail in this thread.

          http://marc.info/?l=linux-kernel&m=128347462130590&w=2

No problem, do it again, if you have any questions about the explanation
please feel free to let me know.

issue #1:(musb device not work with cable connected with PC)
        -twl->asleep is set as zero in .probe since bootloader has not
powerdown phy
        -EVENT_VBUS returned from twl4030_usb_linkstat since usb cable
is connected with PC
        -twl4030_phy_resume is called but does nothing since
twl->asleep is zero
        -the following are not called to initialize otg phy:
                 twl4030_phy_power / twl4030_i2c_access / twl4030_usb_set_mode
        -so musb device mode does not work

issue #2:(regulator 'unbalanced disables' warnings )
        -twl->asleep is set as zero in .probe since bootloader has
not powerdown phy
        -EVENT_NONE returned from twl4030_usb_linkstat since no usb cable is
connected with board
        -twl4030_phy_suspend is called
        -twl4030_phy_power is called since twl->asleep is zero
        -regulator_disable is called for power down case
        -so cause the kernel warning since no regulator_enable is called before

> that I'm 100% without HW to test against. I'm also in a
> big hurry packing a buch of stuff for moving to another
> appartment, so if there isn't enough information, I can't
> be of any help.

Sorry for disturbing you, hope you have a happy moving, :-)

>
> Still, I would NACK you patch because I don't think it's
> approaching the correct problem. If you only see that WARN,
> you could start by chaging the code so that:
>
> if (!regulator_is_enabled(twl->usb3v1)
>    regulator_enable(twl->usb3v1);

Yes, it is another fix for the issue #2, just different way for
the same problem, right?

>
> and similarly for the regulator_disable() call, then check
> whether there are any other problems. If there are, we will
> take it from there.
>
> All in all, I doubt that patch would cause break anything
> since it's just checking the initial state from the
> transceiver itself to set a flag in the driver structure,
> that's all.

No, .asleep flag is set according to the current link state,
instead of the initial state of transceiver. I don't think the patch
will cause break anything. If you think it will, please describe the
break in detail.

thanks,

-- 
Lei Ming
--
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] USB: otg: twl4030: fix phy initialization

[Felipe Balbi]

Hi,

On Sun, 5 Sep 2010 14:42:51 +0800, Ming Lei <tom.leiming@gmail.com> wrote:
> issue #1:(musb device not work with cable connected with PC)
>         -twl->asleep is set as zero in .probe since bootloader has not
> powerdown phy
>         -EVENT_VBUS returned from twl4030_usb_linkstat since usb cable
> is connected with PC
>         -twl4030_phy_resume is called but does nothing since
> twl->asleep is zero
>         -the following are not called to initialize otg phy:
>                  twl4030_phy_power / twl4030_i2c_access /
>                  twl4030_usb_set_mode
>         -so musb device mode does not work

ok, I see. So what you could do is enable regulators on probe
based on PHYPWR bit.

> Sorry for disturbing you, hope you have a happy moving, :-)

np, tks :-)

> No, .asleep flag is set according to the current link state,
> instead of the initial state of transceiver. I don't think the patch
> will cause break anything. If you think it will, please describe the
> break in detail.

if we revert that patch, you'll see that usb3v1 ldos are left on
if we never plug/unplug usb cable. AFAIR, the reset state of those
LDOs is ON, so that's why we need that patch.

I'll try to work on it monday judging I'll have laptop and HW
available by then.

-- 
balbi

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Felipe Balbi]

Hi,

On Sun, 5 Sep 2010 14:42:51 +0800, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> issue #1:(musb device not work with cable connected with PC)
>         -twl->asleep is set as zero in .probe since bootloader has not
> powerdown phy
>         -EVENT_VBUS returned from twl4030_usb_linkstat since usb cable
> is connected with PC
>         -twl4030_phy_resume is called but does nothing since
> twl->asleep is zero
>         -the following are not called to initialize otg phy:
>                  twl4030_phy_power / twl4030_i2c_access /
>                  twl4030_usb_set_mode
>         -so musb device mode does not work

ok, I see. So what you could do is enable regulators on probe
based on PHYPWR bit.

> Sorry for disturbing you, hope you have a happy moving, :-)

np, tks :-)

> No, .asleep flag is set according to the current link state,
> instead of the initial state of transceiver. I don't think the patch
> will cause break anything. If you think it will, please describe the
> break in detail.

if we revert that patch, you'll see that usb3v1 ldos are left on
if we never plug/unplug usb cable. AFAIR, the reset state of those
LDOs is ON, so that's why we need that patch.

I'll try to work on it monday judging I'll have laptop and HW
available by then.

-- 
balbi
--
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] USB: otg: twl4030: fix phy initialization

[Ming Lei]

2010/9/5 Felipe Balbi <me@felipebalbi.com>:
> Hi,
>
> On Sun, 5 Sep 2010 14:42:51 +0800, Ming Lei <tom.leiming@gmail.com> wrote:
>> issue #1:(musb device not work with cable connected with PC)
>>         -twl->asleep is set as zero in .probe since bootloader has not
>> powerdown phy
>>         -EVENT_VBUS returned from twl4030_usb_linkstat since usb cable
>> is connected with PC
>>         -twl4030_phy_resume is called but does nothing since
>> twl->asleep is zero
>>         -the following are not called to initialize otg phy:
>>                  twl4030_phy_power / twl4030_i2c_access /
>>                  twl4030_usb_set_mode
>>         -so musb device mode does not work
>
> ok, I see. So what you could do is enable regulators on probe
> based on PHYPWR bit.

This patch will enable regulators on probe based on current link state
returned from twl4030_usb_linkstat instead of PHYPWR bit. If vbus or
ID event detected, call __twl4030_phy_resume to enable regulators/
phy power/set mode to make phy into work state, or else call
__twl4030_phy_power(0) to power down otg phy.

See twl4030_usb_phy_init introduced in the patch.

>> No, .asleep flag is set according to the current link state,
>> instead of the initial state of transceiver. I don't think the patch
>> will cause break anything. If you think it will, please describe the
>> break in detail.
>
> if we revert that patch, you'll see that usb3v1 ldos are left on

You commit before does fix one problem, which may keep phy in
power on even no cable connected, so I do not request a simple
revert, :-).

> if we never plug/unplug usb cable. AFAIR, the reset state of those
> LDOs is ON, so that's why we need that patch.

>From Table 5-373 of TPS65950 manual[1], usb3v1 ldo is in sleep state
after reset. But it is put into active state in twl4030_usb_ldo_init now,
seems activating it too early.  I have verified it OK on beagle board to
remove the early activating of usb3v1 in twl4030_usb_ldo_init. I'll merge
it into the v2 of the patch and post it for your review.

>
> I'll try to work on it monday judging I'll have laptop and HW
> available by then.

Very good, thanks your work!

[1], TPS65950 Technical Reference Manual, http://www.ti.com/litv/pdf/swcu050e


thanks,
-- 
Lei Ming

Re: [PATCH] USB: otg: twl4030: fix phy initialization

[Ming Lei]

2010/9/5 Felipe Balbi <me@felipebalbi.com>:
> Hi,
>
> On Sun, 5 Sep 2010 14:42:51 +0800, Ming Lei <tom.leiming@gmail.com> wrote:
>> issue #1:(musb device not work with cable connected with PC)
>>         -twl->asleep is set as zero in .probe since bootloader has not
>> powerdown phy
>>         -EVENT_VBUS returned from twl4030_usb_linkstat since usb cable
>> is connected with PC
>>         -twl4030_phy_resume is called but does nothing since
>> twl->asleep is zero
>>         -the following are not called to initialize otg phy:
>>                  twl4030_phy_power / twl4030_i2c_access /
>>                  twl4030_usb_set_mode
>>         -so musb device mode does not work
>
> ok, I see. So what you could do is enable regulators on probe
> based on PHYPWR bit.

This patch will enable regulators on probe based on current link state
returned from twl4030_usb_linkstat instead of PHYPWR bit. If vbus or
ID event detected, call __twl4030_phy_resume to enable regulators/
phy power/set mode to make phy into work state, or else call
__twl4030_phy_power(0) to power down otg phy.

See twl4030_usb_phy_init introduced in the patch.

>> No, .asleep flag is set according to the current link state,
>> instead of the initial state of transceiver. I don't think the patch
>> will cause break anything. If you think it will, please describe the
>> break in detail.
>
> if we revert that patch, you'll see that usb3v1 ldos are left on

You commit before does fix one problem, which may keep phy in
power on even no cable connected, so I do not request a simple
revert, :-).

> if we never plug/unplug usb cable. AFAIR, the reset state of those
> LDOs is ON, so that's why we need that patch.

From Table 5-373 of TPS65950 manual[1], usb3v1 ldo is in sleep state
after reset. But it is put into active state in twl4030_usb_ldo_init now,
seems activating it too early.  I have verified it OK on beagle board to
remove the early activating of usb3v1 in twl4030_usb_ldo_init. I'll merge
it into the v2 of the patch and post it for your review.

>
> I'll try to work on it monday judging I'll have laptop and HW
> available by then.

Very good, thanks your work!

[1], TPS65950 Technical Reference Manual, http://www.ti.com/litv/pdf/swcu050e


thanks,
-- 
Lei Ming