On 1/8/2021 4:44 PM, Thinh Nguyen wrote:

Hi,

John Stultz wrote:

quoted

On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi [off-list ref] wrote:

quoted

Hi,

John Stultz [off-list ref] writes:

quoted

From: Yu Chen <chenyu56@huawei.com>

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@mail.gmail.com/__;!!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@kernel.org>
Cc: Tejas Joglekar <redacted>
Cc: Yang Fei <redacted>
Cc: YongQin Liu <yongqin.liu@linaro.org>
Cc: Andrzej Pietrasiewicz <redacted>
Cc: Thinh Nguyen <redacted>
Cc: Jun Li <redacted>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <redacted>
---
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@linaro.org/__;!!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?

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project