[v2,2/2] usb: dwc2: host: Clear interrupts before handling them
diff mbox

Message ID 1446582629-30091-2-git-send-email-dianders@chromium.org
State New
Headers show

Commit Message

Doug Anderson Nov. 3, 2015, 8:30 p.m. UTC
In general it is wise to clear interrupts before processing them.  If
you don't do that, you can get:
 1. Interrupt happens
 2. You look at system state and process interrupt
 3. A new interrupt happens
 4. You clear interrupt without processing it.

This patch was actually a first attempt to fix missing device insertions
as described in (usb: dwc2: host: Fix missing device insertions) and it
did solve some of the signal bouncing problems but not all of
them (which is why I submitted the other patch).  Specifically, this
patch itself would sometimes change:
 1. hardware sees connect
 2. hardware sees disconnect
 3. hardware sees connect
 4. dwc2_port_intr() - clears connect interrupt
 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()

...to:
 1. hardware sees connect
 2. hardware sees disconnect
 3. dwc2_port_intr() - clears connect interrupt
 4. hardware sees connect
 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()

...but with different timing then sometimes we'd still miss cable
insertions.

In any case, though this patch doesn't fix any (known) problems, it
still seems wise as a general policy to clear interrupt before handling
them.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v2: None

 drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
 drivers/usb/dwc2/hcd_intr.c  | 16 ++++++-------
 2 files changed, 35 insertions(+), 36 deletions(-)

Comments

John Youn Nov. 5, 2015, 3:08 a.m. UTC | #1
On 11/3/2015 12:31 PM, Douglas Anderson wrote:
> In general it is wise to clear interrupts before processing them.  If
> you don't do that, you can get:
>  1. Interrupt happens
>  2. You look at system state and process interrupt
>  3. A new interrupt happens
>  4. You clear interrupt without processing it.
> 
> This patch was actually a first attempt to fix missing device insertions
> as described in (usb: dwc2: host: Fix missing device insertions) and it
> did solve some of the signal bouncing problems but not all of
> them (which is why I submitted the other patch).  Specifically, this
> patch itself would sometimes change:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. hardware sees connect
>  4. dwc2_port_intr() - clears connect interrupt
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
> 
> ...to:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. dwc2_port_intr() - clears connect interrupt
>  4. hardware sees connect
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
> 
> ...but with different timing then sometimes we'd still miss cable
> insertions.
> 
> In any case, though this patch doesn't fix any (known) problems, it
> still seems wise as a general policy to clear interrupt before handling
> them.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v2: None
> 
>  drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>  drivers/usb/dwc2/hcd_intr.c  | 16 ++++++-------
>  2 files changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 61601d16e233..2a166b7eec41 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>  {
> -	u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> +	u32 hprt0;
>  
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
> +
> +	hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>  	if (hprt0 & HPRT0_ENACHG) {
>  		hprt0 &= ~HPRT0_ENA;
>  		dwc2_writel(hprt0, hsotg->regs + HPRT0);
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>  }
>  
>  /**
> @@ -98,11 +99,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg)
>  {
> -	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
> -		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
> -
>  	/* Clear interrupt */
>  	dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS);
> +
> +	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
> +		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
>  }
>  
>  /**
> @@ -276,9 +277,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  {
> -	u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
> +	u32 gintmsk;
> +
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
>  
>  	/* Need to disable SOF interrupt immediately */
> +	gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
>  	gintmsk &= ~GINTSTS_SOF;
>  	dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
>  
> @@ -295,9 +300,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
>  		spin_lock(&hsotg->lock);
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
>  }
>  
>  /**
> @@ -315,12 +317,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  {
>  	int ret;
>  
> -	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
> -							hsotg->lx_state);
> -
>  	/* Clear interrupt */
>  	dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
>  
> +	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
> +							hsotg->lx_state);
> +
>  	if (dwc2_is_device_mode(hsotg)) {
>  		if (hsotg->lx_state == DWC2_L2) {
>  			ret = dwc2_exit_hibernation(hsotg, true);
> @@ -347,6 +349,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  {
>  	int ret;
> +
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> +
>  	dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n");
>  	dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state);
>  
> @@ -368,10 +374,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  		/* Change to L0 state */
>  		hsotg->lx_state = DWC2_L0;
>  	} else {
> -		if (hsotg->core_params->hibernation) {
> -			dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> +		if (hsotg->core_params->hibernation)
>  			return;
> -		}
> +
>  		if (hsotg->lx_state != DWC2_L1) {
>  			u32 pcgcctl = dwc2_readl(hsotg->regs + PCGCTL);
>  
> @@ -385,9 +390,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  			hsotg->lx_state = DWC2_L0;
>  		}
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>  }
>  
>  /*
> @@ -396,14 +398,14 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
>  {
> +	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
> +
>  	dev_dbg(hsotg->dev, "++Disconnect Detected Interrupt++ (%s) %s\n",
>  		dwc2_is_host_mode(hsotg) ? "Host" : "Device",
>  		dwc2_op_state_str(hsotg));
>  
>  	if (hsotg->op_state == OTG_STATE_A_HOST)
>  		dwc2_hcd_disconnect(hsotg, false);
> -
> -	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
>  }
>  
>  /*
> @@ -419,6 +421,9 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>  	u32 dsts;
>  	int ret;
>  
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
> +
>  	dev_dbg(hsotg->dev, "USB SUSPEND\n");
>  
>  	if (dwc2_is_device_mode(hsotg)) {
> @@ -437,7 +442,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>  			if (!dwc2_is_device_connected(hsotg)) {
>  				dev_dbg(hsotg->dev,
>  						"ignore suspend request before enumeration\n");
> -				goto clear_int;
> +				return;
>  			}
>  
>  			ret = dwc2_enter_hibernation(hsotg);
> @@ -476,10 +481,6 @@ skip_power_saving:
>  			hsotg->op_state = OTG_STATE_A_HOST;
>  		}
>  	}
> -
> -clear_int:
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
>  }
>  
>  #define GINTMSK_COMMON	(GINTSTS_WKUPINT | GINTSTS_SESSREQINT |		\
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 03504ac2fecc..4c7f709b8182 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -122,6 +122,9 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
>  	struct dwc2_qh *qh;
>  	enum dwc2_transaction_type tr_type;
>  
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
> +
>  #ifdef DEBUG_SOF
>  	dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n");
>  #endif
> @@ -146,9 +149,6 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
>  	tr_type = dwc2_hcd_select_transactions(hsotg);
>  	if (tr_type != DWC2_TRANSACTION_NONE)
>  		dwc2_hcd_queue_transactions(hsotg, tr_type);
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
>  }
>  
>  /*
> @@ -347,11 +347,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  	 * Set flag and clear if detected
>  	 */
>  	if (hprt0 & HPRT0_CONNDET) {
> +		writel(hprt0_modify | HPRT0_CONNDET, hsotg->regs + HPRT0);
> +
>  		dev_vdbg(hsotg->dev,
>  			 "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
>  			 hprt0);
>  		dwc2_hcd_connect(hsotg);
> -		hprt0_modify |= HPRT0_CONNDET;
>  
>  		/*
>  		 * The Hub driver asserts a reset when it sees port connect
> @@ -364,10 +365,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  	 * Clear if detected - Set internal flag if disabled
>  	 */
>  	if (hprt0 & HPRT0_ENACHG) {
> +		writel(hprt0_modify | HPRT0_ENACHG, hsotg->regs + HPRT0);
>  		dev_vdbg(hsotg->dev,
>  			 "  --Port Interrupt HPRT0=0x%08x Port Enable Changed (now %d)--\n",
>  			 hprt0, !!(hprt0 & HPRT0_ENA));
> -		hprt0_modify |= HPRT0_ENACHG;
>  		if (hprt0 & HPRT0_ENA)
>  			dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify);
>  		else
> @@ -376,15 +377,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  
>  	/* Overcurrent Change Interrupt */
>  	if (hprt0 & HPRT0_OVRCURRCHG) {
> +		writel(hprt0_modify | HPRT0_OVRCURRCHG, hsotg->regs + HPRT0);
>  		dev_vdbg(hsotg->dev,
>  			 "  --Port Interrupt HPRT0=0x%08x Port Overcurrent Changed--\n",
>  			 hprt0);
>  		hsotg->flags.b.port_over_current_change = 1;
> -		hprt0_modify |= HPRT0_OVRCURRCHG;
>  	}
> -
> -	/* Clear Port Interrupts */
> -	dwc2_writel(hprt0_modify, hsotg->regs + HPRT0);
>  }
>  
>  /*
> 

Acked-by: John Youn <johnyoun@synopsys.com>
Tested-by: John Youn <johnyoun@synopsys.com>

Regards,
John
Felipe Balbi Nov. 16, 2015, 4:28 p.m. UTC | #2
Hi,

Douglas Anderson <dianders@chromium.org> writes:
> In general it is wise to clear interrupts before processing them.  If
> you don't do that, you can get:
>  1. Interrupt happens
>  2. You look at system state and process interrupt
>  3. A new interrupt happens
>  4. You clear interrupt without processing it.
>
> This patch was actually a first attempt to fix missing device insertions
> as described in (usb: dwc2: host: Fix missing device insertions) and it
> did solve some of the signal bouncing problems but not all of
> them (which is why I submitted the other patch).  Specifically, this
> patch itself would sometimes change:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. hardware sees connect
>  4. dwc2_port_intr() - clears connect interrupt
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...to:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. dwc2_port_intr() - clears connect interrupt
>  4. hardware sees connect
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...but with different timing then sometimes we'd still miss cable
> insertions.
>
> In any case, though this patch doesn't fix any (known) problems, it
> still seems wise as a general policy to clear interrupt before handling
> them.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v2: None
>
>  drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>  drivers/usb/dwc2/hcd_intr.c  | 16 ++++++-------
>  2 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 61601d16e233..2a166b7eec41 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>  {
> -	u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> +	u32 hprt0;
>  
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
> +
> +	hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>  	if (hprt0 & HPRT0_ENACHG) {
>  		hprt0 &= ~HPRT0_ENA;
>  		dwc2_writel(hprt0, hsotg->regs + HPRT0);
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);

isn't this a regression ? You're first clearing the interrupts and only
then reading to check what's pending, however, what's pending has just
been cleared. Seems like this should be:

hprt0 = dwc2_readl(HPRT0);
dwc2_writeal(PRTINT, GINTSTS);


Also, these two patches look very large for the -rc. I'll move them to
v4.5 merge window unless you can convince me that they are, indeed, the
smallest change possible to fix the problem you're facing and that they
are, indeed, regressions.
Doug Anderson Nov. 16, 2015, 5:22 p.m. UTC | #3
Felipe,

On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <balbi@ti.com> wrote:
>
> Hi,
>
> Douglas Anderson <dianders@chromium.org> writes:
>> In general it is wise to clear interrupts before processing them.  If
>> you don't do that, you can get:
>>  1. Interrupt happens
>>  2. You look at system state and process interrupt
>>  3. A new interrupt happens
>>  4. You clear interrupt without processing it.
>>
>> This patch was actually a first attempt to fix missing device insertions
>> as described in (usb: dwc2: host: Fix missing device insertions) and it
>> did solve some of the signal bouncing problems but not all of
>> them (which is why I submitted the other patch).  Specifically, this
>> patch itself would sometimes change:
>>  1. hardware sees connect
>>  2. hardware sees disconnect
>>  3. hardware sees connect
>>  4. dwc2_port_intr() - clears connect interrupt
>>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>
>> ...to:
>>  1. hardware sees connect
>>  2. hardware sees disconnect
>>  3. dwc2_port_intr() - clears connect interrupt
>>  4. hardware sees connect
>>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>
>> ...but with different timing then sometimes we'd still miss cable
>> insertions.
>>
>> In any case, though this patch doesn't fix any (known) problems, it
>> still seems wise as a general policy to clear interrupt before handling
>> them.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> Changes in v2: None
>>
>>  drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>>  drivers/usb/dwc2/hcd_intr.c  | 16 ++++++-------
>>  2 files changed, 35 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 61601d16e233..2a166b7eec41 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>>   */
>>  static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>>  {
>> -     u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>> +     u32 hprt0;
>>
>> +     /* Clear interrupt */
>> +     dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>> +
>> +     hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>       if (hprt0 & HPRT0_ENACHG) {
>>               hprt0 &= ~HPRT0_ENA;
>>               dwc2_writel(hprt0, hsotg->regs + HPRT0);
>>       }
>> -
>> -     /* Clear interrupt */
>> -     dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>
> isn't this a regression ? You're first clearing the interrupts and only
> then reading to check what's pending, however, what's pending has just
> been cleared. Seems like this should be:
>
> hprt0 = dwc2_readl(HPRT0);
> dwc2_writeal(PRTINT, GINTSTS);

Actually, we could probably remove the setting of GINTSTS_PRTINT
completely.  The docs I have say that the GINTSTS_PRTINT is read only
and that:

> The core sets this bit to indicate a change in port status of one of the
> DWC_otg core ports in Host mode. The application must read the
> Host Port Control and Status (HPRT) register to determine the exact
> event that caused this interrupt. The application must clear the
> appropriate status bit in the Host Port Control and Status register to
> clear this bit.

...so writing PRTINT is probably useless, but John can confirm.

...if it wasn't useless to clear PRTINT it would depend on how things
were implemented.  One (purely speculative) case where my patch would
be more correct:

1. Interrupt source 1 in HPRT fires.  PRTINT is latched to 1 and
interrupt handler is called.
2. Read HPRT
3. Interrupt source 2 in HPRT fires.  PRTINT is already 1, so no new
interrupt handler called.
4. Clear PRTINT
5. Handle interrupt source 1

...now we'll never get an interrupt for interrupt source 2.


In any case, I think this particular change is a no-op, but other
changes in this patch (probably) aren't.


> Also, these two patches look very large for the -rc. I'll move them to
> v4.5 merge window unless you can convince me that they are, indeed, the
> smallest change possible to fix the problem you're facing and that they
> are, indeed, regressions.

Patch #2 should definitely _not_ go into the RC.  It is a relatively
intrusive patch, fixes no known issues, and has only had light
testing.  I haven't even landed this change locally in ChromeOS since
it's a bit scary to me I haven't found any good use for it.  I do keep
testing with it to see if it fixes any of my issues, though.  ;)


I'd like to see patch #1 should go into the RC since it fixes problems
that are really easy to reproduce.  IMHO It's also neither big nor
scary.  FWIW it's already landed in the ChromeOS tree as of Oct 20th
(almost a month).  While it hasn't hit stable channel yet, it's been
on beta channel and nobody has discovered any issues with it yet.


Thanks!  :)

-Doug
John Youn Nov. 19, 2015, 1:43 a.m. UTC | #4
On 11/16/2015 9:22 AM, Doug Anderson wrote:
> Felipe,
> 
> On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <balbi@ti.com> wrote:
>>
>> Hi,
>>
>> Douglas Anderson <dianders@chromium.org> writes:
>>> In general it is wise to clear interrupts before processing them.  If
>>> you don't do that, you can get:
>>>  1. Interrupt happens
>>>  2. You look at system state and process interrupt
>>>  3. A new interrupt happens
>>>  4. You clear interrupt without processing it.
>>>
>>> This patch was actually a first attempt to fix missing device insertions
>>> as described in (usb: dwc2: host: Fix missing device insertions) and it
>>> did solve some of the signal bouncing problems but not all of
>>> them (which is why I submitted the other patch).  Specifically, this
>>> patch itself would sometimes change:
>>>  1. hardware sees connect
>>>  2. hardware sees disconnect
>>>  3. hardware sees connect
>>>  4. dwc2_port_intr() - clears connect interrupt
>>>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>
>>> ...to:
>>>  1. hardware sees connect
>>>  2. hardware sees disconnect
>>>  3. dwc2_port_intr() - clears connect interrupt
>>>  4. hardware sees connect
>>>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>
>>> ...but with different timing then sometimes we'd still miss cable
>>> insertions.
>>>
>>> In any case, though this patch doesn't fix any (known) problems, it
>>> still seems wise as a general policy to clear interrupt before handling
>>> them.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>> Changes in v2: None
>>>
>>>  drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>>>  drivers/usb/dwc2/hcd_intr.c  | 16 ++++++-------
>>>  2 files changed, 35 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index 61601d16e233..2a166b7eec41 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>>>   */
>>>  static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>>>  {
>>> -     u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>> +     u32 hprt0;
>>>
>>> +     /* Clear interrupt */
>>> +     dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>> +
>>> +     hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>>       if (hprt0 & HPRT0_ENACHG) {
>>>               hprt0 &= ~HPRT0_ENA;
>>>               dwc2_writel(hprt0, hsotg->regs + HPRT0);
>>>       }
>>> -
>>> -     /* Clear interrupt */
>>> -     dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>
>> isn't this a regression ? You're first clearing the interrupts and only
>> then reading to check what's pending, however, what's pending has just
>> been cleared. Seems like this should be:
>>
>> hprt0 = dwc2_readl(HPRT0);
>> dwc2_writeal(PRTINT, GINTSTS);
> 
> Actually, we could probably remove the setting of GINTSTS_PRTINT
> completely.  The docs I have say that the GINTSTS_PRTINT is read only
> and that:
> 
>> The core sets this bit to indicate a change in port status of one of the
>> DWC_otg core ports in Host mode. The application must read the
>> Host Port Control and Status (HPRT) register to determine the exact
>> event that caused this interrupt. The application must clear the
>> appropriate status bit in the Host Port Control and Status register to
>> clear this bit.
> 
> ...so writing PRTINT is probably useless, but John can confirm.
> 

Yup, it seems it can be removed.

John
Doug Anderson Nov. 19, 2015, 4:37 p.m. UTC | #5
Hi,

On Wed, Nov 18, 2015 at 5:43 PM, John Youn <John.Youn@synopsys.com> wrote:
> On 11/16/2015 9:22 AM, Doug Anderson wrote:
>> Felipe,
>>
>> On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <balbi@ti.com> wrote:
>>>
>>> Hi,
>>>
>>> Douglas Anderson <dianders@chromium.org> writes:
>>>> In general it is wise to clear interrupts before processing them.  If
>>>> you don't do that, you can get:
>>>>  1. Interrupt happens
>>>>  2. You look at system state and process interrupt
>>>>  3. A new interrupt happens
>>>>  4. You clear interrupt without processing it.
>>>>
>>>> This patch was actually a first attempt to fix missing device insertions
>>>> as described in (usb: dwc2: host: Fix missing device insertions) and it
>>>> did solve some of the signal bouncing problems but not all of
>>>> them (which is why I submitted the other patch).  Specifically, this
>>>> patch itself would sometimes change:
>>>>  1. hardware sees connect
>>>>  2. hardware sees disconnect
>>>>  3. hardware sees connect
>>>>  4. dwc2_port_intr() - clears connect interrupt
>>>>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>>
>>>> ...to:
>>>>  1. hardware sees connect
>>>>  2. hardware sees disconnect
>>>>  3. dwc2_port_intr() - clears connect interrupt
>>>>  4. hardware sees connect
>>>>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>>
>>>> ...but with different timing then sometimes we'd still miss cable
>>>> insertions.
>>>>
>>>> In any case, though this patch doesn't fix any (known) problems, it
>>>> still seems wise as a general policy to clear interrupt before handling
>>>> them.
>>>>
>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>> ---
>>>> Changes in v2: None
>>>>
>>>>  drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>>>>  drivers/usb/dwc2/hcd_intr.c  | 16 ++++++-------
>>>>  2 files changed, 35 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>> index 61601d16e233..2a166b7eec41 100644
>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>>>>   */
>>>>  static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>>>>  {
>>>> -     u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>>> +     u32 hprt0;
>>>>
>>>> +     /* Clear interrupt */
>>>> +     dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>>> +
>>>> +     hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>>>       if (hprt0 & HPRT0_ENACHG) {
>>>>               hprt0 &= ~HPRT0_ENA;
>>>>               dwc2_writel(hprt0, hsotg->regs + HPRT0);
>>>>       }
>>>> -
>>>> -     /* Clear interrupt */
>>>> -     dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>>
>>> isn't this a regression ? You're first clearing the interrupts and only
>>> then reading to check what's pending, however, what's pending has just
>>> been cleared. Seems like this should be:
>>>
>>> hprt0 = dwc2_readl(HPRT0);
>>> dwc2_writeal(PRTINT, GINTSTS);
>>
>> Actually, we could probably remove the setting of GINTSTS_PRTINT
>> completely.  The docs I have say that the GINTSTS_PRTINT is read only
>> and that:
>>
>>> The core sets this bit to indicate a change in port status of one of the
>>> DWC_otg core ports in Host mode. The application must read the
>>> Host Port Control and Status (HPRT) register to determine the exact
>>> event that caused this interrupt. The application must clear the
>>> appropriate status bit in the Host Port Control and Status register to
>>> clear this bit.
>>
>> ...so writing PRTINT is probably useless, but John can confirm.
>>
>
> Yup, it seems it can be removed.

How do you guys want this handled?  Should I send up a new version of
this patch?  ...or should I send a followon patch that does this
removal?

-Doug
Felipe Balbi Nov. 19, 2015, 6:19 p.m. UTC | #6
Hi,

Doug Anderson <dianders@chromium.org> writes:
>>>> isn't this a regression ? You're first clearing the interrupts and only
>>>> then reading to check what's pending, however, what's pending has just
>>>> been cleared. Seems like this should be:
>>>>
>>>> hprt0 = dwc2_readl(HPRT0);
>>>> dwc2_writeal(PRTINT, GINTSTS);
>>>
>>> Actually, we could probably remove the setting of GINTSTS_PRTINT
>>> completely.  The docs I have say that the GINTSTS_PRTINT is read only
>>> and that:
>>>
>>>> The core sets this bit to indicate a change in port status of one of the
>>>> DWC_otg core ports in Host mode. The application must read the
>>>> Host Port Control and Status (HPRT) register to determine the exact
>>>> event that caused this interrupt. The application must clear the
>>>> appropriate status bit in the Host Port Control and Status register to
>>>> clear this bit.
>>>
>>> ...so writing PRTINT is probably useless, but John can confirm.
>>>
>>
>> Yup, it seems it can be removed.
>
> How do you guys want this handled?  Should I send up a new version of
> this patch?  ...or should I send a followon patch that does this
> removal?

I'll leave the final decision to John, but my opinion is that a new
version of the patch would be preferrable.
John Youn Nov. 19, 2015, 7:09 p.m. UTC | #7
On 11/19/2015 10:19 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Doug Anderson <dianders@chromium.org> writes:
>>>>> isn't this a regression ? You're first clearing the interrupts and only
>>>>> then reading to check what's pending, however, what's pending has just
>>>>> been cleared. Seems like this should be:
>>>>>
>>>>> hprt0 = dwc2_readl(HPRT0);
>>>>> dwc2_writeal(PRTINT, GINTSTS);
>>>>
>>>> Actually, we could probably remove the setting of GINTSTS_PRTINT
>>>> completely.  The docs I have say that the GINTSTS_PRTINT is read only
>>>> and that:
>>>>
>>>>> The core sets this bit to indicate a change in port status of one of the
>>>>> DWC_otg core ports in Host mode. The application must read the
>>>>> Host Port Control and Status (HPRT) register to determine the exact
>>>>> event that caused this interrupt. The application must clear the
>>>>> appropriate status bit in the Host Port Control and Status register to
>>>>> clear this bit.
>>>>
>>>> ...so writing PRTINT is probably useless, but John can confirm.
>>>>
>>>
>>> Yup, it seems it can be removed.
>>
>> How do you guys want this handled?  Should I send up a new version of
>> this patch?  ...or should I send a followon patch that does this
>> removal?
> 
> I'll leave the final decision to John, but my opinion is that a new
> version of the patch would be preferrable.
> 

Hi Doug,

Could you resend with the change?

Regards,
John

Patch
diff mbox

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 61601d16e233..2a166b7eec41 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -80,15 +80,16 @@  static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
 {
-	u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
+	u32 hprt0;
 
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
+
+	hprt0 = dwc2_readl(hsotg->regs + HPRT0);
 	if (hprt0 & HPRT0_ENACHG) {
 		hprt0 &= ~HPRT0_ENA;
 		dwc2_writel(hprt0, hsotg->regs + HPRT0);
 	}
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
 }
 
 /**
@@ -98,11 +99,11 @@  static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg)
 {
-	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
-		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
-
 	/* Clear interrupt */
 	dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS);
+
+	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
+		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
 }
 
 /**
@@ -276,9 +277,13 @@  static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
 {
-	u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+	u32 gintmsk;
+
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
 
 	/* Need to disable SOF interrupt immediately */
+	gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
 	gintmsk &= ~GINTSTS_SOF;
 	dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
 
@@ -295,9 +300,6 @@  static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
 		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
 		spin_lock(&hsotg->lock);
 	}
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
 }
 
 /**
@@ -315,12 +317,12 @@  static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
 
-	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
-							hsotg->lx_state);
-
 	/* Clear interrupt */
 	dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
 
+	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
+							hsotg->lx_state);
+
 	if (dwc2_is_device_mode(hsotg)) {
 		if (hsotg->lx_state == DWC2_L2) {
 			ret = dwc2_exit_hibernation(hsotg, true);
@@ -347,6 +349,10 @@  static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
+
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
+
 	dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n");
 	dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state);
 
@@ -368,10 +374,9 @@  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 		/* Change to L0 state */
 		hsotg->lx_state = DWC2_L0;
 	} else {
-		if (hsotg->core_params->hibernation) {
-			dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
+		if (hsotg->core_params->hibernation)
 			return;
-		}
+
 		if (hsotg->lx_state != DWC2_L1) {
 			u32 pcgcctl = dwc2_readl(hsotg->regs + PCGCTL);
 
@@ -385,9 +390,6 @@  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 			hsotg->lx_state = DWC2_L0;
 		}
 	}
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
 }
 
 /*
@@ -396,14 +398,14 @@  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
 {
+	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
+
 	dev_dbg(hsotg->dev, "++Disconnect Detected Interrupt++ (%s) %s\n",
 		dwc2_is_host_mode(hsotg) ? "Host" : "Device",
 		dwc2_op_state_str(hsotg));
 
 	if (hsotg->op_state == OTG_STATE_A_HOST)
 		dwc2_hcd_disconnect(hsotg, false);
-
-	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
 }
 
 /*
@@ -419,6 +421,9 @@  static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 	u32 dsts;
 	int ret;
 
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
+
 	dev_dbg(hsotg->dev, "USB SUSPEND\n");
 
 	if (dwc2_is_device_mode(hsotg)) {
@@ -437,7 +442,7 @@  static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 			if (!dwc2_is_device_connected(hsotg)) {
 				dev_dbg(hsotg->dev,
 						"ignore suspend request before enumeration\n");
-				goto clear_int;
+				return;
 			}
 
 			ret = dwc2_enter_hibernation(hsotg);
@@ -476,10 +481,6 @@  skip_power_saving:
 			hsotg->op_state = OTG_STATE_A_HOST;
 		}
 	}
-
-clear_int:
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
 }
 
 #define GINTMSK_COMMON	(GINTSTS_WKUPINT | GINTSTS_SESSREQINT |		\
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 03504ac2fecc..4c7f709b8182 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -122,6 +122,9 @@  static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
 	struct dwc2_qh *qh;
 	enum dwc2_transaction_type tr_type;
 
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
+
 #ifdef DEBUG_SOF
 	dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n");
 #endif
@@ -146,9 +149,6 @@  static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
 	tr_type = dwc2_hcd_select_transactions(hsotg);
 	if (tr_type != DWC2_TRANSACTION_NONE)
 		dwc2_hcd_queue_transactions(hsotg, tr_type);
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
 }
 
 /*
@@ -347,11 +347,12 @@  static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 	 * Set flag and clear if detected
 	 */
 	if (hprt0 & HPRT0_CONNDET) {
+		writel(hprt0_modify | HPRT0_CONNDET, hsotg->regs + HPRT0);
+
 		dev_vdbg(hsotg->dev,
 			 "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
 			 hprt0);
 		dwc2_hcd_connect(hsotg);
-		hprt0_modify |= HPRT0_CONNDET;
 
 		/*
 		 * The Hub driver asserts a reset when it sees port connect
@@ -364,10 +365,10 @@  static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 	 * Clear if detected - Set internal flag if disabled
 	 */
 	if (hprt0 & HPRT0_ENACHG) {
+		writel(hprt0_modify | HPRT0_ENACHG, hsotg->regs + HPRT0);
 		dev_vdbg(hsotg->dev,
 			 "  --Port Interrupt HPRT0=0x%08x Port Enable Changed (now %d)--\n",
 			 hprt0, !!(hprt0 & HPRT0_ENA));
-		hprt0_modify |= HPRT0_ENACHG;
 		if (hprt0 & HPRT0_ENA)
 			dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify);
 		else
@@ -376,15 +377,12 @@  static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 
 	/* Overcurrent Change Interrupt */
 	if (hprt0 & HPRT0_OVRCURRCHG) {
+		writel(hprt0_modify | HPRT0_OVRCURRCHG, hsotg->regs + HPRT0);
 		dev_vdbg(hsotg->dev,
 			 "  --Port Interrupt HPRT0=0x%08x Port Overcurrent Changed--\n",
 			 hprt0);
 		hsotg->flags.b.port_over_current_change = 1;
-		hprt0_modify |= HPRT0_OVRCURRCHG;
 	}
-
-	/* Clear Port Interrupts */
-	dwc2_writel(hprt0_modify, hsotg->regs + HPRT0);
 }
 
 /*