On 2/16/2016 1:28 PM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn <john.y...@synopsys.com> writes:
>> The assignement of EP transfer resources was not handled properly in the
>> dwc3 driver. Commit aebda6187181 ("usb: dwc3: Reset the transfer
>> resource index on SET_INTERFACE") previously fixed one aspect of this
>> where resources may be exhausted with multiple calls to SET_INTERFACE.
>> However, it introduced an issue where composite devices with multiple
>> interfaces can be assigned the same transfer resources for different
>> endpoints. This patch solves both issues.
>>
>> The assignment of transfer resources cannot perfectly follow the data
>> book due to the fact that the controller driver does not have all
>> knowledge of the configuration in advance. It is given this information
>> piecemeal by the composite gadget framework after every
>> SET_CONFIGURATION and SET_INTERFACE. Trying to follow the databook
>> programming model in this scenario can cause errors. For two reasons:
>>
>> 1) The databook says to do DEPSTARTCFG for every SET_CONFIGURATION and
>> SET_INTERFACE (8.1.5). This is incorrect in the scenario of multiple
>> interfaces.
>>
>> 2) The databook does not mention doing more DEPXFERCFG for new endpoint
>> on alt setting (8.1.6).
>>
>> The following simplified method is used instead:
>>
>> All hardware endpoints can be assigned a transfer resource and this
>> setting will stay persistent until either a core reset or hibernation.
>> So whenever we do a DEPSTARTCFG(0) we can go ahead and do DEPXFERCFG for
>> every hardware endpoint as well. We are guaranteed that there are as
>> many transfer resources as endpoints.
>>
>> This patch triggers off of the calling dwc3_gadget_start_config() for
>> EP0-out, which always happens first, and which should only happen in one
>> of the above conditions.
>
> very good commit log. Thank you :-)
>
>> Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on
>> SET_INTERFACE")
>
> you missed:
>
> Cc: <sta...@vger.kernel.org> # v....
>
>> Reported-by: Ravi Babu <ravib...@ti.com>
>> Signed-off-by: John Youn <johny...@synopsys.com>
>> ---
>>
>> Hi Ravi,
>>
>> This is a simplified version of the previous patch. Could you verify
>> that it works with your test scenario?
>>
>> Thanks,
>> John
>>
>> v2:
>> * Simplified assignment of resources by doing all endpoints at once.
>>
>>
>> drivers/usb/dwc3/core.h | 1 -
>> drivers/usb/dwc3/ep0.c | 5 -----
>> drivers/usb/dwc3/gadget.c | 38 ++++++++++++++++++++------------------
>> 3 files changed, 20 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 2913068..e4f8b90 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -856,7 +856,6 @@ struct dwc3 {
>> unsigned pullups_connected:1;
>> unsigned resize_fifos:1;
>> unsigned setup_packet_pending:1;
>> - unsigned start_config_issued:1;
>> unsigned three_stage_setup:1;
>> unsigned usb3_lpm_capable:1;
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 3a9354a..8d6b75c 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -555,7 +555,6 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct
>> usb_ctrlrequest *ctrl)
>> int ret;
>> u32 reg;
>>
>> - dwc->start_config_issued = false;
>> cfg = le16_to_cpu(ctrl->wValue);
>>
>> switch (state) {
>> @@ -737,10 +736,6 @@ static int dwc3_ep0_std_request(struct dwc3 *dwc,
>> struct usb_ctrlrequest *ctrl)
>> dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_ISOCH_DELAY");
>> ret = dwc3_ep0_set_isoch_delay(dwc, ctrl);
>> break;
>> - case USB_REQ_SET_INTERFACE:
>> - dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_INTERFACE");
>> - dwc->start_config_issued = false;
>> - /* Fall through */
>> default:
>> dwc3_trace(trace_dwc3_ep0, "Forwarding to gadget driver");
>> ret = dwc3_ep0_delegate_req(dwc, ctrl);
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 7d1dd82..ae2e546 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -385,24 +385,34 @@ static void dwc3_free_trb_pool(struct dwc3_ep *dep)
>> dep->trb_pool_dma = 0;
>> }
>>
>> +static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep
>> *dep);
>> +
>> static int dwc3_gadget_start_config(struct dwc3 *dwc, struct dwc3_ep *dep)
>> {
>> struct dwc3_gadget_ep_cmd_params params;
>> u32 cmd;
>> + int i, ret;
>
> I'd prefer if these two variables would be split into their own lines:
>
> int ret;
> int i;
>
>> + if (dep->number)
>> + return 0;
>>
>> memset(¶ms, 0x00, sizeof(params));
>> + cmd = DWC3_DEPCMD_DEPSTARTCFG;
>>
>> - 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);
>> - }
>> + ret = dwc3_send_gadget_ep_cmd(dwc, 0, cmd, ¶ms);
>> + if (ret)
>> + return ret;
>> +
>> + for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
>> + int epnum, dir;
>
> ditto. Also, (nit-picking, sorry) I'd prefer if the structure
> declaration would come first:
>
> struct dwc3_ep *dep = dwc->eps[i];
> int epnum;
> int dir;
>
> also, do you mind adding a comment somewhere here explaining that we're
> not following the databook for a reason... It could be an almost
> carbon-copy of your commit log, actually.
>
Ok. I'll resubmit with your feedback.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
