hi,
John Youn <John.Youn@xxxxxxxxxxxx> writes:
> On 5/30/2016 7:19 AM, Felipe Balbi wrote:
>> Instead of looping through all endpoints when
>> enabling ep0, let's allow for each endpoint to set
>> its own xfer resource. This solves an issue which
>> happens when we issue END_TRANSFER due to a reset
>> interrupt. Endpoints will be left without a xfer
>> resource to use.
>>
>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>> ---
>> drivers/usb/dwc3/gadget.c | 25 ++++++++-----------------
>> 1 file changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3d0745dece0c..6f5a4feef8af 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -410,30 +410,21 @@ 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;
>> + if (dep->number == 0) {
>> + memset(¶ms, 0x00, sizeof(params));
>> + cmd = DWC3_DEPCMD_DEPSTARTCFG;
>>
>> - for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
>> - struct dwc3_ep *dep = dwc->eps[i];
>> -
>> - if (!dep)
>> - continue;
>> -
>> - ret = dwc3_gadget_set_xfer_resource(dwc, dep);
>> + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
>> if (ret)
>> return ret;
>> }
>>
>> + ret = dwc3_gadget_set_xfer_resource(dwc, dep);
>> + if (ret)
>> + return ret;
>> +
>> return 0;
>> }
>>
>>
>
> Hi Felipe,
>
> 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?
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4a5453c36099..33897f863ec3 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -768,6 +768,8 @@ 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_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 +915,9 @@ struct dwc3 {
u8 lpm_nyet_threshold;
u8 hird_threshold;
+ u16 current_configuration;
+ u8 current_interface;
+
const char *hsphy_interface;
unsigned connected:1;
@@ -926,6 +931,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..492da0bd88c6 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,27 @@ 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;
+ u8 intf;
+
+ intf = le16_to_cpu(ctrl->wIndex) && 0xff;
+
+ ret = dwc3_ep0_delegate_req(dwc, ctrl);
+ if (ret == 0) {
+ u8 curr = dwc->current_interface;
+
+ if (curr != intf)
+ dwc->start_config_issued = false;
+
+ dwc->current_interface = intf;
+ }
+
+ return ret;
+}
+
static int dwc3_ep0_std_request(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
{
int ret;
@@ -743,6 +770,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..efaf23f0eb6d 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, dwc->eps[1],
+ &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);
--
balbi
Attachment:
signature.asc
Description: PGP signature
