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.
--
balbi
signature.asc
Description: PGP signature
