Migrate NXP Kinetis khci to chipidea ci_fs (device + host) + fixes#3763
Open
hathach wants to merge 5 commits into
Open
Migrate NXP Kinetis khci to chipidea ci_fs (device + host) + fixes#3763hathach wants to merge 5 commits into
hathach wants to merge 5 commits into
Conversation
Complete the khci -> chipidea ci_fs migration that was started for device (commit d70403f "host is not yet"): - device: switch kinetis_k/kl/k32l (Makefiles + k32l CMake) to dcd_ci_fs.c - host: add hcd_ci_fs.c (port of hcd_khci.c onto ci_fs_regs_t) and switch all Kinetis families to it; remove src/portable/nxp/khci entirely - enable host examples (device_info, cdc_msc_hid) for mcu:KINETIS_K - README: merge the KL and K32L2 rows into a single "KL, K32L" ci_fs row hcd_ci_fs.c also fixes two pre-existing host bugs found via HIL on frdm_k64f (present in the old hcd_khci.c too): - data toggle was flipped on a NAK in suspend_transfer; a NAK transfers no data so the toggle must be preserved, else the retried bulk packet is silently discarded by the device (MSC CBW/CSW hang). See comment in file. - prepare_packets asserted and dropped a transfer when the single shared BDT was still owned by an in-flight transfer under concurrent activity; now it returns busy and resume_transfer defers/retries on the next SOF. HIL verified on frdm_k64f: device 13/13, host cdc_msc_hid (CDC mount + echo + MSC mount, through a hub). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExGPLP5eU43LR7o6yYLpNi
A multi-packet OUT transfer speculatively arms both even/odd BDTs to avoid NAK. When the host ends the transfer early with a short packet, the sibling BDT was left armed (own=1), desyncing the even/odd ping-pong so the next OUT packet landed at buffer+max_packet_size instead of buffer and the stack read stale data. Disarm the sibling on completion. Fixes device/mtp on Kinetis (GetDeviceInfo command was received into the wrong buffer half -> hang). Pre-existing (MSC only arms single-packet command receives so it never hit the double-buffer path). HIL: frdm_kl25z & frdm_k64f device 13/13. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExGPLP5eU43LR7o6yYLpNi
- board_uart_read was a stub returning 0, so host examples that bridge the UART console to a CDC device (echo test) received nothing. Implement it via an RDRF-interrupt-fed tu_fifo, matching the stm32 family (non-blocking, no RX overrun). board_uart_write is already non-blocking. - implement board_get_unique_id() from the SIM 128-bit UID registers so frdm_k64f/teensy_35 report a real USB serial instead of the fixed default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExGPLP5eU43LR7o6yYLpNi
frdm_k64f as a USB host with a CH9102 CDC (TX-RX loopback) and a Lexar MSC drive behind a hub; flasher = onboard OpenSDA J-Link. host/cdc_msc_hid passes (CDC mount+echo, MSC mount + disk-size check). device_info remains a known device_info/usbh limitation (its synchronous descriptor dump starves a 2nd device's enumeration) and is not ci_fs-specific. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExGPLP5eU43LR7o6yYLpNi
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
Completes the NXP Kinetis USB driver migration from the legacy khci implementation to the shared ChipIdea-FS (ci_fs) register model across device + host, while also addressing two driver correctness issues found via HIL testing.
Changes:
- Switch Kinetis BSP build recipes (Make + CMake) from
src/portable/nxp/khci/*tosrc/portable/chipidea/ci_fs/*and retire the old KHCI DCD implementation. - Add/enable Kinetis host support via
hcd_ci_fs.c, enable additional host examples formcu:KINETIS_K, and extend HIL config with a new FRDM-K64F host test setup. - Fix
dcd_ci_fsOUT ping-pong (even/odd BDT) desync by disarming the sibling BDT on early-terminated OUT transfers.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/hil/tinyusb.json | Adds frdm_k64f host test configuration and attached-device expectations. |
| src/portable/nxp/khci/dcd_khci.c | Removes legacy KHCI device controller driver implementation. |
| src/portable/chipidea/ci_fs/hcd_ci_fs.c | Updates/ports host controller driver to ci_fs_regs_t usage and adds scheduling fixes for busy BDT / toggle handling. |
| src/portable/chipidea/ci_fs/dcd_ci_fs.c | Fixes OUT transfer ping-pong desync by disarming the sibling BDT on completion. |
| README.rst | Updates Kinetis row(s) to reflect ci_fs usage. |
| hw/bsp/kinetis_kl/family.mk | Switches KL build to dcd_ci_fs.c + hcd_ci_fs.c. |
| hw/bsp/kinetis_kl/family.cmake | Switches KL CMake build to hcd_ci_fs.c. |
| hw/bsp/kinetis_k32l/family.mk | Switches K32L build to dcd_ci_fs.c + hcd_ci_fs.c. |
| hw/bsp/kinetis_k32l/family.cmake | Switches K32L CMake build to dcd_ci_fs.c + hcd_ci_fs.c. |
| hw/bsp/kinetis_k/family.mk | Switches K build to dcd_ci_fs.c + hcd_ci_fs.c. |
| hw/bsp/kinetis_k/family.cmake | Switches K CMake build to hcd_ci_fs.c. |
| hw/bsp/kinetis_k/family.c | Improves UART RX handling using an IRQ-fed FIFO and adds SIM-based unique ID support. |
| examples/host/device_info/only.txt | Enables device_info host example for mcu:KINETIS_K. |
| examples/host/cdc_msc_hid/only.txt | Enables cdc_msc_hid host example for mcu:KINETIS_K. |
Comments suppressed due to low confidence (1)
src/portable/chipidea/ci_fs/hcd_ci_fs.c:642
- SOF (SOFTOK) handler clears
_hcd.pendingwholesale (_hcd.pending = 0;). Since_hcd.pendingis a bitmap and this driver now sets bits from multiple paths (NAK retry + shared-BDT busy deferral), clearing all bits will silently drop other pending pipes/transfers. This can stall enumeration or concurrent bulk/interrupt transfers behind a hub.
Clear only the bit you're servicing, and only disable SOFTOK when no pending bits remain.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
|
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| frdm_k64f/board_test | 860 → 1,004 (+144) | — | — | 32 → 44 (+12) | 892 → 1,048 (+156) | +17.5% |
| frdm_k64f/dfu | 13,476 → 13,724 (+248) | — | — | 2,176 → 2,688 (+512) | 15,940 → 16,700 (+760) | +4.8% |
| frdm_k64f/dfu_runtime | 9,964 → 10,200 (+236) | — | — | — | 10,012 → 10,248 (+236) | +2.4% |
| frdm_k64f/hid_generic_inout | 10,804 → 11,040 (+236) | — | — | — | 10,852 → 11,088 (+236) | +2.2% |
| frdm_k64f/hid_multiple_interface | 11,624 → 11,860 (+236) | — | — | — | 11,672 → 11,908 (+236) | +2.0% |
| frdm_k64f/hid_boot_interface | 11,636 → 11,872 (+236) | — | — | — | 11,684 → 11,920 (+236) | +2.0% |
| frdm_k64f/hid_composite | 11,800 → 12,036 (+236) | — | — | — | 11,848 → 12,084 (+236) | +2.0% |
| frdm_k64f/midi_test | 11,780 → 11,984 (+204) | — | — | — | 11,828 → 12,032 (+204) | +1.7% |
| frdm_k64f/hid_composite_freertos | 19,328 → 19,660 (+332) | — | — | — | 19,448 → 19,780 (+332) | +1.7% |
| frdm_k64f/usbtmc | 13,684 → 13,920 (+236) | — | — | — | 13,852 → 14,088 (+236) | +1.7% |
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
hcd_ci_fs shares a single BDT set across all pipes. prepare_packets() speculatively arms the sibling (odd^1) BDT of a multi-packet transfer so it can ping-pong without NAKs. When such a transfer ends early (a short IN packet) or fails, the still-owned sibling was never released, permanently blocking the shared BDT for every other pipe. This deadlocked a 2nd device enumerating behind a hub while another device issued descriptor reads (host/device_info with CDC+MSC): the MSC's control transfers could never acquire the BDT, so it never got Set Address. Release the sibling in process_tokdne()'s completion path, but ONLY for a multi-packet transfer (length > max_packet_size): a single-packet transfer never arms a sibling, so that BDT slot may legitimately belong to another pipe's in-flight transfer and must not be disturbed (doing so unconditionally corrupts concurrent transfers, e.g. the CDC bulk-IN vs MSC enum in host/cdc_msc_hid). Mirrors the equivalent device-side fix in dcd_ci_fs.c; the host needs the multi-packet guard because its BDT set is shared across pipes. Verified on frdm_k64f (HIL): host/device_info now enumerates both CDC+MSC behind a hub, host/cdc_msc_hid still mounts the MSC (no regression). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExGPLP5eU43LR7o6yYLpNi
Hardware-in-the-loop (HIL) Test Reporthfp-iar✅ 52 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
hfp.json✅ 52 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
tinyusb.json✅ 314 passed · ❌ 105 failed · ⚪ 28 skipped · blank not run
|
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.
Summary
Completes the NXP Kinetis khci → chipidea ci_fs driver migration that was started for device only (commit
d70403f1f— "host is not yet"), and fixes two pre-existing driver bugs surfaced by HIL testing.kinetis_k/kl/k32l(Makefiles + k32l CMake) todcd_ci_fs.chcd_ci_fs.c(port ofhcd_khci.conto the sharedci_fs_regs_t) and switch all Kinetis families to it; removesrc/portable/nxp/khci/entirelydevice_info,cdc_msc_hid) formcu:KINETIS_KKL, K32Lci_fs rowBug fixes (pre-existing — present in the old khci driver too)
dcd_ci_fs— device OUT even/odd BDT desync. A multi-packet OUT arms both even/odd BDTs; on a short packet the sibling was left armed, desyncing the ping-pong so the next OUT landed in the wrong buffer half → stack read stale data. Fixesdevice/mtphanging on Kinetis (MSC only arms single-packet command receives, so it never hit this path).hcd_ci_fs— host data toggle flipped on NAK. A NAK transfers no data, so the toggle must be preserved; the retried bulk packet was going out with the wrong DATA0/1 and being silently discarded (MSC CBW/CSW hang). Also:prepare_packetsasserted and dropped a transfer when the single shared BDT was busy under concurrent activity — now it defers and retries on the next SOF.BSP / CI
kinetis_k: implement non-blockingboard_uart_readvia an RDRF-interrupt RX FIFO (matching stm32;board_uart_writealready non-blocking), andboard_get_unique_id()from the SIM 128-bit UID.frdm_k64fhost test (CDC + MSC behind a hub) totest/hil/tinyusb.json.HIL verification (real hardware)
frdm_kl25zandfrdm_k64f— 13/13 (cdc, msc, dfu, throughput, audio/iso, hid, midi, printer, mtp, …).frdm_k64f):host/cdc_msc_hidpasses — CDC mount + line coding + 1024-byte echo, and MSC mount + disk-size check, through a USB hub.Known limitation
host/device_infoenumerates only the first of two devices behind a hub: its continuous synchronous descriptor dump starves the 2nd device's enumeration at the usbh control-transfer layer. This is adevice_info/usbh interaction (not ci_fs —cdc_msc_hidenumerates both fine) and is left as-is.Net: +~90 / −1200 lines (khci retired).
🤖 Generated with Claude Code
https://claude.ai/code/session_01ExGPLP5eU43LR7o6yYLpNi