Re: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

[Felipe Balbi <balbi@xxxxxxxxxx>]

Hi,

John Youn <John.Youn@xxxxxxxxxxxx> writes:
<snip>

>> If it wasn't for that flag, we would always allocate transfer resource 3
>> for every newly enabled endpoint. Can you check with your RTL engineers
>> if it's valid to *always* issue DEPSTARTCFG with a proper parameter
>> depending on endpoint number ? Basically, something like below:
>> 
>> @@ -306,20 +306,14 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc, struct dwc3_ep *dep)
>>  
>>  	memset(&params, 0x00, sizeof(params));
>>  
>> -	if (dep->number != 1) {
>> -		cmd = DWC3_DEPCMD_DEPSTARTCFG;
>> -		/* XferRscIdx == 0 for ep0 and 2 for the remaining */
>> -		if (dep->number > 1) {
>> -			if (dwc->start_config_issued)
>> -				return 0;
>> -			dwc->start_config_issued = true;
>> -			cmd |= DWC3_DEPCMD_PARAM(2);
>> -		}
>> -
>> -		return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, &params);
>> +	cmd = DWC3_DEPCMD_DEPSTARTCFG;
>> +	/* XferRscIdx == 0 for ep0 and 2 for the remaining */
>> +	if (dep->number > 1) {
>> +		dwc->start_config_issued = true;
>> +		cmd |= DWC3_DEPCMD_PARAM(dwc->current_resource);
>>  	}
>>  
>> -	return 0;
>> +	return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, &params);
>>  }
>>  
>>  static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep,
>> @@ -388,13 +382,20 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep,
>>  static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep *dep)
>>  {
>>  	struct dwc3_gadget_ep_cmd_params params;
>> +	int				ret;
>>  
>>  	memset(&params, 0x00, sizeof(params));
>>  
>>  	params.param0 = DWC3_DEPXFERCFG_NUM_XFER_RES(1);
>>  
>> -	return dwc3_send_gadget_ep_cmd(dwc, dep->number,
>> +	ret = dwc3_send_gadget_ep_cmd(dwc, dep->number,
>>  			DWC3_DEPCMD_SETTRANSFRESOURCE, &params);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dwc->current_resource++;
>> +
>> +	return 0;
>>  }
>>  
>>  /**
>> 
>> With this we will *always* use DEPSTARTCFG any time we're enabling an
>> endpoint which hasn't been enabled, but we will always keep transfer
>> resources around. (Note that this won't really work as is, I haven't
>> defined current_resource nor have I made sure to decrement
>> current_resource whenever we disable an endpoint. Also, it might be that
>> using a 32-bit flag instead of a counter might be better, dunno)
>> 
>
> Something like this might work too.
>
> I actually have a patch now which *greatly* simplifies all of this
> code and eliminates the start_config_issued flag. But I need the RTL
> engineers to confirm. It should be ok as it relies on the same
> behavior that this current patch does.
>
> Basically assign all the resources in advance.

I thought about that, but wouldn't this, essentially, enable all
endpoints unconditionally ? This could, potentially, increase power
consumption on some systems, right ? This could also cause "spurious"
interrupts if a bogus host tries to move data on an endpoint which
hasn't been enabled yet.

Not sure this is a good approach here.

-- 
balbi

Attachment: signature.asc
Description: PGP signature