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

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

[Roger Quadros]

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,
-- 
Roger Quadros <rogerq@kernel.org>

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

[Siddharth Vadapalli]

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

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

[Roger Quadros]

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

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

[Siddharth Vadapalli]

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

Yes, that will require significant effort and testing.

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

Sure. Thank you for reviewing the suggestions and sharing your feedback.

Regards,
Siddharth.

Re: [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@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 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>
>>>
> 

-- 
cheers,
-roger

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

[Siddharth Vadapalli]

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

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

[Roger Quadros]

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

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

[Siddharth Vadapalli]

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.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()

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

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.

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

[Tom Rini]

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!

-- 
Tom