From f3a6a40f62858b9b1b7e5073f52f7bf7b064f5d4 Mon Sep 17 00:00:00 2001 From: Wolfgang Reissnegger Date: Fri, 15 Jul 2016 18:32:53 -0700 Subject: [PATCH 1/6] SAM3/4 Serial: Fix warning when CONFIG_SUPPRESS_UART_CONFIG is set. --- arch/arm/src/sam34/sam_serial.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/src/sam34/sam_serial.c b/arch/arm/src/sam34/sam_serial.c index 46df27954a1..36043e9deb8 100644 --- a/arch/arm/src/sam34/sam_serial.c +++ b/arch/arm/src/sam34/sam_serial.c @@ -692,8 +692,8 @@ static void up_disableallints(struct up_dev_s *priv, uint32_t *imr) static int up_setup(struct uart_dev_s *dev) { - struct up_dev_s *priv = (struct up_dev_s *)dev->priv; #ifndef CONFIG_SUPPRESS_UART_CONFIG + struct up_dev_s *priv = (struct up_dev_s *)dev->priv; uint32_t regval; uint32_t imr; From 81f7758848722cf5d4b0bba1f4b0c60179f87e14 Mon Sep 17 00:00:00 2001 From: Wolfgang Reissnegger Date: Sat, 23 Jul 2016 20:10:20 -0700 Subject: [PATCH 2/6] USBMSC: Fix usbtrace() string. --- drivers/usbdev/usbdev_strings.c | 2 +- drivers/usbdev/usbmsc_scsi.c | 2 +- include/nuttx/usb/usbdev_trace.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/usbdev/usbdev_strings.c b/drivers/usbdev/usbdev_strings.c index 9dc9afdc407..d8c35189f0a 100644 --- a/drivers/usbdev/usbdev_strings.c +++ b/drivers/usbdev/usbdev_strings.c @@ -177,7 +177,7 @@ const struct trace_msg_t g_usb_trace_strings_clserror[] = TRACE_STR(USBMSC_TRACEERR_CMDREADREADFAIL), TRACE_STR(USBMSC_TRACEERR_CMDREADSUBMIT), TRACE_STR(USBMSC_TRACEERR_CMDREADWRRQEMPTY), - TRACE_STR(USBMSC_TRACEERR_CMDSTATUSRDREQLISTEMPTY), + TRACE_STR(USBMSC_TRACEERR_CMDSTATUSWRREQLISTEMPTY), TRACE_STR(USBMSC_TRACEERR_CMDUNEVIOLATION), TRACE_STR(USBMSC_TRACEERR_CMDWRITERDSUBMIT), TRACE_STR(USBMSC_TRACEERR_CMDWRITERDRQEMPTY), diff --git a/drivers/usbdev/usbmsc_scsi.c b/drivers/usbdev/usbmsc_scsi.c index fd9e55cb6b4..33d7eb7629f 100644 --- a/drivers/usbdev/usbmsc_scsi.c +++ b/drivers/usbdev/usbmsc_scsi.c @@ -2537,7 +2537,7 @@ static int usbmsc_cmdstatusstate(FAR struct usbmsc_dev_s *priv) if (!privreq) { - usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_CMDSTATUSRDREQLISTEMPTY), 0); + usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_CMDSTATUSWRREQLISTEMPTY), 0); return -ENOMEM; } diff --git a/include/nuttx/usb/usbdev_trace.h b/include/nuttx/usb/usbdev_trace.h index 4ef4e170e11..55915caee83 100644 --- a/include/nuttx/usb/usbdev_trace.h +++ b/include/nuttx/usb/usbdev_trace.h @@ -295,7 +295,7 @@ #define USBMSC_TRACEERR_CMDREADREADFAIL 0x0094 #define USBMSC_TRACEERR_CMDREADSUBMIT 0x0095 #define USBMSC_TRACEERR_CMDREADWRRQEMPTY 0x0096 -#define USBMSC_TRACEERR_CMDSTATUSRDREQLISTEMPTY 0x0097 +#define USBMSC_TRACEERR_CMDSTATUSWRREQLISTEMPTY 0x0097 #define USBMSC_TRACEERR_CMDUNEVIOLATION 0x0098 #define USBMSC_TRACEERR_CMDWRITERDSUBMIT 0x0099 #define USBMSC_TRACEERR_CMDWRITERDRQEMPTY 0x009a From 2974d2b07edce4e8f2606de219ee62ba9d1e298d Mon Sep 17 00:00:00 2001 From: Wolfgang Reissnegger Date: Sat, 23 Jul 2016 20:10:41 -0700 Subject: [PATCH 3/6] USBMSC: Add locks when removing request from queue. --- drivers/usbdev/usbmsc_scsi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usbdev/usbmsc_scsi.c b/drivers/usbdev/usbmsc_scsi.c index 33d7eb7629f..b478eb55621 100644 --- a/drivers/usbdev/usbmsc_scsi.c +++ b/drivers/usbdev/usbmsc_scsi.c @@ -2263,7 +2263,9 @@ static int usbmsc_cmdwritestate(FAR struct usbmsc_dev_s *priv) * data to be written. */ + irqstate_t flags = enter_critical_section(); privreq = (FAR struct usbmsc_req_s *)sq_remfirst(&priv->rdreqlist); + leave_critical_section(flags); /* If there no request data available, then just return an error. * This will cause us to remain in the CMDWRITE state. When a filled request is From 4f83d58b24b6993fc3c4bd70648c25ba5e4d5d12 Mon Sep 17 00:00:00 2001 From: Wolfgang Reissnegger Date: Sat, 23 Jul 2016 20:11:00 -0700 Subject: [PATCH 4/6] USBMSC: Fix reversed logic on waiting for SCSI thread start. The scsi thread was waiting for the wrong condition. However, this was masked by the fact that the code creating the scsi thread was also holding usbmsc_scsi_lock(priv) while initializing data, hence this lock synchronized the scsi thread start with init completion. --- drivers/usbdev/usbmsc_scsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usbdev/usbmsc_scsi.c b/drivers/usbdev/usbmsc_scsi.c index b478eb55621..d088312ca94 100644 --- a/drivers/usbdev/usbmsc_scsi.c +++ b/drivers/usbdev/usbmsc_scsi.c @@ -2641,8 +2641,8 @@ int usbmsc_scsi_main(int argc, char *argv[]) uinfo("Waiting to be signalled\n"); usbmsc_scsi_lock(priv); priv->thstate = USBMSC_STATE_STARTED; - while ((priv->theventset & USBMSC_EVENT_READY) != 0 && - (priv->theventset & USBMSC_EVENT_TERMINATEREQUEST) != 0) + while ((priv->theventset & USBMSC_EVENT_READY) == 0 && + (priv->theventset & USBMSC_EVENT_TERMINATEREQUEST) == 0) { usbmsc_scsi_wait(priv); } From cc191a977dc579e256787ab10d99562b431ab81e Mon Sep 17 00:00:00 2001 From: Wolfgang Reissnegger Date: Sat, 23 Jul 2016 16:17:20 -0700 Subject: [PATCH 5/6] SAM3/4 UDP: Remove redundant EP state assignment. --- arch/arm/src/sam34/sam_udp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/arm/src/sam34/sam_udp.c b/arch/arm/src/sam34/sam_udp.c index 6fe76f4f57b..b8fc4f2611a 100644 --- a/arch/arm/src/sam34/sam_udp.c +++ b/arch/arm/src/sam34/sam_udp.c @@ -1873,7 +1873,6 @@ static void sam_ep_bankinterrupt(struct sam_usbdev_s *priv, * transferred from the FIFO. */ - privep->epstate = UDP_EPSTATE_IDLE; (void)sam_req_read(priv, privep, pktsize, bank); } From c0fa319f2b729f313347d80138852ef683861264 Mon Sep 17 00:00:00 2001 From: Wolfgang Reissnegger Date: Sat, 23 Jul 2016 16:19:05 -0700 Subject: [PATCH 6/6] SAM3/4 UDP: Fix handling of endpoint RX FIFO banks. This fixes a race condition where the HW fills a FIFO bank while the SW is busy, resulting in out of sequence USB packets. --- arch/arm/src/sam34/sam_udp.c | 88 ++++++++++++++++++++++++++++-------- 1 file changed, 68 insertions(+), 20 deletions(-) diff --git a/arch/arm/src/sam34/sam_udp.c b/arch/arm/src/sam34/sam_udp.c index b8fc4f2611a..ed2016ea518 100644 --- a/arch/arm/src/sam34/sam_udp.c +++ b/arch/arm/src/sam34/sam_udp.c @@ -305,6 +305,7 @@ struct sam_ep_s uint8_t zlpneeded:1; /* Zero length packet needed at end of transfer */ uint8_t zlpsent:1; /* Zero length packet has been sent */ uint8_t txbusy:1; /* Write request queue is busy (recursion avoidance kludge) */ + uint8_t lastbank:1; /* Last bank we read data from */ }; struct sam_usbdev_s @@ -1188,9 +1189,14 @@ static int sam_req_read(struct sam_usbdev_s *priv, struct sam_ep_s *privep, /* We get here when an RXDATABK0/1 interrupt occurs. That interrupt * cannot be cleared until all of the data has been taken from the RX - * FIFO. But we can + * FIFO. + * + * Also, we need to remember which bank we read last so the interrupt handler + * can determine the correct bank read sequence for future reads. */ + privep->lastbank = bank; + sam_csr_clrbits(epno, bank ? UDPEP_CSR_RXDATABK1 : UDPEP_CSR_RXDATABK0); /* Complete the transfer immediately and give the data to the class @@ -2019,34 +2025,75 @@ static void sam_ep_interrupt(struct sam_usbdev_s *priv, int epno) } } - /* OUT packet received in data bank 0 */ - if ((csr & UDPEP_CSR_RXDATABK0) != 0) + /* OUT packet received. + * + * OUT packets are received in two banks. The hardware does not provide + * information about which bank has been filled last. Therefore we need to + * keep track about which bank we read last to figure out which bank(s) we + * need to read next. + * + * When we get here either none, one or both banks can be filled with data. + * Depending on which bank we read last and which bank(s) contain data we + * need to correctly sequence the FIFO reads: + * + * case lastbank bk0 bk1 read sequence + * 1. 0 0 0 No data to read + * 2. 0 1 0 Only read bank 0 + * 3. 0 0 1 Only read bank 1 + * 4. 0 1 1 Read bank 1, then read bank 0 + * + * 5. 1 0 0 No data to read + * 6. 1 1 0 Only read bank 0 + * 7. 1 0 1 Only read bank 1 (should not happen) + * 8. 1 1 1 Read bank 0, then read bank 1 + * + * lastbank will be updated in sam_req_read() after the FIFO has been read + * and clear RXDATABKx. + */ + + bool bk0 = (csr & UDPEP_CSR_RXDATABK0) != 0; + bool bk1 = (csr & UDPEP_CSR_RXDATABK1) != 0; + + /* 2. and 6. - Only read bank 0 */ + if (bk0 && !bk1) { usbtrace(TRACE_INTDECODE(SAM_TRACEINTID_RXDATABK0), (uint16_t)csr); - - /* Handle data received on Bank 0. sam_ep_bankinterrupt will - * clear the RXDATABK0 interrupt once that data has been - * transferred from the FIFO. - */ - sam_ep_bankinterrupt(priv, privep, csr, 0); } - - /* OUT packet received in data bank 1 */ - - else if ((csr & UDPEP_CSR_RXDATABK1) != 0) + /* 3. and 7. - Only read bank 1*/ + else if (!bk0 && bk1) { +#ifdef CONFIG_DEBUG + if (privep->lastbank == 1) + { + ulldbg("Unexpected USB RX case.\n"); + } +#endif usbtrace(TRACE_INTDECODE(SAM_TRACEINTID_RXDATABK1), (uint16_t)csr); - DEBUGASSERT(SAM_UDP_NBANKS(epno) > 1); - - /* Handle data received on Bank 1. sam_ep_bankinterrupt will - * clear the RXDATABK1 interrupt once that data has been - * transferred from the FIFO. - */ - sam_ep_bankinterrupt(priv, privep, csr, 1); } + else if (bk0 && bk1) + { + /* 4. - Read bank 1, then read bank 0 */ + if (privep->lastbank == 0) + { + usbtrace(TRACE_INTDECODE(SAM_TRACEINTID_RXDATABK1), (uint16_t)csr); + sam_ep_bankinterrupt(priv, privep, csr, 1); + + usbtrace(TRACE_INTDECODE(SAM_TRACEINTID_RXDATABK0), (uint16_t)csr); + sam_ep_bankinterrupt(priv, privep, csr, 0); + } + /* 8. - Read bank 0, then read bank 1 */ + else + { + usbtrace(TRACE_INTDECODE(SAM_TRACEINTID_RXDATABK0), (uint16_t)csr); + sam_ep_bankinterrupt(priv, privep, csr, 0); + + usbtrace(TRACE_INTDECODE(SAM_TRACEINTID_RXDATABK1), (uint16_t)csr); + sam_ep_bankinterrupt(priv, privep, csr, 1); + } + } /* STALL sent */ @@ -2509,6 +2556,7 @@ static void sam_ep_reset(struct sam_usbdev_s *priv, uint8_t epno) privep->zlpneeded = false; privep->zlpsent = false; privep->txbusy = false; + privep->lastbank = 1; } /****************************************************************************