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

Fix: ADIv5 DP init #1544

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Conversation

perigoso
Copy link
Contributor

@perigoso perigoso commented Jul 19, 2023

Detailed description

This PR does a much needed cleanup in adiv5_dp_init, related to the TRY_CATCH blocks

TRY_CATCH brings clobbering errors, forcing us to volatile certain variables and thus blocking certain optimizations

This contains the problematic code and unblocks optimizations, bringing a small code size saving

Tested with BMDA (J-Link V8) and native on a RP2040 target

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Closing issues

This try execpt block was watching for a timeout, but there are at the
moment no timeout exceptions thrown in the codebase

The original exeption present in the adiv5_dp_read stack this code was
likely handling was removed in commit:
0c63903
where the abort to recover from bad access implemented by the
exeption case of this block was also implemented at a lower level
this means the exception block code was never run, and the re-read on
timeout probably stoped happening since then

Currently a protocol recovery mechanism is implemented which should
provide the functionality this code was originally trying to employ
@dragonmux dragonmux changed the title Fix/adiv5 dp init Fix: ADIv5 DP init Jul 19, 2023
@dragonmux dragonmux added this to the v1.10 milestone Jul 19, 2023
@dragonmux dragonmux added Bug Confirmed bug Enhancement General project improvement labels Jul 19, 2023
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

LGTM, there's only one item we've spotted in review and with that fixed this looks good to merge. Really great job!

src/target/adiv5.c Outdated Show resolved Hide resolved
This sees the introduction of adiv5_read_dpidr, which keeps the
try except funkiness contained and prevents the need for volatile
variables in the scope of more complex functions blocking
possible optimizations
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

LGTM, merging.

@dragonmux dragonmux merged commit 640e853 into blackmagic-debug:main Jul 19, 2023
6 checks passed
@perigoso perigoso deleted the fix/adiv5_dp_init branch July 19, 2023 21:40
Copy link
Contributor

@ALTracer ALTracer left a comment

Choose a reason for hiding this comment

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

Testing this as part of rebasing PR1546 yields a possible regression for me with STM32MP15.

src/target/adiv5.c Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bug Enhancement General project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants