applications: nrf_desktop: Remove extra USB thread on nRF54H

Removes the extra USB thread on nRF54H as it's no longer needed.
Sets the CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT timeout to allow the
usbd_enable() function to return error if the USB cable is not
connected.

Jira: NCSDK-28000
Jira: NCSDK-28381

Signed-off-by: Aleksander Strzebonski <[email protected]>

Author: alstrzebonski

applications/nrf_desktop/doc/usb_state.rst

@@ -236,17 +236,15 @@ The module sends the report using :c:struct:`hid_report_event`, that is handled
 nRF54H20 support
 ================
 
-Due to the characteristics of the nRF54H20 USB Device Controller (UDC), several changes have been made in the USB state module to support the nRF54H20 platform:
+Due to the characteristics of the nRF54H20 USB Device Controller (UDC), following change has been made in the USB state module to support the nRF54H20 platform:
 
-* The USB state module creates a separate thread to initialize, enable, and disable the USB stack.
 * The module disables the USB stack when the USB cable is disconnected and enables the stack when the cable is connected.
 
-These changes are applicable to the nRF54H20 platform only.
-They are necessary to ensure proper USB stack operation on the nRF54H20 platform.
+This change is applicable to the nRF54H20 platform only.
+It is necessary to ensure proper USB stack operation on the nRF54H20 platform.
 
-The USB stack cannot be initialized from the system workqueue thread, because it causes a deadlock.
-Because of that, a separate thread is used to initialize the USB stack.
-For more details, see the :ref:`CONFIG_DESKTOP_USB_INIT_THREAD <config_desktop_app_options>` Kconfig option.
+The :kconfig:option:`CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT` Kconfig option is set to a non-zero value to prevent the :c:func:`usbd_enable` function from blocking the application forever when the USB cable is not connected.
+Instead, the function returns an error on timeout.
 The UDC is powered down whenever the USB cable is disconnected, failing to trigger the necessary callbacks to the USB stack.
 It may cause the USB stack to become non-functional.
 The USB stack is disabled upon disconnecting the cable to work around this issue.

applications/nrf_desktop/src/modules/Kconfig.usb_state

@@ -57,37 +57,6 @@ config DESKTOP_USB_HID_REPORT_SENT_ON_SOF
 	  report pipeline with two sequential reports is required to ensure that
 	  the USB peripheral can provide a HID report on every USB poll.
 
-config DESKTOP_USB_INIT_THREAD
-	bool
-	default y if SOC_SERIES_NRF54HX
-	help
-	  Initialize USB stack in a separate thread instead of doing it in
-	  the context of the system workqueue.
-	  For the nRF54HX SoC use-case, the usbd_init() function blocks while
-	  waiting for a synchronization object that is received after the system
-	  controller sends the USB service event through the NRFS and IPC with
-	  the ICMSG backend to the application core.
-	  The ICMSG backend uses the system workqueue context for both
-	  initialization and on the incoming message handling, so calling the
-	  usbd_init() function from the same context causes deadlock during
-	  application boot.
-
-config DESKTOP_USB_INIT_THREAD_STACK_SIZE
-	int
-	default 2048
-
-config DESKTOP_USB_INIT_THREAD_PRIORITY
-	int
-	default -1
-
-config DESKTOP_USB_INIT_THREAD_DELAY_MS
-	int
-	default 30
-	help
-	  Due to an issue in the USB stack initialization on the nRF54HX SoC,
-	  the usbd_init() cannot be called too early. The delay is introduced
-	  to delay this call.
-
 choice DESKTOP_USB_STACK
 	prompt "USB stack"
 	default DESKTOP_USB_STACK_LEGACY
@@ -206,20 +175,29 @@ config USBD_HID_IN_BUF_COUNT
 config DESKTOP_USB_STACK_NEXT_DISABLE_ON_VBUS_REMOVAL
 	bool
 	default y if SOC_SERIES_NRF54HX
-	depends on DESKTOP_USB_INIT_THREAD
-	select REBOOT
-	help
-	  Disable USB stack on VBUS removal. This is a workaround for the USB
-	  driver not working correctly on the nRF54HX SoC. After the USB cable
-	  is removed, the USB driver is powered down and doesn't call appropriate
-	  callbacks to the USB stack. It may lead to the USB stack ending in a
-	  broken state. Calling usbd_disable() on VBUS removal workarounds this
-	  issue. When this option is enabled, the usb_init_thread will be used to
-	  call the usbd_enable() and usbd_disable() functions on VBUS events. This
-	  is done because the usbd_enable() function blocks if the USB cable is
-	  not connected. A separate thread allows the application to trigger a
-	  reboot from the workqueue context to prevent blocking the application
-	  forever.
+	help
+	  Disable the USB stack on VBUS removal and enable it on VBUS ready to
+	  workaround UDC driver limitations of the nRF54HX SoC.
+
+	  After the USB cable is removed, the USB driver is powered down and
+	  doesn't call appropriate callbacks to the USB stack. It may lead to
+	  the USB stack ending in a broken state. Calling usbd_disable() on VBUS
+	  removal workarounds this issue.
+
+	  The usbd_enable() function blocks until USB cable is connected. If the
+	  cable is not connected during the predefined wait period
+	  (CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT), the function returns
+	  earlier with a timeout error. The function is called on VBUS ready to
+	  avoid calling usbd_enable() periodically.
+
+config UDC_DWC2_USBHS_VBUS_READY_TIMEOUT
+	int
+	depends on SOC_SERIES_NRF54HX
+	default 100
+	help
+	  For the nRF54HX SoC the timeout must be set to a non-zero value to
+	  prevent the usbd_enable() function from blocking the application
+	  forever when the USB cable is not connected.
 
 choice USBD_LOG_LEVEL_CHOICE
 	default USBD_LOG_LEVEL_WRN

applications/nrf_desktop/src/modules/usb_state.c

@@ -11,8 +11,6 @@
 #include <zephyr/types.h>
 #include <zephyr/sys/byteorder.h>
 #include <zephyr/sys/util.h>
-#include <zephyr/kernel.h>
-#include <zephyr/sys/reboot.h>
 
 #include <zephyr/usb/usbd.h>
 #include <zephyr/usb/usbd_msg.h>
@@ -131,26 +129,17 @@ BUILD_ASSERT(!IS_ENABLED(CONFIG_DESKTOP_HID_STATE_ENABLE) ||
 	     IS_ENABLED(CONFIG_DESKTOP_USB_SELECTIVE_REPORT_SUBSCRIPTION) ||
 	     (ARRAY_SIZE(usb_hid_device) <= 1));
 
+#if CONFIG_SOC_SERIES_NRF54HX
+BUILD_ASSERT(CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT > 0,
+	     "Timeout must be set to prevent the usbd_enable() function from blocking the "
+	     "application forever when the USB cable is not connected.");
+#endif
+
 static struct usbd_context *usbd_ctx;
 static bool usb_enabled;
 
 static struct config_channel_transport cfg_chan_transport;
 
-static struct k_thread usb_init_thread;
-static K_THREAD_STACK_DEFINE(usb_init_thread_stack, CONFIG_DESKTOP_USB_INIT_THREAD_STACK_SIZE);
-
-#define USB_REQ_TIMEOUT_MS	100
-
-/* USB events */
-enum {
-	USB_REQ_ENABLE		= BIT(0),
-	USB_REQ_DISABLE		= BIT(1),
-	USB_RSP_SUCCESS		= BIT(2),
-	USB_RSP_FAIL		= BIT(3),
-};
-
-static K_EVENT_DEFINE(usb_event);
-
 static void report_sent(struct usb_hid_device *usb_hid, struct usb_hid_buf *buf, bool error);
 
 static uint8_t usb_hid_buf_get_report_id(struct usb_hid_buf *buf)
@@ -1204,53 +1193,39 @@ static int usb_init_next_hids_init(void)
 	return err;
 }
 
-static int usb_thread_req_send(bool enable)
-{
-	uint32_t events;
-	int err;
-
-	__ASSERT_NO_MSG(!k_event_test(&usb_event, USB_REQ_ENABLE | USB_REQ_DISABLE));
-
-	LOG_DBG("%sabling USB", enable ? "En" : "Dis");
-	(void) k_event_set(&usb_event, enable ? USB_REQ_ENABLE : USB_REQ_DISABLE);
-	events = k_event_wait(&usb_event,
-			      USB_RSP_SUCCESS | USB_RSP_FAIL,
-			      false,
-			      K_MSEC(USB_REQ_TIMEOUT_MS));
-	(void) k_event_clear(&usb_event, events);
-
-	if (events & USB_RSP_SUCCESS) {
-		LOG_DBG("USB %sabled", enable ? "en" : "dis");
-		usb_enabled = enable;
-		err = 0;
-	} else if (events & USB_RSP_FAIL) {
-		LOG_ERR("Failed to %sable USB", enable ? "en" : "dis");
-		module_set_state(MODULE_STATE_ERROR);
-		err = -EIO;
-	} else {
-		LOG_ERR("Fatal error - USB %sable timeout. Rebooting...", enable ? "en" : "dis");
-		LOG_PANIC();
-		sys_reboot(SYS_REBOOT_WARM);
-		err = -EIO;
-	}
-
-	return err;
-}
-
 static int handle_usbd_state_on_status_change(enum usbd_msg_type type)
 {
 	int err = 0;
 
 	switch (type) {
 	case USBD_MSG_VBUS_READY:
 		if (!usb_enabled) {
-			err = usb_thread_req_send(true);
+			err = usbd_enable(usbd_ctx);
+			if (err == -ETIMEDOUT) {
+				/* Probably the USB cable was disconnected before the usbd_enable
+				 * was executed. Ignoring the error. The USB will be enabled once
+				 * the cable is connected again.
+				 */
+				LOG_WRN("usbd_enable timed out");
+				err = 0;
+			} else if (err) {
+				LOG_ERR("usbd_enable failed (err: %d)", err);
+				module_set_state(MODULE_STATE_ERROR);
+			} else {
+				usb_enabled = true;
+			}
 		}
 		break;
 
 	case USBD_MSG_VBUS_REMOVED:
 		if (usb_enabled) {
-			err = usb_thread_req_send(false);
+			err = usbd_disable(usbd_ctx);
+			if (err) {
+				LOG_ERR("usbd_disable failed (err: %d)", err);
+				module_set_state(MODULE_STATE_ERROR);
+			} else {
+				usb_enabled = false;
+			}
 		}
 		break;
 
@@ -1553,57 +1528,6 @@ static int usb_init(void)
 	return err;
 }
 
-static void wait_for_usb_requests(void)
-{
-	while (true) {
-		uint32_t events;
-		int err;
-
-		events = k_event_wait(&usb_event,
-				      USB_REQ_ENABLE | USB_REQ_DISABLE,
-				      false,
-				      K_FOREVER);
-		(void) k_event_clear(&usb_event, events);
-		__ASSERT_NO_MSG(!(events & USB_REQ_ENABLE) || !(events & USB_REQ_DISABLE));
-
-		__ASSERT_NO_MSG(usbd_ctx);
-		if (events & USB_REQ_ENABLE) {
-			err = usbd_enable(usbd_ctx);
-			if (err) {
-				LOG_ERR("usbd_enable failed (err: %d)", err);
-			}
-		} else if (events & USB_REQ_DISABLE) {
-			err = usbd_disable(usbd_ctx);
-			if (err) {
-				LOG_ERR("usbd_disable failed (err: %d)", err);
-			}
-		} else {
-			__ASSERT_NO_MSG(false);
-			err = -EINVAL;
-		}
-
-		__ASSERT_NO_MSG(!k_event_test(&usb_event, USB_RSP_FAIL | USB_RSP_SUCCESS));
-		(void) k_event_post(&usb_event, err ? USB_RSP_FAIL : USB_RSP_SUCCESS);
-	}
-}
-
-static void usb_init_thread_fn(void *dummy0, void *dummy1, void *dummy2)
-{
-	ARG_UNUSED(dummy0);
-	ARG_UNUSED(dummy1);
-	ARG_UNUSED(dummy2);
-
-	if (usb_init()) {
-		module_set_state(MODULE_STATE_ERROR);
-	} else {
-		module_set_state(MODULE_STATE_READY);
-	}
-
-	if (IS_ENABLED(CONFIG_DESKTOP_USB_STACK_NEXT_DISABLE_ON_VBUS_REMOVAL)) {
-		wait_for_usb_requests();
-	}
-}
-
 static bool app_event_handler(const struct app_event_header *aeh)
 {
 	if (is_hid_report_event(aeh)) {
@@ -1619,17 +1543,10 @@ static bool app_event_handler(const struct app_event_header *aeh)
 			__ASSERT_NO_MSG(!initialized);
 			initialized = true;
 
-			if (IS_ENABLED(CONFIG_DESKTOP_USB_INIT_THREAD)) {
-				(void) k_thread_create(
-					&usb_init_thread,
-					usb_init_thread_stack,
-					K_THREAD_STACK_SIZEOF(usb_init_thread_stack),
-					usb_init_thread_fn,
-					NULL, NULL, NULL,
-					CONFIG_DESKTOP_USB_INIT_THREAD_PRIORITY, 0,
-					K_MSEC(CONFIG_DESKTOP_USB_INIT_THREAD_DELAY_MS));
+			if (usb_init()) {
+				module_set_state(MODULE_STATE_ERROR);
 			} else {
-				usb_init_thread_fn(NULL, NULL, NULL);
+				module_set_state(MODULE_STATE_READY);
 			}
 		}