musb: improve endpoint 0 implementation approach#3626
Open
tzy0002088 wants to merge 1 commit intohathach:masterfrom
Open
musb: improve endpoint 0 implementation approach#3626tzy0002088 wants to merge 1 commit intohathach:masterfrom
tzy0002088 wants to merge 1 commit intohathach:masterfrom
Conversation
tzy0002088
force-pushed
the
musb_update
branch
2 times, most recently
from
8386648 to
23a8a89
Compare
Collaborator
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| msp_exp432e401y/dfu_runtime | 8,836 → 9,036 (+200) | — | — | 1,044 → 1,060 (+16) | 10,612 → 10,828 (+216) | +2.0% |
| msp_exp432e401y/hid_generic_inout | 9,908 → 10,112 (+204) | — | — | 1,248 → 1,264 (+16) | 11,888 → 12,108 (+220) | +1.9% |
| msp_exp432e401y/hid_multiple_interface | 10,692 → 10,896 (+204) | — | — | 1,124 → 1,140 (+16) | 12,548 → 12,768 (+220) | +1.8% |
| msp_exp432e401y/hid_boot_interface | 10,704 → 10,908 (+204) | — | — | 1,124 → 1,140 (+16) | 12,560 → 12,780 (+220) | +1.8% |
| msp_exp432e401y/hid_composite | 10,916 → 11,120 (+204) | — | — | 1,112 → 1,128 (+16) | 12,760 → 12,980 (+220) | +1.7% |
| msp_exp432e401y/midi_test | 11,024 → 11,224 (+200) | — | — | 1,244 → 1,260 (+16) | 13,000 → 13,216 (+216) | +1.7% |
| ek_tm4c123gxl/dfu_runtime | 8,808 → 9,004 (+196) | — | — | 608 → 624 (+16) | 13,576 → 13,788 (+212) | +1.6% |
| msp_exp432e401y/cdc_dual_ports | 11,864 → 12,064 (+200) | — | — | 1,428 → 1,444 (+16) | 14,024 → 14,240 (+216) | +1.5% |
| msp_exp432e401y/printer_to_cdc | 11,996 → 12,196 (+200) | — | — | 1,408 → 1,424 (+16) | 14,128 → 14,344 (+216) | +1.5% |
| msp_exp432e401y/webusb_serial | 12,372 → 12,576 (+204) | — | — | 1,416 → 1,432 (+16) | 14,520 → 14,740 (+220) | +1.5% |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reworks the MUSB device controller’s Endpoint 0 (control) handling to better tolerate merged/level-triggered EP0 interrupt conditions under heavy CPU load, by replacing the prior EP0 state handling with an explicit stage-based EP0 state machine.
Changes:
- Replaces the prior EP0 state tracking with a new
ep0_state_t(SETUP/DATA/STATUS stages) and updatesdcd_data_taccordingly. - Reimplements
edpt0_xfer()andprocess_ep0()around the new EP0 stage machine and addsep0_setup()helper. - Adjusts reset, stall, and set-address flows to use the new EP0 state fields.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+484
to
+489
| if (_dcd.remaining_ctrl) { | ||
| dcd_event_xfer_complete(rhport, | ||
| tu_edpt_addr(0, TUSB_DIR_IN), | ||
| _dcd.pipe0.length - _dcd.pipe0.remaining, | ||
| XFER_RESULT_SUCCESS, true); | ||
| } |
Comment on lines
+471
to
+473
| TU_LOG3(false, "SetupEnd came in a wrong ep0stage %s\n", _dcd.ep0_state); | ||
| } | ||
| csrl = _dcd.ep0_state; |
| if (_dcd.setup_packet.wLength == 0) { | ||
| /* No data stage, must send zlp to host */ | ||
| _dcd.ep0_state = USB_EP0_STAGE_PRE_STATUSIN; | ||
| if (_dcd.setup_packet.bRequest == 5) { |
| ep_csr->csr0l = MUSB_CSRL0_RXRDYC; | ||
| while ((ep_csr->csr0l & MUSB_CSRL0_RXRDY) != 0); | ||
| } else { | ||
| /* OUT stage, recive data from host */ |
Comment on lines
+450
to
+452
| if (csrl & MUSB_CSRL0_DATAEND) { | ||
| return; | ||
| } |
Comment on lines
+371
to
+384
| const unsigned rem = _dcd.remaining_ctrl; | ||
| const unsigned len = TU_MIN(TU_MIN(rem, 64), total_bytes); | ||
| tu_hwfifo_write(&musb_regs->fifo[0], buffer, len, NULL); | ||
|
|
||
| _dcd.pipe0.buf = buffer + len; | ||
| _dcd.pipe0.length = len; | ||
| _dcd.pipe0.remaining = 0; | ||
|
|
||
| _dcd.remaining_ctrl = rem - len; | ||
| if ((len < 64) || (rem == len)) { | ||
| _dcd.ep0_state = USB_EP0_STAGE_STATUSOUT; | ||
| ep_csr->csr0l = MUSB_CSRL0_TXRDY | MUSB_CSRL0_DATAEND; | ||
| } else { | ||
| _dcd.ep0_state = USB_EP0_STAGE_TX; |
| // First event of the STATUS OUT pair — wait for the IRQ to fire complete. | ||
| _dcd.pipe0.state = PIPE0_STATE_STATUS_OUT_PENDING; | ||
| break; | ||
| while (ep_csr->count0 != 8); |
Comment on lines
+499
to
+500
| const unsigned len = TU_MIN(TU_MIN(rem, 64), vld); | ||
| tu_hwfifo_read(&musb_regs->fifo[0], _dcd.pipe0.buf, len, NULL); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All interrupt flags for MUSB Endpoint 0 (EP0) are contained within a single register. Occasionally, multiple EP0 states may be satisfied simultaneously, causing them to be merged into a single interrupt request sent to the CPU. In such cases, the driver must be capable of handling these concurrent conditions.
Due to heavy system load, global interrupts might be disabled elsewhere. Since the MUSB IP utilizes level-triggered interrupts, multiple internal events that should have triggered separate interrupts may become pending. Consequently, the CPU services them as a single interrupt request once global interrupts are re-enabled.
Instead of asserting distinct interrupts for the Data and Status stages in the blue region, the MUSB controller aggregated these pending flags into the following Setup stage interrupt request.
Capture of normal transfer without racing, IRQ (blue trace) triggered after each transfer stage.
When CPU is heavily loaded, IRQs are not served in time, we see only one IRQ fired at the end with multiples flags pending.
The Status stage complete interrupt was delayed into the Setup stage of the subsequent packet, followed by the triggering of the Setup interrupt