Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ITM: exhaustively check feature support during configure; improve standard correctness, documentation #383

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

tmplt
Copy link
Contributor

@tmplt tmplt commented Jan 4, 2022

As discussed in #382, this PR checks that the requested features are ensured before continuing in ITM::configure. Because some register fields in ITM_TCR are RAZ/WI and RAZ or RAO (and no dedicated RO flags exists that we can otherwise check) there are cases when we end up with partially applied configurations. These cases are documented on the relevant errors.

No ITM::configure_unchecked in this PR because of the code duplication it would require. But I do not see a common use-case for configuring the ITM outside of an init function which (in my current case at least) allows for a few more registers reads.

This PR also ensures that the ITM is unlocked (if a software lock is implemented) and disabled before modifying any registers. When the configuration has been correctly applied, the ITM is re-locked, as per CoreSight recommendation. The ITM is not re-locked if the configuration fails to apply.

@tmplt tmplt requested a review from a team as a code owner January 4, 2022 12:54
@rust-highfive
Copy link

r? @thalesfragoso

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Jan 4, 2022
@tmplt tmplt force-pushed the ref/itm-conf-verify branch from 34ff684 to c77e175 Compare January 4, 2022 13:22
@tmplt tmplt changed the title ITM: check feature support during configuration ITM: check feature support during configuration, add busy flag, docs improvement Jan 4, 2022
@tmplt
Copy link
Contributor Author

tmplt commented Jan 4, 2022

dc46425 can be fixup'd into 622a9b7.

src/peripheral/itm.rs Outdated Show resolved Hide resolved
src/peripheral/itm.rs Outdated Show resolved Hide resolved
@tmplt tmplt force-pushed the ref/itm-conf-verify branch from dc46425 to bb7319b Compare January 5, 2022 19:34
@tmplt
Copy link
Contributor Author

tmplt commented Jan 5, 2022

Wait with eventual merge for me to test these checks. It should be done by next week.

@tmplt
Copy link
Contributor Author

tmplt commented Jan 13, 2022

#392 must be handled in this PR.

@tmplt tmplt changed the title ITM: check feature support during configuration, add busy flag, docs improvement ITM: exhaustively check feature support during configure; improve standard correctness, documentation Jan 13, 2022
@tmplt tmplt force-pushed the ref/itm-conf-verify branch from 42c950d to 4e1230e Compare January 13, 2022 16:56
src/peripheral/itm.rs Outdated Show resolved Hide resolved
src/peripheral/itm.rs Outdated Show resolved Hide resolved
src/peripheral/itm.rs Outdated Show resolved Hide resolved
@tmplt
Copy link
Contributor Author

tmplt commented Jan 14, 2022

Only TraceBusID remains.

@tmplt
Copy link
Contributor Author

tmplt commented Jan 14, 2022

ARM DDI 0403E.d, p. C1-716:

If multi-source trace is in use, the debugger must write a non-zero value to this field.

C-718:

If a system supports multiple trace streams, the debugger must write a nonzero value to the ITM_TCR.TraceBusID field.
For information about permitted values for this field in a CoreSight-compliant implementation, see the ARM IHI 0029E
[...] An example of a system with multiple trace streams is an ARMv7-M core with ETM. In this case, the debugger must program the ETM TraceBusID register with a different nonzero identifier for the ETM trace stream.

ARM IHI 0029E does not contain the term "TraceBusID".


It is not very clear whether non-zero should be written if multi-source trace is wanted, or if the target implements it. If the latter, we'll need to write non-zero even if we only want a single trace source.

@tmplt tmplt force-pushed the ref/itm-conf-verify branch from aef25a4 to a9a6f55 Compare January 14, 2022 14:11
@tmplt
Copy link
Contributor Author

tmplt commented Jan 14, 2022

This refactored implementation of ITM::configure works as expected on the atsame51n. I've sent an email to ARM in hopes of some clarification on TraceBusID; eventual fixes for which can be postponed to another PR.

I consider this PR ready for review.

src/peripheral/dcb.rs Outdated Show resolved Hide resolved
src/peripheral/itm.rs Show resolved Hide resolved
src/peripheral/itm.rs Show resolved Hide resolved
@tmplt
Copy link
Contributor Author

tmplt commented Jan 15, 2022

lock and unlock has been streamlined a bit and in the process has_software_lock and locked have been hidden. lock and unlock could potentially be hidden later when #393 and similar close so that configure is exhaustive.

TDHolmes
TDHolmes previously approved these changes Jan 15, 2022
Copy link
Contributor

@TDHolmes TDHolmes left a comment

Choose a reason for hiding this comment

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

LGTM

At least one more error enum will be added in the future. See rust-embedded#393.
@tmplt
Copy link
Contributor Author

tmplt commented Jan 19, 2022

Latest changes work as expected on hardware. Also tested on the atsame51n.

@tmplt
Copy link
Contributor Author

tmplt commented Jan 24, 2022

On TraceBusID: after a brief back-and-forth with ARM it seems to be valid to write non-zero. If only ITM is supported on the system, TraceBusID is potentially RAZ (in ARMv8-M, at least). Some rules do apply when multi-source trace is in use, but that's for another day.

@tmplt
Copy link
Contributor Author

tmplt commented Mar 9, 2022

This PR is ready for final review, @thalesfragoso.

@tmplt
Copy link
Contributor Author

tmplt commented May 18, 2022

The re-locking sequence currently incorrectly always assumes that a software lock is implemented. Amendmend will follow.

@korken89
Copy link
Contributor

Hi all,

What happened with this PR? Looking over it, it seems ready to go.

Ping @rust-embedded/cortex-m

Copy link
Contributor

@thejpster thejpster left a comment

Choose a reason for hiding this comment

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

Looks ok to me, but I'm not an ITM expert

@tmplt
Copy link
Contributor Author

tmplt commented Apr 22, 2024

IIRC

The re-locking sequence currently incorrectly always assumes that a software lock is implemented

is still relevant, and should be fixed before merge.

I do not have access to relevant hardware, and my knowledge of the area has faded somewhat. Is anyone else interested in taking up the torch?

@korken89
Copy link
Contributor

@tmplt I could probably give it a look. What HW is needed? I can get some ordered for testing.

@tmplt
Copy link
Contributor Author

tmplt commented Apr 28, 2024

I've tested the code on SAME51 and SAMV71 boards. IIRC one of these lack the software lock, resulting in a busy wait that never exits. I don't think the ITM coverage is exhaustively detailed in the data sheets, and so must be detected at run-time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants