Re: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd

[Felipe Balbi]

Hi,

On Mon, May 23, 2011 at 09:41:16AM +0300, Tatyana Brokhman wrote:
> This patch adds SS support to the dummy hcd module. It may be used to test
> SS device when no (SS) HW is available.
> USB 3.0 hub includes 2 hubs - HS and SS ones.
> This patch adds support for a SS hub in the dummy_hcd module. Thus, when
> dummy_hcd enabled it will register 2 root hubs (SS and HS).
> A new module parameter was added to simulate a SuperSpeed connection.
> When a new device is connected it will enumerate via the HS hub by default.
> In order to simulate SuperSpeed connection set the is_super_speed module
> parameter to true.
> 
> Signed-off-by: Tatyana Brokhman <tlinder@xxxxxxxxxxxxxx>
> 
> ---
>  drivers/usb/gadget/dummy_hcd.c |  540 +++++++++++++++++++++++++++++++++++-----
>  1 files changed, 483 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index bf7981d..c2731d3 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC);
>  MODULE_AUTHOR ("David Brownell");
>  MODULE_LICENSE ("GPL");
>  
> +struct dummy_hcd_module_parameters {
> +	bool is_super_speed;
> +};
> +
> +static struct dummy_hcd_module_parameters mod_data = {
> +	.is_super_speed = false
> +};
> +module_param_named(is_super_speed, mod_data.is_super_speed, bool, S_IRUGO);
> +MODULE_PARM_DESC(is_super_speed, "true to simulate SuperSpeed connection");

you shouldn't need this. You should always enable SuperSpeed for this
driver.

>  /*-------------------------------------------------------------------------*/
>  
>  /* gadget side driver data structres */
> @@ -188,6 +197,7 @@ struct dummy {
>  	 * MASTER/HOST side support
>  	 */
>  	struct dummy_hcd		*hs_hcd;
> +	struct dummy_hcd		*ss_hcd;
>  };
>  
>  static inline struct dummy_hcd *hcd_to_dummy_hcd(struct usb_hcd *hcd)
> @@ -218,7 +228,10 @@ static inline struct dummy *ep_to_dummy (struct dummy_ep *ep)
>  static inline struct dummy_hcd *gadget_to_dummy_hcd(struct usb_gadget *gadget)
>  {
>  	struct dummy *dum = container_of(gadget, struct dummy, gadget);
> -	return dum->hs_hcd;
> +	if (dum->gadget.speed == USB_SPEED_SUPER)
> +		return dum->ss_hcd;
> +	else
> +		return dum->hs_hcd;
>  }
>  
>  static inline struct dummy *gadget_dev_to_dummy (struct device *dev)
> @@ -266,10 +279,88 @@ stop_activity (struct dummy *dum)
>  	/* driver now does any non-usb quiescing necessary */
>  }
>  
> +/**
> + * set_ss_link_state() - Sets the current state of the SuperSpeed link
> + * @dum_hcd: pointer to the dummy_hcd structure to update the link
> + *     state for
> + *
> + * This function updates the port_status according to the link
> + * state. The old status is saved befor updating.
> + * Note: this function should be called only for SuperSpeed
> + * master and the caller must hold the lock.
> + */
> +static void
> +set_ss_link_state(struct dummy_hcd *dum_hcd)

a significant part of this function is similar to the USB2.0 version
set_link_state(), so it's better to phase out the similar parts to an
internal function and use that on both set_*_link_state(). Add something
like:

__set_link_state_by_speed(struct dummy_hcd *dum_hcd, enum
	usb_device_speed speed);

then do all the similar parts there copying for speed, then just call
that function on both set_ss_link_state() and set_link_state().

> +{
> +	struct dummy *dum = dum_hcd->dum;

add a blank line here

> +	if (dum->gadget.speed < USB_SPEED_SUPER)

this assumes USB_SPEED_SUPER has an assigned value bigger than the
others which might not be true if someone changes the order of the
enumeration. So it's better to use:

if (dum->gadget.speed != USB_SPEED_SUPER)

> +		return;
> +	dum_hcd->active = 0;
> +	if ((dum_hcd->port_status & USB_SS_PORT_STAT_POWER) == 0)
> +		dum_hcd->port_status = 0;

if one branch has {} all of them should have :-) You can fix it while
re-factoring this code.

> +	/* UDC suspend must cause a disconnect */
> +	else if (!dum->pullup || dum->udc_suspended) {
> +		dum_hcd->port_status &= ~(USB_PORT_STAT_CONNECTION |
> +					USB_PORT_STAT_ENABLE);
> +		if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0)
> +			dum_hcd->port_status |=
> +					(USB_PORT_STAT_C_CONNECTION << 16);
> +	} else {
> +		/* device is connected and not suspended */
> +		dum_hcd->port_status |= (USB_PORT_STAT_CONNECTION |
> +				     USB_PORT_STAT_SPEED_5GBPS) ;
> +		if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) == 0)
> +			dum_hcd->port_status |=
> +				(USB_PORT_STAT_C_CONNECTION << 16);
> +		if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 1 &&
> +		    (dum_hcd->port_status & USB_SS_PORT_LS_U0) == 1 &&
> +		    dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
> +			dum_hcd->active = 1;
> +	}
> +
> +

only one blank line :-)

> +	if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 0 ||
> +	     dum_hcd->active)
> +		dum_hcd->resuming = 0;
> +
> +	/* if !connected or reset */
> +	if ((dum_hcd->port_status & USB_PORT_STAT_CONNECTION) == 0 ||
> +			(dum_hcd->port_status & USB_PORT_STAT_RESET) != 0) {
> +		/*
> +		 * We're connected and not reseted (reset occured now),

'reseted' isn't proper spelling :-)

> +		 * and driver attached - disconnect!
> +		 */
> +		if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0 &&
> +		    (dum_hcd->old_status & USB_PORT_STAT_RESET) == 0 &&
> +		    dum->driver) {
> +			stop_activity(dum);
> +			spin_unlock(&dum->lock);
> +			dum->driver->disconnect(&dum->gadget);
> +			spin_lock(&dum->lock);
> +		}
> +	} else if (dum_hcd->active != dum_hcd->old_active) {
> +		if (dum_hcd->old_active && dum->driver->suspend) {
> +			spin_unlock(&dum->lock);
> +			dum->driver->suspend(&dum->gadget);
> +			spin_lock(&dum->lock);
> +		} else if (!dum_hcd->old_active &&  dum->driver->resume) {
> +			spin_unlock(&dum->lock);
> +			dum->driver->resume(&dum->gadget);
> +			spin_lock(&dum->lock);
> +		}
> +	}
> +
> +	dum_hcd->old_status = dum_hcd->port_status;
> +	dum_hcd->old_active = dum_hcd->active;
> +}
> +
>  /* caller must hold lock */
>  static void set_link_state(struct dummy_hcd *dum_hcd)
>  {
>  	struct dummy *dum = dum_hcd->dum;

add a blank line here.

> @@ -1371,6 +1577,10 @@ static void dummy_timer(unsigned long _dum_hcd)
>  	case USB_SPEED_HIGH:
>  		total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
>  		break;
> +	case USB_SPEED_SUPER:
> +		/* Bus speed is 500000 bytes/ms, so use a little less */

isn't it 500000 bits/ms ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature