-
Notifications
You must be signed in to change notification settings - Fork 157
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
base: master
Are you sure you want to change the base?
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
34ff684
to
c77e175
Compare
dc46425
to
bb7319b
Compare
Wait with eventual merge for me to test these checks. It should be done by next week. |
#392 must be handled in this PR. |
configure
; improve standard correctness, documentation
42c950d
to
4e1230e
Compare
Only TraceBusID remains. |
ARM DDI 0403E.d, p. C1-716:
C-718:
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. |
aef25a4
to
a9a6f55
Compare
This refactored implementation of I consider this PR ready for review. |
There functions are internally queried inside lock, unlock.
|
There was a problem hiding this 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.
Latest changes work as expected on hardware. Also tested on the |
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. |
This PR is ready for final review, @thalesfragoso. |
The re-locking sequence currently incorrectly always assumes that a software lock is implemented. Amendmend will follow. |
Hi all, What happened with this PR? Looking over it, it seems ready to go. Ping @rust-embedded/cortex-m |
There was a problem hiding this 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
IIRC
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? |
@tmplt I could probably give it a look. What HW is needed? I can get some ordered for testing. |
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. |
As discussed in #382, this PR checks that the requested features are ensured before continuing in
ITM::configure
. Because some register fields inITM_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 aninit
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.