I think that little mail got lost somehow ... I hate Paris.
----- Mail transféré -----
De: "robert jarzmik" <robert.jarzmik@xxxxxxx>
À: "Vernon Sauder" <vernoninhand@xxxxxxxxx>
Cc: linux-usb@xxxxxxxxxxxxxxx
Envoyé: Mercredi 21 Janvier 2009 13:13:59 GMT +01:00 Amsterdam / Berlin / Berne / Rome / Stockholm / Vienne
Objet: Re: [PATCH 7/9] pxa27x_udc: single-thread setup requests
Hi Vernon,
I did the review, and I'm thinking as David that a simpler solution exists.
Now, I need your help because :
(a) I'm stuck in a hotel with no wifi connectivity
(b) I'm writing this through GPRS, and beginning to long for my old 9600 bauds modem
(c) Have a proposal about this patch
(d) Have no way to test it
So, this is the deal : you try the patch that is joined to that mail, and if it works, you take credit for the patch. If it doesn't, I'll have a serious conversation with you about the options we've got.
Cheers.
--
Robert
PS: I didn't see your replies to the other patches, and will address them as soon as I'm back to my sanctuary (ie. house/airport/whatever with DSL/WIFI connectivity).
>From 34f64c5000b83750b2640bb9ee37517563e6fc7e Mon Sep 17 00:00:00 2001
From: Vernon Sauder <vsauder@xxxxxxxxxx>
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.
---
drivers/usb/gadget/pxa27x_udc.c | 42 ++++++++++++++++++++++++++------------
drivers/usb/gadget/pxa27x_udc.h | 2 +
2 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/pxa27x_udc.c b/drivers/usb/gadget/pxa27x_udc.c
index 9858fe2..1c70eec 100644
--- a/drivers/usb/gadget/pxa27x_udc.c
+++ b/drivers/usb/gadget/pxa27x_udc.c
@@ -485,6 +485,20 @@ static inline void udc_clear_mask_UDCCR(struct pxa_udc *udc, int mask)
}
/**
+ * ep_set_mask_UDCCSR - set bits in UDCCSR
+ * @udc: udc device
+ * @mask: bits to set in UDCCR
+ *
+ * Sets bits in UDCCSR (UDCCSR0 and UDCCSR*).
+ */
+static inline void ep_set_mask_UDCCSR(struct pxa_ep *ep, int mask)
+{
+ u32 udccsr = udc_ep_readl(ep, UDCCSR);
+
+ udc_ep_writel(ep, UDCCSR, udccsr | mask);
+}
+
+/**
* ep_count_bytes_remain - get how many bytes in udc endpoint
* @ep: udc endpoint
*
@@ -872,7 +886,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_set_mask_UDCCSR(ep, UDCCSR_PC);
return count;
}
@@ -980,12 +994,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_set_mask_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_set_mask_UDCCSR(ep, UDCCSR_TRN);
}
count = write_packet(ep, req, max);
@@ -1007,7 +1021,7 @@ static int write_fifo(struct pxa_ep *ep, struct pxa27x_request *req)
}
if (is_short)
- udc_ep_writel(ep, UDCCSR, UDCCSR_SP);
+ ep_set_mask_UDCCSR(ep, UDCCSR_SP);
/* requests complete when all IN data is in the FIFO */
if (is_last) {
@@ -1040,7 +1054,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_set_mask_UDCCSR(ep, UDCCSR0_OPC);
inc_ep_stats_bytes(ep, count, !USB_DIR_IN);
is_short = (count < ep->fifo_size);
@@ -1085,7 +1099,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_set_mask_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" : "",
@@ -1293,7 +1307,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_set_mask_UDCCSR(ep, UDCCSR_FST | UDCCSR_FEF);
if (is_ep0(ep))
set_ep0state(ep->dev, STALL);
@@ -1359,7 +1373,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_set_mask_UDCCSR(ep,
UDCCSR_PC | UDCCSR_FEF | UDCCSR_TRN
| (EPXFERTYPE_is_ISO(ep) ? 0 : UDCCSR_SST));
}
@@ -1591,6 +1605,7 @@ static void udc_enable(struct pxa_udc *udc)
memset(&udc->stats, 0, sizeof(udc->stats));
udc_set_mask_UDCCR(udc, UDCCR_UDE);
+ ep_set_mask_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");
@@ -1765,7 +1780,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_set_mask_UDCCSR(ep, UDCCSR0_SA | UDCCSR0_OPC);
i = udc->driver->setup(&udc->gadget, &u.r);
if (i < 0)
@@ -1775,7 +1790,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_set_mask_UDCCSR(ep, UDCCSR0_FST | UDCCSR0_FTF);
set_ep0state(udc, STALL);
goto out;
}
@@ -1869,7 +1884,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_set_mask_UDCCSR(ep, UDCCSR0_OPC);
if (req && !ep_is_full(ep))
completed = write_ep0_fifo(ep, req);
if (completed)
@@ -1882,7 +1897,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_set_mask_UDCCSR(ep, UDCCSR0_FST);
break;
case IN_STATUS_STAGE:
/*
@@ -1977,6 +1992,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_set_mask_UDCCSR(&udc->pxa_ep[0], UDCCSR0_AREN);
}
/**
@@ -2126,7 +2142,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_set_mask_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 1d1b793..407cbb8 100644
--- a/drivers/usb/gadget/pxa27x_udc.h
+++ b/drivers/usb/gadget/pxa27x_udc.h
@@ -129,6 +129,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.5.6.5
