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

[Roman Bacik <rbacik@broadcom.com>]

> -----Original Message-----
> From: John Youn [mailto:John.Youn@synopsys.com]
> Sent: August-25-15 2:52 PM
> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; bcm-kernel-feedback-list; Roman Bacik
> Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> 
> On 8/18/2015 8:45 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>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > Signed-off-by: Scott Branden <sbranden@broadcom.com>
> > ---
> >  drivers/usb/dwc2/core.h   |  1 +
> >  drivers/usb/dwc2/gadget.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/usb/dwc2/hw.h     |  1 +
> >  3 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
> > 0ed87620..954d1cd 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            parity_set:1;
> >
> >  	char                    name[10];
> >  };
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 4d47b7c..28e4393 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -1954,6 +1954,8 @@ static void s3c_hsotg_epint(struct dwc2_hsotg
> *hsotg, unsigned int idx,
> >  		ints &= ~DXEPINT_XFERCOMPL;
> >
> >  	if (ints & DXEPINT_XFERCOMPL) {
> > +		if (hs_ep->isochronous && !hs_ep->parity_set)
> > +			hs_ep->parity_set = 1;
> >  		if (hs_ep->isochronous && hs_ep->interval == 1) {
> >  			if (ctrl & DXEPCTL_EOFRNUM)
> >  				ctrl |= DXEPCTL_SETEVENFR;
> > @@ -2316,7 +2318,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 +2584,48 @@ irq_retry:
> >  		s3c_hsotg_dump(hsotg);
> >  	}
> >
> > +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> > +		u32 idx;
> > +		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->parity_set) {
> > +				u32 epctl_reg = DIEPCTL(idx);
> > +				u32 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;
> > +		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->parity_set) {
> > +				u32 epctl_reg = DOEPCTL(idx);
> > +				u32 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 +2712,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->parity_set = 0;
> 
> 
> I'm not quite sure what the parity_set flag does in this patch.
> Shouldn't you be able to just toggle the even/odd frame when you get the
> interrupt?
> 
> John
> 

When Transfer Complete interrupt is received, we have the correct parity. Therefore we set the flag and we stop toggling. The parity_set flag indicates whether we have the correct parity set.
Regards,

Roman