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

[John Youn]

On 8/25/2015 3:00 PM, 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;
>>>  		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,
>>>  		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++) {
>>> +			hs_ep = hsotg->eps_in[idx];
>>> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
>>> +				u32 epctl_reg = DIEPCTL(idx);
>>> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
>>> +
>>> +				if (ctrl & DXEPCTL_EOFRNUM)
>>> +					ctrl |= DXEPCTL_SETEVENFR;
>>> +				else
>>> +					ctrl |= DXEPCTL_SETODDFR;
>>> +				writel(ctrl, hsotg->regs + epctl_reg);
>>> +			}
>>> +		}
>>> +		writel(GINTSTS_INCOMPL_SOIN, hsotg->regs + GINTSTS);
>>> +	}
>>> +
>>> +	if (gintsts & GINTSTS_INCOMPL_SOOUT) {
>>> +		u32 idx;
>>> +		struct s3c_hsotg_ep *hs_ep;
>>> +
>>> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOOUT\n",
>> __func__);
>>> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {
>>> +			hs_ep = hsotg->eps_out[idx];
>>> +			if (hs_ep->isochronous && !hs_ep->parity_set) {
>>> +				u32 epctl_reg = DOEPCTL(idx);
>>> +				u32 ctrl = readl(hsotg->regs + epctl_reg);
>>> +
>>> +				if (ctrl & DXEPCTL_EOFRNUM)
>>> +					ctrl |= DXEPCTL_SETEVENFR;
>>> +				else
>>> +					ctrl |= DXEPCTL_SETODDFR;
>>> +				writel(ctrl, hsotg->regs + epctl_reg);
>>> +			}
>>> +		}
>>> +		writel(GINTSTS_INCOMPL_SOOUT, hsotg->regs + GINTSTS);
>>> +	}
>>> +
>>>  	/*
>>>  	 * if we've had fifo events, we should try and go around the
>>>  	 * loop again to see if there's any point in returning yet.
>>> @@ -2667,6 +2712,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep
>> *ep,
>>>  	hs_ep->periodic = 0;
>>>  	hs_ep->halted = 0;
>>>  	hs_ep->interval = desc->bInterval;
>>> +	hs_ep->parity_set = 0;
>>
>>
>> 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.
> Regards,
> 
> Roman
> 

I'm not sure that "parity" is the proper term in this context but
I can't think of a more concise way to phrase it.

What if the host switches again some time after the first xfer
complete?

What function or gadget driver are you using to test this? Did
you test both the ISO IN and OUT case?

John



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