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

[Roger Quadros]

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 at f900000
>>>  usb_gadget    0  [   ]   dwc3-generic-periphe  |   |   |-- usb at 31000000
>>>  usb           0  [ + ]   xhci-dwc3             |   |   `-- usb at 31000000
>>>  usb_hub       0  [ + ]   usb_hub               |   |       `-- usb_hub
>>>  simple_bus    6  [ + ]   dwc3-am62             |   |-- dwc3-usb at f910000
>>>  usb           1  [ + ]   dwc3-generic-host     |   |   |-- usb at 31100000
>>>  usb_hub       1  [ + ]   usb_hub               |   |   |   `-- usb_hub
>>>  usb           1  [ + ]   xhci-dwc3             |   |   `-- usb at 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 at f900000
>>>  usb_gadget    0  [   ]   dwc3-generic-periphe  |   |   `-- usb at 31000000
>>>  simple_bus    6  [ + ]   dwc3-am62             |   |-- dwc3-usb at f910000
>>>  usb           1  [ + ]   dwc3-generic-host     |   |   `-- usb at 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 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 at kernel.org>
>>
>> Reviewed-by: Siddharth Vadapalli <s-vadapalli at 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 at kernel.org>
>>>
> 

-- 
cheers,
-roger