On Mon, 3 Dec 2012, Sarah Sharp wrote:
> On Mon, Dec 03, 2012 at 08:07:06PM +0100, Sebastian Andrzej Siewior wrote:
> > The USB 2.0 specification says that bMaxPower is the maximum power
> > consumption expressed in 2 mA units and the USB 3.0 specification says
> > that it is expressed in 8 mA units.
>
> Thanks for catching this, Sebastian! However, I think this patch needs
> to be extended in several ways to work properly.
>
> First, it looks like you need to update hcd->bus_mA. The USB 3.0 spec
> increases the bus power from 500mA to 900mA. See section 9.2.5.1:
>
> "The amount of current draw for SuperSpeed devices are increased to 150
> mA for low-power devices and 900 mA for high-power"
Excellent point! We ought to parameterize the values for unit load and
maximum load according to the device speed instead of using constants
all over the place.
> This code in hcd.c:usb_add_hcd needs to change:
>
> /* starting here, usbcore will pay attention to this root hub */
> rhdev->bus_mA = min(500u, hcd->power_budget);
In fact, this line is wrong. The 500u should be multiplied by the
number of ports -- and of course, it should be larger for SuperSpeed
root hubs.
> This code in hub.c:hub_configure also needs to change:
>
> if (hdev == hdev->bus->root_hub) {
> if (hdev->bus_mA == 0 || hdev->bus_mA >= 500)
> hub->mA_per_port = 500;
> else {
> hub->mA_per_port = hdev->bus_mA;
> hub->limited_power = 1;
> }
>
> It's capping the mA_per_port to 500mA, which shouldn't be done for USB
> 3.0 roothubs. Instead of having hard coded values, we probably either
> need to add a per-hub bus power, or have some #defines and set the
> mA_per_port based on roothub speed.
I vote for the latter.
> The else statement after the above code also needs to change:
>
> } else if ((hubstatus & (1 << USB_DEVICE_SELF_POWERED)) == 0) {
> dev_dbg(hub_dev, "hub controller current requirement: %dmA\n",
> hub->descriptor->bHubContrCurrent);
> hub->limited_power = 1;
> if (hdev->maxchild > 0) {
Hmmm... This test doesn't look very important. Who cares about hubs
with no ports?
> int remaining = hdev->bus_mA -
> hub->descriptor->bHubContrCurrent;
>
> if (remaining < hdev->maxchild * 100)
> dev_warn(hub_dev,
> "insufficient power available "
> "to use all downstream ports\n");
> hub->mA_per_port = 100; /* 7.2.1.1 */
>
> I think this is trying to set the mA_per_port to the unconfigured device
> power, which 7.2.1.1 says is one unit load.
Not to the unconfigured device power but simply to one unit load.
7.2.1 says:
External ports in a bus-powered hub can supply only one unit
load per port regardless of the current draw on the other ports
of that hub.
The comment actually should refer to 7.2.1, not 7.2.1.1.
> USB 2.0 uses 100 mA as the
> unit load. However, the USB 3.0 spec says, "When operating in
> SuperSpeed mode, 150 mA equals one unit load" and increases the
> unconfigured power consumption limit to 150 mA. So this code needs to
> set mA_per_port to 150 for SuperSpeed hubs.
Right.
> The 500 mA and 100 mA limits seem to be hard coded all over the place.
> For example, I think this code in hub.c:hub_port_connect_change() is
> looking for whether the above else statement has run:
>
> if (udev->descriptor.bDeviceClass == USB_CLASS_HUB
> && udev->bus_mA <= 100) {
>
> Of course, that code won't work properly when you change the bus_mA to
> be 150 mA for SuperSpeed hubs. Perhaps we need a new function to get
> the unconfigured bus power, that takes a usb_hub, and returns either 100
> or 150 based on device speed?
This test is looking for two consecutive bus-powered hubs. The 100
should be replaced with the appropriate unit load value.
> And this code has to change as well:
>
> } else { /* Self-powered external hub */
> /* FIXME: What about battery-powered external hubs that
> * provide less current per port? */
> hub->mA_per_port = 500;
> }
> if (hub->mA_per_port < 500)
> dev_dbg(hub_dev, "%umA bus power budget for each child\n",
> hub->mA_per_port);
>
> You might want to break out that code into its own function, in a
> separate patch.
Perhaps define unit_load and full_load variables at the start of this
section and initialize them to the appropriate values.
> And I think all this code will break once the USB Power Delivery spec
> gets implemented, and the USB host controllers can put out something
> like 12V to devices that ask for it. But that's a different issue.
>
> http://www.usb.org/developers/powerdelivery/
Indeed.
Alan Stern
--
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
