Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 6 additions & 0 deletions port/dwc2/usb_dc_dwc2.c
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,11 @@ void dwc2_ep_write(uint8_t busid, uint8_t ep_idx, uint8_t *src, uint16_t len)
}
}

__WEAK void dwc2_override_hw_params(uint32_t reg_base, struct dwc2_hw_params *hw) {
(void)reg_base;
(void)hw;
}

void dwc2_ep_read(uint8_t busid, uint8_t *dest, uint16_t len)
{
uint32_t *p32;
Expand Down Expand Up @@ -495,6 +500,7 @@ int usb_dc_init(uint8_t busid)

dwc2_get_hwparams(USBD_BASE, &g_dwc2_udc[busid].hw_params);
dwc2_get_user_params(USBD_BASE, &g_dwc2_udc[busid].user_params);
dwc2_override_hw_params(USBD_BASE, &g_dwc2_udc[busid].hw_params);

if (g_dwc2_udc[busid].user_params.phy_utmi_width == 0) {
g_dwc2_udc[busid].user_params.phy_utmi_width = 8;
Expand Down
83 changes: 83 additions & 0 deletions port/dwc2/usb_glue_gd_arm.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Copyright (c) 2024, sakumisu
*
* SPDX-License-Identifier: Apache-2.0
*/
#include "usbd_core.h"
#include "stdint.h"
#include "usb_dwc2_reg.h"
#include "usb_dwc2_param.h"
#include "board_config.h"
Comment on lines +6 to +10

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing <string.h> and prefer angle-bracket form for <stdint.h>.

memcpy is called at line 53 but <string.h> is not included anywhere in this file (it may transitively be available via usbd_core.h, but relying on that is fragile). Also, stdint.h is a system header and should use angle brackets, matching the rest of the project.

🛠️ Proposed fix
 `#include` "usbd_core.h"
-#include "stdint.h"
+#include <stdint.h>
+#include <string.h>
 `#include` "usb_dwc2_reg.h"
 `#include` "usb_dwc2_param.h"
 `#include` "board_config.h"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#include "usbd_core.h"
#include "stdint.h"
#include "usb_dwc2_reg.h"
#include "usb_dwc2_param.h"
#include "board_config.h"
`#include` "usbd_core.h"
`#include` <stdint.h>
`#include` <string.h>
`#include` "usb_dwc2_reg.h"
`#include` "usb_dwc2_param.h"
`#include` "board_config.h"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@port/dwc2/usb_glue_gd_arm.c` around lines 6 - 10, Add a direct include for
the C string header and switch the system stdint include to angle-bracket form:
explicitly include <string.h> (so memcpy is guaranteed available and not relied
on transitively via usbd_core.h) and change "stdint.h" to use <stdint.h>; update
the include list near the top of usb_glue_gd_arm.c (look for the existing
"stdint.h" and uses of memcpy) to make these changes.


const struct dwc2_user_params param_pa11_pa12 = {
.phy_type = DWC2_PHY_TYPE_PARAM_FS,
.device_dma_enable = false,
.device_dma_desc_enable = false,
.device_rx_fifo_size = (320 - 16 * 4),
.device_tx_fifo_size = {
[0] = 16, // 64 byte
[1] = 16, // 64 byte
[2] = 16, // 64 byte
[3] = 16, // 64 byte
[4] = 0,
[5] = 0,
[6] = 0,
[7] = 0,
[8] = 0,
[9] = 0,
[10] = 0,
[11] = 0,
[12] = 0,
[13] = 0,
[14] = 0,
[15] = 0 },
.device_gccfg = ((1 << 16) | (1 << 19) |(1 << 21)),
.total_fifo_size = 320 // 1280 byte
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

#if CONFIG_USBDEV_EP_NUM != 4 && CONFIG_USBDEV_EP_NUM != 6
#error "gd32 only has 4 endpoints for pa11/pa12 and 6 endpoints for pb14/pb15"
#endif

void usb_dc_low_level_init(uint8_t busid) {
NVIC_EnableIRQ(USBFS_IRQn);
}

void usb_dc_low_level_deinit(uint8_t busid) {
NVIC_DisableIRQ(USBFS_IRQn);
}
Comment on lines +42 to +48

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm whether USBFS clock / GPIO setup happens elsewhere in the tree.
rg -nP -C2 'RCU_USBFS|rcu_usb_clock_config|rcu_periph_clock_enable\s*\(\s*RCU_USBFS'
rg -nP -C2 '\b(GPIO|gpio)_(init|mode_set|af_set).*PA1[12]\b'
fd -t f -e c -e h | xargs rg -lP 'usb_dc_low_level_init' 2>/dev/null

Repository: cherry-embedded/CherryUSB

Length of output: 996


🏁 Script executed:

cat -n port/dwc2/usb_glue_gd.c

Repository: cherry-embedded/CherryUSB

Length of output: 1489


🏁 Script executed:

cat -n port/dwc2/usb_glue_gd_arm.c

Repository: cherry-embedded/CherryUSB

Length of output: 2892


🏁 Script executed:

rg -n 'RCU_USBFS|rcu_usb_clock_config' port/dwc2/ -A 3 -B 3

Repository: cherry-embedded/CherryUSB

Length of output: 51


🏁 Script executed:

rg -n 'PA1[12]|PB1[45]' port/dwc2/ -A 2 -B 2

Repository: cherry-embedded/CherryUSB

Length of output: 530


🏁 Script executed:

wc -l port/dwc2/usb_glue_gd.c

Repository: cherry-embedded/CherryUSB

Length of output: 96


🏁 Script executed:

cat -n port/dwc2/usb_glue_gd.c | tail -100

Repository: cherry-embedded/CherryUSB

Length of output: 1489


🏁 Script executed:

find . -name '*gd*' -path '*/port/*' -type f | grep -E '\.(c|h)$'

Repository: cherry-embedded/CherryUSB

Length of output: 125


🏁 Script executed:

rg -n 'RCU_USBFS|rcu_periph_clock_enable.*USBFS|rcu_usb_clock_config' --type c --type h

Repository: cherry-embedded/CherryUSB

Length of output: 51


🏁 Script executed:

rg -n 'board_config|board.h' port/dwc2/usb_glue_gd_arm.c

Repository: cherry-embedded/CherryUSB

Length of output: 98


🏁 Script executed:

find . -name 'board_config.h' -o -name 'board.h' | head -5

Repository: cherry-embedded/CherryUSB

Length of output: 51


🏁 Script executed:

rg -n 'rcu_periph_clock_enable|RCU_' --type c --type h | head -20

Repository: cherry-embedded/CherryUSB

Length of output: 51


🏁 Script executed:

rg -n 'gpio.*init|GPIO.*AFIO' --type c --type h -i | grep -i 'gd32\|pa1[12]\|pb1[45]' | head -10

Repository: cherry-embedded/CherryUSB

Length of output: 51


🏁 Script executed:

rg -n 'usb_dc_low_level_init|usb_dc_init' -A 10 port/ --type c | head -80

Repository: cherry-embedded/CherryUSB

Length of output: 4321


🏁 Script executed:

rg -n 'rcu_periph_clock_enable|clock.*enable' port/ --type c -B2 -A2 | grep -E '(usb|USB)' | head -30

Repository: cherry-embedded/CherryUSB

Length of output: 1805


🏁 Script executed:

ls -la port/dwc2/ | grep glue

Repository: cherry-embedded/CherryUSB

Length of output: 684


🏁 Script executed:

cat -n port/dwc2/usb_glue_st.c | head -100

Repository: cherry-embedded/CherryUSB

Length of output: 3461


🏁 Script executed:

cat -n port/dwc2/usb_glue_esp.c | head -100

Repository: cherry-embedded/CherryUSB

Length of output: 4179


🏁 Script executed:

rg -n 'void usb_dc_low_level_init' port/dwc2/ -A 15 --type c

Repository: cherry-embedded/CherryUSB

Length of output: 7618


usb_dc_low_level_init() only enables NVIC; clock and GPIO setup are missing or undocumented.

The function only calls NVIC_EnableIRQ(USBFS_IRQn). Typical GD32 USBFS initialization also requires:

  • Enabling the USBFS bus clock (rcu_periph_clock_enable(RCU_USBFS))
  • Configuring USB clock prescaler for 48 MHz (rcu_usb_clock_config)
  • Setting up PA11/PA12 (or PB14/PB15) GPIO alternate-function pins

Other DWC2 port implementations (ST, Nation, Infineon, HC, Kendryte) perform such setup in their usb_dc_low_level_init() functions. If your board code handles this via the included board_config.h, add a comment documenting the contract; otherwise this file is incomplete.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@port/dwc2/usb_glue_gd_arm.c` around lines 42 - 48, usb_dc_low_level_init
currently only enables the IRQ (NVIC_EnableIRQ(USBFS_IRQn)) but omits USB clock
and GPIO setup; either implement the missing hardware init (enable USBFS
peripheral clock via rcu_periph_clock_enable(RCU_USBFS), configure USB 48MHz
prescaler with rcu_usb_clock_config(...), and configure USB D+ / D- pins (e.g.,
PA11/PA12 or PB14/PB15) to the USB AF) within usb_dc_low_level_init (and
undo/disable in usb_dc_low_level_deinit), or add a clear comment at the top of
this file stating that board_config.h is responsible for RCU and GPIO setup so
callers know the contract; reference usb_dc_low_level_init,
usb_dc_low_level_deinit, NVIC_EnableIRQ, USBFS_IRQn, rcu_periph_clock_enable,
rcu_usb_clock_config, and PA11/PA12 (or PB14/PB15) when making the change.


#ifndef CONFIG_USB_DWC2_CUSTOM_PARAM
void dwc2_get_user_params(uint32_t reg_base, struct dwc2_user_params *params)
{
memcpy(params, &param_pa11_pa12, sizeof(struct dwc2_user_params));

#ifdef CONFIG_USB_DWC2_CUSTOM_FIFO
struct usb_dwc2_user_fifo_config s_dwc2_fifo_config;

dwc2_get_user_fifo_config(reg_base, &s_dwc2_fifo_config);

params->device_rx_fifo_size = s_dwc2_fifo_config.device_rx_fifo_size;
for (uint8_t i = 0; i < MAX_EPS_CHANNELS; i++) {
params->device_tx_fifo_size[i] = s_dwc2_fifo_config.device_tx_fifo_size[i];
}
#endif
}
void dwc2_override_hw_params(uint32_t reg_base, struct dwc2_hw_params *hw) {
/* HWCFG2 reads 0, this is unknown why */
hw->num_dev_ep = 4;
}
Comment on lines +67 to +73

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -nP --type=c -C2 '\bnum_dev_ep\b' port/dwc2/usb_dc_dwc2.c
rg -nP --type=c -C1 '\bUSBD_BASE\b|\bUSBFS_BASE\b' port/dwc2/

Repository: cherry-embedded/CherryUSB

Length of output: 2750


🏁 Script executed:

#!/bin/bash
# Search for CONFIG_USBDEV_EP_NUM definition and usage
rg -n 'CONFIG_USBDEV_EP_NUM' --type=c --type=h

# Search for all definitions of USBD_BASE and USBFS_BASE
rg -n 'define\s+(USBD_BASE|USBFS_BASE)' --type=c --type=h

# Search for 0x50000000 to see where the magic literal appears
rg -n '0x50000000' --type=c --type=h

# Check GD32-related header includes
rg -n 'include.*gd32' --type=c --type=h -i | head -20

Repository: cherry-embedded/CherryUSB

Length of output: 5664


🏁 Script executed:

#!/bin/bash
# Search for GD32-related includes or USB definitions
fd -type f -name "*.h" | xargs grep -l "GD32\|gd32" 2>/dev/null | head -20

# Search for USB base defines in the codebase
rg -n 'define.*BASE.*0x50000000' --type=h --type=c

# Check what's in the GD files - look for the glue file itself and related headers
cat -n port/dwc2/usb_glue_gd_arm.c | head -75

# Look for includes in usb_glue_gd_arm.c
rg -n '#include' port/dwc2/usb_glue_gd_arm.c

Repository: cherry-embedded/CherryUSB

Length of output: 3084


🏁 Script executed:

#!/bin/bash
# Find board_config.h references
rg -l 'board_config.h' --type=c --type=h | head -10

# Search for USBFS or USB related defines in GD32 context
rg -n 'USBFS\|USBD' --type=h | grep -i gd32 | head -20

# Look for any GD32 CMakeLists or config files
fd -name 'CMakeLists.txt' -o -name '*.cmake' | xargs grep -l 'gd32\|GD32' 2>/dev/null

# Check examples directory structure for GD32
find . -type d -name '*gd*' 2>/dev/null | head -10

# Search for where USBFS_IRQn is defined (used in usb_glue_gd_arm.c)
rg -n 'USBFS_IRQn' --type=c --type=h

Repository: cherry-embedded/CherryUSB

Length of output: 493


🏁 Script executed:

#!/bin/bash
# Search for any GD32-related header files or documentation
find . -type f \( -name "*.h" -o -name "*.md" \) -exec grep -l "GD32\|0x50000000" {} \; | head -20

# Look at includes in the glue file more carefully
grep -E '#include|USBFS' port/dwc2/usb_glue_gd_arm.c

# Search for any constants that might map to 0x50000000
rg '0x50000000' -B2 -A2 --type=c --type=h

# Check the dwc2_override_hw_params function calls
rg -A5 -B5 'dwc2_override_hw_params' --type=c | head -40

Repository: cherry-embedded/CherryUSB

Length of output: 3329


Fix the off-by-one error in num_dev_ep and replace the magic literal with a platform constant.

Two critical issues require correction:

  1. Off-by-one on num_dev_ep. The core loops at usb_dc_dwc2.c:530, 602, 1156 iterate as i < (num_dev_ep + 1), confirming num_dev_ep is the highest zero-based EP index, not the count. With CONFIG_USBDEV_EP_NUM = 4 (EPs 0–3), num_dev_ep must be 3, not 4. Setting it to 4 causes 5 iterations, accessing device_tx_fifo_size[4] (which is 0), violating FIFO semantics. Use hw->num_dev_ep = CONFIG_USBDEV_EP_NUM - 1;.

  2. Magic literal 0x50000000UL. Replace with a GD32 platform constant (e.g., from the GD32 CMSIS header included via board_config.h). A magic literal drifts silently if the platform header changes and blocks future variant support (see the TODO comment).

🛠️ Proposed fix
 void dwc2_override_hw_params(uint32_t reg_base, struct dwc2_hw_params *hw) {
-    /* HWCFG2 reads 0, this is unknown why */
-    if(reg_base == 0x50000000UL) {
-        hw->num_dev_ep = 4;
-    }
+    /* HWCFG2 reads 0 on GD32C103/E103; supply known values manually. */
+    if (reg_base == USBFS_BASE) {
+        hw->num_dev_ep = CONFIG_USBDEV_EP_NUM - 1; /* highest EP index */
+    }
     /* TODO: For other GD32, potentially will need to update like this as well*/
 }

Note: Verify that USBFS_BASE is defined in your GD32 CMSIS headers (included via board_config.h). If unavailable, define a local constant for this variant.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@port/dwc2/usb_glue_gd_arm.c` around lines 67 - 73, In dwc2_override_hw_params
update the hardcoded num_dev_ep and magic address: set hw->num_dev_ep to
CONFIG_USBDEV_EP_NUM - 1 (since num_dev_ep is the highest zero-based EP index
used by the core loops) instead of 4, and replace the literal 0x50000000UL with
the platform constant (e.g., USBFS_BASE from your GD32 CMSIS headers included
via board_config.h); if USBFS_BASE may be missing on some variants, add a local
conditional fallback define for that platform constant and use it in the
reg_base comparison.

#endif
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

void usbd_dwc2_delay_ms(uint8_t ms)
{
uint32_t count = SystemCoreClock / 1000 * ms;
while (count--) {
__asm volatile("nop");
}
}
Comment on lines +75 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all callers of usbd_dwc2_delay_ms and the duration values they pass,
# to gauge timing sensitivity.
rg -nP '\busbd_dwc2_delay_ms\s*\(' -C1

Repository: cherry-embedded/CherryUSB

Length of output: 1818


🏁 Script executed:

#!/bin/bash
# Check implementations across multiple glue files to see if pattern is consistent
echo "=== GD ARM implementation ==="
sed -n '72,78p' port/dwc2/usb_glue_gd_arm.c

echo -e "\n=== GD implementation ==="
sed -n '44,50p' port/dwc2/usb_glue_gd.c

echo -e "\n=== ST implementation ==="
sed -n '717,723p' port/dwc2/usb_glue_st.c

echo -e "\n=== HC implementation ==="
sed -n '297,303p' port/dwc2/usb_glue_hc.c

Repository: cherry-embedded/CherryUSB

Length of output: 776


🏁 Script executed:

#!/bin/bash
# Search for broader context of the delay calls and related functions
echo "=== Context around delay calls in usb_dc_dwc2.c ==="
sed -n '125,145p' port/dwc2/usb_dc_dwc2.c

echo -e "\n=== Context around second delay call ==="
sed -n '660,670p' port/dwc2/usb_dc_dwc2.c

echo -e "\n=== Search for dwc2_set_mode and usbd_set_remote_wakeup mentioned in review ==="
rg -n 'dwc2_set_mode|usbd_set_remote_wakeup' port/dwc2/ -A2 -B2 | head -50

Repository: cherry-embedded/CherryUSB

Length of output: 2448


🏁 Script executed:

#!/bin/bash
# Check if there's SystemCoreClock definition or any other delay implementations
echo "=== Looking for other delay timing approaches in the codebase ==="
rg -n 'SysTick|DWT|SystemTick' port/dwc2/ | head -20
echo ""
echo "=== Checking if there are calibration constants or timing configs ==="
rg -n 'calibrat|timing|cycle' port/dwc2/ -i | head -20

Repository: cherry-embedded/CherryUSB

Length of output: 345


Loop-based delay is subject to significant cycle overhead.

The implementation assumes each loop iteration takes exactly one cycle, but the actual loop body (count--, compare, branch, plus volatile nop) typically consumes 3+ cycles on Cortex-M. This results in an actual delay roughly 3× longer than the calculation intends. For the current 10 ms calls (in dwc2_set_mode and usbd_set_remote_wakeup), this stretches the delay to ~30 ms or more, which is acceptable for USB mode switching but still inaccurate and fragile.

Recommend either:

  1. Use a SysTick- or DWT-cycle-counter-based delay for accurate timing.
  2. Calibrate the loop and divide the count, e.g. count = SystemCoreClock / 1000 / 4 * ms after measurement on the target.

Additionally, ms parameter is uint8_t, limiting the API to 255 ms. Consider widening to uint32_t for consistency with typical delay signatures.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@port/dwc2/usb_glue_gd_arm.c` around lines 72 - 78, The busy-wait in
usbd_dwc2_delay_ms assumes one cycle per iteration and is inaccurate and
fragile; change the prototype to accept uint32_t ms, and replace the loop with
one of two options: (A) use a hardware timer (preferably DWT cycle counter or
SysTick) to compute exact delay based on SystemCoreClock and busy-wait on the
cycle count for precise timing, or (B) if hardware timers are unavailable,
calibrate the loop by measuring cycles/iteration on the target and divide the
computed count (e.g., count = SystemCoreClock / 1000 / N * ms) so the loop
matches real cycles; update all callers (e.g., dwc2_set_mode and
usbd_set_remote_wakeup) to pass uint32_t and verify delays remain ~10 ms.


uint32_t usbd_dwc2_get_system_clock(void)
{
return SystemCoreClock;
}