Message-ID: <D2C8DF24C22DB24BA948A8B0D097284002F27AD7@SJEXCHMB05.corp.ad.broadcom.com>
Date:	Thu, 10 Sep 2015 19:45:10 +0000
From:	Roman Bacik <rbacik@...adcom.com>
To:	John Youn <John.Youn@...opsys.com>,
	Scott Branden <sbranden@...adcom.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>
Subject: RE: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode

> -----Original Message-----
> From: John Youn [mailto:John.Youn@...opsys.com]
> Sent: September-10-15 12:08 PM
> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman; linux-
> usb@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org; bcm-kernel-feedback-list
> Subject: Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode
> 
> On 9/10/2015 10:56 AM, Roman Bacik wrote:
> >> -----Original Message-----
> >> From: Roman Bacik
> >> Sent: September-09-15 7:59 PM
> >> To: 'John Youn'; Scott Branden; 'Greg Kroah-Hartman'; 'linux-
> >> usb@...r.kernel.org'
> >> Cc: 'linux-kernel@...r.kernel.org'; bcm-kernel-feedback-list
> >> Subject: RE: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in
> >> isochronous mode
> >>
> >>> -----Original Message-----
> >>> From: Roman Bacik
> >>> Sent: September-09-15 7:36 PM
> >>> To: 'John Youn'; Scott Branden; Greg Kroah-Hartman; linux-
> >>> usb@...r.kernel.org
> >>> Cc: linux-kernel@...r.kernel.org; bcm-kernel-feedback-list
> >>> Subject: RE: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in
> >>> isochronous mode
> >>>
> >>>> -----Original Message-----
> >>>> From: John Youn [mailto:John.Youn@...opsys.com]
> >>>> Sent: September-09-15 7:25 PM
> >>>> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman;
> >>>> linux- usb@...r.kernel.org
> >>>> Cc: linux-kernel@...r.kernel.org; bcm-kernel-feedback-list
> >>>> Subject: Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in
> >>>> isochronous mode
> >>>>
> >>>> On 9/9/2015 7:16 PM, Roman Bacik wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: John Youn [mailto:John.Youn@...opsys.com]
> >>>>>> Sent: September-09-15 7:11 PM
> >>>>>> To: Roman Bacik; John Youn; Scott Branden; Greg Kroah-Hartman;
> >>>>>> linux- usb@...r.kernel.org
> >>>>>> Cc: linux-kernel@...r.kernel.org; bcm-kernel-feedback-list
> >>>>>> Subject: Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in
> >>>>>> isochronous mode
> >>>>>>
> >>>>>> On 9/9/2015 11:16 AM, Roman Bacik wrote:
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: John Youn [mailto:John.Youn@...opsys.com]
> >>>>>>>> Sent: September-03-15 11:53 PM
> >>>>>>>> To: Scott Branden; John Youn; Greg Kroah-Hartman; linux-
> >>>>>>>> usb@...r.kernel.org; Roman Bacik
> >>>>>>>> Cc: linux-kernel@...r.kernel.org; bcm-kernel-feedback-list
> >>>>>>>> Subject: Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in
> >>>>>>>> isochronous mode
> >>>>>>>>
> >>>>>>>> On 8/31/2015 9:17 AM, Scott Branden wrote:
> >>>>>>>>> From: Roman Bacik <rbacik@...adcom.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@...adcom.com>
> >>>>>>>>> Reviewed-by: Abhinav Ratna <aratna@...adcom.com>
> >>>>>>>>> Reviewed-by: Srinath Mannam
> >> <srinath.mannam@...adcom.com>
> >>>>>>>>> Signed-off-by: Scott Branden <sbranden@...adcom.com>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/usb/dwc2/core.h   |  1 +
> >>>>>>>>>  drivers/usb/dwc2/gadget.c | 51
> >>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++-
> >>>>>>>>>  drivers/usb/dwc2/hw.h     |  1 +
> >>>>>>>>>  3 files changed, 52 insertions(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> >>>>>>>>> index 0ed87620..a5634fd 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            has_correct_parity:1;
> >>>>>>>>>
> >>>>>>>>>  	char                    name[10];
> >>>>>>>>>  };
> >>>>>>>>> diff --git a/drivers/usb/dwc2/gadget.c
> >>>>>>>>> b/drivers/usb/dwc2/gadget.c index 4d47b7c..fac3e2f 100644
> >>>>>>>>> --- a/drivers/usb/dwc2/gadget.c
> >>>>>>>>> +++ b/drivers/usb/dwc2/gadget.c
> >>>>>>>>> @@ -1954,6 +1954,7 @@ static void s3c_hsotg_epint(struct
> >>>>>>>>> dwc2_hsotg
> >>>>>>>> *hsotg, unsigned int idx,
> >>>>>>>>>  		ints &= ~DXEPINT_XFERCOMPL;
> >>>>>>>>>
> >>>>>>>>>  	if (ints & DXEPINT_XFERCOMPL) {
> >>>>>>>>> +		hs_ep->has_correct_parity = 1;
> >>>>>>>>>  		if (hs_ep->isochronous && hs_ep->interval == 1) {
> >>>>>>>>>  			if (ctrl & DXEPCTL_EOFRNUM)
> >>>>>>>>>  				ctrl |= DXEPCTL_SETEVENFR; @@ -
> >> 2316,7 +2317,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 +2583,52 @@ irq_retry:
> >>>>>>>>>  		s3c_hsotg_dump(hsotg);
> >>>>>>>>>  	}
> >>>>>>>>>
> >>>>>>>>> +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> >>>>>>>>> +		u32 idx, epctl_reg, ctrl;
> >>>>>>>>> +		struct s3c_hsotg_ep *hs_ep;
> >>>>>>>>> +
> >>>>>>>>> +		dev_dbg(hsotg->dev, "%s:
> >>>> GINTSTS_INCOMPL_SOIN\n",
> >>>>>>>> __func__);
> >>>>>>>>> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
> >>>>>>>>
> >>>>>>>> Valid endpoints are only up to hsotg->num_of_eps so this might
> >>>>>>>> crash on certain configurations.
> >>>>>>>>
> >>>>>>>> Also, have you tried to find the endpoint which caused the
> >>>>>>>> incomplete interrupt by reading the control registers as
> >>>>>>>> described in
> >>>> the databook?
> >>>>>>>>
> >>>>>>>
> >>>>>>> We are using procedure based on description from this source:
> >>>>>>>
> >>>>>>> Synopsys, Inc. SolvNet 527
> >>>>>>> DesignWare.com
> >>>>>>> 3.00a
> >>>>>>> April 2012
> >>>>>>> USB 2.0 Hi-Speed On-The-Go (OTG) Databook Isochronous
> Endpoints
> >>> in
> >>>>>>> DWC_otg Slave Mode
> >>>>>>>
> >>>>>>> Synopsys databook is not in a public domain to quote the exact
> >>>>>>> paragraph
> >>>>>> here. You can find it in Chapter E, pp 526-527. There is no
> >>>>>> register in this databook, which would provide information about
> >>>>>> the source endpoint of the incomplete interrupt, as you have
> >> described.
> >>>>>>>
> >>>>>>> Please, provide an exact reference and possibly enough
> >>>>>>> information that
> >>>>>> we can turn it into a working code.
> >>>>>>>
> >>>>>>
> >>>>>> You can check the source of the interrupt by reading the DSTS and
> >>>>>> ep control registers as described in the databook page 526, last
> >>>>>> bullet, and 527 first bullet.
> >>>>>>
> >>>>>> John
> >>>>>>
> >>>>>
> >>>>> Yes, that is exactly what we are doing what is described in the
> >>>>> last bullet of
> >>>> 526 and the first two bullets of page 527. So what is the problem?
> >>>>>
> >>>>
> >>>> Maybe I missed it. Where exactly are you doing it like it says in
> >>>> these two paragraphs?
> >>>>
> >>>> Quote:
> >>>>
> >>>> GINTSTS.incompISOIN is a global interrupt. Therefore, when more
> >>>> than one isochronous endpoints is active, then the application must
> >>>> determine which isochronous IN endpoints have incomplete data.
> >>>>
> >>>> To accomplish this, read DSTS and DIEPCTLn for all isochronous
> >>>> endpoints. If DSTS.SOFFN[0] =DIEPCTLn.EO_FrNum (Bit 16),then that
> >>>> endpoint has an incomplete transfer
> >>>>
> >>>> John
> >>>>
> >>>>
> >>>
> >
> > We have replaced the original code with the following code for both IN and
> OUT endpoints and retested:
> >
> > 	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> > 		u32 idx, epctl_reg, ctrl, dsts;
> > 		struct s3c_hsotg_ep *hs_ep;
> >
> > +		dsts = readl(hsotg->regs + DSTS);
> > 		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-
> >has_correct_parity)
> > 				continue;
> >
> > 			epctl_reg = DIEPCTL(idx);
> > 			ctrl = readl(hsotg->regs + epctl_reg);
> >
> > +			if (((dsts >> DSTS_SOFFN_SHIFT) & 1) !=
> > +				((ctrl >> DXEPCTL_EOFRNUM) & 1))
> > +				continue;
> >
> > 			if (ctrl & DXEPCTL_EOFRNUM)
> > 				ctrl |= DXEPCTL_SETEVENFR;
> > 			else
> > 				ctrl |= DXEPCTL_SETODDFR;
> > 			writel(ctrl, hsotg->regs + epctl_reg);
> > 			break;
> > 		}
> > 		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
> > 	}
> >
> 
> Yes that is what I was asking about, checking for the source of the interrupt.
> 
> But I'm convinced that the original code should be ok even with multiple iso
> endpoints given that all you are doing is toggling to find the correct parity the
> first time.
> 
> However a full implementation of this interrupt would need to know the
> exact source because, even with parity set, you could get this interrupt and
> will need to handle it properly. For example if you are slow reading or writing
> the fifo.
> 
> 
> > It still works. However, based on our experience with the audio gadget,
> USB host would either always use odd microframes or even microframes. It
> does not change the parity and hence gadget either always works or never
> works without this patch. So we can call hosts either odd or even.
> >
> > When a gadget connects to even host, since it has even parity by default, it
> would receive transfer complete packets. So it recognizes it has correct parity
> and stop flipping.
> >
> > When a gadget connects to an odd host, it would flip all parities to the
> correct one on all endpoints after receiving an incomplete interrupt from any
> of the endpoints connected to this odd host using the original code. On the
> other hand it would require an interrupt for each connected endpoint in
> order to get the parity right using the new code above.
> >
> 
> You would still get this interrupt just once per microframe. And you would
> determine all the endpoints that missed that microframe at once in the loop.
> The result should be the same as before, the only overhead being the
> register read/check per iteration.
> 
> 
> > Even when connected to multiple PCs with different parities, the worst
> case number of swaps (and hence lost packets) would be the number of
> connected hosts when using the original code, and the total number of
> endpoints, which is bigger than the number of connected hosts, when using
> the new code.
> >
> > Please, let us know if you would prefer the new code anyway.
> 
> The original code is fine. But we do plan to update the gadget code for
> isochronous so we may change how this is handled in the future.
> 
> If you resubmit with my other feedback I will review and test on our
> platforms.
> 
> Regards,
> John
> 
> 

I will resubmit with the change to the for loop bound replacing MAX_EPS_CHANNELS with hsotg->num_of_eps and add a method for parity change for three times repeated code:

			if (ctrl & DXEPCTL_EOFRNUM)
				ctrl |= DXEPCTL_SETEVENFR;
			else
				ctrl |= DXEPCTL_SETODDFR;
			writel(ctrl, hsotg->regs + epctl_reg);

Regards,

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

Powered by blists - more mailing lists