> -----Original Message----- > From: John Youn [mailto:John.Youn@xxxxxxxxxxxx] > Sent: September-10-15 12:08 PM > To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman; linux- > usb@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx; bcm-kernel-feedback-list > Subject: Re: [PATCH 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@xxxxxxxxxxxxxxx' > >> Cc: 'linux-kernel@xxxxxxxxxxxxxxx'; 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@xxxxxxxxxxxxxxx > >>> Cc: linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxx] > >>>> Sent: September-09-15 7:25 PM > >>>> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman; > >>>> linux- usb@xxxxxxxxxxxxxxx > >>>> Cc: linux-kernel@xxxxxxxxxxxxxxx; bcm-kernel-feedback-list > >>>> Subject: Re: [PATCH 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@xxxxxxxxxxxx] > >>>>>> Sent: September-09-15 7:11 PM > >>>>>> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman; > >>>>>> linux- usb@xxxxxxxxxxxxxxx > >>>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx; bcm-kernel-feedback-list > >>>>>> Subject: Re: [PATCH 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@xxxxxxxxxxxx] > >>>>>>>> Sent: September-03-15 11:53 PM > >>>>>>>> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux- > >>>>>>>> usb@xxxxxxxxxxxxxxx; Roman Bacik > >>>>>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx; bcm-kernel-feedback-list > >>>>>>>> Subject: Re: [PATCH 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@xxxxxxxxxxxx> > >>>>>>>>> > >>>>>>>>> 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@xxxxxxxxxxxx> > >>>>>>>>> Reviewed-by: Abhinav Ratna <aratna@xxxxxxxxxxxx> > >>>>>>>>> Reviewed-by: Srinath Mannam > >> <srinath.mannam@xxxxxxxxxxxx> > >>>>>>>>> Signed-off-by: Scott Branden <sbranden@xxxxxxxxxxxx> > >>>>>>>>> --- > >>>>>>>>> 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-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
