Skip to content

drivers: usb: udc_stm32: Handle HAL callbacks on separate thread #80820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kartben merged 3 commits into from
Jan 28, 2025
Merged

drivers: usb: udc_stm32: Handle HAL callbacks on separate thread #80820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
700e9fb
drivers: udc: stm32: Offload data callbacks to thread
topisani Nov 3, 2024
bd9d7a7
drivers: udc: stm32: set the hs capability if high speed is available
topisani Nov 20, 2024
cedc8c6
drivers: udc: stm32: Fix RX FIFO min size
topisani Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions drivers/usb/udc/Kconfig.stm32
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,26 @@ config UDC_STM32
default y
help
STM32 USB device controller driver.

if UDC_STM32

config UDC_STM32_STACK_SIZE
int "UDC controller driver internal thread stack size"
default 512
help
STM32 USB device controller driver internal thread stack size.

config UDC_STM32_THREAD_PRIORITY
int "STM32 USB controller driver thread priority"
default 8
help
STM32 USB device controller driver thread priority.

config UDC_STM32_MAX_QMESSAGES
int "STM32 UDC driver maximum number of ISR event messages"
range 4 64
default 8
help
Maximum number of messages for handling of STM32 USBD ISR events.

endif
205 changes: 148 additions & 57 deletions drivers/usb/udc/udc_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@

#include "udc_common.h"

#include "stm32_hsem.h"

#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(udc_stm32, CONFIG_UDC_DRIVER_LOG_LEVEL);

Expand All @@ -50,6 +48,8 @@ struct udc_stm32_data {
void (*pcd_prepare)(const struct device *dev);
int (*clk_enable)(void);
int (*clk_disable)(void);
struct k_thread thread_data;
struct k_msgq msgq_data;
};

struct udc_stm32_config {
Expand All @@ -58,6 +58,19 @@ struct udc_stm32_config {
uint32_t dram_size;
uint16_t ep0_mps;
uint16_t ep_mps;
int speed_idx;
};

enum udc_stm32_msg_type {
UDC_STM32_MSG_SETUP,
UDC_STM32_MSG_DATA_OUT,
UDC_STM32_MSG_DATA_IN,
};

struct udc_stm32_msg {
uint8_t type;
uint8_t ep;
uint16_t rx_count;
};

static int udc_stm32_lock(const struct device *dev)
Expand Down Expand Up @@ -125,6 +138,26 @@ void HAL_PCD_ResumeCallback(PCD_HandleTypeDef *hpcd)
udc_submit_event(priv->dev, UDC_EVT_RESUME, 0);
}

void HAL_PCD_SetupStageCallback(PCD_HandleTypeDef *hpcd)
{
struct udc_stm32_data *priv = hpcd2data(hpcd);
struct udc_stm32_msg msg = {.type = UDC_STM32_MSG_SETUP};
int err;

err = k_msgq_put(&priv->msgq_data, &msg, K_NO_WAIT);

if (err < 0) {
LOG_ERR("UDC Message queue overrun");
}
}

void HAL_PCD_SOFCallback(PCD_HandleTypeDef *hpcd)
{
struct udc_stm32_data *priv = hpcd2data(hpcd);

udc_submit_event(priv->dev, UDC_EVT_SOF, 0);
}

static int usbd_ctrl_feed_dout(const struct device *dev, const size_t length)
{
struct udc_stm32_data *priv = udc_get_private(dev);
Expand All @@ -143,57 +176,6 @@ static int usbd_ctrl_feed_dout(const struct device *dev, const size_t length)
return 0;
}

void HAL_PCD_SetupStageCallback(PCD_HandleTypeDef *hpcd)
{
struct udc_stm32_data *priv = hpcd2data(hpcd);
struct usb_setup_packet *setup = (void *)priv->pcd.Setup;
const struct device *dev = priv->dev;
struct net_buf *buf;
int err;

buf = udc_ctrl_alloc(dev, USB_CONTROL_EP_OUT,
sizeof(struct usb_setup_packet));
if (buf == NULL) {
LOG_ERR("Failed to allocate for setup");
return;
}

udc_ep_buf_set_setup(buf);
memcpy(buf->data, setup, 8);
net_buf_add(buf, 8);

udc_ctrl_update_stage(dev, buf);

if (!buf->len) {
return;
}

if ((setup->bmRequestType == 0) &&
(setup->bRequest == USB_SREQ_SET_ADDRESS)) {
/* HAL requires we set the address before submitting status */
HAL_PCD_SetAddress(&priv->pcd, setup->wValue);
}

if (udc_ctrl_stage_is_data_out(dev)) {
/* Allocate and feed buffer for data OUT stage */
err = usbd_ctrl_feed_dout(dev, udc_data_stage_length(buf));
if (err == -ENOMEM) {
udc_submit_ep_event(dev, buf, err);
}
} else if (udc_ctrl_stage_is_data_in(dev)) {
udc_ctrl_submit_s_in_status(dev);
} else {
udc_ctrl_submit_s_status(dev);
}
}

void HAL_PCD_SOFCallback(PCD_HandleTypeDef *hpcd)
{
struct udc_stm32_data *priv = hpcd2data(hpcd);

udc_submit_event(priv->dev, UDC_EVT_SOF, 0);
}

static void udc_stm32_flush_tx_fifo(const struct device *dev)
{
struct udc_stm32_data *priv = udc_get_private(dev);
Expand Down Expand Up @@ -275,6 +257,36 @@ void HAL_PCD_DataOutStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
{
uint32_t rx_count = HAL_PCD_EP_GetRxCount(hpcd, epnum);
struct udc_stm32_data *priv = hpcd2data(hpcd);
struct udc_stm32_msg msg = {
.type = UDC_STM32_MSG_DATA_OUT,
.ep = epnum,
.rx_count = rx_count,
};
int err;

err = k_msgq_put(&priv->msgq_data, &msg, K_NO_WAIT);
if (err != 0) {
LOG_ERR("UDC Message queue overrun");
}
}

void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
{
struct udc_stm32_data *priv = hpcd2data(hpcd);
struct udc_stm32_msg msg = {
.type = UDC_STM32_MSG_DATA_IN,
.ep = epnum,
};
int err;

err = k_msgq_put(&priv->msgq_data, &msg, K_NO_WAIT);
if (err != 0) {
LOG_ERR("UDC Message queue overrun");
}
}

static void handle_msg_data_out(struct udc_stm32_data *priv, uint8_t epnum, uint16_t rx_count)
{
const struct device *dev = priv->dev;
uint8_t ep = epnum | USB_EP_DIR_OUT;
struct net_buf *buf;
Expand Down Expand Up @@ -312,9 +324,8 @@ void HAL_PCD_DataOutStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
}
}

void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
static void handle_msg_data_in(struct udc_stm32_data *priv, uint8_t epnum)
{
struct udc_stm32_data *priv = hpcd2data(hpcd);
const struct device *dev = priv->dev;
uint8_t ep = epnum | USB_EP_DIR_IN;
struct net_buf *buf;
Expand Down Expand Up @@ -378,6 +389,69 @@ void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
}
}

static void handle_msg_setup(struct udc_stm32_data *priv)
{
struct usb_setup_packet *setup = (void *)priv->pcd.Setup;
const struct device *dev = priv->dev;
struct net_buf *buf;
int err;

buf = udc_ctrl_alloc(dev, USB_CONTROL_EP_OUT, sizeof(struct usb_setup_packet));
if (buf == NULL) {
LOG_ERR("Failed to allocate for setup");
return;
}

udc_ep_buf_set_setup(buf);
memcpy(buf->data, setup, 8);
net_buf_add(buf, 8);

udc_ctrl_update_stage(dev, buf);

if (!buf->len) {
return;
}

if ((setup->bmRequestType == 0) && (setup->bRequest == USB_SREQ_SET_ADDRESS)) {
/* HAL requires we set the address before submitting status */
HAL_PCD_SetAddress(&priv->pcd, setup->wValue);
}

if (udc_ctrl_stage_is_data_out(dev)) {
/* Allocate and feed buffer for data OUT stage */
err = usbd_ctrl_feed_dout(dev, udc_data_stage_length(buf));
if (err == -ENOMEM) {
udc_submit_ep_event(dev, buf, err);
}
} else if (udc_ctrl_stage_is_data_in(dev)) {
udc_ctrl_submit_s_in_status(dev);
} else {
udc_ctrl_submit_s_status(dev);
}
}

static void udc_stm32_thread_handler(void *arg1, void *arg2, void *arg3)
{
const struct device *dev = arg1;
struct udc_stm32_data *priv = udc_get_private(dev);
struct udc_stm32_msg msg;

while (true) {
k_msgq_get(&priv->msgq_data, &msg, K_FOREVER);
switch (msg.type) {
case UDC_STM32_MSG_SETUP:
handle_msg_setup(priv);
break;
case UDC_STM32_MSG_DATA_IN:
handle_msg_data_in(priv, msg.ep);
break;
case UDC_STM32_MSG_DATA_OUT:
handle_msg_data_out(priv, msg.ep, msg.rx_count);
break;
}
}
}

#if DT_INST_NODE_HAS_PROP(0, disconnect_gpios)
void HAL_PCDEx_SetConnectionState(PCD_HandleTypeDef *hpcd, uint8_t state)
{
Expand Down Expand Up @@ -472,9 +546,9 @@ static void udc_stm32_mem_init(const struct device *dev)
}

/* The documentation is not clear at all about RX FiFo size requirement,
* Allocate a minimum of 0x40 words, which seems to work reliably.
* 160 has been selected through trial and error.
*/
words = MAX(0x40, cfg->ep_mps / 4);
words = MAX(160, cfg->ep_mps / 4);
Copy link
Contributor

@jfischer-no jfischer-no Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On USB HS, the previous lower limit of 64 is too small, the value
160 has been chosen apparently through trial and error.

At a minimum, this value should be selected based on the controller type and capabilities, not "trial and error".

Copy link
Contributor Author

@topisani topisani Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the discussion here: #62530
I've been digging through documentation and related issues from when this change was introduced on the old usb device driver, and i cannot seem to find any satisfactory explanation other than "this value works". This configuration is what worked in the old driver, which has been used a lot in practice. But maybe someone at ST can help settle where this value comes from (@erwango)?

Copy link
Member

@erwango erwango Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately @Desvauxm-st left the company. So I don't have more information other than what is available in the PR. @marwaiehm-st maybe ?

Copy link
Contributor

@marwaiehm-st marwaiehm-st Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we tried to find a satisfactory explanation other than "this value works", but until now, we haven't found an explanation for why the minimum value of FIFO RX needs to be 160.

Copy link
Member

@erwango erwango Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfischer-no We're trying to make a better picture on the whole USB next for STM32. In the mean time would you be ok to move on with this ? At least current PR allows to fix some known issues and we need it to progress on remaining points.

Copy link
Contributor

@jfischer-no jfischer-no Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with this value. But, "Calculating FIFO Size" is described in DWC2 Programming Guide, you should have access to this documentation internally. IIRC the FIFO calculation for the one other type is much simpler. Instead of continuing "trial and error" with this driver, STM32 controller should start using DWC2 controller and clean up this driver to be used with non-DWC2 type controller.

Copy link
Contributor Author

@topisani topisani Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STM32 controller should start using DWC2 controller and clean up this driver to be used with non-DWC2 type controller.

Is this a discussion we can track in a separate issue? Some consideration was made years ago (#4412), but it seems like an area where a decision should be made, and this PR seems like the wrong place to have that discussion.

Copy link
Contributor

@jfischer-no jfischer-no Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is the right place to make a comment about it and it is related to FIFO size calculation.

HAL_PCDEx_SetRxFiFo(&priv->pcd, words);
priv->occupied_mem = words * 4;

Expand Down Expand Up @@ -887,6 +961,7 @@ static const struct udc_stm32_config udc0_cfg = {
.pma_offset = USB_BTABLE_SIZE,
.ep0_mps = EP0_MPS,
.ep_mps = EP_MPS,
.speed_idx = DT_ENUM_IDX_OR(DT_DRV_INST(0), maximum_speed, 1),
};

#if defined(USB_OTG_FS) || defined(USB_OTG_HS)
Expand Down Expand Up @@ -1128,6 +1203,10 @@ static const struct gpio_dt_spec ulpi_reset =
GPIO_DT_SPEC_GET_OR(DT_PHANDLE(DT_INST(0, st_stm32_otghs), phys), reset_gpios, {0});
#endif

static char udc_msgq_buf_0[CONFIG_UDC_STM32_MAX_QMESSAGES * sizeof(struct udc_stm32_msg)];

K_THREAD_STACK_DEFINE(udc_stm32_stack_0, CONFIG_UDC_STM32_STACK_SIZE);

static int udc_stm32_driver_init0(const struct device *dev)
{
struct udc_stm32_data *priv = udc_get_private(dev);
Expand Down Expand Up @@ -1178,13 +1257,25 @@ static int udc_stm32_driver_init0(const struct device *dev)
data->caps.rwup = true;
data->caps.out_ack = false;
data->caps.mps0 = UDC_MPS0_64;
if (cfg->speed_idx == 2) {
data->caps.hs = true;
}

priv->dev = dev;
priv->irq = UDC_STM32_IRQ;
priv->clk_enable = priv_clock_enable;
priv->clk_disable = priv_clock_disable;
priv->pcd_prepare = priv_pcd_prepare;

k_msgq_init(&priv->msgq_data, udc_msgq_buf_0, sizeof(struct udc_stm32_msg),
CONFIG_UDC_STM32_MAX_QMESSAGES);

k_thread_create(&priv->thread_data, udc_stm32_stack_0,
K_THREAD_STACK_SIZEOF(udc_stm32_stack_0), udc_stm32_thread_handler,
(void *)dev, NULL, NULL, K_PRIO_COOP(CONFIG_UDC_STM32_THREAD_PRIORITY),
K_ESSENTIAL, K_NO_WAIT);
k_thread_name_set(&priv->thread_data, dev->name);

IRQ_CONNECT(UDC_STM32_IRQ, UDC_STM32_IRQ_PRI, udc_stm32_irq,
DEVICE_DT_INST_GET(0), 0);

Expand Down
Loading