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