Date: Thu, 18 Jul 2013 10:08:59 +0200 From: Hans Petter Selasky <hps@bitfrost.no> To: Oleksandr Tymoshenko <gonzo@bluezbox.com> Cc: arm@freebsd.org, usb@freebsd.org Subject: Re: Beaglebone USB driver (Mentor Graphics OTG) Message-ID: <51E7A29B.6040701@bitfrost.no> In-Reply-To: <49E5BE45-208C-4AAD-980D-590F32D9B600@bluezbox.com> References: <51608AA4.2020804@bluezbox.com> <51611A7B.2010105@bitfrost.no> <0927BB4C-6917-408D-B102-AB98F72314B6@bluezbox.com> <51CBDFEA.7050203@bitfrost.no> <A9072D24-1E28-47D2-A152-D962C74D1811@bluezbox.com> <49E5BE45-208C-4AAD-980D-590F32D9B600@bluezbox.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------070101070502010609080900
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Hi Oleksandr,
Got some time to review the code you committed to HEAD:
1) You need to distinguish between STALL and other errors. This is a bit
important, because STALL is a PID and other errors can mean anything.
See attached patch.
2) You should not use the NAKTO feature, because USB devices are allowed
to NAK forever if they wish to. You should restart a USB transfer
seamlessly after NAKTO or should use infinite NAKing. The USB stack has
a timeout on the transfers, and will cancel when timeout happens. By
feeding these error codes into the USB stack you risk loosing data on
mouse/keyboard/modem devices! Upon cancel you need to use that trick I
told you (included in the attached patch), else the double reset of the
FIFO will crash! Could you clean this up and test a bit. I don't have
hardware at hand to test at the moment.
Thank you!
--HPS
--------------070101070502010609080900
Content-Type: text/x-patch;
name="musb_check_for_stall.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="musb_check_for_stall.diff"
diff --git a/sys/dev/usb/controller/musb_otg.c b/sys/dev/usb/controller/musb_otg.c
index 5113d7a..bac8dd9 100644
--- a/sys/dev/usb/controller/musb_otg.c
+++ b/sys/dev/usb/controller/musb_otg.c
@@ -169,6 +169,9 @@ musbotg_channel_alloc(struct musbotg_softc *sc, struct musbotg_td *td)
if (sc->sc_channel_mask & (1 << 0))
return (-1);
sc->sc_channel_mask |= (1 << 0);
+ /* Clear previous status bits */
+ MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
+ /* Enable interrupts */
musbotg_ep_int_set(sc, ep, 1);
return (0);
}
@@ -176,6 +179,9 @@ musbotg_channel_alloc(struct musbotg_softc *sc, struct musbotg_td *td)
for (ch = 1; ch < MUSB2_EP_MAX; ch++) {
if (!(sc->sc_channel_mask & (1 << ch))) {
sc->sc_channel_mask |= (1 << ch);
+ /* Clear previous status bits */
+ MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
+ /* Enable interrupts */
musbotg_ep_int_set(sc, ch, 1);
return (ch);
}
@@ -203,6 +209,19 @@ musbotg_channel_free(struct musbotg_softc *sc, struct musbotg_td *td)
musbotg_ep_int_set(sc, td->channel, 0);
sc->sc_channel_mask &= ~(1 << td->channel);
+ /* select endpoint */
+ MUSB2_WRITE_1(sc, MUSB2_REG_EPINDEX, td->channel);
+
+ /* clear NAK limit */
+ MUSB2_WRITE_1(sc, MUSB2_REG_TXNAKLIMIT, 0);
+
+ /*
+ * Set non-existing device address so that any NAK'ing only
+ * transfers get cancelled! Should wait 250us before using
+ * channel again.
+ */
+ MUSB2_WRITE_1(sc, MUSB2_REG_TXFADDR(td->channel), MUSB2_RST_ADDR);
+
td->channel = -1;
}
@@ -518,13 +537,15 @@ musbotg_host_ctrl_setup_tx(struct musbotg_td *td)
return (1);
/* Failed */
- if (csr & (MUSB2_MASK_CSR0L_RXSTALL |
- MUSB2_MASK_CSR0L_ERROR))
- {
- /* Clear status bit */
- MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
+ if (csr & MUSB2_MASK_CSR0L_RXSTALL) {
DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
- td->error = 1;
+ td->error_any = 1;
+ td->error_stall = 1;
+ return (0); /* complete */
+ } else if (csr & MUSB2_MASK_CSR0L_ERROR) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ return (0); /* complete */
}
if (csr & MUSB2_MASK_CSR0L_NAKTIMO) {
@@ -546,10 +567,10 @@ musbotg_host_ctrl_setup_tx(struct musbotg_td *td)
csr &= ~MUSB2_MASK_CSR0L_NAKTIMO;
MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, csr);
- td->error = 1;
+ td->error_any = 1;
}
- if (td->error) {
+ if (td->error_any) {
musbotg_channel_free(sc, td);
return (0);
}
@@ -636,7 +657,7 @@ musbotg_dev_ctrl_data_rx(struct musbotg_td *td)
/*
* USB Host Aborted the transfer.
*/
- td->error = 1;
+ td->error_any = 1;
return (0); /* complete */
}
if (!(csr & MUSB2_MASK_CSR0L_RXPKTRDY)) {
@@ -653,14 +674,14 @@ musbotg_dev_ctrl_data_rx(struct musbotg_td *td)
got_short = 1;
} else {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
return (0); /* we are complete */
}
}
/* verify the packet byte count */
if (count > td->remainder) {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
return (0); /* we are complete */
}
while (count > 0) {
@@ -769,7 +790,7 @@ musbotg_dev_ctrl_data_tx(struct musbotg_td *td)
* The current transfer was aborted
* by the USB Host
*/
- td->error = 1;
+ td->error_any = 1;
return (0); /* complete */
}
if (csr & MUSB2_MASK_CSR0L_TXPKTRDY) {
@@ -911,22 +932,19 @@ musbotg_host_ctrl_data_rx(struct musbotg_td *td)
csr &= ~MUSB2_MASK_CSR0L_NAKTIMO;
MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, csr);
- td->error = 1;
+ td->error_any = 1; XXX;
}
/* Failed */
- if (csr & (MUSB2_MASK_CSR0L_RXSTALL |
- MUSB2_MASK_CSR0L_ERROR))
- {
- /* Clear status bit */
- MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
+ if (csr & MUSB2_MASK_CSR0L_RXSTALL) {
DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
- td->error = 1;
- }
-
- if (td->error) {
- musbotg_channel_free(sc, td);
- return (0); /* we are complete */
+ td->error_any = 1;
+ td->error_stall = 1;
+ return (0); /* complete */
+ } else if (csr & MUSB2_MASK_CSR0L_ERROR) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ return (0); /* complete */
}
if (!(csr & MUSB2_MASK_CSR0L_RXPKTRDY))
@@ -943,7 +961,7 @@ musbotg_host_ctrl_data_rx(struct musbotg_td *td)
got_short = 1;
} else {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* we are complete */
}
@@ -951,7 +969,7 @@ musbotg_host_ctrl_data_rx(struct musbotg_td *td)
/* verify the packet byte count */
if (count > td->remainder) {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* we are complete */
}
@@ -1064,11 +1082,16 @@ musbotg_host_ctrl_data_tx(struct musbotg_td *td)
csr = MUSB2_READ_1(sc, MUSB2_REG_TXCSRL);
DPRINTFN(4, "csr=0x%02x\n", csr);
- if (csr & (MUSB2_MASK_CSR0L_RXSTALL |
- MUSB2_MASK_CSR0L_ERROR)) {
- /* clear status bits */
- MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
- td->error = 1;
+ /* Failed */
+ if (csr & MUSB2_MASK_CSR0L_RXSTALL) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ td->error_stall = 1;
+ return (0); /* complete */
+ } else if (csr & MUSB2_MASK_CSR0L_ERROR) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ return (0); /* complete */
}
if (csr & MUSB2_MASK_CSR0L_NAKTIMO ) {
@@ -1089,11 +1112,11 @@ musbotg_host_ctrl_data_tx(struct musbotg_td *td)
csr &= ~MUSB2_MASK_CSR0L_NAKTIMO;
MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, csr);
- td->error = 1;
+ td->error_any = 1;
}
- if (td->error) {
+ if (td->error_any) {
musbotg_channel_free(sc, td);
return (0); /* complete */
}
@@ -1315,24 +1338,20 @@ musbotg_host_ctrl_status_rx(struct musbotg_td *td)
csr &= ~MUSB2_MASK_CSR0L_NAKTIMO;
MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, csr);
- td->error = 1;
+ td->error_any = 1;
}
/* Failed */
- if (csr & (MUSB2_MASK_CSR0L_RXSTALL |
- MUSB2_MASK_CSR0L_ERROR))
- {
- /* Clear status bit */
- MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
+ if (csr & MUSB2_MASK_CSR0L_RXSTALL) {
DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
- td->error = 1;
- }
-
- if (td->error) {
- musbotg_channel_free(sc, td);
- return (0);
+ td->error_any = 1;
+ td->error_stall = 1;
+ return (0); /* complete */
+ } else if (csr & MUSB2_MASK_CSR0L_ERROR) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ return (0); /* complete */
}
-
return (1); /* Not ready yet */
}
@@ -1366,15 +1385,15 @@ musbotg_host_ctrl_status_tx(struct musbotg_td *td)
return (1);
/* Failed */
- if (csr & (MUSB2_MASK_CSR0L_RXSTALL |
- MUSB2_MASK_CSR0L_ERROR))
- {
- /* Clear status bit */
- MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
+ if (csr & MUSB2_MASK_CSR0L_RXSTALL) {
DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
- td->error = 1;
- musbotg_channel_free(sc, td);
- return (0); /* complete */
+ td->error_any = 1;
+ td->error_stall = 1;
+ return (0); /* complete */
+ } else if (csr & MUSB2_MASK_CSR0L_ERROR) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ return (0); /* complete */
}
if (td->transaction_started) {
@@ -1460,7 +1479,7 @@ repeat:
got_short = 1;
} else {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* we are complete */
}
@@ -1468,7 +1487,7 @@ repeat:
/* verify the packet byte count */
if (count > td->remainder) {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* we are complete */
}
@@ -1766,22 +1785,19 @@ repeat:
MUSB2_WRITE_1(sc, MUSB2_REG_RXCSRL, csr);
}
- td->error = 1;
- }
-
- if (csr & MUSB2_MASK_CSRL_RXERROR) {
- DPRINTFN(4, "RXERROR\n");
- td->error = 1;
- }
-
- if (csr & MUSB2_MASK_CSRL_RXSTALL) {
- DPRINTFN(4, "RXSTALL\n");
- td->error = 1;
+ td->error_any = 1;
}
- if (td->error) {
- musbotg_channel_free(sc, td);
- return (0); /* we are complete */
+ /* Failed */
+ if (csr & MUSB2_MASK_CSR0L_RXSTALL) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ td->error_stall = 1;
+ return (0); /* complete */
+ } else if (csr & MUSB2_MASK_CSR0L_ERROR) {
+ DPRINTFN(1, "error bit set, csr=0x%02x\n", csr);
+ td->error_any = 1;
+ return (0); /* complete */
}
if (!(csr & MUSB2_MASK_CSRL_RXPKTRDY)) {
@@ -1804,7 +1820,7 @@ repeat:
got_short = 1;
} else {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* we are complete */
}
@@ -1813,7 +1829,7 @@ repeat:
/* verify the packet byte count */
if (count > td->remainder) {
/* invalid USB packet */
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* we are complete */
}
@@ -1933,7 +1949,7 @@ musbotg_host_data_tx(struct musbotg_td *td)
MUSB2_MASK_CSRL_TXERROR)) {
/* clear status bits */
MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, 0);
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* complete */
}
@@ -1956,7 +1972,7 @@ musbotg_host_data_tx(struct musbotg_td *td)
csr &= ~MUSB2_MASK_CSRL_TXNAKTO;
MUSB2_WRITE_1(sc, MUSB2_REG_TXCSRL, csr);
- td->error = 1;
+ td->error_any = 1;
musbotg_channel_free(sc, td);
return (0); /* complete */
}
@@ -2122,7 +2138,7 @@ musbotg_xfer_do_fifo(struct usb_xfer *xfer)
if (((void *)td) == xfer->td_transfer_last) {
goto done;
}
- if (td->error) {
+ if (td->error_any) {
goto done;
} else if (td->remainder > 0) {
/*
@@ -2350,7 +2366,8 @@ musbotg_setup_standard_chain_sub(struct musbotg_std_temp *temp)
td->pc = temp->pc;
td->offset = temp->offset;
td->remainder = temp->len;
- td->error = 0;
+ td->error_any = 0;
+ td->error_stall = 0;
td->transaction_started = 0;
td->did_stall = temp->did_stall;
td->short_pkt = temp->short_pkt;
@@ -2674,7 +2691,7 @@ musbotg_standard_done_sub(struct usb_xfer *xfer)
{
struct musbotg_td *td;
uint32_t len;
- uint8_t error;
+ usb_error_t error;
DPRINTFN(8, "\n");
@@ -2691,15 +2708,16 @@ musbotg_standard_done_sub(struct usb_xfer *xfer)
* the remainder from "frlengths[]":
*/
if (len > xfer->frlengths[xfer->aframes]) {
- td->error = 1;
+ td->error_any = 1;
} else {
xfer->frlengths[xfer->aframes] -= len;
}
}
/* Check for transfer error */
- if (td->error) {
+ if (td->error_any) {
/* the transfer is finished */
- error = 1;
+ error = (td->error_stall ?
+ USB_ERR_STALLED : USB_ERR_IOERROR);
td = NULL;
break;
}
@@ -2731,8 +2749,7 @@ musbotg_standard_done_sub(struct usb_xfer *xfer)
xfer->td_transfer_cache = td;
- return (error ?
- USB_ERR_STALLED : USB_ERR_NORMAL_COMPLETION);
+ return (error);
}
static void
diff --git a/sys/dev/usb/controller/musb_otg.h b/sys/dev/usb/controller/musb_otg.h
index 649cab8..b68ea21 100644
--- a/sys/dev/usb/controller/musb_otg.h
+++ b/sys/dev/usb/controller/musb_otg.h
@@ -32,7 +32,8 @@
#ifndef _MUSB2_OTG_H_
#define _MUSB2_OTG_H_
-#define MUSB2_MAX_DEVICES USB_MAX_DEVICES
+#define MUSB2_RST_ADDR 127
+#define MUSB2_MAX_DEVICES MIN(USB_MAX_DEVICES, MUSB2_RST_ADDR)
/* Common registers */
@@ -319,7 +320,8 @@ struct musbotg_td {
uint8_t ep_no;
uint8_t transfer_type;
uint8_t max_packet;
- uint8_t error:1;
+ uint8_t error_any:1;
+ uint8_t error_stall:1;
uint8_t alt_next:1;
uint8_t short_pkt:1;
uint8_t support_multi_buffer:1;
--------------070101070502010609080900--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?51E7A29B.6040701>