Skip to content

Commit

Permalink
applications: nrf_desktop: Remove extra USB thread on nRF54H
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@alstrzebonski @rlubos
alstrzebonski authored and rlubos committed Oct 23, 2024
1 parent 696f67a commit 6cb02b2
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 165 deletions.
12 changes: 5 additions & 7 deletions applications/nrf_desktop/doc/usb_state.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
68 changes: 23 additions & 45 deletions applications/nrf_desktop/src/modules/Kconfig.usb_state
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
143 changes: 30 additions & 113 deletions applications/nrf_desktop/src/modules/usb_state.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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)) {
Expand All @@ -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);
}
}

Expand Down

0 comments on commit 6cb02b2

Please sign in to comment.