All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
@ 2015-09-11  1:13 Scott Branden
  2015-09-11  1:13 ` [PATCH v3 1/1] usb: dwc2: gadget: " Scott Branden
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Scott Branden @ 2015-09-11  1:13 UTC (permalink / raw)
  To: John Youn, Greg Kroah-Hartman, linux-usb, Roman Bacik
  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.

Changes from v2:
 - created s2c_hsotg_chage_ep_iso_parity function to call function in 3 places in code
 - used hsotg->num_of_eps instead of MAX_EPS_CHANNELS

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 | 58 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/usb/dwc2/hw.h     |  1 +
 3 files changed, 54 insertions(+), 6 deletions(-)

-- 
2.5.0


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

* [PATCH v3 1/1] usb: dwc2: gadget: parity fix in isochronous mode
  2015-09-11  1:13 [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode Scott Branden
@ 2015-09-11  1:13 ` Scott Branden
  2015-09-23  7:35 ` [PATCH v3 0/1] USB DWC2 " Scott Branden
  2015-09-24  4:21 ` John Youn
  2 siblings, 0 replies; 11+ messages in thread
From: Scott Branden @ 2015-09-11  1:13 UTC (permalink / raw)
  To: John Youn, Greg Kroah-Hartman, linux-usb, Roman Bacik
  Cc: linux-kernel, bcm-kernel-feedback-list, 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>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 58 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/usb/dwc2/hw.h     |  1 +
 3 files changed, 54 insertions(+), 6 deletions(-)

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..efaab76 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1916,6 +1916,19 @@ static void s3c_hsotg_complete_in(struct dwc2_hsotg *hsotg,
 	s3c_hsotg_complete_request(hsotg, hs_ep, hs_req, 0);
 }
 
+static void s3c_hsotg_change_ep_iso_parity(struct dwc2_hsotg *hsotg,
+			u32 epctl_reg)
+{
+	u32 ctrl;
+
+	ctrl = readl(hsotg->regs + epctl_reg);
+	if (ctrl & DXEPCTL_EOFRNUM)
+		ctrl |= DXEPCTL_SETEVENFR;
+	else
+		ctrl |= DXEPCTL_SETODDFR;
+	writel(ctrl, hsotg->regs + epctl_reg);
+}
+
 /**
  * s3c_hsotg_epint - handle an in/out endpoint interrupt
  * @hsotg: The driver state
@@ -1954,12 +1967,9 @@ 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;
-			else
-				ctrl |= DXEPCTL_SETODDFR;
-			writel(ctrl, hsotg->regs + epctl_reg);
+			s3c_hsotg_change_ep_iso_parity(hsotg, epctl_reg);
 		}
 
 		dev_dbg(hsotg->dev,
@@ -2316,7 +2326,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 +2592,40 @@ irq_retry:
 		s3c_hsotg_dump(hsotg);
 	}
 
+	if (gintsts & GINTSTS_INCOMPL_SOIN) {
+		u32 idx, epctl_reg;
+		struct s3c_hsotg_ep *hs_ep;
+
+		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n", __func__);
+		for (idx = 1; idx < hsotg->num_of_eps; idx++) {
+			hs_ep = hsotg->eps_in[idx];
+
+			if (!hs_ep->isochronous || hs_ep->has_correct_parity)
+				continue;
+
+			epctl_reg = DIEPCTL(idx);
+			s3c_hsotg_change_ep_iso_parity(hsotg, epctl_reg);
+		}
+		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
+	}
+
+	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
+		u32 idx, epctl_reg;
+		struct s3c_hsotg_ep *hs_ep;
+
+		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOOUT\n", __func__);
+		for (idx = 1; idx < hsotg->num_of_eps; idx++) {
+			hs_ep = hsotg->eps_out[idx];
+
+			if (!hs_ep->isochronous || hs_ep->has_correct_parity)
+				continue;
+
+			epctl_reg = DOEPCTL(idx);
+			s3c_hsotg_change_ep_iso_parity(hsotg, 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->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


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

* Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-11  1:13 [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode Scott Branden
  2015-09-11  1:13 ` [PATCH v3 1/1] usb: dwc2: gadget: " Scott Branden
@ 2015-09-23  7:35 ` Scott Branden
  2015-09-23  8:11   ` John Youn
  2015-09-24  4:21 ` John Youn
  2 siblings, 1 reply; 11+ messages in thread
From: Scott Branden @ 2015-09-23  7:35 UTC (permalink / raw)
  To: John Youn, Greg Kroah-Hartman, linux-usb, Roman Bacik
  Cc: linux-kernel, bcm-kernel-feedback-list

Hi John,

Could you please review the v3 Patch.  I believe we have address all of 
your comments?

On 15-09-10 06:13 PM, Scott Branden wrote:
> 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 v2:
>   - created s2c_hsotg_chage_ep_iso_parity function to call function in 3 places in code
>   - used hsotg->num_of_eps instead of MAX_EPS_CHANNELS
>
> 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 | 58 ++++++++++++++++++++++++++++++++++++++++++-----
>   drivers/usb/dwc2/hw.h     |  1 +
>   3 files changed, 54 insertions(+), 6 deletions(-)
>


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

* Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-23  7:35 ` [PATCH v3 0/1] USB DWC2 " Scott Branden
@ 2015-09-23  8:11   ` John Youn
  2015-09-23  8:18     ` John Youn
  0 siblings, 1 reply; 11+ messages in thread
From: John Youn @ 2015-09-23  8:11 UTC (permalink / raw)
  To: Scott Branden, John Youn, Greg Kroah-Hartman, linux-usb, Roman Bacik
  Cc: linux-kernel, bcm-kernel-feedback-list

On 9/23/2015 12:36 AM, Scott Branden wrote:
> Hi John,
> 
> Could you please review the v3 Patch.  I believe we have address all of 
> your comments?
> 

Yes I've been meaning to test it on our platforms. I should be
able to get to it tomorrow.

Regards,
John


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

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

On 9/23/2015 1:11 AM, John Youn wrote:
> On 9/23/2015 12:36 AM, Scott Branden wrote:
>> Hi John,
>>
>> Could you please review the v3 Patch.  I believe we have address all of 
>> your comments?
>>
> 
> Yes I've been meaning to test it on our platforms. I should be
> able to get to it tomorrow.
> 

Also if you could rebase off of Felipe's testing/next branch it
would make it easier when it comes time to merge it in.

Regards,
John


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

* Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-23  8:18     ` John Youn
@ 2015-09-23 15:39       ` Scott Branden
  2015-09-23 18:05         ` John Youn
  0 siblings, 1 reply; 11+ messages in thread
From: Scott Branden @ 2015-09-23 15:39 UTC (permalink / raw)
  To: John Youn, Greg Kroah-Hartman, linux-usb, Roman Bacik
  Cc: linux-kernel, bcm-kernel-feedback-list

On 15-09-23 01:18 AM, John Youn wrote:
> On 9/23/2015 1:11 AM, John Youn wrote:
>> On 9/23/2015 12:36 AM, Scott Branden wrote:
>>> Hi John,
>>>
>>> Could you please review the v3 Patch.  I believe we have address all of
>>> your comments?
>>>
>>
>> Yes I've been meaning to test it on our platforms. I should be
>> able to get to it tomorrow.
>>
>
> Also if you could rebase off of Felipe's testing/next branch it
> would make it easier when it comes time to merge it in.

Could you point me at Felipe's git repo as I am unfamiliar with the 
location?

Thanks,
  Scott

>
> Regards,
> John
>


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

* Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-23 15:39       ` Scott Branden
@ 2015-09-23 18:05         ` John Youn
  0 siblings, 0 replies; 11+ messages in thread
From: John Youn @ 2015-09-23 18:05 UTC (permalink / raw)
  To: Scott Branden, John Youn, Greg Kroah-Hartman, linux-usb, Roman Bacik
  Cc: linux-kernel, bcm-kernel-feedback-list

On 9/23/2015 8:39 AM, Scott Branden wrote:
> On 15-09-23 01:18 AM, John Youn wrote:
>> On 9/23/2015 1:11 AM, John Youn wrote:
>>> On 9/23/2015 12:36 AM, Scott Branden wrote:
>>>> Hi John,
>>>>
>>>> Could you please review the v3 Patch.  I believe we have address all of
>>>> your comments?
>>>>
>>>
>>> Yes I've been meaning to test it on our platforms. I should be
>>> able to get to it tomorrow.
>>>
>>
>> Also if you could rebase off of Felipe's testing/next branch it
>> would make it easier when it comes time to merge it in.
> 
> Could you point me at Felipe's git repo as I am unfamiliar with the 
> location?
> 

git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git


Regards,
John


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

* Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-11  1:13 [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode Scott Branden
  2015-09-11  1:13 ` [PATCH v3 1/1] usb: dwc2: gadget: " Scott Branden
  2015-09-23  7:35 ` [PATCH v3 0/1] USB DWC2 " Scott Branden
@ 2015-09-24  4:21 ` John Youn
  2015-09-24 17:28   ` Roman Bacik
  2 siblings, 1 reply; 11+ messages in thread
From: John Youn @ 2015-09-24  4:21 UTC (permalink / raw)
  To: Scott Branden, John Youn, Greg Kroah-Hartman, linux-usb, Roman Bacik
  Cc: linux-kernel, bcm-kernel-feedback-list

On 9/10/2015 6:14 PM, Scott Branden wrote:
> 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 v2:
>  - created s2c_hsotg_chage_ep_iso_parity function to call function in 3 places in code
>  - used hsotg->num_of_eps instead of MAX_EPS_CHANNELS
> 
> 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 | 58 ++++++++++++++++++++++++++++++++++++++++++-----
>  drivers/usb/dwc2/hw.h     |  1 +
>  3 files changed, 54 insertions(+), 6 deletions(-)
> 

This seems to break slave mode on my platform. It seems to be
dropping a lot of packets. I tried bInterval=4,5,6, ISO OUT.
I'll need to take a closer look to determine why. Probably later
this week.

Are you able to run in slave mode on your platform? If so can you
try it out?

Regards,
John


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

* RE: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-24  4:21 ` John Youn
@ 2015-09-24 17:28   ` Roman Bacik
  2015-09-25  3:15     ` John Youn
  0 siblings, 1 reply; 11+ messages in thread
From: Roman Bacik @ 2015-09-24 17:28 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: September-23-15 9:21 PM
> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org; Roman Bacik
> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list
> Subject: Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
> 
> On 9/10/2015 6:14 PM, Scott Branden wrote:
> > 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 v2:
> >  - created s2c_hsotg_chage_ep_iso_parity function to call function in
> > 3 places in code
> >  - used hsotg->num_of_eps instead of MAX_EPS_CHANNELS
> >
> > 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 | 58
> ++++++++++++++++++++++++++++++++++++++++++-----
> >  drivers/usb/dwc2/hw.h     |  1 +
> >  3 files changed, 54 insertions(+), 6 deletions(-)
> >
> 
> This seems to break slave mode on my platform. It seems to be dropping a
> lot of packets. I tried bInterval=4,5,6, ISO OUT.
> I'll need to take a closer look to determine why. Probably later this week.
> 
> Are you able to run in slave mode on your platform? If so can you try it out?
> 
> Regards,
> John

Our current test procedure is as follows:

Build Linux kernel with: 

Device Drivers
    - Sound Card Support 'SOUND=y': ALSA 'SND=y'
        - Generic sound devices: Dummy (/dev/null) soundcard 'SND_DUMMY=y'
    - USB support 'USB_SUPPORT=y':
        DesignWare USB2 DRD Core support 'USB_DWC2=y'
            - Gadget only mode 'USB_DWC2_PERIPHERAL=y'
        DWC2 Platform 'USB_DWC2_PLATFORM=y'
        - USB Gadget Support 'USB_GADGET=y': Audio Gadget 'USB_AUDIO=y'
    - PHY Subsystem: Broadcom Kona USB2 PHY Driver 'BCM_KONA_USB2_PHY=y'

If you have only even hosts, you can change the default micro frame parity to odd:

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index ad45b0b..80bde75 100644
@@ -2709,7 +2709,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
     switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
     case USB_ENDPOINT_XFER_ISOC:
         epctrl |= DXEPCTL_EPTYPE_ISO;
-        epctrl |= DXEPCTL_SETEVENFR;
+        epctrl |= DXEPCTL_SETODDFR;
         hs_ep->isochronous = 1;
         if (dir_in)
             hs_ep->periodic = 1;
@@ -2777,7 +2777,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
  
     /* for non control endpoints, set PID to D0 */
     if (index)
-        epctrl |= DXEPCTL_SETD0PID;
+        epctrl |= DXEPCTL_SETD1PID;
  
     dev_dbg(hsotg->dev, "%s: write DxEPCTL=0x%08x\n",
         __func__, epctrl);

To test OUT direction:
Host:
aplay -D plughw:2 -r 48000 -f S16_LE tone_48stereo.wav
Device:
arecord -D plughw:0 -r 48000 -f S16_LE -c 1 /tmp/rec.pcm

To test IN direction:
Host:
arecord -D plughw:2 -r 48000 -f S16_LE -c 1 /tmp/rec.pcm
Device:
aplay -D plughw:0 -r 48000 -f S16_LE /tmp/rec.pcm

If you would like, we can try to test your procedure provided you send us enough details.
Regards,

Roman


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

* Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-24 17:28   ` Roman Bacik
@ 2015-09-25  3:15     ` John Youn
  2015-09-28 15:27       ` Roman Bacik
  0 siblings, 1 reply; 11+ messages in thread
From: John Youn @ 2015-09-25  3:15 UTC (permalink / raw)
  To: Roman Bacik, John Youn, Scott Branden, Greg Kroah-Hartman, linux-usb
  Cc: linux-kernel, bcm-kernel-feedback-list

On 9/24/2015 10:28 AM, Roman Bacik wrote:
>> -----Original Message-----
>> From: John Youn [mailto:John.Youn@synopsys.com]
>> Sent: September-23-15 9:21 PM
>> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
>> usb@vger.kernel.org; Roman Bacik
>> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list
>> Subject: Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
>>
>> On 9/10/2015 6:14 PM, Scott Branden wrote:
>>> 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 v2:
>>>  - created s2c_hsotg_chage_ep_iso_parity function to call function in
>>> 3 places in code
>>>  - used hsotg->num_of_eps instead of MAX_EPS_CHANNELS
>>>
>>> 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 | 58
>> ++++++++++++++++++++++++++++++++++++++++++-----
>>>  drivers/usb/dwc2/hw.h     |  1 +
>>>  3 files changed, 54 insertions(+), 6 deletions(-)
>>>
>>
>> This seems to break slave mode on my platform. It seems to be dropping a
>> lot of packets. I tried bInterval=4,5,6, ISO OUT.
>> I'll need to take a closer look to determine why. Probably later this week.
>>
>> Are you able to run in slave mode on your platform? If so can you try it out?
>>
>> Regards,
>> John
> 
> Our current test procedure is as follows:
> 
> Build Linux kernel with: 
> 
> Device Drivers
>     - Sound Card Support 'SOUND=y': ALSA 'SND=y'
>         - Generic sound devices: Dummy (/dev/null) soundcard 'SND_DUMMY=y'
>     - USB support 'USB_SUPPORT=y':
>         DesignWare USB2 DRD Core support 'USB_DWC2=y'
>             - Gadget only mode 'USB_DWC2_PERIPHERAL=y'
>         DWC2 Platform 'USB_DWC2_PLATFORM=y'
>         - USB Gadget Support 'USB_GADGET=y': Audio Gadget 'USB_AUDIO=y'
>     - PHY Subsystem: Broadcom Kona USB2 PHY Driver 'BCM_KONA_USB2_PHY=y'
> 
> If you have only even hosts, you can change the default micro frame parity to odd:
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index ad45b0b..80bde75 100644
> @@ -2709,7 +2709,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
>      switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
>      case USB_ENDPOINT_XFER_ISOC:
>          epctrl |= DXEPCTL_EPTYPE_ISO;
> -        epctrl |= DXEPCTL_SETEVENFR;
> +        epctrl |= DXEPCTL_SETODDFR;
>          hs_ep->isochronous = 1;
>          if (dir_in)
>              hs_ep->periodic = 1;
> @@ -2777,7 +2777,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
>   
>      /* for non control endpoints, set PID to D0 */
>      if (index)
> -        epctrl |= DXEPCTL_SETD0PID;
> +        epctrl |= DXEPCTL_SETD1PID;
>   
>      dev_dbg(hsotg->dev, "%s: write DxEPCTL=0x%08x\n",
>          __func__, epctrl);
> 
> To test OUT direction:
> Host:
> aplay -D plughw:2 -r 48000 -f S16_LE tone_48stereo.wav
> Device:
> arecord -D plughw:0 -r 48000 -f S16_LE -c 1 /tmp/rec.pcm
> 
> To test IN direction:
> Host:
> arecord -D plughw:2 -r 48000 -f S16_LE -c 1 /tmp/rec.pcm
> Device:
> aplay -D plughw:0 -r 48000 -f S16_LE /tmp/rec.pcm
> 
> If you would like, we can try to test your procedure provided you send us enough details.
> Regards,
> 
> Roman
> 
> 

I looked at it a bit more and I think there are two issues.

In slave-mode, the xfercomplete interrupt is not used for OUT
transfers, so the has_correct_parity flag is never set.

The other issue is that we occasionally get the incomplete
interrupt twice in a microframe causing the parity to be set
incorrectly for the next frame. Thus we will continuously miss
until this occurs again, correcting it.

These two problems in combination cause dropped packets
throughout operation.

If you give me some time I can see if I can fix up this patch to
work with slave mode.

I've tried all our hosts here and they are all even hosts, so I
tried flipping the default parity. The issue exists in either
case.

Regards,
John



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

* RE: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
  2015-09-25  3:15     ` John Youn
@ 2015-09-28 15:27       ` Roman Bacik
  0 siblings, 0 replies; 11+ messages in thread
From: Roman Bacik @ 2015-09-28 15:27 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: September-24-15 8:16 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 v3 0/1] USB DWC2 parity fix in isochronous mode
> 
> On 9/24/2015 10:28 AM, Roman Bacik wrote:
> >> -----Original Message-----
> >> From: John Youn [mailto:John.Youn@synopsys.com]
> >> Sent: September-23-15 9:21 PM
> >> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> >> usb@vger.kernel.org; Roman Bacik
> >> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list
> >> Subject: Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
> >>
> >> On 9/10/2015 6:14 PM, Scott Branden wrote:
> >>> 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 v2:
> >>>  - created s2c_hsotg_chage_ep_iso_parity function to call function
> >>> in
> >>> 3 places in code
> >>>  - used hsotg->num_of_eps instead of MAX_EPS_CHANNELS
> >>>
> >>> 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 | 58
> >> ++++++++++++++++++++++++++++++++++++++++++-----
> >>>  drivers/usb/dwc2/hw.h     |  1 +
> >>>  3 files changed, 54 insertions(+), 6 deletions(-)
> >>>
> >>
> >> This seems to break slave mode on my platform. It seems to be
> >> dropping a lot of packets. I tried bInterval=4,5,6, ISO OUT.
> >> I'll need to take a closer look to determine why. Probably later this week.
> >>
> >> Are you able to run in slave mode on your platform? If so can you try it
> out?
> >>
> >> Regards,
> >> John
> >
> > Our current test procedure is as follows:
> >
> > Build Linux kernel with:
> >
> > Device Drivers
> >     - Sound Card Support 'SOUND=y': ALSA 'SND=y'
> >         - Generic sound devices: Dummy (/dev/null) soundcard
> 'SND_DUMMY=y'
> >     - USB support 'USB_SUPPORT=y':
> >         DesignWare USB2 DRD Core support 'USB_DWC2=y'
> >             - Gadget only mode 'USB_DWC2_PERIPHERAL=y'
> >         DWC2 Platform 'USB_DWC2_PLATFORM=y'
> >         - USB Gadget Support 'USB_GADGET=y': Audio Gadget 'USB_AUDIO=y'
> >     - PHY Subsystem: Broadcom Kona USB2 PHY Driver
> 'BCM_KONA_USB2_PHY=y'
> >
> > If you have only even hosts, you can change the default micro frame parity
> to odd:
> >
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index ad45b0b..80bde75 100644 @@ -2709,7 +2709,7 @@ static int
> > s3c_hsotg_ep_enable(struct usb_ep *ep,
> >      switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
> >      case USB_ENDPOINT_XFER_ISOC:
> >          epctrl |= DXEPCTL_EPTYPE_ISO;
> > -        epctrl |= DXEPCTL_SETEVENFR;
> > +        epctrl |= DXEPCTL_SETODDFR;
> >          hs_ep->isochronous = 1;
> >          if (dir_in)
> >              hs_ep->periodic = 1;
> > @@ -2777,7 +2777,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep
> > *ep,
> >
> >      /* for non control endpoints, set PID to D0 */
> >      if (index)
> > -        epctrl |= DXEPCTL_SETD0PID;
> > +        epctrl |= DXEPCTL_SETD1PID;
> >
> >      dev_dbg(hsotg->dev, "%s: write DxEPCTL=0x%08x\n",
> >          __func__, epctrl);
> >
> > To test OUT direction:
> > Host:
> > aplay -D plughw:2 -r 48000 -f S16_LE tone_48stereo.wav
> > Device:
> > arecord -D plughw:0 -r 48000 -f S16_LE -c 1 /tmp/rec.pcm
> >
> > To test IN direction:
> > Host:
> > arecord -D plughw:2 -r 48000 -f S16_LE -c 1 /tmp/rec.pcm
> > Device:
> > aplay -D plughw:0 -r 48000 -f S16_LE /tmp/rec.pcm
> >
> > If you would like, we can try to test your procedure provided you send us
> enough details.
> > Regards,
> >
> > Roman
> >
> >
> 
> I looked at it a bit more and I think there are two issues.
> 
> In slave-mode, the xfercomplete interrupt is not used for OUT transfers, so
> the has_correct_parity flag is never set.
> 
> The other issue is that we occasionally get the incomplete interrupt twice in a
> microframe causing the parity to be set incorrectly for the next frame. Thus
> we will continuously miss until this occurs again, correcting it.
> 
> These two problems in combination cause dropped packets throughout
> operation.
> 
> If you give me some time I can see if I can fix up this patch to work with slave
> mode.
> 
> I've tried all our hosts here and they are all even hosts, so I tried flipping the
> default parity. The issue exists in either case.
> 
> Regards,
> John
> 

Is there a way to recognize we are in slave-mode that we would not flip parity in that case?
Thanks,

Roman

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

end of thread, other threads:[~2015-09-28 15:28 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-11  1:13 [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode Scott Branden
2015-09-11  1:13 ` [PATCH v3 1/1] usb: dwc2: gadget: " Scott Branden
2015-09-23  7:35 ` [PATCH v3 0/1] USB DWC2 " Scott Branden
2015-09-23  8:11   ` John Youn
2015-09-23  8:18     ` John Youn
2015-09-23 15:39       ` Scott Branden
2015-09-23 18:05         ` John Youn
2015-09-24  4:21 ` John Youn
2015-09-24 17:28   ` Roman Bacik
2015-09-25  3:15     ` John Youn
2015-09-28 15:27       ` Roman Bacik

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.