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

2013-05-30 Thread 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 hol...@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 hol...@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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-05-30 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-05-30 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-05-29 Thread 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 hol...@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 intquirks;
 #defineXHCI_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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-05-29 Thread 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 hol...@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 intquirks;
  #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
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-05-29 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html