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

[Felipe Balbi]

Hi,

John Youn <john.y...@synopsys.com> 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

signature.asc
Description: PGP signature