usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD

In reviewing the previous patch, Thinh Nguyen pointed out that the DRD mode change sequence should be like the following when switching from host -> device according to the programming guide (for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

The current code does:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset (added in previous
patch)
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

So this patch adds the DCTL.CSftRst soft reset that was currently missing from the dwc3 mode switching.

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 John Stultz <[email protected]>

---
Feedback would be appreciated. I'm a little worried I should be conditionalizing the DCTL.CSftRst on DRD mode controllers, but I'm really not sure what the right thing to do is for non-DRD mode controllers.

---
 drivers/usb/dwc3/core.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/usb/dwc3/core.c +2 -0

index b6a6b90eb2d5..71f8b07ecb99 100644
@@ -40,6 +40,8 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 
+static int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 /**
  * dwc3_get_dr_mode - Validates and sets dr_mode
  * @dwc: pointer to our context structure

Quoted 54 lines · John Stultz

In reviewing the previous patch, Thinh Nguyen pointed out that the DRD mode change sequence should be like the following when switching from host -> device according to the programming guide (for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

The current code does:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset (added in previous
patch)
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

So this patch adds the DCTL.CSftRst soft reset that was currently missing from the dwc3 mode switching.

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 John Stultz <[email protected]>

---
Feedback would be appreciated. I'm a little worried I should be conditionalizing the DCTL.CSftRst on DRD mode controllers, but I'm really not sure what the right thing to do is for non-DRD mode controllers.

---
 drivers/usb/dwc3/core.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/usb/dwc3/core.c +2 -0

index b6a6b90eb2d5..71f8b07ecb99 100644
@@ -40,6 +40,8 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 
+static int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 /**
  * dwc3_get_dr_mode - Validates and sets dr_mode
  * @dwc: pointer to our context structure

drivers/usb/dwc3/core.c static void __dwc3_set_mode(struct work_struct *work) · line 182

Quoted 4 lines

drivers/usb/dwc3/core.c +1 -0

@@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
+	dwc3_core_soft_reset(dwc);

Hi John/Thinh/Felipe,

I actually added this change into my local branch, because we were seeing an issue when switching from host mode --> peripheral mode. What was happening was that the RXFIFO register did not update back to the expected value for peripheral mode by the time dwc3_gadget_init_out_endpoint() was executed. With the logic to calculate the EP max packet limit based on RXFIFO reg, this caused all EPs to be set with an EP max limit of 0.

With this change, it seemed to help with the above issue. However, can we consider moving the core soft reset outside the spinlock? At least with our PHY init routines, we have some msleep() calls for waiting for the PHYs to be ready, which will end up as a sleeping while atomic bug. (not sure if PHY init is required to be called in atomic context)

Thanks
Wesley Cheng

Quoted 70 lines · Wesley Cheng

Quoted 54 lines · John Stultz

In reviewing the previous patch, Thinh Nguyen pointed out that the DRD mode change sequence should be like the following when switching from host -> device according to the programming guide (for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

The current code does:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset (added in previous
patch)
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

So this patch adds the DCTL.CSftRst soft reset that was currently missing from the dwc3 mode switching.

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 John Stultz <[email protected]>

---
Feedback would be appreciated. I'm a little worried I should be conditionalizing the DCTL.CSftRst on DRD mode controllers, but I'm really not sure what the right thing to do is for non-DRD mode controllers.

---
 drivers/usb/dwc3/core.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/usb/dwc3/core.c +2 -0

index b6a6b90eb2d5..71f8b07ecb99 100644
@@ -40,6 +40,8 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 
+static int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 /**
  * dwc3_get_dr_mode - Validates and sets dr_mode
  * @dwc: pointer to our context structure

drivers/usb/dwc3/core.c static void __dwc3_set_mode(struct work_struct *work) · line 182

Quoted 4 lines

drivers/usb/dwc3/core.c +1 -0

@@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
+	dwc3_core_soft_reset(dwc);

Hi John/Thinh/Felipe,

I actually added this change into my local branch, because we were seeing an issue when switching from host mode --> peripheral mode. What was happening was that the RXFIFO register did not update back to the expected value for peripheral mode by the time dwc3_gadget_init_out_endpoint() was executed. With the logic to calculate the EP max packet limit based on RXFIFO reg, this caused all EPs to be set with an EP max limit of 0.

Quoted 8 lines

With this change, it seemed to help with the above issue. However, can we consider moving the core soft reset outside the spinlock? At least with our PHY init routines, we have some msleep() calls for waiting for the PHYs to be ready, which will end up as a sleeping while atomic bug. (not sure if PHY init is required to be called in atomic context)

Thanks
Wesley Cheng

Hi Wesley,

Thanks for letting us know the issue you're having also.

Yes, you need to wait a certain amount of time to synchronize with the PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and less for older versions). When removing the spinlock to use msleep(), just make sure that there's no race issue. BTW, how long does your setup need to msleep()?

Quoted 82 lines · Thinh Nguyen

Quoted 70 lines · Wesley Cheng

Quoted 54 lines · John Stultz

In reviewing the previous patch, Thinh Nguyen pointed out that the DRD mode change sequence should be like the following when switching from host -> device according to the programming guide (for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

The current code does:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset (added in previous
patch)
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

So this patch adds the DCTL.CSftRst soft reset that was currently missing from the dwc3 mode switching.

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 John Stultz <[email protected]>

---
Feedback would be appreciated. I'm a little worried I should be conditionalizing the DCTL.CSftRst on DRD mode controllers, but I'm really not sure what the right thing to do is for non-DRD mode controllers.

---
 drivers/usb/dwc3/core.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/usb/dwc3/core.c +2 -0

index b6a6b90eb2d5..71f8b07ecb99 100644
@@ -40,6 +40,8 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 
+static int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 /**
  * dwc3_get_dr_mode - Validates and sets dr_mode
  * @dwc: pointer to our context structure

drivers/usb/dwc3/core.c static void __dwc3_set_mode(struct work_struct *work) · line 182

Quoted 4 lines

drivers/usb/dwc3/core.c +1 -0

@@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
+	dwc3_core_soft_reset(dwc);

Hi John/Thinh/Felipe,

I actually added this change into my local branch, because we were seeing an issue when switching from host mode --> peripheral mode. What was happening was that the RXFIFO register did not update back to the expected value for peripheral mode by the time dwc3_gadget_init_out_endpoint() was executed. With the logic to calculate the EP max packet limit based on RXFIFO reg, this caused all EPs to be set with an EP max limit of 0.

Quoted 8 lines

With this change, it seemed to help with the above issue. However, can we consider moving the core soft reset outside the spinlock? At least with our PHY init routines, we have some msleep() calls for waiting for the PHYs to be ready, which will end up as a sleeping while atomic bug. (not sure if PHY init is required to be called in atomic context)

Thanks
Wesley Cheng

Hi Wesley,

Quoted 8 lines

Thanks for letting us know the issue you're having also.

Yes, you need to wait a certain amount of time to synchronize with the PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and less for older versions). When removing the spinlock to use msleep(), just make sure that there's no race issue. BTW, how long does your setup need to msleep()?

Hi Thinh,

Sorry for the late response. My mistake, its actually just a usleep() for a less than 100uS (polling for a status bit change, so it will exit early if possible). For this change, can we just move the dwc3_core_soft_reset() outside of the spinlock?

Thanks
Wesley Cheng

Quoted 95 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 70 lines · Wesley Cheng

Quoted 54 lines · John Stultz

In reviewing the previous patch, Thinh Nguyen pointed out that the DRD mode change sequence should be like the following when switching from host -> device according to the programming guide (for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

The current code does:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset (added in previous
patch)
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

So this patch adds the DCTL.CSftRst soft reset that was currently missing from the dwc3 mode switching.

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 John Stultz <[email protected]>

---
Feedback would be appreciated. I'm a little worried I should be conditionalizing the DCTL.CSftRst on DRD mode controllers, but I'm really not sure what the right thing to do is for non-DRD mode controllers.

---
 drivers/usb/dwc3/core.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/usb/dwc3/core.c +2 -0

index b6a6b90eb2d5..71f8b07ecb99 100644
@@ -40,6 +40,8 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 
+static int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 /**
  * dwc3_get_dr_mode - Validates and sets dr_mode
  * @dwc: pointer to our context structure

drivers/usb/dwc3/core.c static void __dwc3_set_mode(struct work_struct *work) · line 182

Quoted 4 lines

drivers/usb/dwc3/core.c +1 -0

@@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
+	dwc3_core_soft_reset(dwc);

Hi John/Thinh/Felipe,

I actually added this change into my local branch, because we were seeing an issue when switching from host mode --> peripheral mode. What was happening was that the RXFIFO register did not update back to the expected value for peripheral mode by the time dwc3_gadget_init_out_endpoint() was executed. With the logic to calculate the EP max packet limit based on RXFIFO reg, this caused all EPs to be set with an EP max limit of 0.

Quoted 8 lines

With this change, it seemed to help with the above issue. However, can we consider moving the core soft reset outside the spinlock? At least with our PHY init routines, we have some msleep() calls for waiting for the PHYs to be ready, which will end up as a sleeping while atomic bug. (not sure if PHY init is required to be called in atomic context)

Thanks
Wesley Cheng

Hi Wesley,

Quoted 8 lines

Thanks for letting us know the issue you're having also.

Yes, you need to wait a certain amount of time to synchronize with the PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and less for older versions). When removing the spinlock to use msleep(), just make sure that there's no race issue. BTW, how long does your setup need to msleep()?

Hi Thinh,

Quoted 8 lines

Sorry for the late response. My mistake, its actually just a usleep() for a less than 100uS (polling for a status bit change, so it will exit early if possible). For this change, can we just move the dwc3_core_soft_reset() outside of the spinlock?

Thanks
Wesley Cheng

Hi Wesley,

dwc3 can get notified at any time to queue a work to switch mode. So you need protect it from a potential race. I think you can use a mutex for this.

Also, what status are you polling? Note that there's no status bit for GCTL.coresoftreset. For DCTL.CSFTRST, different controller versions behave differently. Use dwc3_core_soft_reset() for DCTL.CSFTRST to get the logic from there.

1 more thing, make sure that this flow only applies for DRD mode controller and not OTG from older DWC_usb3 IP.

Thanks,
Thinh

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

Hi,

Quoted 45 lines · John Stultz

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

Quoted 3 lines

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.

Quoted 49 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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

Hi,

Quoted 61 lines · John Stultz

Quoted 49 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 75 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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. :)

Quoted 3 lines

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?

Quoted 77 lines · Wesley Cheng

Quoted 75 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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. :)

Quoted 9 lines

Quoted 3 lines

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.

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 70 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 8 lines

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,

Quoted 9 lines

Quoted 4 lines

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

Quoted 95 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 70 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 8 lines

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,

Quoted 9 lines

Quoted 4 lines

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,

Quoted 9 lines

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

Quoted 106 lines · Thinh Nguyen

Quoted 95 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 70 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 8 lines

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,

Quoted 9 lines

Quoted 4 lines

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,

Quoted 9 lines

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

Quoted 9 lines

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

Quoted 118 lines · Wesley Cheng

Quoted 106 lines · Thinh Nguyen

Quoted 95 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 70 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 8 lines

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,

Quoted 9 lines

Quoted 4 lines

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,

Quoted 9 lines

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

Quoted 9 lines

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

Quoted 7 lines

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

Quoted 130 lines · Wesley Cheng

Quoted 118 lines · Wesley Cheng

Quoted 106 lines · Thinh Nguyen

Quoted 95 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 70 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 8 lines

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,

Quoted 9 lines

Quoted 4 lines

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,

Quoted 9 lines

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

Quoted 9 lines

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

Quoted 7 lines

Hi Thinh,

Sure, my DWC3_VER_TYPE output = 0x67612A2A

Thanks
Wesley Cheng

Hi Thinh,

Quoted 8 lines

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

Quoted 148 lines · Thinh Nguyen

Quoted 130 lines · Wesley Cheng

Quoted 118 lines · Wesley Cheng

Quoted 106 lines · Thinh Nguyen

Quoted 95 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 70 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 8 lines

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,

Quoted 9 lines

Quoted 4 lines

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,

Quoted 9 lines

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

Quoted 9 lines

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

Quoted 7 lines

Hi Thinh,

Sure, my DWC3_VER_TYPE output = 0x67612A2A

Thanks
Wesley Cheng

Hi Thinh,

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 159 lines · Wesley Cheng

Quoted 148 lines · Thinh Nguyen

Quoted 130 lines · Wesley Cheng

Quoted 118 lines · Wesley Cheng

Quoted 106 lines · Thinh Nguyen

Quoted 95 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 70 lines · Thinh Nguyen

Hi,

Quoted 60 lines · John Stultz

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 45 lines · John Stultz

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

Quoted 6 lines

Quoted 3 lines

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$

Quoted 7 lines

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

Quoted 8 lines

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,

Quoted 9 lines

Quoted 4 lines

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,

Quoted 9 lines

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

Quoted 9 lines

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

Quoted 7 lines

Hi Thinh,

Sure, my DWC3_VER_TYPE output = 0x67612A2A

Thanks
Wesley Cheng

Hi Thinh,

Quoted 8 lines

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.

Quoted 7 lines

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

Quoted 7 lines

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