On Tuesday 21 April 2009, Robert Jarzmik 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.
>
> 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>
Acked-by: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
... just be glad you're not working with a pxa250, from
the days when this family of UDC was even *more* broken!
> ---
> 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
