diff mbox series

[5/5] xhci: support omitting shared hcd if either of the root hubs has no ports

Message ID e73bc0c1-e530-7419-0197-1b7de02c87c5@gmail.com (mailing list archive)
State Superseded
Headers show
Series usb: host: xhci-plat: omit shared hcd if either of the root hubs has no ports | expand

Commit Message

Heiner Kallweit March 4, 2022, 6:37 p.m. UTC
If either of the root hubs has no ports, then we can get rid of
overhead like the shared hcd. A major internal change is that now
the main hcd can be USB2 or USB3.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/usb/host/xhci-mem.c | 11 +++++------
 drivers/usb/host/xhci.c     |  9 ++++++---
 2 files changed, 11 insertions(+), 9 deletions(-)

Comments

Mathias Nyman March 8, 2022, 2:55 p.m. UTC | #1
On 4.3.2022 20.37, Heiner Kallweit wrote:
> If either of the root hubs has no ports, then we can get rid of
> overhead like the shared hcd. A major internal change is that now
> the main hcd can be USB2 or USB3.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/usb/host/xhci-mem.c | 11 +++++------
>  drivers/usb/host/xhci.c     |  9 ++++++---
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index a1a17713a..ced139583 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2362,12 +2362,11 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
>  		xhci->usb2_rhub.num_ports = USB_MAXCHILDREN;
>  	}
>  
> -	xhci->needs_shared_hcd = 1;
> -
> -	/*
> -	 * Note we could have all USB 3.0 ports, or all USB 2.0 ports.
> -	 * Not sure how the USB core will handle a hub with no ports...
> -	 */
> +	if (xhci->usb2_rhub.num_ports && xhci->usb3_rhub.num_ports)
> +		xhci->needs_shared_hcd = 1;
> +	else
> +		xhci_info(xhci, "USB%u root hub has no ports\n",
> +			  xhci->usb2_rhub.num_ports ? 3 : 2);

This now works for xhci controllers using xhci-plat.c, but in all other cases
the the secondary hcd will still be added.

Would it make sense to instead of setting xhci->needs_shared_hcd, we
set a xhci->allow_single_roothub flag in the .reset override function?
In the xhci-plat.c case this would be in xhci_plat_setup()

We would only add the flag if the respective probe supports one roothub.

Add a helper function to check if we if really should set up just one 
hcd in probe, and should call xhci_run_finished() already the he first time
xhci_run() is called (like you do in patch 3/5).

Something like:

bool xhci_has_one_roothub(struct xhci_hcd *xhci)
{
	return xhci->allow_single_roothub && (!xhci->usb2_rhub.num_ports != !xhci->usb3_rhub.num_ports);
}

Thanks
-Mathias
Heiner Kallweit March 10, 2022, 9:10 p.m. UTC | #2
On 08.03.2022 15:55, Mathias Nyman wrote:
> On 4.3.2022 20.37, Heiner Kallweit wrote:
>> If either of the root hubs has no ports, then we can get rid of
>> overhead like the shared hcd. A major internal change is that now
>> the main hcd can be USB2 or USB3.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/usb/host/xhci-mem.c | 11 +++++------
>>  drivers/usb/host/xhci.c     |  9 ++++++---
>>  2 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index a1a17713a..ced139583 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -2362,12 +2362,11 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
>>  		xhci->usb2_rhub.num_ports = USB_MAXCHILDREN;
>>  	}
>>  
>> -	xhci->needs_shared_hcd = 1;
>> -
>> -	/*
>> -	 * Note we could have all USB 3.0 ports, or all USB 2.0 ports.
>> -	 * Not sure how the USB core will handle a hub with no ports...
>> -	 */
>> +	if (xhci->usb2_rhub.num_ports && xhci->usb3_rhub.num_ports)
>> +		xhci->needs_shared_hcd = 1;
>> +	else
>> +		xhci_info(xhci, "USB%u root hub has no ports\n",
>> +			  xhci->usb2_rhub.num_ports ? 3 : 2);
> 
> This now works for xhci controllers using xhci-plat.c, but in all other cases
> the the secondary hcd will still be added.
> 
> Would it make sense to instead of setting xhci->needs_shared_hcd, we
> set a xhci->allow_single_roothub flag in the .reset override function?
> In the xhci-plat.c case this would be in xhci_plat_setup()
> 
> We would only add the flag if the respective probe supports one roothub.
> 
> Add a helper function to check if we if really should set up just one 
> hcd in probe, and should call xhci_run_finished() already the he first time
> xhci_run() is called (like you do in patch 3/5).
> 
> Something like:
> 
> bool xhci_has_one_roothub(struct xhci_hcd *xhci)
> {
> 	return xhci->allow_single_roothub && (!xhci->usb2_rhub.num_ports != !xhci->usb3_rhub.num_ports);
> }

Looks (very) good to me. I'll incorporate this and reorder the patches,
then I'll send a v2.

> 
> Thanks
> -Mathias
Heiner
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index a1a17713a..ced139583 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2362,12 +2362,11 @@  static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
 		xhci->usb2_rhub.num_ports = USB_MAXCHILDREN;
 	}
 
-	xhci->needs_shared_hcd = 1;
-
-	/*
-	 * Note we could have all USB 3.0 ports, or all USB 2.0 ports.
-	 * Not sure how the USB core will handle a hub with no ports...
-	 */
+	if (xhci->usb2_rhub.num_ports && xhci->usb3_rhub.num_ports)
+		xhci->needs_shared_hcd = 1;
+	else
+		xhci_info(xhci, "USB%u root hub has no ports\n",
+			  xhci->usb2_rhub.num_ports ? 3 : 2);
 
 	xhci_create_rhub_port_array(xhci, &xhci->usb2_rhub, flags);
 	xhci_create_rhub_port_array(xhci, &xhci->usb3_rhub, flags);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 83a54a566..623901890 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5279,9 +5279,7 @@  int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 
 	xhci = hcd_to_xhci(hcd);
 
-	if (usb_hcd_is_primary_hcd(hcd)) {
-		xhci_hcd_init_usb2_data(xhci, hcd);
-	} else {
+	if (!usb_hcd_is_primary_hcd(hcd)) {
 		xhci_hcd_init_usb3_data(xhci, hcd);
 		return 0;
 	}
@@ -5363,6 +5361,11 @@  int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 		return retval;
 	xhci_dbg(xhci, "Called HCD init\n");
 
+	if (xhci_hcd_is_usb3(hcd))
+		xhci_hcd_init_usb3_data(xhci, hcd);
+	else
+		xhci_hcd_init_usb2_data(xhci, hcd);
+
 	xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%016llx\n",
 		  xhci->hcc_params, xhci->hci_version, xhci->quirks);