> -----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-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
