David Brownell <david-b@xxxxxxxxxxx> writes:
> On Friday 16 January 2009, Vernon Sauder wrote:
>> Since the PXA 27x UDC automatically ACK's some control packets such as
>> SET_INTERFACE, the gadgets may not get a chance to process the request
>> before another control packet is received. The Linux gadgets do not expect
>> to receive setup callbacks out of order. The file storage gadget only
>> saves the "highest" priority request.
>
> Hmm, ISTR from waaay back in pre-history that one of the big
> differences between PXA 250 and PXA 255 was that PXA 255 had
> a way to work around that misfeature. The UDCCFR register
> seems to be where that bugfix resided ... yes, set ACM bit
> (ACK Control Mode) to make it wait for the driver to set
> the AREN (ACK Response ENable) flag.
>
> Now, while it's true that the PXA UDC designers seem to be
> the best rationale for the war-on-some-drugs, it'd still
> surprise me to hear that they reverted that design bugfix.
Hey, I like drugs : coca-cola, coffee, tea :)
But I like patches better. So Vernon, would test the patch included here to see
if it solves your issue. I intend to submit it shortly, as my testusb campaign
is still running fine, but I'd like a little "Tested-by" from you, as you're the
only one I know who does reproduce that problem.
Cheers.
--
Robert
>From 7ed87821a3365ffbcd192bf8a97100656ee4ff09 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@xxxxxxx>
Date: Fri, 16 Jan 2009 17:08:49 -0500
Subject: [PATCH] pxa27x_udc: single-thread setup requests
Since the PXA 27x UDC automatically ACK's some control
packets such as SET_INTERFACE, the gadgets may not get a
chance to process the request before another control packet
is received. The Linux gadgets do not expect to receive
setup callbacks out of order. The file storage gadget only
saves the "highest" priority request.
The PXA27x UDC driver must make sure it only sends one up at
a time, allowing the gadget to make changes before
continuing. In theory, the host would be NACK'd while the
gadget processes the change but the UDC has already ACK'd
the request. If another request is sent by the host that is
not automatically ACK'd by the UDC, then the throttling
happens properly to regain sync.
The observed case was the file_storage gadget timing out on
a BulkReset request because the SET_INTERFACE was being
processed by the gadget. Since SET_INTERFACE is higher
priority than BulkReset, the BulkReset was dropped. This
was exacerbated by turning on the debug which delayed the
fsg signal processing thread.
This also fixes the "should never get in
WAIT_ACK_SET_CONF_INTERF state here!!!" warning.
Reported-by: Vernon Sauder <vernoninhand@xxxxxxxxx>
Signed-off-by: Robert Jarzmik <robert.jarzmik@xxxxxxx>
---
drivers/usb/gadget/pxa27x_udc.c | 50 +++++++++++++++++++++++++++-----------
drivers/usb/gadget/pxa27x_udc.h | 2 +
2 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/gadget/pxa27x_udc.c b/drivers/usb/gadget/pxa27x_udc.c
index 51790b0..1937d8c 100644
--- a/drivers/usb/gadget/pxa27x_udc.c
+++ b/drivers/usb/gadget/pxa27x_udc.c
@@ -473,6 +473,23 @@ static inline void udc_clear_mask_UDCCR(struct pxa_udc *udc, int mask)
}
/**
+ * ep_write_UDCCSR - set bits in UDCCSR
+ * @udc: udc device
+ * @mask: bits to set in UDCCR
+ *
+ * Sets bits in UDCCSR (UDCCSR0 and UDCCSR*).
+ *
+ * A specific case is applied to ep0 : the ACM bit is always set to 1, for
+ * SET_INTERFACE and SET_CONFIGURATION.
+ */
+static inline void ep_write_UDCCSR(struct pxa_ep *ep, int mask)
+{
+ if (is_ep0(ep))
+ mask |= UDCCSR0_ACM;
+ udc_ep_writel(ep, UDCCSR, mask);
+}
+
+/**
* ep_count_bytes_remain - get how many bytes in udc endpoint
* @ep: udc endpoint
*
@@ -860,7 +877,7 @@ static int read_packet(struct pxa_ep *ep, struct pxa27x_request *req)
*buf++ = udc_ep_readl(ep, UDCDR);
req->req.actual += count;
- udc_ep_writel(ep, UDCCSR, UDCCSR_PC);
+ ep_write_UDCCSR(ep, UDCCSR_PC);
return count;
}
@@ -968,12 +985,12 @@ static int write_fifo(struct pxa_ep *ep, struct pxa27x_request *req)
if (udccsr & UDCCSR_PC) {
ep_vdbg(ep, "Clearing Transmit Complete, udccsr=%x\n",
udccsr);
- udc_ep_writel(ep, UDCCSR, UDCCSR_PC);
+ ep_write_UDCCSR(ep, UDCCSR_PC);
}
if (udccsr & UDCCSR_TRN) {
ep_vdbg(ep, "Clearing Underrun on, udccsr=%x\n",
udccsr);
- udc_ep_writel(ep, UDCCSR, UDCCSR_TRN);
+ ep_write_UDCCSR(ep, UDCCSR_TRN);
}
count = write_packet(ep, req, max);
@@ -995,7 +1012,7 @@ static int write_fifo(struct pxa_ep *ep, struct pxa27x_request *req)
}
if (is_short)
- udc_ep_writel(ep, UDCCSR, UDCCSR_SP);
+ ep_write_UDCCSR(ep, UDCCSR_SP);
/* requests complete when all IN data is in the FIFO */
if (is_last) {
@@ -1028,7 +1045,7 @@ static int read_ep0_fifo(struct pxa_ep *ep, struct pxa27x_request *req)
while (epout_has_pkt(ep)) {
count = read_packet(ep, req);
- udc_ep_writel(ep, UDCCSR, UDCCSR0_OPC);
+ ep_write_UDCCSR(ep, UDCCSR0_OPC);
inc_ep_stats_bytes(ep, count, !USB_DIR_IN);
is_short = (count < ep->fifo_size);
@@ -1073,7 +1090,7 @@ static int write_ep0_fifo(struct pxa_ep *ep, struct pxa27x_request *req)
/* Sends either a short packet or a 0 length packet */
if (unlikely(is_short))
- udc_ep_writel(ep, UDCCSR, UDCCSR0_IPR);
+ ep_write_UDCCSR(ep, UDCCSR0_IPR);
ep_dbg(ep, "in %d bytes%s%s, %d left, req=%p, udccsr0=0x%03x\n",
count, is_short ? "/S" : "", is_last ? "/L" : "",
@@ -1276,7 +1293,7 @@ static int pxa_ep_set_halt(struct usb_ep *_ep, int value)
/* FST, FEF bits are the same for control and non control endpoints */
rc = 0;
- udc_ep_writel(ep, UDCCSR, UDCCSR_FST | UDCCSR_FEF);
+ ep_write_UDCCSR(ep, UDCCSR_FST | UDCCSR_FEF);
if (is_ep0(ep))
set_ep0state(ep->dev, STALL);
@@ -1342,7 +1359,7 @@ static void pxa_ep_fifo_flush(struct usb_ep *_ep)
udc_ep_readl(ep, UDCDR);
} else {
/* most IN status is the same, but ISO can't stall */
- udc_ep_writel(ep, UDCCSR,
+ ep_write_UDCCSR(ep,
UDCCSR_PC | UDCCSR_FEF | UDCCSR_TRN
| (EPXFERTYPE_is_ISO(ep) ? 0 : UDCCSR_SST));
}
@@ -1727,6 +1744,7 @@ static void udc_enable(struct pxa_udc *udc)
memset(&udc->stats, 0, sizeof(udc->stats));
udc_set_mask_UDCCR(udc, UDCCR_UDE);
+ ep_write_UDCCSR(&udc->pxa_ep[0], UDCCSR0_ACM);
udelay(2);
if (udc_readl(udc, UDCCR) & UDCCR_EMCE)
dev_err(udc->dev, "Configuration errors, udc disabled\n");
@@ -1899,7 +1917,7 @@ static void handle_ep0_ctrl_req(struct pxa_udc *udc,
* packet. Generalize to pxa27x CPUs.
*/
if (epout_has_pkt(ep) && (ep_count_bytes_remain(ep) == 0))
- udc_ep_writel(ep, UDCCSR, UDCCSR0_OPC);
+ ep_write_UDCCSR(ep, UDCCSR0_OPC);
/* read SETUP packet */
for (i = 0; i < 2; i++) {
@@ -1927,7 +1945,7 @@ static void handle_ep0_ctrl_req(struct pxa_udc *udc,
set_ep0state(udc, OUT_DATA_STAGE);
/* Tell UDC to enter Data Stage */
- udc_ep_writel(ep, UDCCSR, UDCCSR0_SA | UDCCSR0_OPC);
+ ep_write_UDCCSR(ep, UDCCSR0_SA | UDCCSR0_OPC);
i = udc->driver->setup(&udc->gadget, &u.r);
if (i < 0)
@@ -1937,7 +1955,7 @@ out:
stall:
ep_dbg(ep, "protocol STALL, udccsr0=%03x err %d\n",
udc_ep_readl(ep, UDCCSR), i);
- udc_ep_writel(ep, UDCCSR, UDCCSR0_FST | UDCCSR0_FTF);
+ ep_write_UDCCSR(ep, UDCCSR0_FST | UDCCSR0_FTF);
set_ep0state(udc, STALL);
goto out;
}
@@ -2008,7 +2026,7 @@ static void handle_ep0(struct pxa_udc *udc, int fifo_irq, int opc_irq)
if (udccsr0 & UDCCSR0_SST) {
ep_dbg(ep, "clearing stall status\n");
nuke(ep, -EPIPE);
- udc_ep_writel(ep, UDCCSR, UDCCSR0_SST);
+ ep_write_UDCCSR(ep, UDCCSR0_SST);
ep0_idle(udc);
}
@@ -2033,7 +2051,7 @@ static void handle_ep0(struct pxa_udc *udc, int fifo_irq, int opc_irq)
break;
case IN_DATA_STAGE: /* GET_DESCRIPTOR */
if (epout_has_pkt(ep))
- udc_ep_writel(ep, UDCCSR, UDCCSR0_OPC);
+ ep_write_UDCCSR(ep, UDCCSR0_OPC);
if (req && !ep_is_full(ep))
completed = write_ep0_fifo(ep, req);
if (completed)
@@ -2046,7 +2064,7 @@ static void handle_ep0(struct pxa_udc *udc, int fifo_irq, int opc_irq)
ep0_end_out_req(ep, req);
break;
case STALL:
- udc_ep_writel(ep, UDCCSR, UDCCSR0_FST);
+ ep_write_UDCCSR(ep, UDCCSR0_FST);
break;
case IN_STATUS_STAGE:
/*
@@ -2141,6 +2159,7 @@ static void pxa27x_change_configuration(struct pxa_udc *udc, int config)
set_ep0state(udc, WAIT_ACK_SET_CONF_INTERF);
udc->driver->setup(&udc->gadget, &req);
+ ep_write_UDCCSR(&udc->pxa_ep[0], UDCCSR0_AREN);
}
/**
@@ -2169,6 +2188,7 @@ static void pxa27x_change_interface(struct pxa_udc *udc, int iface, int alt)
set_ep0state(udc, WAIT_ACK_SET_CONF_INTERF);
udc->driver->setup(&udc->gadget, &req);
+ ep_write_UDCCSR(&udc->pxa_ep[0], UDCCSR0_AREN);
}
/*
@@ -2290,7 +2310,7 @@ static void irq_udc_reset(struct pxa_udc *udc)
memset(&udc->stats, 0, sizeof udc->stats);
nuke(ep, -EPROTO);
- udc_ep_writel(ep, UDCCSR, UDCCSR0_FTF | UDCCSR0_OPC);
+ ep_write_UDCCSR(ep, UDCCSR0_FTF | UDCCSR0_OPC);
ep0_idle(udc);
}
diff --git a/drivers/usb/gadget/pxa27x_udc.h b/drivers/usb/gadget/pxa27x_udc.h
index db58125..e25225e 100644
--- a/drivers/usb/gadget/pxa27x_udc.h
+++ b/drivers/usb/gadget/pxa27x_udc.h
@@ -130,6 +130,8 @@
#define UP2OCR_HXOE (1 << 17) /* Transceiver Output Enable */
#define UP2OCR_SEOS (1 << 24) /* Single-Ended Output Select */
+#define UDCCSR0_ACM (1 << 9) /* Ack Control Mode */
+#define UDCCSR0_AREN (1 << 8) /* Ack Response Enable */
#define UDCCSR0_SA (1 << 7) /* Setup Active */
#define UDCCSR0_RNE (1 << 6) /* Receive FIFO Not Empty */
#define UDCCSR0_FST (1 << 5) /* Force Stall */
--
1.6.2.1
--
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
