On Sun, Sep 25, 2022 at 12:21 PM Ferry Toth <fntoth@xxxxxxxxx> 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@xxxxxxxxx> 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@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. > >>> > >> 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)
