follow up to PR3506#3509
Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to PR3506 that performs code cleanup and refactoring in the DWC2 USB driver. The changes improve variable naming consistency and clarify the distinction between PHY capability and actual operating mode.
Changes:
- Renamed function parameter from
highspeed_phytois_hs_phyindwc2_core_init()for clarity - Modified
dfifo_host_init()to accept runtime operating speed instead of PHY capability - Applied consistent indentation to preprocessor directives inside functions (2-space indentation)
- Minor whitespace cleanup and comment improvements
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/portable/synopsys/dwc2/hcd_dwc2.c | Updated variable naming from highspeed_phy to is_hs_phy, changed dfifo_host_init to accept runtime is_highspeed parameter instead of querying PHY capability, applied consistent preprocessor directive indentation |
| src/portable/synopsys/dwc2/dwc2_common.h | Updated function declaration parameter name from highspeed_phy to is_hs_phy |
| src/portable/synopsys/dwc2/dwc2_common.c | Updated function implementation parameter name from highspeed_phy to is_hs_phy |
| src/portable/synopsys/dwc2/dcd_dwc2.c | Updated variable naming from highspeed_phy to is_hs_phy for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eef6f4f4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // disable high speed mode | ||
| dwc2->hcfg |= HCFG_FSLS_ONLY; | ||
| bool is_highspeed; | ||
| if (!TUH_OPT_HIGH_SPEED || rh_init->speed < TUSB_SPEED_HIGH) { |
There was a problem hiding this comment.
This branch derives is_highspeed only from TUH_OPT_HIGH_SPEED and rh_init->speed, but dwc2_core_init() may already have selected a full-speed PHY (is_hs_phy == false) in the same function. In that mismatch case (e.g. no HS PHY available or HS PHY disabled via DWC2 config), dfifo_host_init() is called with is_highspeed=true and allocates HS-sized FIFOs, which can exceed FS FIFO budgets and trigger the internal TU_ASSERT, leaving FIFO registers uninitialized; before this commit, FIFO sizing followed dwc2_core_is_highspeed_phy() and avoided that regression.
Useful? React with 👍 / 👎.
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in size
No changes
|
| // configure fixed-allocated fifo scheme | ||
| dfifo_host_init(rhport); | ||
| // configure a fixed-allocated fifo scheme | ||
| dfifo_host_init(rhport, is_highspeed); |
There was a problem hiding this comment.
Sorry if I don't really know what I'm talking about, but Is it really a good idea to reduce the size of the buffers if the core is able to have bigger buffers?
There is this comment in dfifo_host_init() that implies that a "small" buffer is allocated because of the core limitation:
// - ptx_largest is limited to 256 for FS since most FS core only has 1024 bytes total
But that certainly does not apply if the core is HS-capable.
There was a problem hiding this comment.
hmm, you are right. Maybe it is ok just leave it as before.
| } else { | ||
| const bool has_fs_phy = (ghwcfg2.fs_phy_type != GHWCFG2_FSPHY_NOT_SUPPORTED); | ||
| // false if has fs phy, otherwise true since hs phy is the only available phy | ||
| return !has_fs_phy && has_hs_phy; |
There was a problem hiding this comment.
@HiFiPhile U5 fall into this category with UTMI+ without fs_phy
No additional information provided.