[PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[Aman Deep]

xhci-hub used some numerical values for initialisation of root hub
descriptors. #define values are addded in usb 2.0 hub specification
file and these values are used for root hub characteristics
initialisation.

Also use some #defines in places where magic numbers are being used.

Signed-off-by: Aman Deep <amandeep3986@gmail.com>
---
 drivers/usb/host/xhci-hub.c |   18 ++++++++----------
 include/linux/usb/ch11.h    |   19 ++++++++++++++-----
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 430e88f..35e257f 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -57,17 +57,15 @@ static void xhci_common_hub_descriptor(struct xhci_hcd *xhci,
 	desc->bHubContrCurrent = 0;
 
 	desc->bNbrPorts = ports;
-	/* Ugh, these should be #defines, FIXME */
-	/* Using table 11-13 in USB 2.0 spec. */
 	temp = 0;
-	/* Bits 1:0 - support port power switching, or power always on */
+	/* Bits 1:0 - support per-port power switching, or power always on */
 	if (HCC_PPC(xhci->hcc_params))
-		temp |= 0x0001;
+		temp |= HUB_CHAR_INDV_PORT_LPSM;
 	else
-		temp |= 0x0002;
+		temp |= HUB_CHAR_NO_LPSM;
 	/* Bit  2 - root hubs are not part of a compound device */
 	/* Bits 4:3 - individual port over current protection */
-	temp |= 0x0008;
+	temp |= HUB_CHAR_INDV_PORT_OCPM;
 	/* Bits 6:5 - no TTs in root ports */
 	/* Bit  7 - no port indicators */
 	desc->wHubCharacteristics = cpu_to_le16(temp);
@@ -86,9 +84,9 @@ static void xhci_usb2_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
 	ports = xhci->num_usb2_ports;
 
 	xhci_common_hub_descriptor(xhci, desc, ports);
-	desc->bDescriptorType = 0x29;
+	desc->bDescriptorType = USB_DT_HUB;
 	temp = 1 + (ports / 8);
-	desc->bDescLength = 7 + 2 * temp;
+	desc->bDescLength = USB_DT_HUB_NONVAR_SIZE + 2 * temp;
 
 	/* The Device Removable bits are reported on a byte granularity.
 	 * If the port doesn't exist within that byte, the bit is set to 0.
@@ -137,8 +135,8 @@ static void xhci_usb3_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
 
 	ports = xhci->num_usb3_ports;
 	xhci_common_hub_descriptor(xhci, desc, ports);
-	desc->bDescriptorType = 0x2a;
-	desc->bDescLength = 12;
+	desc->bDescriptorType = USB_DT_SS_HUB;
+	desc->bDescLength = USB_DT_SS_HUB_SIZE;
 
 	/* header decode latency should be zero for roothubs,
 	 * see section 4.23.5.2.
diff --git a/include/linux/usb/ch11.h b/include/linux/usb/ch11.h
index 4ebaf08..537f354 100644
--- a/include/linux/usb/ch11.h
+++ b/include/linux/usb/ch11.h
@@ -165,11 +165,20 @@ struct usb_port_status {
  * wHubCharacteristics (masks)
  * See USB 2.0 spec Table 11-13, offset 3
  */
-#define HUB_CHAR_LPSM		0x0003 /* D1 .. D0 */
-#define HUB_CHAR_COMPOUND	0x0004 /* D2       */
-#define HUB_CHAR_OCPM		0x0018 /* D4 .. D3 */
-#define HUB_CHAR_TTTT           0x0060 /* D6 .. D5 */
-#define HUB_CHAR_PORTIND        0x0080 /* D7       */
+#define HUB_CHAR_LPSM		0x0003 /* Logical Power Switching Mode mask */
+#define HUB_CHAR_COMMON_LPSM	0x0000 /* All ports power control at once */
+#define HUB_CHAR_INDV_PORT_LPSM	0x0001 /* per-port power control */
+#define HUB_CHAR_NO_LPSM	0x0002 /* no power switching */
+
+#define HUB_CHAR_COMPOUND	0x0004 /* hub is part of a compound device */
+
+#define HUB_CHAR_OCPM		0x0018 /* Over-Current Protection Mode mask */
+#define HUB_CHAR_COMMON_OCPM	0x0000 /* All ports Over-Current reporting */
+#define HUB_CHAR_INDV_PORT_OCPM	0x0008 /* per-port Over-current reporting */
+#define HUB_CHAR_NO_OCPM	0x0010 /* No Over-current Protection support */
+
+#define HUB_CHAR_TTTT           0x0060 /* TT Think Time mask */
+#define HUB_CHAR_PORTIND        0x0080 /* per-port indicators (LEDs) */
 
 struct usb_hub_status {
 	__le16 wHubStatus;
-- 
1.7.6

Re: [PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[Alan Stern]

On Tue, 22 Nov 2011, Aman Deep wrote:

> xhci-hub used some numerical values for initialisation of root hub
> descriptors. #define values are addded in usb 2.0 hub specification
> file and these values are used for root hub characteristics
> initialisation.
> 
> Also use some #defines in places where magic numbers are being used.

This is good.  Can you also add similar changes to hub_configure() in 
drivers/usb/core/hub.c?

Alan Stern

Re: [PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[Sarah Sharp]

On Tue, Nov 22, 2011 at 07:33:36PM +0530, Aman Deep wrote:
> xhci-hub used some numerical values for initialisation of root hub
> descriptors. #define values are addded in usb 2.0 hub specification
> file and these values are used for root hub characteristics
> initialisation.
> 
> Also use some #defines in places where magic numbers are being used.

Looks fine to me for the xHCI changes.  I copied most of the hub
descriptor code from EHCI, so it will need the same treatment.

Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>

The #defines look a bit odd to me, like you're using spaces instead of
tabs to separate the name and value, or your mail program is converting
tabs to spaces.  Do the numbers line up for you?

> Signed-off-by: Aman Deep <amandeep3986@gmail.com>
> ---
>  drivers/usb/host/xhci-hub.c |   18 ++++++++----------
>  include/linux/usb/ch11.h    |   19 ++++++++++++++-----
>  2 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 430e88f..35e257f 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -57,17 +57,15 @@ static void xhci_common_hub_descriptor(struct xhci_hcd *xhci,
>  	desc->bHubContrCurrent = 0;
>  
>  	desc->bNbrPorts = ports;
> -	/* Ugh, these should be #defines, FIXME */
> -	/* Using table 11-13 in USB 2.0 spec. */
>  	temp = 0;
> -	/* Bits 1:0 - support port power switching, or power always on */
> +	/* Bits 1:0 - support per-port power switching, or power always on */
>  	if (HCC_PPC(xhci->hcc_params))
> -		temp |= 0x0001;
> +		temp |= HUB_CHAR_INDV_PORT_LPSM;
>  	else
> -		temp |= 0x0002;
> +		temp |= HUB_CHAR_NO_LPSM;
>  	/* Bit  2 - root hubs are not part of a compound device */
>  	/* Bits 4:3 - individual port over current protection */
> -	temp |= 0x0008;
> +	temp |= HUB_CHAR_INDV_PORT_OCPM;
>  	/* Bits 6:5 - no TTs in root ports */
>  	/* Bit  7 - no port indicators */
>  	desc->wHubCharacteristics = cpu_to_le16(temp);
> @@ -86,9 +84,9 @@ static void xhci_usb2_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
>  	ports = xhci->num_usb2_ports;
>  
>  	xhci_common_hub_descriptor(xhci, desc, ports);
> -	desc->bDescriptorType = 0x29;
> +	desc->bDescriptorType = USB_DT_HUB;
>  	temp = 1 + (ports / 8);
> -	desc->bDescLength = 7 + 2 * temp;
> +	desc->bDescLength = USB_DT_HUB_NONVAR_SIZE + 2 * temp;
>  
>  	/* The Device Removable bits are reported on a byte granularity.
>  	 * If the port doesn't exist within that byte, the bit is set to 0.
> @@ -137,8 +135,8 @@ static void xhci_usb3_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
>  
>  	ports = xhci->num_usb3_ports;
>  	xhci_common_hub_descriptor(xhci, desc, ports);
> -	desc->bDescriptorType = 0x2a;
> -	desc->bDescLength = 12;
> +	desc->bDescriptorType = USB_DT_SS_HUB;
> +	desc->bDescLength = USB_DT_SS_HUB_SIZE;
>  
>  	/* header decode latency should be zero for roothubs,
>  	 * see section 4.23.5.2.
> diff --git a/include/linux/usb/ch11.h b/include/linux/usb/ch11.h
> index 4ebaf08..537f354 100644
> --- a/include/linux/usb/ch11.h
> +++ b/include/linux/usb/ch11.h
> @@ -165,11 +165,20 @@ struct usb_port_status {
>   * wHubCharacteristics (masks)
>   * See USB 2.0 spec Table 11-13, offset 3
>   */
> -#define HUB_CHAR_LPSM		0x0003 /* D1 .. D0 */
> -#define HUB_CHAR_COMPOUND	0x0004 /* D2       */
> -#define HUB_CHAR_OCPM		0x0018 /* D4 .. D3 */
> -#define HUB_CHAR_TTTT           0x0060 /* D6 .. D5 */
> -#define HUB_CHAR_PORTIND        0x0080 /* D7       */
> +#define HUB_CHAR_LPSM		0x0003 /* Logical Power Switching Mode mask */
> +#define HUB_CHAR_COMMON_LPSM	0x0000 /* All ports power control at once */
> +#define HUB_CHAR_INDV_PORT_LPSM	0x0001 /* per-port power control */
> +#define HUB_CHAR_NO_LPSM	0x0002 /* no power switching */
> +
> +#define HUB_CHAR_COMPOUND	0x0004 /* hub is part of a compound device */
> +
> +#define HUB_CHAR_OCPM		0x0018 /* Over-Current Protection Mode mask */
> +#define HUB_CHAR_COMMON_OCPM	0x0000 /* All ports Over-Current reporting */
> +#define HUB_CHAR_INDV_PORT_OCPM	0x0008 /* per-port Over-current reporting */
> +#define HUB_CHAR_NO_OCPM	0x0010 /* No Over-current Protection support */
> +
> +#define HUB_CHAR_TTTT           0x0060 /* TT Think Time mask */
> +#define HUB_CHAR_PORTIND        0x0080 /* per-port indicators (LEDs) */
>  
>  struct usb_hub_status {
>  	__le16 wHubStatus;
> -- 
> 1.7.6
> 

Re: [PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[Aman Deep]

Hi Sarah,


> >
> > Also use some #defines in places where magic numbers are being used.
>
> Looks fine to me for the xHCI changes.  I copied most of the hub
> descriptor code from EHCI, so it will need the same treatment.

Okay, thanks. I will try to make some of these changes into other host
controller drivers also as
soon as I can.

>
> Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>

Thanks for the ACK, Sarah.

>
> The #defines look a bit odd to me, like you're using spaces instead of
> tabs to separate the name and value, or your mail program is converting
> tabs to spaces.  Do the numbers line up for you?

Actually, both the HUB_CHAR_TTTT and HUB_CHAR_PORTIND were already using
spaces instead of tabs. I just copied them to the lines below, thats
why they still have
spaces between define and value.

But thanks for info. I will take more care about this in future.

Do you want me to make any changes in this patch ?

>
> > Signed-off-by: Aman Deep <amandeep3986@gmail.com>
> > ---

Re: [PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[Aman Deep]

Hi Alan,

>
> This is good.  Can you also add similar changes to hub_configure() in
> drivers/usb/core/hub.c?
>

Thanks, Alan. I will try to make similar changes into hub_configure()
and other host
controller drivers also.

- Aman

Re: [PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[Sarah Sharp]

On Wed, Nov 23, 2011 at 10:20:10AM +0530, Aman Deep wrote:
> Hi Sarah,
> 
> 
> > >
> > > Also use some #defines in places where magic numbers are being used.
> >
> > Looks fine to me for the xHCI changes.  I copied most of the hub
> > descriptor code from EHCI, so it will need the same treatment.
> 
> Okay, thanks. I will try to make some of these changes into other host
> controller drivers also as
> soon as I can.
> 
> >
> > Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> 
> Thanks for the ACK, Sarah.
> 
> >
> > The #defines look a bit odd to me, like you're using spaces instead of
> > tabs to separate the name and value, or your mail program is converting
> > tabs to spaces.  Do the numbers line up for you?
> 
> Actually, both the HUB_CHAR_TTTT and HUB_CHAR_PORTIND were already using
> spaces instead of tabs. I just copied them to the lines below, thats
> why they still have
> spaces between define and value.
> 
> But thanks for info. I will take more care about this in future.
> 
> Do you want me to make any changes in this patch ?

Fixing them up to have tabs instead of spaces would be nice, but is not
required. :)

Sarah Sharp

Re: [PATCH 1/1] xHCI: Adding #define values used for hub descriptor

[Greg KH]

On Wed, Nov 23, 2011 at 09:00:06AM -0800, Sarah Sharp wrote:
> On Wed, Nov 23, 2011 at 10:20:10AM +0530, Aman Deep wrote:
> > Hi Sarah,
> > 
> > 
> > > >
> > > > Also use some #defines in places where magic numbers are being used.
> > >
> > > Looks fine to me for the xHCI changes.  I copied most of the hub
> > > descriptor code from EHCI, so it will need the same treatment.
> > 
> > Okay, thanks. I will try to make some of these changes into other host
> > controller drivers also as
> > soon as I can.
> > 
> > >
> > > Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> > 
> > Thanks for the ACK, Sarah.
> > 
> > >
> > > The #defines look a bit odd to me, like you're using spaces instead of
> > > tabs to separate the name and value, or your mail program is converting
> > > tabs to spaces.  Do the numbers line up for you?
> > 
> > Actually, both the HUB_CHAR_TTTT and HUB_CHAR_PORTIND were already using
> > spaces instead of tabs. I just copied them to the lines below, thats
> > why they still have
> > spaces between define and value.
> > 
> > But thanks for info. I will take more care about this in future.
> > 
> > Do you want me to make any changes in this patch ?
> 
> Fixing them up to have tabs instead of spaces would be nice, but is not
> required. :)

I fixed them up by hand, so don't worry about it.

greg k-h