[PATCH v3 00/12] Remove unnecessary USBx dr_mode configuration.

[PATCH v3 00/12] Remove unnecessary USBx dr_mode configuration.

[Julien Panis]

For all TI platforms handled in this series, USBx dr_mode
was configured as peripheral in '<board>-u-boot.dtsi'.
This series removes these fragments, and modifies dwc3 and
cdns3 USB drivers so that they override 'otg' to 'peripheral'
mode.

This follows a discussion about a similar topic for am3 boards:
https://lore.kernel.org/u-boot/20230621-fix_usb_ether_init-v4-0-5f4977bb7678@baylibre.com/

Signed-off-by: Julien Panis <jpanis@baylibre.com>
---
Changes in v3:
- Handle 'otg' mode as 'peripheral' in cdns3 usb driver, which is used
  by am64/j7200/j721e platforms.
- Link to v2: https://lore.kernel.org/r/20230706-handle-otg-as-periph-v2-0-e26c028a2b28@baylibre.com

Changes in v2:
- Handle 'unknown' mode as 'otg' in dwc3 usb driver.
- Link to v1: https://lore.kernel.org/r/20230706-handle-otg-as-periph-v1-0-bd85d15ffd50@baylibre.com

---
Julien Panis (12):
      usb: dwc3: Handle unknown mode as otg
      arm: dts: dra7-evm-u-boot: Remove usb1 mode configuration
      arm: dts: dra71-evm-u-boot: Remove usb1 mode configuration
      arm: dts: dra72-evm-revc-u-boot: Remove usb1 mode configuration
      arm: dts: dra72-evm-u-boot: Remove usb1 mode configuration
      arm: dts: dra76-evm-u-boot: Remove usb1 mode configuration
      arm: dts: keystone-k2e-evm-u-boot: Remove usb1 mode configuration
      arm: dts: k3-am654-r5-base-board-u-boot: Remove usb mode configuration
      usb: cdns3: Handle otg mode as peripheral
      arm: dts: k3-am642-evm-u-boot: Remove usb0 mode configuration
      arm: dts: k3-j7200-common-proc-board-u-boot: Remove usb0 mode configuration
      arm: dts: k3-j721e-common-proc-board-u-boot: Remove usb0 mode configuration

 arch/arm/dts/dra7-evm-u-boot.dtsi                   | 1 -
 arch/arm/dts/dra71-evm-u-boot.dtsi                  | 1 -
 arch/arm/dts/dra72-evm-revc-u-boot.dtsi             | 1 -
 arch/arm/dts/dra72-evm-u-boot.dtsi                  | 1 -
 arch/arm/dts/dra76-evm-u-boot.dtsi                  | 1 -
 arch/arm/dts/k3-am642-evm-u-boot.dtsi               | 1 -
 arch/arm/dts/k3-am654-r5-base-board-u-boot.dtsi     | 5 -----
 arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi | 1 -
 arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi | 1 -
 arch/arm/dts/keystone-k2e-evm-u-boot.dtsi           | 1 -
 drivers/usb/cdns3/core.c                            | 4 ++++
 drivers/usb/dwc3/dwc3-generic.c                     | 4 ++--
 12 files changed, 6 insertions(+), 16 deletions(-)
---
base-commit: 19b77d3d23966a0d6dbb3c86187765f11100fb6f
change-id: 20230706-handle-otg-as-periph-4546e874cd15

Best regards,
-- 
Julien Panis <jpanis@baylibre.com>

[PATCH v3 01/12] usb: dwc3: Handle unknown mode as otg

[Julien Panis]

In case dr_mode attribute is not passed via DT,
USB DRD controllers should default to OTG.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/usb/dwc3/dwc3-generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 66da5a8d6f8c..857ae806e11d 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -182,8 +182,8 @@ static int dwc3_generic_of_to_plat(struct udevice *dev)
 		node = dev_ofnode(dev->parent);
 		plat->dr_mode = usb_get_dr_mode(node);
 		if (plat->dr_mode == USB_DR_MODE_UNKNOWN) {
-			pr_err("Invalid usb mode setup\n");
-			return -ENODEV;
+			pr_info("No USB mode specified. Using OTG.\n");
+			plat->dr_mode = USB_DR_MODE_OTG;
 		}
 	}
 

-- 
2.37.3

Re: [PATCH v3 01/12] usb: dwc3: Handle unknown mode as otg

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Thu, Jul 13, 2023 at 03:45:34PM +0200, Julien Panis wrote:

> In case dr_mode attribute is not passed via DT,
> USB DRD controllers should default to OTG.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> Acked-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/usb/dwc3/dwc3-generic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
> index 66da5a8d6f8c..857ae806e11d 100644
> --- a/drivers/usb/dwc3/dwc3-generic.c
> +++ b/drivers/usb/dwc3/dwc3-generic.c
> @@ -182,8 +182,8 @@ static int dwc3_generic_of_to_plat(struct udevice *dev)
>  		node = dev_ofnode(dev->parent);
>  		plat->dr_mode = usb_get_dr_mode(node);
>  		if (plat->dr_mode == USB_DR_MODE_UNKNOWN) {
> -			pr_err("Invalid usb mode setup\n");
> -			return -ENODEV;
> +			pr_info("No USB mode specified. Using OTG.\n");
> +			plat->dr_mode = USB_DR_MODE_OTG;
>  		}
>  	}

Is this part OK with you Marek?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v3 02/12] arm: dts: dra7-evm-u-boot: Remove usb1 mode configuration

[Julien Panis]

USB1 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/dra7-evm-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/dra7-evm-u-boot.dtsi b/arch/arm/dts/dra7-evm-u-boot.dtsi
index 87b2451a8e8b..f6fd21f2c660 100644
--- a/arch/arm/dts/dra7-evm-u-boot.dtsi
+++ b/arch/arm/dts/dra7-evm-u-boot.dtsi
@@ -39,7 +39,6 @@
 
 &usb1 {
 	bootph-pre-ram;
-	dr_mode = "peripheral";
 };
 
 &usb2_phy1 {

-- 
2.37.3

[PATCH v3 03/12] arm: dts: dra71-evm-u-boot: Remove usb1 mode configuration

[Julien Panis]

USB1 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/dra71-evm-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/dra71-evm-u-boot.dtsi b/arch/arm/dts/dra71-evm-u-boot.dtsi
index 8e7dc719bf85..48a194c941d7 100644
--- a/arch/arm/dts/dra71-evm-u-boot.dtsi
+++ b/arch/arm/dts/dra71-evm-u-boot.dtsi
@@ -51,7 +51,6 @@
 
 &usb1 {
 	bootph-pre-ram;
-	dr_mode = "peripheral";
 };
 
 &usb2_phy1 {

-- 
2.37.3

[PATCH v3 04/12] arm: dts: dra72-evm-revc-u-boot: Remove usb1 mode configuration

[Julien Panis]

USB1 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/dra72-evm-revc-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/dra72-evm-revc-u-boot.dtsi b/arch/arm/dts/dra72-evm-revc-u-boot.dtsi
index 8e7dc719bf85..48a194c941d7 100644
--- a/arch/arm/dts/dra72-evm-revc-u-boot.dtsi
+++ b/arch/arm/dts/dra72-evm-revc-u-boot.dtsi
@@ -51,7 +51,6 @@
 
 &usb1 {
 	bootph-pre-ram;
-	dr_mode = "peripheral";
 };
 
 &usb2_phy1 {

-- 
2.37.3

[PATCH v3 05/12] arm: dts: dra72-evm-u-boot: Remove usb1 mode configuration

[Julien Panis]

USB1 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/dra72-evm-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/dra72-evm-u-boot.dtsi b/arch/arm/dts/dra72-evm-u-boot.dtsi
index 91a3b6b742a0..18350ed08909 100644
--- a/arch/arm/dts/dra72-evm-u-boot.dtsi
+++ b/arch/arm/dts/dra72-evm-u-boot.dtsi
@@ -11,7 +11,6 @@
 
 &usb1 {
 	bootph-pre-ram;
-	dr_mode = "peripheral";
 };
 
 &usb2_phy1 {

-- 
2.37.3

[PATCH v3 06/12] arm: dts: dra76-evm-u-boot: Remove usb1 mode configuration

[Julien Panis]

USB1 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/dra76-evm-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/dra76-evm-u-boot.dtsi b/arch/arm/dts/dra76-evm-u-boot.dtsi
index 1216d93bdcd6..47a92e214d27 100644
--- a/arch/arm/dts/dra76-evm-u-boot.dtsi
+++ b/arch/arm/dts/dra76-evm-u-boot.dtsi
@@ -31,7 +31,6 @@
 
 &usb1 {
 	bootph-pre-ram;
-	dr_mode = "peripheral";
 };
 
 &usb2_phy1 {

-- 
2.37.3

[PATCH v3 07/12] arm: dts: keystone-k2e-evm-u-boot: Remove usb1 mode configuration

[Julien Panis]

USB1 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/keystone-k2e-evm-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/keystone-k2e-evm-u-boot.dtsi b/arch/arm/dts/keystone-k2e-evm-u-boot.dtsi
index 970d452f0804..a75f78377c28 100644
--- a/arch/arm/dts/keystone-k2e-evm-u-boot.dtsi
+++ b/arch/arm/dts/keystone-k2e-evm-u-boot.dtsi
@@ -39,7 +39,6 @@
 &usb1 {
 	dwc3@25010000 {
 		phys = <&usb1_phy>;
-		dr_mode = "peripheral";
 		snps,u2ss_inp3_quirk;
 		status = "okay";
 	};

-- 
2.37.3

[PATCH v3 08/12] arm: dts: k3-am654-r5-base-board-u-boot: Remove usb mode configuration

[Julien Panis]

USB0 and USB1 dual-role feature is already handled
as peripheral only in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/k3-am654-r5-base-board-u-boot.dtsi | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/dts/k3-am654-r5-base-board-u-boot.dtsi b/arch/arm/dts/k3-am654-r5-base-board-u-boot.dtsi
index 4516ab1437e7..3ccd42985fd8 100644
--- a/arch/arm/dts/k3-am654-r5-base-board-u-boot.dtsi
+++ b/arch/arm/dts/k3-am654-r5-base-board-u-boot.dtsi
@@ -169,10 +169,6 @@
 	bootph-pre-ram;
 };
 
-&usb1 {
-	dr_mode = "peripheral";
-};
-
 &fss {
 	bootph-pre-ram;
 };
@@ -198,7 +194,6 @@
 &usb0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&usb0_pins_default>;
-	dr_mode = "peripheral";
 	bootph-pre-ram;
 };
 

-- 
2.37.3

[PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Julien Panis]

Override 'otg' to 'peripheral' mode, since 'otg' mode
is not yet supported by u-boot.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Suggested-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/usb/cdns3/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 644a9791b9c9..bd763fc593e1 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
 
 	dr_mode = best_dr_mode;
 
+	/* u-boot doesn't yet support OTG so limit to PERIPHERAL */
+	if (dr_mode == USB_DR_MODE_OTG)
+		dr_mode = USB_DR_MODE_PERIPHERAL;
+
 #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
 	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
 		ret = cdns3_host_init(cdns);

-- 
2.37.3

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:

> Override 'otg' to 'peripheral' mode, since 'otg' mode
> is not yet supported by u-boot.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> Suggested-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/usb/cdns3/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 644a9791b9c9..bd763fc593e1 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>  
>  	dr_mode = best_dr_mode;
>  
> +	/* u-boot doesn't yet support OTG so limit to PERIPHERAL */
> +	if (dr_mode == USB_DR_MODE_OTG)
> +		dr_mode = USB_DR_MODE_PERIPHERAL;
> +
>  #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>  	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>  		ret = cdns3_host_init(cdns);

Julien, why don't we support otg mode here?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Roger Quadros]

Hi Tom,

On 14/08/2023 20:17, Tom Rini wrote:
> On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
> 
>> Override 'otg' to 'peripheral' mode, since 'otg' mode
>> is not yet supported by u-boot.
>>
>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/usb/cdns3/core.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index 644a9791b9c9..bd763fc593e1 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>  
>>  	dr_mode = best_dr_mode;
>>  
>> +	/* u-boot doesn't yet support OTG so limit to PERIPHERAL */
>> +	if (dr_mode == USB_DR_MODE_OTG)
>> +		dr_mode = USB_DR_MODE_PERIPHERAL;
>> +
>>  #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>>  	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>  		ret = cdns3_host_init(cdns);
> 
> Julien, why don't we support otg mode here?
> 

dr_mode will never be OTG at this point as the previous if condition
would have forced it to PERIPHERAL.

My understanding was that u-boot USB framework doesn't support OTG mode
so we force it to PERIPHERAL.

-- 
cheers,
-roger

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On Thu, Aug 17, 2023 at 11:15:17AM +0300, Roger Quadros wrote:
> Hi Tom,
> 
> On 14/08/2023 20:17, Tom Rini wrote:
> > On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
> > 
> >> Override 'otg' to 'peripheral' mode, since 'otg' mode
> >> is not yet supported by u-boot.
> >>
> >> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> >> Suggested-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >>  drivers/usb/cdns3/core.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> >> index 644a9791b9c9..bd763fc593e1 100644
> >> --- a/drivers/usb/cdns3/core.c
> >> +++ b/drivers/usb/cdns3/core.c
> >> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
> >>  
> >>  	dr_mode = best_dr_mode;
> >>  
> >> +	/* u-boot doesn't yet support OTG so limit to PERIPHERAL */
> >> +	if (dr_mode == USB_DR_MODE_OTG)
> >> +		dr_mode = USB_DR_MODE_PERIPHERAL;
> >> +
> >>  #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
> >>  	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> >>  		ret = cdns3_host_init(cdns);
> > 
> > Julien, why don't we support otg mode here?
> > 
> 
> dr_mode will never be OTG at this point as the previous if condition
> would have forced it to PERIPHERAL.
> 
> My understanding was that u-boot USB framework doesn't support OTG mode
> so we force it to PERIPHERAL.

Well, the first part of this series is "make unknown state be OTG" for
DWC3, so we do in general (and Marek told me off-list he's used it on
DWC2 as well), so it sounds like these (CDNS3, MUSB) drivers need fixing
/ updating.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Roger Quadros]

On 18/08/2023 21:48, Tom Rini wrote:
> On Thu, Aug 17, 2023 at 11:15:17AM +0300, Roger Quadros wrote:
>> Hi Tom,
>>
>> On 14/08/2023 20:17, Tom Rini wrote:
>>> On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
>>>
>>>> Override 'otg' to 'peripheral' mode, since 'otg' mode
>>>> is not yet supported by u-boot.
>>>>
>>>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>>  drivers/usb/cdns3/core.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>> index 644a9791b9c9..bd763fc593e1 100644
>>>> --- a/drivers/usb/cdns3/core.c
>>>> +++ b/drivers/usb/cdns3/core.c
>>>> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>>  
>>>>  	dr_mode = best_dr_mode;
>>>>  
>>>> +	/* u-boot doesn't yet support OTG so limit to PERIPHERAL */
>>>> +	if (dr_mode == USB_DR_MODE_OTG)
>>>> +		dr_mode = USB_DR_MODE_PERIPHERAL;
>>>> +
>>>>  #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>>>>  	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>>  		ret = cdns3_host_init(cdns);
>>>
>>> Julien, why don't we support otg mode here?
>>>
>>
>> dr_mode will never be OTG at this point as the previous if condition
>> would have forced it to PERIPHERAL.
>>
>> My understanding was that u-boot USB framework doesn't support OTG mode
>> so we force it to PERIPHERAL.
> 
> Well, the first part of this series is "make unknown state be OTG" for
> DWC3, so we do in general (and Marek told me off-list he's used it on
> DWC2 as well), so it sounds like these (CDNS3, MUSB) drivers need fixing
> / updating.
> 

CDNS3 does not have an internal OTG state machine. We mostly really care
about role switching and not the full OTG stack. There needs to be
some kind of SW framework to do that in u-boot.

Marek could you please advise what can be done here?

-- 
cheers,
-roger

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Marek Vasut]

On 8/18/23 21:26, Roger Quadros wrote:
> 
> 
> On 18/08/2023 21:48, Tom Rini wrote:
>> On Thu, Aug 17, 2023 at 11:15:17AM +0300, Roger Quadros wrote:
>>> Hi Tom,
>>>
>>> On 14/08/2023 20:17, Tom Rini wrote:
>>>> On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
>>>>
>>>>> Override 'otg' to 'peripheral' mode, since 'otg' mode
>>>>> is not yet supported by u-boot.
>>>>>
>>>>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>>> ---
>>>>>   drivers/usb/cdns3/core.c | 4 ++++
>>>>>   1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>> index 644a9791b9c9..bd763fc593e1 100644
>>>>> --- a/drivers/usb/cdns3/core.c
>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>>>   
>>>>>   	dr_mode = best_dr_mode;
>>>>>   
>>>>> +	/* u-boot doesn't yet support OTG so limit to PERIPHERAL */
>>>>> +	if (dr_mode == USB_DR_MODE_OTG)
>>>>> +		dr_mode = USB_DR_MODE_PERIPHERAL;
>>>>> +
>>>>>   #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>>>>>   	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>>>   		ret = cdns3_host_init(cdns);
>>>>
>>>> Julien, why don't we support otg mode here?
>>>>
>>>
>>> dr_mode will never be OTG at this point as the previous if condition
>>> would have forced it to PERIPHERAL.
>>>
>>> My understanding was that u-boot USB framework doesn't support OTG mode
>>> so we force it to PERIPHERAL.
>>
>> Well, the first part of this series is "make unknown state be OTG" for
>> DWC3, so we do in general (and Marek told me off-list he's used it on
>> DWC2 as well), so it sounds like these (CDNS3, MUSB) drivers need fixing
>> / updating.
>>
> 
> CDNS3 does not have an internal OTG state machine. We mostly really care
> about role switching and not the full OTG stack. There needs to be
> some kind of SW framework to do that in u-boot.
> 
> Marek could you please advise what can be done here?

As far as I can tell, MX8MM (CI HDRC), STM32MP1 (DWC2) all can do the 
host/peripheral switching. Why is that a problem with CDNS3 ? I am not 
familiar with this controller btw.

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Roger Quadros]

+Peter & Pawel.

On 19/08/2023 02:47, Marek Vasut wrote:
> On 8/18/23 21:26, Roger Quadros wrote:
>>
>>
>> On 18/08/2023 21:48, Tom Rini wrote:
>>> On Thu, Aug 17, 2023 at 11:15:17AM +0300, Roger Quadros wrote:
>>>> Hi Tom,
>>>>
>>>> On 14/08/2023 20:17, Tom Rini wrote:
>>>>> On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
>>>>>
>>>>>> Override 'otg' to 'peripheral' mode, since 'otg' mode
>>>>>> is not yet supported by u-boot.
>>>>>>
>>>>>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>>>> ---
>>>>>>   drivers/usb/cdns3/core.c | 4 ++++
>>>>>>   1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>> index 644a9791b9c9..bd763fc593e1 100644
>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>>>>         dr_mode = best_dr_mode;
>>>>>>   +    /* u-boot doesn't yet support OTG so limit to PERIPHERAL */
>>>>>> +    if (dr_mode == USB_DR_MODE_OTG)
>>>>>> +        dr_mode = USB_DR_MODE_PERIPHERAL;
>>>>>> +
>>>>>>   #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>>>>>>       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>>>>           ret = cdns3_host_init(cdns);
>>>>>
>>>>> Julien, why don't we support otg mode here?
>>>>>
>>>>
>>>> dr_mode will never be OTG at this point as the previous if condition
>>>> would have forced it to PERIPHERAL.
>>>>
>>>> My understanding was that u-boot USB framework doesn't support OTG mode
>>>> so we force it to PERIPHERAL.
>>>
>>> Well, the first part of this series is "make unknown state be OTG" for
>>> DWC3, so we do in general (and Marek told me off-list he's used it on
>>> DWC2 as well), so it sounds like these (CDNS3, MUSB) drivers need fixing
>>> / updating.
>>>
>>
>> CDNS3 does not have an internal OTG state machine. We mostly really care
>> about role switching and not the full OTG stack. There needs to be
>> some kind of SW framework to do that in u-boot.
>>
>> Marek could you please advise what can be done here?
> 
> As far as I can tell, MX8MM (CI HDRC), STM32MP1 (DWC2) all can do the host/peripheral switching. Why is that a problem with CDNS3 ? I am not familiar with this controller btw.

So for MX8MM and STM32MP1, do  we support on the fly host/peripheral switching based on USB plug type?
As we don't have interrupts, do you keep polling for ID status change?

CDNS3 does support host/peripheral switching. i.e. cdsn3_hw_role_state_machine() and drd.c.
What we are missing is the interrupt logic to map ID pin status change to role change.

-- 
cheers,
-roger

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Marek Vasut]

On 8/19/23 09:00, Roger Quadros wrote:
> +Peter & Pawel.
> 
> On 19/08/2023 02:47, Marek Vasut wrote:
>> On 8/18/23 21:26, Roger Quadros wrote:
>>>
>>>
>>> On 18/08/2023 21:48, Tom Rini wrote:
>>>> On Thu, Aug 17, 2023 at 11:15:17AM +0300, Roger Quadros wrote:
>>>>> Hi Tom,
>>>>>
>>>>> On 14/08/2023 20:17, Tom Rini wrote:
>>>>>> On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
>>>>>>
>>>>>>> Override 'otg' to 'peripheral' mode, since 'otg' mode
>>>>>>> is not yet supported by u-boot.
>>>>>>>
>>>>>>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>>>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>>>>> ---
>>>>>>>    drivers/usb/cdns3/core.c | 4 ++++
>>>>>>>    1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>>> index 644a9791b9c9..bd763fc593e1 100644
>>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>>> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>>>>>          dr_mode = best_dr_mode;
>>>>>>>    +    /* u-boot doesn't yet support OTG so limit to PERIPHERAL */
>>>>>>> +    if (dr_mode == USB_DR_MODE_OTG)
>>>>>>> +        dr_mode = USB_DR_MODE_PERIPHERAL;
>>>>>>> +
>>>>>>>    #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>>>>>>>        if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>>>>>            ret = cdns3_host_init(cdns);
>>>>>>
>>>>>> Julien, why don't we support otg mode here?
>>>>>>
>>>>>
>>>>> dr_mode will never be OTG at this point as the previous if condition
>>>>> would have forced it to PERIPHERAL.
>>>>>
>>>>> My understanding was that u-boot USB framework doesn't support OTG mode
>>>>> so we force it to PERIPHERAL.
>>>>
>>>> Well, the first part of this series is "make unknown state be OTG" for
>>>> DWC3, so we do in general (and Marek told me off-list he's used it on
>>>> DWC2 as well), so it sounds like these (CDNS3, MUSB) drivers need fixing
>>>> / updating.
>>>>
>>>
>>> CDNS3 does not have an internal OTG state machine. We mostly really care
>>> about role switching and not the full OTG stack. There needs to be
>>> some kind of SW framework to do that in u-boot.
>>>
>>> Marek could you please advise what can be done here?
>>
>> As far as I can tell, MX8MM (CI HDRC), STM32MP1 (DWC2) all can do the host/peripheral switching. Why is that a problem with CDNS3 ? I am not familiar with this controller btw.
> 
> So for MX8MM and STM32MP1, do  we support on the fly host/peripheral switching based on USB plug type?
> As we don't have interrupts, do you keep polling for ID status change?

Nope, you just plug in one cable or the other and run the matching 
command -- either 'usb reset' for Host or 'ums/dfu/...' for Peripheral .

> CDNS3 does support host/peripheral switching. i.e. cdsn3_hw_role_state_machine() and drd.c.
> What we are missing is the interrupt logic to map ID pin status change to role change.

I don't think you need any interrupt logic for this to work ?

Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Roger Quadros]

On 19/08/2023 15:20, Marek Vasut wrote:
> On 8/19/23 09:00, Roger Quadros wrote:
>> +Peter & Pawel.
>>
>> On 19/08/2023 02:47, Marek Vasut wrote:
>>> On 8/18/23 21:26, Roger Quadros wrote:
>>>>
>>>>
>>>> On 18/08/2023 21:48, Tom Rini wrote:
>>>>> On Thu, Aug 17, 2023 at 11:15:17AM +0300, Roger Quadros wrote:
>>>>>> Hi Tom,
>>>>>>
>>>>>> On 14/08/2023 20:17, Tom Rini wrote:
>>>>>>> On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
>>>>>>>
>>>>>>>> Override 'otg' to 'peripheral' mode, since 'otg' mode
>>>>>>>> is not yet supported by u-boot.
>>>>>>>>
>>>>>>>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>>>>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>>>>>> ---
>>>>>>>>    drivers/usb/cdns3/core.c | 4 ++++
>>>>>>>>    1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>>>> index 644a9791b9c9..bd763fc593e1 100644
>>>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>>>> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>>>>>>          dr_mode = best_dr_mode;
>>>>>>>>    +    /* u-boot doesn't yet support OTG so limit to PERIPHERAL */
>>>>>>>> +    if (dr_mode == USB_DR_MODE_OTG)
>>>>>>>> +        dr_mode = USB_DR_MODE_PERIPHERAL;
>>>>>>>> +
>>>>>>>>    #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>>>>>>>>        if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>>>>>>            ret = cdns3_host_init(cdns);
>>>>>>>
>>>>>>> Julien, why don't we support otg mode here?
>>>>>>>
>>>>>>
>>>>>> dr_mode will never be OTG at this point as the previous if condition
>>>>>> would have forced it to PERIPHERAL.
>>>>>>
>>>>>> My understanding was that u-boot USB framework doesn't support OTG mode
>>>>>> so we force it to PERIPHERAL.
>>>>>
>>>>> Well, the first part of this series is "make unknown state be OTG" for
>>>>> DWC3, so we do in general (and Marek told me off-list he's used it on
>>>>> DWC2 as well), so it sounds like these (CDNS3, MUSB) drivers need fixing
>>>>> / updating.
>>>>>
>>>>
>>>> CDNS3 does not have an internal OTG state machine. We mostly really care
>>>> about role switching and not the full OTG stack. There needs to be
>>>> some kind of SW framework to do that in u-boot.
>>>>
>>>> Marek could you please advise what can be done here?
>>>
>>> As far as I can tell, MX8MM (CI HDRC), STM32MP1 (DWC2) all can do the host/peripheral switching. Why is that a problem with CDNS3 ? I am not familiar with this controller btw.
>>
>> So for MX8MM and STM32MP1, do  we support on the fly host/peripheral switching based on USB plug type?
>> As we don't have interrupts, do you keep polling for ID status change?
> 
> Nope, you just plug in one cable or the other and run the matching command -- either 'usb reset' for Host or 'ums/dfu/...' for Peripheral .

OK.
> 
>> CDNS3 does support host/peripheral switching. i.e. cdsn3_hw_role_state_machine() and drd.c.
>> What we are missing is the interrupt logic to map ID pin status change to role change.
> 
> I don't think you need any interrupt logic for this to work ?

For the use case you described, no.
Julien, I think this can be easily achieved with the current driver.
Would be nice if driver prevents user from setting incorrect role than
what cable is set to.

-- 
cheers,
-roger

[PATCH v3 10/12] arm: dts: k3-am642-evm-u-boot: Remove usb0 mode configuration

[Julien Panis]

USB0 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/k3-am642-evm-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-am642-evm-u-boot.dtsi b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
index 64857b09099d..f3f0d07df49f 100644
--- a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
+++ b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
@@ -56,7 +56,6 @@
 };
 
 &usb0 {
-	dr_mode="peripheral";
 	bootph-pre-ram;
 };
 

-- 
2.37.3

[PATCH v3 11/12] arm: dts: k3-j7200-common-proc-board-u-boot: Remove usb0 mode configuration

[Julien Panis]

USB0 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi b/arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi
index f57c2306ba1a..dbe22fc3a459 100644
--- a/arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi
+++ b/arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi
@@ -157,7 +157,6 @@
 };
 
 &usb0 {
-	dr_mode = "peripheral";
 	bootph-pre-ram;
 };
 

-- 
2.37.3

[PATCH v3 12/12] arm: dts: k3-j721e-common-proc-board-u-boot: Remove usb0 mode configuration

[Julien Panis]

USB0 dual-role feature is already handled as peripheral only
in dwc3-generic driver.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Acked-by: Roger Quadros <rogerq@kernel.org>
---
 arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi b/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
index 867ec2bb1aff..d14f3346caf7 100644
--- a/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
+++ b/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
@@ -143,7 +143,6 @@
 };
 
 &usb0 {
-	dr_mode = "peripheral";
 	bootph-pre-ram;
 };
 

-- 
2.37.3

Re: [PATCH v3 00/12] Remove unnecessary USBx dr_mode configuration.

[Roger Quadros]

On 13/07/2023 16:45, Julien Panis wrote:
> For all TI platforms handled in this series, USBx dr_mode
> was configured as peripheral in '<board>-u-boot.dtsi'.
> This series removes these fragments, and modifies dwc3 and
> cdns3 USB drivers so that they override 'otg' to 'peripheral'
> mode.
> 
> This follows a discussion about a similar topic for am3 boards:
> https://lore.kernel.org/u-boot/20230621-fix_usb_ether_init-v4-0-5f4977bb7678@baylibre.com/
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> ---
> Changes in v3:
> - Handle 'otg' mode as 'peripheral' in cdns3 usb driver, which is used
>   by am64/j7200/j721e platforms.
> - Link to v2: https://lore.kernel.org/r/20230706-handle-otg-as-periph-v2-0-e26c028a2b28@baylibre.com
> 
> Changes in v2:
> - Handle 'unknown' mode as 'otg' in dwc3 usb driver.
> - Link to v1: https://lore.kernel.org/r/20230706-handle-otg-as-periph-v1-0-bd85d15ffd50@baylibre.com
> 
> ---
> Julien Panis (12):
>       usb: dwc3: Handle unknown mode as otg
>       arm: dts: dra7-evm-u-boot: Remove usb1 mode configuration
>       arm: dts: dra71-evm-u-boot: Remove usb1 mode configuration
>       arm: dts: dra72-evm-revc-u-boot: Remove usb1 mode configuration
>       arm: dts: dra72-evm-u-boot: Remove usb1 mode configuration
>       arm: dts: dra76-evm-u-boot: Remove usb1 mode configuration
>       arm: dts: keystone-k2e-evm-u-boot: Remove usb1 mode configuration
>       arm: dts: k3-am654-r5-base-board-u-boot: Remove usb mode configuration
>       usb: cdns3: Handle otg mode as peripheral
>       arm: dts: k3-am642-evm-u-boot: Remove usb0 mode configuration
>       arm: dts: k3-j7200-common-proc-board-u-boot: Remove usb0 mode configuration
>       arm: dts: k3-j721e-common-proc-board-u-boot: Remove usb0 mode configuration
> 
>  arch/arm/dts/dra7-evm-u-boot.dtsi                   | 1 -
>  arch/arm/dts/dra71-evm-u-boot.dtsi                  | 1 -
>  arch/arm/dts/dra72-evm-revc-u-boot.dtsi             | 1 -
>  arch/arm/dts/dra72-evm-u-boot.dtsi                  | 1 -
>  arch/arm/dts/dra76-evm-u-boot.dtsi                  | 1 -
>  arch/arm/dts/k3-am642-evm-u-boot.dtsi               | 1 -
>  arch/arm/dts/k3-am654-r5-base-board-u-boot.dtsi     | 5 -----
>  arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi | 1 -
>  arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi | 1 -
>  arch/arm/dts/keystone-k2e-evm-u-boot.dtsi           | 1 -
>  drivers/usb/cdns3/core.c                            | 4 ++++
>  drivers/usb/dwc3/dwc3-generic.c                     | 4 ++--
>  12 files changed, 6 insertions(+), 16 deletions(-)
> ---
> base-commit: 19b77d3d23966a0d6dbb3c86187765f11100fb6f
> change-id: 20230706-handle-otg-as-periph-4546e874cd15
> 

for this series:
Acked-by: Roger Quadros <rogerq@kernel.org>

-- 
cheers,
-roger