Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1216295 > unrolled thread

[PATCH v2 0/1] USB DWC2 parity fix in isochronous mode

Started byScott Branden <sbranden@broadcom.com>
First post2015-08-31 18:20 +0200
Last post2015-09-04 09:00 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/1] USB DWC2 parity fix in isochronous mode Scott Branden <sbranden@broadcom.com> - 2015-08-31 18:20 +0200
    [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode Scott Branden <sbranden@broadcom.com> - 2015-08-31 18:20 +0200
      RE: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode Roman Bacik <rbacik@broadcom.com> - 2015-09-03 20:50 +0200
      Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode John Youn <John.Youn@synopsys.com> - 2015-09-04 09:00 +0200

#1216295 — [PATCH v2 0/1] USB DWC2 parity fix in isochronous mode

FromScott Branden <sbranden@broadcom.com>
Date2015-08-31 18:20 +0200
Subject[PATCH v2 0/1] USB DWC2 parity fix in isochronous mode
Message-ID<q3zYl-4sz-9@gated-at.bofh.it>
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.

Changes from v1:
 - Address code review comments as per previous responses:
 - renamed parity_set to has_correct_parity and reorder some logic


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

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

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1216301 — [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode

FromScott Branden <sbranden@broadcom.com>
Date2015-08-31 18:20 +0200
Subject[PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode
Message-ID<q3zYn-4sz-39@gated-at.bofh.it>
In reply to#1216295
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>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/dwc2/hw.h     |  1 +
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 0ed87620..a5634fd 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            has_correct_parity:1;
 
 	char                    name[10];
 };
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 4d47b7c..fac3e2f 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1954,6 +1954,7 @@ static void s3c_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx,
 		ints &= ~DXEPINT_XFERCOMPL;
 
 	if (ints & DXEPINT_XFERCOMPL) {
+		hs_ep->has_correct_parity = 1;
 		if (hs_ep->isochronous && hs_ep->interval == 1) {
 			if (ctrl & DXEPCTL_EOFRNUM)
 				ctrl |= DXEPCTL_SETEVENFR;
@@ -2316,7 +2317,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 +2583,52 @@ irq_retry:
 		s3c_hsotg_dump(hsotg);
 	}
 
+	if (gintsts & GINTSTS_INCOMPL_SOIN) {
+		u32 idx, epctl_reg, ctrl;
+		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->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);
+		}
+		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
+	}
+
+	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
+		u32 idx, epctl_reg, ctrl;
+		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->has_correct_parity)
+				continue;
+
+			epctl_reg = DOEPCTL(idx);
+			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 +2715,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->has_correct_parity = 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.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218512 — RE: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode

FromRoman Bacik <rbacik@broadcom.com>
Date2015-09-03 20:50 +0200
SubjectRE: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode
Message-ID<q4HKa-3tz-29@gated-at.bofh.it>
In reply to#1216301
Is there anything else we should address in this patch?
Thanks,

Roman

> -----Original Message-----
> From: Scott Branden [mailto:sbranden@broadcom.com]
> Sent: August-31-15 9:17 AM
> To: John Youn; Greg Kroah-Hartman; linux-usb@vger.kernel.org; Roman
> Bacik
> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list; Scott Branden
> Subject: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> 
> 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>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 51
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc2/hw.h     |  1 +
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
> 0ed87620..a5634fd 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            has_correct_parity:1;
> 
>  	char                    name[10];
>  };
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
> 4d47b7c..fac3e2f 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1954,6 +1954,7 @@ static void s3c_hsotg_epint(struct dwc2_hsotg
> *hsotg, unsigned int idx,
>  		ints &= ~DXEPINT_XFERCOMPL;
> 
>  	if (ints & DXEPINT_XFERCOMPL) {
> +		hs_ep->has_correct_parity = 1;
>  		if (hs_ep->isochronous && hs_ep->interval == 1) {
>  			if (ctrl & DXEPCTL_EOFRNUM)
>  				ctrl |= DXEPCTL_SETEVENFR;
> @@ -2316,7 +2317,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 +2583,52 @@ irq_retry:
>  		s3c_hsotg_dump(hsotg);
>  	}
> 
> +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> +		u32 idx, epctl_reg, ctrl;
> +		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-
> >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);
> +		}
> +		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
> +	}
> +
> +	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
> +		u32 idx, epctl_reg, ctrl;
> +		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-
> >has_correct_parity)
> +				continue;
> +
> +			epctl_reg = DOEPCTL(idx);
> +			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 +2715,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->has_correct_parity = 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.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218691 — Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode

FromJohn Youn <John.Youn@synopsys.com>
Date2015-09-04 09:00 +0200
SubjectRe: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode
Message-ID<q4T8C-2Tq-9@gated-at.bofh.it>
In reply to#1216301
On 8/31/2015 9:17 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>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc2/hw.h     |  1 +
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 0ed87620..a5634fd 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            has_correct_parity:1;
>  
>  	char                    name[10];
>  };
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 4d47b7c..fac3e2f 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1954,6 +1954,7 @@ static void s3c_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx,
>  		ints &= ~DXEPINT_XFERCOMPL;
>  
>  	if (ints & DXEPINT_XFERCOMPL) {
> +		hs_ep->has_correct_parity = 1;
>  		if (hs_ep->isochronous && hs_ep->interval == 1) {
>  			if (ctrl & DXEPCTL_EOFRNUM)
>  				ctrl |= DXEPCTL_SETEVENFR;
> @@ -2316,7 +2317,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 +2583,52 @@ irq_retry:
>  		s3c_hsotg_dump(hsotg);
>  	}
>  
> +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> +		u32 idx, epctl_reg, ctrl;
> +		struct s3c_hsotg_ep *hs_ep;
> +
> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n", __func__);
> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {

Valid endpoints are only up to hsotg->num_of_eps so this might
crash on certain configurations.

Also, have you tried to find the endpoint which caused the
incomplete interrupt by reading the control registers as
described in the databook?

> +			hs_ep = hsotg->eps_in[idx];
> +
> +			if (!hs_ep->isochronous || 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);

This toggling code could be moved out to a helper function to
reduce redundancy as it is in three places now.

Regards,
John

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web