hi,
Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
>> This reverts back to the original buggy behavior. This will fail when
>> a SET_INTERFACE occurs multiple times.
>>
>> You can run testusb to see the failure:
>> testusb -t 9 -c 5000 -s 2048 -a
>
> I came up with something else for this. It's still unstable and I need
> to debug further to figure out where we're wrong. But it seems to me
> that we're following databook down to the last comma with patch
> below. Note that we're partially reverting your original changes and
> adding some extra knowledge about current configuration and interface,
> then we only reassign transfer resources if those change. Care to have a
> look?
here's a version that passes testusb and normal enumeration with g_zero
and g_mass_storage. After some experimenting, it seems like we should
always MODIFY resource allocation, unless we're doing a SetConfiguration
to the same configuration that is already chosen or a SetInterface to
the same interface/alt-setting pair that's already being used. This is
working for me. Can you test on your end too?
8<---------------------------------------------------------------------
From 327475d7f63aa161bdc3bf7f9d693d9aafcd4518 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
Date: Thu, 2 Jun 2016 12:47:13 +0300
Subject: [PATCH] usb: dwc3: gadget: fix endpoint resource allocation
There were still corner cases which current code
wouldn't cover. This new commit tries to cover them
all by partially reverting commit c450960187f4
("usb: dwc3: Fix assignment of EP transfer
resources") and implementing a new way which follows
Synopsys Databook correctly.
Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
---
drivers/usb/dwc3/core.h | 8 +++++
drivers/usb/dwc3/ep0.c | 41 ++++++++++++++++++++++-
drivers/usb/dwc3/gadget.c | 84 ++++++++++++++++++-----------------------------
3 files changed, 80 insertions(+), 53 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4a5453c36099..8fb6361d719d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -768,6 +768,9 @@ struct dwc3_scratchpad_array {
* @test_mode_nr: test feature selector
* @lpm_nyet_threshold: LPM NYET response threshold
* @hird_threshold: HIRD threshold
+ * @current_configuration: current configuration number
+ * @current_alt_setting: current alternate setting
+ * @current_interface: current interface number
* @hsphy_interface: "utmi" or "ulpi"
* @connected: true when we're connected to a host, false otherwise
* @delayed_status: true when gadget driver asks for delayed status
@@ -913,6 +916,10 @@ struct dwc3 {
u8 lpm_nyet_threshold;
u8 hird_threshold;
+ u16 current_configuration;
+ u16 current_alt_setting;
+ u16 current_interface;
+
const char *hsphy_interface;
unsigned connected:1;
@@ -926,6 +933,7 @@ struct dwc3 {
unsigned pending_events:1;
unsigned pullups_connected: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 fe79d771dee4..ff07122db8d3 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -576,6 +576,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
/* if the cfg matches and the cfg is non zero */
if (cfg && (!ret || (ret == USB_GADGET_DELAYED_STATUS))) {
+ if (dwc->current_configuration != cfg)
+ dwc->start_config_issued = false;
+ dwc->current_configuration = cfg;
+
/*
* only change state if set_config has already
* been processed. If gadget driver returns
@@ -598,9 +602,11 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
case USB_STATE_CONFIGURED:
ret = dwc3_ep0_delegate_req(dwc, ctrl);
- if (!cfg && !ret)
+ if (!cfg && !ret) {
usb_gadget_set_state(&dwc->gadget,
USB_STATE_ADDRESS);
+ dwc->current_configuration = 0;
+ }
break;
default:
ret = -EINVAL;
@@ -710,6 +716,35 @@ static int dwc3_ep0_set_isoch_delay(struct dwc3 *dwc, struct usb_ctrlrequest *ct
return 0;
}
+static int dwc3_ep0_set_interface(struct dwc3 *dwc,
+ struct usb_ctrlrequest *ctrl)
+{
+ int ret;
+ u16 intf;
+ u16 alt;
+
+ intf = le16_to_cpu(ctrl->wIndex);
+ alt = le16_to_cpu(ctrl->wValue);
+
+ ret = dwc3_ep0_delegate_req(dwc, ctrl);
+ if (ret == 0) {
+ u16 curr;
+
+ curr = dwc->current_interface;
+ if (curr != intf)
+ dwc->start_config_issued = false;
+
+ curr = dwc->current_alt_setting;
+ if (curr != alt)
+ dwc->start_config_issued = false;
+
+ dwc->current_interface = intf;
+ dwc->current_alt_setting = alt;
+ }
+
+ return ret;
+}
+
static int dwc3_ep0_std_request(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
{
int ret;
@@ -743,6 +778,10 @@ 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");
+ ret = dwc3_ep0_set_interface(dwc, ctrl);
+ break;
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 32f97c73499c..866470f7207f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -372,66 +372,23 @@ 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);
-
-/**
- * dwc3_gadget_start_config - Configure EP resources
- * @dwc: pointer to our controller context structure
- * @dep: endpoint that is being enabled
- *
- * 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 function is called for each endpoint when it is being enabled
- * but is triggered only when called for EP0-out, which always happens
- * first, and which should only happen in one of the above conditions.
- */
static int dwc3_gadget_start_config(struct dwc3 *dwc, struct dwc3_ep *dep)
{
struct dwc3_gadget_ep_cmd_params params;
u32 cmd;
- int i;
- int ret;
-
- if (dep->number)
- return 0;
memset(¶ms, 0x00, sizeof(params));
- cmd = DWC3_DEPCMD_DEPSTARTCFG;
- ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
- if (ret)
- return ret;
-
- for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
- struct dwc3_ep *dep = dwc->eps[i];
-
- if (!dep)
- continue;
+ if (dep->number != 1) {
+ cmd = DWC3_DEPCMD_DEPSTARTCFG;
+ /* XferRscIdx == 0 for ep0 and 2 for the remaining */
+ if (dwc->start_config_issued)
+ return 0;
+ dwc->start_config_issued = true;
+ if (dep->number > 1)
+ cmd |= DWC3_DEPCMD_PARAM(2);
- ret = dwc3_gadget_set_xfer_resource(dwc, dep);
- if (ret)
- return ret;
+ return dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
}
return 0;
@@ -517,6 +474,8 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep *dep)
¶ms);
}
+static struct usb_endpoint_descriptor dwc3_gadget_ep0_desc;
+
/**
* __dwc3_gadget_ep_enable - Initializes a HW endpoint
* @dep: endpoint to be initialized
@@ -536,6 +495,19 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
dwc3_trace(trace_dwc3_gadget, "Enabling %s", dep->name);
if (!(dep->flags & DWC3_EP_ENABLED)) {
+ if (dep->number == 1 && !dwc->start_config_issued) {
+ /*
+ * According to section 8.1.5 of the databook,
+ * we must first issue a DEPCFG command to EP1
+ * with action set to modify
+ */
+ ret = dwc3_gadget_set_ep_config(dwc, dep,
+ &dwc3_gadget_ep0_desc, NULL, true,
+ false);
+ if (ret)
+ return ret;
+ }
+
ret = dwc3_gadget_start_config(dwc, dep);
if (ret)
return ret;
@@ -550,6 +522,10 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
struct dwc3_trb *trb_st_hw;
struct dwc3_trb *trb_link;
+ ret = dwc3_gadget_set_xfer_resource(dwc, dep);
+ if (ret)
+ return ret;
+
dep->endpoint.desc = desc;
dep->comp_desc = comp_desc;
dep->type = usb_endpoint_type(desc);
@@ -1718,6 +1694,8 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
dwc3_gadget_setup_nump(dwc);
+ dwc->start_config_issued = false;
+
/* Start with SuperSpeed Default */
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
@@ -2373,6 +2351,7 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_DCTL, reg);
dwc3_disconnect_gadget(dwc);
+ dwc->start_config_issued = false;
dwc->gadget.speed = USB_SPEED_UNKNOWN;
dwc->setup_packet_pending = false;
@@ -2420,6 +2399,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
dwc3_stop_active_transfers(dwc);
dwc3_clear_stall_all_ep(dwc);
+ dwc->start_config_issued = false;
dwc3_reset_gadget(dwc);
--
2.8.3
--
balbi
Attachment:
signature.asc
Description: PGP signature
