[PATCH 1/2] USB: xhci: rename ambiguous named XHCI_NEC_HOST to XHCI_NEC_SHOW_FW

[PATCH 1/2] USB: xhci: rename ambiguous named XHCI_NEC_HOST to XHCI_NEC_SHOW_FW

[Alexander Holler]

Current Renesas Electronics XHCI hosts (which were formerly NEC)
do support the same vendor command to show the firmware. Rename the
ambigious named define XHCI_NEC_HOST to XHCI_NEC_SHOW_FW because it's
only used to display the firmware version. Besides that, change the
output "... NEC firmware version x.y" to "... firmware version x.y"
to not confuse owners of Renesas USB hosts.

(so only cosmetic, no functional changes)

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/usb/host/xhci-pci.c  | 2 +-
 drivers/usb/host/xhci-ring.c | 6 +++---
 drivers/usb/host/xhci.c      | 4 ++--
 drivers/usb/host/xhci.h      | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1a30c38..f47d3d2 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -79,7 +79,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	}
 
 	if (pdev->vendor == PCI_VENDOR_ID_NEC)
-		xhci->quirks |= XHCI_NEC_HOST;
+		xhci->quirks |= XHCI_NEC_SHOW_FW;
 
 	if (pdev->vendor == PCI_VENDOR_ID_AMD && xhci->hci_version == 0x96)
 		xhci->quirks |= XHCI_AMD_0x96_HOST;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1969c00..761d566 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1489,11 +1489,11 @@ bandwidth_change:
 					"for disabled slot %u\n", slot_id);
 		break;
 	case TRB_TYPE(TRB_NEC_GET_FW):
-		if (!(xhci->quirks & XHCI_NEC_HOST)) {
+		if (!(xhci->quirks & XHCI_NEC_SHOW_FW)) {
 			xhci->error_bitmask |= 1 << 6;
 			break;
 		}
-		xhci_dbg(xhci, "NEC firmware version %2x.%02x\n",
+		xhci_dbg(xhci, "firmware version %2x.%02x\n",
 			 NEC_FW_MAJOR(le32_to_cpu(event->status)),
 			 NEC_FW_MINOR(le32_to_cpu(event->status)));
 		break;
@@ -1512,7 +1512,7 @@ static void handle_vendor_event(struct xhci_hcd *xhci,
 
 	trb_type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->generic.field[3]));
 	xhci_dbg(xhci, "Vendor specific event TRB type = %u\n", trb_type);
-	if (trb_type == TRB_NEC_CMD_COMP && (xhci->quirks & XHCI_NEC_HOST))
+	if (trb_type == TRB_NEC_CMD_COMP && (xhci->quirks & XHCI_NEC_SHOW_FW))
 		handle_cmd_completion(xhci, &event->event_cmd);
 }
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b4aa79d..b961e9a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -588,7 +588,7 @@ static int xhci_run_finished(struct xhci_hcd *xhci)
 	xhci->shared_hcd->state = HC_STATE_RUNNING;
 	xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
 
-	if (xhci->quirks & XHCI_NEC_HOST)
+	if (xhci->quirks & XHCI_NEC_SHOW_FW)
 		xhci_ring_cmd_db(xhci);
 
 	xhci_dbg(xhci, "Finished xhci_run for USB3 roothub\n");
@@ -673,7 +673,7 @@ int xhci_run(struct usb_hcd *hcd)
 			&xhci->ir_set->irq_pending);
 	xhci_print_ir_set(xhci, 0);
 
-	if (xhci->quirks & XHCI_NEC_HOST)
+	if (xhci->quirks & XHCI_NEC_SHOW_FW)
 		xhci_queue_vendor_command(xhci, 0, 0, 0,
 				TRB_TYPE(TRB_NEC_GET_FW));
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 29c978e..e483f12 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1493,7 +1493,7 @@ struct xhci_hcd {
 	unsigned int		quirks;
 #define	XHCI_LINK_TRB_QUIRK	(1 << 0)
 #define XHCI_RESET_EP_QUIRK	(1 << 1)
-#define XHCI_NEC_HOST		(1 << 2)
+#define XHCI_NEC_SHOW_FW	(1 << 2)
 #define XHCI_AMD_PLL_FIX	(1 << 3)
 #define XHCI_SPURIOUS_SUCCESS	(1 << 4)
 /*
-- 
1.8.1.5

[PATCH 2/2] USB: xhci: show firmware version for Renesas XHCI hosts if debug is enabled

[Alexander Holler]

They support the same vendor specific command as former NEC hosts.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/usb/host/xhci-pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index f47d3d2..9977cee 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -78,7 +78,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 	}
 
-	if (pdev->vendor == PCI_VENDOR_ID_NEC)
+	if (pdev->vendor == PCI_VENDOR_ID_NEC ||
+			pdev->vendor == PCI_VENDOR_ID_RENESAS)
 		xhci->quirks |= XHCI_NEC_SHOW_FW;
 
 	if (pdev->vendor == PCI_VENDOR_ID_AMD && xhci->hci_version == 0x96)
-- 
1.8.1.5

Re: [PATCH 1/2] USB: xhci: rename ambiguous named XHCI_NEC_HOST to XHCI_NEC_SHOW_FW

[Sarah Sharp]

On Wed, May 29, 2013 at 11:14:32PM +0200, Alexander Holler wrote:
> Current Renesas Electronics XHCI hosts (which were formerly NEC)
> do support the same vendor command to show the firmware. Rename the
> ambigious named define XHCI_NEC_HOST to XHCI_NEC_SHOW_FW because it's
> only used to display the firmware version. Besides that, change the
> output "... NEC firmware version x.y" to "... firmware version x.y"
> to not confuse owners of Renesas USB hosts.
> 
> (so only cosmetic, no functional changes)

I'm actually inclined to say you should just rip out the firmware
version code entirely.  I haven't needed to use it for years, and if
Renesas changed their vendor command set, I would rather not submit
random commands to the host.

So, can you redo this patch to just rip out XHCI_NEC_HOST and everything
that uses it?

Sarah Sharp

> 
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> ---
>  drivers/usb/host/xhci-pci.c  | 2 +-
>  drivers/usb/host/xhci-ring.c | 6 +++---
>  drivers/usb/host/xhci.c      | 4 ++--
>  drivers/usb/host/xhci.h      | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 1a30c38..f47d3d2 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -79,7 +79,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  	}
>  
>  	if (pdev->vendor == PCI_VENDOR_ID_NEC)
> -		xhci->quirks |= XHCI_NEC_HOST;
> +		xhci->quirks |= XHCI_NEC_SHOW_FW;
>  
>  	if (pdev->vendor == PCI_VENDOR_ID_AMD && xhci->hci_version == 0x96)
>  		xhci->quirks |= XHCI_AMD_0x96_HOST;
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1969c00..761d566 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1489,11 +1489,11 @@ bandwidth_change:
>  					"for disabled slot %u\n", slot_id);
>  		break;
>  	case TRB_TYPE(TRB_NEC_GET_FW):
> -		if (!(xhci->quirks & XHCI_NEC_HOST)) {
> +		if (!(xhci->quirks & XHCI_NEC_SHOW_FW)) {
>  			xhci->error_bitmask |= 1 << 6;
>  			break;
>  		}
> -		xhci_dbg(xhci, "NEC firmware version %2x.%02x\n",
> +		xhci_dbg(xhci, "firmware version %2x.%02x\n",
>  			 NEC_FW_MAJOR(le32_to_cpu(event->status)),
>  			 NEC_FW_MINOR(le32_to_cpu(event->status)));
>  		break;
> @@ -1512,7 +1512,7 @@ static void handle_vendor_event(struct xhci_hcd *xhci,
>  
>  	trb_type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->generic.field[3]));
>  	xhci_dbg(xhci, "Vendor specific event TRB type = %u\n", trb_type);
> -	if (trb_type == TRB_NEC_CMD_COMP && (xhci->quirks & XHCI_NEC_HOST))
> +	if (trb_type == TRB_NEC_CMD_COMP && (xhci->quirks & XHCI_NEC_SHOW_FW))
>  		handle_cmd_completion(xhci, &event->event_cmd);
>  }
>  
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index b4aa79d..b961e9a 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -588,7 +588,7 @@ static int xhci_run_finished(struct xhci_hcd *xhci)
>  	xhci->shared_hcd->state = HC_STATE_RUNNING;
>  	xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
>  
> -	if (xhci->quirks & XHCI_NEC_HOST)
> +	if (xhci->quirks & XHCI_NEC_SHOW_FW)
>  		xhci_ring_cmd_db(xhci);
>  
>  	xhci_dbg(xhci, "Finished xhci_run for USB3 roothub\n");
> @@ -673,7 +673,7 @@ int xhci_run(struct usb_hcd *hcd)
>  			&xhci->ir_set->irq_pending);
>  	xhci_print_ir_set(xhci, 0);
>  
> -	if (xhci->quirks & XHCI_NEC_HOST)
> +	if (xhci->quirks & XHCI_NEC_SHOW_FW)
>  		xhci_queue_vendor_command(xhci, 0, 0, 0,
>  				TRB_TYPE(TRB_NEC_GET_FW));
>  
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 29c978e..e483f12 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1493,7 +1493,7 @@ struct xhci_hcd {
>  	unsigned int		quirks;
>  #define	XHCI_LINK_TRB_QUIRK	(1 << 0)
>  #define XHCI_RESET_EP_QUIRK	(1 << 1)
> -#define XHCI_NEC_HOST		(1 << 2)
> +#define XHCI_NEC_SHOW_FW	(1 << 2)
>  #define XHCI_AMD_PLL_FIX	(1 << 3)
>  #define XHCI_SPURIOUS_SUCCESS	(1 << 4)
>  /*
> -- 
> 1.8.1.5
> 

Re: [PATCH 1/2] USB: xhci: rename ambiguous named XHCI_NEC_HOST to XHCI_NEC_SHOW_FW

[Alexander Holler]

Am 30.05.2013 00:25, schrieb Sarah Sharp:
> 
> On Wed, May 29, 2013 at 11:14:32PM +0200, Alexander Holler wrote:
>> Current Renesas Electronics XHCI hosts (which were formerly NEC)
>> do support the same vendor command to show the firmware. Rename the
>> ambigious named define XHCI_NEC_HOST to XHCI_NEC_SHOW_FW because it's
>> only used to display the firmware version. Besides that, change the
>> output "... NEC firmware version x.y" to "... firmware version x.y"
>> to not confuse owners of Renesas USB hosts.
>>
>> (so only cosmetic, no functional changes)
> 
> I'm actually inclined to say you should just rip out the firmware
> version code entirely.  I haven't needed to use it for years, and if
> Renesas changed their vendor command set, I would rather not submit
> random commands to the host.
> 
> So, can you redo this patch to just rip out XHCI_NEC_HOST and everything
> that uses it?

Hmm, I find the firmware version rather informational and would even
display it unconditionally (instead of with xhci_debug). It prevents the
need to boot Windows to checkout if the latest version is installed,
especially if someone is hunting a bug.

I just dont't like the name, because e.g. in my case, it made me to have
a deeper look at what that quirk does, because I had the hope it might
solve a problem. Therefor I think it's useful to rename it.

Regards,

Alexander Holler

Re: [PATCH 1/2] USB: xhci: rename ambiguous named XHCI_NEC_HOST to XHCI_NEC_SHOW_FW

[Alexander Holler]

On Thu, May 30, 2013 at 06:16:34AM +0200, Alexander Holler wrote:
> Am 30.05.2013 00:25, schrieb Sarah Sharp:
> > 
> > On Wed, May 29, 2013 at 11:14:32PM +0200, Alexander Holler wrote:
> >> Current Renesas Electronics XHCI hosts (which were formerly NEC)
> >> do support the same vendor command to show the firmware. Rename the
> >> ambigious named define XHCI_NEC_HOST to XHCI_NEC_SHOW_FW because it's
> >> only used to display the firmware version. Besides that, change the
> >> output "... NEC firmware version x.y" to "... firmware version x.y"
> >> to not confuse owners of Renesas USB hosts.
> >>
> >> (so only cosmetic, no functional changes)
> > 
> > I'm actually inclined to say you should just rip out the firmware
> > version code entirely.  I haven't needed to use it for years, and if
> > Renesas changed their vendor command set, I would rather not submit
> > random commands to the host.
> > 
> > So, can you redo this patch to just rip out XHCI_NEC_HOST and everything
> > that uses it?
> 
> Hmm, I find the firmware version rather informational and would even
> display it unconditionally (instead of with xhci_debug). It prevents the
> need to boot Windows to checkout if the latest version is installed,
> especially if someone is hunting a bug.

Below is the patch I'm using locally on top of the two previous patches.
Feel free to use/submit/merge it too.

Regards,

Alexander Holler


>From 9bf9e555600626b87371a8825c2557bdf5621e76 Mon Sep 17 00:00:00 2001
From: Alexander Holler <holler@ahsoftware.de>
Date: Thu, 30 May 2013 18:04:14 +0200
Subject: [PATCH] USB: xhci: always show the firmware version for NEC/Renesas
 hosts

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/usb/host/xhci-ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 761d566..8873b3e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1493,7 +1493,7 @@ bandwidth_change:
 			xhci->error_bitmask |= 1 << 6;
 			break;
 		}
-		xhci_dbg(xhci, "firmware version %2x.%02x\n",
+		dev_info(xhci_to_hcd(xhci)->self.controller, "firmware version %2x.%02x\n",
 			 NEC_FW_MAJOR(le32_to_cpu(event->status)),
 			 NEC_FW_MINOR(le32_to_cpu(event->status)));
 		break;
-- 
1.8.1.4

Re: [PATCH 1/2] USB: xhci: rename ambiguous named XHCI_NEC_HOST to XHCI_NEC_SHOW_FW

[Sarah Sharp]

On Thu, May 30, 2013 at 06:16:34AM +0200, Alexander Holler wrote:
> Am 30.05.2013 00:25, schrieb Sarah Sharp:
> > 
> > On Wed, May 29, 2013 at 11:14:32PM +0200, Alexander Holler wrote:
> >> Current Renesas Electronics XHCI hosts (which were formerly NEC)
> >> do support the same vendor command to show the firmware. Rename the
> >> ambigious named define XHCI_NEC_HOST to XHCI_NEC_SHOW_FW because it's
> >> only used to display the firmware version. Besides that, change the
> >> output "... NEC firmware version x.y" to "... firmware version x.y"
> >> to not confuse owners of Renesas USB hosts.
> >>
> >> (so only cosmetic, no functional changes)
> > 
> > I'm actually inclined to say you should just rip out the firmware
> > version code entirely.  I haven't needed to use it for years, and if
> > Renesas changed their vendor command set, I would rather not submit
> > random commands to the host.
> > 
> > So, can you redo this patch to just rip out XHCI_NEC_HOST and everything
> > that uses it?
> 
> Hmm, I find the firmware version rather informational and would even
> display it unconditionally (instead of with xhci_debug). It prevents the
> need to boot Windows to checkout if the latest version is installed,
> especially if someone is hunting a bug.

Right, but we need to stop sending commands to Renesas hosts that don't
support this command.  We don't know what that command does in the hosts
that don't support the firmware version command.  For all we know, we
could be setting the host into a debugging mode, or asking it to only
report USB 2.0 device connects, or other things that I can't imagine.

The point is that unless Renesas tells us how to know if a host
supports the firmware fetch vendor command, we should stop issuing that
command to the host.  I think my contacts at Renesas have moved onto
other jobs, but maybe you know someone there?

> I just dont't like the name, because e.g. in my case, it made me to have
> a deeper look at what that quirk does, because I had the hope it might
> solve a problem. Therefor I think it's useful to rename it.

I understand.  If the command worked fine on all Renesas hosts, I would
be fine with renaming it and printing it with dev_info instead of
xhci_dbg.  However, since some Renesas hosts don't support the command,
I'm concerned we may be forced to rip out the code.  If you don't do it,
I will have to.

Sarah Sharp

Re: [PATCH 1/2] USB: xhci: rename ambiguous named XHCI_NEC_HOST to XHCI_NEC_SHOW_FW

[Alexander Holler]

Am 30.05.2013 20:20, schrieb Sarah Sharp:
> On Thu, May 30, 2013 at 06:16:34AM +0200, Alexander Holler wrote:

> The point is that unless Renesas tells us how to know if a host
> supports the firmware fetch vendor command, we should stop issuing that
> command to the host.  I think my contacts at Renesas have moved onto
> other jobs, but maybe you know someone there?

No, sorry.

>> I just dont't like the name, because e.g. in my case, it made me to have
>> a deeper look at what that quirk does, because I had the hope it might
>> solve a problem. Therefor I think it's useful to rename it.
> 
> I understand.  If the command worked fine on all Renesas hosts, I would
> be fine with renaming it and printing it with dev_info instead of
> xhci_dbg.  However, since some Renesas hosts don't support the command,
> I'm concerned we may be forced to rip out the code.  If you don't do it,
> I will have to.

I don't want to do it as it works fine on my Renesas upd720202.

Instead of removing this feature completely, you could just use the
first patch and forget the second one. So only NEC hosts will still have
it and I assume there will not any new appear, as they are now Renesas.

Another possibility would be to use the device ID too (for Renesas
devices), mine has 0x0015.

The reason why I really like this feature is that there were already 2-3
firmware updates and since USB 3.0 still isn't that widely used, I
assume more will appear if more people actually start using such devices.

Regards,

Alexander Holler