diff mbox series

[v3] usb: hub: Add quirk to decrease IN-ep poll interval for Microchip USB491x hub

Message ID 20231201144705.97385-1-hgajjar@de.adit-jv.com (mailing list archive)
State Superseded
Headers show
Series [v3] usb: hub: Add quirk to decrease IN-ep poll interval for Microchip USB491x hub | expand

Commit Message

Hardik Gajjar Dec. 1, 2023, 2:47 p.m. UTC 
There is a potential delay in notifying Linux USB drivers of downstream
USB bus activity when connecting a high-speed or superSpeed device via the
Microchip USB491x hub. This delay is due to the fixed bInterval value of
12 in the silicon of the Microchip USB491x hub.

Microchip requested to ignore the device descriptor and decrease that
value to 9 as it was too late to modify that in silicon.

This patch speeds up the USB enummeration process that helps to pass
Apple Carplay certifications and improve the User experience when utilizing
the USB device via Microchip Multihost USB491x Hub.

A new hub quirk HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL speeds up
the notification process for Microchip USB491x hub by limiting
the maximum bInterval value to 9.

Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
---
changes since version 1:
	- Move implementation from config.c and quirk.c to hub.c as this is hub
	  specific changes.
	- Improve commit message.
	- Link to v1 - https://lore.kernel.org/all/20231123081948.58776-1-hgajjar@de.adit-jv.com/

changes since version 2:
    - Call usb_set_interface after updating the bInterval to Tell the HCD about modification
	- Link to v2 - https://lore.kernel.org/all/20231130084855.119937-1-hgajjar@de.adit-jv.com/
---
 drivers/usb/core/hub.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Alan Stern Dec. 1, 2023, 4:13 p.m. UTC | #1 
On Fri, Dec 01, 2023 at 03:47:05PM +0100, Hardik Gajjar wrote:
> There is a potential delay in notifying Linux USB drivers of downstream
> USB bus activity when connecting a high-speed or superSpeed device via the
> Microchip USB491x hub. This delay is due to the fixed bInterval value of
> 12 in the silicon of the Microchip USB491x hub.
> 
> Microchip requested to ignore the device descriptor and decrease that
> value to 9 as it was too late to modify that in silicon.
> 
> This patch speeds up the USB enummeration process that helps to pass
> Apple Carplay certifications and improve the User experience when utilizing
> the USB device via Microchip Multihost USB491x Hub.
> 
> A new hub quirk HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL speeds up
> the notification process for Microchip USB491x hub by limiting
> the maximum bInterval value to 9.
> 
> Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> ---
> changes since version 1:
> 	- Move implementation from config.c and quirk.c to hub.c as this is hub
> 	  specific changes.
> 	- Improve commit message.
> 	- Link to v1 - https://lore.kernel.org/all/20231123081948.58776-1-hgajjar@de.adit-jv.com/
> 
> changes since version 2:
>     - Call usb_set_interface after updating the bInterval to Tell the HCD about modification
> 	- Link to v2 - https://lore.kernel.org/all/20231130084855.119937-1-hgajjar@de.adit-jv.com/
> ---
>  drivers/usb/core/hub.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index b4584a0cd484..b5ac29c5f016 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -47,12 +47,18 @@
>  #define USB_VENDOR_TEXAS_INSTRUMENTS		0x0451
>  #define USB_PRODUCT_TUSB8041_USB3		0x8140
>  #define USB_PRODUCT_TUSB8041_USB2		0x8142
> +#define USB_VENDOR_MICROCHIP			0x0424
> +#define USB_PRODUCT_USB4913			0x4913
> +#define USB_PRODUCT_USB4914			0x4914
> +#define USB_PRODUCT_USB4915			0x4915
>  #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
>  #define HUB_QUIRK_DISABLE_AUTOSUSPEND		0x02
> +#define HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL	0x08

Why use 0x08 instead of 0x04?

Alan Stern
Greg KH Dec. 4, 2023, 7:02 a.m. UTC | #2 
On Fri, Dec 01, 2023 at 11:13:53AM -0500, Alan Stern wrote:
> On Fri, Dec 01, 2023 at 03:47:05PM +0100, Hardik Gajjar wrote:
> > There is a potential delay in notifying Linux USB drivers of downstream
> > USB bus activity when connecting a high-speed or superSpeed device via the
> > Microchip USB491x hub. This delay is due to the fixed bInterval value of
> > 12 in the silicon of the Microchip USB491x hub.
> > 
> > Microchip requested to ignore the device descriptor and decrease that
> > value to 9 as it was too late to modify that in silicon.
> > 
> > This patch speeds up the USB enummeration process that helps to pass
> > Apple Carplay certifications and improve the User experience when utilizing
> > the USB device via Microchip Multihost USB491x Hub.
> > 
> > A new hub quirk HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL speeds up
> > the notification process for Microchip USB491x hub by limiting
> > the maximum bInterval value to 9.
> > 
> > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> > ---
> > changes since version 1:
> > 	- Move implementation from config.c and quirk.c to hub.c as this is hub
> > 	  specific changes.
> > 	- Improve commit message.
> > 	- Link to v1 - https://lore.kernel.org/all/20231123081948.58776-1-hgajjar@de.adit-jv.com/
> > 
> > changes since version 2:
> >     - Call usb_set_interface after updating the bInterval to Tell the HCD about modification
> > 	- Link to v2 - https://lore.kernel.org/all/20231130084855.119937-1-hgajjar@de.adit-jv.com/
> > ---
> >  drivers/usb/core/hub.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index b4584a0cd484..b5ac29c5f016 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -47,12 +47,18 @@
> >  #define USB_VENDOR_TEXAS_INSTRUMENTS		0x0451
> >  #define USB_PRODUCT_TUSB8041_USB3		0x8140
> >  #define USB_PRODUCT_TUSB8041_USB2		0x8142
> > +#define USB_VENDOR_MICROCHIP			0x0424
> > +#define USB_PRODUCT_USB4913			0x4913
> > +#define USB_PRODUCT_USB4914			0x4914
> > +#define USB_PRODUCT_USB4915			0x4915
> >  #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
> >  #define HUB_QUIRK_DISABLE_AUTOSUSPEND		0x02
> > +#define HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL	0x08
> 
> Why use 0x08 instead of 0x04?

And why not use BIT() as that's what these are.

thanks,

greg k-h
Hardik Gajjar Dec. 4, 2023, 7:25 a.m. UTC | #3 
On Fri, Dec 01, 2023 at 11:13:53AM -0500, Alan Stern wrote:
> On Fri, Dec 01, 2023 at 03:47:05PM +0100, Hardik Gajjar wrote:
> > There is a potential delay in notifying Linux USB drivers of downstream
> > USB bus activity when connecting a high-speed or superSpeed device via the
> > Microchip USB491x hub. This delay is due to the fixed bInterval value of
> > 12 in the silicon of the Microchip USB491x hub.
> > 
> > Microchip requested to ignore the device descriptor and decrease that
> > value to 9 as it was too late to modify that in silicon.
> > 
> > This patch speeds up the USB enummeration process that helps to pass
> > Apple Carplay certifications and improve the User experience when utilizing
> > the USB device via Microchip Multihost USB491x Hub.
> > 
> > A new hub quirk HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL speeds up
> > the notification process for Microchip USB491x hub by limiting
> > the maximum bInterval value to 9.
> > 
> > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> > ---
> > changes since version 1:
> > 	- Move implementation from config.c and quirk.c to hub.c as this is hub
> > 	  specific changes.
> > 	- Improve commit message.
> > 	- Link to v1 - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20231123081948.58776-2D1-2Dhgajjar-40de.adit-2Djv.com_&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=QQ3QedNypxcZh4lyJ03w82T23GGGhDS9PuCb678l0L0&m=JINv_OISTxW8v8yb9tolCAxLxPiVc7UqJhl5BjTAuILGMvqrKEwJhrwzz8yj7Uqi&s=_UbxbqWhpCXaOnmryralngzR1cMrf3bKTJQKyEX1AGw&e=
> > 
> > changes since version 2:
> >     - Call usb_set_interface after updating the bInterval to Tell the HCD about modification
> > 	- Link to v2 - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20231130084855.119937-2D1-2Dhgajjar-40de.adit-2Djv.com_&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=QQ3QedNypxcZh4lyJ03w82T23GGGhDS9PuCb678l0L0&m=JINv_OISTxW8v8yb9tolCAxLxPiVc7UqJhl5BjTAuILGMvqrKEwJhrwzz8yj7Uqi&s=Fy-O0g4cYUiedPpzRIJU7rX8zvrhvjFfjZ7xo-K19yg&e=
> > ---
> >  drivers/usb/core/hub.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index b4584a0cd484..b5ac29c5f016 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -47,12 +47,18 @@
> >  #define USB_VENDOR_TEXAS_INSTRUMENTS		0x0451
> >  #define USB_PRODUCT_TUSB8041_USB3		0x8140
> >  #define USB_PRODUCT_TUSB8041_USB2		0x8142
> > +#define USB_VENDOR_MICROCHIP			0x0424
> > +#define USB_PRODUCT_USB4913			0x4913
> > +#define USB_PRODUCT_USB4914			0x4914
> > +#define USB_PRODUCT_USB4915			0x4915
> >  #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
> >  #define HUB_QUIRK_DISABLE_AUTOSUSPEND		0x02
> > +#define HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL	0x08
> 
> Why use 0x08 instead of 0x04?
> 
> Alan Stern

Ahh, This is a mistake. Thanks for pointing out. 
In my vendor kernel there is already a quirk with 0x04 so started the development and testing
with 0x08, but forgot to change to 0x04 when cherry-picked to mainline.

Thanks,
Hardik
Hardik Gajjar Dec. 4, 2023, 3:25 p.m. UTC | #4 
On Mon, Dec 04, 2023 at 08:02:50AM +0100, Greg KH wrote:
> On Fri, Dec 01, 2023 at 11:13:53AM -0500, Alan Stern wrote:
> > On Fri, Dec 01, 2023 at 03:47:05PM +0100, Hardik Gajjar wrote:
> > > There is a potential delay in notifying Linux USB drivers of downstream
> > > USB bus activity when connecting a high-speed or superSpeed device via the
> > > Microchip USB491x hub. This delay is due to the fixed bInterval value of
> > > 12 in the silicon of the Microchip USB491x hub.
> > > 
> > > Microchip requested to ignore the device descriptor and decrease that
> > > value to 9 as it was too late to modify that in silicon.
> > > 
> > > This patch speeds up the USB enummeration process that helps to pass
> > > Apple Carplay certifications and improve the User experience when utilizing
> > > the USB device via Microchip Multihost USB491x Hub.
> > > 
> > > A new hub quirk HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL speeds up
> > > the notification process for Microchip USB491x hub by limiting
> > > the maximum bInterval value to 9.
> > > 
> > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> > > ---
> > > changes since version 1:
> > > 	- Move implementation from config.c and quirk.c to hub.c as this is hub
> > > 	  specific changes.
> > > 	- Improve commit message.
> > > 	- Link to v1 - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20231123081948.58776-2D1-2Dhgajjar-40de.adit-2Djv.com_&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=MwlH5BZuYXKwMbYoZm0ibbRgqL9CLBFJs3W-um9OX810KDZtChbSXjDyPjCZhJDf&s=xgoOKLqU4fKUbsJMZHzIQzrWKH2W9ikdmKvGntoPcaA&e=
> > > 
> > > changes since version 2:
> > >     - Call usb_set_interface after updating the bInterval to Tell the HCD about modification
> > > 	- Link to v2 - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20231130084855.119937-2D1-2Dhgajjar-40de.adit-2Djv.com_&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=MwlH5BZuYXKwMbYoZm0ibbRgqL9CLBFJs3W-um9OX810KDZtChbSXjDyPjCZhJDf&s=Ck2GM1MhxBKLkLrlkZd_QwoIXz07CbJGVqpVss0rSEI&e=
> > > ---
> > >  drivers/usb/core/hub.c | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index b4584a0cd484..b5ac29c5f016 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -47,12 +47,18 @@
> > >  #define USB_VENDOR_TEXAS_INSTRUMENTS		0x0451
> > >  #define USB_PRODUCT_TUSB8041_USB3		0x8140
> > >  #define USB_PRODUCT_TUSB8041_USB2		0x8142
> > > +#define USB_VENDOR_MICROCHIP			0x0424
> > > +#define USB_PRODUCT_USB4913			0x4913
> > > +#define USB_PRODUCT_USB4914			0x4914
> > > +#define USB_PRODUCT_USB4915			0x4915
> > >  #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
> > >  #define HUB_QUIRK_DISABLE_AUTOSUSPEND		0x02
> > > +#define HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL	0x08
> > 
> > Why use 0x08 instead of 0x04?
> 
> And why not use BIT() as that's what these are.
> 
> thanks,
> 
> greg k-h

I am considering aligning with existing quirks.
Is it advisable to use something like HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL BIT(2)? 
I'm thinking to submit two patches – one to replace hard values with BIT() in existing quirks and a second patch containing my changes.

Thanks,
Hardik
Alan Stern Dec. 4, 2023, 3:44 p.m. UTC | #5 
On Mon, Dec 04, 2023 at 04:25:43PM +0100, Hardik Gajjar wrote:
> On Mon, Dec 04, 2023 at 08:02:50AM +0100, Greg KH wrote:
> > And why not use BIT() as that's what these are.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I am considering aligning with existing quirks.
> Is it advisable to use something like HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL BIT(2)? 
> I'm thinking to submit two patches – one to replace hard values with BIT() in existing quirks and a second patch containing my changes.

That's a good idea.

Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b4584a0cd484..b5ac29c5f016 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -47,12 +47,18 @@ 
 #define USB_VENDOR_TEXAS_INSTRUMENTS		0x0451
 #define USB_PRODUCT_TUSB8041_USB3		0x8140
 #define USB_PRODUCT_TUSB8041_USB2		0x8142
+#define USB_VENDOR_MICROCHIP			0x0424
+#define USB_PRODUCT_USB4913			0x4913
+#define USB_PRODUCT_USB4914			0x4914
+#define USB_PRODUCT_USB4915			0x4915
 #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
 #define HUB_QUIRK_DISABLE_AUTOSUSPEND		0x02
+#define HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL	0x08
 
 #define USB_TP_TRANSMISSION_DELAY	40	/* ns */
 #define USB_TP_TRANSMISSION_DELAY_MAX	65535	/* ns */
 #define USB_PING_RESPONSE_TIME		400	/* ns */
+#define USB_REDUCE_FRAME_INTR_BINTERVAL	9
 
 /* Protect struct usb_device->state and ->children members
  * Note: Both are also protected by ->dev.sem, except that ->state can
@@ -1927,6 +1933,14 @@  static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 		usb_autopm_get_interface_no_resume(intf);
 	}
 
+	if ((id->driver_info & HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL) &&
+	    desc->endpoint[0].desc.bInterval > USB_REDUCE_FRAME_INTR_BINTERVAL) {
+		desc->endpoint[0].desc.bInterval =
+			USB_REDUCE_FRAME_INTR_BINTERVAL;
+		/* Tell the HCD about the interrupt ep's new bInterval */
+		usb_set_interface(hdev, 0, 0);
+	}
+
 	if (hub_configure(hub, &desc->endpoint[0].desc) >= 0) {
 		onboard_hub_create_pdevs(hdev, &hub->onboard_hub_devs);
 
@@ -5918,6 +5932,21 @@  static const struct usb_device_id hub_id_table[] = {
       .idVendor = USB_VENDOR_TEXAS_INSTRUMENTS,
       .idProduct = USB_PRODUCT_TUSB8041_USB3,
       .driver_info = HUB_QUIRK_DISABLE_AUTOSUSPEND},
+	{ .match_flags = USB_DEVICE_ID_MATCH_VENDOR
+			| USB_DEVICE_ID_MATCH_PRODUCT,
+	  .idVendor = USB_VENDOR_MICROCHIP,
+	  .idProduct = USB_PRODUCT_USB4913,
+	  .driver_info = HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL},
+	{ .match_flags = USB_DEVICE_ID_MATCH_VENDOR
+			| USB_DEVICE_ID_MATCH_PRODUCT,
+	  .idVendor = USB_VENDOR_MICROCHIP,
+	  .idProduct = USB_PRODUCT_USB4914,
+	  .driver_info = HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL},
+	{ .match_flags = USB_DEVICE_ID_MATCH_VENDOR
+			| USB_DEVICE_ID_MATCH_PRODUCT,
+	  .idVendor = USB_VENDOR_MICROCHIP,
+	  .idProduct = USB_PRODUCT_USB4915,
+	  .driver_info = HUB_QUIRK_REDUCE_FRAME_INTR_BINTERVAL},
     { .match_flags = USB_DEVICE_ID_MATCH_DEV_CLASS,
       .bDeviceClass = USB_CLASS_HUB},
     { .match_flags = USB_DEVICE_ID_MATCH_INT_CLASS,