Re: [PATCH] configs: am62x_evm_*: Fix USB DFU configuration

[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:
> 1. 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.

> 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 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>
>>

-- 
cheers,
-roger