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

From: Ming Lei
Date: Thu Sep 02 2010 - 20:43:30 EST

Next message: FUJITA Tomonori: "[PATCH -mm 2/8] alpha: enable ARCH_DMA_ADDR_T_64BIT"
Previous message: Rafael J. Wysocki: "Re: [PATCH] PM: Prevent waiting forever on asynchronous resume after abort"
In reply to: Felipe Balbi: "Re: [PATCH] USB: otg: twl4030: fix phy initialization"
Next in thread: Ming Lei: "Re: [PATCH] USB: otg: twl4030: fix phy initialization"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

2010/9/3 Felipe Balbi <me@xxxxxxxxxxxxxxx>:
> Hi,

thanks for your comments.

>
> On Thu, 2 Sep 2010 23:58:18 +0800, tom.leiming@xxxxxxxxx wrote:
>> From: Ming Lei <tom.leiming@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: FUJITA Tomonori: "[PATCH -mm 2/8] alpha: enable ARCH_DMA_ADDR_T_64BIT"
Previous message: Rafael J. Wysocki: "Re: [PATCH] PM: Prevent waiting forever on asynchronous resume after abort"
In reply to: Felipe Balbi: "Re: [PATCH] USB: otg: twl4030: fix phy initialization"
Next in thread: Ming Lei: "Re: [PATCH] USB: otg: twl4030: fix phy initialization"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]