Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD

[Thinh Nguyen]

Wesley Cheng wrote:
>
> On 3/6/2021 3:41 PM, Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>> On 1/8/2021 4:44 PM, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> John Stultz wrote:
>>>>> On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> John Stultz <john.stultz@xxxxxxxxxx> writes:
>>>>>>> From: Yu Chen <chenyu56@xxxxxxxxxx>
>>>>>>>
>>>>>>> Just resending this, as discussion died out a bit and I'm not
>>>>>>> sure how to make further progress. See here for debug data that
>>>>>>> was requested last time around:
>>>>>>>   https://urldefense.com/v3/__https://lore.kernel.org/lkml/CALAqxLXdnaUfJKx0aN9xWwtfWVjMWigPpy2aqsNj56yvnbU80g@xxxxxxxxxxxxxx/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnsnlqd3w$ 
>>>>>>>
>>>>>>> With the current dwc3 code on the HiKey960 we often see the
>>>>>>> COREIDLE flag get stuck off in __dwc3_gadget_start(), which
>>>>>>> seems to prevent the reset irq and causes the USB gadget to
>>>>>>> fail to initialize.
>>>>>>>
>>>>>>> We had seen occasional initialization failures with older
>>>>>>> kernels but with recent 5.x era kernels it seemed to be becoming
>>>>>>> much more common, so I dug back through some older trees and
>>>>>>> realized I dropped this quirk from Yu Chen during upstreaming
>>>>>>> as I couldn't provide a proper rational for it and it didn't
>>>>>>> seem to be necessary. I now realize I was wrong.
>>>>>>>
>>>>>>> After resubmitting the quirk, Thinh Nguyen pointed out that it
>>>>>>> shouldn't be a quirk at all and it is actually mentioned in the
>>>>>>> programming guide that it should be done when switching modes
>>>>>>> in DRD.
>>>>>>>
>>>>>>> So, to avoid these !COREIDLE lockups seen on HiKey960, this
>>>>>>> patch issues GCTL soft reset when switching modes if the
>>>>>>> controller is in DRD mode.
>>>>>>>
>>>>>>> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
>>>>>>> Cc: Tejas Joglekar <tejas.joglekar@xxxxxxxxxxxx>
>>>>>>> Cc: Yang Fei <fei.yang@xxxxxxxxx>
>>>>>>> Cc: YongQin Liu <yongqin.liu@xxxxxxxxxx>
>>>>>>> Cc: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
>>>>>>> Cc: Thinh Nguyen <thinhn@xxxxxxxxxxxx>
>>>>>>> Cc: Jun Li <lijun.kernel@xxxxxxxxx>
>>>>>>> Cc: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
>>>>>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>>>>> Cc: linux-usb@xxxxxxxxxxxxxxx
>>>>>>> Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx>
>>>>>>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> * Rework to always call the GCTL soft reset in DRD mode,
>>>>>>>   rather then using a quirk as suggested by Thinh Nguyen
>>>>>>>
>>>>>>> v3:
>>>>>>> * Move GCTL soft reset under the spinlock as suggested by
>>>>>>>   Thinh Nguyen
>>>>>> Because this is such an invasive change, I would prefer that we get
>>>>>> Tested-By tags from a good fraction of the users before applying these
>>>>>> two changes.
>>>>> I'm happy to reach out to folks to try to get that. Though I'm
>>>>> wondering if it would be better to put it behind a dts quirk flag, as
>>>>> originally proposed?
>>>>>    https://urldefense.com/v3/__https://lore.kernel.org/lkml/20201021181803.79650-1-john.stultz@xxxxxxxxxx/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnRWITZfc$ 
>>>>>
>>>>> That way folks can enable it for devices as they need?
>>>>>
>>>>> Again, I'm not trying to force this in as-is, just mostly sending it
>>>>> out again for discussion to understand what other approach might work.
>>>>>
>>>>> thanks
>>>>> -john
>>>> A quirk would imply something is broken/diverged from the design right?
>>>> But it's not the case here, and at least this is needed for HiKey960.
>>>> Also, I think Rob will be ok with not adding 1 more quirk to the dwc3
>>>> devicetree. :)
>>>>
>>>> BR,
>>>> Thinh
>>>>
>>> Hi All,
>>>
>>> Sorry for jumping in, but I checked the SNPS v1.90a databook, and that
>>> seemed to remove the requirement for the GCTL.softreset before writing
>>> to PRTCAPDIR.  Should we consider adding a controller version/IP check?
>>>
>> Hi Wesley,
>>
>> From what I see in the v1.90a databook and others, the flow remains the
>> same. I need to check internally, but I'm not aware of the change.
>>
> Hi Thinh,
>
> Hmmm, can you help check the register description for the PRTCAPDIR on
> your v1.90a databook?  (Table 1-19 Fields for Register: GCTL (Continued)
> Pg73)  When we compared the sequence in the description there to the
> previous versions it removed the GCTL.softreset.  If it still shows up
> on yours, then maybe my v1.90a isn't the final version?
>
> Thanks
> Wesley Cheng
>

Hi Wesley,

Actually your IP version type may use the newer flow. Can you print your
DWC3_VER_TYPE? I still need to verify internally to know which versions
need the update if any.

Thanks,
Thinh