[v4] usb: dwc3: Don't switch OTG -> peripheral if extcon is present

Commit Message

Andrey Smirnov April 3, 2022, 4:49 p.m. UTC

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.

Cc: Felipe Balbi <[e-mail]>
Cc: Thinh Nguyen <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Reviewed-by: Thinh Nguyen <[e-mail]>
Signed-off-by: Andrey Smirnov <[e-mail]>
---

Changes since [v3] of the patch:

  - Rebased to apply on "main" of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git

Changes since [v2] of the patch:

   - Fixed "Minor formatting change" to reflect what was meant by
     review comment

Changes since [v1] of the patch:

   - Reworded commit message
   - Minor formatting change


[v3] https://lore.kernel.org/linux-usb/[e-mail]/
[v2] https://lore.kernel.org/linux-usb/[e-mail]/
[v1] https://lore.kernel.org/linux-usb/[e-mail]/T/#u

previons discussion:

https://lore.kernel.org/linux-usb/[e-mail]/


 drivers/usb/dwc3/core.c | 55 ++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/dwc3/drd.c  | 50 -------------------------------------
 2 files changed, 54 insertions(+), 51 deletions(-)

--
2.25.1

Comments

Andy Shevchenko Sept. 22, 2022, 10:08 a.m. UTC | #1

On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
> 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?

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).

Andy Shevchenko Sept. 22, 2022, 1:29 p.m. UTC | #2

On Thu, Sep 22, 2022 at 12:23:04PM +0200, Ferry Toth wrote:
> On 22-09-2022 12:08, Andy Shevchenko wrote:
> > On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
> > > 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.

For clean revert you might need to revert the merge conflict fixes first:
8bd6b8c4b100 ("USB: fixup for merge issue with "usb: dwc3: Don't switch
OTG -> peripheral if extcon is present"").

> I would be happy to test other approaches too.

Ferry Toth Sept. 22, 2022, 2:30 p.m. UTC | #3

Hi,

Resending this due to html reject, sorry.

On 22-09-2022 12:08, Andy Shevchenko wrote:
> On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
>> 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.

Ferry Toth Sept. 22, 2022, 8:35 p.m. UTC | #4

Hi,

Op 22-09-2022 om 15:29 schreef Andy Shevchenko:
> On Thu, Sep 22, 2022 at 12:23:04PM +0200, Ferry Toth wrote:
>> On 22-09-2022 12:08, Andy Shevchenko wrote:
>>> On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
>>>> 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?
git bisect start
# bad: [3d7cb6b04c3f3115719235cc6866b10326de34cd] Linux 5.19
git bisect bad 3d7cb6b04c3f3115719235cc6866b10326de34cd
# good: [4b0986a3613c92f4ec1bdc7f60ec66fea135991f] Linux 5.18
git bisect good 4b0986a3613c92f4ec1bdc7f60ec66fea135991f
# good: [c011dd537ffe47462051930413fed07dbdc80313] Merge tag 
'arm-soc-5.19' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
git bisect good c011dd537ffe47462051930413fed07dbdc80313
# good: [5d4af9c1f04ab0411ba5818baad9a68e87f33099] Merge branch 
'mv88e6xxx-fixes-for-reading-serdes-state'
git bisect good 5d4af9c1f04ab0411ba5818baad9a68e87f33099
# bad: [7a68065eb9cd194cf03f135c9211eeb2d5c4c0a0] Merge tag 
'gpio-fixes-for-v5.19-rc2' of 
git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux
git bisect bad 7a68065eb9cd194cf03f135c9211eeb2d5c4c0a0
# bad: [54c2cc79194c961a213c1d375fe3aa4165664cc4] Merge tag 
'usb-5.19-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect bad 54c2cc79194c961a213c1d375fe3aa4165664cc4
# good: [6a31a95135da0bb2c5349e49e37d76e9909ab7ea] staging: r8188eu: 
remove include/rtw_debug.h
git bisect good 6a31a95135da0bb2c5349e49e37d76e9909ab7ea
# good: [04d93b2b8bc7a68ec45a6a156f34a611ede5aa60] Merge tag 
'spdx-5.19-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/spdx
git bisect good 04d93b2b8bc7a68ec45a6a156f34a611ede5aa60
# good: [c069d2756c01ed36121fae6a42c14fdf1325c71d] serial: sifive: 
Sanitize CSIZE and c_iflag
git bisect good c069d2756c01ed36121fae6a42c14fdf1325c71d
# bad: [3120aac6d0ecd9accf56894aeac0e265f74d3d5a] usb: dwc2: gadget: 
don't reset gadget's driver->bus
git bisect bad 3120aac6d0ecd9accf56894aeac0e265f74d3d5a
# bad: [0f01017191384e3962fa31520a9fd9846c3d352f] usb: dwc3: Don't 
switch OTG -> peripheral if extcon is present
git bisect bad 0f01017191384e3962fa31520a9fd9846c3d352f
# good: [b8a19881337678c02bb3d72ae821602e1a4c377d] usb: gadget: u_audio: 
clean up some inconsistent indenting
git bisect good b8a19881337678c02bb3d72ae821602e1a4c377d
# good: [424bef51fa530389b0b9008c9e144e40c10e8458] usb: musb: Fix 
missing of_node_put() in omap2430_probe
git bisect good 424bef51fa530389b0b9008c9e144e40c10e8458

>>> 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).
5c29e864999763baec9eedb9ea5bd557aa4cbd77
>>> 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.
> For clean revert you might need to revert the merge conflict fixes first:
> 8bd6b8c4b100 ("USB: fixup for merge issue with "usb: dwc3: Don't switch
> OTG -> peripheral if extcon is present"").

Tested successfully on Intel Merrifield with v6.0-rc6 with reverted:

8bd6b8c4 ("USB: fixup for merge issue with "usb: dwc3: Don't switch OTG -> peripheral if extcon is present"")

0f010171 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present")

I also tested on v5.19 which in addition requires a back port of

d4a0a189 ("phy: ti: tusb1210: Don't check for write errors when powering 
on")

>> I would be happy to test other approaches too.

Andrey Smirnov Sept. 22, 2022, 11:32 p.m. UTC | #5

On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <[e-mail]> wrote:
>
> Hi,
>
> On 22-09-2022 12:08, Andy Shevchenko wrote:
>
> On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
>
> 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.

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

 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? Does tracing the "mrfld_bcove_pwrsrc" driver (the
excton provider in this case AFIACT) show anything interesting?

Andy Shevchenko Sept. 23, 2022, 4:42 p.m. UTC | #6

On Thu, Sep 22, 2022 at 04:32:55PM -0700, Andrey Smirnov wrote:
> On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <[e-mail]> 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.

> > 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.

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?

>  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?

It is not always possible to have an extcon driver available, that's why in
some cases the probe of it defers. I dunno how your patch supposed to work
in that case.

> Does tracing the "mrfld_bcove_pwrsrc" driver (the
> excton provider in this case AFIACT) show anything interesting?

I believe there is nothing interesting.

Andrey Smirnov Sept. 23, 2022, 6:23 p.m. UTC | #7

On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
<[e-mail]> 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 <[e-mail]> 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?
>
> It is not always possible to have an extcon driver available, that's why in
> some cases the probe of it defers. I dunno how your patch supposed to work
> in that case.

I'm not sure I understand what you mean. AFAIU the logic is that if
the platform specifies the presence of extcon either via DT or, like
Merrifield, via and explicit "linux,extcon-name" device property in
the code then extcon is a mandatory component of the DRD stack and the
driver is expected to be present for the whole thing to work. I don't
think I really changed that logic with my patch, even after the revert
dwc3_get_extcon() will be called as a part of a probing codepath, so
if the a missing driver is causing a probe deferral it should still be
happening, unless I missed something.

>
> > Does tracing the "mrfld_bcove_pwrsrc" driver (the
> > excton provider in this case AFIACT) show anything interesting?
>
> I believe there is nothing interesting.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Sven Peter Sept. 23, 2022, 6:35 p.m. UTC | #8

On Fri, Sep 23, 2022, at 20:23, Andrey Smirnov wrote:
> On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
> <[e-mail]> 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 <[e-mail]> 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

I noticed this a while ago when I finally rebased the M1 USB3 PHY WIP branch
and have been meaning to send a fix. Then life unfortunately got in the way and
I completely forgot about it again.

Both patches were sent at approximately the same time and I think got merged into
two separate branches. The conflict resolution [1] then went bad but I didn't notice
until weeks later :(


Best,

Sven


[1] https://lore.kernel.org/lkml/[e-mail]/

Andy Shevchenko Sept. 23, 2022, 6:54 p.m. UTC | #9

On Fri, Sep 23, 2022 at 11:23:23AM -0700, Andrey Smirnov wrote:
> On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
> <[e-mail]> 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 <[e-mail]> 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

I'm not sure I follow. Your patch has been merged and after that some kind of
merge conflict was resolved by an additional change. To revert your stuff
cleanly we need to revert the merge update patch first. That's why revert is a
series of patches and not a single one. I have no idea how above mentioned
commit at all related to all this.

Can you elaborate more, please?

> > > > 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.

Ferry, can you try that (but I believe it won't help anyway, because I don't
see how we handle deferred probe).

> > >  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?
> >
> > It is not always possible to have an extcon driver available, that's why in
> > some cases the probe of it defers. I dunno how your patch supposed to work
> > in that case.
> 
> I'm not sure I understand what you mean. AFAIU the logic is that if
> the platform specifies the presence of extcon either via DT or, like
> Merrifield, via and explicit "linux,extcon-name" device property in
> the code then extcon is a mandatory component of the DRD stack and the
> driver is expected to be present for the whole thing to work.

> I don't
> think I really changed that logic with my patch, even after the revert
> dwc3_get_extcon() will be called as a part of a probing codepath,

But it's not true as proved by the experiment. So with your patch it doesn't
work anymore, so the logic _is_ changed.

>	so
> if the a missing driver is causing a probe deferral it should still be
> happening, unless I missed something.

The merge fix removes deferred probe by some reason.

> > > Does tracing the "mrfld_bcove_pwrsrc" driver (the
> > > excton provider in this case AFIACT) show anything interesting?
> >
> > I believe there is nothing interesting.

Andy Shevchenko Sept. 23, 2022, 6:58 p.m. UTC | #10

+Stephen to help to realize what the mess we have now...

On Fri, Sep 23, 2022 at 08:35:13PM +0200, Sven Peter wrote:
> On Fri, Sep 23, 2022, at 20:23, Andrey Smirnov wrote:
> > On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
> > <[e-mail]> 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 <[e-mail]> 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
> 
> I noticed this a while ago when I finally rebased the M1 USB3 PHY WIP branch
> and have been meaning to send a fix. Then life unfortunately got in the way and
> I completely forgot about it again.
> 
> Both patches were sent at approximately the same time and I think got merged into
> two separate branches. The conflict resolution [1] then went bad but I didn't notice
> until weeks later :(

Folks, I have no idea what you are talking about. Can you check that revert
series [2] gets your change still in? Because I have no clue how it's involved at
all into discussion.

> [1] https://lore.kernel.org/lkml/[e-mail]/

[2]: https://lore.kernel.org/linux-usb/[e-mail]/

Ferry Toth Sept. 23, 2022, 8:10 p.m. UTC | #11

Hi

Op 23-09-2022 om 20:23 schreef Andrey Smirnov:
> On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
> <[e-mail]> 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 <[e-mail]> 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.

I see one Edison + Arduino board on eBay $99.99

>> 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?
>>
>> It is not always possible to have an extcon driver available, that's why in
>> some cases the probe of it defers. I dunno how your patch supposed to work
>> in that case.
> 
> I'm not sure I understand what you mean. AFAIU the logic is that if
> the platform specifies the presence of extcon either via DT or, like
> Merrifield, via and explicit "linux,extcon-name" device property in
> the code then extcon is a mandatory component of the DRD stack and the
> driver is expected to be present for the whole thing to work. I don't
> think I really changed that logic with my patch, even after the revert
> dwc3_get_extcon() will be called as a part of a probing codepath, so
> if the a missing driver is causing a probe deferral it should still be
> happening, unless I missed something.
> 
>>
>>> Does tracing the "mrfld_bcove_pwrsrc" driver (the
>>> excton provider in this case AFIACT) show anything interesting?
>>
>> I believe there is nothing interesting.
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>

Ferry Toth Sept. 23, 2022, 9:12 p.m. UTC | #12

Hi,

Op 23-09-2022 om 18:42 schreef Andy Shevchenko:
> On Thu, Sep 22, 2022 at 04:32:55PM -0700, Andrey Smirnov wrote:
>> On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <[e-mail]> 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.
>
>>> 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.
>
> 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?

On v6.0-rc6 and reverting manually only this line

- if (mode == USB_DR_MODE_OTG && !dwc->edev &&

+ if (mode == USB_DR_MODE_OTG &&

host mode still does not work (no change visible).

>
>>   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?
> It is not always possible to have an extcon driver available, that's why in
> some cases the probe of it defers. I dunno how your patch supposed to work
> in that case.
>
>> Does tracing the "mrfld_bcove_pwrsrc" driver (the
>> excton provider in this case AFIACT) show anything interesting?
> I believe there is nothing interesting.
>

Andrey Smirnov Sept. 24, 2022, 1:07 a.m. UTC | #13

On Fri, Sep 23, 2022 at 11:58 AM Andy Shevchenko
<[e-mail]> wrote:
>
> +Stephen to help to realize what the mess we have now...
>
> On Fri, Sep 23, 2022 at 08:35:13PM +0200, Sven Peter wrote:
> > On Fri, Sep 23, 2022, at 20:23, Andrey Smirnov wrote:
> > > On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
> > > <[e-mail]> 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 <[e-mail]> 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
> >
> > I noticed this a while ago when I finally rebased the M1 USB3 PHY WIP branch
> > and have been meaning to send a fix. Then life unfortunately got in the way and
> > I completely forgot about it again.
> >
> > Both patches were sent at approximately the same time and I think got merged into
> > two separate branches. The conflict resolution [1] then went bad but I didn't notice
> > until weeks later :(
>
> Folks, I have no idea what you are talking about. Can you check that revert
> series [2] gets your change still in? Because I have no clue how it's involved at
> all into discussion.
>
> > [1] https://lore.kernel.org/lkml/[e-mail]/
>
> [2]: https://lore.kernel.org/linux-usb/[e-mail]/
>

Here's Sven's diff:

diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index b60b5f7b6dff4..8cad9e7d33687 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -584,16 +584,15 @@ int dwc3_drd_init(struct dwc3 *dwc)
{
int ret, irq;
+ if (ROLE_SWITCH &&
+ device_property_read_bool(dwc->dev, "usb-role-switch"))
+ return dwc3_setup_role_switch(dwc);
+
dwc->edev = dwc3_get_extcon(dwc);
if (IS_ERR(dwc->edev))
return PTR_ERR(dwc->edev);
- if (ROLE_SWITCH &&
- device_property_read_bool(dwc->dev, "usb-role-switch")) {
- ret = dwc3_setup_role_switch(dwc);
- if (ret < 0)
- return ret;
- } else if (dwc->edev) {
+ if (dwc->edev) {


Here's your revert of my patch:

@@ -538,6 +584,10 @@ int dwc3_drd_init(struct dwc3 *dwc)
 {
        int ret, irq;

+       dwc->edev = dwc3_get_extcon(dwc);
+       if (IS_ERR(dwc->edev))
+               return PTR_ERR(dwc->edev);
+
        if (ROLE_SWITCH &&
            device_property_read_bool(dwc->dev, "usb-role-switch"))
                return dwc3_setup_role_switch(dwc);


There's an order of operations difference. Dwc3_get_extcon() Needs to
be happening after if (ROLE_SWITCH

Andrey Smirnov Sept. 24, 2022, 1:27 a.m. UTC | #14

On Fri, Sep 23, 2022 at 11:54 AM Andy Shevchenko
<[e-mail]> wrote:
>
> On Fri, Sep 23, 2022 at 11:23:23AM -0700, Andrey Smirnov wrote:
> > On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
> > <[e-mail]> 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 <[e-mail]> 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
>
> I'm not sure I follow. Your patch has been merged and after that some kind of
> merge conflict was resolved by an additional change. To revert your stuff
> cleanly we need to revert the merge update patch first. That's why revert is a
> series of patches and not a single one. I have no idea how above mentioned
> commit at all related to all this.
>
> Can you elaborate more, please?
>

It's not important to clarify, just me voicing my confusion, we have
way too many threads of discussion already.

> > > > > 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.
>
> Ferry, can you try that (but I believe it won't help anyway, because I don't
> see how we handle deferred probe).
>
> > > >  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?
> > >
> > > It is not always possible to have an extcon driver available, that's why in
> > > some cases the probe of it defers. I dunno how your patch supposed to work
> > > in that case.
> >
> > I'm not sure I understand what you mean. AFAIU the logic is that if
> > the platform specifies the presence of extcon either via DT or, like
> > Merrifield, via and explicit "linux,extcon-name" device property in
> > the code then extcon is a mandatory component of the DRD stack and the
> > driver is expected to be present for the whole thing to work.
>
> > I don't
> > think I really changed that logic with my patch, even after the revert
> > dwc3_get_extcon() will be called as a part of a probing codepath,
>
> But it's not true as proved by the experiment. So with your patch it doesn't
> work anymore, so the logic _is_ changed.
>

I think you are jumping the gun here. We know that the patch breaks
USB host functionality on Merrifield. We know that "Seemingly tusb1210
is not probed". Do we know that dwc3.ko (I think that'd be the
driver's name) is not probed? Did Ferry share that info with you in
some other thread? I don't deny it is possible, but I don't think this
is really clear at this moment to say definitively.

> >       so
> > if the a missing driver is causing a probe deferral it should still be
> > happening, unless I missed something.
>
> The merge fix removes deferred probe by some reason.
>
> > > > Does tracing the "mrfld_bcove_pwrsrc" driver (the
> > > > excton provider in this case AFIACT) show anything interesting?
> > >
> > > I believe there is nothing interesting.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Andrey Smirnov Sept. 24, 2022, 1:34 a.m. UTC | #15

On Fri, Sep 23, 2022 at 2:12 PM Ferry Toth <[e-mail]> wrote:
>
> Hi,
>
> Op 23-09-2022 om 18:42 schreef Andy Shevchenko:
> > On Thu, Sep 22, 2022 at 04:32:55PM -0700, Andrey Smirnov wrote:
> >> On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <[e-mail]> 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.
> >
> >>> 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.
> >
> > 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?
>
> On v6.0-rc6 and reverting manually only this line
>
> - if (mode == USB_DR_MODE_OTG && !dwc->edev &&
>
> + if (mode == USB_DR_MODE_OTG &&
>
> host mode still does not work (no change visible).

Cool, thanks for checking that. Don't think I have any more
experiments off the top of my head to run. I'll have to go read that
code more. I'll reply in the thread if I have something new to
try/say.

>
> >
> >>   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?
> > It is not always possible to have an extcon driver available, that's why in
> > some cases the probe of it defers. I dunno how your patch supposed to work
> > in that case.
> >
> >> Does tracing the "mrfld_bcove_pwrsrc" driver (the
> >> excton provider in this case AFIACT) show anything interesting?
> > I believe there is nothing interesting.
> >

Ferry Toth Sept. 24, 2022, 11:55 a.m. UTC | #16

Hi,

Op 24-09-2022 om 03:27 schreef Andrey Smirnov:
> On Fri, Sep 23, 2022 at 11:54 AM Andy Shevchenko
> <[e-mail]> wrote:
>> On Fri, Sep 23, 2022 at 11:23:23AM -0700, Andrey Smirnov wrote:
>>> On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
>>> <[e-mail]> 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 <[e-mail]> 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
>> I'm not sure I follow. Your patch has been merged and after that some kind of
>> merge conflict was resolved by an additional change. To revert your stuff
>> cleanly we need to revert the merge update patch first. That's why revert is a
>> series of patches and not a single one. I have no idea how above mentioned
>> commit at all related to all this.
>>
>> Can you elaborate more, please?
>>
> It's not important to clarify, just me voicing my confusion, we have
> way too many threads of discussion already.
>
>>>>>> 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.
>> Ferry, can you try that (but I believe it won't help anyway, because I don't
>> see how we handle deferred probe).
>>
>>>>>   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?
>>>> It is not always possible to have an extcon driver available, that's why in
>>>> some cases the probe of it defers. I dunno how your patch supposed to work
>>>> in that case.
>>> I'm not sure I understand what you mean. AFAIU the logic is that if
>>> the platform specifies the presence of extcon either via DT or, like
>>> Merrifield, via and explicit "linux,extcon-name" device property in
>>> the code then extcon is a mandatory component of the DRD stack and the
>>> driver is expected to be present for the whole thing to work.
>>> I don't
>>> think I really changed that logic with my patch, even after the revert
>>> dwc3_get_extcon() will be called as a part of a probing codepath,
>> But it's not true as proved by the experiment. So with your patch it doesn't
>> work anymore, so the logic _is_ changed.
>>
> I think you are jumping the gun here. We know that the patch breaks
> USB host functionality on Merrifield. We know that "Seemingly tusb1210
> is not probed". Do we know that dwc3.ko (I think that'd be the
> driver's name) is not probed? Did Ferry share that info with you in
> some other thread? I don't deny it is possible, but I don't think this
> is really clear at this moment to say definitively.

I am not sure. I have dwc3 builtin. And intel_soc_pmic_mrfld and 
extcon-intel-mrfld as a module.

But with the USB host broken this returns nothing:

root@yuna:~# journalctl -k -b -0 | grep -i dwc

While with the 2 reverts (and host working):

root@yuna:~# journalctl -k -b -1 | grep -i dwc
Sep 22 22:57:38 yuna kernel: tusb1210 dwc3.0.auto.ulpi: GPIO lookup for 
consumer reset
...

Sep 22 22:57:38 yuna kernel: debugfs: Directory 'dwc3.0.auto' with 
parent 'ulpi' already present!
...

Sep 22 22:57:39 yuna kernel: tusb1210 dwc3.0.auto.ulpi: error -110 
writing val 0x41 to reg 0x80

Like I mentioned before, when host works I get warnings and errors on 
the console as well as in the logs.

I also get this one, but believe that is related to another problem, 
something in hub.c. Which happens when the host works as a hub is plugin 
there.

Sep 22 22:57:39 yuna kernel: DMA-API: dwc3 dwc3.0.auto: cacheline 
tracking EEXIST, overlapping mappings aren't supported

>>>        so
>>> if the a missing driver is causing a probe deferral it should still be
>>> happening, unless I missed something.
>> The merge fix removes deferred probe by some reason.
>>
>>>>> Does tracing the "mrfld_bcove_pwrsrc" driver (the
>>>>> excton provider in this case AFIACT) show anything interesting?
>>>> I believe there is nothing interesting.
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>

Ferry Toth Sept. 24, 2022, 4:06 p.m. UTC | #17

Hi,

Maybe some inspiration below.

Op 24-09-2022 om 03:34 schreef Andrey Smirnov:
> On Fri, Sep 23, 2022 at 2:12 PM Ferry Toth <[e-mail]> wrote:
>> Hi,
>>
>> Op 23-09-2022 om 18:42 schreef Andy Shevchenko:
>>> On Thu, Sep 22, 2022 at 04:32:55PM -0700, Andrey Smirnov wrote:
>>>> On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <[e-mail]> 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.
>>>
>>>>> 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.
>>>
>>> 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?
>> On v6.0-rc6 and reverting manually only this line
>>
>> - if (mode == USB_DR_MODE_OTG && !dwc->edev &&
>>
>> + if (mode == USB_DR_MODE_OTG &&
>>
>> host mode still does not work (no change visible).
> Cool, thanks for checking that. Don't think I have any more
> experiments off the top of my head to run. I'll have to go read that
> code more. I'll reply in the thread if I have something new to
> try/say.

It seems the problem is not extcon directly. When I switch to device 
mode, usb gadgets are working fire. Also when I

# cat /sys/class/extcon/extcon0/state
USB=0
USB-HOST=1
SDP=0
CDP=0
DCP=0
ACA=0

USB-HOST changes nicely to 0 and back when I flip the switch.

Also, in host mode I normally have (and now with host mode not working 
it the same):

root@yuna:~# dmesg | grep -i usb
ACPI: bus type USB registered
...
usbcore: registered new interface driver usbhid
usbhid: USB HID core driver
xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 1
xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 2
xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, 
bcdDevice= 6.00
usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb1: Product: xHCI Host Controller
usb usb1: Manufacturer: Linux 6.0.0-rc6-edison-acpi-standard xhci-hcd
usb usb1: SerialNumber: xhci-hcd.1.auto
hub 1-0:1.0: USB hub found
usb usb2: We don't know the algorithms for LPM for this host, disabling LPM.
usb usb2: New USB device found, idVendor=1d6b, idProduct=0003, 
bcdDevice= 6.00
usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb2: Product: xHCI Host Controller
usb usb2: Manufacturer: Linux 6.0.0-rc6-edison-acpi-standard xhci-hcd
usb usb2: SerialNumber: xhci-hcd.1.auto
hub 2-0:1.0: USB hub found

So, extcon works, xhci host controller "works". The problem may be no 
ulpi (tusb1210). Checking on 6.0-rc6 with host mode not working:

root@yuna:~# cat /sys/bus/ulpi/devices/dwc3.0.auto.ulpi/uevent
DEVTYPE=ulpi_device
MODALIAS=ulpi:v0000p0000
root@yuna:~# cat 
/sys/bus/ulpi/devices/dwc3.0.auto.ulpi/waiting_for_supplier
0
And on 5.15 with host mode working:

root@edison:~# cat /sys/bus/ulpi/devices/dwc3.0.auto.ulpi/uevent
DEVTYPE=ulpi_device
DRIVER=tusb1210
MODALIAS=ulpi:v0451p1508
root@edison:~#  cat 
/sys/bus/ulpi/devices/dwc3.0.auto.ulpi/waiting_for_supplier
cat: /sys/bus/ulpi/devices/dwc3.0.auto.ulpi/waiting_for_supplier: No 
such file or directory

Ulpi is there but is waiting for driver.

>>>>    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?
>>> It is not always possible to have an extcon driver available, that's why in
>>> some cases the probe of it defers. I dunno how your patch supposed to work
>>> in that case.
>>>
>>>> Does tracing the "mrfld_bcove_pwrsrc" driver (the
>>>> excton provider in this case AFIACT) show anything interesting?
>>> I believe there is nothing interesting.
>>>

Ferry Toth Sept. 24, 2022, 9:29 p.m. UTC | #18

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
> <[e-mail]> 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 <[e-mail]> 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;

Ferry Toth Sept. 25, 2022, 7:21 p.m. UTC | #19

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
>> <[e-mail]> 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 <[e-mail]> 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?

Andrey Smirnov Sept. 26, 2022, 5:43 a.m. UTC | #20

On Sun, Sep 25, 2022 at 12:21 PM Ferry Toth <[e-mail]> wrote:
>
> 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
> >> <[e-mail]> 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 <[e-mail]> 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.

I think we'd want to figure out why the ordering is important if we
want to justify the above fix.

>
> 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?
>

This will need some extra code to fix the case Sven's patch was fixing

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc6&id=ab7aa2866d295438dc60522f85c5421c6b4f1507

IMHO instead of trying to rush something in it be prudent to revert my
patch _and_ address the fact that above patch was lost during the
merge (Andy's revert needs to be updated)

Andy Shevchenko Sept. 26, 2022, 10:19 a.m. UTC | #21

On Sun, Sep 25, 2022 at 10:43:07PM -0700, Andrey Smirnov wrote:
> On Sun, Sep 25, 2022 at 12:21 PM Ferry Toth <[e-mail]> wrote:

...

> I think we'd want to figure out why the ordering is important if we
> want to justify the above fix.

At least we all on the same page (I hope) on justification for reverts.

...

> IMHO instead of trying to rush something in it be prudent to revert my
> patch _and_ address the fact that above patch was lost during the
> merge (Andy's revert needs to be updated)

I'm not an expert in your fixes for DWC3, so please come up with
the solution sooner than later, otherwise I will try to get my
reverts into the final release, because they obviously fix the
regression.

Andy Shevchenko Sept. 26, 2022, 10:21 a.m. UTC | #22

On Sat, Sep 24, 2022 at 06:06:04PM +0200, Ferry Toth wrote:
> Op 24-09-2022 om 03:34 schreef Andrey Smirnov:

...

> Ulpi is there but is waiting for driver.

This rings a bell to the fw_devlink broken series [1].

[1]: https://lore.kernel.org/r/CAOesGMh5GHCONTQ9M1Ro7zW-hkL_1F7Xt=xRV0vYSfPY=[e-mail]

Sven Peter Sept. 26, 2022, 10:45 a.m. UTC | #23

On Sun, Sep 25, 2022, at 21:21, Ferry Toth wrote:
> 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
>>> <[e-mail]> 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 <[e-mail]> 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.

err5 is correct there, that failure path starts to clean up what dwc3_core_init did.

>
> Thoughts? Can we get something like this in quick or should we revert first?

I don't know anything about that platform and following this thread is a bit hard
for me since I lack context so this is a total guess: dwc3_core_init brings up the
PHYs and also soft resets the core. Could any of these two things interact with your
extcon and somehow break it?


Sven

Andrey Smirnov Sept. 26, 2022, 6:31 p.m. UTC | #24

On Mon, Sep 26, 2022 at 3:19 AM Andy Shevchenko
<[e-mail]> wrote:
>
> On Sun, Sep 25, 2022 at 10:43:07PM -0700, Andrey Smirnov wrote:
> > On Sun, Sep 25, 2022 at 12:21 PM Ferry Toth <[e-mail]> wrote:
>
> ...
>
> > I think we'd want to figure out why the ordering is important if we
> > want to justify the above fix.
>
> At least we all on the same page (I hope) on justification for reverts.

Yes, there's clearly a regression here (multiple ones, really).

>
> ...
>
> > IMHO instead of trying to rush something in it be prudent to revert my
> > patch _and_ address the fact that above patch was lost during the
> > merge (Andy's revert needs to be updated)
>
> I'm not an expert in your fixes for DWC3, so please come up with
> the solution sooner than later, otherwise I will try to get my
> reverts into the final release, because they obviously fix the
> regression.

You don't need to be an expert here. All that's required is that your
revert get the code to look like it looks in

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc6&id=ab7aa2866d295438dc60522f85c5421c6b4f1507

so the last hunk in your patch instead of looking like:

@@ -538,6 +584,10 @@ int dwc3_drd_init(struct dwc3 *dwc)
 {
        int ret, irq;

+       dwc->edev = dwc3_get_extcon(dwc);
+       if (IS_ERR(dwc->edev))
+               return PTR_ERR(dwc->edev);
+
        if (ROLE_SWITCH &&
            device_property_read_bool(dwc->dev, "usb-role-switch"))
                return dwc3_setup_role_switch(dwc);

should look like

@@ -538,6 +584,10 @@ int dwc3_drd_init(struct dwc3 *dwc)
 {
        int ret, irq;

        if (ROLE_SWITCH &&
            device_property_read_bool(dwc->dev, "usb-role-switch"))
                return dwc3_setup_role_switch(dwc);

+       dwc->edev = dwc3_get_extcon(dwc);
+       if (IS_ERR(dwc->edev))
+               return PTR_ERR(dwc->edev);
+

Can you update your series accordingly or do you need me to do that? I
won't have the cycles until the end of the week (Sat).


>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Andy Shevchenko Sept. 27, 2022, 12:21 p.m. UTC | #25

On Mon, Sep 26, 2022 at 11:31:58AM -0700, Andrey Smirnov wrote:
> On Mon, Sep 26, 2022 at 3:19 AM Andy Shevchenko
> <[e-mail]> wrote:
> > On Sun, Sep 25, 2022 at 10:43:07PM -0700, Andrey Smirnov wrote:
> > > On Sun, Sep 25, 2022 at 12:21 PM Ferry Toth <[e-mail]> wrote:

...

> > > IMHO instead of trying to rush something in it be prudent to revert my
> > > patch _and_ address the fact that above patch was lost during the
> > > merge (Andy's revert needs to be updated)
> >
> > I'm not an expert in your fixes for DWC3, so please come up with
> > the solution sooner than later, otherwise I will try to get my
> > reverts into the final release, because they obviously fix the
> > regression.
> 
> You don't need to be an expert here. All that's required is that your
> revert get the code to look like it looks in
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc6&id=ab7aa2866d295438dc60522f85c5421c6b4f1507
> 
> so the last hunk in your patch instead of looking like:
> 
> @@ -538,6 +584,10 @@ int dwc3_drd_init(struct dwc3 *dwc)
>  {
>         int ret, irq;
> 
> +       dwc->edev = dwc3_get_extcon(dwc);
> +       if (IS_ERR(dwc->edev))
> +               return PTR_ERR(dwc->edev);
> +
>         if (ROLE_SWITCH &&
>             device_property_read_bool(dwc->dev, "usb-role-switch"))
>                 return dwc3_setup_role_switch(dwc);
> 
> should look like
> 
> @@ -538,6 +584,10 @@ int dwc3_drd_init(struct dwc3 *dwc)
>  {
>         int ret, irq;
> 
>         if (ROLE_SWITCH &&
>             device_property_read_bool(dwc->dev, "usb-role-switch"))
>                 return dwc3_setup_role_switch(dwc);
> 
> +       dwc->edev = dwc3_get_extcon(dwc);
> +       if (IS_ERR(dwc->edev))
> +               return PTR_ERR(dwc->edev);
> +
> 
> Can you update your series accordingly or do you need me to do that? I
> won't have the cycles until the end of the week (Sat).

Thanks for elaboration. I will do it (hopefully today).

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1170b800acdc..39956846b16d 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -23,6 +23,7 @@ 
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/acpi.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/reset.h>
@@ -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)
 	}
 }

+static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
+{
+	struct device *dev = dwc->dev;
+	struct device_node *np_phy;
+	struct extcon_dev *edev = NULL;
+	const char *name;
+
+	if (device_property_read_bool(dev, "extcon"))
+		return extcon_get_edev_by_phandle(dev, 0);
+
+	/*
+	 * Device tree platforms should get extcon via phandle.
+	 * On ACPI platforms, we get the name from a device property.
+	 * This device property is for kernel internal use only and
+	 * is expected to be set by the glue code.
+	 */
+	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0) {
+		edev = extcon_get_extcon_dev(name);
+		if (!edev)
+			return ERR_PTR(-EPROBE_DEFER);
+
+		return edev;
+	}
+
+	/*
+	 * Try to get an extcon device from the USB PHY controller's "port"
+	 * node. Check if it has the "port" node first, to avoid printing the
+	 * error message from underlying code, as it's a valid case: extcon
+	 * device (and "port" node) may be missing in case of "usb-role-switch"
+	 * or OTG mode.
+	 */
+	np_phy = of_parse_phandle(dev->of_node, "phys", 0);
+	if (of_graph_is_present(np_phy)) {
+		struct device_node *np_conn;
+
+		np_conn = of_graph_get_remote_node(np_phy, -1, -1);
+		if (np_conn)
+			edev = extcon_find_edev_by_node(np_conn);
+		of_node_put(np_conn);
+	}
+	of_node_put(np_phy);
+
+	return edev;
+}
+
 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;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index b60b5f7b6dff..f277bebdaa09 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -8,7 +8,6 @@ 
  */

 #include <linux/extcon.h>
-#include <linux/of_graph.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
@@ -439,51 +438,6 @@  static int dwc3_drd_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }

-static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
-{
-	struct device *dev = dwc->dev;
-	struct device_node *np_phy;
-	struct extcon_dev *edev = NULL;
-	const char *name;
-
-	if (device_property_read_bool(dev, "extcon"))
-		return extcon_get_edev_by_phandle(dev, 0);
-
-	/*
-	 * Device tree platforms should get extcon via phandle.
-	 * On ACPI platforms, we get the name from a device property.
-	 * This device property is for kernel internal use only and
-	 * is expected to be set by the glue code.
-	 */
-	if (device_property_read_string(dev, "linux,extcon-name", &name) == 0) {
-		edev = extcon_get_extcon_dev(name);
-		if (!edev)
-			return ERR_PTR(-EPROBE_DEFER);
-
-		return edev;
-	}
-
-	/*
-	 * Try to get an extcon device from the USB PHY controller's "port"
-	 * node. Check if it has the "port" node first, to avoid printing the
-	 * error message from underlying code, as it's a valid case: extcon
-	 * device (and "port" node) may be missing in case of "usb-role-switch"
-	 * or OTG mode.
-	 */
-	np_phy = of_parse_phandle(dev->of_node, "phys", 0);
-	if (of_graph_is_present(np_phy)) {
-		struct device_node *np_conn;
-
-		np_conn = of_graph_get_remote_node(np_phy, -1, -1);
-		if (np_conn)
-			edev = extcon_find_edev_by_node(np_conn);
-		of_node_put(np_conn);
-	}
-	of_node_put(np_phy);
-
-	return edev;
-}
-
 #if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
 #define ROLE_SWITCH 1
 static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
@@ -584,10 +538,6 @@  int dwc3_drd_init(struct dwc3 *dwc)
 {
 	int ret, irq;

-	dwc->edev = dwc3_get_extcon(dwc);
-	if (IS_ERR(dwc->edev))
-		return PTR_ERR(dwc->edev);
-
 	if (ROLE_SWITCH &&
 	    device_property_read_bool(dwc->dev, "usb-role-switch")) {
 		ret = dwc3_setup_role_switch(dwc);