RE: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode

[Roman Bacik <rbacik@broadcom.com>]

> -----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