[PATCH v3] usb: dwc3: core: fix a issue about clear connect state

[PATCH v3] usb: dwc3: core: fix a issue about clear connect state

[Dejin Zheng]

According to Synopsys Programming Guide chapter 2.2 Register Resets,
it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
reset, if DWC3 controller as a slave device and stay connected with a usb
host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
slave device when the DWC3 controller did not power off. because the
connection status is incorrect, so we also need to clear DCTL.RUN_STOP
bit for disabling connect when doing core soft reset. There will still
be other stale configuration in DCTL, so reset the other fields of DCTL
to the default value 0.

Fixes: f59dcab176293b6 ("usb: dwc3: core: improve reset sequence")
Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v2 -> v3:
	* Reset all fields of DCTL register by Thinh's suggest,
	  Thanks for Thinh's help!
v1 -> v2:
	* modify some commit messages by Sergei's suggest, Thanks
	  very much for Sergei's help!

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

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2eb34c8b4065..86375cfd9481 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -254,9 +254,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
 	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
 		return 0;
 
-	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-	reg |= DWC3_DCTL_CSFTRST;
-	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+	dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
 
 	/*
 	 * For DWC_usb31 controller 1.90a and later, the DCTL.CSFRST bit
-- 
2.25.0

Re: [PATCH v3] usb: dwc3: core: fix a issue about clear connect state

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]


Hi,

Dejin Zheng <zhengdejin5@gmail.com> writes:
> According to Synopsys Programming Guide chapter 2.2 Register Resets,
> it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
> reset, if DWC3 controller as a slave device and stay connected with a usb
> host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
> slave device when the DWC3 controller did not power off. because the
> connection status is incorrect, so we also need to clear DCTL.RUN_STOP
> bit for disabling connect when doing core soft reset. There will still
> be other stale configuration in DCTL, so reset the other fields of DCTL
> to the default value 0.

This commit log is a bit hard to understand. When does this problem
actually happen? It seems like it's in the case of, perhaps, kexecing
into a new kernel, is that right?

At the time dwc3_core_soft_reset() is called, the assumption is that
we're starting with a clean core, from power up. If we have stale
configuration from a previous run, we should fix this on the exit
path. Note that if we're reaching probe with pull up connected, we
already have issues elsewhere.

I think this is not the right fix for the problem.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v3] usb: dwc3: core: fix a issue about clear connect state

[Dejin Zheng]

On Tue, Oct 27, 2020 at 10:33:09AM +0200, Felipe Balbi wrote:
Hi Balbi:

Thank you so much for your comment.

> 
> Hi,
> 
> Dejin Zheng <zhengdejin5@gmail.com> writes:
> > According to Synopsys Programming Guide chapter 2.2 Register Resets,
> > it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
> > reset, if DWC3 controller as a slave device and stay connected with a usb
> > host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
> > slave device when the DWC3 controller did not power off. because the
> > connection status is incorrect, so we also need to clear DCTL.RUN_STOP
> > bit for disabling connect when doing core soft reset. There will still
> > be other stale configuration in DCTL, so reset the other fields of DCTL
> > to the default value 0.
> 
> This commit log is a bit hard to understand. When does this problem
> actually happen? It seems like it's in the case of, perhaps, kexecing
> into a new kernel, is that right?
> 
It happens when entering the kernel for the second time after the reboot
command.

> At the time dwc3_core_soft_reset() is called, the assumption is that
> we're starting with a clean core, from power up. If we have stale
> configuration from a previous run, we should fix this on the exit
> path. Note that if we're reaching probe with pull up connected, we
> already have issues elsewhere.
> 
> I think this is not the right fix for the problem.
>
I think you are right, Thinh also suggested me fix it on the exit path
in the previous patch v2. Do you think I can do these cleanups in the
shutdown hook of this driver? Balbi, is there a more suitable place to
do this by your rich experience? Thanks!

BR,
Dejin
> -- 
> balbi

Re: [PATCH v3] usb: dwc3: core: fix a issue about clear connect state

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]


Hi,

Dejin Zheng <zhengdejin5@gmail.com> writes:
>> Dejin Zheng <zhengdejin5@gmail.com> writes:
>> > According to Synopsys Programming Guide chapter 2.2 Register Resets,
>> > it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
>> > reset, if DWC3 controller as a slave device and stay connected with a usb
>> > host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
>> > slave device when the DWC3 controller did not power off. because the
>> > connection status is incorrect, so we also need to clear DCTL.RUN_STOP
>> > bit for disabling connect when doing core soft reset. There will still
>> > be other stale configuration in DCTL, so reset the other fields of DCTL
>> > to the default value 0.
>> 
>> This commit log is a bit hard to understand. When does this problem
>> actually happen? It seems like it's in the case of, perhaps, kexecing
>> into a new kernel, is that right?
>> 
> It happens when entering the kernel for the second time after the reboot
> command.
>
>> At the time dwc3_core_soft_reset() is called, the assumption is that
>> we're starting with a clean core, from power up. If we have stale
>> configuration from a previous run, we should fix this on the exit
>> path. Note that if we're reaching probe with pull up connected, we
>> already have issues elsewhere.
>> 
>> I think this is not the right fix for the problem.
>>
> I think you are right, Thinh also suggested me fix it on the exit path
> in the previous patch v2. Do you think I can do these cleanups in the
> shutdown hook of this driver? Balbi, is there a more suitable place to
> do this by your rich experience? Thanks!

I don't think shutdown is called during removal, I'm not sure. I think
we had some fixes done in shutdown time, though. Test it out, but make
sure there are no issues with a regular modprobe cycle.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v3] usb: dwc3: core: fix a issue about clear connect state

[Dejin Zheng]

On Wed, Oct 28, 2020 at 03:57:03PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Dejin Zheng <zhengdejin5@gmail.com> writes:
> >> Dejin Zheng <zhengdejin5@gmail.com> writes:
> >> > According to Synopsys Programming Guide chapter 2.2 Register Resets,
> >> > it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
> >> > reset, if DWC3 controller as a slave device and stay connected with a usb
> >> > host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
> >> > slave device when the DWC3 controller did not power off. because the
> >> > connection status is incorrect, so we also need to clear DCTL.RUN_STOP
> >> > bit for disabling connect when doing core soft reset. There will still
> >> > be other stale configuration in DCTL, so reset the other fields of DCTL
> >> > to the default value 0.
> >> 
> >> This commit log is a bit hard to understand. When does this problem
> >> actually happen? It seems like it's in the case of, perhaps, kexecing
> >> into a new kernel, is that right?
> >> 
> > It happens when entering the kernel for the second time after the reboot
> > command.
> >
> >> At the time dwc3_core_soft_reset() is called, the assumption is that
> >> we're starting with a clean core, from power up. If we have stale
> >> configuration from a previous run, we should fix this on the exit
> >> path. Note that if we're reaching probe with pull up connected, we
> >> already have issues elsewhere.
> >> 
> >> I think this is not the right fix for the problem.
> >>
> > I think you are right, Thinh also suggested me fix it on the exit path
> > in the previous patch v2. Do you think I can do these cleanups in the
> > shutdown hook of this driver? Balbi, is there a more suitable place to
> > do this by your rich experience? Thanks!
> 
> I don't think shutdown is called during removal, I'm not sure. I think
> we had some fixes done in shutdown time, though. Test it out, but make
> sure there are no issues with a regular modprobe cycle.
>
Balbi, thanks for your suggestions, I will do a test in the shutdown
hook first.
> -- 
> balbi

Re: [PATCH v3] usb: dwc3: core: fix a issue about clear connect state

[Dejin Zheng]

On Wed, Oct 28, 2020 at 03:57:03PM +0200, Felipe Balbi wrote:
Hi Balbi and all:
> 
> Hi,
> 
> Dejin Zheng <zhengdejin5@gmail.com> writes:
> >> Dejin Zheng <zhengdejin5@gmail.com> writes:
> >> > According to Synopsys Programming Guide chapter 2.2 Register Resets,
> >> > it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
> >> > reset, if DWC3 controller as a slave device and stay connected with a usb
> >> > host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
> >> > slave device when the DWC3 controller did not power off. because the
> >> > connection status is incorrect, so we also need to clear DCTL.RUN_STOP
> >> > bit for disabling connect when doing core soft reset. There will still
> >> > be other stale configuration in DCTL, so reset the other fields of DCTL
> >> > to the default value 0.
> >> 
> >> This commit log is a bit hard to understand. When does this problem
> >> actually happen? It seems like it's in the case of, perhaps, kexecing
> >> into a new kernel, is that right?
> >> 
> > It happens when entering the kernel for the second time after the reboot
> > command.
> >
> >> At the time dwc3_core_soft_reset() is called, the assumption is that
> >> we're starting with a clean core, from power up. If we have stale
> >> configuration from a previous run, we should fix this on the exit
> >> path. Note that if we're reaching probe with pull up connected, we
> >> already have issues elsewhere.
> >> 
> >> I think this is not the right fix for the problem.
> >>
> > I think you are right, Thinh also suggested me fix it on the exit path
> > in the previous patch v2. Do you think I can do these cleanups in the
> > shutdown hook of this driver? Balbi, is there a more suitable place to
> > do this by your rich experience? Thanks!
> 
> I don't think shutdown is called during removal, I'm not sure. I think
> we had some fixes done in shutdown time, though. Test it out, but make
> sure there are no issues with a regular modprobe cycle.
> 
It has some errors in my commit message, I describe the process of linux
restart is wrong. A PC is connected to our arm soc development board
through the usb cable, the adb program runs via usb connection. there is
a very important application in our linux system. when it goes wrong(halt
or kernel panic), we want to restart linux. my wrong description happened
here, when I manually kill this important application for testing, I
thought it was calling the reboot command to restart linux, which is wrong.
our real implementation is through watchdog, when the application no
longer sets the watchdog and the watchdog times out, but watchdog can't 
reset the whole soc. our soc has 3 cpu clusters, one cluster has a arm 
Cortex R5 cpu for boot and security. one cluster has 2 arm Cortex A55 for
linux system. the other cluster for android. when the Cortex R5 detect
the watchdog timeout and want to restart linux system, it will stop Cortex
A55 cpu to run, and load linux image to DDR memory from eMMC flash, then
set Cortex A55 cpu to run new linux system, but it was not reset usb
controller. so the usb controller's status is incorrect for boot new linux
system.

   ------------------------------------------------------------------
   |                                                                |
   |	Boot and Security          Linux             Android        |
   |	----------------     ----------------    ----------------   |
   |	|  1 Cortex R5  |    | 2 Cortex A55 |    | 4 Cortex A72 |   |
   |	|    cluster    |    |    cluster   |    |   cluster    |   |
   |	|---------------|    |--------------|    |--------------|   |
   |                                                                |
   |                           SOC                                  |
   |-----------------------------------------------------------------

Under normal circumstances, run the reboot command and rmmod the
corresponding usb module, it will carry out the corresponding state
processing, all of which can work well.

Balbi, for this case, Currently, the way I can think of is to reset the
DCTL register every initial time. Could you help me and give me some
suggestions? thank you very much!

BR,
Dejin

> -- 
> balbi