[PATCH v2 0/2] AM62A DWC3: Add support for USB DFU boot in OTG mode

[PATCH v2 0/2] AM62A DWC3: Add support for USB DFU boot in OTG mode

[Siddharth Vadapalli]

Hello,

This series adds support for USB DFU boot on TI's AM62A SoC which has
two instances of DWC3 USB Controllers namely USB0 and USB1. The USB0
instance of the USB Controller supports USB DFU boot:
ROM => tiboot3.bin => tispl.bin => u-boot.img

USB DFU Boot requires the USB Controller to be configured for Gadget
mode of operation. Since the USB0 instance of the DWC3 USB Controller
supports both Host and Gadget modes of operation via the Type-C interface
on the AM62A7-SK board, the device-tree specifies the "dr_mode" as "OTG".

The function dwc3_generic_probe() is invoked by either of:
1. dwc3_generic_peripheral_probe()
2. dwc3_generic_host_probe()
on the basis of the command executed by the user for either device or
host mode of operation respectively. Currently, dwc3_generic_probe()
sets the mode based on the device-tree rather than setting it on the
basis of the caller. While this might be correct when the mode is "host"
or "peripheral" in the device-tree, it is wrong when the mode is "otg"
in the device-tree. It is wrong because of two reasons:
1. There is no OTG state machine in U-Boot. Hence the role will never
switch to the correct one eventually among host or peripheral.
2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
register of the USB Controller being set to 11b. According to the
datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
should never be set to any value other than 01b (Host) and 10b (Device).
Quoting the datasheet:
"Programming this field with random data causes the controller
to keep toggling between the host mode and the device mode."

Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
that the caller specifies the intended role, rather than simply using
the "dr_mode" property to set the role, set the role on the basis of the
caller (intended role) and the device-tree (platform support).

This could be extended further to utilize the USB ID pin to determine
the expected role, but that can be implemented as an incremental
feature.

v1:
https://patchwork.ozlabs.org/project/uboot/list/?series=434253&state=%2A&archive=both
Changes since v1:
- Rebased series on commit
  acab6e78aca common: relocate fdt_blob in global_data for FDTSRC_EMBED case
  of the master branch of U-Boot.
- Based on the feedback received on the v1 series, the device-tree
  specified role is also taken into account in dwc3_generic_probe(), in
  addition to the caller of dwc3_generic_probe().

In addition to the patches in this series, the following device-tree
changes will be required to test USB DFU on AM62A (bootph-all property
to be added to ensure that USB Controller is present at all stages
for DFU Boot):
https://gist.github.com/Siddharth-Vadapalli-at-TI/53ba02cb0ff4a09c47e920d08247065f
The above device-tree changes will be made to the Linux device-tree,
which shall ensure that the same shall be a part of U-Boot device-tree
eventually.

The USB DFU config fragments for AM62x have been used for enabling
USB DFU boot on AM62a as follows:
R5  => am62ax_evm_r5_defconfig + am62x_r5_usbdfu.config
A53 => am62ax_evm_a53_defconfig + am62x_a53_usbdfu.config

Logs validating USB DFU boot with this series on AM62A7-SK:
https://gist.github.com/Siddharth-Vadapalli-at-TI/13f9a539ef3297ab2822bc1bc1976a7f

Regards,
Siddharth.

Siddharth Vadapalli (2):
  usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()
  board: ti: am62ax: env: include environment for DFU

 board/ti/am62ax/am62ax.env      |  1 +
 drivers/usb/dwc3/dwc3-generic.c | 21 +++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Siddharth Vadapalli]

There are only two callers of "dwc3_generic_probe()", namely:
1. dwc3_generic_peripheral_probe()
2. dwc3_generic_host_probe()
Currently, the "mode" is set based on the device-tree node of the
platform device. Also, the DWC3 core doesn't support updating the "mode"
dynamically at runtime if it is set to "OTG", i.e. "OTG" is treated as a
separate mode in itself, rather than being treated as a mode which should
eventually lead to "host"/"peripheral".

Given that the callers of "dwc3_generic_probe()" clarify the expected
"mode" of the USB Controller, use that "mode" instead of the one
specified in the device-tree. This shall allow the USB Controller to
function both as a "Host" and as a "Peripheral" when the "mode" is "otg"
in the device-tree, based on the caller of "dwc3_generic_probe()".

Ideally, the USB ID pin should be used to determine whether or not the
requested role can be enabled. However, that can be implemented in the
future as an incremental feature over the current implementation.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

v1:
https://patchwork.ozlabs.org/project/uboot/list/?series=434253&state=%2A&archive=both
Changes since v1:
- Based on the feedback received on the v1 series, the device-tree
  specified role is also taken into account in dwc3_generic_probe(), in
  addition to the caller of dwc3_generic_probe().

 drivers/usb/dwc3/dwc3-generic.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 2ab41cbae45..fe98b50c42c 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
 };
 
 static int dwc3_generic_probe(struct udevice *dev,
-			      struct dwc3_generic_priv *priv)
+			      struct dwc3_generic_priv *priv,
+			      enum usb_dr_mode mode)
 {
 	int rc;
 	struct dwc3_generic_plat *plat = dev_get_plat(dev);
@@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
 
 	dwc3->dev = dev;
 	dwc3->maximum_speed = plat->maximum_speed;
-	dwc3->dr_mode = plat->dr_mode;
+
+	/*
+	 * If the controller supports OTG as indicated by plat->dr_mode,
+	 * then either Host or Peripheral mode is acceptable.
+	 * Otherwise, error out since the platform cannot support the mode
+	 * being requested by the caller of dwc3_generic_probe().
+	 */
+	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
+		pr_err("Requested usb mode is not supported by platform\n");
+		return -EINVAL;
+	}
+	dwc3->dr_mode = mode;
+
 #if CONFIG_IS_ENABLED(OF_CONTROL)
 	dwc3_of_parse(dwc3);
 
@@ -197,7 +210,7 @@ static int dwc3_generic_peripheral_probe(struct udevice *dev)
 {
 	struct dwc3_generic_priv *priv = dev_get_priv(dev);
 
-	return dwc3_generic_probe(dev, priv);
+	return dwc3_generic_probe(dev, priv, USB_DR_MODE_PERIPHERAL);
 }
 
 static int dwc3_generic_peripheral_remove(struct udevice *dev)
@@ -241,7 +254,7 @@ static int dwc3_generic_host_probe(struct udevice *dev)
 	struct dwc3_generic_host_priv *priv = dev_get_priv(dev);
 	int rc;
 
-	rc = dwc3_generic_probe(dev, &priv->gen_priv);
+	rc = dwc3_generic_probe(dev, &priv->gen_priv, USB_DR_MODE_HOST);
 	if (rc)
 		return rc;
 
-- 
2.43.0

[PATCH v2 2/2] board: ti: am62ax: env: include environment for DFU

[Siddharth Vadapalli]

Include the TI K3 DFU environment to support DFU Boot and DFU Flash.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

v1:
https://patchwork.ozlabs.org/project/uboot/list/?series=434253&state=%2A&archive=both
No changes since v1.

 board/ti/am62ax/am62ax.env | 1 +
 1 file changed, 1 insertion(+)

diff --git a/board/ti/am62ax/am62ax.env b/board/ti/am62ax/am62ax.env
index 97122fb57ba..96d9e1e2797 100644
--- a/board/ti/am62ax/am62ax.env
+++ b/board/ti/am62ax/am62ax.env
@@ -1,5 +1,6 @@
 #include <env/ti/ti_common.env>
 #include <env/ti/mmc.env>
+#include <env/ti/k3_dfu.env>
 #if CONFIG_CMD_REMOTEPROC
 #include <env/ti/k3_rproc.env>
 #endif
-- 
2.43.0

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros]

Hi Siddharth,


On 03/12/2024 11:37, Siddharth Vadapalli wrote:
> There are only two callers of "dwc3_generic_probe()", namely:
> 1. dwc3_generic_peripheral_probe()
> 2. dwc3_generic_host_probe()
> Currently, the "mode" is set based on the device-tree node of the
> platform device. Also, the DWC3 core doesn't support updating the "mode"
> dynamically at runtime if it is set to "OTG", i.e. "OTG" is treated as a
> separate mode in itself, rather than being treated as a mode which should
> eventually lead to "host"/"peripheral".

Actually this is not entirely true. Sorry for not catching this earlier.
I did a deep dive today to debug why XHCI is being registered for both ports on am62.

I see that configs/am62x_a53_usbdfu.config is setting CONFIG_USB_XHCI_DWC3.
This is wrong. It should not be used for AM62x platform.

Also.

Please see dwc3_glue_bind_common().

        if (CONFIG_IS_ENABLED(DM_USB_GADGET) &&
            (dr_mode == USB_DR_MODE_PERIPHERAL || dr_mode == USB_DR_MODE_OTG)) {
                printf("%s: dr_mode: OTG or Peripheral\n", __func__);
                driver = "dwc3-generic-peripheral";
        } else if (CONFIG_IS_ENABLED(USB_HOST) && dr_mode == USB_DR_MODE_HOST) {
                printf("%s: dr_mode: HOST\n", __func__);
                driver = "dwc3-generic-host";
        } else {
                printf("%s: unsupported dr_mode %d\n", __func__, dr_mode);
                return -ENODEV;
        }

        ret = device_bind_driver_to_node(parent, driver, name,
                                         node, &dev);
        if (ret) {


So if dr_mode is "otg" it will be bound to dwc3-generic-peripheral driver only.
I verified that even if I change dr_mode from "peripheral" to "otg" DFU mode
does work from u-boot shell.

Which makes my suggestions for checking dr_mode during and your patch seem unnecessary.

What is not clear though is, why is DFU mode not working for you. At which stage
DFU is failing? R5 SPL/A53 SPL?

Can you please try disabling CONFIG_USB_XHCI_DWC3 in all configs as it will
hijack the controller exclusively for host mode.

> 
> Given that the callers of "dwc3_generic_probe()" clarify the expected
> "mode" of the USB Controller, use that "mode" instead of the one
> specified in the device-tree. This shall allow the USB Controller to
> function both as a "Host" and as a "Peripheral" when the "mode" is "otg"
> in the device-tree, based on the caller of "dwc3_generic_probe()".
> 
> Ideally, the USB ID pin should be used to determine whether or not the
> requested role can be enabled. However, that can be implemented in the
> future as an incremental feature over the current implementation.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
> 
> v1:
> https://patchwork.ozlabs.org/project/uboot/list/?series=434253&state=%2A&archive=both
> Changes since v1:
> - Based on the feedback received on the v1 series, the device-tree
>   specified role is also taken into account in dwc3_generic_probe(), in
>   addition to the caller of dwc3_generic_probe().
> 
>  drivers/usb/dwc3/dwc3-generic.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
> index 2ab41cbae45..fe98b50c42c 100644
> --- a/drivers/usb/dwc3/dwc3-generic.c
> +++ b/drivers/usb/dwc3/dwc3-generic.c
> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
>  };
>  
>  static int dwc3_generic_probe(struct udevice *dev,
> -			      struct dwc3_generic_priv *priv)
> +			      struct dwc3_generic_priv *priv,
> +			      enum usb_dr_mode mode)

This is not necessary as dwc3_generic_host_probe will only be
registered if dr_mode == "host" and dwc3_generic_peripheral_probe
will only be registered if dr_mode == "host" or "peripheral"

>  {
>  	int rc;
>  	struct dwc3_generic_plat *plat = dev_get_plat(dev);
> @@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
>  
>  	dwc3->dev = dev;
>  	dwc3->maximum_speed = plat->maximum_speed;
> -	dwc3->dr_mode = plat->dr_mode;
> +
> +	/*
> +	 * If the controller supports OTG as indicated by plat->dr_mode,
> +	 * then either Host or Peripheral mode is acceptable.
> +	 * Otherwise, error out since the platform cannot support the mode
> +	 * being requested by the caller of dwc3_generic_probe().
> +	 */
> +	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
> +		pr_err("Requested usb mode is not supported by platform\n");

This is actually not necessary. Sorry for suggesting this approach before.

> +		return -EINVAL;
> +	}
> +	dwc3->dr_mode = mode;
> +
>  #if CONFIG_IS_ENABLED(OF_CONTROL)
>  	dwc3_of_parse(dwc3);
>  
> @@ -197,7 +210,7 @@ static int dwc3_generic_peripheral_probe(struct udevice *dev)
>  {
>  	struct dwc3_generic_priv *priv = dev_get_priv(dev);
>  
> -	return dwc3_generic_probe(dev, priv);
> +	return dwc3_generic_probe(dev, priv, USB_DR_MODE_PERIPHERAL);
>  }
>  
>  static int dwc3_generic_peripheral_remove(struct udevice *dev)
> @@ -241,7 +254,7 @@ static int dwc3_generic_host_probe(struct udevice *dev)
>  	struct dwc3_generic_host_priv *priv = dev_get_priv(dev);
>  	int rc;
>  
> -	rc = dwc3_generic_probe(dev, &priv->gen_priv);
> +	rc = dwc3_generic_probe(dev, &priv->gen_priv, USB_DR_MODE_HOST);
>  	if (rc)
>  		return rc;
>  

-- 
cheers,
-roger

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Siddharth Vadapalli]

On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:

Hello Roger,

> Hi Siddharth,
> 
> 
> On 03/12/2024 11:37, Siddharth Vadapalli wrote:
> > There are only two callers of "dwc3_generic_probe()", namely:
> > 1. dwc3_generic_peripheral_probe()
> > 2. dwc3_generic_host_probe()
> > Currently, the "mode" is set based on the device-tree node of the
> > platform device. Also, the DWC3 core doesn't support updating the "mode"
> > dynamically at runtime if it is set to "OTG", i.e. "OTG" is treated as a
> > separate mode in itself, rather than being treated as a mode which should
> > eventually lead to "host"/"peripheral".
> 
> Actually this is not entirely true. Sorry for not catching this earlier.
> I did a deep dive today to debug why XHCI is being registered for both ports on am62.
> 
> I see that configs/am62x_a53_usbdfu.config is setting CONFIG_USB_XHCI_DWC3.
> This is wrong. It should not be used for AM62x platform.

The issue however is at R5 SPL stage and not A53 SPL/U-Boot.
configs/am62x_r5_usbdfu.config doesn't have CONFIG_USB_XHCI_DWC3
and I run into the issue where the "alt_info" for tispl and u-boot doesn't
show up when tiboot3.bin built for USB DFU boot is sent using dfu-util.

Please test USB DFU boot on AM625-SK with dr_mode set to "otg" in the
device-tree. That should help you recreate the issue that I am observing
with AM62A7-SK.

> 
> Also.
> 
> Please see dwc3_glue_bind_common().
> 
>         if (CONFIG_IS_ENABLED(DM_USB_GADGET) &&
>             (dr_mode == USB_DR_MODE_PERIPHERAL || dr_mode == USB_DR_MODE_OTG)) {
>                 printf("%s: dr_mode: OTG or Peripheral\n", __func__);
>                 driver = "dwc3-generic-peripheral";
>         } else if (CONFIG_IS_ENABLED(USB_HOST) && dr_mode == USB_DR_MODE_HOST) {
>                 printf("%s: dr_mode: HOST\n", __func__);
>                 driver = "dwc3-generic-host";
>         } else {
>                 printf("%s: unsupported dr_mode %d\n", __func__, dr_mode);
>                 return -ENODEV;
>         }
> 
>         ret = device_bind_driver_to_node(parent, driver, name,
>                                          node, &dev);
>         if (ret) {
> 
> 
> So if dr_mode is "otg" it will be bound to dwc3-generic-peripheral driver only.
> I verified that even if I change dr_mode from "peripheral" to "otg" DFU mode
> does work from u-boot shell.
> 
> Which makes my suggestions for checking dr_mode during and your patch seem unnecessary.
> 
> What is not clear though is, why is DFU mode not working for you. At which stage
> DFU is failing? R5 SPL/A53 SPL?

R5 SPL.

> 
> Can you please try disabling CONFIG_USB_XHCI_DWC3 in all configs as it will
> hijack the controller exclusively for host mode.

It is already disabled as mentioned above.

> 
> > 
> > Given that the callers of "dwc3_generic_probe()" clarify the expected
> > "mode" of the USB Controller, use that "mode" instead of the one
> > specified in the device-tree. This shall allow the USB Controller to
> > function both as a "Host" and as a "Peripheral" when the "mode" is "otg"
> > in the device-tree, based on the caller of "dwc3_generic_probe()".
> > 
> > Ideally, the USB ID pin should be used to determine whether or not the
> > requested role can be enabled. However, that can be implemented in the
> > future as an incremental feature over the current implementation.
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> > 
> > v1:
> > https://patchwork.ozlabs.org/project/uboot/list/?series=434253&state=%2A&archive=both
> > Changes since v1:
> > - Based on the feedback received on the v1 series, the device-tree
> >   specified role is also taken into account in dwc3_generic_probe(), in
> >   addition to the caller of dwc3_generic_probe().
> > 
> >  drivers/usb/dwc3/dwc3-generic.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
> > index 2ab41cbae45..fe98b50c42c 100644
> > --- a/drivers/usb/dwc3/dwc3-generic.c
> > +++ b/drivers/usb/dwc3/dwc3-generic.c
> > @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
> >  };
> >  
> >  static int dwc3_generic_probe(struct udevice *dev,
> > -			      struct dwc3_generic_priv *priv)
> > +			      struct dwc3_generic_priv *priv,
> > +			      enum usb_dr_mode mode)
> 
> This is not necessary as dwc3_generic_host_probe will only be
> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
> will only be registered if dr_mode == "host" or "peripheral"

But I don't see it happening that way. dwc3_generic_host_probe() or
dwc3_generic_peripheral_probe() seem to be invoked based on the command
executed - USB Host command will trigger generic_host_probe() while USB
Device/Gadget command will trigger generic_peripheral_probe().


> 
> >  {
> >  	int rc;
> >  	struct dwc3_generic_plat *plat = dev_get_plat(dev);
> > @@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
> >  
> >  	dwc3->dev = dev;
> >  	dwc3->maximum_speed = plat->maximum_speed;
> > -	dwc3->dr_mode = plat->dr_mode;
> > +
> > +	/*
> > +	 * If the controller supports OTG as indicated by plat->dr_mode,
> > +	 * then either Host or Peripheral mode is acceptable.
> > +	 * Otherwise, error out since the platform cannot support the mode
> > +	 * being requested by the caller of dwc3_generic_probe().
> > +	 */
> > +	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
> > +		pr_err("Requested usb mode is not supported by platform\n");
> 
> This is actually not necessary. Sorry for suggesting this approach before.

While the check may not be necessary, dr_mode *should* be set based on
the caller and not based on plat->dr_mode. The reason I say so is that
plat->dr_mode could be "otg" in the device-tree. But "otg" shouldn't be
used for further configuration. I had indicated the reason for this in
the cover-letter, which I am pasting below for your reference:

---------------------------------------------------------------------------
Currently, dwc3_generic_probe() sets the mode based on the device-tree
rather than setting it on the basis of the caller. While this might be
correct when the mode is "host" or "peripheral" in the device-tree, it is
wrong when the mode is "otg" in the device-tree. It is wrong because of two
reasons:
1. There is no OTG state machine in U-Boot. Hence the role will never
switch to the correct one eventually among host or peripheral.
2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
register of the USB Controller being set to 11b. According to the
datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
should never be set to any value other than 01b (Host) and 10b (Device).
Quoting the datasheet:
"Programming this field with random data causes the controller
to keep toggling between the host mode and the device mode."

Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
that the caller specifies the intended role, rather than simply using
the "dr_mode" property to set the role, set the role on the basis of the
caller (intended role) and the device-tree (platform support).
----------------------------------------------------------------------------

Point #2 above is the main reason why dr_mode cannot be set to "otg" in
dwc3_generic_probe(). The only difference now is that the check can be
dropped as you have indicated, so the last paragraph above will change
to:
...set the role on the basis of the caller (intended role).
Device-tree will no longer be used i.e. plat->dr_mode shouldn't be used.

Regards,
Siddharth.

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros]

On 04/12/2024 07:16, Siddharth Vadapalli wrote:
> On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:
> 
> Hello Roger,
> 
>> Hi Siddharth,
>>
>>
>> On 03/12/2024 11:37, Siddharth Vadapalli wrote:
>>> There are only two callers of "dwc3_generic_probe()", namely:
>>> 1. dwc3_generic_peripheral_probe()
>>> 2. dwc3_generic_host_probe()
>>> Currently, the "mode" is set based on the device-tree node of the
>>> platform device. Also, the DWC3 core doesn't support updating the "mode"
>>> dynamically at runtime if it is set to "OTG", i.e. "OTG" is treated as a
>>> separate mode in itself, rather than being treated as a mode which should
>>> eventually lead to "host"/"peripheral".
>>
>> Actually this is not entirely true. Sorry for not catching this earlier.
>> I did a deep dive today to debug why XHCI is being registered for both ports on am62.
>>
>> I see that configs/am62x_a53_usbdfu.config is setting CONFIG_USB_XHCI_DWC3.
>> This is wrong. It should not be used for AM62x platform.
> 
> The issue however is at R5 SPL stage and not A53 SPL/U-Boot.
> configs/am62x_r5_usbdfu.config doesn't have CONFIG_USB_XHCI_DWC3
> and I run into the issue where the "alt_info" for tispl and u-boot doesn't
> show up when tiboot3.bin built for USB DFU boot is sent using dfu-util.
> 
> Please test USB DFU boot on AM625-SK with dr_mode set to "otg" in the
> device-tree. That should help you recreate the issue that I am observing
> with AM62A7-SK.
> 
>>
>> Also.
>>
>> Please see dwc3_glue_bind_common().
>>
>>         if (CONFIG_IS_ENABLED(DM_USB_GADGET) &&
>>             (dr_mode == USB_DR_MODE_PERIPHERAL || dr_mode == USB_DR_MODE_OTG)) {
>>                 printf("%s: dr_mode: OTG or Peripheral\n", __func__);
>>                 driver = "dwc3-generic-peripheral";
>>         } else if (CONFIG_IS_ENABLED(USB_HOST) && dr_mode == USB_DR_MODE_HOST) {
>>                 printf("%s: dr_mode: HOST\n", __func__);
>>                 driver = "dwc3-generic-host";
>>         } else {
>>                 printf("%s: unsupported dr_mode %d\n", __func__, dr_mode);
>>                 return -ENODEV;
>>         }
>>
>>         ret = device_bind_driver_to_node(parent, driver, name,
>>                                          node, &dev);
>>         if (ret) {
>>
>>
>> So if dr_mode is "otg" it will be bound to dwc3-generic-peripheral driver only.
>> I verified that even if I change dr_mode from "peripheral" to "otg" DFU mode
>> does work from u-boot shell.
>>
>> Which makes my suggestions for checking dr_mode during and your patch seem unnecessary.
>>
>> What is not clear though is, why is DFU mode not working for you. At which stage
>> DFU is failing? R5 SPL/A53 SPL?
> 
> R5 SPL.
> 
>>
>> Can you please try disabling CONFIG_USB_XHCI_DWC3 in all configs as it will
>> hijack the controller exclusively for host mode.
> 
> It is already disabled as mentioned above.
> 
>>
>>>
>>> Given that the callers of "dwc3_generic_probe()" clarify the expected
>>> "mode" of the USB Controller, use that "mode" instead of the one
>>> specified in the device-tree. This shall allow the USB Controller to
>>> function both as a "Host" and as a "Peripheral" when the "mode" is "otg"
>>> in the device-tree, based on the caller of "dwc3_generic_probe()".
>>>
>>> Ideally, the USB ID pin should be used to determine whether or not the
>>> requested role can be enabled. However, that can be implemented in the
>>> future as an incremental feature over the current implementation.
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>
>>> v1:
>>> https://patchwork.ozlabs.org/project/uboot/list/?series=434253&state=%2A&archive=both
>>> Changes since v1:
>>> - Based on the feedback received on the v1 series, the device-tree
>>>   specified role is also taken into account in dwc3_generic_probe(), in
>>>   addition to the caller of dwc3_generic_probe().
>>>
>>>  drivers/usb/dwc3/dwc3-generic.c | 21 +++++++++++++++++----
>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
>>> index 2ab41cbae45..fe98b50c42c 100644
>>> --- a/drivers/usb/dwc3/dwc3-generic.c
>>> +++ b/drivers/usb/dwc3/dwc3-generic.c
>>> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
>>>  };
>>>  
>>>  static int dwc3_generic_probe(struct udevice *dev,
>>> -			      struct dwc3_generic_priv *priv)
>>> +			      struct dwc3_generic_priv *priv,
>>> +			      enum usb_dr_mode mode)
>>
>> This is not necessary as dwc3_generic_host_probe will only be
>> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
>> will only be registered if dr_mode == "host" or "peripheral"
> 
> But I don't see it happening that way. dwc3_generic_host_probe() or
> dwc3_generic_peripheral_probe() seem to be invoked based on the command
> executed - USB Host command will trigger generic_host_probe() while USB
> Device/Gadget command will trigger generic_peripheral_probe().
> 

But, when dr_mode is "otg" generic host driver is *not* bound to the
DWC3 device. So dwc3_generic_host_probe() will never be called for it.

> 
>>
>>>  {
>>>  	int rc;
>>>  	struct dwc3_generic_plat *plat = dev_get_plat(dev);
>>> @@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
>>>  
>>>  	dwc3->dev = dev;
>>>  	dwc3->maximum_speed = plat->maximum_speed;
>>> -	dwc3->dr_mode = plat->dr_mode;
>>> +
>>> +	/*
>>> +	 * If the controller supports OTG as indicated by plat->dr_mode,
>>> +	 * then either Host or Peripheral mode is acceptable.
>>> +	 * Otherwise, error out since the platform cannot support the mode
>>> +	 * being requested by the caller of dwc3_generic_probe().
>>> +	 */
>>> +	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
>>> +		pr_err("Requested usb mode is not supported by platform\n");
>>
>> This is actually not necessary. Sorry for suggesting this approach before.
> 
> While the check may not be necessary, dr_mode *should* be set based on
> the caller and not based on plat->dr_mode. The reason I say so is that
> plat->dr_mode could be "otg" in the device-tree. But "otg" shouldn't be
> used for further configuration. I had indicated the reason for this in
> the cover-letter, which I am pasting below for your reference:
> 
> ---------------------------------------------------------------------------
> Currently, dwc3_generic_probe() sets the mode based on the device-tree
> rather than setting it on the basis of the caller. While this might be
> correct when the mode is "host" or "peripheral" in the device-tree, it is
> wrong when the mode is "otg" in the device-tree. It is wrong because of two
> reasons:
> 1. There is no OTG state machine in U-Boot. Hence the role will never
> switch to the correct one eventually among host or peripheral.
> 2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
> register of the USB Controller being set to 11b. According to the
> datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
> should never be set to any value other than 01b (Host) and 10b (Device).
> Quoting the datasheet:
> "Programming this field with random data causes the controller
> to keep toggling between the host mode and the device mode."
> 
> Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
> that the caller specifies the intended role, rather than simply using
> the "dr_mode" property to set the role, set the role on the basis of the
> caller (intended role) and the device-tree (platform support).
> ----------------------------------------------------------------------------
> 
> Point #2 above is the main reason why dr_mode cannot be set to "otg" in
> dwc3_generic_probe(). The only difference now is that the check can be
> dropped as you have indicated, so the last paragraph above will change

Finally we are at the root of the problem. But the solution is not to
change the dr_mode but instead handle it correctly in dwc3_core_init_mode()

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a35b8c2f646..7f42b6e62c6 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -752,13 +752,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
-		ret = dwc3_host_init(dwc);
-		if (ret) {
-			dev_err(dwc->dev, "failed to initialize host\n");
-			return ret;
-		}
-
+		/* We don't support dual-role so restrict to Device mode */
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
 			dev_err(dwc->dev, "failed to initialize gadget\n");



> to:
> ...set the role on the basis of the caller (intended role).
> Device-tree will no longer be used i.e. plat->dr_mode shouldn't be used.
> 
> Regards,
> Siddharth.

-- 
cheers,
-roger

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Siddharth Vadapalli]

On Wed, Dec 04, 2024 at 09:32:23AM +0200, Roger Quadros wrote:
> 
> 
> On 04/12/2024 07:16, Siddharth Vadapalli wrote:
> > On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:

[...]

> >>> +++ b/drivers/usb/dwc3/dwc3-generic.c
> >>> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
> >>>  };
> >>>  
> >>>  static int dwc3_generic_probe(struct udevice *dev,
> >>> -			      struct dwc3_generic_priv *priv)
> >>> +			      struct dwc3_generic_priv *priv,
> >>> +			      enum usb_dr_mode mode)
> >>
> >> This is not necessary as dwc3_generic_host_probe will only be
> >> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
> >> will only be registered if dr_mode == "host" or "peripheral"
> > 
> > But I don't see it happening that way. dwc3_generic_host_probe() or
> > dwc3_generic_peripheral_probe() seem to be invoked based on the command
> > executed - USB Host command will trigger generic_host_probe() while USB
> > Device/Gadget command will trigger generic_peripheral_probe().
> > 
> 
> But, when dr_mode is "otg" generic host driver is *not* bound to the
> DWC3 device. So dwc3_generic_host_probe() will never be called for it.

In USB DFU boot, "dwc3_generic_peripheral_boot()" is invoked. Please
test it out on AM625-SK. I was able to observe this on AM62A7-SK.

> 
> > 
> >>
> >>>  {
> >>>  	int rc;
> >>>  	struct dwc3_generic_plat *plat = dev_get_plat(dev);
> >>> @@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
> >>>  
> >>>  	dwc3->dev = dev;
> >>>  	dwc3->maximum_speed = plat->maximum_speed;
> >>> -	dwc3->dr_mode = plat->dr_mode;
> >>> +
> >>> +	/*
> >>> +	 * If the controller supports OTG as indicated by plat->dr_mode,
> >>> +	 * then either Host or Peripheral mode is acceptable.
> >>> +	 * Otherwise, error out since the platform cannot support the mode
> >>> +	 * being requested by the caller of dwc3_generic_probe().
> >>> +	 */
> >>> +	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
> >>> +		pr_err("Requested usb mode is not supported by platform\n");
> >>
> >> This is actually not necessary. Sorry for suggesting this approach before.
> > 
> > While the check may not be necessary, dr_mode *should* be set based on
> > the caller and not based on plat->dr_mode. The reason I say so is that
> > plat->dr_mode could be "otg" in the device-tree. But "otg" shouldn't be
> > used for further configuration. I had indicated the reason for this in
> > the cover-letter, which I am pasting below for your reference:
> > 
> > ---------------------------------------------------------------------------
> > Currently, dwc3_generic_probe() sets the mode based on the device-tree
> > rather than setting it on the basis of the caller. While this might be
> > correct when the mode is "host" or "peripheral" in the device-tree, it is
> > wrong when the mode is "otg" in the device-tree. It is wrong because of two
> > reasons:
> > 1. There is no OTG state machine in U-Boot. Hence the role will never
> > switch to the correct one eventually among host or peripheral.
> > 2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
> > register of the USB Controller being set to 11b. According to the
> > datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
> > should never be set to any value other than 01b (Host) and 10b (Device).
> > Quoting the datasheet:
> > "Programming this field with random data causes the controller
> > to keep toggling between the host mode and the device mode."
> > 
> > Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
> > that the caller specifies the intended role, rather than simply using
> > the "dr_mode" property to set the role, set the role on the basis of the
> > caller (intended role) and the device-tree (platform support).
> > ----------------------------------------------------------------------------
> > 
> > Point #2 above is the main reason why dr_mode cannot be set to "otg" in
> > dwc3_generic_probe(). The only difference now is that the check can be
> > dropped as you have indicated, so the last paragraph above will change
> 
> Finally we are at the root of the problem. But the solution is not to
> change the dr_mode but instead handle it correctly in dwc3_core_init_mode()
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a35b8c2f646..7f42b6e62c6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -752,13 +752,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> -		ret = dwc3_host_init(dwc);
> -		if (ret) {
> -			dev_err(dwc->dev, "failed to initialize host\n");
> -			return ret;
> -		}
> -
> +		/* We don't support dual-role so restrict to Device mode */
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);

But is this acceptable? Why not default to Host? I went through a series
for defaulting OTG to Device for the Cadence Controller which was
objected at:
https://lore.kernel.org/r/62c5fe13-7f0e-e736-273e-31a8bbddbf13@kernel.org/
with the suggestion to use the cable to determine the role.

Regards,
Siddharth.

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros]

On 04/12/2024 09:47, Siddharth Vadapalli wrote:
> On Wed, Dec 04, 2024 at 09:32:23AM +0200, Roger Quadros wrote:
>>
>>
>> On 04/12/2024 07:16, Siddharth Vadapalli wrote:
>>> On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:
> 
> [...]
> 
>>>>> +++ b/drivers/usb/dwc3/dwc3-generic.c
>>>>> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
>>>>>  };
>>>>>  
>>>>>  static int dwc3_generic_probe(struct udevice *dev,
>>>>> -			      struct dwc3_generic_priv *priv)
>>>>> +			      struct dwc3_generic_priv *priv,
>>>>> +			      enum usb_dr_mode mode)
>>>>
>>>> This is not necessary as dwc3_generic_host_probe will only be
>>>> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
>>>> will only be registered if dr_mode == "host" or "peripheral"
>>>
>>> But I don't see it happening that way. dwc3_generic_host_probe() or
>>> dwc3_generic_peripheral_probe() seem to be invoked based on the command
>>> executed - USB Host command will trigger generic_host_probe() while USB
>>> Device/Gadget command will trigger generic_peripheral_probe().
>>>
>>
>> But, when dr_mode is "otg" generic host driver is *not* bound to the
>> DWC3 device. So dwc3_generic_host_probe() will never be called for it.
> 
> In USB DFU boot, "dwc3_generic_peripheral_boot()" is invoked. Please
> test it out on AM625-SK. I was able to observe this on AM62A7-SK.

We do want dwc3_generic_peripheral_boot() to be invoked with dr_mode is "otg".
As we still do not support dual-role switching in u-boot.

And you need that for DFU as well. Did I miss something?

> 
>>
>>>
>>>>
>>>>>  {
>>>>>  	int rc;
>>>>>  	struct dwc3_generic_plat *plat = dev_get_plat(dev);
>>>>> @@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
>>>>>  
>>>>>  	dwc3->dev = dev;
>>>>>  	dwc3->maximum_speed = plat->maximum_speed;
>>>>> -	dwc3->dr_mode = plat->dr_mode;
>>>>> +
>>>>> +	/*
>>>>> +	 * If the controller supports OTG as indicated by plat->dr_mode,
>>>>> +	 * then either Host or Peripheral mode is acceptable.
>>>>> +	 * Otherwise, error out since the platform cannot support the mode
>>>>> +	 * being requested by the caller of dwc3_generic_probe().
>>>>> +	 */
>>>>> +	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
>>>>> +		pr_err("Requested usb mode is not supported by platform\n");
>>>>
>>>> This is actually not necessary. Sorry for suggesting this approach before.
>>>
>>> While the check may not be necessary, dr_mode *should* be set based on
>>> the caller and not based on plat->dr_mode. The reason I say so is that
>>> plat->dr_mode could be "otg" in the device-tree. But "otg" shouldn't be
>>> used for further configuration. I had indicated the reason for this in
>>> the cover-letter, which I am pasting below for your reference:
>>>
>>> ---------------------------------------------------------------------------
>>> Currently, dwc3_generic_probe() sets the mode based on the device-tree
>>> rather than setting it on the basis of the caller. While this might be
>>> correct when the mode is "host" or "peripheral" in the device-tree, it is
>>> wrong when the mode is "otg" in the device-tree. It is wrong because of two
>>> reasons:
>>> 1. There is no OTG state machine in U-Boot. Hence the role will never
>>> switch to the correct one eventually among host or peripheral.
>>> 2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
>>> register of the USB Controller being set to 11b. According to the
>>> datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
>>> should never be set to any value other than 01b (Host) and 10b (Device).
>>> Quoting the datasheet:
>>> "Programming this field with random data causes the controller
>>> to keep toggling between the host mode and the device mode."
>>>
>>> Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
>>> that the caller specifies the intended role, rather than simply using
>>> the "dr_mode" property to set the role, set the role on the basis of the
>>> caller (intended role) and the device-tree (platform support).
>>> ----------------------------------------------------------------------------
>>>
>>> Point #2 above is the main reason why dr_mode cannot be set to "otg" in
>>> dwc3_generic_probe(). The only difference now is that the check can be
>>> dropped as you have indicated, so the last paragraph above will change
>>
>> Finally we are at the root of the problem. But the solution is not to
>> change the dr_mode but instead handle it correctly in dwc3_core_init_mode()
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index a35b8c2f646..7f42b6e62c6 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -752,13 +752,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>  		}
>>  		break;
>>  	case USB_DR_MODE_OTG:
>> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>> -		ret = dwc3_host_init(dwc);
>> -		if (ret) {
>> -			dev_err(dwc->dev, "failed to initialize host\n");
>> -			return ret;
>> -		}
>> -
>> +		/* We don't support dual-role so restrict to Device mode */
>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> 
> But is this acceptable? Why not default to Host? I went through a series

I will let others chime in if it is acceptable or not.

> Why not default to host? 

Because we don't bind the generic_host driver at all when dr_mode = "otg".
So XHCI driver will not be probed and host mode will not function.

> I went through a series
> for defaulting OTG to Device for the Cadence Controller which was
> objected at:
> https://lore.kernel.org/r/62c5fe13-7f0e-e736-273e-31a8bbddbf13@kernel.org/
> with the suggestion to use the cable to determine the role.
> 

dual-role is a new feature for this driver in u-boot which can be added
in the future if needed.
The current problem is to get DFU/device mode to work when dr_mode is "otg".

As the dwc3-generic driver only registers the device controller when
dr_mode is "otg" I don't see any other neater way to fix it than to
limit the controller to device mode if dr_mode is "otg".

The only only other driver calling dwc3_init() is dwc3-layerscape.c. whose
commit log states
"OTG mode is not supported yet. The dr_mode in the devicetree will either
 have to be set to peripheral or host."

Or you can go all the way and get the dual-role mode working the right way
where you check user request along with cable status and allow/prevent a
certain role.

-- 
cheers,
-roger

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Siddharth Vadapalli]

On Wed, Dec 04, 2024 at 11:57:14AM +0200, Roger Quadros wrote:
> 
> 
> On 04/12/2024 09:47, Siddharth Vadapalli wrote:
> > On Wed, Dec 04, 2024 at 09:32:23AM +0200, Roger Quadros wrote:
> >>
> >>
> >> On 04/12/2024 07:16, Siddharth Vadapalli wrote:
> >>> On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:
> > 
> > [...]
> > 
> >>>>> +++ b/drivers/usb/dwc3/dwc3-generic.c
> >>>>> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
> >>>>>  };
> >>>>>  
> >>>>>  static int dwc3_generic_probe(struct udevice *dev,
> >>>>> -			      struct dwc3_generic_priv *priv)
> >>>>> +			      struct dwc3_generic_priv *priv,
> >>>>> +			      enum usb_dr_mode mode)
> >>>>
> >>>> This is not necessary as dwc3_generic_host_probe will only be
> >>>> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
> >>>> will only be registered if dr_mode == "host" or "peripheral"
> >>>
> >>> But I don't see it happening that way. dwc3_generic_host_probe() or
> >>> dwc3_generic_peripheral_probe() seem to be invoked based on the command
> >>> executed - USB Host command will trigger generic_host_probe() while USB
> >>> Device/Gadget command will trigger generic_peripheral_probe().
> >>>
> >>
> >> But, when dr_mode is "otg" generic host driver is *not* bound to the
> >> DWC3 device. So dwc3_generic_host_probe() will never be called for it.
> > 
> > In USB DFU boot, "dwc3_generic_peripheral_boot()" is invoked. Please
> > test it out on AM625-SK. I was able to observe this on AM62A7-SK.
> 
> We do want dwc3_generic_peripheral_boot() to be invoked with dr_mode is "otg".
> As we still do not support dual-role switching in u-boot.
> 
> And you need that for DFU as well. Did I miss something?

You were mentioning that "dwc3_generic_host_probe()" will not be called,
but that is unrelated to the issue. I was pointing out that
"dwc3_generic_peripheral_probe()" is invoked, with plat->dr_mode set to
"otg" which currently results in PRTCAPDIR set to 11b which results in
the issue. So the patch aims to address this by setting mode based on
caller rather than using what is mentioned in the device-tree. This
should be in sync when "dr_mode" is either "host"/"peripheral", but in
the case of "otg", we run into issues which is fixed by this patch.

[...]

> >>> 2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
> >>> register of the USB Controller being set to 11b. According to the
> >>> datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
> >>> should never be set to any value other than 01b (Host) and 10b (Device).
> >>> Quoting the datasheet:
> >>> "Programming this field with random data causes the controller
> >>> to keep toggling between the host mode and the device mode."
> >>>
> >>> Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
> >>> that the caller specifies the intended role, rather than simply using
> >>> the "dr_mode" property to set the role, set the role on the basis of the
> >>> caller (intended role) and the device-tree (platform support).
> >>> ----------------------------------------------------------------------------
> >>>
> >>> Point #2 above is the main reason why dr_mode cannot be set to "otg" in
> >>> dwc3_generic_probe(). The only difference now is that the check can be
> >>> dropped as you have indicated, so the last paragraph above will change
> >>
> >> Finally we are at the root of the problem. But the solution is not to
> >> change the dr_mode but instead handle it correctly in dwc3_core_init_mode()
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >> index a35b8c2f646..7f42b6e62c6 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >> @@ -752,13 +752,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
> >>  		}
> >>  		break;
> >>  	case USB_DR_MODE_OTG:
> >> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> >> -		ret = dwc3_host_init(dwc);
> >> -		if (ret) {
> >> -			dev_err(dwc->dev, "failed to initialize host\n");
> >> -			return ret;
> >> -		}
> >> -
> >> +		/* We don't support dual-role so restrict to Device mode */
> >> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> > 
> > But is this acceptable? Why not default to Host? I went through a series
> 
> I will let others chime in if it is acceptable or not.
> 
> > Why not default to host? 
> 
> Because we don't bind the generic_host driver at all when dr_mode = "otg".
> So XHCI driver will not be probed and host mode will not function.
> 
> > I went through a series
> > for defaulting OTG to Device for the Cadence Controller which was
> > objected at:
> > https://lore.kernel.org/r/62c5fe13-7f0e-e736-273e-31a8bbddbf13@kernel.org/
> > with the suggestion to use the cable to determine the role.
> > 
> 
> dual-role is a new feature for this driver in u-boot which can be added
> in the future if needed.
> The current problem is to get DFU/device mode to work when dr_mode is "otg".
> 
> As the dwc3-generic driver only registers the device controller when
> dr_mode is "otg" I don't see any other neater way to fix it than to
> limit the controller to device mode if dr_mode is "otg".

Why not do what this patch does? What is the issue with the current
patch (apart from the checks, which I could retain as well if others
wish so)? The caller clearly mentions the expected role, so why not use
that?

> 
> The only only other driver calling dwc3_init() is dwc3-layerscape.c. whose
> commit log states
> "OTG mode is not supported yet. The dr_mode in the devicetree will either
>  have to be set to peripheral or host."
> 
> Or you can go all the way and get the dual-role mode working the right way
> where you check user request along with cable status and allow/prevent a
> certain role.

The current patch seems to be an intermediate solution (actually, it
might be what should have been done when the code was first introduced
itself. Why use plat->dr_mode at all, except maybe to check if the
platform supports what is requested by the caller?)

Regards,
Siddharth.

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros]

On 04/12/2024 12:05, Siddharth Vadapalli wrote:
> On Wed, Dec 04, 2024 at 11:57:14AM +0200, Roger Quadros wrote:
>>
>>
>> On 04/12/2024 09:47, Siddharth Vadapalli wrote:
>>> On Wed, Dec 04, 2024 at 09:32:23AM +0200, Roger Quadros wrote:
>>>>
>>>>
>>>> On 04/12/2024 07:16, Siddharth Vadapalli wrote:
>>>>> On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:
>>>
>>> [...]
>>>
>>>>>>> +++ b/drivers/usb/dwc3/dwc3-generic.c
>>>>>>> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
>>>>>>>  };
>>>>>>>  
>>>>>>>  static int dwc3_generic_probe(struct udevice *dev,
>>>>>>> -			      struct dwc3_generic_priv *priv)
>>>>>>> +			      struct dwc3_generic_priv *priv,
>>>>>>> +			      enum usb_dr_mode mode)
>>>>>>
>>>>>> This is not necessary as dwc3_generic_host_probe will only be
>>>>>> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
>>>>>> will only be registered if dr_mode == "host" or "peripheral"
>>>>>
>>>>> But I don't see it happening that way. dwc3_generic_host_probe() or
>>>>> dwc3_generic_peripheral_probe() seem to be invoked based on the command
>>>>> executed - USB Host command will trigger generic_host_probe() while USB
>>>>> Device/Gadget command will trigger generic_peripheral_probe().
>>>>>
>>>>
>>>> But, when dr_mode is "otg" generic host driver is *not* bound to the
>>>> DWC3 device. So dwc3_generic_host_probe() will never be called for it.
>>>
>>> In USB DFU boot, "dwc3_generic_peripheral_boot()" is invoked. Please
>>> test it out on AM625-SK. I was able to observe this on AM62A7-SK.
>>
>> We do want dwc3_generic_peripheral_boot() to be invoked with dr_mode is "otg".
>> As we still do not support dual-role switching in u-boot.
>>
>> And you need that for DFU as well. Did I miss something?
> 
> You were mentioning that "dwc3_generic_host_probe()" will not be called,
> but that is unrelated to the issue. I was pointing out that
> "dwc3_generic_peripheral_probe()" is invoked, with plat->dr_mode set to
> "otg" which currently results in PRTCAPDIR set to 11b which results in
> the issue. So the patch aims to address this by setting mode based on
> caller rather than using what is mentioned in the device-tree. This
> should be in sync when "dr_mode" is either "host"/"peripheral", but in
> the case of "otg", we run into issues which is fixed by this patch.
> 
> [...]
> 
>>>>> 2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
>>>>> register of the USB Controller being set to 11b. According to the
>>>>> datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
>>>>> should never be set to any value other than 01b (Host) and 10b (Device).
>>>>> Quoting the datasheet:
>>>>> "Programming this field with random data causes the controller
>>>>> to keep toggling between the host mode and the device mode."
>>>>>
>>>>> Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
>>>>> that the caller specifies the intended role, rather than simply using
>>>>> the "dr_mode" property to set the role, set the role on the basis of the
>>>>> caller (intended role) and the device-tree (platform support).
>>>>> ----------------------------------------------------------------------------
>>>>>
>>>>> Point #2 above is the main reason why dr_mode cannot be set to "otg" in
>>>>> dwc3_generic_probe(). The only difference now is that the check can be
>>>>> dropped as you have indicated, so the last paragraph above will change
>>>>
>>>> Finally we are at the root of the problem. But the solution is not to
>>>> change the dr_mode but instead handle it correctly in dwc3_core_init_mode()
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index a35b8c2f646..7f42b6e62c6 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -752,13 +752,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>>>  		}
>>>>  		break;
>>>>  	case USB_DR_MODE_OTG:
>>>> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>>>> -		ret = dwc3_host_init(dwc);
>>>> -		if (ret) {
>>>> -			dev_err(dwc->dev, "failed to initialize host\n");
>>>> -			return ret;
>>>> -		}
>>>> -
>>>> +		/* We don't support dual-role so restrict to Device mode */
>>>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>>>
>>> But is this acceptable? Why not default to Host? I went through a series
>>
>> I will let others chime in if it is acceptable or not.
>>
>>> Why not default to host? 
>>
>> Because we don't bind the generic_host driver at all when dr_mode = "otg".
>> So XHCI driver will not be probed and host mode will not function.
>>
>>> I went through a series
>>> for defaulting OTG to Device for the Cadence Controller which was
>>> objected at:
>>> https://lore.kernel.org/r/62c5fe13-7f0e-e736-273e-31a8bbddbf13@kernel.org/
>>> with the suggestion to use the cable to determine the role.
>>>
>>
>> dual-role is a new feature for this driver in u-boot which can be added
>> in the future if needed.
>> The current problem is to get DFU/device mode to work when dr_mode is "otg".
>>
>> As the dwc3-generic driver only registers the device controller when
>> dr_mode is "otg" I don't see any other neater way to fix it than to
>> limit the controller to device mode if dr_mode is "otg".
> 
> Why not do what this patch does? What is the issue with the current
> patch (apart from the checks, which I could retain as well if others
> wish so)? The caller clearly mentions the expected role, so why not use
> that?

Because the patch is more complicated than it needs to be, now that we know:
1) generic_host_probe() will only be called when dr_mode is "host",
2) generic_peripheral_probe() will only be called when dr_mode is "peripheral"
or "otg".

The problem case is generic_peripheral_probe() being called with when
dr_mode is "otg".

Another alternative could be to override dr_mode to "peripheral" if
it was "otg" in dwc3_generic_probe before calling dwc3_init() but
I don't prefer that as we are loosing platform information here
and it will have to be reverted when dual-role support is added in core.c.

So better to deal with this in core.c and add a note why we are doing it.

> 
>>
>> The only only other driver calling dwc3_init() is dwc3-layerscape.c. whose
>> commit log states
>> "OTG mode is not supported yet. The dr_mode in the devicetree will either
>>  have to be set to peripheral or host."
>>
>> Or you can go all the way and get the dual-role mode working the right way
>> where you check user request along with cable status and allow/prevent a
>> certain role.
> 
> The current patch seems to be an intermediate solution (actually, it
> might be what should have been done when the code was first introduced
> itself. Why use plat->dr_mode at all, except maybe to check if the
> platform supports what is requested by the caller?)
> 
> Regards,
> Siddharth.

-- 
cheers,
-roger

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Siddharth Vadapalli]

On Wed, Dec 04, 2024 at 02:34:56PM +0200, Roger Quadros wrote:
> 
> 
> On 04/12/2024 12:05, Siddharth Vadapalli wrote:

[...]

> > Why not do what this patch does? What is the issue with the current
> > patch (apart from the checks, which I could retain as well if others
> > wish so)? The caller clearly mentions the expected role, so why not use
> > that?
> 
> Because the patch is more complicated than it needs to be, now that we know:
> 1) generic_host_probe() will only be called when dr_mode is "host",
> 2) generic_peripheral_probe() will only be called when dr_mode is "peripheral"
> or "otg".
> 
> The problem case is generic_peripheral_probe() being called with when
> dr_mode is "otg".

We can also be sure that generic_periperhal_probe() expects dr_mode to
be "peripheral" only and not "otg". So why not fix it at the root, which
is what this patch does? Independent of the issue, is the following a
valid use-case?

generic_peripheral_probe() with plat->dr_mode = "otg" and hence
generic_probe() using "otg" rather than "peripheral"
as is the case now. Is this resulting in something functional for other
users of the USB Designware Controller? If the user requests "peripheral"
mode of operation, will "otg" meet the user's requirement? If yes, I have
no further objections. Irrespective of this, if you believe that the fix
should be in the core as you indicated below, please feel free to post
the patch.

> 
> Another alternative could be to override dr_mode to "peripheral" if
> it was "otg" in dwc3_generic_probe before calling dwc3_init() but
> I don't prefer that as we are loosing platform information here
> and it will have to be reverted when dual-role support is added in core.c.
> 
> So better to deal with this in core.c and add a note why we are doing it.
> 

[...]

Regards,
Siddharth.

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros]

On 04/12/2024 15:37, Siddharth Vadapalli wrote:
> On Wed, Dec 04, 2024 at 02:34:56PM +0200, Roger Quadros wrote:
>>
>>
>> On 04/12/2024 12:05, Siddharth Vadapalli wrote:
> 
> [...]
> 
>>> Why not do what this patch does? What is the issue with the current
>>> patch (apart from the checks, which I could retain as well if others
>>> wish so)? The caller clearly mentions the expected role, so why not use
>>> that?
>>
>> Because the patch is more complicated than it needs to be, now that we know:
>> 1) generic_host_probe() will only be called when dr_mode is "host",
>> 2) generic_peripheral_probe() will only be called when dr_mode is "peripheral"
>> or "otg".
>>
>> The problem case is generic_peripheral_probe() being called with when
>> dr_mode is "otg".
> 
> We can also be sure that generic_periperhal_probe() expects dr_mode to
> be "peripheral" only and not "otg". So why not fix it at the root, which

generic_peripheral_probe() expects nothing from dr_mode.
It just expects the USB controller to work in peripheral mode.

dr_mode is just a reflection of platform/device-tree dr_mode. Let's keep it
that way and not abuse dr_mode to "peripheral" when it is actually "otg".

> is what this patch does? Independent of the issue, is the following a
> valid use-case?
> 
> generic_peripheral_probe() with plat->dr_mode = "otg" and hence
> generic_probe() using "otg" rather than "peripheral"

It is a valid use case. The Platform says that the port is "otg"
i.e. (plat->dr_mode ="otg")
and the user says he wants to use it as peripheral
i.e. (by invoking generic_peripheral_probe())
Don't you see this as valid.

> as is the case now. Is this resulting in something functional for other
> users of the USB Designware Controller? If the user requests "peripheral"
> mode of operation, will "otg" meet the user's requirement? If yes, I have
> no further objections. Irrespective of this, if you believe that the fix
> should be in the core as you indicated below, please feel free to post
> the patch.

Let's look at this from the other way around.
Why do you not agree that the fix should be in core.c and abusing dr_mode
is the correct way to go?

> 
>>
>> Another alternative could be to override dr_mode to "peripheral" if
>> it was "otg" in dwc3_generic_probe before calling dwc3_init() but
>> I don't prefer that as we are loosing platform information here
>> and it will have to be reverted when dual-role support is added in core.c.
>>
>> So better to deal with this in core.c and add a note why we are doing it.
>>
> 
> [...]
> 
> Regards,
> Siddharth.

-- 
cheers,
-roger

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Siddharth Vadapalli]

On Wed, Dec 04, 2024 at 04:36:53PM +0200, Roger Quadros wrote:
> 
> 
> On 04/12/2024 15:37, Siddharth Vadapalli wrote:
> > On Wed, Dec 04, 2024 at 02:34:56PM +0200, Roger Quadros wrote:
> >>
> >>
> >> On 04/12/2024 12:05, Siddharth Vadapalli wrote:
> > 
> > [...]
> > 
> >>> Why not do what this patch does? What is the issue with the current
> >>> patch (apart from the checks, which I could retain as well if others
> >>> wish so)? The caller clearly mentions the expected role, so why not use
> >>> that?
> >>
> >> Because the patch is more complicated than it needs to be, now that we know:
> >> 1) generic_host_probe() will only be called when dr_mode is "host",
> >> 2) generic_peripheral_probe() will only be called when dr_mode is "peripheral"
> >> or "otg".
> >>
> >> The problem case is generic_peripheral_probe() being called with when
> >> dr_mode is "otg".
> > 
> > We can also be sure that generic_periperhal_probe() expects dr_mode to
> > be "peripheral" only and not "otg". So why not fix it at the root, which
> 
> generic_peripheral_probe() expects nothing from dr_mode.
> It just expects the USB controller to work in peripheral mode.

Yes. So configuring the USB controller for peripheral mode of operation
should be fine, in which case, irrespective of what "dr_mode" says,
using "peripheral" should enable the desired functionality.

> 
> dr_mode is just a reflection of platform/device-tree dr_mode. Let's keep it
> that way and not abuse dr_mode to "peripheral" when it is actually "otg".

I am not asking to change what the device-tree has. I am asking the need
to use "otg" as specified in the device-tree when in U-Boot, we cannot
switch between Host and Peripheral dynamically using "otg". At the end
of the day, there is no "generic_otg_probe()" which could enable such
functionality. So I fail to understand why configuring the controller
for "peripheral" mode as requested by "generic_peripheral_probe()" is
equivalent to abusing the device-tree.

> 
> > is what this patch does? Independent of the issue, is the following a
> > valid use-case?
> > 
> > generic_peripheral_probe() with plat->dr_mode = "otg" and hence
> > generic_probe() using "otg" rather than "peripheral"
> 
> It is a valid use case. The Platform says that the port is "otg"
> i.e. (plat->dr_mode ="otg")
> and the user says he wants to use it as peripheral
> i.e. (by invoking generic_peripheral_probe())
> Don't you see this as valid.

Does peripheral mode work when user sets "otg" in the device-tree?
Clearly it doesn't and hence this patch. Linux supports "otg" mode in
the sense that we can switch between "host" and "peripheral". U-Boot on
the other hand cannot even enable "peripheral" functionality when
dr_mode is "otg" in the device-tree, let alone switching roles.

> 
> > as is the case now. Is this resulting in something functional for other
> > users of the USB Designware Controller? If the user requests "peripheral"
> > mode of operation, will "otg" meet the user's requirement? If yes, I have
> > no further objections. Irrespective of this, if you believe that the fix
> > should be in the core as you indicated below, please feel free to post
> > the patch.
> 
> Let's look at this from the other way around.
> Why do you not agree that the fix should be in core.c and abusing dr_mode
> is the correct way to go?

Isn't "otg" meant to convey that the role isn't fixed and can be
switched? So how is hard-coding it to "peripheral", not considered as
abusing the core? I don't intend to argue any further. I probably
shouldn't have looked at other discussions on the mailing list regarding
hard-coding "otg" as "peripheral" being incorrect. I should have simply
posted the PRTCAPDIR fix in the core, instead of trying to come up with
an acceptable fix. It is only because I felt that it didn't seem acceptable
that I spent time finding out this inconsistency in "dwc3_generic_probe"
where configuring the controller for "otg" mode as indicated by
plat->dr_mode when the caller is "generic_peripheral_probe()" is somehow
supposed to work in U-Boot where there is no OTG state machine.

If the fix in the core is the right way to go, please post the patch. I
only wanted to make USB DFU boot functional on AM62A and didn't want to
end up in an argument while doing so.

Regards,
Siddharth.

Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros]

On 04/12/2024 17:00, Siddharth Vadapalli wrote:
> On Wed, Dec 04, 2024 at 04:36:53PM +0200, Roger Quadros wrote:
>>
>>
>> On 04/12/2024 15:37, Siddharth Vadapalli wrote:
>>> On Wed, Dec 04, 2024 at 02:34:56PM +0200, Roger Quadros wrote:
>>>>
>>>>
>>>> On 04/12/2024 12:05, Siddharth Vadapalli wrote:
>>>
>>> [...]
>>>
>>>>> Why not do what this patch does? What is the issue with the current
>>>>> patch (apart from the checks, which I could retain as well if others
>>>>> wish so)? The caller clearly mentions the expected role, so why not use
>>>>> that?
>>>>
>>>> Because the patch is more complicated than it needs to be, now that we know:
>>>> 1) generic_host_probe() will only be called when dr_mode is "host",
>>>> 2) generic_peripheral_probe() will only be called when dr_mode is "peripheral"
>>>> or "otg".
>>>>
>>>> The problem case is generic_peripheral_probe() being called with when
>>>> dr_mode is "otg".
>>>
>>> We can also be sure that generic_periperhal_probe() expects dr_mode to
>>> be "peripheral" only and not "otg". So why not fix it at the root, which
>>
>> generic_peripheral_probe() expects nothing from dr_mode.
>> It just expects the USB controller to work in peripheral mode.
> 
> Yes. So configuring the USB controller for peripheral mode of operation
> should be fine, in which case, irrespective of what "dr_mode" says,
> using "peripheral" should enable the desired functionality.
> 
>>
>> dr_mode is just a reflection of platform/device-tree dr_mode. Let's keep it
>> that way and not abuse dr_mode to "peripheral" when it is actually "otg".
> 
> I am not asking to change what the device-tree has. I am asking the need
> to use "otg" as specified in the device-tree when in U-Boot, we cannot
> switch between Host and Peripheral dynamically using "otg". At the end
> of the day, there is no "generic_otg_probe()" which could enable such
> functionality. So I fail to understand why configuring the controller
> for "peripheral" mode as requested by "generic_peripheral_probe()" is
> equivalent to abusing the device-tree.
> 
>>
>>> is what this patch does? Independent of the issue, is the following a
>>> valid use-case?
>>>
>>> generic_peripheral_probe() with plat->dr_mode = "otg" and hence
>>> generic_probe() using "otg" rather than "peripheral"
>>
>> It is a valid use case. The Platform says that the port is "otg"
>> i.e. (plat->dr_mode ="otg")
>> and the user says he wants to use it as peripheral
>> i.e. (by invoking generic_peripheral_probe())
>> Don't you see this as valid.
> 
> Does peripheral mode work when user sets "otg" in the device-tree?
> Clearly it doesn't and hence this patch. Linux supports "otg" mode in
> the sense that we can switch between "host" and "peripheral". U-Boot on
> the other hand cannot even enable "peripheral" functionality when
> dr_mode is "otg" in the device-tree, let alone switching roles.
> 
>>
>>> as is the case now. Is this resulting in something functional for other
>>> users of the USB Designware Controller? If the user requests "peripheral"
>>> mode of operation, will "otg" meet the user's requirement? If yes, I have
>>> no further objections. Irrespective of this, if you believe that the fix
>>> should be in the core as you indicated below, please feel free to post
>>> the patch.
>>
>> Let's look at this from the other way around.
>> Why do you not agree that the fix should be in core.c and abusing dr_mode
>> is the correct way to go?
> 
> Isn't "otg" meant to convey that the role isn't fixed and can be
> switched? So how is hard-coding it to "peripheral", not considered as
> abusing the core? I don't intend to argue any further. I probably
> shouldn't have looked at other discussions on the mailing list regarding
> hard-coding "otg" as "peripheral" being incorrect. I should have simply
> posted the PRTCAPDIR fix in the core, instead of trying to come up with
> an acceptable fix. It is only because I felt that it didn't seem acceptable
> that I spent time finding out this inconsistency in "dwc3_generic_probe"
> where configuring the controller for "otg" mode as indicated by
> plat->dr_mode when the caller is "generic_peripheral_probe()" is somehow
> supposed to work in U-Boot where there is no OTG state machine.
> 
> If the fix in the core is the right way to go, please post the patch. I
> only wanted to make USB DFU boot functional on AM62A and didn't want to
> end up in an argument while doing so.

Sorry if you took this as an argument. I was only weighing in if there were
better alternatives.
If we have to go with your solution then your v1 series is better
than v2. I will give my Reviewed-by: there.

-- 
cheers,
-roger