* [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET during bootup
@ 2022-07-12 17:44 Rohith Kollalsi
2022-07-14 1:06 ` Thinh Nguyen
0 siblings, 1 reply; 3+ messages in thread
From: Rohith Kollalsi @ 2022-07-12 17:44 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Philipp Zabel
Cc: linux-usb, linux-kernel, Rohith Kollalsi
According to the programming guide, it is recommended to
perform a GCTL_CORE_SOFTRESET only when switching the mode
from device to host or host to device. However, it is found
that during bootup when __dwc3_set_mode() is called for the
first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17)
of DWC3_GUSB3PIPECTL set. This some times leads to issues
like controller going into bad state and controller registers
reading value zero. Until GCTL_CORESOFTRESET is done and
run/stop bit is set core initialization is not complete.
Setting suspendable bit of DWC3_GUSB3PIPECTL and then
performing GCTL_CORESOFTRESET is therefore not recommended.
Avoid this by only performing the reset if current_dr_role is set,
that is, when doing subsequent role switching.
Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode")
Signed-off-by: Rohith Kollalsi <quic_rkollals@quicinc.com>
---
drivers/usb/dwc3/core.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 050b2ba5986d..7ce0e033d755 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work)
break;
}
- /* For DRD host or device mode only */
- if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
- dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
+ /*
+ * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET.
+ * Perform it only for DRD host or device mode.
+ */
+ if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
+ DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
+ dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
reg = dwc3_readl(dwc->regs, DWC3_GCTL);
reg |= DWC3_GCTL_CORESOFTRESET;
dwc3_writel(dwc->regs, DWC3_GCTL, reg);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET during bootup
2022-07-12 17:44 [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET during bootup Rohith Kollalsi
@ 2022-07-14 1:06 ` Thinh Nguyen
2022-07-14 11:14 ` Rohith Kollalsi
0 siblings, 1 reply; 3+ messages in thread
From: Thinh Nguyen @ 2022-07-14 1:06 UTC (permalink / raw)
To: Rohith Kollalsi, Felipe Balbi, Greg Kroah-Hartman, Philipp Zabel
Cc: linux-usb, linux-kernel
On 7/12/2022, Rohith Kollalsi wrote:
> According to the programming guide, it is recommended to
> perform a GCTL_CORE_SOFTRESET only when switching the mode
> from device to host or host to device. However, it is found
> that during bootup when __dwc3_set_mode() is called for the
> first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17)
> of DWC3_GUSB3PIPECTL set. This some times leads to issues
> like controller going into bad state and controller registers
> reading value zero. Until GCTL_CORESOFTRESET is done and
> run/stop bit is set core initialization is not complete.
> Setting suspendable bit of DWC3_GUSB3PIPECTL and then
> performing GCTL_CORESOFTRESET is therefore not recommended.
> Avoid this by only performing the reset if current_dr_role is set,
> that is, when doing subsequent role switching.
>
> Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode")
> Signed-off-by: Rohith Kollalsi <quic_rkollals@quicinc.com>
> ---
> drivers/usb/dwc3/core.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 050b2ba5986d..7ce0e033d755 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work)
> break;
> }
>
> - /* For DRD host or device mode only */
> - if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
> - dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
> + /*
> + * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET.
The line above doesn't really explain why. Can you rephrase it to
something like this or the equivalent:
"When current_dr_role is not set, there's no role switching. Only
perform GCTL.CoreSoftReset when there's DRD role switching."
Thanks,
Thinh
> + * Perform it only for DRD host or device mode.
> + */
> + if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
> + DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
> + dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
> reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> reg |= DWC3_GCTL_CORESOFTRESET;
> dwc3_writel(dwc->regs, DWC3_GCTL, reg);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET during bootup
2022-07-14 1:06 ` Thinh Nguyen
@ 2022-07-14 11:14 ` Rohith Kollalsi
0 siblings, 0 replies; 3+ messages in thread
From: Rohith Kollalsi @ 2022-07-14 11:14 UTC (permalink / raw)
To: Thinh Nguyen, Felipe Balbi, Greg Kroah-Hartman, Philipp Zabel
Cc: linux-usb, linux-kernel
On 7/14/2022 6:36 AM, Thinh Nguyen wrote:
> On 7/12/2022, Rohith Kollalsi wrote:
>> According to the programming guide, it is recommended to
>> perform a GCTL_CORE_SOFTRESET only when switching the mode
>> from device to host or host to device. However, it is found
>> that during bootup when __dwc3_set_mode() is called for the
>> first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17)
>> of DWC3_GUSB3PIPECTL set. This some times leads to issues
>> like controller going into bad state and controller registers
>> reading value zero. Until GCTL_CORESOFTRESET is done and
>> run/stop bit is set core initialization is not complete.
>> Setting suspendable bit of DWC3_GUSB3PIPECTL and then
>> performing GCTL_CORESOFTRESET is therefore not recommended.
>> Avoid this by only performing the reset if current_dr_role is set,
>> that is, when doing subsequent role switching.
>>
>> Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode")
>> Signed-off-by: Rohith Kollalsi <quic_rkollals@quicinc.com>
>> ---
>> drivers/usb/dwc3/core.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 050b2ba5986d..7ce0e033d755 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work)
>> break;
>> }
>>
>> - /* For DRD host or device mode only */
>> - if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
>> - dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
>> + /*
>> + * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET.
>
> The line above doesn't really explain why. Can you rephrase it to
> something like this or the equivalent:
> "When current_dr_role is not set, there's no role switching. Only
> perform GCTL.CoreSoftReset when there's DRD role switching."
>
> Thanks,
> Thinh
Thanks for the review Thinh, I have updated the next version of the
patch with this suggestion.
Thanks,
Rohith
>
>> + * Perform it only for DRD host or device mode.
>> + */
>> + if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
>> + DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
>> + dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
>> reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> reg |= DWC3_GCTL_CORESOFTRESET;
>> dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-14 11:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-12 17:44 [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET during bootup Rohith Kollalsi
2022-07-14 1:06 ` Thinh Nguyen
2022-07-14 11:14 ` Rohith Kollalsi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.