RE: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode

[Roman Bacik]

> -----Original Message-----
> From: John Youn [mailto:John.Youn@xxxxxxxxxxxx]
> Sent: September-24-15 8:16 PM
> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman; linux-
> usb@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxx]
> >> Sent: September-23-15 9:21 PM
> >> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> >> usb@xxxxxxxxxxxxxxx; Roman Bacik
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/