RE: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
> -Original Message-
> From: John Youn [mailto:john.y...@synopsys.com]
> Sent: September-24-15 8:16 PM
> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman; linux-
> u...@vger.kernel.org
> Cc: linux-ker...@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.y...@synopsys.com]
> >> Sent: September-23-15 9:21 PM
> >> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> >> u...@vger.kernel.org; Roman Bacik
> >> Cc: linux-ker...@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-mo
RE: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
> -Original Message-
> From: John Youn [mailto:john.y...@synopsys.com]
> Sent: September-23-15 9:21 PM
> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> u...@vger.kernel.org; Roman Bacik
> Cc: linux-ker...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.y...@synopsys.com]
>> Sent: September-23-15 9:21 PM
>> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
>> u...@vger.kernel.org; Roman Bacik
>> Cc: linux-ker...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/1] USB DWC2 parity fix in isochronous mode
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
