Message-ID: <f9e859f5-102f-c18a-ed83-ff83d223e410@gmail.com>
Date:   Sat, 24 Sep 2022 13:55:10 +0200
From:   Ferry Toth <fntoth@...il.com>
To:     Andrey Smirnov <andrew.smirnov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     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

Hi,

Op 24-09-2022 om 03:27 schreef Andrey Smirnov:
> 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.

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@...a:~# journalctl -k -b -0 | grep -i dwc

While with the 2 reverts (and host working):

root@...a:~# 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
>>
>>

Powered by blists - more mailing lists