> -----Original Message-----
> From: Felipe Balbi [mailto:ba...@ti.com]
> Sent: August-25-15 3:36 PM
> To: Roman Bacik
> Cc: John Youn; Scott Branden; Greg Kroah-Hartman; linux-
> u...@vger.kernel.org; linux-kernel@vger.kernel.org; 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.y...@synopsys.com]
> > > Sent: August-25-15 2:52 PM
> > > To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> > > u...@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 <rba...@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 <rba...@broadcom.com>
> > > > Reviewed-by: Abhinav Ratna <ara...@broadcom.com>
> > > > Reviewed-by: Srinath Mannam <srinath.man...@broadcom.com>
> > > > Reviewed-by: Scott Branden <sbran...@broadcom.com>
> > > > Signed-off-by: Scott Branden <sbran...@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;
>
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
