linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] USB DWC2 parity fix in isochronous mode
@ 2015-08-18 15:45 Scott Branden
  2015-08-18 15:45 ` [PATCH 1/1] usb: dwc2: gadget: " Scott Branden
  0 siblings, 1 reply; 8+ messages in thread
From: Scott Branden @ 2015-08-18 15:45 UTC (permalink / raw)
  To: John Youn, Greg Kroah-Hartman, linux-usb
  Cc: linux-kernel, bcm-kernel-feedback-list, Scott Branden

This patch contains a fix for a real world interop problem found
when using the Synopsis DWC2 USB controller with isochronous audio as
detailed in the commit message.

Roman Bacik (1):
  usb: dwc2: gadget: parity fix in isochronous mode

 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/dwc2/hw.h     |  1 +
 3 files changed, 49 insertions(+), 1 deletion(-)

-- 
2.4.6


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-08-18 15:45 [PATCH 0/1] USB DWC2 parity fix in isochronous mode Scott Branden
@ 2015-08-18 15:45 ` Scott Branden
  2015-08-25 21:51   ` John Youn
  0 siblings, 1 reply; 8+ messages in thread
From: Scott Branden @ 2015-08-18 15:45 UTC (permalink / raw)
  To: John Youn, Greg Kroah-Hartman, linux-usb
  Cc: linux-kernel, bcm-kernel-feedback-list, Roman Bacik, Scott Branden

From: Roman Bacik <rbacik@broadcom.com>

USB OTG driver in isochronous mode has to set the parity of the receiving
microframe. The parity is set to even by default. This causes problems for
an audio gadget, if the host starts transmitting on odd microframes.

This fix uses Incomplete Periodic Transfer interrupt to toggle between
even and odd parity until the Transfer Complete interrupt is received.

Signed-off-by: Roman Bacik <rbacik@broadcom.com>
Reviewed-by: Abhinav Ratna <aratna@broadcom.com>
Reviewed-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/dwc2/hw.h     |  1 +
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 0ed87620..954d1cd 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
 	unsigned int            periodic:1;
 	unsigned int            isochronous:1;
 	unsigned int            send_zlp:1;
+	unsigned int            parity_set:1;
 
 	char                    name[10];
 };
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 4d47b7c..28e4393 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx,
 		ints &= ~DXEPINT_XFERCOMPL;
 
 	if (ints & DXEPINT_XFERCOMPL) {
+		if (hs_ep->isochronous && !hs_ep->parity_set)
+			hs_ep->parity_set = 1;
 		if (hs_ep->isochronous && hs_ep->interval == 1) {
 			if (ctrl & DXEPCTL_EOFRNUM)
 				ctrl |= DXEPCTL_SETEVENFR;
@@ -2316,7 +2318,8 @@ void s3c_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
 		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
 		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
 		GINTSTS_OTGINT | GINTSTS_USBSUSP |
-		GINTSTS_WKUPINT,
+		GINTSTS_WKUPINT |
+		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,
 		hsotg->regs + GINTMSK);
 
 	if (using_dma(hsotg))
@@ -2581,6 +2584,48 @@ irq_retry:
 		s3c_hsotg_dump(hsotg);
 	}
 
+	if (gintsts & GINTSTS_INCOMPL_SOIN) {
+		u32 idx;
+		struct s3c_hsotg_ep *hs_ep;
+
+		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n", __func__);
+		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
+			hs_ep = hsotg->eps_in[idx];
+			if (hs_ep->isochronous && !hs_ep->parity_set) {
+				u32 epctl_reg = DIEPCTL(idx);
+				u32 ctrl = readl(hsotg->regs + epctl_reg);
+
+				if (ctrl & DXEPCTL_EOFRNUM)
+					ctrl |= DXEPCTL_SETEVENFR;
+				else
+					ctrl |= DXEPCTL_SETODDFR;
+				writel(ctrl, hsotg->regs + epctl_reg);
+			}
+		}
+		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
+	}
+
+	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
+		u32 idx;
+		struct s3c_hsotg_ep *hs_ep;
+
+		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOOUT\n", __func__);
+		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
+			hs_ep = hsotg->eps_out[idx];
+			if (hs_ep->isochronous && !hs_ep->parity_set) {
+				u32 epctl_reg = DOEPCTL(idx);
+				u32 ctrl = readl(hsotg->regs + epctl_reg);
+
+				if (ctrl & DXEPCTL_EOFRNUM)
+					ctrl |= DXEPCTL_SETEVENFR;
+				else
+					ctrl |= DXEPCTL_SETODDFR;
+				writel(ctrl, hsotg->regs + epctl_reg);
+			}
+		}
+		writel(GINTSTS_INCOMPL_SOOUT, hsotg->regs + GINTSTS);
+	}
+
 	/*
 	 * if we've had fifo events, we should try and go around the
 	 * loop again to see if there's any point in returning yet.
@@ -2667,6 +2712,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
 	hs_ep->periodic = 0;
 	hs_ep->halted = 0;
 	hs_ep->interval = desc->bInterval;
+	hs_ep->parity_set = 0;
 
 	if (hs_ep->interval > 1 && hs_ep->mc > 1)
 		dev_err(hsotg->dev, "MC > 1 when interval is not 1\n");
diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
index d0a5ed8..553f246 100644
--- a/drivers/usb/dwc2/hw.h
+++ b/drivers/usb/dwc2/hw.h
@@ -142,6 +142,7 @@
 #define GINTSTS_RESETDET		(1 << 23)
 #define GINTSTS_FET_SUSP		(1 << 22)
 #define GINTSTS_INCOMPL_IP		(1 << 21)
+#define GINTSTS_INCOMPL_SOOUT		(1 << 21)
 #define GINTSTS_INCOMPL_SOIN		(1 << 20)
 #define GINTSTS_OEPINT			(1 << 19)
 #define GINTSTS_IEPINT			(1 << 18)
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-08-18 15:45 ` [PATCH 1/1] usb: dwc2: gadget: " Scott Branden
@ 2015-08-25 21:51   ` John Youn
  2015-08-25 22:00     ` Roman Bacik
  0 siblings, 1 reply; 8+ messages in thread
From: John Youn @ 2015-08-25 21:51 UTC (permalink / raw)
  To: Scott Branden, John Youn, Greg Kroah-Hartman, linux-usb
  Cc: linux-kernel, bcm-kernel-feedback-list, Roman Bacik

On 8/18/2015 8:45 AM, Scott Branden wrote:
> From: Roman Bacik <rbacik@broadcom.com>
> 
> USB OTG driver in isochronous mode has to set the parity of the receiving
> microframe. The parity is set to even by default. This causes problems for
> an audio gadget, if the host starts transmitting on odd microframes.
> 
> This fix uses Incomplete Periodic Transfer interrupt to toggle between
> even and odd parity until the Transfer Complete interrupt is received.
> 
> Signed-off-by: Roman Bacik <rbacik@broadcom.com>
> Reviewed-by: Abhinav Ratna <aratna@broadcom.com>
> Reviewed-by: Srinath Mannam <srinath.mannam@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc2/hw.h     |  1 +
>  3 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 0ed87620..954d1cd 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
>  	unsigned int            periodic:1;
>  	unsigned int            isochronous:1;
>  	unsigned int            send_zlp:1;
> +	unsigned int            parity_set:1;
>  
>  	char                    name[10];
>  };
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 4d47b7c..28e4393 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx,
>  		ints &= ~DXEPINT_XFERCOMPL;
>  
>  	if (ints & DXEPINT_XFERCOMPL) {
> +		if (hs_ep->isochronous && !hs_ep->parity_set)
> +			hs_ep->parity_set = 1;
>  		if (hs_ep->isochronous && hs_ep->interval == 1) {
>  			if (ctrl & DXEPCTL_EOFRNUM)
>  				ctrl |= DXEPCTL_SETEVENFR;
> @@ -2316,7 +2318,8 @@ void s3c_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>  		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
>  		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
>  		GINTSTS_OTGINT | GINTSTS_USBSUSP |
> -		GINTSTS_WKUPINT,
> +		GINTSTS_WKUPINT |
> +		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,
>  		hsotg->regs + GINTMSK);
>  
>  	if (using_dma(hsotg))
> @@ -2581,6 +2584,48 @@ irq_retry:
>  		s3c_hsotg_dump(hsotg);
>  	}
>  
> +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> +		u32 idx;
> +		struct s3c_hsotg_ep *hs_ep;
> +
> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n", __func__);
> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> +			hs_ep = hsotg->eps_in[idx];
> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
> +				u32 epctl_reg = DIEPCTL(idx);
> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
> +
> +				if (ctrl & DXEPCTL_EOFRNUM)
> +					ctrl |= DXEPCTL_SETEVENFR;
> +				else
> +					ctrl |= DXEPCTL_SETODDFR;
> +				writel(ctrl, hsotg->regs + epctl_reg);
> +			}
> +		}
> +		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
> +	}
> +
> +	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
> +		u32 idx;
> +		struct s3c_hsotg_ep *hs_ep;
> +
> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOOUT\n", __func__);
> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> +			hs_ep = hsotg->eps_out[idx];
> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
> +				u32 epctl_reg = DOEPCTL(idx);
> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
> +
> +				if (ctrl & DXEPCTL_EOFRNUM)
> +					ctrl |= DXEPCTL_SETEVENFR;
> +				else
> +					ctrl |= DXEPCTL_SETODDFR;
> +				writel(ctrl, hsotg->regs + epctl_reg);
> +			}
> +		}
> +		writel(GINTSTS_INCOMPL_SOOUT, hsotg->regs + GINTSTS);
> +	}
> +
>  	/*
>  	 * if we've had fifo events, we should try and go around the
>  	 * loop again to see if there's any point in returning yet.
> @@ -2667,6 +2712,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
>  	hs_ep->periodic = 0;
>  	hs_ep->halted = 0;
>  	hs_ep->interval = desc->bInterval;
> +	hs_ep->parity_set = 0;


I'm not quite sure what the parity_set flag does in this patch.
Shouldn't you be able to just toggle the even/odd frame when you
get the interrupt?

John



^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-08-25 21:51   ` John Youn
@ 2015-08-25 22:00     ` Roman Bacik
  2015-08-25 22:35       ` Felipe Balbi
  2015-08-26  2:05       ` John Youn
  0 siblings, 2 replies; 8+ messages in thread
From: Roman Bacik @ 2015-08-25 22:00 UTC (permalink / raw)
  To: John Youn, Scott Branden, Greg Kroah-Hartman, linux-usb
  Cc: linux-kernel, bcm-kernel-feedback-list

> -----Original Message-----
> From: John Youn [mailto:John.Youn@synopsys.com]
> Sent: August-25-15 2:52 PM
> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list; Roman Bacik
> Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> 
> On 8/18/2015 8:45 AM, Scott Branden wrote:
> > From: Roman Bacik <rbacik@broadcom.com>
> >
> > USB OTG driver in isochronous mode has to set the parity of the
> > receiving microframe. The parity is set to even by default. This
> > causes problems for an audio gadget, if the host starts transmitting on odd
> microframes.
> >
> > This fix uses Incomplete Periodic Transfer interrupt to toggle between
> > even and odd parity until the Transfer Complete interrupt is received.
> >
> > Signed-off-by: Roman Bacik <rbacik@broadcom.com>
> > Reviewed-by: Abhinav Ratna <aratna@broadcom.com>
> > Reviewed-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > Signed-off-by: Scott Branden <sbranden@broadcom.com>
> > ---
> >  drivers/usb/dwc2/core.h   |  1 +
> >  drivers/usb/dwc2/gadget.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/usb/dwc2/hw.h     |  1 +
> >  3 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
> > 0ed87620..954d1cd 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
> >  	unsigned int            periodic:1;
> >  	unsigned int            isochronous:1;
> >  	unsigned int            send_zlp:1;
> > +	unsigned int            parity_set:1;
> >
> >  	char                    name[10];
> >  };
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 4d47b7c..28e4393 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct dwc2_hsotg
> *hsotg, unsigned int idx,
> >  		ints &= ~DXEPINT_XFERCOMPL;
> >
> >  	if (ints & DXEPINT_XFERCOMPL) {
> > +		if (hs_ep->isochronous && !hs_ep->parity_set)
> > +			hs_ep->parity_set = 1;
> >  		if (hs_ep->isochronous && hs_ep->interval == 1) {
> >  			if (ctrl & DXEPCTL_EOFRNUM)
> >  				ctrl |= DXEPCTL_SETEVENFR;
> > @@ -2316,7 +2318,8 @@ void s3c_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> >  		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
> >  		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
> >  		GINTSTS_OTGINT | GINTSTS_USBSUSP |
> > -		GINTSTS_WKUPINT,
> > +		GINTSTS_WKUPINT |
> > +		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,
> >  		hsotg->regs + GINTMSK);
> >
> >  	if (using_dma(hsotg))
> > @@ -2581,6 +2584,48 @@ irq_retry:
> >  		s3c_hsotg_dump(hsotg);
> >  	}
> >
> > +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> > +		u32 idx;
> > +		struct s3c_hsotg_ep *hs_ep;
> > +
> > +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n",
> __func__);
> > +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> > +			hs_ep = hsotg->eps_in[idx];
> > +			if (hs_ep->isochronous && !hs_ep->parity_set) {
> > +				u32 epctl_reg = DIEPCTL(idx);
> > +				u32 ctrl = readl(hsotg->regs + epctl_reg);
> > +
> > +				if (ctrl & DXEPCTL_EOFRNUM)
> > +					ctrl |= DXEPCTL_SETEVENFR;
> > +				else
> > +					ctrl |= DXEPCTL_SETODDFR;
> > +				writel(ctrl, hsotg->regs + epctl_reg);
> > +			}
> > +		}
> > +		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
> > +	}
> > +
> > +	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
> > +		u32 idx;
> > +		struct s3c_hsotg_ep *hs_ep;
> > +
> > +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOOUT\n",
> __func__);
> > +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> > +			hs_ep = hsotg->eps_out[idx];
> > +			if (hs_ep->isochronous && !hs_ep->parity_set) {
> > +				u32 epctl_reg = DOEPCTL(idx);
> > +				u32 ctrl = readl(hsotg->regs + epctl_reg);
> > +
> > +				if (ctrl & DXEPCTL_EOFRNUM)
> > +					ctrl |= DXEPCTL_SETEVENFR;
> > +				else
> > +					ctrl |= DXEPCTL_SETODDFR;
> > +				writel(ctrl, hsotg->regs + epctl_reg);
> > +			}
> > +		}
> > +		writel(GINTSTS_INCOMPL_SOOUT, hsotg->regs + GINTSTS);
> > +	}
> > +
> >  	/*
> >  	 * if we've had fifo events, we should try and go around the
> >  	 * loop again to see if there's any point in returning yet.
> > @@ -2667,6 +2712,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep
> *ep,
> >  	hs_ep->periodic = 0;
> >  	hs_ep->halted = 0;
> >  	hs_ep->interval = desc->bInterval;
> > +	hs_ep->parity_set = 0;
> 
> 
> I'm not quite sure what the parity_set flag does in this patch.
> Shouldn't you be able to just toggle the even/odd frame when you get the
> interrupt?
> 
> John
> 

When Transfer Complete interrupt is received, we have the correct parity. Therefore we set the flag and we stop toggling. The parity_set flag indicates whether we have the correct parity set.
Regards,

Roman



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-08-25 22:00     ` Roman Bacik
@ 2015-08-25 22:35       ` Felipe Balbi
  2015-08-25 23:00         ` Roman Bacik
  2015-08-26  2:05       ` John Youn
  1 sibling, 1 reply; 8+ messages in thread
From: Felipe Balbi @ 2015-08-25 22:35 UTC (permalink / raw)
  To: Roman Bacik
  Cc: John Youn, Scott Branden, Greg Kroah-Hartman, linux-usb,
	linux-kernel, bcm-kernel-feedback-list

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

On Tue, Aug 25, 2015 at 10:00:17PM +0000, Roman Bacik wrote:
> > -----Original Message-----
> > From: John Youn [mailto:John.Youn@synopsys.com]
> > Sent: August-25-15 2:52 PM
> > To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> > usb@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list; Roman Bacik
> > Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> > 
> > On 8/18/2015 8:45 AM, Scott Branden wrote:
> > > From: Roman Bacik <rbacik@broadcom.com>
> > >
> > > USB OTG driver in isochronous mode has to set the parity of the
> > > receiving microframe. The parity is set to even by default. This
> > > causes problems for an audio gadget, if the host starts transmitting on odd
> > microframes.
> > >
> > > This fix uses Incomplete Periodic Transfer interrupt to toggle between
> > > even and odd parity until the Transfer Complete interrupt is received.
> > >
> > > Signed-off-by: Roman Bacik <rbacik@broadcom.com>
> > > Reviewed-by: Abhinav Ratna <aratna@broadcom.com>
> > > Reviewed-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > > Signed-off-by: Scott Branden <sbranden@broadcom.com>
> > > ---
> > >  drivers/usb/dwc2/core.h   |  1 +
> > >  drivers/usb/dwc2/gadget.c | 48
> > ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  drivers/usb/dwc2/hw.h     |  1 +
> > >  3 files changed, 49 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
> > > 0ed87620..954d1cd 100644
> > > --- a/drivers/usb/dwc2/core.h
> > > +++ b/drivers/usb/dwc2/core.h
> > > @@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
> > >  	unsigned int            periodic:1;
> > >  	unsigned int            isochronous:1;
> > >  	unsigned int            send_zlp:1;
> > > +	unsigned int            parity_set:1;
> > >
> > >  	char                    name[10];
> > >  };
> > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > > index 4d47b7c..28e4393 100644
> > > --- a/drivers/usb/dwc2/gadget.c
> > > +++ b/drivers/usb/dwc2/gadget.c
> > > @@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct dwc2_hsotg
> > *hsotg, unsigned int idx,
> > >  		ints &= ~DXEPINT_XFERCOMPL;
> > >
> > >  	if (ints & DXEPINT_XFERCOMPL) {
> > > +		if (hs_ep->isochronous && !hs_ep->parity_set)
> > > +			hs_ep->parity_set = 1;

it shouldn't be a problem to set the flag which was already set, so this
could be simplified to:

		hs_ep->has_correct_parity = !!hs_ep0>isochronous;

> > >  		if (hs_ep->isochronous && hs_ep->interval == 1) {
> > >  			if (ctrl & DXEPCTL_EOFRNUM)
> > >  				ctrl |= DXEPCTL_SETEVENFR;
> > > @@ -2316,7 +2318,8 @@ void s3c_hsotg_core_init_disconnected(struct
> > dwc2_hsotg *hsotg,
> > >  		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
> > >  		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
> > >  		GINTSTS_OTGINT | GINTSTS_USBSUSP |
> > > -		GINTSTS_WKUPINT,
> > > +		GINTSTS_WKUPINT |
> > > +		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,

why the two extra bits ? What are they doing ?

> > >  		hsotg->regs + GINTMSK);
> > >
> > >  	if (using_dma(hsotg))
> > > @@ -2581,6 +2584,48 @@ irq_retry:
> > >  		s3c_hsotg_dump(hsotg);
> > >  	}
> > >
> > > +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> > > +		u32 idx;
> > > +		struct s3c_hsotg_ep *hs_ep;
> > > +
> > > +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n",
> > __func__);
> > > +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {

			u32 epctl_reg;
			u32 ctrl;

> > > +			hs_ep = hsotg->eps_in[idx];

you can decrease some indentation here:

			if (!hs_ep->isochronous)
				continue;

			if (hs_ep->has_correct_parity)
				continue;

			epctl_reg = DIEPCTL(idx);
			ctrl = readl(hsotg->regs + epctl_reg);

			if (ctrl & DXEPCTL_EOFRNUM)
				ctrl |= DXEPCTL_SETEVENFR;
			else
				ctrl |= DXEPCTL_SETODDFR;
			writel(ctrl, hsotg->regs + epctl_reg);


ditto to the other loop below

<snip>

> > I'm not quite sure what the parity_set flag does in this patch.
> > Shouldn't you be able to just toggle the even/odd frame when you get the
> > interrupt?
> > 
> > John
> > 
> 
> When Transfer Complete interrupt is received, we have the correct
> parity. Therefore we set the flag and we stop toggling. The parity_set
> flag indicates whether we have the correct parity set.

then how about calling it has_correct_parity instead ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-08-25 22:35       ` Felipe Balbi
@ 2015-08-25 23:00         ` Roman Bacik
  0 siblings, 0 replies; 8+ messages in thread
From: Roman Bacik @ 2015-08-25 23:00 UTC (permalink / raw)
  To: balbi
  Cc: John Youn, Scott Branden, Greg Kroah-Hartman, linux-usb,
	linux-kernel, bcm-kernel-feedback-list

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: August-25-15 3:36 PM
> To: Roman Bacik
> Cc: John Youn; Scott Branden; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org; linux-kernel@vger.kernel.org; bcm-kernel-feedback-
> list
> Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> 
> On Tue, Aug 25, 2015 at 10:00:17PM +0000, Roman Bacik wrote:
> > > -----Original Message-----
> > > From: John Youn [mailto:John.Youn@synopsys.com]
> > > Sent: August-25-15 2:52 PM
> > > To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> > > usb@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list; Roman
> > > Bacik
> > > Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in
> > > isochronous mode
> > >
> > > On 8/18/2015 8:45 AM, Scott Branden wrote:
> > > > From: Roman Bacik <rbacik@broadcom.com>
> > > >
> > > > USB OTG driver in isochronous mode has to set the parity of the
> > > > receiving microframe. The parity is set to even by default. This
> > > > causes problems for an audio gadget, if the host starts
> > > > transmitting on odd
> > > microframes.
> > > >
> > > > This fix uses Incomplete Periodic Transfer interrupt to toggle
> > > > between even and odd parity until the Transfer Complete interrupt is
> received.
> > > >
> > > > Signed-off-by: Roman Bacik <rbacik@broadcom.com>
> > > > Reviewed-by: Abhinav Ratna <aratna@broadcom.com>
> > > > Reviewed-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > > > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > > > Signed-off-by: Scott Branden <sbranden@broadcom.com>
> > > > ---
> > > >  drivers/usb/dwc2/core.h   |  1 +
> > > >  drivers/usb/dwc2/gadget.c | 48
> > > ++++++++++++++++++++++++++++++++++++++++++++++-
> > > >  drivers/usb/dwc2/hw.h     |  1 +
> > > >  3 files changed, 49 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > > > index 0ed87620..954d1cd 100644
> > > > --- a/drivers/usb/dwc2/core.h
> > > > +++ b/drivers/usb/dwc2/core.h
> > > > @@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
> > > >  	unsigned int            periodic:1;
> > > >  	unsigned int            isochronous:1;
> > > >  	unsigned int            send_zlp:1;
> > > > +	unsigned int            parity_set:1;
> > > >
> > > >  	char                    name[10];
> > > >  };
> > > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > > > index 4d47b7c..28e4393 100644
> > > > --- a/drivers/usb/dwc2/gadget.c
> > > > +++ b/drivers/usb/dwc2/gadget.c
> > > > @@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct
> > > > dwc2_hsotg
> > > *hsotg, unsigned int idx,
> > > >  		ints &= ~DXEPINT_XFERCOMPL;
> > > >
> > > >  	if (ints & DXEPINT_XFERCOMPL) {
> > > > +		if (hs_ep->isochronous && !hs_ep->parity_set)
> > > > +			hs_ep->parity_set = 1;
> 
> it shouldn't be a problem to set the flag which was already set, so this could
> be simplified to:
> 
> 		hs_ep->has_correct_parity = !!hs_ep0>isochronous;
> 

It can be simplified to:
		hs_ep->has_correct_parity = 1;
I just thought that the original shows better what we are trying to do. I do not mind to simplify it and remove the condition.

> > > >  		if (hs_ep->isochronous && hs_ep->interval == 1) {
> > > >  			if (ctrl & DXEPCTL_EOFRNUM)
> > > >  				ctrl |= DXEPCTL_SETEVENFR;
> > > > @@ -2316,7 +2318,8 @@ void s3c_hsotg_core_init_disconnected(struct
> > > dwc2_hsotg *hsotg,
> > > >  		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
> > > >  		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
> > > >  		GINTSTS_OTGINT | GINTSTS_USBSUSP |
> > > > -		GINTSTS_WKUPINT,
> > > > +		GINTSTS_WKUPINT |
> > > > +		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,
> 
> why the two extra bits ? What are they doing ?
> 

This fix uses Incomplete Periodic Transfer interrupt (GINTSTS_INCOMPL) to toggle between even and odd parity until the Transfer Complete interrupt is received. We also need to set correct parity on both IN and OUT endpoints.

> > > >  		hsotg->regs + GINTMSK);
> > > >
> > > >  	if (using_dma(hsotg))
> > > > @@ -2581,6 +2584,48 @@ irq_retry:
> > > >  		s3c_hsotg_dump(hsotg);
> > > >  	}
> > > >
> > > > +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> > > > +		u32 idx;
> > > > +		struct s3c_hsotg_ep *hs_ep;
> > > > +
> > > > +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n",
> > > __func__);
> > > > +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> 
> 			u32 epctl_reg;
> 			u32 ctrl;
> 
> > > > +			hs_ep = hsotg->eps_in[idx];
> 
> you can decrease some indentation here:
> 
> 			if (!hs_ep->isochronous)
> 				continue;
> 
> 			if (hs_ep->has_correct_parity)
> 				continue;
> 
> 			epctl_reg = DIEPCTL(idx);
> 			ctrl = readl(hsotg->regs + epctl_reg);
> 
> 			if (ctrl & DXEPCTL_EOFRNUM)
> 				ctrl |= DXEPCTL_SETEVENFR;
> 			else
> 				ctrl |= DXEPCTL_SETODDFR;
> 			writel(ctrl, hsotg->regs + epctl_reg);
> 
> 
> ditto to the other loop below
> 
> <snip>
> 

ok

> > > I'm not quite sure what the parity_set flag does in this patch.
> > > Shouldn't you be able to just toggle the even/odd frame when you get
> > > the interrupt?
> > >
> > > John
> > >
> >
> > When Transfer Complete interrupt is received, we have the correct
> > parity. Therefore we set the flag and we stop toggling. The parity_set
> > flag indicates whether we have the correct parity set.
> 
> then how about calling it has_correct_parity instead ?
> 

ok

> --
> balbi

Roman


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-08-25 22:00     ` Roman Bacik
  2015-08-25 22:35       ` Felipe Balbi
@ 2015-08-26  2:05       ` John Youn
  2015-08-26 14:44         ` Roman Bacik
  1 sibling, 1 reply; 8+ messages in thread
From: John Youn @ 2015-08-26  2:05 UTC (permalink / raw)
  To: Roman Bacik, John Youn, Scott Branden, Greg Kroah-Hartman, linux-usb
  Cc: linux-kernel, bcm-kernel-feedback-list

On 8/25/2015 3:00 PM, Roman Bacik wrote:
>> -----Original Message-----
>> From: John Youn [mailto:John.Youn@synopsys.com]
>> Sent: August-25-15 2:52 PM
>> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
>> usb@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list; Roman Bacik
>> Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
>>
>> On 8/18/2015 8:45 AM, Scott Branden wrote:
>>> From: Roman Bacik <rbacik@broadcom.com>
>>>
>>> USB OTG driver in isochronous mode has to set the parity of the
>>> receiving microframe. The parity is set to even by default. This
>>> causes problems for an audio gadget, if the host starts transmitting on odd
>> microframes.
>>>
>>> This fix uses Incomplete Periodic Transfer interrupt to toggle between
>>> even and odd parity until the Transfer Complete interrupt is received.
>>>
>>> Signed-off-by: Roman Bacik <rbacik@broadcom.com>
>>> Reviewed-by: Abhinav Ratna <aratna@broadcom.com>
>>> Reviewed-by: Srinath Mannam <srinath.mannam@broadcom.com>
>>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>>> Signed-off-by: Scott Branden <sbranden@broadcom.com>
>>> ---
>>>  drivers/usb/dwc2/core.h   |  1 +
>>>  drivers/usb/dwc2/gadget.c | 48
>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>>  drivers/usb/dwc2/hw.h     |  1 +
>>>  3 files changed, 49 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
>>> 0ed87620..954d1cd 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
>>>  	unsigned int            periodic:1;
>>>  	unsigned int            isochronous:1;
>>>  	unsigned int            send_zlp:1;
>>> +	unsigned int            parity_set:1;
>>>
>>>  	char                    name[10];
>>>  };
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 4d47b7c..28e4393 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct dwc2_hsotg
>> *hsotg, unsigned int idx,
>>>  		ints &= ~DXEPINT_XFERCOMPL;
>>>
>>>  	if (ints & DXEPINT_XFERCOMPL) {
>>> +		if (hs_ep->isochronous && !hs_ep->parity_set)
>>> +			hs_ep->parity_set = 1;
>>>  		if (hs_ep->isochronous && hs_ep->interval == 1) {
>>>  			if (ctrl & DXEPCTL_EOFRNUM)
>>>  				ctrl |= DXEPCTL_SETEVENFR;
>>> @@ -2316,7 +2318,8 @@ void s3c_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>>>  		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
>>>  		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
>>>  		GINTSTS_OTGINT | GINTSTS_USBSUSP |
>>> -		GINTSTS_WKUPINT,
>>> +		GINTSTS_WKUPINT |
>>> +		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,
>>>  		hsotg->regs + GINTMSK);
>>>
>>>  	if (using_dma(hsotg))
>>> @@ -2581,6 +2584,48 @@ irq_retry:
>>>  		s3c_hsotg_dump(hsotg);
>>>  	}
>>>
>>> +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
>>> +		u32 idx;
>>> +		struct s3c_hsotg_ep *hs_ep;
>>> +
>>> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n",
>> __func__);
>>> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
>>> +			hs_ep = hsotg->eps_in[idx];
>>> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
>>> +				u32 epctl_reg = DIEPCTL(idx);
>>> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
>>> +
>>> +				if (ctrl & DXEPCTL_EOFRNUM)
>>> +					ctrl |= DXEPCTL_SETEVENFR;
>>> +				else
>>> +					ctrl |= DXEPCTL_SETODDFR;
>>> +				writel(ctrl, hsotg->regs + epctl_reg);
>>> +			}
>>> +		}
>>> +		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
>>> +	}
>>> +
>>> +	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
>>> +		u32 idx;
>>> +		struct s3c_hsotg_ep *hs_ep;
>>> +
>>> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOOUT\n",
>> __func__);
>>> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
>>> +			hs_ep = hsotg->eps_out[idx];
>>> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
>>> +				u32 epctl_reg = DOEPCTL(idx);
>>> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
>>> +
>>> +				if (ctrl & DXEPCTL_EOFRNUM)
>>> +					ctrl |= DXEPCTL_SETEVENFR;
>>> +				else
>>> +					ctrl |= DXEPCTL_SETODDFR;
>>> +				writel(ctrl, hsotg->regs + epctl_reg);
>>> +			}
>>> +		}
>>> +		writel(GINTSTS_INCOMPL_SOOUT, hsotg->regs + GINTSTS);
>>> +	}
>>> +
>>>  	/*
>>>  	 * if we've had fifo events, we should try and go around the
>>>  	 * loop again to see if there's any point in returning yet.
>>> @@ -2667,6 +2712,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep
>> *ep,
>>>  	hs_ep->periodic = 0;
>>>  	hs_ep->halted = 0;
>>>  	hs_ep->interval = desc->bInterval;
>>> +	hs_ep->parity_set = 0;
>>
>>
>> I'm not quite sure what the parity_set flag does in this patch.
>> Shouldn't you be able to just toggle the even/odd frame when you get the
>> interrupt?
>>
>> John
>>
> 
> When Transfer Complete interrupt is received, we have the correct parity. Therefore we set the flag and we stop toggling. The parity_set flag indicates whether we have the correct parity set.
> Regards,
> 
> Roman
> 

I'm not sure that "parity" is the proper term in this context but
I can't think of a more concise way to phrase it.

What if the host switches again some time after the first xfer
complete?

What function or gadget driver are you using to test this? Did
you test both the ISO IN and OUT case?

John




^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-08-26  2:05       ` John Youn
@ 2015-08-26 14:44         ` Roman Bacik
  0 siblings, 0 replies; 8+ messages in thread
From: Roman Bacik @ 2015-08-26 14:44 UTC (permalink / raw)
  To: John Youn, Scott Branden, Greg Kroah-Hartman, linux-usb
  Cc: linux-kernel, bcm-kernel-feedback-list

> -----Original Message-----
> From: John Youn [mailto:John.Youn@synopsys.com]
> Sent: August-25-15 7:06 PM
> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list
> Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> 
> On 8/25/2015 3:00 PM, Roman Bacik wrote:
> >> -----Original Message-----
> >> From: John Youn [mailto:John.Youn@synopsys.com]
> >> Sent: August-25-15 2:52 PM
> >> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> >> usb@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list; Roman
> >> Bacik
> >> Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous
> >> mode
> >>
> >> On 8/18/2015 8:45 AM, Scott Branden wrote:
> >>> From: Roman Bacik <rbacik@broadcom.com>
> >>>
> >>> USB OTG driver in isochronous mode has to set the parity of the
> >>> receiving microframe. The parity is set to even by default. This
> >>> causes problems for an audio gadget, if the host starts transmitting
> >>> on odd
> >> microframes.
> >>>
> >>> This fix uses Incomplete Periodic Transfer interrupt to toggle
> >>> between even and odd parity until the Transfer Complete interrupt is
> received.
> >>>
> >>> Signed-off-by: Roman Bacik <rbacik@broadcom.com>
> >>> Reviewed-by: Abhinav Ratna <aratna@broadcom.com>
> >>> Reviewed-by: Srinath Mannam <srinath.mannam@broadcom.com>
> >>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> >>> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> >>> ---
> >>>  drivers/usb/dwc2/core.h   |  1 +
> >>>  drivers/usb/dwc2/gadget.c | 48
> >> ++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  drivers/usb/dwc2/hw.h     |  1 +
> >>>  3 files changed, 49 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
> >>> 0ed87620..954d1cd 100644
> >>> --- a/drivers/usb/dwc2/core.h
> >>> +++ b/drivers/usb/dwc2/core.h
> >>> @@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
> >>>  	unsigned int            periodic:1;
> >>>  	unsigned int            isochronous:1;
> >>>  	unsigned int            send_zlp:1;
> >>> +	unsigned int            parity_set:1;
> >>>
> >>>  	char                    name[10];
> >>>  };
> >>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> >>> index 4d47b7c..28e4393 100644
> >>> --- a/drivers/usb/dwc2/gadget.c
> >>> +++ b/drivers/usb/dwc2/gadget.c
> >>> @@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct dwc2_hsotg
> >> *hsotg, unsigned int idx,
> >>>  		ints &= ~DXEPINT_XFERCOMPL;
> >>>
> >>>  	if (ints & DXEPINT_XFERCOMPL) {
> >>> +		if (hs_ep->isochronous && !hs_ep->parity_set)
> >>> +			hs_ep->parity_set = 1;
> >>>  		if (hs_ep->isochronous && hs_ep->interval == 1) {
> >>>  			if (ctrl & DXEPCTL_EOFRNUM)
> >>>  				ctrl |= DXEPCTL_SETEVENFR;
> >>> @@ -2316,7 +2318,8 @@ void s3c_hsotg_core_init_disconnected(struct
> >> dwc2_hsotg *hsotg,
> >>>  		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
> >>>  		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
> >>>  		GINTSTS_OTGINT | GINTSTS_USBSUSP |
> >>> -		GINTSTS_WKUPINT,
> >>> +		GINTSTS_WKUPINT |
> >>> +		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,
> >>>  		hsotg->regs + GINTMSK);
> >>>
> >>>  	if (using_dma(hsotg))
> >>> @@ -2581,6 +2584,48 @@ irq_retry:
> >>>  		s3c_hsotg_dump(hsotg);
> >>>  	}
> >>>
> >>> +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> >>> +		u32 idx;
> >>> +		struct s3c_hsotg_ep *hs_ep;
> >>> +
> >>> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n",
> >> __func__);
> >>> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> >>> +			hs_ep = hsotg->eps_in[idx];
> >>> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
> >>> +				u32 epctl_reg = DIEPCTL(idx);
> >>> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
> >>> +
> >>> +				if (ctrl & DXEPCTL_EOFRNUM)
> >>> +					ctrl |= DXEPCTL_SETEVENFR;
> >>> +				else
> >>> +					ctrl |= DXEPCTL_SETODDFR;
> >>> +				writel(ctrl, hsotg->regs + epctl_reg);
> >>> +			}
> >>> +		}
> >>> +		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
> >>> +	}
> >>> +
> >>> +	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
> >>> +		u32 idx;
> >>> +		struct s3c_hsotg_ep *hs_ep;
> >>> +
> >>> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOOUT\n",
> >> __func__);
> >>> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> >>> +			hs_ep = hsotg->eps_out[idx];
> >>> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
> >>> +				u32 epctl_reg = DOEPCTL(idx);
> >>> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
> >>> +
> >>> +				if (ctrl & DXEPCTL_EOFRNUM)
> >>> +					ctrl |= DXEPCTL_SETEVENFR;
> >>> +				else
> >>> +					ctrl |= DXEPCTL_SETODDFR;
> >>> +				writel(ctrl, hsotg->regs + epctl_reg);
> >>> +			}
> >>> +		}
> >>> +		writel(GINTSTS_INCOMPL_SOOUT, hsotg->regs + GINTSTS);
> >>> +	}
> >>> +
> >>>  	/*
> >>>  	 * if we've had fifo events, we should try and go around the
> >>>  	 * loop again to see if there's any point in returning yet.
> >>> @@ -2667,6 +2712,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep
> >> *ep,
> >>>  	hs_ep->periodic = 0;
> >>>  	hs_ep->halted = 0;
> >>>  	hs_ep->interval = desc->bInterval;
> >>> +	hs_ep->parity_set = 0;
> >>
> >>
> >> I'm not quite sure what the parity_set flag does in this patch.
> >> Shouldn't you be able to just toggle the even/odd frame when you get
> >> the interrupt?
> >>
> >> John
> >>
> >
> > When Transfer Complete interrupt is received, we have the correct parity.
> Therefore we set the flag and we stop toggling. The parity_set flag indicates
> whether we have the correct parity set.
> > Regards,
> >
> > Roman
> >
> 
> I'm not sure that "parity" is the proper term in this context but I can't think of
> a more concise way to phrase it.
> 
> What if the host switches again some time after the first xfer complete?
> 
> What function or gadget driver are you using to test this? Did you test both
> the ISO IN and OUT case?
> 
> John
> 
> 

This patch is related to isochronous or periodic transfers. So server uses the same microframe number and hence parity for transfers. I tested both IN and OUT directions using audio gadget on Altera SocKit and Broadcom development boards.

Regards,

Roman

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-08-26 14:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-18 15:45 [PATCH 0/1] USB DWC2 parity fix in isochronous mode Scott Branden
2015-08-18 15:45 ` [PATCH 1/1] usb: dwc2: gadget: " Scott Branden
2015-08-25 21:51   ` John Youn
2015-08-25 22:00     ` Roman Bacik
2015-08-25 22:35       ` Felipe Balbi
2015-08-25 23:00         ` Roman Bacik
2015-08-26  2:05       ` John Youn
2015-08-26 14:44         ` Roman Bacik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).