From: Yu Chen <[email protected]>
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://lore.kernel.org/lkml/CALAqxLXdnaUfJKx0aN9xWwtfWVjMWigPpy2aqsNj56yvnbU80g@mail.gmail.com/
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 <[email protected]>
Cc: Tejas Joglekar <[email protected]>
Cc: Yang Fei <[email protected]>
Cc: YongQin Liu <[email protected]>
Cc: Andrzej Pietrasiewicz <[email protected]>
Cc: Thinh Nguyen <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Signed-off-by: Yu Chen <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
---
drivers/usb/dwc3/core.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 841daec70b6e..b6a6b90eb2d5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -114,10 +114,24 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
dwc->current_dr_role = mode;
}
+static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
+{
+ int reg;
+
+ reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+ reg |= (DWC3_GCTL_CORESOFTRESET);
+ dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+ reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+ reg &= ~(DWC3_GCTL_CORESOFTRESET);
+ dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+}
+
static void __dwc3_set_mode(struct work_struct *work)
{
struct dwc3 *dwc = work_to_dwc(work);
unsigned long flags;
+ int hw_mode;
int ret;
u32 reg;
@@ -156,6 +170,11 @@ static void __dwc3_set_mode(struct work_struct *work)
spin_lock_irqsave(&dwc->lock, flags);
+ /* Execute a GCTL Core Soft Reset when switch mode in DRD*/
+ hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+ if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
+ dwc3_gctl_core_soft_reset(dwc);
+
dwc3_set_prtcap(dwc, dwc->desired_dr_role);
spin_unlock_irqrestore(&dwc->lock, flags);
--
2.17.1
Hi,
John Stultz <[email protected]> writes:
> From: Yu Chen <[email protected]>
>
> 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://lore.kernel.org/lkml/CALAqxLXdnaUfJKx0aN9xWwtfWVjMWigPpy2aqsNj56yvnbU80g@mail.gmail.com/
>
> 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 <[email protected]>
> Cc: Tejas Joglekar <[email protected]>
> Cc: Yang Fei <[email protected]>
> Cc: YongQin Liu <[email protected]>
> Cc: Andrzej Pietrasiewicz <[email protected]>
> Cc: Thinh Nguyen <[email protected]>
> Cc: Jun Li <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Signed-off-by: Yu Chen <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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.
--
balbi
On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi <[email protected]> wrote:
>
>
> Hi,
>
> John Stultz <[email protected]> writes:
> > From: Yu Chen <[email protected]>
> >
> > 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://lore.kernel.org/lkml/CALAqxLXdnaUfJKx0aN9xWwtfWVjMWigPpy2aqsNj56yvnbU80g@mail.gmail.com/
> >
> > 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 <[email protected]>
> > Cc: Tejas Joglekar <[email protected]>
> > Cc: Yang Fei <[email protected]>
> > Cc: YongQin Liu <[email protected]>
> > Cc: Andrzej Pietrasiewicz <[email protected]>
> > Cc: Thinh Nguyen <[email protected]>
> > Cc: Jun Li <[email protected]>
> > Cc: Mauro Carvalho Chehab <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Yu Chen <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > 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://lore.kernel.org/lkml/[email protected]/
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
Hi,
John Stultz wrote:
> On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi <[email protected]> wrote:
>>
>> Hi,
>>
>> John Stultz <[email protected]> writes:
>>> From: Yu Chen <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Cc: Tejas Joglekar <[email protected]>
>>> Cc: Yang Fei <[email protected]>
>>> Cc: YongQin Liu <[email protected]>
>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>> Cc: Thinh Nguyen <[email protected]>
>>> Cc: Jun Li <[email protected]>
>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>> Cc: Greg Kroah-Hartman <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Yu Chen <[email protected]>
>>> Signed-off-by: John Stultz <[email protected]>
>>> ---
>>> 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/[email protected]/__;!!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
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 <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> John Stultz <[email protected]> writes:
>>>> From: Yu Chen <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Cc: Tejas Joglekar <[email protected]>
>>>> Cc: Yang Fei <[email protected]>
>>>> Cc: YongQin Liu <[email protected]>
>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>> Cc: Thinh Nguyen <[email protected]>
>>>> Cc: Jun Li <[email protected]>
>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>> Cc: [email protected]
>>>> Signed-off-by: Yu Chen <[email protected]>
>>>> Signed-off-by: John Stultz <[email protected]>
>>>> ---
>>>> 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/[email protected]/__;!!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
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 <[email protected]> wrote:
>>>> Hi,
>>>>
>>>> John Stultz <[email protected]> writes:
>>>>> From: Yu Chen <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>> Cc: Yang Fei <[email protected]>
>>>>> Cc: YongQin Liu <[email protected]>
>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>> Cc: Jun Li <[email protected]>
>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>> ---
>>>>> 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/[email protected]/__;!!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.
BR,
Thinh
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 <[email protected]> wrote:
>>>>> Hi,
>>>>>
>>>>> John Stultz <[email protected]> writes:
>>>>>> From: Yu Chen <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>>> Cc: Yang Fei <[email protected]>
>>>>>> Cc: YongQin Liu <[email protected]>
>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>>> Cc: Jun Li <[email protected]>
>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>>> Cc: [email protected]
>>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>>> ---
>>>>>> 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/[email protected]/__;!!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
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> John Stultz <[email protected]> writes:
>>>>>>> From: Yu Chen <[email protected]>
>>>>>>>
>>>>>>> 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 <[email protected]>
>>>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>>>> Cc: Yang Fei <[email protected]>
>>>>>>> Cc: YongQin Liu <[email protected]>
>>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>>>> Cc: Jun Li <[email protected]>
>>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>>>> Cc: [email protected]
>>>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>>>> ---
>>>>>>> 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/[email protected]/__;!!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
On 3/8/2021 7:05 PM, Thinh Nguyen wrote:
> 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 <[email protected]> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> John Stultz <[email protected]> writes:
>>>>>>>> From: Yu Chen <[email protected]>
>>>>>>>>
>>>>>>>> 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 <[email protected]>
>>>>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>>>>> Cc: Yang Fei <[email protected]>
>>>>>>>> Cc: YongQin Liu <[email protected]>
>>>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>>>>> Cc: Jun Li <[email protected]>
>>>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>>>>> Cc: [email protected]
>>>>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>>>>> ---
>>>>>>>> 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/[email protected]/__;!!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
>
Hi Thinh,
Sure, my DWC3_VER_TYPE output = 0x67612A2A
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 3/8/2021 10:33 PM, Wesley Cheng wrote:
>
>
> On 3/8/2021 7:05 PM, Thinh Nguyen wrote:
>> 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 <[email protected]> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> John Stultz <[email protected]> writes:
>>>>>>>>> From: Yu Chen <[email protected]>
>>>>>>>>>
>>>>>>>>> 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 <[email protected]>
>>>>>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>>>>>> Cc: Yang Fei <[email protected]>
>>>>>>>>> Cc: YongQin Liu <[email protected]>
>>>>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>>>>>> Cc: Jun Li <[email protected]>
>>>>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>>>>>> Cc: [email protected]
>>>>>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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/[email protected]/__;!!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
>>
> Hi Thinh,
>
> Sure, my DWC3_VER_TYPE output = 0x67612A2A
>
> Thanks
> Wesley Cheng
>
Hi Thinh,
Would you happen to have an update on the required sequence on the
version shared? Sorry for pushing, but we just wanted to finalize on
it, since it does cause some functional issues w/o the soft reset in
place, and causes a crash if we have the GCTL.softreset.
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Wesley Cheng wrote:
>
>
> On 3/8/2021 10:33 PM, Wesley Cheng wrote:
>>
>>
>> On 3/8/2021 7:05 PM, Thinh Nguyen wrote:
>>> 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 <[email protected]> wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> John Stultz <[email protected]> writes:
>>>>>>>>>> From: Yu Chen <[email protected]>
>>>>>>>>>>
>>>>>>>>>> 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 <[email protected]>
>>>>>>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>>>>>>> Cc: Yang Fei <[email protected]>
>>>>>>>>>> Cc: YongQin Liu <[email protected]>
>>>>>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>>>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>>>>>>> Cc: Jun Li <[email protected]>
>>>>>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>>>>>>> Cc: [email protected]
>>>>>>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>>>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> 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/[email protected]/__;!!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
>>>
>> Hi Thinh,
>>
>> Sure, my DWC3_VER_TYPE output = 0x67612A2A
>>
>> Thanks
>> Wesley Cheng
>>
> Hi Thinh,
>
> Would you happen to have an update on the required sequence on the
> version shared? Sorry for pushing, but we just wanted to finalize on
> it, since it does cause some functional issues w/o the soft reset in
> place, and causes a crash if we have the GCTL.softreset.
>
> Thanks
> Wesley Cheng
>
Hi Wesley,
I'm still trying to get that info for you. The versions without
GCTL.softreset should be very new. The flow with GCTL.softreset should
work for all versions and should not cause functional impact. We can
create a change to optimize and remove GCTL.softreset for the newer
controller versions at a later time.
Since you and John Stultz have the setup that can be verified in the
real world. It would be great if John or you provide a tested patch(es)
to resolve this issue.
Thanks,
Thinh
On 3/19/2021 4:09 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>>
>> On 3/8/2021 10:33 PM, Wesley Cheng wrote:
>>>
>>>
>>> On 3/8/2021 7:05 PM, Thinh Nguyen wrote:
>>>> 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 <[email protected]> wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> John Stultz <[email protected]> writes:
>>>>>>>>>>> From: Yu Chen <[email protected]>
>>>>>>>>>>>
>>>>>>>>>>> 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 <[email protected]>
>>>>>>>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>>>>>>>> Cc: Yang Fei <[email protected]>
>>>>>>>>>>> Cc: YongQin Liu <[email protected]>
>>>>>>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>>>>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>>>>>>>> Cc: Jun Li <[email protected]>
>>>>>>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>>>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>>>>>>>> Cc: [email protected]
>>>>>>>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>>>>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>>>>>>>> ---
>>>>>>>>>>> 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/[email protected]/__;!!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
>>>>
>>> Hi Thinh,
>>>
>>> Sure, my DWC3_VER_TYPE output = 0x67612A2A
>>>
>>> Thanks
>>> Wesley Cheng
>>>
>> Hi Thinh,
>>
>> Would you happen to have an update on the required sequence on the
>> version shared? Sorry for pushing, but we just wanted to finalize on
>> it, since it does cause some functional issues w/o the soft reset in
>> place, and causes a crash if we have the GCTL.softreset.
>>
>> Thanks
>> Wesley Cheng
>>
>
> Hi Wesley,
>
> I'm still trying to get that info for you. The versions without
> GCTL.softreset should be very new. The flow with GCTL.softreset should
> work for all versions and should not cause functional impact. We can
> create a change to optimize and remove GCTL.softreset for the newer
> controller versions at a later time.
>
> Since you and John Stultz have the setup that can be verified in the
> real world. It would be great if John or you provide a tested patch(es)
> to resolve this issue.
>
> Thanks,
> Thinh
>
Hi Thinh,
Thanks for the input as always. I tested the GCTL.softreset change just
now, and it is working fine at least on my set up.
Not sure if we'd need input from other vendors as well to get this
change merged.
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Mon, Mar 29, 2021 at 3:15 PM Wesley Cheng <[email protected]> wrote:
>
>
>
> On 3/19/2021 4:09 PM, Thinh Nguyen wrote:
> > Wesley Cheng wrote:
> >>
> >>
> >> On 3/8/2021 10:33 PM, Wesley Cheng wrote:
> >>>
> >>>
> >>> On 3/8/2021 7:05 PM, Thinh Nguyen wrote:
> >>>> 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 <[email protected]> wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> John Stultz <[email protected]> writes:
> >>>>>>>>>>> From: Yu Chen <[email protected]>
> >>>>>>>>>>>
> >>>>>>>>>>> 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 <[email protected]>
> >>>>>>>>>>> Cc: Tejas Joglekar <[email protected]>
> >>>>>>>>>>> Cc: Yang Fei <[email protected]>
> >>>>>>>>>>> Cc: YongQin Liu <[email protected]>
> >>>>>>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
> >>>>>>>>>>> Cc: Thinh Nguyen <[email protected]>
> >>>>>>>>>>> Cc: Jun Li <[email protected]>
> >>>>>>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
> >>>>>>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
> >>>>>>>>>>> Cc: [email protected]
> >>>>>>>>>>> Signed-off-by: Yu Chen <[email protected]>
> >>>>>>>>>>> Signed-off-by: John Stultz <[email protected]>
> >>>>>>>>>>> ---
> >>>>>>>>>>> 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/[email protected]/__;!!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
> >>>>
> >>> Hi Thinh,
> >>>
> >>> Sure, my DWC3_VER_TYPE output = 0x67612A2A
> >>>
> >>> Thanks
> >>> Wesley Cheng
> >>>
> >> Hi Thinh,
> >>
> >> Would you happen to have an update on the required sequence on the
> >> version shared? Sorry for pushing, but we just wanted to finalize on
> >> it, since it does cause some functional issues w/o the soft reset in
> >> place, and causes a crash if we have the GCTL.softreset.
> >>
> >> Thanks
> >> Wesley Cheng
> >>
> >
> > Hi Wesley,
> >
> > I'm still trying to get that info for you. The versions without
> > GCTL.softreset should be very new. The flow with GCTL.softreset should
> > work for all versions and should not cause functional impact. We can
> > create a change to optimize and remove GCTL.softreset for the newer
> > controller versions at a later time.
> >
> > Since you and John Stultz have the setup that can be verified in the
> > real world. It would be great if John or you provide a tested patch(es)
> > to resolve this issue.
> >
> > Thanks,
> > Thinh
> >
> Hi Thinh,
>
> Thanks for the input as always. I tested the GCTL.softreset change just
> now, and it is working fine at least on my set up.
>
> Not sure if we'd need input from other vendors as well to get this
> change merged.
Did you test the original patch from this thread unchanged, or did you
have any modifications? If the latter, feel free to send it out and
I'll validate it on my side.
thanks
-john
On 3/29/2021 3:20 PM, John Stultz wrote:
> On Mon, Mar 29, 2021 at 3:15 PM Wesley Cheng <[email protected]> wrote:
>>
>>
>>
>> On 3/19/2021 4:09 PM, Thinh Nguyen wrote:
>>> Wesley Cheng wrote:
>>>>
>>>>
>>>> On 3/8/2021 10:33 PM, Wesley Cheng wrote:
>>>>>
>>>>>
>>>>> On 3/8/2021 7:05 PM, Thinh Nguyen wrote:
>>>>>> 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 <[email protected]> wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> John Stultz <[email protected]> writes:
>>>>>>>>>>>>> From: Yu Chen <[email protected]>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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 <[email protected]>
>>>>>>>>>>>>> Cc: Tejas Joglekar <[email protected]>
>>>>>>>>>>>>> Cc: Yang Fei <[email protected]>
>>>>>>>>>>>>> Cc: YongQin Liu <[email protected]>
>>>>>>>>>>>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>>>>>>>>>>>> Cc: Thinh Nguyen <[email protected]>
>>>>>>>>>>>>> Cc: Jun Li <[email protected]>
>>>>>>>>>>>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>>>>>>>>>>>> Cc: Greg Kroah-Hartman <[email protected]>
>>>>>>>>>>>>> Cc: [email protected]
>>>>>>>>>>>>> Signed-off-by: Yu Chen <[email protected]>
>>>>>>>>>>>>> Signed-off-by: John Stultz <[email protected]>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> 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/[email protected]/__;!!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
>>>>>>
>>>>> Hi Thinh,
>>>>>
>>>>> Sure, my DWC3_VER_TYPE output = 0x67612A2A
>>>>>
>>>>> Thanks
>>>>> Wesley Cheng
>>>>>
>>>> Hi Thinh,
>>>>
>>>> Would you happen to have an update on the required sequence on the
>>>> version shared? Sorry for pushing, but we just wanted to finalize on
>>>> it, since it does cause some functional issues w/o the soft reset in
>>>> place, and causes a crash if we have the GCTL.softreset.
>>>>
>>>> Thanks
>>>> Wesley Cheng
>>>>
>>>
>>> Hi Wesley,
>>>
>>> I'm still trying to get that info for you. The versions without
>>> GCTL.softreset should be very new. The flow with GCTL.softreset should
>>> work for all versions and should not cause functional impact. We can
>>> create a change to optimize and remove GCTL.softreset for the newer
>>> controller versions at a later time.
>>>
>>> Since you and John Stultz have the setup that can be verified in the
>>> real world. It would be great if John or you provide a tested patch(es)
>>> to resolve this issue.
>>>
>>> Thanks,
>>> Thinh
>>>
>> Hi Thinh,
>>
>> Thanks for the input as always. I tested the GCTL.softreset change just
>> now, and it is working fine at least on my set up.
>>
>> Not sure if we'd need input from other vendors as well to get this
>> change merged.
>
> Did you test the original patch from this thread unchanged, or did you
> have any modifications? If the latter, feel free to send it out and
> I'll validate it on my side.
>
> thanks
> -john
>
Hi John,
I'm using your change as is. Didn't make any modifications to it.
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project