[PATCH v2 0/2] AM62A DWC3: Add support for USB DFU boot in OTG mode
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... 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/53ba02cb0ff4a09c47e920d082... 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/13f9a539ef3297ab2822bc1bc1...
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(-)
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... 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;
Hi Siddharth,
On 03/12/2024 11:37, Siddharth Vadapalli wrote:
There are only two callers of "dwc3_generic_probe()", namely:
- dwc3_generic_peripheral_probe()
- 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... 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;
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:
- dwc3_generic_peripheral_probe()
- 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... 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.
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:
- dwc3_generic_peripheral_probe()
- 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... 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:
- 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.
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:
- 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.
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:
- 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.
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.
[...]
- 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.
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.
[...]
- 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.
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:
- generic_host_probe() will only be called when dr_mode is "host",
- 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.
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:
- generic_host_probe() will only be called when dr_mode is "host",
- 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.
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:
- generic_host_probe() will only be called when dr_mode is "host",
- 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.
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:
- generic_host_probe() will only be called when dr_mode is "host",
- 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.
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... 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
participants (2)
-
Roger Quadros -
Siddharth Vadapalli