[PATCH] configs: am62x_evm_*: Fix USB DFU configuration
CONFIG_USB_XHCI_DWC3 is not required for AM62x as the XHCI driver is registered through the dwc3-generic driver.
CONFIG_USB_XHCI_DWC3 causes problems by hijacking the USB controller even if it is not set for Host mode in device tree.
'dm tree' output after 'usb start' is fixed from
simple_bus 5 [ + ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | |-- usb@31000000 usb 0 [ + ] xhci-dwc3 | | `-- usb@31000000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | |-- usb@31100000 usb_hub 1 [ + ] usb_hub | | | `-- usb_hub usb 1 [ + ] xhci-dwc3 | | `-- usb@31100000 usb_hub 2 [ + ] usb_hub | | `-- usb_hub
[notice that 'xhci-dwc3' and 'usb_hub' drivers are probed for both USB instances although the first instance is supposed to be 'peripheral' only]
to
simple_bus 5 [ ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | `-- usb@31000000 simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | `-- usb@31100000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub
Fixes: dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support") Signed-off-by: Roger Quadros rogerq@kernel.org --- configs/am62x_a53_usbdfu.config | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am62x_a53_usbdfu.config b/configs/am62x_a53_usbdfu.config index 3a19cf23287..0d3c6df1e73 100644 --- a/configs/am62x_a53_usbdfu.config +++ b/configs/am62x_a53_usbdfu.config @@ -16,7 +16,6 @@ CONFIG_USB=y CONFIG_DM_USB_GADGET=y CONFIG_SPL_DM_USB_GADGET=y CONFIG_USB_XHCI_HCD=y -CONFIG_USB_XHCI_DWC3=y CONFIG_USB_DWC3=y CONFIG_USB_DWC3_GENERIC=y CONFIG_SPL_USB_DWC3_GENERIC=y
--- base-commit: 3073246d1be682071d8b3d07d06c2484907aed60 change-id: 20241203-am62-usb-xhci-be62bc9584c9
Best regards,
On Tue, Dec 03, 2024 at 10:40:29PM +0200, Roger Quadros wrote:
Hello Roger,
CONFIG_USB_XHCI_DWC3 is not required for AM62x as the XHCI driver is registered through the dwc3-generic driver.
CONFIG_USB_XHCI_DWC3 causes problems by hijacking the USB controller even if it is not set for Host mode in device tree.
'dm tree' output after 'usb start' is fixed from
simple_bus 5 [ + ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | |-- usb@31000000 usb 0 [ + ] xhci-dwc3 | | `-- usb@31000000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | |-- usb@31100000 usb_hub 1 [ + ] usb_hub | | | `-- usb_hub usb 1 [ + ] xhci-dwc3 | | `-- usb@31100000 usb_hub 2 [ + ] usb_hub | | `-- usb_hub
[notice that 'xhci-dwc3' and 'usb_hub' drivers are probed for both USB instances although the first instance is supposed to be 'peripheral' only]
to
simple_bus 5 [ ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | `-- usb@31000000 simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | `-- usb@31100000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub
While this fixes the issue, I am wondering if the issue lies elsewhere. In U-Boot, the compatible "snps,dwc3" is associated with: drivers/usb/host/xhci-dwc3.c while in Linux, the compatible "snps,dwc3" is associated with: drivers/usb/dwc3/core.c
So there seem to be two alternatives that I could think of: 1. Modify U-Boot to match Linux in the sense that we associate "snps,dwc3" with drivers/usb/dwc3/core.c in U-Boot. 2. With the understanding that "dr_mode" doesn't have to be host/otg for the compatible "snps,dwc3" which is tied to drivers/usb/host/xhci-dwc3.c, do the following to exit probe when "dr_mode" is "peripheral": ------------------------------------------------------------------------- diff --git a/drivers/usb/host/xhci-dwc3.c b/drivers/usb/host/xhci-dwc3.c index e3e0ceff43e..edfd7b97a73 100644 --- a/drivers/usb/host/xhci-dwc3.c +++ b/drivers/usb/host/xhci-dwc3.c @@ -208,6 +208,8 @@ static int xhci_dwc3_probe(struct udevice *dev) writel(reg, &dwc3_reg->g_usb2phycfg[0]);
dr_mode = usb_get_dr_mode(dev_ofnode(dev)); + if (dr_mode == USB_DR_MODE_PERIPHERAL) + return -ENODEV; if (dr_mode == USB_DR_MODE_OTG && dev_read_bool(dev, "usb-role-switch")) { dr_mode = usb_get_role_switch_default_mode(dev_ofnode(dev)); ------------------------------------------------------------------------- which will still show "xhci-dwc3" in the output of "dm tree" after "usb start", but the driver won't be probed (absence of "+" in the "Probed" column of "dm tree" output).
I just wanted to point this out as a possible fix for other scenarios. Disabling "CONFIG_USB_XHCI_DWC3" seems to be the best choice in the current scenario (i.e. it shouldn't have been set in "USB DFU" fragment to begin with).
Fixes: dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support") Signed-off-by: Roger Quadros rogerq@kernel.org
Reviewed-by: Siddharth Vadapalli s-vadapalli@ti.com
configs/am62x_a53_usbdfu.config | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am62x_a53_usbdfu.config b/configs/am62x_a53_usbdfu.config index 3a19cf23287..0d3c6df1e73 100644 --- a/configs/am62x_a53_usbdfu.config +++ b/configs/am62x_a53_usbdfu.config @@ -16,7 +16,6 @@ CONFIG_USB=y CONFIG_DM_USB_GADGET=y CONFIG_SPL_DM_USB_GADGET=y CONFIG_USB_XHCI_HCD=y -CONFIG_USB_XHCI_DWC3=y CONFIG_USB_DWC3=y CONFIG_USB_DWC3_GENERIC=y CONFIG_SPL_USB_DWC3_GENERIC=y
base-commit: 3073246d1be682071d8b3d07d06c2484907aed60 change-id: 20241203-am62-usb-xhci-be62bc9584c9
Best regards,
Roger Quadros rogerq@kernel.org
Hello Siddharth,
On 06/12/2024 09:19, Siddharth Vadapalli wrote:
On Tue, Dec 03, 2024 at 10:40:29PM +0200, Roger Quadros wrote:
Hello Roger,
CONFIG_USB_XHCI_DWC3 is not required for AM62x as the XHCI driver is registered through the dwc3-generic driver.
CONFIG_USB_XHCI_DWC3 causes problems by hijacking the USB controller even if it is not set for Host mode in device tree.
'dm tree' output after 'usb start' is fixed from
simple_bus 5 [ + ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | |-- usb@31000000 usb 0 [ + ] xhci-dwc3 | | `-- usb@31000000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | |-- usb@31100000 usb_hub 1 [ + ] usb_hub | | | `-- usb_hub usb 1 [ + ] xhci-dwc3 | | `-- usb@31100000 usb_hub 2 [ + ] usb_hub | | `-- usb_hub
[notice that 'xhci-dwc3' and 'usb_hub' drivers are probed for both USB instances although the first instance is supposed to be 'peripheral' only]
to
simple_bus 5 [ ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | `-- usb@31000000 simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | `-- usb@31100000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub
While this fixes the issue, I am wondering if the issue lies elsewhere. In U-Boot, the compatible "snps,dwc3" is associated with: drivers/usb/host/xhci-dwc3.c while in Linux, the compatible "snps,dwc3" is associated with: drivers/usb/dwc3/core.c
So there seem to be two alternatives that I could think of:
- Modify U-Boot to match Linux in the sense that we associate
"snps,dwc3" with drivers/usb/dwc3/core.c in U-Boot.
Many platforms (grep for CONFIG_USB_XHCI_DWC3 in configs/) use xhci-dwc3 to get Host mode working without the need for core.c. Maybe it was more simpler that way? Also see USB_XHCI_DWC3_OF_SIMPLE.
So if we drop "snps,dwc3" from xhci-dwc3, we will have to ensure each and every platform works.
- With the understanding that "dr_mode" doesn't have to be host/otg for
the compatible "snps,dwc3" which is tied to drivers/usb/host/xhci-dwc3.c, do the following to exit probe when "dr_mode" is "peripheral":
diff --git a/drivers/usb/host/xhci-dwc3.c b/drivers/usb/host/xhci-dwc3.c index e3e0ceff43e..edfd7b97a73 100644 --- a/drivers/usb/host/xhci-dwc3.c +++ b/drivers/usb/host/xhci-dwc3.c @@ -208,6 +208,8 @@ static int xhci_dwc3_probe(struct udevice *dev) writel(reg, &dwc3_reg->g_usb2phycfg[0]);
dr_mode = usb_get_dr_mode(dev_ofnode(dev));
if (dr_mode == USB_DR_MODE_PERIPHERAL)return -ENODEV; if (dr_mode == USB_DR_MODE_OTG && dev_read_bool(dev, "usb-role-switch")) { dr_mode = usb_get_role_switch_default_mode(dev_ofnode(dev));
which will still show "xhci-dwc3" in the output of "dm tree" after "usb start", but the driver won't be probed (absence of "+" in the "Probed" column of "dm tree" output).
I agree with your second proposal. But it is a separate bug (in xhci-dwc3 driver) which also fixes the issue this patch is fixing.
Regardless, I think both fixes should go in.
Could you please send a patch to fix xhci-dwc3? Thanks!
I just wanted to point this out as a possible fix for other scenarios. Disabling "CONFIG_USB_XHCI_DWC3" seems to be the best choice in the current scenario (i.e. it shouldn't have been set in "USB DFU" fragment to begin with).
Fixes: dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support") Signed-off-by: Roger Quadros rogerq@kernel.org
Reviewed-by: Siddharth Vadapalli s-vadapalli@ti.com
configs/am62x_a53_usbdfu.config | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am62x_a53_usbdfu.config b/configs/am62x_a53_usbdfu.config index 3a19cf23287..0d3c6df1e73 100644 --- a/configs/am62x_a53_usbdfu.config +++ b/configs/am62x_a53_usbdfu.config @@ -16,7 +16,6 @@ CONFIG_USB=y CONFIG_DM_USB_GADGET=y CONFIG_SPL_DM_USB_GADGET=y CONFIG_USB_XHCI_HCD=y -CONFIG_USB_XHCI_DWC3=y CONFIG_USB_DWC3=y CONFIG_USB_DWC3_GENERIC=y CONFIG_SPL_USB_DWC3_GENERIC=y
base-commit: 3073246d1be682071d8b3d07d06c2484907aed60 change-id: 20241203-am62-usb-xhci-be62bc9584c9
Best regards,
Roger Quadros rogerq@kernel.org
On Fri, Dec 06, 2024 at 11:17:28AM +0200, Roger Quadros wrote:
Hello Siddharth,
On 06/12/2024 09:19, Siddharth Vadapalli wrote:
[...]
While this fixes the issue, I am wondering if the issue lies elsewhere. In U-Boot, the compatible "snps,dwc3" is associated with: drivers/usb/host/xhci-dwc3.c while in Linux, the compatible "snps,dwc3" is associated with: drivers/usb/dwc3/core.c
So there seem to be two alternatives that I could think of:
- Modify U-Boot to match Linux in the sense that we associate
"snps,dwc3" with drivers/usb/dwc3/core.c in U-Boot.
Many platforms (grep for CONFIG_USB_XHCI_DWC3 in configs/) use xhci-dwc3 to get Host mode working without the need for core.c. Maybe it was more simpler that way? Also see USB_XHCI_DWC3_OF_SIMPLE.
So if we drop "snps,dwc3" from xhci-dwc3, we will have to ensure each and every platform works.
Yes, that will require significant effort and testing.
- With the understanding that "dr_mode" doesn't have to be host/otg for
the compatible "snps,dwc3" which is tied to drivers/usb/host/xhci-dwc3.c, do the following to exit probe when "dr_mode" is "peripheral":
diff --git a/drivers/usb/host/xhci-dwc3.c b/drivers/usb/host/xhci-dwc3.c index e3e0ceff43e..edfd7b97a73 100644 --- a/drivers/usb/host/xhci-dwc3.c +++ b/drivers/usb/host/xhci-dwc3.c @@ -208,6 +208,8 @@ static int xhci_dwc3_probe(struct udevice *dev) writel(reg, &dwc3_reg->g_usb2phycfg[0]);
dr_mode = usb_get_dr_mode(dev_ofnode(dev));
if (dr_mode == USB_DR_MODE_PERIPHERAL)return -ENODEV; if (dr_mode == USB_DR_MODE_OTG && dev_read_bool(dev, "usb-role-switch")) { dr_mode = usb_get_role_switch_default_mode(dev_ofnode(dev));
which will still show "xhci-dwc3" in the output of "dm tree" after "usb start", but the driver won't be probed (absence of "+" in the "Probed" column of "dm tree" output).
I agree with your second proposal. But it is a separate bug (in xhci-dwc3 driver) which also fixes the issue this patch is fixing.
Regardless, I think both fixes should go in.
Could you please send a patch to fix xhci-dwc3? Thanks!
Sure. Thank you for reviewing the suggestions and sharing your feedback.
Regards, Siddharth.
On 06/12/2024 11:17, Roger Quadros wrote:
Hello Siddharth,
On 06/12/2024 09:19, Siddharth Vadapalli wrote:
On Tue, Dec 03, 2024 at 10:40:29PM +0200, Roger Quadros wrote:
Hello Roger,
CONFIG_USB_XHCI_DWC3 is not required for AM62x as the XHCI driver is registered through the dwc3-generic driver.
CONFIG_USB_XHCI_DWC3 causes problems by hijacking the USB controller even if it is not set for Host mode in device tree.
'dm tree' output after 'usb start' is fixed from
simple_bus 5 [ + ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | |-- usb@31000000 usb 0 [ + ] xhci-dwc3 | | `-- usb@31000000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | |-- usb@31100000 usb_hub 1 [ + ] usb_hub | | | `-- usb_hub usb 1 [ + ] xhci-dwc3 | | `-- usb@31100000 usb_hub 2 [ + ] usb_hub | | `-- usb_hub
[notice that 'xhci-dwc3' and 'usb_hub' drivers are probed for both USB instances although the first instance is supposed to be 'peripheral' only]
to
simple_bus 5 [ ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | `-- usb@31000000 simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ + ] dwc3-generic-host | | `-- usb@31100000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub
While this fixes the issue, I am wondering if the issue lies elsewhere. In U-Boot, the compatible "snps,dwc3" is associated with: drivers/usb/host/xhci-dwc3.c while in Linux, the compatible "snps,dwc3" is associated with: drivers/usb/dwc3/core.c
So there seem to be two alternatives that I could think of:
- Modify U-Boot to match Linux in the sense that we associate
"snps,dwc3" with drivers/usb/dwc3/core.c in U-Boot.
Many platforms (grep for CONFIG_USB_XHCI_DWC3 in configs/) use xhci-dwc3 to get Host mode working without the need for core.c. Maybe it was more simpler that way? Also see USB_XHCI_DWC3_OF_SIMPLE.
So if we drop "snps,dwc3" from xhci-dwc3, we will have to ensure each and every platform works.
- With the understanding that "dr_mode" doesn't have to be host/otg for
the compatible "snps,dwc3" which is tied to drivers/usb/host/xhci-dwc3.c, do the following to exit probe when "dr_mode" is "peripheral":
diff --git a/drivers/usb/host/xhci-dwc3.c b/drivers/usb/host/xhci-dwc3.c index e3e0ceff43e..edfd7b97a73 100644 --- a/drivers/usb/host/xhci-dwc3.c +++ b/drivers/usb/host/xhci-dwc3.c @@ -208,6 +208,8 @@ static int xhci_dwc3_probe(struct udevice *dev) writel(reg, &dwc3_reg->g_usb2phycfg[0]);
dr_mode = usb_get_dr_mode(dev_ofnode(dev));
if (dr_mode == USB_DR_MODE_PERIPHERAL)return -ENODEV; if (dr_mode == USB_DR_MODE_OTG && dev_read_bool(dev, "usb-role-switch")) { dr_mode = usb_get_role_switch_default_mode(dev_ofnode(dev));
which will still show "xhci-dwc3" in the output of "dm tree" after "usb start", but the driver won't be probed (absence of "+" in the "Probed" column of "dm tree" output).
I realized later that if dr_mode == USB_DR_MODE_OTG/HOST which is the case for am62a, then xhci-dwc3.c will still be probed and we have 2 drivers probed for the same controller?
We do not want that right? So not having CONFIG_USB_XHCI_DWC3 is still required for am62*.
Maybe we still need some Kconfig guards that prevent both CONFIG_USB_XHCI_DWC3 and USB_DWC3_GENERIC to be set together as 2 drivers will claim the controller if dr_mdoe is USB_DR_MODE_OTG or USB_DR_MODE_HOST.
I agree with your second proposal. But it is a separate bug (in xhci-dwc3 driver) which also fixes the issue this patch is fixing.
Regardless, I think both fixes should go in.
Could you please send a patch to fix xhci-dwc3? Thanks!
I just wanted to point this out as a possible fix for other scenarios. Disabling "CONFIG_USB_XHCI_DWC3" seems to be the best choice in the current scenario (i.e. it shouldn't have been set in "USB DFU" fragment to begin with).
Fixes: dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support") Signed-off-by: Roger Quadros rogerq@kernel.org
Reviewed-by: Siddharth Vadapalli s-vadapalli@ti.com
configs/am62x_a53_usbdfu.config | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am62x_a53_usbdfu.config b/configs/am62x_a53_usbdfu.config index 3a19cf23287..0d3c6df1e73 100644 --- a/configs/am62x_a53_usbdfu.config +++ b/configs/am62x_a53_usbdfu.config @@ -16,7 +16,6 @@ CONFIG_USB=y CONFIG_DM_USB_GADGET=y CONFIG_SPL_DM_USB_GADGET=y CONFIG_USB_XHCI_HCD=y -CONFIG_USB_XHCI_DWC3=y CONFIG_USB_DWC3=y CONFIG_USB_DWC3_GENERIC=y CONFIG_SPL_USB_DWC3_GENERIC=y
base-commit: 3073246d1be682071d8b3d07d06c2484907aed60 change-id: 20241203-am62-usb-xhci-be62bc9584c9
Best regards,
Roger Quadros rogerq@kernel.org
On Fri, Dec 06, 2024 at 11:44:38AM +0200, Roger Quadros wrote:
On 06/12/2024 11:17, Roger Quadros wrote:
Hello Siddharth,
On 06/12/2024 09:19, Siddharth Vadapalli wrote:
[...]
- With the understanding that "dr_mode" doesn't have to be host/otg for
the compatible "snps,dwc3" which is tied to drivers/usb/host/xhci-dwc3.c, do the following to exit probe when "dr_mode" is "peripheral":
diff --git a/drivers/usb/host/xhci-dwc3.c b/drivers/usb/host/xhci-dwc3.c index e3e0ceff43e..edfd7b97a73 100644 --- a/drivers/usb/host/xhci-dwc3.c +++ b/drivers/usb/host/xhci-dwc3.c @@ -208,6 +208,8 @@ static int xhci_dwc3_probe(struct udevice *dev) writel(reg, &dwc3_reg->g_usb2phycfg[0]);
dr_mode = usb_get_dr_mode(dev_ofnode(dev));
if (dr_mode == USB_DR_MODE_PERIPHERAL)return -ENODEV; if (dr_mode == USB_DR_MODE_OTG && dev_read_bool(dev, "usb-role-switch")) { dr_mode = usb_get_role_switch_default_mode(dev_ofnode(dev));
which will still show "xhci-dwc3" in the output of "dm tree" after "usb start", but the driver won't be probed (absence of "+" in the "Probed" column of "dm tree" output).
I realized later that if dr_mode == USB_DR_MODE_OTG/HOST which is the case for am62a, then xhci-dwc3.c will still be probed and we have 2 drivers probed for the same controller?
Yes, that's true!
We do not want that right? So not having CONFIG_USB_XHCI_DWC3 is still required for am62*.
Yes.
Maybe we still need some Kconfig guards that prevent both CONFIG_USB_XHCI_DWC3 and USB_DWC3_GENERIC to be set together as 2 drivers will claim the controller if dr_mdoe is USB_DR_MODE_OTG or USB_DR_MODE_HOST.
Looking at the list of compatibles in dwc3-generic.c and focusing on the compatible "ti,keystone-dwc3", I see that this compatible is present in: dts/upstream/src/arm/ti/keystone/keystone.dtsi among other keystone device-tree files. The hierarchy of the nodes and compatibles for the USB Subsystem and the USB Controller in Keystone is similar to that of AM62*. The difference however is the following: A) For non AM62* devices: USB Subsystem [Wrapper] driver is dwc3-generic.c USB Controller [DWC3] driver is xhci-dwc3.c B) For AM62* devices: USB Subsystem [Wrapper] driver is dwc3-am62.c which exposes its own ".glue_configure" callback similar to what is done in dwc3-generic.c for other devices. USB Controller [DWC3] driver is xhci-dwc3.c So maybe non AM62* devices have: dwc3-generic.c for Subsystem + xhci-dwc3.c for USB Controller while AM62* devices have: dwc3-am62.c for Subsystem + dwc3-generic.c (neither Subsystem nor Controller?) + xhci-dwc3.c for USB Controller We could probably end up with the two driver situation similar to non AM62* devices if we update dwc3-generic.c to handle the configuration performed by dwc3-am62.c. But that still means we have two drivers: dwc3-generic.c and xhci-dwc3.c Maybe viewing the output of "dm tree" on a non AM62* device with both CONFIG_USB_XHCI_DWC3 and CONFIG_USB_DWC3_GENERIC enabled will let us know whether that is a valid configuration or not.
Regards, Siddharth.
On 06/12/2024 12:07, Siddharth Vadapalli wrote:
On Fri, Dec 06, 2024 at 11:44:38AM +0200, Roger Quadros wrote:
On 06/12/2024 11:17, Roger Quadros wrote:
Hello Siddharth,
On 06/12/2024 09:19, Siddharth Vadapalli wrote:
[...]
- With the understanding that "dr_mode" doesn't have to be host/otg for
the compatible "snps,dwc3" which is tied to drivers/usb/host/xhci-dwc3.c, do the following to exit probe when "dr_mode" is "peripheral":
diff --git a/drivers/usb/host/xhci-dwc3.c b/drivers/usb/host/xhci-dwc3.c index e3e0ceff43e..edfd7b97a73 100644 --- a/drivers/usb/host/xhci-dwc3.c +++ b/drivers/usb/host/xhci-dwc3.c @@ -208,6 +208,8 @@ static int xhci_dwc3_probe(struct udevice *dev) writel(reg, &dwc3_reg->g_usb2phycfg[0]);
dr_mode = usb_get_dr_mode(dev_ofnode(dev));
if (dr_mode == USB_DR_MODE_PERIPHERAL)return -ENODEV; if (dr_mode == USB_DR_MODE_OTG && dev_read_bool(dev, "usb-role-switch")) { dr_mode = usb_get_role_switch_default_mode(dev_ofnode(dev));
which will still show "xhci-dwc3" in the output of "dm tree" after "usb start", but the driver won't be probed (absence of "+" in the "Probed" column of "dm tree" output).
I realized later that if dr_mode == USB_DR_MODE_OTG/HOST which is the case for am62a, then xhci-dwc3.c will still be probed and we have 2 drivers probed for the same controller?
Yes, that's true!
We do not want that right? So not having CONFIG_USB_XHCI_DWC3 is still required for am62*.
Yes.
Maybe we still need some Kconfig guards that prevent both CONFIG_USB_XHCI_DWC3 and USB_DWC3_GENERIC to be set together as 2 drivers will claim the controller if dr_mdoe is USB_DR_MODE_OTG or USB_DR_MODE_HOST.
Looking at the list of compatibles in dwc3-generic.c and focusing on the compatible "ti,keystone-dwc3", I see that this compatible is present in: dts/upstream/src/arm/ti/keystone/keystone.dtsi among other keystone device-tree files. The hierarchy of the nodes and compatibles for the USB Subsystem and the USB Controller in Keystone is similar to that of AM62*. The difference however is the following: A) For non AM62* devices:
note there are 2 cases here:
A1) that enable CONFIG_USB_DWC3_GENERIC
USB Subsystem [Wrapper] driver is dwc3-generic.c
Yes.
USB Controller [DWC3] driver is xhci-dwc3.c
No.
Note that none of the platforms that enable CONFIG_USB_DWC3_GENERIC enable CONFIG_USB_XHCI_DWC3. They should not because dwc3-generic.c takes care of registering the XHCI driver via xhci_register/deregister()
A2) that don't enable CONFIG_USB_DWC3_GENERIC
These are usually only interested in host mode, they enable CONFIG_USB_XHCI_DWC3 and may also enable CONFIG_USB_XHCI_DWC3_OF_SIMPLE
So USB Wrapper/glue driver is DWC3_OF_SIMPLE USB driver is xhci-dwc3.c
Some platforms have their own glue CONFIG_USB_XHCI_STI dwc3-sti-glue.c CONFIG_USB_XHCI_OCTEON dwc3-octeon-glue.c
B) For AM62* devices: USB Subsystem [Wrapper] driver is dwc3-am62.c which exposes its own ".glue_configure" callback similar to what is done in dwc3-generic.c for other devices.
The Subsystem driver is still dwc3-generic.c. Glue driver is dwc3-am62.c
USB Controller [DWC3] driver is xhci-dwc3.c
No. We don't require CONFIG_USB_XHCI_DWC3 as XHCI registration is done by dwc3-generic. USB Host driver is USB_XHCI_HCD.
So maybe non AM62* devices have: dwc3-generic.c for Subsystem + xhci-dwc3.c for USB Controller while AM62* devices have: dwc3-am62.c for Subsystem + dwc3-generic.c (neither Subsystem nor Controller?) + xhci-dwc3.c for USB Controller We could probably end up with the two driver situation similar to non AM62* devices if we update dwc3-generic.c to handle the configuration performed by dwc3-am62.c. But that still means we have two drivers: dwc3-generic.c and xhci-dwc3.c Maybe viewing the output of "dm tree" on a non AM62* device with both CONFIG_USB_XHCI_DWC3 and CONFIG_USB_DWC3_GENERIC enabled will let us know whether that is a valid configuration or not.
Enabling both is an invalid configuration and no config does it. AM62 was doing it wrong and so we fix it with this patch.
On Mon, Dec 09, 2024 at 02:32:37PM +0200, Roger Quadros wrote:
Hello Roger,
On 06/12/2024 12:07, Siddharth Vadapalli wrote:
[...]
Looking at the list of compatibles in dwc3-generic.c and focusing on the compatible "ti,keystone-dwc3", I see that this compatible is present in: dts/upstream/src/arm/ti/keystone/keystone.dtsi among other keystone device-tree files. The hierarchy of the nodes and compatibles for the USB Subsystem and the USB Controller in Keystone is similar to that of AM62*. The difference however is the following: A) For non AM62* devices:
note there are 2 cases here:
A1) that enable CONFIG_USB_DWC3_GENERIC
USB Subsystem [Wrapper] driver is dwc3-generic.c
Yes.
USB Controller [DWC3] driver is xhci-dwc3.cNo.
Note that none of the platforms that enable CONFIG_USB_DWC3_GENERIC enable CONFIG_USB_XHCI_DWC3. They should not because dwc3-generic.c takes care of registering the XHCI driver via xhci_register/deregister()
Thank you for pointing this out.
A2) that don't enable CONFIG_USB_DWC3_GENERIC
These are usually only interested in host mode, they enable CONFIG_USB_XHCI_DWC3 and may also enable CONFIG_USB_XHCI_DWC3_OF_SIMPLE
So USB Wrapper/glue driver is DWC3_OF_SIMPLE USB driver is xhci-dwc3.c
Some platforms have their own glue CONFIG_USB_XHCI_STI dwc3-sti-glue.c CONFIG_USB_XHCI_OCTEON dwc3-octeon-glue.c
B) For AM62* devices: USB Subsystem [Wrapper] driver is dwc3-am62.c which exposes its own ".glue_configure" callback similar to what is done in dwc3-generic.c for other devices.
The Subsystem driver is still dwc3-generic.c. Glue driver is dwc3-am62.c
USB Controller [DWC3] driver is xhci-dwc3.cNo. We don't require CONFIG_USB_XHCI_DWC3 as XHCI registration is done by dwc3-generic. USB Host driver is USB_XHCI_HCD.
Ok.
So maybe non AM62* devices have: dwc3-generic.c for Subsystem + xhci-dwc3.c for USB Controller while AM62* devices have: dwc3-am62.c for Subsystem + dwc3-generic.c (neither Subsystem nor Controller?) + xhci-dwc3.c for USB Controller We could probably end up with the two driver situation similar to non AM62* devices if we update dwc3-generic.c to handle the configuration performed by dwc3-am62.c. But that still means we have two drivers: dwc3-generic.c and xhci-dwc3.c Maybe viewing the output of "dm tree" on a non AM62* device with both CONFIG_USB_XHCI_DWC3 and CONFIG_USB_DWC3_GENERIC enabled will let us know whether that is a valid configuration or not.
Enabling both is an invalid configuration and no config does it. AM62 was doing it wrong and so we fix it with this patch.
Understood. Thank you for explaining. I agree that the fix in this patch is correct and your earlier suggestion of Kconfig guards: "Maybe we still need some Kconfig guards that prevent both CONFIG_USB_XHCI_DWC3 and USB_DWC3_GENERIC to be set together as 2 drivers will claim the controller if dr_mode is USB_DR_MODE_OTG or USB_DR_MODE_HOST." could be implemented in a follow up patch.
Regards, Siddharth.
On Tue, 03 Dec 2024 22:40:29 +0200, Roger Quadros wrote:
CONFIG_USB_XHCI_DWC3 is not required for AM62x as the XHCI driver is registered through the dwc3-generic driver.
CONFIG_USB_XHCI_DWC3 causes problems by hijacking the USB controller even if it is not set for Host mode in device tree.
[...]
Applied to u-boot/master, thanks!
participants (3)
-
Roger Quadros -
Siddharth Vadapalli -
Tom Rini