usb: chipidea: tegra: fix hardlock with invalid dr mode

PATCH In review Peter Geis 4 participants 11 messages Jan 27, 2020 – Feb 5, 2020 6 messages not in the archive

Subsystems (3)

USB Tegra architecture support Chipidea USB high speed dual role controller

lore

Share & tools

lore

Peter Geis

author

2020-01-27 02:35 UTC #0 lore raw

PATCH 1/1 usb: chipidea: tegra: fix hardlock with invalid dr mode

The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.

Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.

Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")

Signed-off-by Peter Geis <[email protected]>

---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/usb/chipidea/ci_hdrc_tegra.c +7 -0

index 7455df0ede49..2f6f6ce3e8f5 100644
@@ -89,6 +89,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
 	udc->data.usb_phy = udc->phy;
 	udc->data.capoffset = DEF_CAPOFFSET;
 
+	/* check the dual mode and warn about bad configurations */
+	if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_OTG &&
+	   !of_property_read_bool(pdev->dev.of_node, "extcon")) {
+		dev_warn(&pdev->dev, "no extcon registered, otg unavailable");
+		udc->data.flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
+	}
+
 	udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
 				      pdev->num_resources, &udc->data);
 	if (IS_ERR(udc->dev)) {

Signature · 1 line

2.17.1

A message that is not in the archive (sent off-list or not yet received): 20200127023548.27107-1-pgwipeout-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Greg KH

2020-01-27 07:17 UTC #2 lore raw

Quoted 6 lines · Peter Geis

The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.

Quoted 6 lines

Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")

Please use 12 digits for kernel sha1 values, this should be:

Fixes dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")

As proof of the need of this, just using the values you gave here causes git to complain:

$ git show dfebb5f
error: short SHA1 dfebb5f is ambiguous
hint: The candidates are:
hint:   dfebb5f43a78 commit 2017-08-16 - usb: chipidea: Add support for Tegra20/30/114/124
hint:   dfebb5fec744 tree
fatal: ambiguous argument 'dfebb5f': unknown revision or path not in the working tree.

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): [email protected]

Peter Geis

author

2020-01-27 15:25 UTC #4 lore raw

Quoted 13 lines · Greg KH

Quoted 11 lines · Peter Geis
The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.
Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.

Quoted 4 lines

Quoted 1 line
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Please use 12 digits for kernel sha1 values, this should be:
Fixes dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")

Understood, thank you.
I will ensure that is the case in the future.

Quoted 14 lines

As proof of the need of this, just using the values you gave here causes git to complain:

$ git show dfebb5f
error: short SHA1 dfebb5f is ambiguous
hint: The candidates are:
hint:   dfebb5f43a78 commit 2017-08-16 - usb: chipidea: Add support for Tegra20/30/114/124
hint:   dfebb5fec744 tree
fatal: ambiguous argument 'dfebb5f': unknown revision or path not in the working tree.

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): CAMdYzYqctkfqhHN0-WDvX9kKdXH-AVir193SpcrR_t3K=VN=0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Dmitry Osipenko

2020-01-27 15:38 UTC #6 lore raw

27.01.2020 18:25, Peter Geis пишет:

Quoted 14 lines · Greg KH

Quoted 13 lines · Peter Geis
Quoted 11 lines
The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.
Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Please use 12 digits for kernel sha1 values, this should be:
Fixes dfebb5f43a78 ("usb: chipidea: Add support for Tegra20/30/114/124")
Understood, thank you.
I will ensure that is the case in the future.

Please also take a look at the kernel's scripts/checkpatch.pl, running 'scripts/checkpatch.pl --strict path_to.patch' will help to catch such things.

Peter Chen

2020-01-31 05:27 UTC #7 lore raw

In reply to a message not in the archive

Quoted 5 lines · Peter Geis

The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.

If the driver only supports peripheral mode, you could set dr_mode as peripheral-only at glue layer, it would not be override by core.c. See ci_get_platdata.

But one thing I could not understand, if the driver doesn't support dual-role, how could you do manual role switch?

Quoted 17 lines

Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Signed-off-by Peter Geis <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 7455df0ede49..2f6f6ce3e8f5 100644

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 98

Quoted 10 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +7 -0

@@ -89,6 +89,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
 	udc->data.usb_phy = udc->phy;
 	udc->data.capoffset = DEF_CAPOFFSET;
 
+	/* check the dual mode and warn about bad configurations */
+	if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_OTG &&
+	   !of_property_read_bool(pdev->dev.of_node, "extcon")) {
+		dev_warn(&pdev->dev, "no extcon registered, otg unavailable");
+		udc->data.flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
+	}
+

The CI_HDRC_DUAL_ROLE_NOT_OTG flag may not be suitable here, please see comments for it.

Quoted 6 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0

 	udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
 				      pdev->num_resources, &udc->data);
 	if (IS_ERR(udc->dev)) {

Signature · 1 line

2.17.1

Signature · 2 lines

Thanks,
Peter Chen

Peter Geis

author

2020-01-31 20:43 UTC #8 lore raw

Quoted 11 lines · Peter Chen

Quoted 5 lines · Peter Geis
The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.
If the driver only supports peripheral mode, you could set dr_mode as peripheral-only at glue layer, it would not be override by core.c. See ci_get_platdata.

The mode is set via the device tree dr_mode property. Even though the current tegra_udc driver does not currently support mode switching, board device tree files such as the apalis-eval and colibri-eval-v3 have dr_mode set to otg.

They also seem to be missing the extcon phandle, which means automatic mode switching is not possible?

Quoted 3 lines

But one thing I could not understand, if the driver doesn't support dual-role, how could you do manual role switch?

Manual role switching is conducted via debugfs,
/sys/kernel/debug/usb/ci_hdrc.0/role

Quoted 21 lines

Quoted 21 lines
Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Signed-off-by Peter Geis <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 7455df0ede49..2f6f6ce3e8f5 100644
@@ -89,6 +89,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
      udc->data.usb_phy = udc->phy;
      udc->data.capoffset = DEF_CAPOFFSET;
 

Quoted 10 lines

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 98
Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +7 -0
+     /* check the dual mode and warn about bad configurations */
+     if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_OTG &&
+        !of_property_read_bool(pdev->dev.of_node, "extcon")) {
+             dev_warn(&pdev->dev, "no extcon registered, otg unavailable");
+             udc->data.flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
+     }
+
The CI_HDRC_DUAL_ROLE_NOT_OTG flag may not be suitable here, please see comments for it.

I've dug around the various mailing lists and I fail to find any reference to why this is not a valid use case.
The commit message specifically references dual role capable devices without proper otg implementations, such as missing the otgsc register.

My current use case is the Ouya, which deadlocks the kernel durning probe if the otg capable usb controller is set to dr_mode=otg. It works with this flag set.
Unfortunately there isn't a property for dr_mode set to non_otg_dr_mode.

I found a post stating that the driver blindly touches registers in otg mode, which leads to the deadlock if those registers are read only or non-existent,
Eventually someone should look into why this deadlock occurs and make a proper fix that applies to all users.
Unfortunately I do not have the knowledge yet to accomplish this task.

With some simple modifications to the tegra_udc driver it is possible to get host mode working, vice using the tegra-ehci driver. At this point role switch works

I also managed to move all of the functionality of the tegra-ehci driver into the tegra-udc driver.
Unfortunately it doesn't behave correctly under some cases, so I never submitted it.

Do you have a recommendation for handling this?

Quoted 12 lines

Quoted 7 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +0 -1

      udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
                                    pdev->num_resources, &udc->data);
      if (IS_ERR(udc->dev)) {
--

2.17.1

Thanks,
Peter Chen

A message that is not in the archive (sent off-list or not yet received): CAMdYzYqwz9HLsjvc1hDmovzWqiV_Vswe57d_gWhwBnvb2aNPyQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Peter Chen

2020-02-01 06:13 UTC #10 lore raw

Quoted 9 lines · Peter Geis

Quoted 8 lines · Peter Chen
Quoted 5 lines · Peter Geis
The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.

Quoted 9 lines

Quoted 3 lines
If the driver only supports peripheral mode, you could set dr_mode as peripheral-only at glue layer, it would not be override by core.c. See ci_get_platdata.
The mode is set via the device tree dr_mode property. Even though the current tegra_udc driver does not currently support mode switching, board device tree files such as the apalis-eval and colibri-eval-v3 have dr_mode set to otg.

If you would like fixing wrong dts setting issue, you could do below:

drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0

index 0c9911d44ee5..5d85363ad0f4 100644
@@ -95,6 +95,7 @@ static int tegra_udc_probe(struct platform_device *pdev)
 	udc->data.flags = soc->flags;
 	udc->data.usb_phy = udc->phy;
 	udc->data.capoffset = DEF_CAPOFFSET;
+	udc->data.dr_mode = USB_DR_MODE_PERIPHERAL;
 
 	udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
 				      pdev->num_resources, &udc->data);

Quoted 64 lines

They also seem to be missing the extcon phandle, which means automatic mode switching is not possible?
Quoted 3 lines
But one thing I could not understand, if the driver doesn't support dual-role, how could you do manual role switch?
Manual role switching is conducted via debugfs,
/sys/kernel/debug/usb/ci_hdrc.0/role
Quoted 21 lines
Quoted 21 lines
Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Signed-off-by Peter Geis <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 7455df0ede49..2f6f6ce3e8f5 100644
@@ -89,6 +89,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
      udc->data.usb_phy = udc->phy;
      udc->data.capoffset = DEF_CAPOFFSET;
 
Quoted 10 lines
drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 98
Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +7 -0
+     /* check the dual mode and warn about bad configurations */
+     if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_OTG &&
+        !of_property_read_bool(pdev->dev.of_node, "extcon")) {
+             dev_warn(&pdev->dev, "no extcon registered, otg unavailable");
+             udc->data.flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
+     }
+
The CI_HDRC_DUAL_ROLE_NOT_OTG flag may not be suitable here, please see comments for it.
I've dug around the various mailing lists and I fail to find any reference to why this is not a valid use case.
The commit message specifically references dual role capable devices without proper otg implementations, such as missing the otgsc register.
My current use case is the Ouya, which deadlocks the kernel durning probe if the otg capable usb controller is set to dr_mode=otg. It works with this flag set.
Unfortunately there isn't a property for dr_mode set to non_otg_dr_mode.
I found a post stating that the driver blindly touches registers in otg mode, which leads to the deadlock if those registers are read only or non-existent,
Eventually someone should look into why this deadlock occurs and make a proper fix that applies to all users.
Unfortunately I do not have the knowledge yet to accomplish this task.
With some simple modifications to the tegra_udc driver it is possible to get host mode working, vice using the tegra-ehci driver. At this point role switch works

Quoted 6 lines

I also managed to move all of the functionality of the tegra-ehci driver into the tegra-udc driver.
Unfortunately it doesn't behave correctly under some cases, so I never submitted it.
Do you have a recommendation for handling this?

If you would like adding host function in chipidea driver for tegra, and want dual-role switch to work, first, you may need to know which dual-role switch you need:
- Through controller's id and vbus pins
- Through extcon, eg,GPIO.
- Through usb-role-switch class, eg, Type-C controller
- Through sysfs, /sys/bus/platform/devices/ci_hdrc.0/role

The first three are automatically switch, the last one is manually. Current chipidea core should support all above switches, maybe there are some limitations for them, the fixes are welcome.

Second, you may check if touch otgsc will hang or deadlock the system. If you can't touch otgsc when portsc.phcd = 0, you may need the flag CI_HDRC_DUAL_ROLE_NOT_OTG, afaik, few SoCs can't touch otgsc if it supports dual-role.

When you add host function, you may set dr_mode as host-only first, it could make thing easier.

Signature · 2 lines

Thanks,
Peter Chen

Peter Geis

author

2020-02-03 17:22 UTC #11 lore raw

Resending due to html screwup, apologies.

Quoted 28 lines · Peter Chen

Quoted 9 lines · Peter Geis
Quoted 8 lines · Peter Chen
Quoted 5 lines · Peter Geis
The ci_hdrc_tegra driver does not currently support dual role mode, but it does not explicitly prevent its use.
Certain devices support dual role mode, but not automatic role switch. This can cause the device to lock up during initialization of the driver.
Quoted 9 lines
Quoted 3 lines
If the driver only supports peripheral mode, you could set dr_mode as peripheral-only at glue layer, it would not be override by core.c. See ci_get_platdata.
The mode is set via the device tree dr_mode property. Even though the current tegra_udc driver does not currently support mode switching, board device tree files such as the apalis-eval and colibri-eval-v3 have dr_mode set to otg.
If you would like fixing wrong dts setting issue, you could do below:
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 0c9911d44ee5..5d85363ad0f4 100644

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 101

Quoted 7 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0

@@ -95,6 +95,7 @@ static int tegra_udc_probe(struct platform_device *pdev)
        udc->data.flags = soc->flags;
        udc->data.usb_phy = udc->phy;
        udc->data.capoffset = DEF_CAPOFFSET;
+       udc->data.dr_mode = USB_DR_MODE_PERIPHERAL;
 
        udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
                                      pdev->num_resources, &udc->data);

I do not wish to set the dr_mode manually, since this could break things on devices I do not own.
Future work is needed to address the tegra_udc capabilities, as well as fully correct the hang issue.
This patch merely aims to fix a corner case.

Quoted 80 lines

Quoted 65 lines
They also seem to be missing the extcon phandle, which means automatic mode switching is not possible?
Quoted 3 lines
But one thing I could not understand, if the driver doesn't support dual-role, how could you do manual role switch?
Manual role switching is conducted via debugfs,
/sys/kernel/debug/usb/ci_hdrc.0/role
Quoted 21 lines
Quoted 21 lines
Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Signed-off-by Peter Geis <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 7455df0ede49..2f6f6ce3e8f5 100644
@@ -89,6 +89,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
      udc->data.usb_phy = udc->phy;
      udc->data.capoffset = DEF_CAPOFFSET;
 
Quoted 10 lines
drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 98
Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +7 -0
+     /* check the dual mode and warn about bad configurations */
+     if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_OTG &&
+        !of_property_read_bool(pdev->dev.of_node, "extcon")) {
+             dev_warn(&pdev->dev, "no extcon registered, otg unavailable");
+             udc->data.flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
+     }
+
The CI_HDRC_DUAL_ROLE_NOT_OTG flag may not be suitable here, please see comments for it.
I've dug around the various mailing lists and I fail to find any reference to why this is not a valid use case.
The commit message specifically references dual role capable devices without proper otg implementations, such as missing the otgsc register.
My current use case is the Ouya, which deadlocks the kernel durning probe if the otg capable usb controller is set to dr_mode=otg. It works with this flag set.
Unfortunately there isn't a property for dr_mode set to non_otg_dr_mode.
I found a post stating that the driver blindly touches registers in otg mode, which leads to the deadlock if those registers are read only or non-existent,
Eventually someone should look into why this deadlock occurs and make a proper fix that applies to all users.
Unfortunately I do not have the knowledge yet to accomplish this task.
With some simple modifications to the tegra_udc driver it is possible to get host mode working, vice using the tegra-ehci driver. At this point role switch works
Quoted 6 lines
I also managed to move all of the functionality of the tegra-ehci driver into the tegra-udc driver.
Unfortunately it doesn't behave correctly under some cases, so I never submitted it.
Do you have a recommendation for handling this?
If you would like adding host function in chipidea driver for tegra, and want dual-role switch to work, first, you may need to know which dual-role switch you need:
- Through controller's id and vbus pins
- Through extcon, eg,GPIO.
- Through usb-role-switch class, eg, Type-C controller
- Through sysfs, /sys/bus/platform/devices/ci_hdrc.0/role

Quoted 3 lines

The first three are automatically switch, the last one is manually. Current chipidea core should support all above switches, maybe there are some limitations for them, the fixes are welcome.

AFAIK reading through the comments, the chipidea driver intended to exclusively use extcon for automatic role switching, please correct me if I'm wrong here.
USB-C support is not a likely scenario, as currently the chipidea driver only supports usb 2.0.

Quoted 5 lines

Second, you may check if touch otgsc will hang or deadlock the system. If you can't touch otgsc when portsc.phcd = 0, you may need the flag CI_HDRC_DUAL_ROLE_NOT_OTG, afaik, few SoCs can't touch otgsc if it supports dual-role.

I added some traces to the driver, and it doesn't actually appear to be a register read/write that is breaking things. (Not directly anyways).
The hang occurs after it enumerates the usb gadgets and hub. Still trying to track down exactly where the hang occurs.

Quoted 8 lines

When you add host function, you may set dr_mode as host-only first, it could make thing easier.
--
Thanks,
Peter Chen

A message that is not in the archive (sent off-list or not yet received): CAMdYzYrbvsTunwxJLcC_-ZhczsQfyDLOjTnZ+eorb325qO-QhA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Peter Chen

2020-02-04 07:44 UTC #13 lore raw

Quoted 99 lines · Peter Geis

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 101
Quoted 12 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0
index 0c9911d44ee5..5d85363ad0f4 100644
@@ -95,6 +95,7 @@ static int tegra_udc_probe(struct platform_device *pdev)
        udc->data.flags = soc->flags;
        udc->data.usb_phy = udc->phy;
        udc->data.capoffset = DEF_CAPOFFSET;
+       udc->data.dr_mode = USB_DR_MODE_PERIPHERAL;
 
        udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
                                      pdev->num_resources, &udc->data);
I do not wish to set the dr_mode manually, since this could break things on devices I do not own.
Future work is needed to address the tegra_udc capabilities, as well as fully correct the hang issue.
This patch merely aims to fix a corner case.
Quoted 80 lines
Quoted 65 lines
They also seem to be missing the extcon phandle, which means automatic mode switching is not possible?
Quoted 3 lines
But one thing I could not understand, if the driver doesn't support dual-role, how could you do manual role switch?
Manual role switching is conducted via debugfs,
/sys/kernel/debug/usb/ci_hdrc.0/role
Quoted 21 lines
Quoted 21 lines
Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Signed-off-by Peter Geis <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 7455df0ede49..2f6f6ce3e8f5 100644
@@ -89,6 +89,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
      udc->data.usb_phy = udc->phy;
      udc->data.capoffset = DEF_CAPOFFSET;
 
Quoted 10 lines
drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 98
Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +7 -0
+     /* check the dual mode and warn about bad configurations */
+     if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_OTG &&
+        !of_property_read_bool(pdev->dev.of_node, "extcon")) {
+             dev_warn(&pdev->dev, "no extcon registered, otg unavailable");
+             udc->data.flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
+     }
+
The CI_HDRC_DUAL_ROLE_NOT_OTG flag may not be suitable here, please see comments for it.
I've dug around the various mailing lists and I fail to find any reference to why this is not a valid use case.
The commit message specifically references dual role capable devices without proper otg implementations, such as missing the otgsc register.
My current use case is the Ouya, which deadlocks the kernel durning probe if the otg capable usb controller is set to dr_mode=otg. It works with this flag set.
Unfortunately there isn't a property for dr_mode set to non_otg_dr_mode.
I found a post stating that the driver blindly touches registers in otg mode, which leads to the deadlock if those registers are read only or non-existent,
Eventually someone should look into why this deadlock occurs and make a proper fix that applies to all users.
Unfortunately I do not have the knowledge yet to accomplish this task.
With some simple modifications to the tegra_udc driver it is possible to get host mode working, vice using the tegra-ehci driver. At this point role switch works
Quoted 6 lines
I also managed to move all of the functionality of the tegra-ehci driver into the tegra-udc driver.
Unfortunately it doesn't behave correctly under some cases, so I never submitted it.
Do you have a recommendation for handling this?
If you would like adding host function in chipidea driver for tegra, and want dual-role switch to work, first, you may need to know which dual-role switch you need:
- Through controller's id and vbus pins
- Through extcon, eg,GPIO.
- Through usb-role-switch class, eg, Type-C controller
- Through sysfs, /sys/bus/platform/devices/ci_hdrc.0/role

Quoted 7 lines

Quoted 3 lines
The first three are automatically switch, the last one is manually. Current chipidea core should support all above switches, maybe there are some limitations for them, the fixes are welcome.
AFAIK reading through the comments, the chipidea driver intended to exclusively use extcon for automatic role switching, please correct me if I'm wrong here.

No, extcon is supported.

Quoted 2 lines

USB-C support is not a likely scenario, as currently the chipidea driver only supports usb 2.0.

USB-C is a connector, not related to USB HW revision. For USB 2.0 USB-C solution, there is no SS TX/RX connected on connector.

Quoted 7 lines

Quoted 6 lines
Second, you may check if touch otgsc will hang or deadlock the system. If you can't touch otgsc when portsc.phcd = 0, you may need the flag CI_HDRC_DUAL_ROLE_NOT_OTG, afaik, few SoCs can't touch otgsc if it supports dual-role.

Quoted 6 lines

I added some traces to the driver, and it doesn't actually appear to be a register read/write that is breaking things. (Not directly anyways).
The hang occurs after it enumerates the usb gadgets and hub. Still trying to track down exactly where the hang occurs.

Try to see if it is related to runtime power management.

Signature · 2 lines

Thanks,
Peter Chen

Peter Geis

author

2020-02-04 14:35 UTC #14 lore raw

Quoted 124 lines · Peter Chen

Quoted 99 lines · Peter Geis
drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 101
Quoted 12 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0
index 0c9911d44ee5..5d85363ad0f4 100644
@@ -95,6 +95,7 @@ static int tegra_udc_probe(struct platform_device *pdev)
        udc->data.flags = soc->flags;
        udc->data.usb_phy = udc->phy;
        udc->data.capoffset = DEF_CAPOFFSET;
+       udc->data.dr_mode = USB_DR_MODE_PERIPHERAL;
 
        udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
                                      pdev->num_resources, &udc->data);
I do not wish to set the dr_mode manually, since this could break things on devices I do not own.
Future work is needed to address the tegra_udc capabilities, as well as fully correct the hang issue.
This patch merely aims to fix a corner case.
Quoted 80 lines
Quoted 65 lines
They also seem to be missing the extcon phandle, which means automatic mode switching is not possible?
Quoted 3 lines
But one thing I could not understand, if the driver doesn't support dual-role, how could you do manual role switch?
Manual role switching is conducted via debugfs,
/sys/kernel/debug/usb/ci_hdrc.0/role
Quoted 21 lines
Quoted 21 lines
Detect this state by checking for the extcon phandle when dual role mode is set to otg.
If it doesn't exist request the driver to only enable manual role switching.
Fixes dfebb5f ("usb: chipidea: Add support for Tegra20/30/114/124")
Signed-off-by Peter Geis <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 7455df0ede49..2f6f6ce3e8f5 100644
@@ -89,6 +89,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
      udc->data.usb_phy = udc->phy;
      udc->data.capoffset = DEF_CAPOFFSET;
 
Quoted 10 lines
drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_probe(struct platform_device *pdev) · line 98
Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +7 -0
+     /* check the dual mode and warn about bad configurations */
+     if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_OTG &&
+        !of_property_read_bool(pdev->dev.of_node, "extcon")) {
+             dev_warn(&pdev->dev, "no extcon registered, otg unavailable");
+             udc->data.flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
+     }
+
The CI_HDRC_DUAL_ROLE_NOT_OTG flag may not be suitable here, please see comments for it.
I've dug around the various mailing lists and I fail to find any reference to why this is not a valid use case.
The commit message specifically references dual role capable devices without proper otg implementations, such as missing the otgsc register.
My current use case is the Ouya, which deadlocks the kernel durning probe if the otg capable usb controller is set to dr_mode=otg. It works with this flag set.
Unfortunately there isn't a property for dr_mode set to non_otg_dr_mode.
I found a post stating that the driver blindly touches registers in otg mode, which leads to the deadlock if those registers are read only or non-existent,
Eventually someone should look into why this deadlock occurs and make a proper fix that applies to all users.
Unfortunately I do not have the knowledge yet to accomplish this task.
With some simple modifications to the tegra_udc driver it is possible to get host mode working, vice using the tegra-ehci driver. At this point role switch works
Quoted 6 lines
I also managed to move all of the functionality of the tegra-ehci driver into the tegra-udc driver.
Unfortunately it doesn't behave correctly under some cases, so I never submitted it.
Do you have a recommendation for handling this?
If you would like adding host function in chipidea driver for tegra, and want dual-role switch to work, first, you may need to know which dual-role switch you need:
- Through controller's id and vbus pins
- Through extcon, eg,GPIO.
- Through usb-role-switch class, eg, Type-C controller
- Through sysfs, /sys/bus/platform/devices/ci_hdrc.0/role
Quoted 7 lines
Quoted 3 lines
The first three are automatically switch, the last one is manually. Current chipidea core should support all above switches, maybe there are some limitations for them, the fixes are welcome.
AFAIK reading through the comments, the chipidea driver intended to exclusively use extcon for automatic role switching, please correct me if I'm wrong here.
No, extcon is supported.
Quoted 2 lines
USB-C support is not a likely scenario, as currently the chipidea driver only supports usb 2.0.
USB-C is a connector, not related to USB HW revision. For USB 2.0 USB-C solution, there is no SS TX/RX connected on connector.
Quoted 7 lines
Quoted 6 lines
Second, you may check if touch otgsc will hang or deadlock the system. If you can't touch otgsc when portsc.phcd = 0, you may need the flag CI_HDRC_DUAL_ROLE_NOT_OTG, afaik, few SoCs can't touch otgsc if it supports dual-role.

Quoted 8 lines

Quoted 6 lines
I added some traces to the driver, and it doesn't actually appear to be a register read/write that is breaking things. (Not directly anyways).
The hang occurs after it enumerates the usb gadgets and hub. Still trying to track down exactly where the hang occurs.
Try to see if it is related to runtime power management.

That's it!
There doesn't appear to be a method to disable PM inside the chipidea driver. Do you have a suggestion on how to do it, aside from the global method?

Quoted 5 lines

--
Thanks,
Peter Chen

A message that is not in the archive (sent off-list or not yet received): CAMdYzYo8Vgw8h=LtfLnQNF4j-rVzgKJTp1hCyf7BFKrdhAhAHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Peter Chen

2020-02-05 11:17 UTC #16 lore raw

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 5 lines
Second, you may check if touch otgsc will hang or deadlock the system. If you can't touch otgsc when portsc.phcd = 0, you may need the flag CI_HDRC_DUAL_ROLE_NOT_OTG, afaik, few SoCs can't touch otgsc if it supports dual-role.
I added some traces to the driver, and it doesn't actually appear to be a register read/write that is breaking things. (Not directly anyways).
The hang occurs after it enumerates the usb gadgets and hub. Still trying to track down exactly where the hang occurs.

Quoted 6 lines

Quoted 1 line
Try to see if it is related to runtime power management.
That's it!
There doesn't appear to be a method to disable PM inside the chipidea driver. Do you have a suggestion on how to do it, aside from the global method?

See drivers/usb/chipidea/ci_hdrc_imx.c for reference please.

Peter