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
