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.
> >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.
> 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.
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.
> 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.
> >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.
> 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.
> 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.
> 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.
--
balbi
Attachment:
signature.asc
Description: Digital signature
