Message-ID: <CAHQ1cqG8KfZAFwvjkn1f+aKrOkPNKW=NJo-rz-Zqp+WFZLoF4g@mail.gmail.com>
Date:   Fri, 23 Sep 2022 18:27:18 -0700
From:   Andrey Smirnov <andrew.smirnov@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     Ferry Toth <fntoth@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>,
        Thinh Nguyen <thinhn@...opsys.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Sven Peter <sven@...npeter.dev>
Subject: Re: [PATCH v4] usb: dwc3: Don't switch OTG -> peripheral if extcon is present

On Fri, Sep 23, 2022 at 11:54 AM Andy Shevchenko
<andriy.shevchenko@...el.com> 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
> > <andriy.shevchenko@...el.com> wrote:
> > > On Thu, Sep 22, 2022 at 04:32:55PM -0700, Andrey Smirnov wrote:
> > > > On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <fntoth@...il.com> 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
>
>

Powered by blists - more mailing lists