On Thu, 15 Jul 2010, John Youn wrote:
> Made some changes to work with superspeed hubs:
> - Added new constants in header
> - Changes for new superspeed hub descriptor
> - Set the hub depth after enumeration
> - Modify the port status bits from the hub
>
> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
> ---
> Hi Sarah,
> This is our latest patch for 3.0 Hubs. Now applies to 2.6.35.
I really don't like the approach taken by this patch. IMO it would be
much better to fix xhci-hcd, to make it register both a high-speed root
hub and a SuperSpeed root hub. That should be the first step.
Then this work would be simpler. All those tests for root hubs could
be removed, because now you would support external hubs too.
> +static inline int hub_is_superspeed(struct usb_device *hdev)
> +{
> + return (hdev->descriptor.bDeviceProtocol == 3);
> +}
It would be better to add a new flag in struct usb_hub for this.
> /* Protect struct usb_device->state and ->children members
> * Note: Both are also protected by ->dev.sem, except that ->state can
> @@ -176,10 +180,17 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data, int size)
> {
> int i, ret;
>
> + unsigned dtype = USB_DT_HUB;
> +
> + if (hub_is_superspeed(hdev)) {
> + dtype = USB_DT_SS_HUB;
> + size = 12;
> + }
This is silly. Why pass size as an argument if the subroutine is going
to change it? Remove the size argument entirely -- this routine is
called from only one place anyhow.
> @@ -365,6 +376,17 @@ static int hub_port_status(struct usb_hub *hub, int port1,
> } else {
> *status = le16_to_cpu(hub->status->port.wPortStatus);
> *change = le16_to_cpu(hub->status->port.wPortChange);
> +
> + if ((hub->hdev->parent != NULL) && hub_is_superspeed(hub->hdev)) {
> + /* Translate the USB 3 port status */
> + u16 tmp = *status & USB3_PORT_STAT_MASK;
> + if (*status & USB3_PORT_STAT_POWER)
> + tmp |= USB_PORT_STAT_POWER;
> + if ((*status & USB3_PORT_STAT_SPEED) == USB3_PORT_STAT_SPEED_SUPER)
> + tmp |= USB_PORT_STAT_SUPER_SPEED;
> + *status = tmp;
None of this should be necessary. The code that checks port status
values should use different tests for SuperSpeed and non-SuperSpeed
hubs.
> + }
> +
> ret = 0;
> }
> mutex_unlock(&hub->status_mutex);
> @@ -607,7 +629,7 @@ static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
> if (hdev->children[port1-1] && set_state)
> usb_set_device_state(hdev->children[port1-1],
> USB_STATE_NOTATTACHED);
> - if (!hub->error)
> + if (!hub->error && !hub_is_superspeed(hub->hdev))
> ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_ENABLE);
What's the reason for this? Can't we disable SuperSpeed ports? What
if we _need_ to disable the port?
> /*
> + * USB 3.0 wPortStatus bit fields
> + * See USB 3.0 spec Table 10-10
> + */
> +/* Bits that are the same from USB 2.0 */
> +#define USB3_PORT_STAT_MASK (USB_PORT_STAT_CONNECTION | \
> + USB_PORT_STAT_ENABLE | \
> + USB_PORT_STAT_OVERCURRENT | \
> + USB_PORT_STAT_RESET)
Do we really need this? It isn't mentioned in the spec, is it?
> +/* 3.0 specific bits */
> +#define USB3_PORT_STAT_LINK_STATE 0x01e0
> +#define USB3_PORT_STAT_POWER 0x0200
> +#define USB3_PORT_STAT_SPEED 0x1c00
This should be called USB3_PORT_STAT_SPEED_MASK.
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
