Merge branch 'bugfix/usb_serial_jtag_simplify_v5.1' into 'release/v5.1'

usb-serial-jtag driver simplification (backport v5.1)

See merge request espressif/esp-idf!33909

Author: Jiang Jiang Jian

components/driver/usb_serial_jtag/usb_serial_jtag.c

@@ -15,122 +15,107 @@
 #include "esp_intr_alloc.h"
 #include "driver/usb_serial_jtag.h"
 #include "soc/periph_defs.h"
+#include "soc/soc_caps.h"
 #include "esp_check.h"
+#include "esp_private/periph_ctrl.h"
 
-typedef enum {
-    FIFO_IDLE = 0,  /*!< Indicates the fifo is in idle state */
-    FIFO_BUSY = 1,  /*!< Indicates the fifo is in busy state */
-} fifo_status_t;
+/*
+ Note: Before you add a workaround for an issue in this driver, please please try
+ to figure out the actual root cause first. The USB-serial-JTAG is a simple device,
+ it shouldn't need anything more than a simple, straightforward driver.
+*/
 
-// The hardware buffer max size is 64
+// The hardware buffer max size is 64, both for RX and TX.
 #define USB_SER_JTAG_ENDP_SIZE          (64)
-#define USB_SER_JTAG_RX_MAX_SIZE        (64)
+#define USB_SER_JTAG_RX_MAX_SIZE        (USB_SER_JTAG_ENDP_SIZE)
 
 typedef struct{
     intr_handle_t intr_handle;          /*!< USB-SERIAL-JTAG interrupt handler */
-    portMUX_TYPE spinlock;              /*!< Spinlock for usb_serial_jtag */
-    _Atomic fifo_status_t fifo_status;  /*!< Record the status of fifo */
 
     // RX parameters
     RingbufHandle_t rx_ring_buf;        /*!< RX ring buffer handler */
-    uint32_t rx_buf_size;               /*!< TX buffer size */
-    uint8_t rx_data_buf[USB_SER_JTAG_ENDP_SIZE];            /*!< Data buffer to stash FIFO data */
 
     // TX parameters
-    uint32_t tx_buf_size;               /*!< TX buffer size */
     RingbufHandle_t tx_ring_buf;        /*!< TX ring buffer handler */
-    uint8_t tx_data_buf[USB_SER_JTAG_ENDP_SIZE];  /*!< Data buffer to stash TX FIFO data */
+    uint8_t tx_stash_buf[USB_SER_JTAG_ENDP_SIZE];  /*!< Data buffer to stash TX FIFO data */
     size_t tx_stash_cnt;                          /*!< Number of stashed TX FIFO bytes */
 } usb_serial_jtag_obj_t;
 
 static usb_serial_jtag_obj_t *p_usb_serial_jtag_obj = NULL;
 
 static const char* USB_SERIAL_JTAG_TAG = "usb_serial_jtag";
 
-static size_t usb_serial_jtag_write_and_flush(const uint8_t *buf, uint32_t wr_len)
+static void usb_serial_jtag_isr_handler_default(void *arg)
 {
-    size_t size = usb_serial_jtag_ll_write_txfifo(buf, wr_len);
-    usb_serial_jtag_ll_txfifo_flush();
-    return size;
-}
-
-static void usb_serial_jtag_isr_handler_default(void *arg) {
-    portBASE_TYPE xTaskWoken = 0;
+    BaseType_t xTaskWoken = 0;
     uint32_t usbjtag_intr_status = 0;
     usbjtag_intr_status = usb_serial_jtag_ll_get_intsts_mask();
 
     if (usbjtag_intr_status & USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY) {
+        //Clear interrupt so we won't be called until the next transfer finishes.
+        usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
+
         // Interrupt tells us the host picked up the data we sent.
         // If we have more data, we can put it in the buffer and the host will pick that up next.
-        // Send data in isr.
-        // If the hardware fifo is available, write in it. Otherwise, do nothing.
+        // We expect the TX FIFO to be writable for this. If it's not, somehow someone else
+        // (ROM print routines?) have snuck in a full buffer before we got here. In that case,
+        // we simply ignore the interrupt, a new one will come if the buffer is empty again.
         if (usb_serial_jtag_ll_txfifo_writable() == 1) {
-            // We disable the interrupt here so that the interrupt won't be triggered if there is no data to send.
-
+            // Retrieve data from either the stash buffer or, if that's empty, from the ring buffer.
             size_t queued_size;
-            uint8_t *queued_buff = NULL;
-            bool is_stashed_data = false;
+            uint8_t *queued_buf = NULL;
             if (p_usb_serial_jtag_obj->tx_stash_cnt != 0) {
                 // Send stashed tx bytes before reading bytes from ring buffer
-                queued_buff = p_usb_serial_jtag_obj->tx_data_buf;
+                queued_buf = p_usb_serial_jtag_obj->tx_stash_buf;
                 queued_size = p_usb_serial_jtag_obj->tx_stash_cnt;
-                is_stashed_data = true;
             } else {
                 // Max 64 data payload size in a single EndPoint
-                queued_buff = (uint8_t *)xRingbufferReceiveUpToFromISR(p_usb_serial_jtag_obj->tx_ring_buf, &queued_size, USB_SER_JTAG_ENDP_SIZE);
+                queued_buf = (uint8_t *)xRingbufferReceiveUpToFromISR(p_usb_serial_jtag_obj->tx_ring_buf, &queued_size, USB_SER_JTAG_ENDP_SIZE);
             }
 
-            usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
-
-            if (queued_buff != NULL) {
-
-                // Although tx_queued_bytes may be larger than 0, we may have
-                // interrupted before xRingbufferSend() was called.
-                // Copy the queued buffer into the TX FIFO
-
-                // On ringbuffer wrap-around the size can be 0 even though the buffer returned is not NULL
-                if (queued_size > 0) {
-                    portENTER_CRITICAL_ISR(&p_usb_serial_jtag_obj->spinlock);
-                    atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_BUSY);
-                    uint32_t sent_size = usb_serial_jtag_write_and_flush(queued_buff, queued_size);
-                    portEXIT_CRITICAL_ISR(&p_usb_serial_jtag_obj->spinlock);
-
-                    if (sent_size < queued_size) {
-                        // Not all bytes could be sent at once; stash the unwritten bytes in a tx buffer
-                        // stash_size will not larger than USB_SER_JTAG_ENDP_SIZE because queued_size is got from xRingbufferReceiveUpToFromISR
-                        size_t stash_size = queued_size - sent_size;
-                        memcpy(p_usb_serial_jtag_obj->tx_data_buf, &queued_buff[sent_size], stash_size);
-                        p_usb_serial_jtag_obj->tx_stash_cnt = stash_size;
-                    } else {
-                        p_usb_serial_jtag_obj->tx_stash_cnt = 0;
-                        // assert if sent_size is larger than queued_size.
-                        assert(sent_size <= queued_size);
-                    }
+            if (queued_buf != NULL && queued_size > 0) {
+                // We have some data to send. Send it.
+                uint32_t sent_size = usb_serial_jtag_ll_write_txfifo(queued_buf, queued_size);
+                usb_serial_jtag_ll_txfifo_flush();
+
+                // Check if we were able to send everything.
+                if (sent_size < queued_size) {
+                    // Not all bytes could be sent at once; stash the unwritten bytes in a buffer
+                    // This will happen if e.g. the rom output functions manage to sneak a few bytes into the
+                    // TX FIFO before this interrupt triggers. Note stash_size will not larger than
+                    // USB_SER_JTAG_ENDP_SIZE because queued_size is obtained from xRingbufferReceiveUpToFromISR.
+                    size_t stash_size = queued_size - sent_size;
+                    memcpy(p_usb_serial_jtag_obj->tx_stash_buf, &queued_buf[sent_size], stash_size);
+                    p_usb_serial_jtag_obj->tx_stash_cnt = stash_size;
+                } else {
+                    p_usb_serial_jtag_obj->tx_stash_cnt = 0;
                 }
-                if (is_stashed_data == false) {
-                    vRingbufferReturnItemFromISR(p_usb_serial_jtag_obj->tx_ring_buf, queued_buff, &xTaskWoken);
+                // Return the buffer if we got it from the ring buffer.
+                if (queued_buf != p_usb_serial_jtag_obj->tx_stash_buf) {
+                    vRingbufferReturnItemFromISR(p_usb_serial_jtag_obj->tx_ring_buf, queued_buf, &xTaskWoken);
                 }
             } else {
+                // No data to send.
                 // The last transmit may have sent a full EP worth of data. The host will interpret
                 // this as a transaction that hasn't finished yet and keep the data in its internal
                 // buffers rather than releasing it to the program listening on the CDC serial port.
                 // We need to flush again in order to send a 0-byte packet that ends the transaction.
                 usb_serial_jtag_ll_txfifo_flush();
-                // Note that since this doesn't re-enable USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY, the
-                // flush will not by itself cause this ISR to be called again.
+
+                // We will also disable the interrupt as for now there's no need to handle the
+                // TX interrupt again. We'll re-enable this externally if we need data sent.
+                usb_serial_jtag_ll_disable_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
             }
-        } else {
-            atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_IDLE);
-            usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
         }
     }
 
     if (usbjtag_intr_status & USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT) {
-        // read rx buffer(max length is 64), and send avaliable data to ringbuffer.
-        // Ensure the rx buffer size is larger than RX_MAX_SIZE.
+        // Acknowledge interrupt
         usb_serial_jtag_ll_clr_intsts_mask(USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT);
-        uint32_t rx_fifo_len = usb_serial_jtag_ll_read_rxfifo(p_usb_serial_jtag_obj->rx_data_buf, USB_SER_JTAG_RX_MAX_SIZE);
-        xRingbufferSendFromISR(p_usb_serial_jtag_obj->rx_ring_buf, p_usb_serial_jtag_obj->rx_data_buf, rx_fifo_len, &xTaskWoken);
+        // Read RX FIFO and send available data to ringbuffer.
+        uint8_t buf[USB_SER_JTAG_RX_MAX_SIZE];
+        uint32_t rx_fifo_len = usb_serial_jtag_ll_read_rxfifo(buf, USB_SER_JTAG_RX_MAX_SIZE);
+        xRingbufferSendFromISR(p_usb_serial_jtag_obj->rx_ring_buf, buf, rx_fifo_len, &xTaskWoken);
     }
 
     if (xTaskWoken == pdTRUE) {
@@ -145,18 +130,16 @@ esp_err_t usb_serial_jtag_driver_install(usb_serial_jtag_driver_config_t *usb_se
     ESP_RETURN_ON_FALSE((usb_serial_jtag_config->rx_buffer_size > 0), ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "RX buffer is not prepared");
     ESP_RETURN_ON_FALSE((usb_serial_jtag_config->rx_buffer_size > USB_SER_JTAG_RX_MAX_SIZE), ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "RX buffer prepared is so small, should larger than 64");
     ESP_RETURN_ON_FALSE((usb_serial_jtag_config->tx_buffer_size > 0), ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "TX buffer is not prepared");
-    p_usb_serial_jtag_obj = (usb_serial_jtag_obj_t*) heap_caps_calloc(1, sizeof(usb_serial_jtag_obj_t), MALLOC_CAP_INTERNAL|MALLOC_CAP_8BIT);
-    p_usb_serial_jtag_obj->rx_buf_size = usb_serial_jtag_config->rx_buffer_size;
-    p_usb_serial_jtag_obj->tx_buf_size = usb_serial_jtag_config->tx_buffer_size;
-    p_usb_serial_jtag_obj->tx_stash_cnt = 0;
-    p_usb_serial_jtag_obj->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED;
+    p_usb_serial_jtag_obj = (usb_serial_jtag_obj_t*) heap_caps_calloc(1, sizeof(usb_serial_jtag_obj_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
     if (p_usb_serial_jtag_obj == NULL) {
         ESP_LOGE(USB_SERIAL_JTAG_TAG, "memory allocate error");
-        err = ESP_ERR_NO_MEM;
-        goto _exit;
+        // no `goto _exit` here as there's nothing to clean up and that would make the uninstall
+        // routine unhappy.
+        return ESP_ERR_NO_MEM;
     }
+    p_usb_serial_jtag_obj->tx_stash_cnt = 0;
 
-    p_usb_serial_jtag_obj->rx_ring_buf = xRingbufferCreate(p_usb_serial_jtag_obj->rx_buf_size, RINGBUF_TYPE_BYTEBUF);
+    p_usb_serial_jtag_obj->rx_ring_buf = xRingbufferCreate(usb_serial_jtag_config->rx_buffer_size, RINGBUF_TYPE_BYTEBUF);
     if (p_usb_serial_jtag_obj->rx_ring_buf == NULL) {
         ESP_LOGE(USB_SERIAL_JTAG_TAG, "ringbuffer create error");
         err = ESP_ERR_NO_MEM;
@@ -172,7 +155,6 @@ esp_err_t usb_serial_jtag_driver_install(usb_serial_jtag_driver_config_t *usb_se
 
     // Enable USB-Serial-JTAG peripheral module clock
     usb_serial_jtag_ll_enable_bus_clock(true);
-    atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_IDLE);
 
     // Configure PHY
 #if USB_SERIAL_JTAG_LL_EXT_PHY_SUPPORTED
@@ -231,37 +213,29 @@ int usb_serial_jtag_write_bytes(const void* src, size_t size, TickType_t ticks_t
     ESP_RETURN_ON_FALSE(src != NULL, ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "Invalid buffer pointer.");
     ESP_RETURN_ON_FALSE(p_usb_serial_jtag_obj != NULL, ESP_ERR_INVALID_ARG, USB_SERIAL_JTAG_TAG, "The driver hasn't been initialized");
 
-    size_t sent_data = 0;
     BaseType_t result = pdTRUE;
-    const uint8_t *buff = (const uint8_t *)src;
-    if (p_usb_serial_jtag_obj->fifo_status == FIFO_IDLE) {
-        portENTER_CRITICAL(&p_usb_serial_jtag_obj->spinlock);
-        atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_BUSY);
-        sent_data = usb_serial_jtag_write_and_flush(src, size);
-        portEXIT_CRITICAL(&p_usb_serial_jtag_obj->spinlock);
-    }
-
-    // Blocking method, Sending data to ringbuffer, and handle the data in ISR.
-    if (size - sent_data > 0) {
-        result = xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*) (buff+sent_data), size-sent_data, ticks_to_wait);
-    } else {
-        atomic_store(&p_usb_serial_jtag_obj->fifo_status, FIFO_IDLE);
-    }
+    result = xRingbufferSend(p_usb_serial_jtag_obj->tx_ring_buf, (void*)src, size, ticks_to_wait);
+    // Re-enable the TX interrupt. If this was disabled, this will immediately trigger the ISR
+    // and send the things we just put in the ringbuffer.
     usb_serial_jtag_ll_ena_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY);
     return (result == pdFALSE) ? 0 : size;
 }
 
+//Note that this is also called when usb_serial_jtag_driver_install errors out and as such should
+//work on a half-initialized driver as well.
 esp_err_t usb_serial_jtag_driver_uninstall(void)
 {
-    if(p_usb_serial_jtag_obj == NULL) {
-        ESP_LOGI(USB_SERIAL_JTAG_TAG, "ALREADY NULL");
+    if (p_usb_serial_jtag_obj == NULL) {
+        ESP_LOGE(USB_SERIAL_JTAG_TAG, "uninstall without install called");
         return ESP_OK;
     }
 
-    /* Not disable the module clock and usb_pad_enable here since the USJ stdout might still depends on it. */
-    //Disable tx/rx interrupt.
+    /* Don't disable the module clock and usb_pad_enable here since the USJ stdout might still depends on it. */
+
     usb_serial_jtag_ll_disable_intr_mask(USB_SERIAL_JTAG_INTR_SERIAL_IN_EMPTY | USB_SERIAL_JTAG_INTR_SERIAL_OUT_RECV_PKT);
-    esp_intr_free(p_usb_serial_jtag_obj->intr_handle);
+    if (p_usb_serial_jtag_obj->intr_handle) {
+        esp_intr_free(p_usb_serial_jtag_obj->intr_handle);
+    }
 
     if(p_usb_serial_jtag_obj->rx_ring_buf) {
         vRingbufferDelete(p_usb_serial_jtag_obj->rx_ring_buf);

components/soc/esp32c6/include/soc/usb_serial_jtag_struct.h

@@ -1,5 +1,5 @@
 /**
- * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
  *
  *  SPDX-License-Identifier: Apache-2.0
  */
@@ -23,8 +23,7 @@ typedef union {
          *  can check USB_SERIAL_JTAG_OUT_EP1_WR_ADDR USB_SERIAL_JTAG_OUT_EP0_RD_ADDR to know
          *  how many data is received, then read data from UART Rx FIFO.
          */
-        uint32_t rdwr_byte:8;
-        uint32_t reserved_8:24;
+        uint32_t rdwr_byte:32;
     };
     uint32_t val;
 } usb_serial_jtag_ep1_reg_t;
@@ -131,7 +130,7 @@ typedef union {
          */
         uint32_t test_enable:1;
         /** test_usb_oe : R/W; bitpos: [1]; default: 0;
-         *  USB pad oen in test
+         *  USB pad output enable in test
          */
         uint32_t test_usb_oe:1;
         /** test_tx_dp : R/W; bitpos: [2]; default: 0;
@@ -290,7 +289,7 @@ typedef union {
          */
         uint32_t serial_out_afifo_reset_rd:1;
         /** serial_out_afifo_rempty : RO; bitpos: [4]; default: 1;
-         *  CDC_ACM OUTOUT async FIFO empty signal in read clock domain.
+         *  CDC_ACM OUTPUT async FIFO empty signal in read clock domain.
          */
         uint32_t serial_out_afifo_rempty:1;
         /** serial_in_afifo_wfull : RO; bitpos: [5]; default: 0;

components/soc/esp32h2/include/soc/usb_serial_jtag_struct.h

@@ -1,5 +1,5 @@
 /**
- * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
  *
  *  SPDX-License-Identifier: Apache-2.0
  */
@@ -23,8 +23,7 @@ typedef union {
          *  can check USB_SERIAL_JTAG_OUT_EP1_WR_ADDR USB_SERIAL_JTAG_OUT_EP0_RD_ADDR to know
          *  how many data is received, then read data from UART Rx FIFO.
          */
-        uint32_t rdwr_byte:8;
-        uint32_t reserved_8:24;
+        uint32_t rdwr_byte:32;
     };
     uint32_t val;
 } usb_serial_jtag_ep1_reg_t;
@@ -131,7 +130,7 @@ typedef union {
          */
         uint32_t test_enable:1;
         /** test_usb_oe : R/W; bitpos: [1]; default: 0;
-         *  USB pad oen in test
+         *  USB pad output enable in test
          */
         uint32_t test_usb_oe:1;
         /** test_tx_dp : R/W; bitpos: [2]; default: 0;
@@ -290,7 +289,7 @@ typedef union {
          */
         uint32_t serial_out_afifo_reset_rd:1;
         /** serial_out_afifo_rempty : RO; bitpos: [4]; default: 1;
-         *  CDC_ACM OUTOUT async FIFO empty signal in read clock domain.
+         *  CDC_ACM OUTPUT async FIFO empty signal in read clock domain.
          */
         uint32_t serial_out_afifo_rempty:1;
         /** serial_in_afifo_wfull : RO; bitpos: [5]; default: 0;

components/vfs/vfs_usb_serial_jtag.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -120,6 +120,11 @@ static void usb_serial_jtag_tx_char(int fd, int c)
     do {
         if (usb_serial_jtag_ll_txfifo_writable()) {
             usb_serial_jtag_ll_write_txfifo(&cc, 1);
+            if (c == '\n') {
+                //Make sure line doesn't linger in fifo
+                usb_serial_jtag_ll_txfifo_flush();
+            }
+            //update time of last successful tx to now.
             s_ctx.last_tx_ts = esp_timer_get_time();
             break;
         }
@@ -154,10 +159,6 @@ static ssize_t usb_serial_jtag_write(int fd, const void * data, size_t size)
             }
         }
         s_ctx.tx_func(fd, c);
-        if (c == '\n') {
-            //Make sure line doesn't linger in fifo
-            usb_serial_jtag_ll_txfifo_flush();
-        }
     }
     _lock_release_recursive(&s_ctx.write_lock);
     return size;