7
31
Fork
You've already forked blackmagic
23

Feature: implementation of jtagtap_cycle() for FTDI backend #2181

Merged
ALTracer merged 4 commits from feature/jtagtap_cycle_ftdi into main 2026年02月02日 06:22:12 +01:00
ALTracer commented 2026年01月28日 19:30:19 +01:00 (Migrated from github.com)
Copy link

Detailed description

  • This is a minor new feature.
  • The problem is BMD logic expects to use jtagtap_cycle() sometimes, but BMDA hasn't always provided it.
  • This PR solves it by providing one moderately optimized implementation for FTDI MPSSE backend.

Tested on FT2232HL against a RISC-V MCU. Based on #2179. See #2180 for tracking other backends.
I would like some explanation, is buffering beneficial in the handlers, or does it exist externally (between two flush calls), then I can discard the 2nd commit.

Your checklist for this pull request

Closing issues

## Detailed description * This is a minor new feature. * The problem is BMD logic expects to use `jtagtap_cycle()` sometimes, but BMDA hasn't always provided it. * This PR solves it by providing one moderately optimized implementation for FTDI MPSSE backend. Tested on FT2232HL against a RISC-V MCU. Based on #2179. See #2180 for tracking other backends. I would like some explanation, is buffering beneficial in the handlers, or does it exist externally (between two flush calls), then I can discard the 2nd commit. ## Your checklist for this pull request * [x] I've read the [Code of Conduct](https://github.com/blackmagic-debug/blackmagic/blob/main/CODE_OF_CONDUCT.md) * [x] I've read the [guidelines for contributing](https://github.com/blackmagic-debug/blackmagic/blob/main/CONTRIBUTING.md) to this repository * [x] It builds for hardware native (see [Building the firmware](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-firmware)) * [x] It builds as BMDA (see [Building the BMDA](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-app)) * [x] I've tested it to the best of my ability * [x] My commit messages provide a useful short description of what the commits do ## Closing issues

Please rebase this PR on main and we'll get it reviewed.

Please rebase this PR on `main` and we'll get it reviewed.
dragonmux left a comment
Copy link

Couple of review notes, nothing too major though and with them addressed we'll be happy to merge this.

Couple of review notes, nothing too major though and with them addressed we'll be happy to merge this.
@ -149,6 +149,40 @@ static bool ftdi_jtag_next(const bool tms, const bool tdi)

Perhaps make this --clock_bytes; and add a comment as to why we're decrementing here? (what's the intent)

Perhaps make this `--clock_bytes;` and add a comment as to why we're decrementing here? (what's the intent)

Rather than using % 8U here, does it not make more sense to express this as clock_cycles & 7U? Similar question about why the - 1U too.

Rather than using `% 8U` here, does it not make more sense to express this as `clock_cycles & 7U`? Similar question about why the `- 1U` too.

Perhaps clock_cycles & ~7U? Why is it correct to store this as a uint16_t?

Perhaps `clock_cycles & ~7U`? Why is it correct to store this as a `uint16_t`?
ALTracer (Migrated from github.com) reviewed 2026年01月30日 06:45:46 +01:00
@ -149,6 +149,40 @@ static bool ftdi_jtag_next(const bool tms, const bool tdi)
ALTracer (Migrated from github.com) commented 2026年01月30日 06:45:46 +01:00
Copy link

Per appnote 108 referenced in ftdi_bmp.c, the second opcode 0x8F CLK_BYTES (unused previously) will generate N+1 byte clocks (like 8-bit SPI, which MPSSE really is). 0x8F 0x00 0x00 is 8 clocks, 0x8F 0x01 0x00 is 16 clocks etc. So if I want less than 8 clocks, then this opcode should not be submitted -- the if-clause guards it.

Per appnote 108 referenced in ftdi_bmp.c, the second opcode 0x8F CLK_BYTES (unused previously) will generate N+1 byte clocks (like 8-bit SPI, which MPSSE really is). `0x8F 0x00 0x00` is 8 clocks, `0x8F 0x01 0x00` is 16 clocks etc. So if I want less than 8 clocks, then this opcode should not be submitted -- the if-clause guards it.
ALTracer (Migrated from github.com) reviewed 2026年01月30日 06:48:14 +01:00
@ -149,6 +149,40 @@ static bool ftdi_jtag_next(const bool tms, const bool tdi)
ALTracer (Migrated from github.com) commented 2026年01月30日 06:48:14 +01:00
Copy link

No, it does not make more sense to me, the split between bytes and bits is division and remainder, much more natural operations than bit masking, and I trust the optimizing compiler to pick whatever, especially for hosted code (BMDA).
The decrement is for 0x8E CLK_BITS because it also generates n+1 clocks, not n. 0x8E 0x00 makes 1 clock, 0x8E 0x07 makes 8 clocks (as does 0x8F 0x00 0x00).

No, it does not make more sense to me, the split between bytes and bits is division and remainder, much more natural operations than bit masking, and I trust the optimizing compiler to pick whatever, especially for hosted code (BMDA). The decrement is for 0x8E CLK_BITS because it also generates n+1 clocks, not n. 0x8E 0x00 makes 1 clock, 0x8E 0x07 makes 8 clocks (as does 0x8F 0x00 0x00).
ALTracer (Migrated from github.com) reviewed 2026年01月30日 06:51:16 +01:00
@ -149,6 +149,40 @@ static bool ftdi_jtag_next(const bool tms, const bool tdi)
ALTracer (Migrated from github.com) commented 2026年01月30日 06:51:16 +01:00
Copy link

It should be correct to store it as uint16_t because the MPSSE command for this expects a two-byte length, anything longer (65535+1 bytes i.e. 524288 TCK cycles) should be submitted as multiple commands; but BMDA doesn't call this with more than 51 cycles IIRC. I can add an early-return guard for > 63 cycles (or > 524288?).

It should be correct to store it as uint16_t because the MPSSE command for this expects a two-byte length, anything longer (65535+1 bytes i.e. 524288 TCK cycles) should be submitted as multiple commands; but BMDA doesn't call this with more than 51 cycles IIRC. I can add an early-return guard for > 63 cycles (or > 524288?).
ALTracer commented 2026年01月30日 06:53:40 +01:00 (Migrated from github.com)
Copy link

Your questions made me realize I've designed this with a logic bug, where it commands 0x8e 0xff when 8 clocks are requested; should be 0x8e 0x07 or 0x8f 0x00. Confirmed with a small throwaway unit test snippet.
upd: this might be as easy to fix as if (clock_bits >= 8) return;.

Your questions made me realize I've designed this with a logic bug, where it commands 0x8e 0xff when 8 clocks are requested; should be 0x8e 0x07 or 0x8f 0x00. Confirmed with a small throwaway unit test snippet. upd: this might be as easy to fix as `if (clock_bits >= 8) return;`.
@ -149,6 +149,40 @@ static bool ftdi_jtag_next(const bool tms, const bool tdi)

So please insert a comment about this such as /* Adjust clock_bytes to handle the fact that the CLK_BYTES operation will generate N + 1 byte cycles */ to say why this is done and convey the intent.

So please insert a comment about this such as `/* Adjust clock_bytes to handle the fact that the CLK_BYTES operation will generate N + 1 byte cycles */` to say why this is done and convey the intent.
ALTracer (Migrated from github.com) reviewed 2026年01月30日 18:02:44 +01:00
@ -149,6 +149,40 @@ static bool ftdi_jtag_next(const bool tms, const bool tdi)
ALTracer (Migrated from github.com) commented 2026年01月30日 18:02:44 +01:00
Copy link

Existing comment expanded.

Existing comment expanded.
ALTracer commented 2026年01月30日 18:18:39 +01:00 (Migrated from github.com)
Copy link

https://gist.github.com/ALTracer/3b3b1c67c39b2803044e218ec6d21261

https://gist.github.com/ALTracer/3b3b1c67c39b2803044e218ec6d21261
ALTracer (Migrated from github.com) reviewed 2026年01月30日 19:59:20 +01:00
@ -149,6 +149,40 @@ static bool ftdi_jtag_next(const bool tms, const bool tdi)
ALTracer (Migrated from github.com) commented 2026年01月30日 19:59:20 +01:00
Copy link

Replaced with size_t as incoming argument. A testcase indicates that'll overflow to zero for 524288, which MPSSE technically can handle (because of off-by-one API in its commands).
Added an early-return guard for >= 524288.

Replaced with `size_t` as incoming argument. A testcase indicates that'll overflow to zero for 524288, which MPSSE technically can handle (because of off-by-one API in its commands). Added an early-return guard for `>= 524288`.
dragonmux left a comment
Copy link

LGTM, merging. Thank you for the contribution and this speed-up to the FTDI code for running cycles where TMS and TDI need to be kept constant

LGTM, merging. Thank you for the contribution and this speed-up to the FTDI code for running cycles where TMS and TDI need to be kept constant
Sign in to join this conversation.
No reviewers
Labels
Clear labels
BMD App
Black Magic Debug App (aka. PC hosted) (not firmware)
BMP Firmware
Black Magic Probe Firmware (not PC hosted software)
Bug
Confirmed bug
Build system
Build system
Can't reproduce
Maintainers can't reproduce this problem
CI
Continuous Integration System
Contribution wanted
User contributions welcome
Documentation
Project documentation
Draft
Work in progress draft
Duplicate
This issue or pull request already exists
Enhancement
General project improvement
Feedback wanted
Requires additional submitter feedback
Foreign Host Board
Non Native hardware to runing Black Magic firmware on
GDB
Issue/PR related to GDB
Good first issue
Good for newcommers
HwIssue Mitigation
Solving or mitigating a Hardware issue in Software
Information Needed
Maintainers need more information
NativeHardware
Official Black Magic Debug Hardware
New Host Board
New hardware to run Black Magic firmware on
New Target
New debug target
Off Topic
Something that does not involve the project in any way
Potential Bug
A potential, unconfirmed or very special circumstance bug
Regression
Bug caused by a regression
User Interest Needed
More user interest required before consideration
User Testing Needed
Looking for user testing reports
Won't fix
Outside of the project scope or works as intended
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
blackmagic-debug/blackmagic!2181
Reference in a new issue
blackmagic-debug/blackmagic
No description provided.
Delete branch "feature/jtagtap_cycle_ftdi"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?