[U-Boot] [U-Boot, v3, 6/8] USB: SS: Add support for Super Speed USB interface
Vivek Gautam
gautamvivek1987 at gmail.com
Mon Apr 22 08:43:16 CEST 2013
Hi Julius,
On Fri, Apr 19, 2013 at 11:52 PM, Julius Werner <jwerner at chromium.org> wrote:
> Migrating my comments here for public discussion.
Thanks for your valuable comments here.
>
>> 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).
True, we should be 'ANDing' here with respectve PORT_STAT_ constants,
will modify this as required.
>
>> + 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
Sure,
>
>> 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.
These are simply 'Hub class feature numbers' given in USB 2.0 and 3.0 specs.
Why don't we just keep them like that (I shall be removing the
USB_PORT_FEAT_FULLSPEED
and USB_PORT_FEAT_SUPERSPEED definitely from here). In case we want to
check the port status field,
we would check against PORT_STAT_ constants.
>
>> #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).
Sure, will remove these two constants.
>
> 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.
Alright, shall rename it to USB_PORT_STAT_SPEED_MASK.
>
>> +
>> +/*
>> + * 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.
This bit fields are actually additions to USB 3.0 framework. These
fields are not defined in
USB 2.0 specs, or else they are reserved there. If you insist i can
modify this as suggested.
>
>> + * 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?
Yea, my mistake, typo !!
>
>> + */
>> +#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).
True, shall change this to make things consistent.
>
>> +#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_
Sure, will add the relavant prefix, and write proper comment.
>
>> +#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
--
Best Regards
Vivek
More information about the U-Boot
mailing list