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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]

On Tue, Aug 25, 2015 at 10:00:17PM +0000, Roman Bacik wrote:
> > -----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;

it shouldn't be a problem to set the flag which was already set, so this
could be simplified to:

		hs_ep->has_correct_parity = !!hs_ep0>isochronous;

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

why the two extra bits ? What are they doing ?

> > >  		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++) {

			u32 epctl_reg;
			u32 ctrl;

> > > +			hs_ep = hsotg->eps_in[idx];

you can decrease some indentation here:

			if (!hs_ep->isochronous)
				continue;

			if (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);


ditto to the other loop below

<snip>

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

then how about calling it has_correct_parity instead ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]