update hpmicro port files #274
Conversation
Signed-off-by: Zhihong Chen <[email protected]>
Signed-off-by: Zhihong Chen <[email protected]>
WalkthroughThe changes in this pull request enhance USB functionality across three files. In Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant USB_Device
participant USB_Host_Controller
User->>USB_Device: Connect
USB_Device->>USB_Host_Controller: Initialize
USB_Host_Controller->>USB_Device: Set Configuration
USB_Device->>USB_Host_Controller: Register Video Descriptor
USB_Host_Controller->>USB_Device: Start Video Streaming
USB_Device->>User: Stream Video Data
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (2)
port/ehci/usb_glue_hpm.c (1)
94-105: LGTM! Consider adding error handlingThe ISR declarations and implementations look good. The code properly handles both USB0 and USB1 interrupts with correct bus ID mapping.
Consider adding error logging in the ISRs to catch potential invalid bus IDs:
void isr_usbh0(void) { + if (_hcd_busid[0] >= CONFIG_USBHOST_MAX_BUS) { + // Log error or handle invalid bus ID + return; + } USBH_IRQHandler(_hcd_busid[0]); }port/hpm/usb_dc_hpm.c (1)
Line range hint
315-359: Consider enhancing error handling in the interrupt handler.The interrupt handler could benefit from more detailed error reporting and recovery mechanisms:
- The error case only logs "usbd intr error" without specific details
- Transfer errors could include more diagnostic information
Consider enhancing the error handling:
if (int_status & intr_error) { - USB_LOG_ERR("usbd intr error!\r\n"); + USB_LOG_ERR("USB error: status=0x%08x\r\n", usb_device_get_error_status(handle)); + usb_device_clear_error_status(handle); }if (p_qtd->halted || p_qtd->xact_err || p_qtd->buffer_err) { - USB_LOG_ERR("usbd transfer error!\r\n"); + USB_LOG_ERR("Transfer error: ep=%02x halt=%d xact=%d buf=%d\r\n", + ep_idx, p_qtd->halted, p_qtd->xact_err, p_qtd->buffer_err); ep_cb_req = false; break; }🧰 Tools
🪛 cppcheck
[error] 363-363: There is an unknown macro here somewhere. Configuration is required. If SDK_DECLARE_EXT_ISR_M is a macro then please configure it.
(unknownMacro)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
demo/video_static_h264_template.c(0 hunks)port/ehci/usb_glue_hpm.c(3 hunks)port/hpm/usb_dc_hpm.c(1 hunks)
💤 Files with no reviewable changes (1)
- demo/video_static_h264_template.c
🧰 Additional context used
🪛 cppcheck
port/hpm/usb_dc_hpm.c
[error] 363-363: There is an unknown macro here somewhere. Configuration is required. If SDK_DECLARE_EXT_ISR_M is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (4)
port/ehci/usb_glue_hpm.c (2)
61-61: Document the purpose of the new boolean parameter
The usb_phy_init function now takes an additional boolean parameter set to true, but its purpose is unclear. This change could affect USB hardware initialization.
Please:
- Document the purpose of this parameter
- Verify if this change affects both host and device modes
- Consider adding an enum instead of a boolean for better code clarity if there are multiple initialization modes
#!/bin/bash
# Search for other usb_phy_init calls to ensure consistent usage
ast-grep --pattern 'usb_phy_init($_, $_)'38-41: Verify implications of forcing full-speed mode
The addition of CONFIG_USB_HOST_FORCE_FULL_SPEED to force USB full-speed mode could impact device compatibility and performance. While the implementation is correct, this change needs careful consideration.
Consider:
- Adding documentation explaining when to use this option
- Verifying that dependent devices support full-speed mode
- Adding a warning log when this mode is enabled
✅ Verification successful
Based on the search results, I can now provide a final response:
Force full-speed mode option is correctly implemented with appropriate hardware support
The implementation of CONFIG_USB_HOST_FORCE_FULL_SPEED is valid and well-supported by the hardware. The codebase shows:
- The HPM EHCI controller has proper hardware support via
USB_PORTSC1_PFSC_MASKregister bit - The speed forcing is consistent with other USB controllers in the codebase that support similar functionality
- The implementation follows standard USB speed negotiation patterns seen across other ports
The code change is safe to use when full-speed mode is specifically required, though it should be noted that this will prevent high-speed operation even if both host and device support it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any device-specific speed requirements or dependencies
rg -l "USB_SPEED_(HIGH|FULL)" --type c
Length of output: 43
Script:
#!/bin/bash
# Search for USB speed related constants and configurations
rg -i "speed.*usb|usb.*speed" --type c -A 2
# Look for USB related configuration options
fd -e Kconfig -X cat
Length of output: 60444
port/hpm/usb_dc_hpm.c (2)
Line range hint 89-100: LGTM! Improved remote wakeup implementation.
The removal of the busy-wait loop and addition of suspend state verification makes the implementation more robust and non-blocking.
🧰 Tools
🪛 cppcheck
[error] 363-363: There is an unknown macro here somewhere. Configuration is required. If SDK_DECLARE_EXT_ISR_M is a macro then please configure it.
(unknownMacro)
363-375: Verify interrupt handler registration with the SDK.
The ISR declarations look correct, but please ensure:
- The SDK_DECLARE_EXT_ISR_M macro is properly defined in the SDK headers
- The interrupt priorities are appropriate for USB handling
✅ Verification successful
Let me gather more information about the SDK macro definition and interrupt handling setup.
Let me check for any USB initialization code in the HPM port to see how the interrupts are configured.
Based on the gathered information, I can now provide the final response:
ISR declarations are correctly implemented and integrated with the HPM SDK
The implementation is verified to be correct because:
- The SDK_DECLARE_EXT_ISR_M macro is consistently used across both USB device (usb_dc_hpm.c) and host (usb_glue_hpm.c) implementations
- Interrupt initialization is properly handled in
usb_dc_init()with:- Correct IRQ number assignment based on USB controller (
IRQn_USB0/1) - Proper interrupt mask configuration
- Explicit interrupt enabling via
intc_m_enable_irq()
- Correct IRQ number assignment based on USB controller (
- USB1 support is correctly guarded with
HPM_USB1_BASEconditional compilation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SDK interrupt handler macro definition and usage
# Check for SDK interrupt handler macro definition
rg -l "SDK_DECLARE_EXT_ISR_M" --type c --type h
# Check for other USB ISR declarations to ensure consistency
rg "SDK_DECLARE.*IRQn_USB[01]" --type c --type h
Length of output: 415
Script:
#!/bin/bash
# Check for SDK macro definition in SDK headers
fd -e h -e c | xargs rg -A 5 "SDK_DECLARE_EXT_ISR"
# Check USB interrupt priority setup
rg -B 5 "IRQn_USB[01].*priority" --type c --type h
# Look for related interrupt configuration
rg "NVIC.*USB" --type c --type h
Length of output: 1453
Script:
#!/bin/bash
# Look for USB initialization and interrupt setup in HPM files
rg -B 10 -A 10 "usb.*init|dcd.*init" port/hpm/
# Check for interrupt priority definitions
rg "IRQ.*PRIORITY.*USB" --type c --type h
# Look for any interrupt configuration in the HPM port
rg "intc.*config|interrupt.*config" port/hpm/
Length of output: 2621
🧰 Tools
🪛 cppcheck
[error] 363-363: There is an unknown macro here somewhere. Configuration is required. If SDK_DECLARE_EXT_ISR_M is a macro then please configure it.
(unknownMacro)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation