| Message ID | 1441037838-8864-2-git-send-email-sbranden@broadcom.com |
|---|---|
| State | New |
| Headers | show |
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/
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/
> -----Original Message----- > From: John Youn [mailto:John.Youn@synopsys.com] > Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode > > 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? > We are using procedure based on description from this source: Synopsys, Inc. SolvNet 527 DesignWare.com 3.00a April 2012 USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints in DWC_otg Slave Mode Synopsys databook is not in a public domain to quote the exact paragraph here. You can find it in Chapter E, pp 526-527. There is no register in this databook, which would provide information about the source endpoint of the incomplete interrupt, as you have described. Please, provide an exact reference and possibly enough information that we can turn it into a working code. > > + 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/
On 9/9/2015 11:16 AM, Roman Bacik wrote: >> -----Original Message----- >> From: John Youn [mailto:John.Youn@synopsys.com] >> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode >> >> 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? >> > > We are using procedure based on description from this source: > > Synopsys, Inc. SolvNet 527 > DesignWare.com > 3.00a > April 2012 > USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints in DWC_otg Slave Mode > > Synopsys databook is not in a public domain to quote the exact paragraph here. You can find it in Chapter E, pp 526-527. There is no register in this databook, which would provide information about the source endpoint of the incomplete interrupt, as you have described. > > Please, provide an exact reference and possibly enough information that we can turn it into a working code. > You can check the source of the interrupt by reading the DSTS and ep control registers as described in the databook page 526, last bullet, and 527 first bullet. John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: John Youn [mailto:John.Youn@synopsys.com] > Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode > > On 9/9/2015 11:16 AM, Roman Bacik wrote: > >> -----Original Message----- > >> From: John Youn [mailto:John.Youn@synopsys.com] > >> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in > >> isochronous mode > >> > >> 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? > >> > > > > We are using procedure based on description from this source: > > > > Synopsys, Inc. SolvNet 527 > > DesignWare.com > > 3.00a > > April 2012 > > USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints in > > DWC_otg Slave Mode > > > > Synopsys databook is not in a public domain to quote the exact paragraph > here. You can find it in Chapter E, pp 526-527. There is no register in this > databook, which would provide information about the source endpoint of > the incomplete interrupt, as you have described. > > > > Please, provide an exact reference and possibly enough information that > we can turn it into a working code. > > > > You can check the source of the interrupt by reading the DSTS and ep control > registers as described in the databook page 526, last bullet, and 527 first > bullet. > > John > Yes, that is exactly what we are doing what is described in the last bullet of 526 and the first two bullets of page 527. So what is the problem? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/9/2015 7:16 PM, Roman Bacik wrote: >> -----Original Message----- >> From: John Youn [mailto:John.Youn@synopsys.com] >> Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode >> >> On 9/9/2015 11:16 AM, Roman Bacik wrote: >>>> -----Original Message----- >>>> From: John Youn [mailto:John.Youn@synopsys.com] >>>> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in >>>> isochronous mode >>>> >>>> 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? >>>> >>> >>> We are using procedure based on description from this source: >>> >>> Synopsys, Inc. SolvNet 527 >>> DesignWare.com >>> 3.00a >>> April 2012 >>> USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints in >>> DWC_otg Slave Mode >>> >>> Synopsys databook is not in a public domain to quote the exact paragraph >> here. You can find it in Chapter E, pp 526-527. There is no register in this >> databook, which would provide information about the source endpoint of >> the incomplete interrupt, as you have described. >>> >>> Please, provide an exact reference and possibly enough information that >> we can turn it into a working code. >>> >> >> You can check the source of the interrupt by reading the DSTS and ep control >> registers as described in the databook page 526, last bullet, and 527 first >> bullet. >> >> John >> > > Yes, that is exactly what we are doing what is described in the last bullet of 526 and the first two bullets of page 527. So what is the problem? > Maybe I missed it. Where exactly are you doing it like it says in these two paragraphs? Quote: GINTSTS.incompISOIN is a global interrupt. Therefore, when more than one isochronous endpoints is active, then the application must determine which isochronous IN endpoints have incomplete data. To accomplish this, read DSTS and DIEPCTLn for all isochronous endpoints. If DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16),then that endpoint has an incomplete transfer John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Roman Bacik > Sent: September-09-15 7:17 PM > To: '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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode > > > -----Original Message----- > > From: John Youn [mailto:John.Youn@synopsys.com] > > Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in > > isochronous mode > > > > On 9/9/2015 11:16 AM, Roman Bacik wrote: > > >> -----Original Message----- > > >> From: John Youn [mailto:John.Youn@synopsys.com] > > >> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in > > >> isochronous mode > > >> > > >> 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? > > >> > > > > > > We are using procedure based on description from this source: > > > > > > Synopsys, Inc. SolvNet 527 > > > DesignWare.com > > > 3.00a > > > April 2012 > > > USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints in > > > DWC_otg Slave Mode > > > > > > Synopsys databook is not in a public domain to quote the exact > > > paragraph > > here. You can find it in Chapter E, pp 526-527. There is no register > > in this databook, which would provide information about the source > > endpoint of the incomplete interrupt, as you have described. > > > > > > Please, provide an exact reference and possibly enough information > > > that > > we can turn it into a working code. > > > > > > > You can check the source of the interrupt by reading the DSTS and ep > > control registers as described in the databook page 526, last bullet, > > and 527 first bullet. > > > > John > > > > Yes, that is exactly what we are doing what is described in the last bullet of > 526 and the first two bullets of page 527. So what is the problem? In order to avoid any further confusion, here is the quote of the three bullets: "- GINTSTS.incompISOIN is a global interrupt. Therefore, when more than one isochronous endpoints is active, then the application must determine which isochronous IN endpoints have incomplete data. - To accomplish this, read DSTS and DIEPCTLn for all isochronous endpoints. If DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16), then that endpoint has an incomplete transfer. - When there is an incomplete transfer for an endpoint, then toggle the Even/Odd bit." We believe that this is exactly what we coded in this patch. Regards, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: John Youn [mailto:John.Youn@synopsys.com] > Sent: September-09-15 7:25 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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode > > On 9/9/2015 7:16 PM, Roman Bacik wrote: > >> -----Original Message----- > >> From: John Youn [mailto:John.Youn@synopsys.com] > >> Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in > >> isochronous mode > >> > >> On 9/9/2015 11:16 AM, Roman Bacik wrote: > >>>> -----Original Message----- > >>>> From: John Youn [mailto:John.Youn@synopsys.com] > >>>> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in > >>>> isochronous mode > >>>> > >>>> 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? > >>>> > >>> > >>> We are using procedure based on description from this source: > >>> > >>> Synopsys, Inc. SolvNet 527 > >>> DesignWare.com > >>> 3.00a > >>> April 2012 > >>> USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints in > >>> DWC_otg Slave Mode > >>> > >>> Synopsys databook is not in a public domain to quote the exact > >>> paragraph > >> here. You can find it in Chapter E, pp 526-527. There is no register > >> in this databook, which would provide information about the source > >> endpoint of the incomplete interrupt, as you have described. > >>> > >>> Please, provide an exact reference and possibly enough information > >>> that > >> we can turn it into a working code. > >>> > >> > >> You can check the source of the interrupt by reading the DSTS and ep > >> control registers as described in the databook page 526, last bullet, > >> and 527 first bullet. > >> > >> John > >> > > > > Yes, that is exactly what we are doing what is described in the last bullet of > 526 and the first two bullets of page 527. So what is the problem? > > > > Maybe I missed it. Where exactly are you doing it like it says in these two > paragraphs? > > Quote: > > GINTSTS.incompISOIN is a global interrupt. Therefore, when more than one > isochronous endpoints is active, then the application must determine which > isochronous IN endpoints have incomplete data. > > To accomplish this, read DSTS and DIEPCTLn for all isochronous endpoints. If > DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16),then that endpoint has an > incomplete transfer > > John > > That is the content of the two for loops. I will paste the code here again for your convenience: > + 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); > + } > + -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Roman Bacik > Sent: September-09-15 7:36 PM > To: '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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode > > > -----Original Message----- > > From: John Youn [mailto:John.Youn@synopsys.com] > > Sent: September-09-15 7:25 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 v2 1/1] usb: dwc2: gadget: parity fix in > > isochronous mode > > > > On 9/9/2015 7:16 PM, Roman Bacik wrote: > > >> -----Original Message----- > > >> From: John Youn [mailto:John.Youn@synopsys.com] > > >> Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in > > >> isochronous mode > > >> > > >> On 9/9/2015 11:16 AM, Roman Bacik wrote: > > >>>> -----Original Message----- > > >>>> From: John Youn [mailto:John.Youn@synopsys.com] > > >>>> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in > > >>>> isochronous mode > > >>>> > > >>>> 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? > > >>>> > > >>> > > >>> We are using procedure based on description from this source: > > >>> > > >>> Synopsys, Inc. SolvNet 527 > > >>> DesignWare.com > > >>> 3.00a > > >>> April 2012 > > >>> USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints > in > > >>> DWC_otg Slave Mode > > >>> > > >>> Synopsys databook is not in a public domain to quote the exact > > >>> paragraph > > >> here. You can find it in Chapter E, pp 526-527. There is no > > >> register in this databook, which would provide information about > > >> the source endpoint of the incomplete interrupt, as you have described. > > >>> > > >>> Please, provide an exact reference and possibly enough information > > >>> that > > >> we can turn it into a working code. > > >>> > > >> > > >> You can check the source of the interrupt by reading the DSTS and > > >> ep control registers as described in the databook page 526, last > > >> bullet, and 527 first bullet. > > >> > > >> John > > >> > > > > > > Yes, that is exactly what we are doing what is described in the last > > > bullet of > > 526 and the first two bullets of page 527. So what is the problem? > > > > > > > Maybe I missed it. Where exactly are you doing it like it says in > > these two paragraphs? > > > > Quote: > > > > GINTSTS.incompISOIN is a global interrupt. Therefore, when more than > > one isochronous endpoints is active, then the application must > > determine which isochronous IN endpoints have incomplete data. > > > > To accomplish this, read DSTS and DIEPCTLn for all isochronous > > endpoints. If DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16),then that > > endpoint has an incomplete transfer > > > > John > > > > > > That is the content of the two for loops. I will paste the code here again for > your convenience: > > > + 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); > > + } > > + > > I think I am starting to understand your objection. When the parity is not set on an isochronous endpoint, we flip the parity on that endpoint. I think you would prefer if we flip the parity only on isochronous endpoints, which do not have parity set, and got the actual interrupt. Is this interpretation correct? We believe the way it is now gets correct parity on all isochronous endpoints faster and it was tested working well. But if you prefer adding another condition before flipping the endpoints we can try it out and retest. Regards, Roman -- 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/
> -----Original Message----- > From: Roman Bacik > Sent: September-09-15 7:59 PM > To: '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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode > > > -----Original Message----- > > From: Roman Bacik > > Sent: September-09-15 7:36 PM > > To: '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 v2 1/1] usb: dwc2: gadget: parity fix in > > isochronous mode > > > > > -----Original Message----- > > > From: John Youn [mailto:John.Youn@synopsys.com] > > > Sent: September-09-15 7:25 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 v2 1/1] usb: dwc2: gadget: parity fix in > > > isochronous mode > > > > > > On 9/9/2015 7:16 PM, Roman Bacik wrote: > > > >> -----Original Message----- > > > >> From: John Youn [mailto:John.Youn@synopsys.com] > > > >> Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in > > > >> isochronous mode > > > >> > > > >> On 9/9/2015 11:16 AM, Roman Bacik wrote: > > > >>>> -----Original Message----- > > > >>>> From: John Youn [mailto:John.Youn@synopsys.com] > > > >>>> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in > > > >>>> isochronous mode > > > >>>> > > > >>>> 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? > > > >>>> > > > >>> > > > >>> We are using procedure based on description from this source: > > > >>> > > > >>> Synopsys, Inc. SolvNet 527 > > > >>> DesignWare.com > > > >>> 3.00a > > > >>> April 2012 > > > >>> USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints > > in > > > >>> DWC_otg Slave Mode > > > >>> > > > >>> Synopsys databook is not in a public domain to quote the exact > > > >>> paragraph > > > >> here. You can find it in Chapter E, pp 526-527. There is no > > > >> register in this databook, which would provide information about > > > >> the source endpoint of the incomplete interrupt, as you have > described. > > > >>> > > > >>> Please, provide an exact reference and possibly enough > > > >>> information that > > > >> we can turn it into a working code. > > > >>> > > > >> > > > >> You can check the source of the interrupt by reading the DSTS and > > > >> ep control registers as described in the databook page 526, last > > > >> bullet, and 527 first bullet. > > > >> > > > >> John > > > >> > > > > > > > > Yes, that is exactly what we are doing what is described in the > > > > last bullet of > > > 526 and the first two bullets of page 527. So what is the problem? > > > > > > > > > > Maybe I missed it. Where exactly are you doing it like it says in > > > these two paragraphs? > > > > > > Quote: > > > > > > GINTSTS.incompISOIN is a global interrupt. Therefore, when more than > > > one isochronous endpoints is active, then the application must > > > determine which isochronous IN endpoints have incomplete data. > > > > > > To accomplish this, read DSTS and DIEPCTLn for all isochronous > > > endpoints. If DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16),then that > > > endpoint has an incomplete transfer > > > > > > John > > > > > > > > We have replaced the original code with the following code for both IN and OUT endpoints and retested: if (gintsts & GINTSTS_INCOMPL_SOIN) { u32 idx, epctl_reg, ctrl, dsts; struct s3c_hsotg_ep *hs_ep; + dsts = readl(hsotg->regs + DSTS); 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 (((dsts >> DSTS_SOFFN_SHIFT) & 1) != + ((ctrl >> DXEPCTL_EOFRNUM) & 1)) + continue; if (ctrl & DXEPCTL_EOFRNUM) ctrl |= DXEPCTL_SETEVENFR; else ctrl |= DXEPCTL_SETODDFR; writel(ctrl, hsotg->regs + epctl_reg); break; } writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS); } It still works. However, based on our experience with the audio gadget, USB host would either always use odd microframes or even microframes. It does not change the parity and hence gadget either always works or never works without this patch. So we can call hosts either odd or even. When a gadget connects to even host, since it has even parity by default, it would receive transfer complete packets. So it recognizes it has correct parity and stop flipping. When a gadget connects to an odd host, it would flip all parities to the correct one on all endpoints after receiving an incomplete interrupt from any of the endpoints connected to this odd host using the original code. On the other hand it would require an interrupt for each connected endpoint in order to get the parity right using the new code above. Even when connected to multiple PCs with different parities, the worst case number of swaps (and hence lost packets) would be the number of connected hosts when using the original code, and the total number of endpoints, which is bigger than the number of connected hosts, when using the new code. Please, let us know if you would prefer the new code anyway. Regards, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/10/2015 10:56 AM, Roman Bacik wrote: >> -----Original Message----- >> From: Roman Bacik >> Sent: September-09-15 7:59 PM >> To: '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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode >> >>> -----Original Message----- >>> From: Roman Bacik >>> Sent: September-09-15 7:36 PM >>> To: '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 v2 1/1] usb: dwc2: gadget: parity fix in >>> isochronous mode >>> >>>> -----Original Message----- >>>> From: John Youn [mailto:John.Youn@synopsys.com] >>>> Sent: September-09-15 7:25 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 v2 1/1] usb: dwc2: gadget: parity fix in >>>> isochronous mode >>>> >>>> On 9/9/2015 7:16 PM, Roman Bacik wrote: >>>>>> -----Original Message----- >>>>>> From: John Youn [mailto:John.Youn@synopsys.com] >>>>>> Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in >>>>>> isochronous mode >>>>>> >>>>>> On 9/9/2015 11:16 AM, Roman Bacik wrote: >>>>>>>> -----Original Message----- >>>>>>>> From: John Youn [mailto:John.Youn@synopsys.com] >>>>>>>> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in >>>>>>>> isochronous mode >>>>>>>> >>>>>>>> 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? >>>>>>>> >>>>>>> >>>>>>> We are using procedure based on description from this source: >>>>>>> >>>>>>> Synopsys, Inc. SolvNet 527 >>>>>>> DesignWare.com >>>>>>> 3.00a >>>>>>> April 2012 >>>>>>> USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous Endpoints >>> in >>>>>>> DWC_otg Slave Mode >>>>>>> >>>>>>> Synopsys databook is not in a public domain to quote the exact >>>>>>> paragraph >>>>>> here. You can find it in Chapter E, pp 526-527. There is no >>>>>> register in this databook, which would provide information about >>>>>> the source endpoint of the incomplete interrupt, as you have >> described. >>>>>>> >>>>>>> Please, provide an exact reference and possibly enough >>>>>>> information that >>>>>> we can turn it into a working code. >>>>>>> >>>>>> >>>>>> You can check the source of the interrupt by reading the DSTS and >>>>>> ep control registers as described in the databook page 526, last >>>>>> bullet, and 527 first bullet. >>>>>> >>>>>> John >>>>>> >>>>> >>>>> Yes, that is exactly what we are doing what is described in the >>>>> last bullet of >>>> 526 and the first two bullets of page 527. So what is the problem? >>>>> >>>> >>>> Maybe I missed it. Where exactly are you doing it like it says in >>>> these two paragraphs? >>>> >>>> Quote: >>>> >>>> GINTSTS.incompISOIN is a global interrupt. Therefore, when more than >>>> one isochronous endpoints is active, then the application must >>>> determine which isochronous IN endpoints have incomplete data. >>>> >>>> To accomplish this, read DSTS and DIEPCTLn for all isochronous >>>> endpoints. If DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16),then that >>>> endpoint has an incomplete transfer >>>> >>>> John >>>> >>>> >>> > > We have replaced the original code with the following code for both IN and OUT endpoints and retested: > > if (gintsts & GINTSTS_INCOMPL_SOIN) { > u32 idx, epctl_reg, ctrl, dsts; > struct s3c_hsotg_ep *hs_ep; > > + dsts = readl(hsotg->regs + DSTS); > 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 (((dsts >> DSTS_SOFFN_SHIFT) & 1) != > + ((ctrl >> DXEPCTL_EOFRNUM) & 1)) > + continue; > > if (ctrl & DXEPCTL_EOFRNUM) > ctrl |= DXEPCTL_SETEVENFR; > else > ctrl |= DXEPCTL_SETODDFR; > writel(ctrl, hsotg->regs + epctl_reg); > break; > } > writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS); > } > Yes that is what I was asking about, checking for the source of the interrupt. But I'm convinced that the original code should be ok even with multiple iso endpoints given that all you are doing is toggling to find the correct parity the first time. However a full implementation of this interrupt would need to know the exact source because, even with parity set, you could get this interrupt and will need to handle it properly. For example if you are slow reading or writing the fifo. > It still works. However, based on our experience with the audio gadget, USB host would either always use odd microframes or even microframes. It does not change the parity and hence gadget either always works or never works without this patch. So we can call hosts either odd or even. > > When a gadget connects to even host, since it has even parity by default, it would receive transfer complete packets. So it recognizes it has correct parity and stop flipping. > > When a gadget connects to an odd host, it would flip all parities to the correct one on all endpoints after receiving an incomplete interrupt from any of the endpoints connected to this odd host using the original code. On the other hand it would require an interrupt for each connected endpoint in order to get the parity right using the new code above. > You would still get this interrupt just once per microframe. And you would determine all the endpoints that missed that microframe at once in the loop. The result should be the same as before, the only overhead being the register read/check per iteration. > Even when connected to multiple PCs with different parities, the worst case number of swaps (and hence lost packets) would be the number of connected hosts when using the original code, and the total number of endpoints, which is bigger than the number of connected hosts, when using the new code. > > Please, let us know if you would prefer the new code anyway. The original code is fine. But we do plan to update the gadget code for isochronous so we may change how this is handled in the future. If you resubmit with my other feedback I will review and test on our platforms. 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/
> -----Original Message----- > From: John Youn [mailto:John.Youn@synopsys.com] > Sent: September-10-15 12:08 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 v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode > > On 9/10/2015 10:56 AM, Roman Bacik wrote: > >> -----Original Message----- > >> From: Roman Bacik > >> Sent: September-09-15 7:59 PM > >> To: '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 v2 1/1] usb: dwc2: gadget: parity fix in > >> isochronous mode > >> > >>> -----Original Message----- > >>> From: Roman Bacik > >>> Sent: September-09-15 7:36 PM > >>> To: '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 v2 1/1] usb: dwc2: gadget: parity fix in > >>> isochronous mode > >>> > >>>> -----Original Message----- > >>>> From: John Youn [mailto:John.Youn@synopsys.com] > >>>> Sent: September-09-15 7:25 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 v2 1/1] usb: dwc2: gadget: parity fix in > >>>> isochronous mode > >>>> > >>>> On 9/9/2015 7:16 PM, Roman Bacik wrote: > >>>>>> -----Original Message----- > >>>>>> From: John Youn [mailto:John.Youn@synopsys.com] > >>>>>> Sent: September-09-15 7:11 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 v2 1/1] usb: dwc2: gadget: parity fix in > >>>>>> isochronous mode > >>>>>> > >>>>>> On 9/9/2015 11:16 AM, Roman Bacik wrote: > >>>>>>>> -----Original Message----- > >>>>>>>> From: John Youn [mailto:John.Youn@synopsys.com] > >>>>>>>> Sent: September-03-15 11:53 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 v2 1/1] usb: dwc2: gadget: parity fix in > >>>>>>>> isochronous mode > >>>>>>>> > >>>>>>>> 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? > >>>>>>>> > >>>>>>> > >>>>>>> We are using procedure based on description from this source: > >>>>>>> > >>>>>>> Synopsys, Inc. SolvNet 527 > >>>>>>> DesignWare.com > >>>>>>> 3.00a > >>>>>>> April 2012 > >>>>>>> USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous > Endpoints > >>> in > >>>>>>> DWC_otg Slave Mode > >>>>>>> > >>>>>>> Synopsys databook is not in a public domain to quote the exact > >>>>>>> paragraph > >>>>>> here. You can find it in Chapter E, pp 526-527. There is no > >>>>>> register in this databook, which would provide information about > >>>>>> the source endpoint of the incomplete interrupt, as you have > >> described. > >>>>>>> > >>>>>>> Please, provide an exact reference and possibly enough > >>>>>>> information that > >>>>>> we can turn it into a working code. > >>>>>>> > >>>>>> > >>>>>> You can check the source of the interrupt by reading the DSTS and > >>>>>> ep control registers as described in the databook page 526, last > >>>>>> bullet, and 527 first bullet. > >>>>>> > >>>>>> John > >>>>>> > >>>>> > >>>>> Yes, that is exactly what we are doing what is described in the > >>>>> last bullet of > >>>> 526 and the first two bullets of page 527. So what is the problem? > >>>>> > >>>> > >>>> Maybe I missed it. Where exactly are you doing it like it says in > >>>> these two paragraphs? > >>>> > >>>> Quote: > >>>> > >>>> GINTSTS.incompISOIN is a global interrupt. Therefore, when more > >>>> than one isochronous endpoints is active, then the application must > >>>> determine which isochronous IN endpoints have incomplete data. > >>>> > >>>> To accomplish this, read DSTS and DIEPCTLn for all isochronous > >>>> endpoints. If DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16),then that > >>>> endpoint has an incomplete transfer > >>>> > >>>> John > >>>> > >>>> > >>> > > > > We have replaced the original code with the following code for both IN and > OUT endpoints and retested: > > > > if (gintsts & GINTSTS_INCOMPL_SOIN) { > > u32 idx, epctl_reg, ctrl, dsts; > > struct s3c_hsotg_ep *hs_ep; > > > > + dsts = readl(hsotg->regs + DSTS); > > 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 (((dsts >> DSTS_SOFFN_SHIFT) & 1) != > > + ((ctrl >> DXEPCTL_EOFRNUM) & 1)) > > + continue; > > > > if (ctrl & DXEPCTL_EOFRNUM) > > ctrl |= DXEPCTL_SETEVENFR; > > else > > ctrl |= DXEPCTL_SETODDFR; > > writel(ctrl, hsotg->regs + epctl_reg); > > break; > > } > > writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS); > > } > > > > Yes that is what I was asking about, checking for the source of the interrupt. > > But I'm convinced that the original code should be ok even with multiple iso > endpoints given that all you are doing is toggling to find the correct parity the > first time. > > However a full implementation of this interrupt would need to know the > exact source because, even with parity set, you could get this interrupt and > will need to handle it properly. For example if you are slow reading or writing > the fifo. > > > > It still works. However, based on our experience with the audio gadget, > USB host would either always use odd microframes or even microframes. It > does not change the parity and hence gadget either always works or never > works without this patch. So we can call hosts either odd or even. > > > > When a gadget connects to even host, since it has even parity by default, it > would receive transfer complete packets. So it recognizes it has correct parity > and stop flipping. > > > > When a gadget connects to an odd host, it would flip all parities to the > correct one on all endpoints after receiving an incomplete interrupt from any > of the endpoints connected to this odd host using the original code. On the > other hand it would require an interrupt for each connected endpoint in > order to get the parity right using the new code above. > > > > You would still get this interrupt just once per microframe. And you would > determine all the endpoints that missed that microframe at once in the loop. > The result should be the same as before, the only overhead being the > register read/check per iteration. > > > > Even when connected to multiple PCs with different parities, the worst > case number of swaps (and hence lost packets) would be the number of > connected hosts when using the original code, and the total number of > endpoints, which is bigger than the number of connected hosts, when using > the new code. > > > > Please, let us know if you would prefer the new code anyway. > > The original code is fine. But we do plan to update the gadget code for > isochronous so we may change how this is handled in the future. > > If you resubmit with my other feedback I will review and test on our > platforms. > > Regards, > John > > I will resubmit with the change to the for loop bound replacing MAX_EPS_CHANNELS with hsotg->num_of_eps and add a method for parity change for three times repeated code: if (ctrl & DXEPCTL_EOFRNUM) ctrl |= DXEPCTL_SETEVENFR; else ctrl |= DXEPCTL_SETODDFR; writel(ctrl, hsotg->regs + epctl_reg); Regards, Roman -- 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/
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)