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

[Roman Bacik]

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@xxxxxx]
> Sent: August-25-15 3:36 PM
> To: Roman Bacik
> Cc: John Youn; Scott Branden; Greg Kroah-Hartman; linux-
> usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; bcm-kernel-feedback-
> list
> Subject: Re: [PATCH 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> 
> On Tue, Aug 25, 2015 at 10:00:17PM +0000, Roman Bacik wrote:
> > > -----Original Message-----
> > > From: John Youn [mailto:John.Youn@xxxxxxxxxxxx]
> > > Sent: August-25-15 2:52 PM
> > > To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> > > usb@xxxxxxxxxxxxxxx
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx; 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@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>
> > > > Reviewed-by: Scott Branden <sbranden@xxxxxxxxxxxx>
> > > > Signed-off-by: Scott Branden <sbranden@xxxxxxxxxxxx>
> > > > ---
> > > >  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;
> 

It can be simplified to:
		hs_ep->has_correct_parity = 1;
I just thought that the original shows better what we are trying to do. I do not mind to simplify it and remove the condition.

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

This fix uses Incomplete Periodic Transfer interrupt (GINTSTS_INCOMPL) to toggle between even and odd parity until the Transfer Complete interrupt is received. We also need to set correct parity on both IN and OUT endpoints.

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

ok

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

ok

> --
> balbi

Roman

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/