lkml.org 
[lkml]   [2022]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4] usb: dwc3: Don't switch OTG -> peripheral if extcon is present
On Sun, Sep 25, 2022 at 12:21 PM Ferry Toth <fntoth@gmail.com> 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
> >> <andriy.shevchenko@intel.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@gmail.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
> >>
> >>>>> 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)

\
 
 \ /
  Last update: 2022-09-26 07:43    [W:0.093 / U:0.684 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site