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

[Roger Quadros <rogerq@kernel.org>]

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:
> 
> [...]
> 
>>>> 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?
> 
> 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.

-- 
cheers,
-roger