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

[Ferry Toth]

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 <fntoth@xxxxxxxxx> wrote:
> > > On 22-09-2022 12:08, Andy Shevchenko wrote:
> > > On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
> FYI: For now I sent a revert, but if we got a solution quicker we always
> can choose the course of actions.
>
> > > 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.