Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Thinh Nguyen]

Hi Anurag,

Anurag Kumar Vulisha wrote:
>>> +		return -EINVAL;
>>>
>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>>  	if (set)
>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct
>> usb_ctrlrequest *ctrl)
>>>  			 * nothing is pending from application.
>>>  			 */
>>>  			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>> -			reg |= (DWC3_DCTL_ACCEPTU1ENA |
>> DWC3_DCTL_ACCEPTU2ENA);
>>> +			if (!dwc->dis_u1_entry_quirk)
>>> +				reg |= DWC3_DCTL_ACCEPTU1ENA;
>>> +			if (!dwc->dis_u2_entry_quirk)
>>> +				reg |= DWC3_DCTL_ACCEPTU2ENA;
>>>  			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>>>  		}
>>>  		break;
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index e293400..f2d3112 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>  	return 0;
>>>  }
>>>
>>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>>> +				      struct usb_dcd_config_params *params)
>>> +{
>>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>>> +
>>> +	/* U1 Device exit Latency */
>>> +	if (dwc->dis_u1_entry_quirk)
>>> +		params->bU1devExitLat = 0;
>> It doesn't make sense to have exit latency of 0. Rejecting
>> SET_FEATURE(enable U1/U2) should already let the host know that the
>> device doesn't support U1/U2.
>>
> I am okay to remove this, but I feel that it is better to report zero value instead
> of a non-zero value in exit latency of BOS when U1 or U2 entries are not supported. 
> Advantage of reporting 0 is that some hosts doesn't even send SET_FEATURE(U1/U2)
> requests on seeing zero value in BOS descriptor. Also there can be cases where U1 is
> disabled and U2 entry is allowed or vice versa, for these kind of cases the driver can
> set zero exit latency value for U1 and non-zero exit latency value for U2 . Based on this
> I think it would be better to report 0 when U1/U2 states are not enabled. Please provide
> your opinion on this.

Hm... I assume you're testing against linux usb stack and xhci host. If
that's the case, it looks like host will still request the device to
enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This
needs to be fixed. I think what you have is fine to workaround this issue.

Thanks,
Thinh