Skip to content

CMSIS Driver: enhance support for UHS-I in MCI API - #325

Merged
VladimirUmek merged 9 commits into
mainfrom
mci_api_update
Oct 6, 2026
Merged

VladimirUmek merged 9 commits into
mainfrom
mci_api_update

Conversation

@VladimirUmek

Copy link
Copy Markdown
Contributor

No description provided.

@VladimirUmek
VladimirUmek marked this pull request as ready for review October 5, 2026 10:46
@VladimirUmek
VladimirUmek requested a balanced review from Copilot October 5, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The documentation currently presents invalid tuning and voltage-switch sequences that could cause incorrect implementations.

Review effort: Balanced
Findings: 1 Medium severity · 3 Low severity

Open (4)
What changed in this PR

Enhances the MCI API with phased UHS-I voltage switching, tuning, and re-tuning support.

Changes:

  • Adds UHS-I controls, events, results, and capabilities.
  • Updates API metadata and driver templates to version 2.5.
  • Expands MCI documentation and release histories.
File Description
CMSIS/​Driver/​Include/​Driver_MCI.h Defines the MCI 2.5 API additions.
CMSIS/​Driver/​DriverTemplates/​Driver_MCI.c Extends the capability initializer.
CMSIS/​Documentation/​Doxygen/​General/​src/​revision_history.md Records the release-level MCI update.
CMSIS/​Documentation/​Doxygen/​Driver/​src/​history.md Records the Driver API changes.
CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c Documents the new UHS-I workflows.
ARM.CMSIS.pdsc Updates MCI API version metadata.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread CMSIS/Documentation/Doxygen/Driver/src/Driver_MCI.c
Comment thread ARM.CMSIS.pdsc
Comment thread CMSIS/Documentation/Doxygen/Driver/src/Driver_MCI.c Outdated
Comment thread CMSIS/Documentation/Doxygen/Driver/src/history.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The documentation contains an orphaned comment terminator, incorrect tuning workflow examples, and inconsistent release metadata.

Review effort: Balanced
Findings: 1 High severity · 2 Low severity

Open (3)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid restarting tuning search on CONTINUE

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:260

Repeating all three steps invokes ARM_MCI_UHS_TUNING_START on every iteration, but its definition below says it initializes a new sampling-clock search. A CONTINUE result must preserve the current search and repeat only the tuning-block/result steps; otherwise the documented loop continually resets its progress.

This issue also appears on line 898 of the same file.

Comment thread CMSIS/Documentation/Doxygen/Driver/src/Driver_MCI.c Outdated
Comment thread CMSIS/Documentation/Doxygen/General/src/revision_history.md

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The tuning example queries its result before performing the required tuning-block transfer.

Review effort: Balanced
Findings: None

Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Querying tuning result before transfer completes

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:905

This queries the tuning result immediately after START, before any tuning-block transfer has occurred, contradicting the required sequence documented above (start → send tuning block → inspect result). The example can therefore lead consumers to sample an undefined/stale result; add the appropriate tuning-block transfer and completion wait before this call, and repeat it when CONTINUE is returned.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The hardware-facing public API and protocol sequencing warrant final human validation.

Review effort: Balanced
Findings: None

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

New examples can hang on asynchronous failures, reuse stale events, and contain invalid C.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

Comment thread CMSIS/Documentation/Doxygen/Driver/src/Driver_MCI.c Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The tuning example reuses a stale completion event and contains an invalid value return from a void function.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Clear transfer-complete event before each tuning command

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:362

MCI_Events is OR-ed in the callback but never cleared. When tuning returns ARM_MCI_UHS_TUNING_CONTINUE, the transfer-complete bit from the first block remains set, so every later iteration skips this wait and queries the result before its block completes. Clear the bit before each tuning command.

Medium severity Invalid value returned from void function

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:369

This example declares mci_tune_sampling_clock as returning void, so returning 0U violates its function contract and can make copied sample code fail compilation. Use a value-less return, as on the earlier failure path.

Low severity Fix typo in example comment

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:372

Correct the typo in the example comment.

@VladimirUmek
VladimirUmek requested a balanced review from Copilot October 6, 2026 08:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The new examples can deadlock or advance prematurely because they mishandle asynchronous events and ignore PREPARE failure.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Medium severity Abort CMD11 when PREPARE fails

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:228

The PREPARE result is ignored, so CMD11 is still sent when the host could not enter voltage-switch state (for example, when this control is unsupported). Check the return value and abort before issuing CMD11 on failure.

This issue also appears on line 359 of the same file.

Low severity Remove duplicated “after” in phase description

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:984

Remove the duplicated “after” so the phase description is unambiguous.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The examples can consume stale event flags and continue before the current command or transfer completes.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Medium severity Clear stale COMMAND_COMPLETE before starting CMD11

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:233

MCI_Events accumulates callback bits, so COMMAND_COMPLETE can still be set by an earlier card-initialization command. In that case this wait returns immediately and APPLY may run while CMD11 is still active. Clear the completion bit immediately before starting CMD11, as the existing MCI demo does before SendCommand.

Medium severity Clear stale TRANSFER_COMPLETE before each tuning command

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:361

TRANSFER_COMPLETE is never consumed even though this loop can issue up to 40 tuning commands. After the first CONTINUE result, the next iteration sees the previous completion bit and queries ARM_MCI_UHS_TUNING_RESULT before the new tuning block has finished. Clear the bit before each SendCommand.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The examples can hang on failure events, and tuning completion semantics remain ambiguous.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Define terminal event for invalid tuning samples

CMSIS/​Documentation/​Doxygen/​Driver/​src/​Driver_MCI.c:333

This rule suppresses ARM_MCI_EVENT_TRANSFER_ERROR for an invalid tuning sample, but the API does not specify which terminal event replaces it; elsewhere TRANSFER_COMPLETE is defined only for successful transfers, while the new example waits exclusively for that event. Define the required terminal event for a CRC-failed tuning block (for example, TRANSFER_COMPLETE with validity reported by ARM_MCI_UHS_TUNING_RESULT) so implementations cannot leave callers waiting indefinitely.

This issue also appears on line 370 of the same file.

Comment thread CMSIS/Documentation/Doxygen/Driver/src/Driver_MCI.c
@VladimirUmek
VladimirUmek merged commit 81b5277 into main Oct 6, 2026
2 checks passed
@VladimirUmek
VladimirUmek deleted the mci_api_update branch October 6, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants