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