From 8e9d5a3cf54fcc2bd29cb726410554f4de9f0eee Mon Sep 17 00:00:00 2001 From: Dylan Laduranty Date: Fri, 7 Apr 2023 21:17:08 +0200 Subject: [PATCH 1/2] cpu/stm32/usbdev_fs: fix endpoint registration mechanism For the usbdev_fs peripheral, IN and OUT endpoints of the same index must have the same type. For instance, if EP1 OUT is a bulk endpoint, EP1 IN must either be unused or used as bulk too but it cannot be used as interrupt or isochronous. With the previous check, the following registration pattern (EP OUT Bulk -> EP IN Interrupt -> EP IN Bulk) would assign both EP OUT Bulk and EP IN Interrupt to same endpoint index. So the configuration would be broken. Applying the same registration pattern with this patch would now produce EP OUT Bulk -> 1 / EP IN Interrupt -> 2 / EP IN Bulk 1. Which is a working configuration for this IP Signed-off-by: Dylan Laduranty --- cpu/stm32/periph/usbdev_fs.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/cpu/stm32/periph/usbdev_fs.c b/cpu/stm32/periph/usbdev_fs.c index 5f94d520a123..b23cc98abba5 100644 --- a/cpu/stm32/periph/usbdev_fs.c +++ b/cpu/stm32/periph/usbdev_fs.c @@ -80,7 +80,7 @@ #error "STM32 line is not supported" #endif -/* UUSB IP local base address for the endpoints buffers in PMA SRAM */ +/* USB IP local base address for the endpoints buffers in PMA SRAM */ #define _PMA_BUF_BASE_OFFSET (_ENDPOINT_NUMOF * 8) /* List of instantiated USB peripherals */ @@ -400,16 +400,19 @@ static usbdev_ep_t *_get_ep(stm32_usbdev_fs_t *usbdev, unsigned num, return dir == USB_EP_DIR_IN ? &usbdev->in[num] : &usbdev->out[num]; } -static bool _ep_check(usbdev_ep_t* ep) +static bool _ep_check(stm32_usbdev_fs_t *usbdev, unsigned idx, + usb_ep_dir_t dir, usb_ep_type_t type) { - /* Check if endpoint already configured */ - if (ep->dir == USB_EP_DIR_IN) { - if ((EP_REG(ep->num) & USB_EPTX_STAT) == USB_EP_TX_DIS) { + usbdev_ep_t* ep_in = &usbdev->in[idx]; + usbdev_ep_t* ep_out = &usbdev->out[idx]; + + if (dir == USB_EP_DIR_IN) { + if (ep_out->type == type || ep_out->type == USB_EP_TYPE_NONE) { return true; } } else { - if ((EP_REG(ep->num) & USB_EPRX_STAT) == USB_EP_RX_DIS) { + if (ep_in->type == type || ep_in->type == USB_EP_TYPE_NONE) { return true; } } @@ -471,10 +474,12 @@ static usbdev_ep_t *_usbdev_new_ep(usbdev_t *dev, usb_ep_type_t type, for (unsigned idx = 1; idx < _ENDPOINT_NUMOF && !new_ep; idx++) { usbdev_ep_t *ep = _get_ep(usbdev, idx, dir); /* Check if endpoint is available */ - bool avail = _ep_check(ep); - if (ep->type == USB_EP_TYPE_NONE && avail) { - new_ep = ep; - new_ep->num = idx; + if (ep->type == USB_EP_TYPE_NONE) { + bool avail = _ep_check(usbdev, idx, dir, type); + if (avail) { + new_ep = ep; + new_ep->num = idx; + } } } } From 9311940adc8ecb808d1a6b5dceccf4a80af88df9 Mon Sep 17 00:00:00 2001 From: Dylan Laduranty Date: Fri, 7 Apr 2023 21:21:30 +0200 Subject: [PATCH 2/2] cpu/stm32/usbdev_fs: fix EP_REG(x) assignment for toggleable bits EP_REG(x) registers have a total of 6 toggleable bits. Those bits can only be toggled if we write a one to it, otherwise writing a zero has no effect This commit fixes all the access to these registers to prevent from modifying these bits when not needed Signed-off-by: Dylan Laduranty --- cpu/stm32/periph/usbdev_fs.c | 40 ++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/cpu/stm32/periph/usbdev_fs.c b/cpu/stm32/periph/usbdev_fs.c index b23cc98abba5..5bbbbe7f48a7 100644 --- a/cpu/stm32/periph/usbdev_fs.c +++ b/cpu/stm32/periph/usbdev_fs.c @@ -132,6 +132,10 @@ typedef struct { #error "_PMA_ACCESS_SCHEME is not defined" #endif +/* Bit mask containing all toggleable bits in a EP_REG(x) register */ +#define _EP_REG_TOGGLE_ONLY_BITS_MASK (USB_EP_DTOG_TX | USB_EP_DTOG_RX | \ + USB_EPTX_STAT | USB_EPRX_STAT ) + #define EP_DESC ((ep_buf_desc_t*)USB1_PMAADDR) #define EP_REG(x) (*((volatile uint32_t*) (((uintptr_t)(USB)) + (4*x)))) @@ -313,8 +317,9 @@ void USBDEV_ISR(void) { unsigned epnum = irq_status & 0x000F; usbdev_ep_t *ep = (irq_status & USB_ISTR_DIR) ? &_ep_out[epnum] : &_ep_in[epnum]; /* get type and add endpoint address */ - uint16_t reg = _type_to_reg(ep->type) | ep->num; - + uint16_t reg = (_type_to_reg(ep->type) | ep->num); + /* Avoid toggling by mistake these bits */ + CLRBIT(reg, (_EP_REG_TOGGLE_ONLY_BITS_MASK)); if (irq_status & USB_ISTR_DIR) { /* Clear RX CTR by writing 0, avoid clear TX CTR so leave it to one */ EP_REG(epnum) = reg | USB_EP_CTR_TX; @@ -422,8 +427,10 @@ static bool _ep_check(stm32_usbdev_fs_t *usbdev, unsigned idx, static void _ep_enable(usbdev_ep_t *ep) { uint16_t reg = EP_REG(ep->num); - /* Avoid modification of the following registers */ + /* Avoid modification of the following bits in the register as writing + 1 has no effect on these bits */ SETBIT(reg, USB_EP_CTR_RX | USB_EP_CTR_TX); + CLRBIT(reg, USB_EP_DTOG_RX | USB_EP_DTOG_TX); if (ep->dir == USB_EP_DIR_OUT) { _set_ep_out_status(®, USB_EP_RX_NAK); /* Avoid toggling the other direction */ @@ -441,7 +448,8 @@ static void _ep_enable(usbdev_ep_t *ep) static void _ep_disable(usbdev_ep_t *ep) { uint16_t reg = EP_REG(ep->num); - /* Avoid modification of the following registers */ + /* Avoid modification of the following bits in the register as writing + 1 has no effect on these bits */ SETBIT(reg, USB_EP_CTR_RX | USB_EP_CTR_TX); if (ep->dir == USB_EP_DIR_OUT) { @@ -580,16 +588,17 @@ static void _usbdev_esr(usbdev_t *dev) static void _usbdev_ep_init(usbdev_ep_t *ep) { + uint16_t reg = EP_REG(ep->num); /* Set endpoint type */ - uint16_t reg = _type_to_reg(ep->type); - /* Keep previous state if already set */ - reg |= EP_REG(ep->num) & (USB_EPTX_STAT | USB_EPRX_STAT); - _set_ep_out_status(®, USB_EP_RX_VALID); + reg &= ~(USB_EP_TYPE_MASK); + reg |= _type_to_reg(ep->type); + /* Keep the state ouf the other endpoint direction if already sets */ + CLRBIT(reg, _EP_REG_TOGGLE_ONLY_BITS_MASK); /* Assign EP address */ reg |= (ep->num & 0x000F); /* Ensure interrupts are cleared */ - reg |= USB_EP_CTR_RX | USB_EP_CTR_TX; - + reg &= ~(USB_EP_CTR_RX | USB_EP_CTR_TX); + /* Write the configuration to the register */ EP_REG(ep->num) = reg; if (ep->dir == USB_EP_DIR_IN) { EP_DESC[ep->num].addr_tx = _ep_in_buf[ep->num]; @@ -653,7 +662,8 @@ static int _usbdev_ep_get(usbdev_ep_t *ep, usbopt_ep_t opt, static void _ep_set_stall(usbdev_ep_t *ep, usbopt_enable_t enable) { uint16_t reg = EP_REG(ep->num); - /* Avoid modification of the following registers */ + /* Avoid modification of the following bits in the register as writing + 1 has no effect on these bits */ SETBIT(reg, USB_EP_CTR_RX | USB_EP_CTR_TX); if (ep->dir == USB_EP_DIR_IN) { @@ -742,11 +752,12 @@ static int _usbdev_ep_xmit(usbdev_ep_t *ep, uint8_t* buf, size_t len) { unsigned irq = irq_disable(); uint16_t reg = EP_REG(ep->num); - /* Avoid modification of the following registers */ + /* Avoid modification of the following bits in the register as writing + 1 has no effect on these bits */ SETBIT(reg, USB_EP_CTR_RX | USB_EP_CTR_TX); if (ep->dir == USB_EP_DIR_OUT) { _set_ep_out_status(®, USB_EP_RX_VALID); - _set_ep_in_status(®, USB_EP_TX_NAK); + CLRBIT(reg, USB_EPTX_STAT); if (len == 0) { len = ep->len; } @@ -761,8 +772,7 @@ static int _usbdev_ep_xmit(usbdev_ep_t *ep, uint8_t* buf, size_t len) _app_pbuf[ep->num] = buf; } else { _set_ep_in_status(®, USB_EP_TX_VALID); - _set_ep_out_status(®, USB_EP_RX_VALID); - + CLRBIT(reg, USB_EPRX_STAT); /* Transfer IN buffer content to USB PMA SRAM */ uint16_t* pma_ptr = (uint16_t *)(USB1_PMAADDR + (EP_DESC[ep->num].addr_tx * _PMA_ACCESS_STEP_SIZE));