Re: [PATCH 16/16] ARM: OMAP: omap4panda: Power down the USB PHY and ETH when not in use

[Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 11/22/12 20:21, the mail apparently from Felipe Balbi included:
> Hi,
>
> On Thu, Nov 22, 2012 at 09:13:16AM +0800, Andy Green wrote:
>> On 11/22/12 03:54, the mail apparently from Felipe Balbi included:
>>> Hi,
>>>
>>> On Wed, Nov 21, 2012 at 06:07:57PM +0200, Roger Quadros wrote:
>>>> On 11/21/2012 05:32 PM, Alan Stern wrote:
>>>>> On Wed, 21 Nov 2012, Roger Quadros wrote:
>>>>>
>>>>>> On 11/21/2012 04:52 PM, Alan Stern wrote:
>>>>>>> On Wed, 21 Nov 2012, Felipe Balbi wrote:
>>>>>>>
>>>>>>>> On Thu, Nov 15, 2012 at 04:34:14PM +0200, Roger Quadros wrote:
>>>>>>>>> From: Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>>>>
>>>>>>>>> This patch changes the management of the two GPIO for
>>>>>>>>> "hub reset" (actually controls enable of ULPI PHY and hub reset) and
>>>>>>>>> "hub power" (controls power to hub + eth).
>>>>>>>>
>>>>>>>> looks like this should be done by the hub driver. Alan, what would you
>>>>>>>> say ? Should the hub driver know how to power itself up ?
>>>>>>>
>>>>>>> Not knowing the context, I'm a little confused.  What is this hub
>>>>>>> you're talking about?  Is it a separate USB hub incorporated into the
>>>>>>> IP (like Intel's "rate-matching" hubs in their later chipsets)?  Or is
>>>>>>> it the root hub?
>>>>>>>
>>>>>>
>>>>>> This is actually a USB HUB + Ethernet combo chip (LAN9514) that is hard
>>>>>> wired on the panda board with its Power and Reset pins controlled by 2
>>>>>> GPIOs from the OMAP SoC.
>>>>>>
>>>>>> When powered, this chip can consume significant power (~0.7 W) because
>>>>>> of the (integrated Ethernet even when suspended. I suppose the ethernet
>>>>>> driver SMSC95XX) doesn't put it into a low enough power state on suspend.
>>>>>>
>>>>>> It doesn't make sense to power the chip when USB is not required on the
>>>>>> whole (e.g. ehci_hcd module is not loaded). This is what this patch is
>>>>>> trying to fix.
>>>>>
>>>>> Ah, okay.  Then yes, assuming you want to power this chip only when
>>>>> either ehci-hcd or the ethernet driver is loaded, the right places
>>>>> to manage the power GPIO are in the init and exit routines of the two
>>>>> drivers.
>>>>>
>>>>
>>>> The Ethernet function actually connects over USB within the chip. So
>>>> managing the power only in the ehci-hcd driver should suffice.
>>>
>>> the thing is that this could cause code duplication all over. LAN95xx is
>>
>> I can see this point.  However DT will soak up these regulator
>> definitions, at that point it's "for free".  On Linaro tilt-3.4 we
>> already have this on the dts
>>
>> 	hubpower: fixedregulator@0 {
>> 			compatible = "regulator-fixed";
>> 			regulator-name = "vhub0";
>> 			regulator-min-microvolt = <3300000>;
>> 			regulator-max-microvolt = <3300000>;
>> 			gpio = <&gpio1 1 0>;		/* gpio 1 : HUB Power */
>> 			startup-delay-us = <70000>;
>> 			enable-active-high;
>> 	};
>>
>> 	hubreset: fixedregulator@1 {
>> 			compatible = "regulator-fixed";
>> 			regulator-name = "hsusb0";	/* tag to associate with PORT 1 */
>> 			regulator-min-microvolt = <3300000>;
>> 			regulator-max-microvolt = <3300000>;
>> 			gpio = <&gpio2 30 0>;	/* gpio 62 : HUB & PHY Reset */
>> 			startup-delay-us = <70000>;
>> 			enable-active-high;
>> 			vin-supply = "vhub0"; /* Makes regulator f/w enable power before reset */
>> 	};
>>
>> which is the equivalent to my patch: I don't think we need to sweat
>> about code duplication.
>
> why not ? Just because you have some ready DT files outside of the
> mailine kernel ? Sorry, not a good argument.

That's not my argument: it's the whole basis for bothering with DT, but 
never mind.

>>> a generic USB Hub + Ethernet combo on one of ports from SMSC. *Any*
>>> platform could use it and could hook those power and reset pins to a
>>> GPIO. If we handle this at ehci-omap level, we risk having to duplicate
>>> the same piece of code for ehci-*.c
>>
>> We need to consider power and reset separately.  Reset is a safe bet
>> as a GPIO but power to the smsc chip is not.
>
> so ? I'm saying that it *can* be attached to other architectures and
> they *can* decide to put both on GPIOs. I'm not considering the
> likelyhood of the situation.

There's some confusion here --->

>> On panda they happen to fit a gpio-controlled linear regulator to
>> provide the smsc 3.3V supply.  On another device that can just as
>
> good to know, then we need a regulator driver (as you added on your DT
> file the bindings for it) instead of poking into the GPIO directly.

Assuming you mean "regulator device", if you look at the patch, that is 
what it does.

The original board file code just sets the GPIO as a one-off and forgets 
about it, so the whole show is permanently powered, which is 
objectionable since ~50% of Panda idle power is burned on that.

My patch uses regulator definitions in the board file - I only touch the 
board file - to allow the omap ehci driver to control the power.

> Again it sounds like something that should be done at the hub driver
> level. I mean using the GPIO (for reset) and Power Regulator.
>
> not implementing the regulator part itself, but using it.

If you mean change the regulator managing code from living in omap-hsusb 
to ehci-hcd, it sounds like a good idea.

>> easily be a PMIC regulator channel: it boils down to controlling a
>> power rail.  So using struct regulator as the abstraction for the
>> power is the right way already.
>
> I agree with you here. Nevertheless, I'm arguing that this all should be
> handled by the hub driver, not ehci-omap.

If "hub driver" means ehci-hcd then I agree, why not let all the ehci 
platforms have the same regulator management option instead of just OMAP.

>>> Since that's a hub driver anyway, I wonder if it would be better to play
>>> with those gpios somewhere else ?!?
>>
>> The patch creates a regulator that binds to a magic regulator name
>> defined by the hsusb driver, "hsusb0".
>>
>> That regulator is taken up and down by hsusb driver with the
>> lifecycle of the logical hsusb device.  So inserting ehci-hcd module
>> powers the regulator until the module is removed.
>
> but this is wrong. What if we use a different HUB part, what if the same
> HUB part is used in e.g. Tegra-based platform ?
>
> This is why I say it's *wrong* to handle that at the OMAP USB Host part.
> This is why I'm arguing that this whole thing should be handled by the
> hub driver itself.

Yes if we mean ehci-hcd manages the regulator, it will be better.  AFAIK 
it doesn't right now and the omap bit does, so my patch used what exists 
and works.

>> Originally I bound the regulator to the smsc95xx driver, which also
>
> that's wrong too. You can't assume that the network part of the device
> knows when the USB part needs to be powered up. That's just plain wrong.

 From the POV of the original goal of the patch, it was just to give us 
a way to control static power consumption by modprobe.  It would work 
fine to do that by modprobe [-r] smsc95xx same as ehci-hcd actually, 
although I agree it's backward from usual discover -> udev -> modprobe 
POV.  Anyway that is not what we ended up with so no need to worry about it.

>> offers a struct regulator.  But there is a quirk on Panda that means
>> that is not workable.
>>
>> On Panda, they share one SoC gpio to reset both an external ULPI PHY
>> chip and the smsc95xx that is downstream of it.
>
> of course. the Network part is attached to one of the Hub ports, that's
> why the hub exposes only two ports.

I am not sure how that makes it an "of course".  It's not like there's a 
shortage of SoC gpio to separate them and allow cleaner software.  But 
never mind that either.

>> After you power up the smsc95xx, you must reset it in order for
>> correct operation.  This actually resets the ULPI PHY too.
>>
>> The ULPI PHY is permanently powered, only nRESET is provided to
>> control it.
>>
>> For that reason, as an attribute of being on a Panda board, we need
>> to do the (shared) reset meddling once at hsusb init, and that is why
>> the patch is as it is.
>
> yeah, yeah, but it's not correct to push all that code is the OMAP USB
> Part when the hub driver is the only one who knows when the hub is going
> to be reset and the like.
>
> You're poking into a HUB, not into the EHCI controller.

My patch is just using what's there at the moment.  It only touches the 
board file and does not "push all that code is the OMAP USB part".

I agree with you what's in the OMAP USB part is better in the generic 
part, but I didn't put it there.

If you want that moved and nobody else wants to do it, I can probably do 
that in a new, additional patch.  If so you might want to confirm you're 
OK with the magic naming convention for regulators or (as Roger 
suggested earlier) pass it in by platform_data.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
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