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

start seperating swd multi drop #1184

Merged
merged 4 commits into from
Jul 28, 2023

Conversation

perigoso
Copy link
Contributor

@perigoso perigoso commented Aug 8, 2022

Detailed description

I initially meant to properly abstract away the multidrop handling and swd scan in general, but left it rather early, this PR doesnt have much going for it, just a place to document and later continue work from

as is the jlink scan (and I believe stlink too, but i have not tested or looked at it) does not handle multi-drop, I was meaning to get rp2040 working on jlink as means to develop and test this work but it is unable to comunicate with its DP, the reason is I would guess the Lack of "selection alert sequence for SW-DP-V2" that is present on the regular adiv5_scan

The jlink probe and api should be able to do it, but it needs to be implemented, ideally as a seq_out method, and the cmds in jlink in general could do with some work

since the jlink usb spec is not that easy to find I will leave it here
RM08001_JLinkUSBProtocol_Rev.7.pdf

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

@dragonmux dragonmux added the Enhancement General project improvement label Aug 8, 2022
@dragonmux dragonmux added this to the v1.10 milestone Sep 20, 2022
@dragonmux
Copy link
Member

It would be useful please if this PR could be rebased on main, especially now that the J-Link implementation implements those low-level SWD functions mentioned as being a blocker for you.

Having proper support for multi-drop and it being refactored like this is of great interest to us

@perigoso
Copy link
Contributor Author

It would be useful please if this PR could be rebased on main, especially now that the J-Link implementation implements those low-level SWD functions mentioned as being a blocker for you.

Having proper support for multi-drop and it being refactored like this is of great interest to us

Thank you for letting me know, this was just the start of the implementation there's work to do still, but I was looking for something to pick up!

@perigoso perigoso force-pushed the fix/swd-multi-drop branch 2 times, most recently from 90f0094 to 716da11 Compare July 20, 2023 02:00
@perigoso perigoso marked this pull request as ready for review July 20, 2023 02:02
@perigoso
Copy link
Contributor Author

perigoso commented Jul 20, 2023

Unsure if ready to merge

The context around this PR has changed quite a bit over the time it sat idle, one of the main objectives (J-Link multidrop) was solved in the meantime.
For now this just provides some refactoring and code separation for multi-drop, with some code size saving in the mix.

Note to self, look at targetsel handling correctness and generalization

@perigoso perigoso force-pushed the fix/swd-multi-drop branch 3 times, most recently from ed7d850 to 1b2d9ed Compare July 20, 2023 22:01
@perigoso
Copy link
Contributor Author

perigoso commented Jul 20, 2023

After taking a new look, this is ready for review @dragonmux it doesn't achieve much worthy of note other than code size (and readability I guess), but, stepping stones...

I started to poke the targetsel handling, but I only got higher not lower flash usage and the code didn't improve significantly, so I'm leaving that for another time.

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.

This is looking great and provides some much needed clean-up and magic number naming - thank you for the contribution!

There are a few items that we've spotted in review that could do with addressing before we merge to try to ensure this is the best possible version of a SWD bus scanner, but with them addressed, we'll be happy to merge this.

src/target/adiv5_swd.c Outdated Show resolved Hide resolved
src/target/adiv5_swd.c Outdated Show resolved Hide resolved
src/target/adiv5_swd.c Show resolved Hide resolved
src/target/adiv5_swd.c Outdated Show resolved Hide resolved
src/target/adiv5_swd.c Show resolved Hide resolved
src/target/adiv5_swd.c Outdated Show resolved Hide resolved
src/target/adiv5_swd.c Outdated Show resolved Hide resolved
src/target/adiv5_swd.c Outdated Show resolved Hide resolved
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, thank you for the contribution! Merging.

We'd run our own tests but we know you're careful to thoroughly test these kinds of thing.

@dragonmux dragonmux merged commit b07475e into blackmagic-debug:main Jul 28, 2023
6 checks passed
@perigoso perigoso deleted the fix/swd-multi-drop branch July 28, 2023 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement General project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants