[U-Boot] [PATCH 1/6] usb : musb : Add high speed field in usb_device structure

[U-Boot] [PATCH 1/6] usb : musb : Add high speed field in usb_device structure

[Thomas Abraham]

This patch adds support for identification of high speed devices.
For high spped devices that are connected via hubs, the information
that the device is high speed is recorded. This is required by Mentor
USB Host controller driver.

Signed-off-by: Ravi Babu <ravibabu@ti.com>
Signed-off-by: Swaminathan S <swami.iyer@ti.com>
Signed-off-by: Thomas Abraham <t-abraham@ti.com>
Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>
---
 common/usb.c       |    1 +
 include/usb.h      |    1 +
 include/usb_defs.h |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index ee18152..f8379c9 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1137,6 +1137,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
 	/* Allocate a new device struct for it */
 	usb = usb_alloc_new_device();
 	usb->slow = (portstatus & USB_PORT_STAT_LOW_SPEED) ? 1 : 0;
+	usb->high = (portstatus & USB_PORT_STAT_HIGH_SPEED) ? 1 : 0;
 
 	dev->children[port] = usb;
 	usb->parent = dev;
diff --git a/include/usb.h b/include/usb.h
index 510df95..e6aa551 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -139,6 +139,7 @@ enum {
 struct usb_device {
 	int	devnum;			/* Device number on USB bus */
 	int	slow;			/* Slow device? */
+	int     high;			/* High speed device? */
 	char	mf[32];			/* manufacturer */
 	char	prod[32];		/* product */
 	char	serial[32];		/* serial number */
diff --git a/include/usb_defs.h b/include/usb_defs.h
index 353019f..500a0ad 100644
--- a/include/usb_defs.h
+++ b/include/usb_defs.h
@@ -216,6 +216,7 @@
 #define USB_PORT_STAT_RESET         0x0010
 #define USB_PORT_STAT_POWER         0x0100
 #define USB_PORT_STAT_LOW_SPEED     0x0200
+#define USB_PORT_STAT_HIGH_SPEED    0x0400
 
 /* wPortChange bits */
 #define USB_PORT_STAT_C_CONNECTION  0x0001
-- 
1.5.6

[U-Boot] [PATCH 1/6] usb : musb : Add high speed field in usb_device structure

[Remy Bohmer]

Hello Thomas,

Nice work, but I have a few comments.

2008/12/16 Thomas Abraham <t-abraham@ti.com>:
> This patch adds support for identification of high speed devices.
> For high spped devices that are connected via hubs, the information
> that the device is high speed is recorded. This is required by Mentor
> USB Host controller driver.
>
> Signed-off-by: Ravi Babu <ravibabu@ti.com>
> Signed-off-by: Swaminathan S <swami.iyer@ti.com>
> Signed-off-by: Thomas Abraham <t-abraham@ti.com>
> Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>
> ---
>  common/usb.c       |    1 +
>  include/usb.h      |    1 +
>  include/usb_defs.h |    1 +
>  3 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index ee18152..f8379c9 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1137,6 +1137,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
>        /* Allocate a new device struct for it */
>        usb = usb_alloc_new_device();
>        usb->slow = (portstatus & USB_PORT_STAT_LOW_SPEED) ? 1 : 0;
> +       usb->high = (portstatus & USB_PORT_STAT_HIGH_SPEED) ? 1 : 0;

This patch clashes with the work of  Michael Trimarchi who added high
speed support somewhat different.
Look in the U-boot-usb git tree (next branch) where his patches are
already pending for the next release.
(http://git.denx.de/?p=u-boot/u-boot-usb.git;a=shortlog;h=refs/heads/next)

>        dev->children[port] = usb;
>        usb->parent = dev;
> diff --git a/include/usb.h b/include/usb.h
> index 510df95..e6aa551 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -139,6 +139,7 @@ enum {
>  struct usb_device {
>        int     devnum;                 /* Device number on USB bus */
>        int     slow;                   /* Slow device? */
> +       int     high;                   /* High speed device? */

Same, Michael combined slow+high into 1 single element called 'speed',
what I would prefer here.

Can you please rebase your work on the U-boot-usb tree, 'next' branch?
And streamline these changes with the work Michael already has done?

Kind Regards,

Remy


>        char    mf[32];                 /* manufacturer */
>        char    prod[32];               /* product */
>        char    serial[32];             /* serial number */
> diff --git a/include/usb_defs.h b/include/usb_defs.h
> index 353019f..500a0ad 100644
> --- a/include/usb_defs.h
> +++ b/include/usb_defs.h
> @@ -216,6 +216,7 @@
>  #define USB_PORT_STAT_RESET         0x0010
>  #define USB_PORT_STAT_POWER         0x0100
>  #define USB_PORT_STAT_LOW_SPEED     0x0200
> +#define USB_PORT_STAT_HIGH_SPEED    0x0400
>
>  /* wPortChange bits */
>  #define USB_PORT_STAT_C_CONNECTION  0x0001
> --
> 1.5.6
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH 1/6] usb : musb : Add high speed field in usb_device structure

[Abraham, Thomas]

Hi Remy, 

> -----Original Message-----
> From: l.pinguin at gmail.com [mailto:l.pinguin at gmail.com] On 
> Behalf Of Remy Bohmer
> Sent: Wednesday, December 17, 2008 1:48 AM
> To: Abraham, Thomas
> 

<snip>

> 
> Same, Michael combined slow+high into 1 single element called 
> 'speed', what I would prefer here.
> 
> Can you please rebase your work on the U-boot-usb tree, 'next' branch?
> And streamline these changes with the work Michael already has done?

Ok. I will rebase with u-boot-usb next branch. 

For mentor usb controller, it would still be necessary to record the speed of the device when it is found connected to a hub port. So the update of usb->speed would have to be done as below in 'usb_hub_port_connect_change' function. Would this be acceptable?

 /* Allocate a new device struct for it */
 usb = usb_alloc_new_device();
 usb->speed = (portstatus & USB_PORT_STAT_LOW_SPEED) ? 1 : 0;
+usb->speed = (portstatus & USB_PORT_STAT_HIGH_SPEED) ? 1 : 0;

> 
> Kind Regards,
> 
> Remy

Thanks,
Thomas.

[U-Boot] [PATCH 1/6] usb : musb : Add high speed field in usb_device structure

[Remy Bohmer]

Hello Michael,

> For mentor usb controller, it would still be necessary to record the speed of the device when it is found connected to a hub port. So the update of usb->speed would have to be done as below in 'usb_hub_port_connect_change' function. Would this be acceptable?
>
>  /* Allocate a new device struct for it */
>  usb = usb_alloc_new_device();
>  usb->speed = (portstatus & USB_PORT_STAT_LOW_SPEED) ? 1 : 0;
> +usb->speed = (portstatus & USB_PORT_STAT_HIGH_SPEED) ? 1 : 0;

Would this change break anything you are working on? Or do you have a
better suggestion?

Kind Regards,

Remy

[U-Boot] [PATCH 1/6] usb : musb : Add high speed field in usb_device structure

[michael]

Abraham, Thomas wrote:
> Hi Remy, 
>
>   
>> -----Original Message-----
>> From: l.pinguin at gmail.com [mailto:l.pinguin at gmail.com] On 
>> Behalf Of Remy Bohmer
>> Sent: Wednesday, December 17, 2008 1:48 AM
>> To: Abraham, Thomas
>>
>>     
>
> <snip>
>
>   
>> Same, Michael combined slow+high into 1 single element called 
>> 'speed', what I would prefer here.
>>
>> Can you please rebase your work on the U-boot-usb tree, 'next' branch?
>> And streamline these changes with the work Michael already has done?
>>     
>
> Ok. I will rebase with u-boot-usb next branch. 
>
> For mentor usb controller, it would still be necessary to record the speed of the device when it is found connected to a hub port. So the update of usb->speed would have to be done as below in 'usb_hub_port_connect_change' function. Would this be acceptable?
>
>  /* Allocate a new device struct for it */
>  usb = usb_alloc_new_device();
>  usb->speed = (portstatus & USB_PORT_STAT_LOW_SPEED) ? 1 : 0;
> +usb->speed = (portstatus & USB_PORT_STAT_HIGH_SPEED) ? 1 : 0;
>
>   
I'll send a patch. I think that the correct way to do it is:
+
+       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;
+       else
+               usb->speed = USB_SPEED_FULL;

Like linux does.
>> Kind Regards,
>>
>> Remy
>>     
>
> Thanks,
> Thomas.
>
>
>   
Regards Michael