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@xxxxxxxxxx>
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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
