Re: [PATCH v4] usb: dwc3: Don't switch OTG -> peripheral if extcon is present
From: Ferry Toth
Date: Sun Sep 25 2022 - 15:21:15 EST
Hi,
Promising results below.
Op 24-09-2022 om 23:29 schreef Ferry Toth:
Hi,
One more test
Op 23-09-2022 om 20:23 schreef Andrey Smirnov:
On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
On Thu, Sep 22, 2022 at 04:32:55PM -0700, Andrey Smirnov wrote:
On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <fntoth@xxxxxxxxx> wrote:
On 22-09-2022 12:08, Andy Shevchenko wrote:
On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
FYI: For now I sent a revert, but if we got a solution quicker we
always
can choose the course of actions.
I think we have another problem. This patch happened in parallel to mine
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc6&id=ab7aa2866d295438dc60522f85c5421c6b4f1507
so my changes didn't have that fix in mind and I think your revert
will not preserve that fix. Can you update your revert to take care of
that too, please?
I'm really confused how the above commit could be followed up by:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/dwc3/drd.c?h=v6.0-rc6&id=0f01017191384e3962fa31520a9fd9846c3d352f
the diffs in dwc3_drd_init seem contradictory
If the extcon device exists, get the mode from the extcon device. If
the controller is DRD and the driver is unable to determine the mode,
only then default the dr_mode to USB_DR_MODE_PERIPHERAL.
According to Ferry (Cc'ed) this broke Intel Merrifield platform.
Ferry, can you
share bisect log?
I can but not right now. But what I did was bisect between 5.18.0
(good) and 5.19.0 (bad) then when I got near the culprit (~20
remaining) based on the commit message I tried
0f01017191384e3962fa31520a9fd9846c3d352f "usb: dwc3: Don't switch
OTG -> peripheral if extcon is present" (bad) and commit before
that (good).
The effect of the patch is that on Merrifield (I tested with Intel
Edison Arduino board which has a HW switch to select between host
and device mode) device mode works but in host mode USB is
completely not working.
Currently on host mode - when working - superfluous error messages
from tusb1210 appear. When host mode is not working there are no
tusb1210 messages in the logs / on the console at all. Seemingly
tusb1210 is not probed, which points in the direction of a
relation to extcon.
Taking into account the late cycle, I would like to revert the
change. And
Ferry and I would help to test any other (non-regressive) approach).
I have not yet tested if a simple revert fixes the problem but
will tonight.
I would be happy to test other approaches too.
It's a bit hard for me to suggest an alternative approach without
knowing how things are breaking in this case. I'd love to order one of
those boards to repro and fix this on my end, but it looks like this
HW is EOLed and out of stock in most places. If you guys know how to
get my hands on those boards I'm all ears.
There are still some second hand Intel Edison boards flying around
(but maybe cost a bit more than expected) and there are also
Dell Venue 7 3740 tablets based on the same platform/SoC. The latter
option though requires more actions in order something to be boot
there.
OK, I'll check e-bay just in case.
In any case, it's probably quicker to ask Ferry or me for testing.
(Although currently I have no access to the board to test OTG, it's
remote device which I can only power on and off and it has always
be in host mode.)
Barring that, Ferry can you dig more into this failure? E.g. is it
this hunk
@@ -85,7 +86,7 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc)
* mode. If the controller supports DRD but the
dr_mode is not
* specified or set to OTG, then set the mode to
peripheral.
*/
- if (mode == USB_DR_MODE_OTG &&
+ if (mode == USB_DR_MODE_OTG && !dwc->edev &&
(!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) ||
!device_property_read_bool(dwc->dev, "usb-role-switch")) &&
!DWC3_VER_IS_PRIOR(DWC3, 330A))
@@ -1632,6 +1633,51 @@ static void dwc3_check_params(struct dwc3 *dwc)
}
}
that's problematic or moving
I think you wanted to revert only this line and test?
Yes.
static int dwc3_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1744,6 +1790,13 @@ static int dwc3_probe(struct platform_device
*pdev)
goto err2;
}
+ dwc->edev = dwc3_get_extcon(dwc);
+ if (IS_ERR(dwc->edev)) {
+ ret = PTR_ERR(dwc->edev);
+ dev_err_probe(dwc->dev, ret, "failed to get
extcon\n");
+ goto err3;
+ }
+
ret = dwc3_get_dr_mode(dwc);
if (ret)
goto err3;
to happen earlier?
I tried moving dwc3_get_extcon after dwc3_get_dr_mode like so::
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8c8e32651473..3bf370def546 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1843,6 +1843,10 @@ static int dwc3_probe(struct platform_device
*pdev)
goto err2;
}
+ ret = dwc3_get_dr_mode(dwc);
+ if (ret)
+ goto err3;
+
dwc->edev = dwc3_get_extcon(dwc);
if (IS_ERR(dwc->edev)) {
ret = PTR_ERR(dwc->edev);
@@ -1850,10 +1854,6 @@ static int dwc3_probe(struct platform_device
*pdev)
goto err3;
}
- ret = dwc3_get_dr_mode(dwc);
- if (ret)
- goto err3;
-
ret = dwc3_alloc_scratch_buffers(dwc);
if (ret)
goto err3;
After trying to understand the code a bit, I successfully tested the
following move:
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8c8e32651473..4a38cff8cb16 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1843,13 +1843,6 @@ static int dwc3_probe(struct platform_device *pdev)
goto err2;
}
- dwc->edev = dwc3_get_extcon(dwc);
- if (IS_ERR(dwc->edev)) {
- ret = PTR_ERR(dwc->edev);
- dev_err_probe(dwc->dev, ret, "failed to get extcon\n");
- goto err3;
- }
-
ret = dwc3_get_dr_mode(dwc);
if (ret)
goto err3;
@@ -1867,6 +1860,13 @@ static int dwc3_probe(struct platform_device *pdev)
dwc3_check_params(dwc);
dwc3_debugfs_init(dwc);
+ dwc->edev = dwc3_get_extcon(dwc);
+ if (IS_ERR(dwc->edev)) {
+ ret = PTR_ERR(dwc->edev);
+ dev_err_probe(dwc->dev, ret, "failed to get extcon\n");
+ goto err5;
+ }
+
ret = dwc3_core_init_mode(dwc);
if (ret)
goto err5;
This moves dwc3_get_extcon() until after dwc3_core_init() but just
before dwc3_core_init_mode(). AFAIU initially dwc3_get_extcon() was
called from within dwc3_core_init_mode() but only for case
USB_DR_MODE_OTG. So with this change order of events is more or less
unchanged.
Due to move I modified goto to err5, not sure if that is correct.
Thoughts? Can we get something like this in quick or should we revert first?