[U-Boot] [U-Boot, v3, 6/8] USB: SS: Add support for Super Speed USB interface
Julius Werner
jwerner at chromium.org
Fri Apr 19 20:22:06 CEST 2013
Migrating my comments here for public discussion.
> This adds usb framework support for super-speed usb, which will
> further facilitate to add stack support for xHCI.
>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan at samsung.com>
> Signed-off-by: Vivek Gautam <gautam.vivek at samsung.com>
>
> ---
> Changes from v2:
> - Replacing if-else with switch-case in portspeed() in cmd_usb.c
>
> common/cmd_usb.c | 24 ++++++++++++++++++------
> common/usb.c | 5 +++++
> common/usb_hub.c | 8 ++++++--
> include/usb.h | 6 ++++++
> include/usb_defs.h | 24 +++++++++++++++++++++++-
> 5 files changed, 58 insertions(+), 9 deletions(-)
>
> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> index dacdc2d..594385a 100644
> --- a/common/cmd_usb.c
> +++ b/common/cmd_usb.c
> @@ -271,12 +271,24 @@ static void usb_display_config(struct usb_device *dev)
>
> static inline char *portspeed(int speed)
> {
> - if (speed == USB_SPEED_HIGH)
> - return "480 Mb/s";
> - else if (speed == USB_SPEED_LOW)
> - return "1.5 Mb/s";
> - else
> - return "12 Mb/s";
> + char *speed_str;
> +
> + switch (speed) {
> + case USB_SPEED_SUPER:
> + speed_str = "5 Gb/s";
> + break;
> + case USB_SPEED_HIGH:
> + speed_str = "480 Mb/s";
> + break;
> + case USB_SPEED_LOW:
> + speed_str = "1.5 Mb/s";
> + break;
> + default:
> + speed_str = "12 Mb/s";
> + break;
> + }
> +
> + return speed_str;
> }
>
> /* shows the device tree recursively */
> diff --git a/common/usb.c b/common/usb.c
> index 3a96a34..55fff5b 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -409,6 +409,11 @@ static int usb_parse_config(struct usb_device *dev,
> wMaxPacketSize);
> debug("if %d, ep %d\n", ifno, epno);
> break;
> + case USB_DT_SS_ENDPOINT_COMP:
> + if_desc = &dev->config.if_desc[ifno];
> + memcpy(&if_desc->ss_ep_comp_desc[epno],
> + &buffer[index], buffer[index]);
> + break;
> default:
> if (head->bLength == 0)
> return 1;
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index ab41943..1e225e6 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -165,7 +165,9 @@ static struct usb_hub_device *usb_hub_allocate(void)
>
> static inline char *portspeed(int portstatus)
> {
> - if (portstatus & (1 << USB_PORT_FEAT_HIGHSPEED))
> + if (portstatus & (1 << USB_PORT_FEAT_SUPERSPEED))
It doesn't make sense to use the USB_PORT_FEAT_ constants to read a port status
here. These should be USB_PORT_STAT_ instead (you could use a switch statement
if you mask and shift them correctly).
> + return "5 Gb/s";
> + else if (portstatus & (1 << USB_PORT_FEAT_HIGHSPEED))
> return "480 Mb/s";
> else if (portstatus & (1 << USB_PORT_FEAT_LOWSPEED))
> return "1.5 Mb/s";
> @@ -268,7 +270,9 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
> /* Allocate a new device struct for it */
> usb = usb_alloc_new_device(dev->controller);
>
> - if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> + if (portstatus & USB_PORT_STAT_SUPER_SPEED)
> + usb->speed = USB_SPEED_SUPER;
> + else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> usb->speed = USB_SPEED_HIGH;
> else if (portstatus & USB_PORT_STAT_LOW_SPEED)
> usb->speed = USB_SPEED_LOW;
Should change to switch statement after implementing my suggestion to
usb_defs.h
> diff --git a/include/usb.h b/include/usb.h
> index d79c865..d7b082d 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -76,6 +76,12 @@ struct usb_interface {
> unsigned char act_altsetting;
>
> struct usb_endpoint_descriptor ep_desc[USB_MAXENDPOINTS];
> + /*
> + * Super Speed Device will have Super Speed Endpoint
> + * Companion Descriptor (section 9.6.7 of usb 3.0 spec)
> + * Revision 1.0 June 6th 2011
> + */
> + struct usb_ss_ep_comp_descriptor ss_ep_comp_desc[USB_MAXENDPOINTS];
> } __attribute__ ((packed));
>
> /* Configuration information.. */
> diff --git a/include/usb_defs.h b/include/usb_defs.h
> index 0c78d9d..e2aaef3 100644
> --- a/include/usb_defs.h
> +++ b/include/usb_defs.h
> @@ -203,6 +203,8 @@
> #define USB_PORT_FEAT_POWER 8
> #define USB_PORT_FEAT_LOWSPEED 9
> #define USB_PORT_FEAT_HIGHSPEED 10
> +#define USB_PORT_FEAT_FULLSPEED 11
> +#define USB_PORT_FEAT_SUPERSPEED 12
Speed is never set as a feature anyway. We should just get rid of all of these
and always use USB_PORT_STAT_ for them.
> #define USB_PORT_FEAT_C_CONNECTION 16
> #define USB_PORT_FEAT_C_ENABLE 17
> #define USB_PORT_FEAT_C_SUSPEND 18
> @@ -218,8 +220,20 @@
> #define USB_PORT_STAT_POWER 0x0100
> #define USB_PORT_STAT_LOW_SPEED 0x0200
> #define USB_PORT_STAT_HIGH_SPEED 0x0400 /* support for EHCI */
> +#define USB_PORT_STAT_FULL_SPEED 0x0800
> +#define USB_PORT_STAT_SUPER_SPEED 0x1000 /* support for XHCI */
These two new values are *not* correct. 0x800 is actually PORT_TEST, and 0x1000
is PORT_INDICATOR. Full speed has always been reported by having both the
LOW_SPEED and the HIGH_SPEED bits zero, and you should define it that way (for
use in switch statements).
If you want to fake SuperSpeed into the USB 2.0 hub descriptor (see my comment
on xhci_port_speed in the unreleased CL), you could represent it with 0xc00,
which should be a reserved value for all existing 1.0 and 2.0 hubs.
> #define USB_PORT_STAT_SPEED \
> - (USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED)
> + (USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED | \
> + USB_PORT_STAT_FULL_SPEED | USB_PORT_STAT_SUPER_SPEED)
Maybe this should be renamed to USB_PORT_STAT_SPEED_MASK to make its purpose
clearer.
> +
> +/*
> + * Additions to wPortStatus bit field from USB 3.0
I would reword "Additions" to "Changes" and explicitly state that these fields
concern the new USB 3.0 hub descriptor format, which is different and
exclusively applies to SuperSpeed hubs.
> + * See USB 3.0 spec Table 10-10
In my spec it's Table 10-11... are you sure you have the most recent version?
> + */
> +#define USB_PORT_STAT_LINK_STATE 0x01e0
For consistency, I would prefix all values in here with USB_SS_PORT_STAT_ (to
make extra clear that they belong to something different than the ones above).
> +#define USB_SS_PORT_STAT_POWER 0x0200
> +#define USB_SS_PORT_STAT_SPEED 0x1c00
> +#define USB_PORT_STAT_SPEED_5GBPS 0x0000
>
> /* wPortChange bits */
> #define USB_PORT_STAT_C_CONNECTION 0x0001
> @@ -228,6 +242,14 @@
> #define USB_PORT_STAT_C_OVERCURRENT 0x0008
> #define USB_PORT_STAT_C_RESET 0x0010
>
> +/*
> + * Addition to wPortChange bit fields form USB 3.0
> + * See USB 3.0 spec Table 10-11
> + */
> +#define USB_PORT_STAT_C_BH_RESET 0x0020
Same here. New incompatible port status format for SuperSpeed hubs, please make
the comment clearer and prefix as USB_SS_PORT_STAT_C_
> +#define USB_PORT_STAT_C_LINK_STATE 0x0040
> +#define USB_PORT_STAT_C_CONFIG_ERROR 0x0080
> +
> /* wHubCharacteristics (masks) */
> #define HUB_CHAR_LPSM 0x0003
> #define HUB_CHAR_COMPOUND 0x0004
More information about the U-Boot
mailing list