refactor(usb/hcd): Allow port resets with allocated pipes

This commit updates the HCD API to allow port resets to occur even if pipes
are allocated. The pipes cannot be active and the port reset will simply
restore the pipes (by reinitializing their channel registers) following the
reset.

Changes:

- Allow port resets while channels are allocated
- Remove pipe persistance API 'hcd_pipe_set_persist_reset()'

Author: Dazza0

components/hal/include/hal/usb_dwc_hal.h

@@ -205,7 +205,7 @@ typedef struct {
  * - The peripheral must have been reset and clock un-gated
  * - The USB PHY (internal or external) and associated GPIOs must already be configured
  * - GPIO pins configured
- * - Interrupt allocated but DISABLED (in case of an unknown interupt state)
+ * - Interrupt allocated but DISABLED (in case of an unknown interrupt state)
  * Exit:
  * - Checks to see if DWC_OTG is alive, and if HW version/config is correct
  * - HAl context initialized
@@ -290,7 +290,7 @@ static inline void usb_dwc_hal_port_init(usb_dwc_hal_context_t *hal)
 /**
  * @brief Deinitialize the host port
  *
- * - Will disable the host port's interrupts preventing further port aand channel events from ocurring
+ * - Will disable the host port's interrupts preventing further port aand channel events from occurring
  *
  * @param hal Context of the HAL layer
  */
@@ -333,7 +333,6 @@ static inline void usb_dwc_hal_port_toggle_power(usb_dwc_hal_context_t *hal, boo
  */
 static inline void usb_dwc_hal_port_toggle_reset(usb_dwc_hal_context_t *hal, bool enable)
 {
-    HAL_ASSERT(hal->channels.num_allocd == 0);  //Cannot reset if there are still allocated channels
     usb_dwc_ll_hprt_set_port_reset(hal->dev, enable);
 }
 
@@ -447,7 +446,7 @@ static inline void usb_dwc_hal_port_periodic_enable(usb_dwc_hal_context_t *hal)
 /**
  * @brief Disable periodic scheduling
  *
- * Disabling periodic scheduling will save a bit of DMA bandwith (as the controller will no longer fetch the schedule
+ * Disabling periodic scheduling will save a bit of DMA bandwidth (as the controller will no longer fetch the schedule
  * from the frame list).
  *
  * @note Before disabling periodic scheduling, it is the user's responsibility to ensure that all periodic channels have
@@ -505,17 +504,17 @@ static inline usb_dwc_speed_t usb_dwc_hal_port_get_conn_speed(usb_dwc_hal_contex
  * @brief Disable the debounce lock
  *
  * This function must be called after calling usb_dwc_hal_port_check_if_connected() and will allow connection/disconnection
- * events to occur again. Any pending connection or disconenction interrupts are cleared.
+ * events to occur again. Any pending connection or disconnection interrupts are cleared.
  *
  * @param hal Context of the HAL layer
  */
 static inline void usb_dwc_hal_disable_debounce_lock(usb_dwc_hal_context_t *hal)
 {
     hal->flags.dbnc_lock_enabled = 0;
-    //Clear Conenction and disconenction interrupt in case it triggered again
+    //Clear Connection and disconnection interrupt in case it triggered again
     usb_dwc_ll_gintsts_clear_intrs(hal->dev, USB_DWC_LL_INTR_CORE_DISCONNINT);
     usb_dwc_ll_hprt_intr_clear(hal->dev, USB_DWC_LL_INTR_HPRT_PRTCONNDET);
-    //Reenable the hprt (connection) and disconnection interrupts
+    //Re-enable the hprt (connection) and disconnection interrupts
     usb_dwc_ll_gintmsk_en_intrs(hal->dev, USB_DWC_LL_INTR_CORE_PRTINT | USB_DWC_LL_INTR_CORE_DISCONNINT);
 }
 
@@ -672,10 +671,10 @@ bool usb_dwc_hal_chan_request_halt(usb_dwc_hal_chan_t *chan_obj);
 /**
  * @brief Indicate that a channel is halted after a port error
  *
- * When a port error occurs (e.g., discconect, overcurrent):
+ * When a port error occurs (e.g., disconnect, overcurrent):
  * - Any previously active channels will remain active (i.e., they will not receive a channel interrupt)
  * - Attempting to disable them using usb_dwc_hal_chan_request_halt() will NOT generate an interrupt for ISOC channels
- *   (probalby something to do with the periodic scheduling)
+ *   (probably something to do with the periodic scheduling)
  *
  * However, the channel's enable bit can be left as 1 since after a port error, a soft reset will be done anyways.
  * This function simply updates the channels internal state variable to indicate it is halted (thus allowing it to be

components/usb/hcd_dwc.c

@@ -214,9 +214,7 @@ struct pipe_obj {
             uint32_t waiting_halt: 1;
             uint32_t pipe_cmd_processing: 1;
             uint32_t has_urb: 1;            // Indicates there is at least one URB either pending, in-flight, or done
-            uint32_t persist: 1;            // indicates that this pipe should persist through a run-time port reset
-            uint32_t reset_lock: 1;         // Indicates that this pipe is undergoing a run-time reset
-            uint32_t reserved27: 27;
+            uint32_t reserved29: 29;
         };
         uint32_t val;
     } cs_flags;
@@ -560,28 +558,6 @@ static esp_err_t _pipe_cmd_clear(pipe_t *pipe);
 
 // ------------------------ Port ---------------------------
 
-/**
- * @brief Prepare persistent pipes for reset
- *
- * This function checks if all pipes are reset persistent and proceeds to free their underlying HAL channels for the
- * persistent pipes. This should be called before a run time reset
- *
- * @param port Port object
- * @return true All pipes are persistent and their channels are freed
- * @return false Not all pipes are persistent
- */
-static bool _port_persist_all_pipes(port_t *port);
-
-/**
- * @brief Recovers all persistent pipes after a reset
- *
- * This function will recover all persistent pipes after a reset and reallocate their underlying HAl channels. This
- * function should be called after a reset.
- *
- * @param port Port object
- */
-static void _port_recover_all_pipes(port_t *port);
-
 /**
  * @brief Checks if all pipes are in the halted state
  *
@@ -1162,44 +1138,6 @@ esp_err_t hcd_uninstall(void)
 
 // ----------------------- Helpers -------------------------
 
-static bool _port_persist_all_pipes(port_t *port)
-{
-    if (port->num_pipes_queued > 0) {
-        // All pipes must be idle before we run-time reset
-        return false;
-    }
-    bool all_persist = true;
-    pipe_t *pipe;
-    // Check that each pipe is persistent
-    TAILQ_FOREACH(pipe, &port->pipes_idle_tailq, tailq_entry) {
-        if (!pipe->cs_flags.persist) {
-            all_persist = false;
-            break;
-        }
-    }
-    if (!all_persist) {
-        // At least one pipe is not persistent. All pipes must be freed or made persistent before we can reset
-        return false;
-    }
-    TAILQ_FOREACH(pipe, &port->pipes_idle_tailq, tailq_entry) {
-        pipe->cs_flags.reset_lock = 1;
-        usb_dwc_hal_chan_free(port->hal, pipe->chan_obj);
-    }
-    return true;
-}
-
-static void _port_recover_all_pipes(port_t *port)
-{
-    pipe_t *pipe;
-    TAILQ_FOREACH(pipe, &port->pipes_idle_tailq, tailq_entry) {
-        pipe->cs_flags.persist = 0;
-        pipe->cs_flags.reset_lock = 0;
-        usb_dwc_hal_chan_alloc(port->hal, pipe->chan_obj, (void *)pipe);
-        usb_dwc_hal_chan_set_ep_char(port->hal, pipe->chan_obj, &pipe->ep_char);
-    }
-    CACHE_SYNC_FRAME_LIST(port->frame_list);
-}
-
 static bool _port_check_all_pipes_halted(port_t *port)
 {
     bool all_halted = true;
@@ -1276,20 +1214,26 @@ static esp_err_t _port_cmd_power_off(port_t *port)
 static esp_err_t _port_cmd_reset(port_t *port)
 {
     esp_err_t ret;
-    // Port can only a reset when it is in the enabled or disabled states (in case of new connection)
+
+    // Port can only a reset when it is in the enabled or disabled (in the case of a new connection)states.
     if (port->state != HCD_PORT_STATE_ENABLED && port->state != HCD_PORT_STATE_DISABLED) {
         ret = ESP_ERR_INVALID_STATE;
         goto exit;
     }
-    bool is_runtime_reset = (port->state == HCD_PORT_STATE_ENABLED) ? true : false;
-    if (is_runtime_reset && !_port_persist_all_pipes(port)) {
-        // If this is a run time reset, check all pipes that are still allocated can persist the reset
+    // Port can only be reset if all pipes are idle
+    if (port->num_pipes_queued > 0) {
         ret = ESP_ERR_INVALID_STATE;
         goto exit;
     }
-    // All pipes (if any_) are guaranteed to be persistent at this point. Proceed to resetting the bus
+    /*
+    Proceed to resetting the bus
+    - Update the port's state variable
+    - Hold the bus in the reset state for RESET_HOLD_MS.
+    - Return the bus to the idle state for RESET_RECOVERY_MS
+    */
     port->state = HCD_PORT_STATE_RESETTING;
-    // Put and hold the bus in the reset state. If the port was previously enabled, a disabled event will occur after this
+
+    // Place the bus into the reset state. If the port was previously enabled, a disabled event will occur after this
     usb_dwc_hal_port_toggle_reset(port->hal, true);
     HCD_EXIT_CRITICAL();
     vTaskDelay(pdMS_TO_TICKS(RESET_HOLD_MS));
@@ -1299,7 +1243,8 @@ static esp_err_t _port_cmd_reset(port_t *port)
         ret = ESP_ERR_INVALID_RESPONSE;
         goto bailout;
     }
-    // Return the bus to the idle state and hold it for the required reset recovery time. Port enabled event should occur
+
+    // Return the bus to the idle state. Port enabled event should occur
     usb_dwc_hal_port_toggle_reset(port->hal, false);
     HCD_EXIT_CRITICAL();
     vTaskDelay(pdMS_TO_TICKS(RESET_RECOVERY_MS));
@@ -1309,16 +1254,20 @@ static esp_err_t _port_cmd_reset(port_t *port)
         ret = ESP_ERR_INVALID_RESPONSE;
         goto bailout;
     }
-    // Set FIFO sizes based on the selected biasing
-    usb_dwc_hal_set_fifo_bias(port->hal, port->fifo_bias);
-    // We start periodic scheduling only after a RESET command since SOFs only start after a reset
-    usb_dwc_hal_port_set_frame_list(port->hal, port->frame_list, FRAME_LIST_LEN);
-    usb_dwc_hal_port_periodic_enable(port->hal);
+
+    // Reinitialize port registers.
+    usb_dwc_hal_set_fifo_bias(port->hal, port->fifo_bias);  // Set FIFO biases
+    usb_dwc_hal_port_set_frame_list(port->hal, port->frame_list, FRAME_LIST_LEN);   // Set periodic frame list
+    usb_dwc_hal_port_periodic_enable(port->hal);    // Enable periodic scheduling
+
     ret = ESP_OK;
 bailout:
-    if (is_runtime_reset) {
-        _port_recover_all_pipes(port);
+    // Reinitialize channel registers
+    pipe_t *pipe;
+    TAILQ_FOREACH(pipe, &port->pipes_idle_tailq, tailq_entry) {
+        usb_dwc_hal_chan_set_ep_char(port->hal, pipe->chan_obj, &pipe->ep_char);
     }
+    CACHE_SYNC_FRAME_LIST(port->frame_list);
 exit:
     return ret;
 }
@@ -1987,8 +1936,7 @@ esp_err_t hcd_pipe_free(hcd_pipe_handle_t pipe_hdl)
     HCD_ENTER_CRITICAL();
     // Check that all URBs have been removed and pipe has no pending events
     HCD_CHECK_FROM_CRIT(!pipe->multi_buffer_control.buffer_is_executing
-                        && !pipe->cs_flags.has_urb
-                        && !pipe->cs_flags.reset_lock,
+                        && !pipe->cs_flags.has_urb,
                         ESP_ERR_INVALID_STATE);
     // Remove pipe from the list of idle pipes (it must be in the idle list because it should have no queued URBs)
     TAILQ_REMOVE(&pipe->port->pipes_idle_tailq, pipe, tailq_entry);
@@ -2011,8 +1959,7 @@ esp_err_t hcd_pipe_update_mps(hcd_pipe_handle_t pipe_hdl, int mps)
     HCD_ENTER_CRITICAL();
     // Check if pipe is in the correct state to be updated
     HCD_CHECK_FROM_CRIT(!pipe->cs_flags.pipe_cmd_processing &&
-                        !pipe->cs_flags.has_urb &&
-                        !pipe->cs_flags.reset_lock,
+                        !pipe->cs_flags.has_urb,
                         ESP_ERR_INVALID_STATE);
     pipe->ep_char.mps = mps;
     // Update the underlying channel's registers
@@ -2027,8 +1974,7 @@ esp_err_t hcd_pipe_update_dev_addr(hcd_pipe_handle_t pipe_hdl, uint8_t dev_addr)
     HCD_ENTER_CRITICAL();
     // Check if pipe is in the correct state to be updated
     HCD_CHECK_FROM_CRIT(!pipe->cs_flags.pipe_cmd_processing &&
-                        !pipe->cs_flags.has_urb &&
-                        !pipe->cs_flags.reset_lock,
+                        !pipe->cs_flags.has_urb,
                         ESP_ERR_INVALID_STATE);
     pipe->ep_char.dev_addr = dev_addr;
     // Update the underlying channel's registers
@@ -2043,29 +1989,14 @@ esp_err_t hcd_pipe_update_callback(hcd_pipe_handle_t pipe_hdl, hcd_pipe_callback
     HCD_ENTER_CRITICAL();
     // Check if pipe is in the correct state to be updated
     HCD_CHECK_FROM_CRIT(!pipe->cs_flags.pipe_cmd_processing &&
-                        !pipe->cs_flags.has_urb &&
-                        !pipe->cs_flags.reset_lock,
+                        !pipe->cs_flags.has_urb,
                         ESP_ERR_INVALID_STATE);
     pipe->callback = callback;
     pipe->callback_arg = user_arg;
     HCD_EXIT_CRITICAL();
     return ESP_OK;
 }
 
-esp_err_t hcd_pipe_set_persist_reset(hcd_pipe_handle_t pipe_hdl)
-{
-    pipe_t *pipe = (pipe_t *)pipe_hdl;
-    HCD_ENTER_CRITICAL();
-    // Check if pipe is in the correct state to be updated
-    HCD_CHECK_FROM_CRIT(!pipe->cs_flags.pipe_cmd_processing &&
-                        !pipe->cs_flags.has_urb &&
-                        !pipe->cs_flags.reset_lock,
-                        ESP_ERR_INVALID_STATE);
-    pipe->cs_flags.persist = 1;
-    HCD_EXIT_CRITICAL();
-    return ESP_OK;
-}
-
 void *hcd_pipe_get_context(hcd_pipe_handle_t pipe_hdl)
 {
     pipe_t *pipe = (pipe_t *)pipe_hdl;
@@ -2102,27 +2033,22 @@ esp_err_t hcd_pipe_command(hcd_pipe_handle_t pipe_hdl, hcd_pipe_cmd_t command)
     esp_err_t ret = ESP_OK;
 
     HCD_ENTER_CRITICAL();
-    // Cannot execute pipe commands the pipe is already executing a command, or if the pipe or its port are no longer valid
-    if (pipe->cs_flags.reset_lock) {
-        ret = ESP_ERR_INVALID_STATE;
-    } else {
-        pipe->cs_flags.pipe_cmd_processing = 1;
-        switch (command) {
-        case HCD_PIPE_CMD_HALT: {
-            ret = _pipe_cmd_halt(pipe);
-            break;
-        }
-        case HCD_PIPE_CMD_FLUSH: {
-            ret = _pipe_cmd_flush(pipe);
-            break;
-        }
-        case HCD_PIPE_CMD_CLEAR: {
-            ret = _pipe_cmd_clear(pipe);
-            break;
-        }
-        }
-        pipe->cs_flags.pipe_cmd_processing = 0;
+    pipe->cs_flags.pipe_cmd_processing = 1;
+    switch (command) {
+    case HCD_PIPE_CMD_HALT: {
+        ret = _pipe_cmd_halt(pipe);
+        break;
+    }
+    case HCD_PIPE_CMD_FLUSH: {
+        ret = _pipe_cmd_flush(pipe);
+        break;
+    }
+    case HCD_PIPE_CMD_CLEAR: {
+        ret = _pipe_cmd_clear(pipe);
+        break;
+    }
     }
+    pipe->cs_flags.pipe_cmd_processing = 0;
     HCD_EXIT_CRITICAL();
     return ret;
 }
@@ -2658,8 +2584,7 @@ esp_err_t hcd_urb_enqueue(hcd_pipe_handle_t pipe_hdl, urb_t *urb)
     // Check that pipe and port are in the correct state to receive URBs
     HCD_CHECK_FROM_CRIT(pipe->port->state == HCD_PORT_STATE_ENABLED         // The pipe's port must be in the correct state
                         && pipe->state == HCD_PIPE_STATE_ACTIVE             // The pipe must be in the correct state
-                        && !pipe->cs_flags.pipe_cmd_processing              // Pipe cannot currently be processing a pipe command
-                        && !pipe->cs_flags.reset_lock,                      // Pipe cannot be persisting through a port reset
+                        && !pipe->cs_flags.pipe_cmd_processing,             // Pipe cannot currently be processing a pipe command
                         ESP_ERR_INVALID_STATE);
     // Use the URB's reserved_ptr to store the pipe's
     urb->hcd_ptr = (void *)pipe;

components/usb/hub.c

@@ -285,7 +285,6 @@ static bool enum_stage_start(enum_ctrl_t *enum_ctrl)
 
 static bool enum_stage_second_reset(enum_ctrl_t *enum_ctrl)
 {
-    ESP_ERROR_CHECK(hcd_pipe_set_persist_reset(enum_ctrl->pipe));  // Persist the default pipe through the reset
     if (hcd_port_command(p_hub_driver_obj->constant.root_port_hdl, HCD_PORT_CMD_RESET) != ESP_OK) {
         ESP_LOGE(HUB_DRIVER_TAG, "Failed to issue second reset");
         return false;

components/usb/private_include/hcd.h

@@ -434,20 +434,6 @@ esp_err_t hcd_pipe_update_dev_addr(hcd_pipe_handle_t pipe_hdl, uint8_t dev_addr)
  */
 esp_err_t hcd_pipe_update_callback(hcd_pipe_handle_t pipe_hdl, hcd_pipe_callback_t callback, void *user_arg);
 
-/**
- * @brief Make a pipe persist through a run time reset
- *
- * Normally when a HCD_PORT_CMD_RESET is called, all pipes should already have been freed. However There may be cases
- * (such as during enumeration) when a pipe must persist through a reset. This function will mark a pipe as
- * persistent allowing it to survive a reset. When HCD_PORT_CMD_RESET is called, the pipe can continue to be used after
- * the reset.
- *
- * @param pipe_hdl Pipe handle
- * @retval ESP_OK: Pipe successfully marked as persistent
- * @retval ESP_ERR_INVALID_STATE: Pipe is not in a condition to be made persistent
- */
-esp_err_t hcd_pipe_set_persist_reset(hcd_pipe_handle_t pipe_hdl);
-
 /**
  * @brief Get the context variable of a pipe from its handle
  *