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

[Ming Lei]

2010/9/5 Felipe Balbi <me@xxxxxxxxxxxxxxx>:
> Hi,
>
> On Sat, 4 Sep 2010 23:36:39 +0800, Ming Lei <tom.leiming@xxxxxxxxx> 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-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/