[1/1] usb: host: ehci: always enable interrupt for qtd completion at test mode

[1/1] usb: host: ehci: always enable interrupt for qtd completion at test mode

[Peter Chen]

At former code, the SETUP stage does not enable interrupt
for qtd completion, it relies on IAA watchdog to complete
interrupt, then the transcation would be considered timeout
if the flag need_io_watchdog is cleared by platform code.

In this commit, we always add enable interrupt for qtd completion,
then the qtd completion can be notified by hardware interrupt.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/ehci-q.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 8815832..4cc2609 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -1192,6 +1192,8 @@ static int submit_single_step_set_feature(
 		qtd_fill(ehci, qtd, urb->setup_dma,
 				sizeof(struct usb_ctrlrequest),
 				token | (2 /* "setup" */ << 8), 8);
+		/* Always enable interrupt on qtd completion */
+		qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);
 
 		submit_async(ehci, urb, &qtd_list, GFP_ATOMIC);
 		return 0; /*Return now; we shall come back after 15 seconds*/
@@ -1231,9 +1233,7 @@ static int submit_single_step_set_feature(
 	/* dont fill any data in such packets */
 	qtd_fill(ehci, qtd, 0, 0, token, 0);
 
-	/* by default, enable interrupt on urb completion */
-	if (likely(!(urb->transfer_flags & URB_NO_INTERRUPT)))
-		qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);
+	qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);
 
 	submit_async(ehci, urb, &qtd_list, GFP_KERNEL);
 

[1/1] usb: host: ehci: always enable interrupt for qtd completion at test mode

[Alan Stern]

On Thu, 1 Feb 2018, Peter Chen wrote:

> At former code, the SETUP stage does not enable interrupt
> for qtd completion, it relies on IAA watchdog to complete
> interrupt, then the transcation would be considered timeout
> if the flag need_io_watchdog is cleared by platform code.
> 
> In this commit, we always add enable interrupt for qtd completion,
> then the qtd completion can be notified by hardware interrupt.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/host/ehci-q.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 8815832..4cc2609 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -1192,6 +1192,8 @@ static int submit_single_step_set_feature(
>  		qtd_fill(ehci, qtd, urb->setup_dma,
>  				sizeof(struct usb_ctrlrequest),
>  				token | (2 /* "setup" */ << 8), 8);
> +		/* Always enable interrupt on qtd completion */
> +		qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);

This would be cleaner if you didn't make it a separate statement.  
Before the call to qtd_fill(), you can do:

		token |= QTD_IOC;	/* Interrupt after SETUP completion */


>  
>  		submit_async(ehci, urb, &qtd_list, GFP_ATOMIC);
>  		return 0; /*Return now; we shall come back after 15 seconds*/
> @@ -1231,9 +1233,7 @@ static int submit_single_step_set_feature(
>  	/* dont fill any data in such packets */
>  	qtd_fill(ehci, qtd, 0, 0, token, 0);
>  
> -	/* by default, enable interrupt on urb completion */
> -	if (likely(!(urb->transfer_flags & URB_NO_INTERRUPT)))
> -		qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);
> +	qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);

This could be improved in the same way.

Alan Stern

>  
>  	submit_async(ehci, urb, &qtd_list, GFP_KERNEL);
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[1/1] usb: host: ehci: always enable interrupt for qtd completion at test mode

[Peter Chen]

On Thu, Feb 01, 2018 at 02:24:29PM -0500, Alan Stern wrote:
> On Thu, 1 Feb 2018, Peter Chen wrote:
> 
> > At former code, the SETUP stage does not enable interrupt
> > for qtd completion, it relies on IAA watchdog to complete
> > interrupt, then the transcation would be considered timeout
> > if the flag need_io_watchdog is cleared by platform code.
> > 
> > In this commit, we always add enable interrupt for qtd completion,
> > then the qtd completion can be notified by hardware interrupt.
> > 
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/host/ehci-q.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> > index 8815832..4cc2609 100644
> > --- a/drivers/usb/host/ehci-q.c
> > +++ b/drivers/usb/host/ehci-q.c
> > @@ -1192,6 +1192,8 @@ static int submit_single_step_set_feature(
> >  		qtd_fill(ehci, qtd, urb->setup_dma,
> >  				sizeof(struct usb_ctrlrequest),
> >  				token | (2 /* "setup" */ << 8), 8);
> > +		/* Always enable interrupt on qtd completion */
> > +		qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);
> 
> This would be cleaner if you didn't make it a separate statement.  
> Before the call to qtd_fill(), you can do:
> 
> 		token |= QTD_IOC;	/* Interrupt after SETUP completion */

Since the variable "token" is used for every qtd, but I do not want
interrupt for DATA stage, how about adding QTD_IOC directly at
qtd_fill for both SETUP and STATUS stage?

  		qtd_fill(ehci, qtd, urb->setup_dma,
  				sizeof(struct usb_ctrlrequest),
  				QTD_IOC | token | (2 /* "setup" */ << 8), 8);

[1/1] usb: host: ehci: always enable interrupt for qtd completion at test mode

[Alan Stern]

On Fri, 2 Feb 2018, Peter Chen wrote:

> On Thu, Feb 01, 2018 at 02:24:29PM -0500, Alan Stern wrote:
> > On Thu, 1 Feb 2018, Peter Chen wrote:
> > 
> > > At former code, the SETUP stage does not enable interrupt
> > > for qtd completion, it relies on IAA watchdog to complete
> > > interrupt, then the transcation would be considered timeout
> > > if the flag need_io_watchdog is cleared by platform code.
> > > 
> > > In this commit, we always add enable interrupt for qtd completion,
> > > then the qtd completion can be notified by hardware interrupt.
> > > 
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > >  drivers/usb/host/ehci-q.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> > > index 8815832..4cc2609 100644
> > > --- a/drivers/usb/host/ehci-q.c
> > > +++ b/drivers/usb/host/ehci-q.c
> > > @@ -1192,6 +1192,8 @@ static int submit_single_step_set_feature(
> > >  		qtd_fill(ehci, qtd, urb->setup_dma,
> > >  				sizeof(struct usb_ctrlrequest),
> > >  				token | (2 /* "setup" */ << 8), 8);
> > > +		/* Always enable interrupt on qtd completion */
> > > +		qtd->hw_token |= cpu_to_hc32(ehci, QTD_IOC);
> > 
> > This would be cleaner if you didn't make it a separate statement.  
> > Before the call to qtd_fill(), you can do:
> > 
> > 		token |= QTD_IOC;	/* Interrupt after SETUP completion */
> 
> Since the variable "token" is used for every qtd, but I do not want
> interrupt for DATA stage, how about adding QTD_IOC directly at
> qtd_fill for both SETUP and STATUS stage?
> 
>   		qtd_fill(ehci, qtd, urb->setup_dma,
>   				sizeof(struct usb_ctrlrequest),
>   				QTD_IOC | token | (2 /* "setup" */ << 8), 8);

That would be okay.  You might want to include a comment line as well.

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html