[update] port: hpmicro: update usb_dc_hpm.c#316
[update] port: hpmicro: update usb_dc_hpm.c#316sakumisu merged 1 commit intocherry-embedded:masterfrom
Conversation
- update usb_dc_hpm.c Signed-off-by: Zhihong Chen <zhihong.chen@hpmicro.com>
WalkthroughThe update refactors internal logic in the USB device controller driver for HPM platforms. It standardizes interrupt mask usage, consolidates DCD data storage into a 2D array, simplifies initialization logic, and improves endpoint transfer completion by iterating over QTD chains and marking them as not in use. No public interfaces are changed. Changes
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
port/hpm/usb_dc_hpm.c (2)
311-314:⚠️ Potential issueTypo breaks the build:
USB_SOS_DCD_MAX_QHD_COUNT→USB_SOC_DCD_MAX_QHD_COUNTThe loop uses an undefined macro (
USB_SOS…), leading to a compile-time error.-for (uint8_t ep_idx = 0; ep_idx < USB_SOS_DCD_MAX_QHD_COUNT; ep_idx++) { +for (uint8_t ep_idx = 0; ep_idx < USB_SOC_DCD_MAX_QHD_COUNT; ep_idx++) {
318-331: 🛠️ Refactor suggestionNull-pointer guard required before dereferencing
attached_qtd
p_qhd->attached_qtdcan legally beNULL(e.g. if SW cleared it after recycle).
Dereferencing it unconditionally triggers undefined behaviour.-dcd_qtd_t *p_qtd = p_qhd->attached_qtd; -if (p_qtd == NULL) { - /* Nothing to process – continue with next endpoint */ - continue; -} +dcd_qtd_t *p_qtd = p_qhd->attached_qtd; +if (p_qtd == NULL) { + /* No queued TDs, skip this endpoint */ + continue; +}Optionally, after the while-loop completes, set
p_qhd->attached_qtd = NULLso a reused QTD doesn’t get processed twice.
🧹 Nitpick comments (1)
port/hpm/usb_dc_hpm.c (1)
47-52: Alignment per-bus is implicitly preserved, but add a static assert
HPM_ALIGN_UP(sizeof(dcd_data_t), USB_SOC_DCD_DATA_RAM_ADDRESS_ALIGNMENT)guarantees that the stride is a multiple of the required alignment, so every element stays aligned.
However, if either macro is ever modified the guarantee becomes non-obvious. A compile-time assert would document and enforce the constraint:+#include <assert.h> +#define STATIC_ASSERT(COND,MSG) typedef char static_assertion_##MSG[(COND)?1:-1] + STATIC_ASSERT( (HPM_ALIGN_UP(sizeof(dcd_data_t), USB_SOC_DCD_DATA_RAM_ADDRESS_ALIGNMENT) % USB_SOC_DCD_DATA_RAM_ADDRESS_ALIGNMENT) == 0, dcd_stride_must_be_multiple_of_alignment);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
port/hpm/usb_dc_hpm.c(8 hunks)
🔇 Additional comments (2)
port/hpm/usb_dc_hpm.c (2)
16-23: Interrupt-mask constants look correct – nice cleanupSwitching to the HW-supplied
USB_USBINTR_*masks removes the manual bit-twiddling and eliminates a class of maintenance errors.
LGTM.
84-88: Re-evaluate droppingintr_nakfrom the default maskThe previous implementation enabled NAK interrupts; the new one omits them.
If upper layers rely on NAK events for flow-control (common for isochronous endpoints), this change may cause silent stalls.Please confirm that:
- Higher layers never expect NAK callbacks, or
- Another code path enables
intr_naklater.If neither is true, simply OR it back in:
-int_mask = (intr_usb | intr_error |intr_port_change | intr_reset | intr_suspend); +int_mask = (intr_usb | intr_error | intr_port_change | intr_reset | + intr_suspend | intr_nak);
| g_hpm_udc[busid].handle->dcd_data = (dcd_data_t *)&_dcd_data[busid][0]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid C-aliasing UB by typing the backing store correctly
_dcd_data is declared as uint8_t [...] and then cast to dcd_data_t*.
This can break strict-aliasing optimisations on some compilers.
-uint8_t _dcd_data[CONFIG_USBDEV_MAX_BUS][HPM_ALIGN_UP(sizeof(dcd_data_t), USB_SOC_DCD_DATA_RAM_ADDRESS_ALIGNMENT)];
+dcd_data_t _dcd_data[CONFIG_USBDEV_MAX_BUS] /* already aligned by attribute */;The attribute can stay unchanged – GCC/Clang apply it to the entire object.
With the array typed correctly the cast at line 69 is no longer required.
2 participants
Summary by CodeRabbit