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
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;
_______________________________________________
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to "freebsd-usb-unsubscr...@freebsd.org"

Reply via email to