Messages in this thread | | | Date | Sat, 24 Sep 2022 13:55:10 +0200 | Subject | Re: [PATCH v4] usb: dwc3: Don't switch OTG -> peripheral if extcon is present | From | Ferry Toth <> |
| |
Hi,
Op 24-09-2022 om 03:27 schreef Andrey Smirnov: > On Fri, Sep 23, 2022 at 11:54 AM Andy Shevchenko > <andriy.shevchenko@intel.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@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 >> 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 >> >>
| |