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.

Sign up for GitHub

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

Jump to bottom

Fix: extract method for SWD parity calculation #1700

Merged
merged 5 commits into from
Dec 31, 2023
Merged

Fix: extract method for SWD parity calculation #1700

merged 5 commits into from
Dec 31, 2023

Conversation

ALTracer
Copy link
Contributor

@ALTracer ALTracer commented Dec 17, 2023 •
edited
Loading

Detailed description

This PR is aimed at two problems:

  • Problem 1: swdptap of firmware references both popcount and parity builtins to do the same task (of calculating SWD parity), and __popcountsi2(): 40 bytes is more expensive. Solution 1: create a shim/dispatcher in maths_utils, name it swd_odd_parity and call that from swdptap (the gpio-bitbanging one). This leads to cleaner code at that callsite, and less flash space occupied
  • Problem 2: *_swd.c hosted adapter driver backends also want to calculate parity, and also do that via both popcount and parity builtins. MSVC does not provide these builtins, only the __popcount for x86-64 optional instruction. Solution 2: incorporate this _MSC-specific builtin as well into the shim, and tweak all said hosted parity calculations to call the shim.

This PR is triggered from the discussion of #1688 two weeks ago.
I decided to name the function swd_odd_parity() for where it is going to be used. If future JTAG-PDI code also would want to use it (and attribute alias does not cut it), the reviewer/implementer of that can suggest a better name and I'll mass-rename the touched callsites (with a force-push).
Notice that I drop maths_utils.h from general.h and include it where needed by this PR. The point on making the shim static inline in the header is a valid discussion topic.

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

May help fix building BMDA under MSVC with HOSTED_BMP_ONLY=0 (caveat libusb/hidapi).

@dragonmux dragonmux added this to the v2.0 release milestone Dec 17, 2023
@dragonmux dragonmux added the Enhancement General project improvement label Dec 17, 2023
dragonmux
dragonmux requested changes Dec 17, 2023
View reviewed changes
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.

It's definitely looking good - nice job cleaning things up and rationalising them with this change!

We've spotted a couple of functional issues in review, noted below, and have a couple of name suggestions to pick from for what to name this new function, but we're happy once those are addressed to get this merged.

It's definitely a shame we don't have a nice way of building a sim suite or test suite for the maths functions so we can have in-repo proofs of their correctness, but that's an improvement to strive for in a future PR.

src/platforms/hosted/ftdi_swd.c Outdated Show resolved Hide resolved
src/platforms/common/swdptap.c Show resolved Hide resolved
src/maths_utils.c Outdated Show resolved Hide resolved
src/include/maths_utils.h Outdated Show resolved Hide resolved
src/maths_utils.c Outdated Show resolved Hide resolved
@ALTracer
Copy link
Contributor Author

Requesting next round of review.
Fixup squashing order: keep first 5 commits, incorporate last 4 commits into them: 7-8 into 1st, 6/9 into 4-5. On the other hand, I can replay this manually to make it clean.

dragonmux
dragonmux reviewed Dec 29, 2023
View reviewed changes
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 all looks good to us now - the one review note here is an optional change. Please replay the history to make it clean and we'll get this merged.

src/platforms/hosted/jlink_swd.c Outdated Show resolved Hide resolved
@ALTracer ALTracer force-pushed the fix/parity-builtins branch 2 times, most recently from 344c1e7 to e5ce5ca Compare December 31, 2023 00:06
@ALTracer
general: Decouple maths_utils
c5c91e2 
* The only two users are cortexm_dwt_mask() and stlink_v2_set_frequency().
* This leads to less ccache invalidation when maths_utils.h changes (e.g. odd_parity)
@ALTracer
maths_utils: Factor out calculate_odd_parity() with builtins
4e42c24 
* swdptap of firmware and "adapter"_swd of `hosted` all used parity/popcount
* Extracting this also allows using MSVC with adapter backends
* Use `uint8_t` to correspond to `hosted` libusb packet buffers
@ALTracer
swdptap: Replace popcount with parity
fe3405f 
* BMF on Cortex-M3 relies on libgcc2 software builtin impls,
  not the x86-64-v2 popcnt insn, so adjust accordingly (parity is cheaper)
* More type- and const-correctness (bmp_gpio_get/set)
@ALTracer
swdptap: Abstract away usage of parity builtin
5f32305 
* This still resolves to `__builtin_parity()` for GCC targeting Cortex-M
@ALTracer
Copy link
Contributor Author

Merged fixups, propagated the rename, build-tested intermediate commits with a custom pre-commit extended hook, and rebased to latest main.
Think that's ready for final live testing for SWD parity regressions and any remaining review that the individual commits adhere to expectations. Please try it on a Tigard or something else FTDI.
There's one last __builtin_ctz remaining in ftdi.c:ftdi_swd_init(), and I have some ideas on how to go around it, but maybe not in this PR -- and I'd like MSVC build logs w.r.t. that.

@dragonmux
Copy link
Member

Most excellent! Regarding MSVC, once the new Meson support lands it should become considerably easier to generate full BMDA builds under MSVC and get logs from them.

dragonmux
dragonmux previously approved these changes Dec 31, 2023
View reviewed changes
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, approved pending testing with our Tigard. We'll merge (assuming it all works properly, but we believe it should) once that's been tested.

@ALTracer
hosted/*_swd: Unify all parity calculations in SWD mode adapter code
d7455f5 
* Always use a parity function, not `popcount & 1`
* Hide most `__builtin` usage (from entities like MSVC)
* `ftdi_swd_seq_in_parity_mpsse()`: break up the bitmask calculation to enforce uint32_t intermediate
* `jlink_swd_seq_in_parity()`: cosmetic: fix a typo in comment
@dragonmux
Copy link
Member

We'll get this merged now it's been tested and the one bug found and fixed, good work! 🙂

@dragonmux dragonmux merged commit d7767d4 into blackmagic-debug:main Dec 31, 2023
6 checks passed
@ALTracer ALTracer deleted the fix/parity-builtins branch April 26, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dragonmux dragonmux dragonmux approved these changes

Assignees
No one assigned
Labels
Enhancement General project improvement
Projects
None yet
Milestone
v2.0 release
Development

Successfully merging this pull request may close these issues.
2 participants
@ALTracer @dragonmux