[PATCH 0/2] Allow disabling USB3 ports in xHCI/DWC3

[PATCH 0/2] Allow disabling USB3 ports in xHCI/DWC3

[Sam Edwards]

Hi USB devs,

This patchset is a semi-RFC: I haven't discussed this change yet, and it may
turn out to be a bad idea. But if there is a consensus that this change is
appropriate, these patches are the ones I'd submit for inclusion.

These patches were developed while working with a SoC (Rockchip RK3588) that
contains DWC3-OTG controllers and accompanying USB2 + USB3/DP PHYs. My target
(Turing RK1) uses its first bus in USB2.0-OTG mode only: the associated USB3
PHY is reserved for DP. Worse, a driver for the USBDP block (though it exists)
has not been merged to mainline. Without lighting up the PHY side of the PIPE,
the DWC3 behaves erratically, even for USB2 operation.

This could be addressed by patching in the (out-of-tree) USBDP driver and
enabling only its USB backend. However, I found it cleaner (also from a
user-friendliness standpoint) just to disable the unusable USB3 port. These
patches achieve that by (1) making it possible to tell the xHCI driver to
ignore any USB3 port(s), and (2) (perhaps more controversially) making the DWC3
driver disable USB3 host ports when `maximum-speed` isn't set high enough.

There are other ways to disable the USB3 ports on RK3588, such as via some
syscon registers. I figured I would start with the most general solution
(benefitting other SoCs) first, getting more specific only if necessary. :)

Thank you for your attention,
Sam

Sam Edwards (2):
  xhci: Introduce "disable-usb3" DT property/quirk
  usb: dwc3: host: Disable USB3 ports if maximum-speed doesn't permit
    USB3

 Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++
 drivers/usb/dwc3/host.c                             | 5 ++++-
 drivers/usb/host/xhci-mem.c                         | 4 ++++
 drivers/usb/host/xhci-plat.c                        | 3 +++
 drivers/usb/host/xhci.h                             | 1 +
 5 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.41.0

[PATCH 1/2] xhci: Introduce "disable-usb3" DT property/quirk

[Sam Edwards]

Some systems may have xHCI controllers that enumerate USB 3.0 ports, but
these ports nevertheless cannot be used. Perhaps enabling them triggers a
hardware bug, or perhaps they simply aren't connected and it would be
confusing to the user to see an unusable USB 3.0 rhub show up -- whatever
the case may be, it's reasonable to want to disable these ports.

Add a DT property (and associated quirk) to the xHCI driver that skips
over (i.e. ignores and doesn't initialize) any USB 3.0 ports discovered
during driver initialization.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++
 drivers/usb/host/xhci-mem.c                         | 4 ++++
 drivers/usb/host/xhci-plat.c                        | 3 +++
 drivers/usb/host/xhci.h                             | 1 +
 4 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.yaml b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
index 180a261c3e8f..8a64e747260a 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.yaml
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
@@ -25,6 +25,10 @@ properties:
     description: Set if the controller has broken port disable mechanism
     type: boolean
 
+  disable-usb3:
+    description: Ignore (don't initialize, don't use) USB3 ports
+    type: boolean
+
   imod-interval-ns:
     description: Interrupt moderation interval
     default: 5000
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 0a37f0d511cf..bf8fcab626e4 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1968,6 +1968,10 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
 	minor_revision = XHCI_EXT_PORT_MINOR(temp);
 
 	if (major_revision == 0x03) {
+		/* Ignore USB3 ports entirely if USB3 support is disabled. */
+		if (xhci->quirks & XHCI_DISABLE_USB3)
+			return;
+
 		rhub = &xhci->usb3_rhub;
 		/*
 		 * Some hosts incorrectly use sub-minor version for minor
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index b93161374293..75285fb5bbbc 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -249,6 +249,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
 		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
 			xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
+		if (device_property_read_bool(tmpdev, "disable-usb3"))
+			xhci->quirks |= XHCI_DISABLE_USB3;
+
 		device_property_read_u32(tmpdev, "imod-interval-ns",
 					 &xhci->imod_interval);
 	}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 5df370482521..c53fbeea478f 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1906,6 +1906,7 @@ struct xhci_hcd {
 #define XHCI_RESET_TO_DEFAULT	BIT_ULL(44)
 #define XHCI_ZHAOXIN_TRB_FETCH	BIT_ULL(45)
 #define XHCI_ZHAOXIN_HOST	BIT_ULL(46)
+#define XHCI_DISABLE_USB3	BIT_ULL(47)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
-- 
2.41.0

[PATCH 2/2] usb: dwc3: host: Disable USB3 ports if maximum-speed doesn't permit USB3

[Sam Edwards]

The DWC3 core can be configured (during IP instantiation, and/or via
configuration signals) not to have any USB3 ports. Some SoCs, however,
may have USB3 interfaces enabled that do not have USB3 PHYs driving
them. This can be due to a few circumstances, including:
a) The hardware designer didn't include USB3 PHYs and neglected to
   disable the DWC3 core's USB ports (I know of no instance where this
   has actually happened, however, and it seems pretty unlikely).
b) The USB3 PHYs are present but powered off. Perhaps a driver to enable
   the PHYs has not yet been written or merged, or USB3 capability is
   unneeded in the system and the system designer would like to conserve
   power.
c) The USB3 PHYs are muxed to a different controller. This can happen if
   the PHYs support non-USB protocols and one of these alternate
   functions is needed instead.

In these circumstances, if the DWC3 does not receive clear link status
indication on an enabled USB3 port, the DWC3 may not allow even USB2
to function: in host mode, the DWC3 generates an endless barrage of
PORT_CSC status on the accompanying USB2 port, and the xHCI driver is
unable to bring the USB2 port to a functioning state.

Fix this by first checking if the maximum-speed property in the DT
permits USB3. If not, pass the new `disable-usb3;` property to the
virtual xHCI device, causing the xHCI driver not to enable the USB3
ports. This allows USB2 to function even with USB3 PHYs
missing/misbehaving, and may be useful even when the USB3 PHYs are
well-behaved: a DT author may know that USB3 support is intact, but
disconnected (not exposed off-board) and choose to lower the
maximum-speed property to avoid an unusable USB3 rhub showing up in
sysfs/lsusb where it may mislead end-users.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 drivers/usb/dwc3/host.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 61f57fe5bb78..29f170927e70 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -61,7 +61,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
 
 int dwc3_host_init(struct dwc3 *dwc)
 {
-	struct property_entry	props[4];
+	struct property_entry	props[5];
 	struct platform_device	*xhci;
 	int			ret, irq;
 	int			prop_idx = 0;
@@ -95,6 +95,9 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
+	if (dwc->maximum_speed == USB_SPEED_FULL || dwc->maximum_speed == USB_SPEED_HIGH)
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("disable-usb3");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
-- 
2.41.0

Re: [PATCH 1/2] xhci: Introduce "disable-usb3" DT property/quirk

[Krzysztof Kozlowski]

On 08/12/2023 22:04, Sam Edwards wrote:
> Some systems may have xHCI controllers that enumerate USB 3.0 ports, but
> these ports nevertheless cannot be used. Perhaps enabling them triggers a
> hardware bug, or perhaps they simply aren't connected and it would be
> confusing to the user to see an unusable USB 3.0 rhub show up -- whatever
> the case may be, it's reasonable to want to disable these ports.
> 
> Add a DT property (and associated quirk) to the xHCI driver that skips
> over (i.e. ignores and doesn't initialize) any USB 3.0 ports discovered
> during driver initialization.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++

Bindings are always separate patches.

Please do not sneak in properties without DT review.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

Re: [PATCH 1/2] xhci: Introduce "disable-usb3" DT property/quirk

[Sam Edwards]

On 12/9/23 06:53, Krzysztof Kozlowski wrote:
> On 08/12/2023 22:04, Sam Edwards wrote:
>> Some systems may have xHCI controllers that enumerate USB 3.0 ports, but
>> these ports nevertheless cannot be used. Perhaps enabling them triggers a
>> hardware bug, or perhaps they simply aren't connected and it would be
>> confusing to the user to see an unusable USB 3.0 rhub show up -- whatever
>> the case may be, it's reasonable to want to disable these ports.
>>
>> Add a DT property (and associated quirk) to the xHCI driver that skips
>> over (i.e. ignores and doesn't initialize) any USB 3.0 ports discovered
>> during driver initialization.
>>
>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
>> ---
>>   Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++

Hi Krzysztof,

> 
> Bindings are always separate patches.
> 
> Please do not sneak in properties without DT review.
> 

It makes sense that the new property should be introduced in a separate 
patch. I'll ensure that is the case in v2. (If there is one -- see below.)

> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling.

I have tried (and failed) to find the documentation for this 
linux-devicetree bot. Do you have the link? In particular, I'd like to 
ensure that patch 2/2 (the one that actually changes established 
behavior) is tested sufficiently thoroughly.

> Performing review on untested code might be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.

That's fine; this patch has just failed review anyway (due to the new 
property not being introduced in a separate patch), and I'll need to 
prepare and send a v2 to proceed. However as I mentioned in the cover, 
this is a semi-RFC. I haven't discussed the overall idea with anyone 
yet, so to avoid wasting my own time, I need to give the USB folks ample 
opportunity to object to the proposed changes or suggest improvements 
before investing more effort in refining the patchset.

As of now, I'm only seeking commentary, not formal review. I'd 
appreciate any insights on the approach I've taken and whether there are 
any potential challenges or alternatives that haven't been explored yet. 
Therefore, I'll hold off on CC-ing linux-devicetree at this stage to 
keep the focus on the broader concept, and will loop them in (with any 
other recipients as appropriate) for v2 when (and if!) there's consensus 
here on linux-usb that the general direction is worth pursuing.

> 
> Please kindly resend and include all necessary To/Cc entries.
> 
> Best regards,
> Krzysztof
> 

Happy Saturday,
Sam

Re: [PATCH 1/2] xhci: Introduce "disable-usb3" DT property/quirk

[Krzysztof Kozlowski]

On 09/12/2023 20:26, Sam Edwards wrote:
>> Performing review on untested code might be
>> a waste of time, thus I will skip this patch entirely till you follow
>> the process allowing the patch to be tested.
> 
> That's fine; this patch has just failed review anyway (due to the new 
> property not being introduced in a separate patch), and I'll need to 
> prepare and send a v2 to proceed. However as I mentioned in the cover, 
> this is a semi-RFC. I haven't discussed the overall idea with anyone 
> yet, so to avoid wasting my own time, I need to give the USB folks ample 

It does not really explain why you did not Cc some of the maintainers.
If this is a RFC, even though not marked as such in subject prefix, then
I guess all maintainers should be involved for comments.

Best regards,
Krzysztof

Re: [PATCH 1/2] xhci: Introduce "disable-usb3" DT property/quirk

[Sam Edwards]

On 12/10/23 04:10, Krzysztof Kozlowski wrote:
> It does not really explain why you did not Cc some of the maintainers.
> If this is a RFC, even though not marked as such in subject prefix, then
> I guess all maintainers should be involved for comments.

Hi Krzysztof,

More simply put: I don't want to waste anyone's time by seeking 
review/commentary from linux-devicetree on a property enabling a feature 
that someone here on linux-usb might soon deem (summarily) untenable.

I agree fully that I need (and want) the DT reviewers' sign-off on this 
new property, and I'll get more serious about that (and other things) in 
v2. I just need to know if there's going to be a v2 first.

Warm regards,
Sam

Re: [PATCH 1/2] xhci: Introduce "disable-usb3" DT property/quirk

[Heiko Stuebner]

Am Freitag, 8. Dezember 2023, 22:04:57 CET schrieb Sam Edwards:
> Some systems may have xHCI controllers that enumerate USB 3.0 ports, but
> these ports nevertheless cannot be used. Perhaps enabling them triggers a
> hardware bug, or perhaps they simply aren't connected and it would be
> confusing to the user to see an unusable USB 3.0 rhub show up -- whatever
> the case may be, it's reasonable to want to disable these ports.
> 
> Add a DT property (and associated quirk) to the xHCI driver that skips
> over (i.e. ignores and doesn't initialize) any USB 3.0 ports discovered
> during driver initialization.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

I'm very much unsure, where the line goes between hw-quirk and
dt-is-not-a-configuration-space - in this specific instance.

DT is meant to describe the actual hardware present and not how
any operating system supports it.

So having that usb3phy present in the kernel - even if only in
a more limited form as you describe would be my preference.


But for a short-term thing, the usb3-phy in the binding is optional, so
so you could "just" deduce the no-usb3 state in your code from its
absence from the dt-node?


Heiko



>  Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++
>  drivers/usb/host/xhci-mem.c                         | 4 ++++
>  drivers/usb/host/xhci-plat.c                        | 3 +++
>  drivers/usb/host/xhci.h                             | 1 +
>  4 files changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.yaml b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
> index 180a261c3e8f..8a64e747260a 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.yaml
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
> @@ -25,6 +25,10 @@ properties:
>      description: Set if the controller has broken port disable mechanism
>      type: boolean
>  
> +  disable-usb3:
> +    description: Ignore (don't initialize, don't use) USB3 ports
> +    type: boolean
> +
>    imod-interval-ns:
>      description: Interrupt moderation interval
>      default: 5000
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 0a37f0d511cf..bf8fcab626e4 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1968,6 +1968,10 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
>  	minor_revision = XHCI_EXT_PORT_MINOR(temp);
>  
>  	if (major_revision == 0x03) {
> +		/* Ignore USB3 ports entirely if USB3 support is disabled. */
> +		if (xhci->quirks & XHCI_DISABLE_USB3)
> +			return;
> +
>  		rhub = &xhci->usb3_rhub;
>  		/*
>  		 * Some hosts incorrectly use sub-minor version for minor
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index b93161374293..75285fb5bbbc 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -249,6 +249,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
>  		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
>  			xhci->quirks |= XHCI_BROKEN_PORT_PED;
>  
> +		if (device_property_read_bool(tmpdev, "disable-usb3"))
> +			xhci->quirks |= XHCI_DISABLE_USB3;
> +
>  		device_property_read_u32(tmpdev, "imod-interval-ns",
>  					 &xhci->imod_interval);
>  	}
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 5df370482521..c53fbeea478f 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1906,6 +1906,7 @@ struct xhci_hcd {
>  #define XHCI_RESET_TO_DEFAULT	BIT_ULL(44)
>  #define XHCI_ZHAOXIN_TRB_FETCH	BIT_ULL(45)
>  #define XHCI_ZHAOXIN_HOST	BIT_ULL(46)
> +#define XHCI_DISABLE_USB3	BIT_ULL(47)
>  
>  	unsigned int		num_active_eps;
>  	unsigned int		limit_active_eps;
> 

Re: [PATCH 1/2] xhci: Introduce "disable-usb3" DT property/quirk

[Sam Edwards]

On 12/12/23 12:31, Heiko Stuebner wrote:
> Am Freitag, 8. Dezember 2023, 22:04:57 CET schrieb Sam Edwards:
>> Some systems may have xHCI controllers that enumerate USB 3.0 ports, but
>> these ports nevertheless cannot be used. Perhaps enabling them triggers a
>> hardware bug, or perhaps they simply aren't connected and it would be
>> confusing to the user to see an unusable USB 3.0 rhub show up -- whatever
>> the case may be, it's reasonable to want to disable these ports.
>>
>> Add a DT property (and associated quirk) to the xHCI driver that skips
>> over (i.e. ignores and doesn't initialize) any USB 3.0 ports discovered
>> during driver initialization.
>>
>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Ahoy Heiko,

Glad to have you in the thread!

> I'm very much unsure, where the line goes between hw-quirk and
> dt-is-not-a-configuration-space - in this specific instance.
> 
> DT is meant to describe the actual hardware present and not how
> any operating system supports it.

I can plumb this quirk through something other than DT if there's an 
objection to it; however, the intended meaning of this property is 
indeed hardware-description and not OS-configuration: "This xHC does NOT 
support USB3. Do not let it tell you otherwise by enumerating USB3 
ports; it is mistaken. Attempting to enable the USB3 'ports' will at 
worst provoke a hardware bug and at best register a usb3 rhub for 
something that doesn't physically exist."

For v2, I should probably rename the property from something 
"imperative" ("disable-usb3;") to something "declarative," like 
"no-usb3;" or "usb3-unconnected;" in order to reflect this intended 
meaning better. The linux-devicetree reviewers (which I will be looping 
in with v2) will probably prefer a name like that anyway.

> So having that usb3phy present in the kernel - even if only in
> a more limited form as you describe would be my preference.
> 
> 
> But for a short-term thing, the usb3-phy in the binding is optional, so
> so you could "just" deduce the no-usb3 state in your code from its
> absence from the dt-node?

Ah, so while there is a short-term benefit, this is actually meant to be 
long-term: on the Turing RK1 PCB itself, USB-DWC3 #0 is only wired for 
USB 2.0, not USB 3.0. The corresponding USBDP PHY pins are connected to 
(dedicated) DisplayPort pins.

This means there are 4 options:
1. Proceed without enabling that USB-DWC3 at all, giving up USB 2.0.
2. Enable the USBDP in USB3 mode, gaining USB 2.0 but giving up DP.
3. Enable the USBDP in DP mode, having the USB3-PIPE backend
    initialized (but disconnected from the frontend), as a "pacifier"
    for the DWC3 so it doesn't start misbehaving.
4. Disable the USB3 ports on the xHC side, by...
    a. Telling the xHCI driver that it shouldn't enable USB3 ports.
    b. Configuring the DWC3 not to enable USB3
       (see the RK3588 TRM for usb3otg0_host_u3_port_disable)
    c. Configuring the DWC3's number of USB3 ports
       (usb3otg0_host_num_u3_port)

Users won't like #1 or #2, and may be confused by #3 due to the dummy 
port that shows up. Options #2 and #3 aren't possible until a USBDP 
driver lands (though you're right that this is only a short-term 
concern). Option 4a is what I'm currently trying to do because I believe 
having physically-inaccessible USB3 ports show up in Linux is 
undesirable in any case, but I can switch to 4b/4c if we want something 
more SoC-specific.

So (as far as RK3588 is concerned) with option(s) 4(a/b/c), while this 
does let USB2.0 work before the USBDP driver lands, the "real" benefit 
is that it requires neither a dummy port nor powering up a block of the 
chip (the PHY's USB3-specific block) that isn't actually usable/useful 
because it doesn't connect to a USB 3.0 socket/device.

Though if that still doesn't sound appealing, I'm happy to change up the 
approach -- I guess the question then is whether it's because you *like* 
option 3, or *dislike* option 4.

Cheers,
Sam

> 
> 
> Heiko
> 
> 
> 
>>   Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++
>>   drivers/usb/host/xhci-mem.c                         | 4 ++++
>>   drivers/usb/host/xhci-plat.c                        | 3 +++
>>   drivers/usb/host/xhci.h                             | 1 +
>>   4 files changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.yaml b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
>> index 180a261c3e8f..8a64e747260a 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-xhci.yaml
>> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.yaml
>> @@ -25,6 +25,10 @@ properties:
>>       description: Set if the controller has broken port disable mechanism
>>       type: boolean
>>   
>> +  disable-usb3:
>> +    description: Ignore (don't initialize, don't use) USB3 ports
>> +    type: boolean
>> +
>>     imod-interval-ns:
>>       description: Interrupt moderation interval
>>       default: 5000
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 0a37f0d511cf..bf8fcab626e4 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1968,6 +1968,10 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
>>   	minor_revision = XHCI_EXT_PORT_MINOR(temp);
>>   
>>   	if (major_revision == 0x03) {
>> +		/* Ignore USB3 ports entirely if USB3 support is disabled. */
>> +		if (xhci->quirks & XHCI_DISABLE_USB3)
>> +			return;
>> +
>>   		rhub = &xhci->usb3_rhub;
>>   		/*
>>   		 * Some hosts incorrectly use sub-minor version for minor
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index b93161374293..75285fb5bbbc 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -249,6 +249,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
>>   		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
>>   			xhci->quirks |= XHCI_BROKEN_PORT_PED;
>>   
>> +		if (device_property_read_bool(tmpdev, "disable-usb3"))
>> +			xhci->quirks |= XHCI_DISABLE_USB3;
>> +
>>   		device_property_read_u32(tmpdev, "imod-interval-ns",
>>   					 &xhci->imod_interval);
>>   	}
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 5df370482521..c53fbeea478f 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1906,6 +1906,7 @@ struct xhci_hcd {
>>   #define XHCI_RESET_TO_DEFAULT	BIT_ULL(44)
>>   #define XHCI_ZHAOXIN_TRB_FETCH	BIT_ULL(45)
>>   #define XHCI_ZHAOXIN_HOST	BIT_ULL(46)
>> +#define XHCI_DISABLE_USB3	BIT_ULL(47)
>>   
>>   	unsigned int		num_active_eps;
>>   	unsigned int		limit_active_eps;
>>
> 
> 
> 
> 

Re: [PATCH 0/2] Allow disabling USB3 ports in xHCI/DWC3

[Mathias Nyman]

On 8.12.2023 23.04, Sam Edwards wrote:
> Hi USB devs,
> 
> This patchset is a semi-RFC: I haven't discussed this change yet, and it may
> turn out to be a bad idea. But if there is a consensus that this change is
> appropriate, these patches are the ones I'd submit for inclusion.
> 
> These patches were developed while working with a SoC (Rockchip RK3588) that
> contains DWC3-OTG controllers and accompanying USB2 + USB3/DP PHYs. My target
> (Turing RK1) uses its first bus in USB2.0-OTG mode only: the associated USB3
> PHY is reserved for DP. Worse, a driver for the USBDP block (though it exists)
> has not been merged to mainline. Without lighting up the PHY side of the PIPE,
> the DWC3 behaves erratically, even for USB2 operation.
> 
> This could be addressed by patching in the (out-of-tree) USBDP driver and
> enabling only its USB backend. However, I found it cleaner (also from a
> user-friendliness standpoint) just to disable the unusable USB3 port. These
> patches achieve that by (1) making it possible to tell the xHCI driver to
> ignore any USB3 port(s), and (2) (perhaps more controversially) making the DWC3
> driver disable USB3 host ports when `maximum-speed` isn't set high enough.

I don't think this will work as a generic xhci driver feature.

Even if we ignore all USB3 ports in software they will for most xHC hosts be powered
and enabled in hardware by default after controller reset.

This means they perform link training, generate all kinds of events with interrupts
(connect, over-current etc) that driver now can't handle.

Sound like the setup you are using has a very specific issue, and it would need
a narrow targeted quirk to solve it.

> 
> There are other ways to disable the USB3 ports on RK3588, such as via some
> syscon registers. I figured I would start with the most general solution
> (benefitting other SoCs) first, getting more specific only if necessary. :)

To me a specific solution to a specific problem like this sounds better.

Thanks
Mathias

Re: [PATCH 2/2] usb: dwc3: host: Disable USB3 ports if maximum-speed doesn't permit USB3

[Greg Kroah-Hartman]

On Fri, Dec 08, 2023 at 02:04:58PM -0700, Sam Edwards wrote:
> The DWC3 core can be configured (during IP instantiation, and/or via
> configuration signals) not to have any USB3 ports. Some SoCs, however,
> may have USB3 interfaces enabled that do not have USB3 PHYs driving
> them. This can be due to a few circumstances, including:
> a) The hardware designer didn't include USB3 PHYs and neglected to
>    disable the DWC3 core's USB ports (I know of no instance where this
>    has actually happened, however, and it seems pretty unlikely).
> b) The USB3 PHYs are present but powered off. Perhaps a driver to enable
>    the PHYs has not yet been written or merged, or USB3 capability is
>    unneeded in the system and the system designer would like to conserve
>    power.
> c) The USB3 PHYs are muxed to a different controller. This can happen if
>    the PHYs support non-USB protocols and one of these alternate
>    functions is needed instead.
> 
> In these circumstances, if the DWC3 does not receive clear link status
> indication on an enabled USB3 port, the DWC3 may not allow even USB2
> to function: in host mode, the DWC3 generates an endless barrage of
> PORT_CSC status on the accompanying USB2 port, and the xHCI driver is
> unable to bring the USB2 port to a functioning state.
> 
> Fix this by first checking if the maximum-speed property in the DT
> permits USB3. If not, pass the new `disable-usb3;` property to the
> virtual xHCI device, causing the xHCI driver not to enable the USB3
> ports. This allows USB2 to function even with USB3 PHYs
> missing/misbehaving, and may be useful even when the USB3 PHYs are
> well-behaved: a DT author may know that USB3 support is intact, but
> disconnected (not exposed off-board) and choose to lower the
> maximum-speed property to avoid an unusable USB3 rhub showing up in
> sysfs/lsusb where it may mislead end-users.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  drivers/usb/dwc3/host.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Where is patch 1/2 of this series?

confused,

greg k-h

Re: [PATCH 2/2] usb: dwc3: host: Disable USB3 ports if maximum-speed doesn't permit USB3

[Sam Edwards]

On 12/15/23 05:44, Greg Kroah-Hartman wrote:> Where is patch 1/2 of this 
series?

Hi Greg,

I double-checked and it does look like you were Cc'd on it. Either way, 
it's here:
https://lore.kernel.org/linux-usb/20231208210458.912776-1-CFSworks@gmail.com/T/#m5203732de487136de9b1d8a93b1c2f0b89dfe5e1

> 
> confused,
> 
> greg k-h

Equally confused,
Sam

Re: [PATCH 0/2] Allow disabling USB3 ports in xHCI/DWC3

[Sam Edwards]

Hi Mathias,

On 12/14/23 04:05, Mathias Nyman wrote:
> I don't think this will work as a generic xhci driver feature.
> 
> Even if we ignore all USB3 ports in software they will for most xHC 
> hosts be powered
> and enabled in hardware by default after controller reset.
> 
> This means they perform link training, generate all kinds of events with 
> interrupts
> (connect, over-current etc) that driver now can't handle.

By this do you mean that having the xHCI driver ignore the USB3 ports 
isn't enough to ensure that PP=0 (and the driver would need to do a 
little bit more to make sure that the "parking brake" is on: e.g. 
initialize, but not use, the ports) or that the xHC's PP=0 signal isn't 
sufficient to keep the PHYs from trying to bring the link up and 
generating those interrupts (PP=0 really isn't enough, and there is no 
general "parking brake" to be found here)?

> Sound like the setup you are using has a very specific issue, and it 
> would need
> a narrow targeted quirk to solve it.

I infer from this that you're against having a DT property added to 
xHCI? What if the property were to be narrowed in scope to "ignore the 
USB3 PHYs, they're disabled/absent" vs. this iteration's "disable the 
USB3 ports" meaning?

If this quirk ends up landing in the dwc3 driver (since, arguably, DWC3 
is the real misbehaving hw block in these circumstances), what would be 
your preferred mechanism of signaling to the xHCI layer "the USB3 PHYs 
have been disabled; please ignore"?

> 
>>
>> There are other ways to disable the USB3 ports on RK3588, such as via 
>> some
>> syscon registers. I figured I would start with the most general solution
>> (benefitting other SoCs) first, getting more specific only if 
>> necessary. :)
> 
> To me a specific solution to a specific problem like this sounds better.

I am starting to think so as well. I may shift my focus to DWC3 (with 
xHCI driver changes made only to facilitate them) for now, since 
`maximum-speed = "high-speed";` very reasonably (imo) should prevent 
registering the usb3 rhub -- though something may convince me otherwise 
in the near future. :)

> Thanks
> Mathias

Thanks to you as well, this is exactly the type of feedback I was 
fishing for!

Cheers,
Sam

Re: [PATCH 0/2] Allow disabling USB3 ports in xHCI/DWC3

[Mathias Nyman]

On 15.12.2023 23.59, Sam Edwards wrote:
> Hi Mathias,
> 
> On 12/14/23 04:05, Mathias Nyman wrote:
>> I don't think this will work as a generic xhci driver feature.
>>
>> Even if we ignore all USB3 ports in software they will for most xHC hosts be powered
>> and enabled in hardware by default after controller reset.
>>
>> This means they perform link training, generate all kinds of events with interrupts
>> (connect, over-current etc) that driver now can't handle.
> 
> By this do you mean that having the xHCI driver ignore the USB3 ports isn't enough to ensure that PP=0 (and the driver would need to do a little bit more to make sure that the "parking brake" is on: e.g. initialize, but not use, the ports) or that the xHC's PP=0 signal isn't sufficient to keep the PHYs from trying to bring the link up and generating those interrupts (PP=0 really isn't enough, and there is no general "parking brake" to be found here)?
> 

Yes, in most cases PP==1 after xHC reset, here's some old debug output during boot:

[    2.571057] xhci_hcd 0000:00:0d.0: new USB bus registered, assigned bus number 2
[    2.571061] xhci_hcd 0000:00:0d.0: Host supports USB 3.2 Enhanced SuperSpeed
[    2.571065] xhci_hcd 0000:00:0d.0: // Turn on HC, cmd = 0x5.
[    2.571067] xhci_hcd 0000:00:0d.0: Finished xhci_run for USB3 roothub
[    2.571093] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003, bcdDevice= 5.15
[    2.571095] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    2.571097] usb usb2: Product: xHCI Host Controller
[    2.571098] usb usb2: Manufacturer: Linux 5.15.57-06982-gf7339f7585d8-dirty xhci-hcd
[    2.571099] usb usb2: SerialNumber: 0000:00:0d.0
[    2.571197] xHCI xhci_add_endpoint called for root hub
[    2.571199] xHCI xhci_check_bandwidth called for root hub
[    2.571224] hub 2-0:1.0: USB hub found
[    2.571230] hub 2-0:1.0: 2 ports detected
[    2.571279] xhci_hcd 0000:00:0d.0: set port power 2-1 ON, portsc: 0x2a0

Note that portsc: 0x2a0 entry above has PP=1, and it shows the portsc register value
_before_  port power is set to 1 (ON).

Port Status: 0x2a0
	Disconnected
	Disabled
	Link: Rx Detect
	Powered
	Unknown port speed

Forcing PP=0 could possibly prevent any events from those ports.

>> Sound like the setup you are using has a very specific issue, and it would need
>> a narrow targeted quirk to solve it.
> 
> I infer from this that you're against having a DT property added to xHCI? What if the property were to be narrowed in scope to "ignore the USB3 PHYs, they're disabled/absent" vs. this iteration's "disable the USB3 ports" meaning?
> 
> If this quirk ends up landing in the dwc3 driver (since, arguably, DWC3 is the real misbehaving hw block in these circumstances), what would be your preferred mechanism of signaling to the xHCI layer "the USB3 PHYs have been disabled; please ignore"?

I don't have a good solution in mind for this so I'll just throw some ideas:

What happends if you in some RK3588 platform code disable USB  ports via those
syscon registers, but let the xhci driver be, and USB3 roothub enumerates normally?

Or if this is about a misbehaving USB3 PHY, how about adding the USB3 PHY driver that
describes reality and fails when usb_phy_roothub_init() or usb_phy_roothub_set_mode()
are called by for USB3 hcd during usb_add_hcd(USB3).
xhci driver could then continue without the USB3 hcd. turning off USB3 ports.
      
Adding the 'maximum-speed = "high-speed"' entry could also be one option.

Thanks
Mathias