diff mbox series

usb: hubs: Decrease IN-endpoint poll interval for Microchip USB491x hub

Message ID 20231123081948.58776-1-hgajjar@de.adit-jv.com
State New
Headers
Series usb: hubs: Decrease IN-endpoint poll interval for Microchip USB491x hub |

Commit Message

Hardik Gajjar Nov. 23, 2023, 8:19 a.m. UTC 
  There is a potential delay in announcing downstream USB bus activity to
Linux USB drivers due to the default interrupt endpoint having a poll
interval of 256ms.

Microchip has recommended ignoring the device descriptor and reducing
that value to 32ms, as it was too late to modify it in silicon.

This patch aims to speed up the USB enumeration process, facilitating
the successful completion of Apple CarPlay certifications and enhancing
user experience when utilizing USB devices through the Microchip Multihost
Hub.

A new quirk, USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL, accelerates the
notification process by changing the Endpoint interrupt poll interval
from 256ms to 32ms.

Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++++
 drivers/usb/core/config.c                       |  8 ++++++++
 drivers/usb/core/quirks.c                       | 11 +++++++++++
 include/linux/usb/quirks.h                      |  5 +++++
 4 files changed, 28 insertions(+)

Comments

Alan Stern Nov. 23, 2023, 6:17 p.m. UTC | #1 
On Thu, Nov 23, 2023 at 09:19:48AM +0100, Hardik Gajjar wrote:
> There is a potential delay in announcing downstream USB bus activity to
> Linux USB drivers due to the default interrupt endpoint having a poll
> interval of 256ms.
> 
> Microchip has recommended ignoring the device descriptor and reducing
> that value to 32ms, as it was too late to modify it in silicon.
> 
> This patch aims to speed up the USB enumeration process, facilitating
> the successful completion of Apple CarPlay certifications and enhancing
> user experience when utilizing USB devices through the Microchip Multihost
> Hub.
> 
> A new quirk, USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL, accelerates the
> notification process by changing the Endpoint interrupt poll interval
> from 256ms to 32ms.

But this is meant to apply only to hubs, right?  So shouldn't it be a 
HUB_QUIRK_32_MS_INTR_INTERVAL macro, used in hub.c's hub_id_table, 
rather than a general USB quirk?

> Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>  drivers/usb/core/config.c                       |  8 ++++++++
>  drivers/usb/core/quirks.c                       | 11 +++++++++++
>  include/linux/usb/quirks.h                      |  5 +++++
>  4 files changed, 28 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 65731b060e3f..6b0a66f0e6bf 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6908,6 +6908,10 @@
>  					pause after every control message);
>  				o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra
>  					delay after resetting its port);
> +				p = USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL (Set
> +					bInterval to a Maximum of 9 to Reduce
> +					default Poll Rate from 256 ms to
> +					32 ms);

256 ms and 32 ms are _periods_ (or intervals), not _rates_.

bInterval=9 corresponds to 32 ms only for High Speed and SuperSpeed* 
devices.  For Low and Full Speed it corresponds to 9 ms.  Explanatory 
comments should strive not to be misleading.

Alan Stern
Hardik Gajjar Nov. 24, 2023, 2:50 p.m. UTC | #2 
On Thu, Nov 23, 2023 at 01:17:03PM -0500, Alan Stern wrote:
> On Thu, Nov 23, 2023 at 09:19:48AM +0100, Hardik Gajjar wrote:
> > There is a potential delay in announcing downstream USB bus activity to
> > Linux USB drivers due to the default interrupt endpoint having a poll
> > interval of 256ms.
> > 
> > Microchip has recommended ignoring the device descriptor and reducing
> > that value to 32ms, as it was too late to modify it in silicon.
> > 
> > This patch aims to speed up the USB enumeration process, facilitating
> > the successful completion of Apple CarPlay certifications and enhancing
> > user experience when utilizing USB devices through the Microchip Multihost
> > Hub.
> > 
> > A new quirk, USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL, accelerates the
> > notification process by changing the Endpoint interrupt poll interval
> > from 256ms to 32ms.
> 
> But this is meant to apply only to hubs, right?  So shouldn't it be a 
> HUB_QUIRK_32_MS_INTR_INTERVAL macro, used in hub.c's hub_id_table, 
> rather than a general USB quirk?

Thank you, Alan, for the feedback. To confirm my understanding, are you suggesting
moving all implementations to hub.c, adding the hub-specific quirk, and using the
same quirk to update the bInterval value parsed by usb_get_configuration() in
usb_enumerate_device()?"

> 
> > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  4 ++++
> >  drivers/usb/core/config.c                       |  8 ++++++++
> >  drivers/usb/core/quirks.c                       | 11 +++++++++++
> >  include/linux/usb/quirks.h                      |  5 +++++
> >  4 files changed, 28 insertions(+)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 65731b060e3f..6b0a66f0e6bf 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -6908,6 +6908,10 @@
> >  					pause after every control message);
> >  				o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra
> >  					delay after resetting its port);
> > +				p = USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL (Set
> > +					bInterval to a Maximum of 9 to Reduce
> > +					default Poll Rate from 256 ms to
> > +					32 ms);
> 
> 256 ms and 32 ms are _periods_ (or intervals), not _rates_.
> 
> bInterval=9 corresponds to 32 ms only for High Speed and SuperSpeed* 
> devices.  For Low and Full Speed it corresponds to 9 ms.  Explanatory 
> comments should strive not to be misleading.
> 
> Alan Stern
Alan Stern Nov. 24, 2023, 2:57 p.m. UTC | #3 
On Fri, Nov 24, 2023 at 03:50:05PM +0100, Hardik Gajjar wrote:
> On Thu, Nov 23, 2023 at 01:17:03PM -0500, Alan Stern wrote:
> > On Thu, Nov 23, 2023 at 09:19:48AM +0100, Hardik Gajjar wrote:
> > > There is a potential delay in announcing downstream USB bus activity to
> > > Linux USB drivers due to the default interrupt endpoint having a poll
> > > interval of 256ms.
> > > 
> > > Microchip has recommended ignoring the device descriptor and reducing
> > > that value to 32ms, as it was too late to modify it in silicon.
> > > 
> > > This patch aims to speed up the USB enumeration process, facilitating
> > > the successful completion of Apple CarPlay certifications and enhancing
> > > user experience when utilizing USB devices through the Microchip Multihost
> > > Hub.
> > > 
> > > A new quirk, USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL, accelerates the
> > > notification process by changing the Endpoint interrupt poll interval
> > > from 256ms to 32ms.
> > 
> > But this is meant to apply only to hubs, right?  So shouldn't it be a 
> > HUB_QUIRK_32_MS_INTR_INTERVAL macro, used in hub.c's hub_id_table, 
> > rather than a general USB quirk?
> 
> Thank you, Alan, for the feedback. To confirm my understanding, are you suggesting
> moving all implementations to hub.c, adding the hub-specific quirk, and using the
> same quirk to update the bInterval value parsed by usb_get_configuration() in
> usb_enumerate_device()?"

Basically, yes.  The update should be performed in the hub_probe() 
routine if the quirk flag is set, before hub_configure() is called.  It 
might be necessary to add a usb_set_interface() call as well, in case 
the old bInterval value has already been cached by the host controller 
driver.

Alan Stern
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 65731b060e3f..6b0a66f0e6bf 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6908,6 +6908,10 @@ 
 					pause after every control message);
 				o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra
 					delay after resetting its port);
+				p = USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL (Set
+					bInterval to a Maximum of 9 to Reduce
+					default Poll Rate from 256 ms to
+					32 ms);
 			Example: quirks=0781:5580:bk,0a5c:5834:gij
 
 	usbhid.mousepoll=
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index b19e38d5fd10..4edbb5922872 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -355,6 +355,14 @@  static int usb_parse_endpoint(struct device *ddev, int cfgno,
 				n = clamp(fls(d->bInterval), i, j);
 				i = j = n;
 			}
+
+			/*
+			 * This quirk limits bInterval to 9 (32 ms).
+			 */
+			if (udev->quirks & USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL) {
+				n = clamp(fls(d->bInterval), i, min(j, 9));
+				i = j = n;
+			}
 			break;
 		default:		/* USB_SPEED_FULL or _LOW */
 			/*
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 15e9bd180a1d..243ae5981cc8 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -138,6 +138,9 @@  static int quirks_param_set(const char *value, const struct kernel_param *kp)
 			case 'o':
 				flags |= USB_QUIRK_HUB_SLOW_RESET;
 				break;
+			case 'p':
+				flags |= USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL;
+				break;
 			/* Ignore unrecognized flag characters */
 			}
 		}
@@ -211,6 +214,14 @@  static const struct usb_device_id usb_quirk_list[] = {
 	/* USB3503 */
 	{ USB_DEVICE(0x0424, 0x3503), .driver_info = USB_QUIRK_RESET_RESUME },
 
+	/* USB491x hubs need 32ms instead of 256ms bInterval */
+	{ USB_DEVICE(0x0424, 0x4913), .driver_info =
+			USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL},
+	{ USB_DEVICE(0x0424, 0x4914), .driver_info =
+			USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL},
+	{ USB_DEVICE(0x0424, 0x4915), .driver_info =
+			USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL},
+
 	/* Microsoft Wireless Laser Mouse 6000 Receiver */
 	{ USB_DEVICE(0x045e, 0x00e1), .driver_info = USB_QUIRK_RESET_RESUME },
 
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index eeb7c2157c72..8fbcca4432bf 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,9 @@ 
 /* device has endpoints that should be ignored */
 #define USB_QUIRK_ENDPOINT_IGNORE		BIT(15)
 
+/*
+ * Limits the bInterval value to 9 instead of default value 16
+ */
+#define USB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL	BIT(16)
+
 #endif /* __LINUX_USB_QUIRKS_H */