On 2/10/2016 2:11 AM, Felipe Balbi wrote:
>
> 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(¶ms, 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, ¶ms);
>>> + 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, ¶ms);
>>> }
>>>
>>> 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(¶ms, 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, ¶ms);
>>> + 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.
No, I mean to just assign resources withouth configuring or enabling
the endpoint. I have tested this approach and it works. But I still
need to verify that it won't conflict with anything, such as streams.
>
> Not sure this is a good approach here.
>
In any case, I will also resend the cleaned up version of this patch
as well.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
