lkml.org 
[lkml]   [2022]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v4] usb: dwc3: Don't switch OTG -> peripheral if extcon is present
    From
    Hi

    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.

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

    \
     
     \ /
      Last update: 2022-09-23 22:10    [W:3.861 / U:0.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site