On 11/22/2012 06:12 PM, Felipe Balbi wrote: > Hi, > > On Thu, Nov 22, 2012 at 05:00:45PM +0200, Roger Quadros wrote: >> On 11/22/2012 03:56 PM, Felipe Balbi wrote: >>> Hi, >>> >>> On Thu, Nov 22, 2012 at 09:49:05PM +0800, Andy Green wrote: >>>>> 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. >>> >>> I mean that drivers/usb/core/hub.c should manage it, since that's what >>> actually manages the HUB entity. >> >> Yes. I agree that powering up the on-board hubs should be done >> generically and not in ehci-omap.c. I'm not sure how it can be done in >> hub.c. >> >> I'm assuming that such problem is limited to root hub ports where system > > an external LAN95xx HUB is not the roothub. LAN95xx is connected to the > roothub. > I didn't say LAN95xx is the root hub. It is connected to the root hub. So it is in theory, the root hub port's responsibility to power the LAN95xx. >> designers have the flexibility to provide power to the peripherals >> outside the USB Hub specification. >> >> I can think of two solutions >> >> 1) Associate the regulators with the root hub _ports_ and enable them as >> part of port's power-up routine. > > doesn't make sense. We need to figure out a way to attach the regulator > to the ports which actually have them. In this case it's the external > LAN95xx, right ? I think you are confused here. The LAN95xx's ports are compatible with USB hub specification and they work using the in-band set_port_feature mechanism already. We have a problem powering the LAN95xx itself which ideally should have been powered with set_port_feature on the root hub. (or ehci_port_power() in this case). > >> 2) Have a global list of regulators that are registered by platform code >> and enabled them all at once on hcd init. > > also wrong as it might cause increased power consumption even when only > a single USB port is currently in use. Yes that is true. I'm not for (2) certainly. > > The patch below is *CLEARLY WRONG* it's just to illustrate how this > could be started: > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 1af04bd..662d4cf 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -26,6 +26,7 @@ > #include <linux/mutex.h> > #include <linux/freezer.h> > #include <linux/random.h> > +#include <linux/regulator/consumer.h> > > #include <asm/uaccess.h> > #include <asm/byteorder.h> > @@ -44,6 +45,7 @@ struct usb_port { > struct device dev; > struct dev_state *port_owner; > enum usb_port_connect_type connect_type; > + struct regulator *port_regulator; > }; > > struct usb_hub { > @@ -847,8 +849,41 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay) > else > dev_dbg(hub->intfdev, "trying to enable port power on " > "non-switchable hub\n"); > - for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++) > + for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++) { > + regulator_enable(hub->ports[port1]->port_regulator); > set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER); > + } > + > + /* Wait at least 100 msec for power to become stable */ > + delay = max(pgood_delay, (unsigned) 100); > + if (do_delay) > + msleep(delay); > + return delay; > +} > + > +static unsigned hub_power_off(struct usb_hub *hub, bool do_delay) > +{ > + int port1; > + unsigned pgood_delay = hub->descriptor->bPwrOn2PwrGood * 2; > + unsigned delay; > + u16 wHubCharacteristics = > + le16_to_cpu(hub->descriptor->wHubCharacteristics); > + > + /* Disable power on each port. Some hubs have reserved values > + * of LPSM (> 2) in their descriptors, even though they are > + * USB 2.0 hubs. Some hubs do not implement port-power switching > + * but only emulate it. In all cases, the ports won't work > + * unless we send these messages to the hub. > + */ > + if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2) > + dev_dbg(hub->intfdev, "disabling power on all ports\n"); > + else > + dev_dbg(hub->intfdev, "trying to disable port power on " > + "non-switchable hub\n"); > + for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++) { > + regulator_disable(hub->ports[port1]->port_regulator); > + clear_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER); > + } > > /* Wait at least 100 msec for power to become stable */ > delay = max(pgood_delay, (unsigned) 100); > @@ -1336,6 +1371,9 @@ static int hub_configure(struct usb_hub *hub, > goto fail; > } > > + if (hub->has_external_port_regulator) /* maybe implement with a quirk flag ??? */ > + regulator_get(hub_dev, "hub_port_supply\n"); > + > wHubCharacteristics = le16_to_cpu(hub->descriptor->wHubCharacteristics); > > /* FIXME for USB 3.0, skip for now */ > @@ -4205,8 +4243,10 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, > > /* maybe switch power back on (e.g. root hub was reset) */ > if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2 > - && !port_is_power_on(hub, portstatus)) > + && !port_is_power_on(hub, portstatus)) { > + regulator_enable(hub->ports[port1]->port_regulator); > set_port_feature(hdev, port1, USB_PORT_FEAT_POWER); > + } > > if (portstatus & USB_PORT_STAT_ENABLE) > goto done; > > Note that if we have a single regulator, than all ports' regulators > should point to the same struct regulator so that regulator_get() and > regulator_put() can do proper reference counting. > > This is just an idea though. > Thanks for the example. The only problem is, how do we associate a regulator to a specific port in the system. cheers, -roger -- 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