8
33
Fork
You've already forked blackmagic
23

Fix: ARM JTAG regressions #1389

Merged
esden merged 56 commits from fix/arm-jtag-regressions into main 2023年04月03日 02:24:54 +02:00

Detailed description

This PR addresses several regressions in ARM JTAG handling detailed below and necessarily rewrites the ADIv5 high level remote protocol.

The regressions fixed are as follows:

  • Fixed a regression introduced with 6dcc5d41 which broke non-multiple-of-8 TDI-TDO sequences
  • Fixed an issue whereby CMSIS-DAP adaptors such as ORBTrace should never have worked with JTAG ADIv5 devices due to initialising the IR length information too late
  • Fixed a timing issue in the TDI sequence function much like the TDI-TDO sequence one
  • Fixed the remote protocol going "everything's ok" even when dp->fault is set non-zero and everything is not ok
  • Fixed the remote protocol pretending exceptions don't exist and the ADIv5 JTAG and SWD layers can't throw them (they can) resulting in permanent comms loss when one occurs
  • Fixed the BMDA side of the remote protocol never doing protocol recovery and driving the remote into a state due to not handling any errors at all

As part of the remote protocol rewrite we have pre-emptively split the high-level component of the protocol into generic high-level commands and ADIv5-specific ones to make organisational space for the RISC-V specific ones when v2.0 happens. This happens to also make reading BMDA probe logs easier.

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

Fixes #858

<!-- Filling this template is mandatory --> ## Detailed description This PR addresses several regressions in ARM JTAG handling detailed below and necessarily rewrites the ADIv5 high level remote protocol. The regressions fixed are as follows: * Fixed a regression introduced with 6dcc5d41 which broke non-multiple-of-8 TDI-TDO sequences * Fixed an issue whereby CMSIS-DAP adaptors such as ORBTrace should never have worked with JTAG ADIv5 devices due to initialising the IR length information too late * Fixed a timing issue in the TDI sequence function much like the TDI-TDO sequence one * Fixed the remote protocol going "everything's ok" even when dp->fault is set non-zero and everything is not ok * Fixed the remote protocol pretending exceptions don't exist and the ADIv5 JTAG and SWD layers can't throw them (they can) resulting in permanent comms loss when one occurs * Fixed the BMDA side of the remote protocol never doing protocol recovery and driving the remote into a state due to not handling any errors at all As part of the remote protocol rewrite we have pre-emptively split the high-level component of the protocol into generic high-level commands and ADIv5-specific ones to make organisational space for the RISC-V specific ones when v2.0 happens. This happens to also make reading BMDA probe logs easier. ## 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 (`make PROBE_HOST=native`) * [x] It builds as BMDA (`make PROBE_HOST=hosted`) * [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 Fixes #858
tlyu commented 2023年02月17日 01:37:35 +01:00 (Migrated from github.com)
Copy link

Tested on BMP with v1.9.0, with SWD to a GD32 target. Fallback seems to work. With BMP updated to firmware built from this PR, scan succeeds, but attach fails, with errors similar to what I previously reported on Discord.
bmp-pr-1389-updated-fw.log

Tested on BMP with v1.9.0, with SWD to a GD32 target. Fallback seems to work. With BMP updated to firmware built from this PR, scan succeeds, but attach fails, with errors similar to what I previously reported on Discord. [bmp-pr-1389-updated-fw.log](https://github.com/blackmagic-debug/blackmagic/files/10762112/bmp-pr-1389-updated-fw.log)
tlyu commented 2023年02月19日 02:21:55 +01:00 (Migrated from github.com)
Copy link

Update: with ENABLE_DEBUG=1 on the BMP firmware (I had to disable some targets to get it to fit), I get

SWD access resulted in no response
Recovering and re-trying access

on the aux CDC channel around the time that BMDA is showing

swdptap_seq_in 3 clock_cycles: 00000007
!SI20#
 PFFFFFFFF
swdptap_seq_in_parity 32 clock_cycles: ffffffff ERR
DP Error 0x00000000
ap_mem_write_sized @ e000edf0 len 4, align 4: 03 00 5f a0
!AM00002300004002e000edf00000000403005FA0#
Timeout while waiting for BMP response
remote_adiv5_mem_write_bytes comms error: -4
remote_adiv5_mem_write_bytes error around 0xe000edf0

Which reinforces my suspicion that BMP is attempting the recovery procedure, but is somehow becoming uncommunicative with BMDA during or after.

That's about all I have time to debug at the moment.

Update: with `ENABLE_DEBUG=1` on the BMP firmware (I had to disable some targets to get it to fit), I get ``` SWD access resulted in no response Recovering and re-trying access ``` on the aux CDC channel around the time that BMDA is showing ``` swdptap_seq_in 3 clock_cycles: 00000007 !SI20# PFFFFFFFF swdptap_seq_in_parity 32 clock_cycles: ffffffff ERR DP Error 0x00000000 ap_mem_write_sized @ e000edf0 len 4, align 4: 03 00 5f a0 !AM00002300004002e000edf00000000403005FA0# Timeout while waiting for BMP response remote_adiv5_mem_write_bytes comms error: -4 remote_adiv5_mem_write_bytes error around 0xe000edf0 ``` Which reinforces my suspicion that BMP is attempting the recovery procedure, but is somehow becoming uncommunicative with BMDA during or after. That's about all I have time to debug at the moment.
Author
Owner
Copy link

Thank you, that's incredibly helpful as we weren't certain if that wasn't still going to be a problem post-patch (it clearly is) so giving us something more to dig into to finish stabilising this branch before Esden reviews it with a view to merge

Thank you, that's incredibly helpful as we weren't certain if that wasn't still going to be a problem post-patch (it clearly is) so giving us something more to dig into to finish stabilising this branch before Esden reviews it with a view to merge
Author
Owner
Copy link

So, error recovery was still broken under JTAG, which is now fixed, but we haven't yet dived into error recovery under SWD. Unless we can figure out something simple today, we're inclined to punt fixing that into a new PR so Esden gets a chance to review what's here, test it and merge it. This PR is intentionally not touching SWD as such as it was JTAG that was quite so broken

So, error recovery was still broken under JTAG, which is now fixed, but we haven't yet dived into error recovery under SWD. Unless we can figure out something simple today, we're inclined to punt fixing that into a new PR so Esden gets a chance to review what's here, test it and merge it. This PR is intentionally not touching SWD as such as it was JTAG that was quite so broken
tlyu commented 2023年02月20日 22:40:48 +01:00 (Migrated from github.com)
Copy link

So, error recovery was still broken under JTAG, which is now fixed, but we haven't yet dived into error recovery under SWD. Unless we can figure out something simple today, we're inclined to punt fixing that into a new PR so Esden gets a chance to review what's here, test it and merge it. This PR is intentionally not touching SWD as such as it was JTAG that was quite so broken

Thanks for looking into it! I probably won't have time to test your latest changes today, so I'm fine with leaving the SWD fixes for the high-level protocol for separate PR or issue.

> So, error recovery was still broken under JTAG, which is now fixed, but we haven't yet dived into error recovery under SWD. Unless we can figure out something simple today, we're inclined to punt fixing that into a new PR so Esden gets a chance to review what's here, test it and merge it. This PR is intentionally not touching SWD as such as it was JTAG that was quite so broken Thanks for looking into it! I probably won't have time to test your latest changes today, so I'm fine with leaving the SWD fixes for the high-level protocol for separate PR or issue.
tlyu commented 2023年02月24日 17:21:18 +01:00 (Migrated from github.com)
Copy link

I just tried the latest changes, and there seems to be a regression that causes blackmagic -t to fail to detach the target after the scan. Connecting directly to BMP in GDB, then attaching and detaching the target, allows the target to resume.

I haven't tracked down what causes it yet, but from preliminary tests, it looks like the recent "target resumption" change is not the cause.

I just tried the latest changes, and there seems to be a regression that causes `blackmagic -t` to fail to detach the target after the scan. Connecting directly to BMP in GDB, then attaching and detaching the target, allows the target to resume. I haven't tracked down what causes it yet, but from preliminary tests, it looks like the recent "target resumption" change is _not_ the cause.
tlyu commented 2023年02月24日 17:53:46 +01:00 (Migrated from github.com)
Copy link

I went back to b865bd8c87939d7f7267af5b5774643c2835eef8 on both BMDA and BMP, which I had previously tested, and the regression on blackmagic -t shows up there as well. I guess I didn't test blackmagic -t at that time.

I went back to b865bd8c87939d7f7267af5b5774643c2835eef8 on both BMDA and BMP, which I had previously tested, and the regression on `blackmagic -t` shows up there as well. I guess I didn't test `blackmagic -t` at that time.
Author
Owner
Copy link

Any chance you could share a src/blackmagic -tv 17 log of that behaviour so we can dig into it? We'll aim to add a fix to this PR if possible

Any chance you could share a `src/blackmagic -tv 17` log of that behaviour so we can dig into it? We'll aim to add a fix to this PR if possible
tlyu commented 2023年02月24日 23:05:52 +01:00 (Migrated from github.com)
Copy link

I'll try to get to that soon. Another regression is that it's no longer possible to use blackmagic -t -H to disable the high-level remote protocol for scanning.

I'll try to get to that soon. Another regression is that it's no longer possible to use `blackmagic -t -H` to disable the high-level remote protocol for scanning.
tlyu commented 2023年02月25日 16:47:39 +01:00 (Migrated from github.com)
Copy link

I'll upload logs from blackmagic -tv 17 once I recreate them, because I don't remember which runs caused the target to remain halted or not. I can't see any obvious differences that would cause it.

I do have some suspicions that the clock may be gated off too quickly after sending the detach commands, due to an unintended timing change happening as a side effect of these changes. (ADIv5.2 §B4.1.1 requires at least 8 clocks of idle if the clock is gated off after a command, to ensure that it's fully shifted in to the SW-DP.)

I'll upload logs from `blackmagic -tv 17` once I recreate them, because I don't remember which runs caused the target to remain halted or not. I can't see any obvious differences that would cause it. I do have some suspicions that the clock may be gated off too quickly after sending the detach commands, due to an unintended timing change happening as a side effect of these changes. (ADIv5.2 §B4.1.1 requires at least 8 clocks of idle if the clock is gated off after a command, to ensure that it's fully shifted in to the SW-DP.)
tlyu commented 2023年02月25日 21:00:07 +01:00 (Migrated from github.com)
Copy link

Logs from various combinations of BMDA and BMP firmware. The "pull 1405" happened to be what I had flashed most recently. I can repeat it with main if you want, but I think the changes there aren't going to affect this behavior.

I'm almost tempted to hook up a logic analyzer to see if there's something weird with the clock disable with the firmware built from this patch. It would have to be different between native and remote, despite a lot of shared code, though.

I did try adding swd_proc.seq_out(0, 8); in a couple of places after SWD reads, on the theory that a turnaround plus the required idle cycles would make things behave, but it didn't seem to help.

BMDA pull 1405, firmware pull 1405: no halt

bmp-t-pull-1405-no-halt.log

BMDA pull 1389, firmware pull 1405: no halt

bmp-t-pull-1389-fw-pull-1405-no-halt.log

BMDA pull 1389, firmware pull 1405, -H flag: no halt

bmp-t-H-pull-1389-fw-pull-1405-no-halt.log

BMDA pull 1389, firmware pull 1389: halt

bmp-t-pull-1389-fw-pull-1389-halt.log

Logs from various combinations of BMDA and BMP firmware. The "pull 1405" happened to be what I had flashed most recently. I can repeat it with main if you want, but I think the changes there aren't going to affect this behavior. I'm almost tempted to hook up a logic analyzer to see if there's something weird with the clock disable with the firmware built from this patch. It would have to be different between native and remote, despite a lot of shared code, though. I did try adding `swd_proc.seq_out(0, 8);` in a couple of places after SWD reads, on the theory that a turnaround plus the required idle cycles would make things behave, but it didn't seem to help. BMDA pull 1405, firmware pull 1405: no halt [bmp-t-pull-1405-no-halt.log](https://github.com/blackmagic-debug/blackmagic/files/10832175/bmp-t-pull-1405-no-halt.log) BMDA pull 1389, firmware pull 1405: no halt [bmp-t-pull-1389-fw-pull-1405-no-halt.log](https://github.com/blackmagic-debug/blackmagic/files/10832178/bmp-t-pull-1389-fw-pull-1405-no-halt.log) BMDA pull 1389, firmware pull 1405, `-H` flag: no halt [bmp-t-H-pull-1389-fw-pull-1405-no-halt.log](https://github.com/blackmagic-debug/blackmagic/files/10832180/bmp-t-H-pull-1389-fw-pull-1405-no-halt.log) BMDA pull 1389, firmware pull 1389: halt [bmp-t-pull-1389-fw-pull-1389-halt.log](https://github.com/blackmagic-debug/blackmagic/files/10832181/bmp-t-pull-1389-fw-pull-1389-halt.log)
tlyu (Migrated from github.com) requested changes 2023年02月25日 23:20:59 +01:00
tlyu (Migrated from github.com) left a comment
Copy link

See file comment for the possible cause of the regression with high-level SWD.

See file comment for the possible cause of the regression with high-level SWD.
tlyu (Migrated from github.com) commented 2023年02月25日 23:20:03 +01:00
Copy link

This should probably be unhexify(data, packet + 32U, length). I finally got a logic analyzer on the SWD lines (had to slow it down because it was either loading the lines or coupling too much noise) and saw that the data bits actually written on the wire were always the same, regardless of the request.

A local patch with that change seems to resolve at least some of the regressions for me.

This should probably be `unhexify(data, packet + 32U, length)`. I finally got a logic analyzer on the SWD lines (had to slow it down because it was either loading the lines or coupling too much noise) and saw that the data bits actually written on the wire were always the same, regardless of the request. A local patch with that change seems to resolve at least some of the regressions for me.
tlyu commented 2023年02月25日 23:42:54 +01:00 (Migrated from github.com)
Copy link

There's still a regression where blackmagic -t -H fails, but I haven't finished investigating that yet.

There's still a regression where `blackmagic -t -H` fails, but I haven't finished investigating that yet.
tlyu (Migrated from github.com) requested changes 2023年02月26日 00:17:00 +01:00
tlyu (Migrated from github.com) left a comment
Copy link

The changes to remote_respond made a substantial change to the protocol with all-zeros responses returning K instead of K0. This causes some things to break, including blackmagic -t -H. Unfortunately, BMDA wasn't adjusted to expect it. It's probably easiest to revert to the previous protocol.

The changes to `remote_respond` made a substantial change to the protocol with all-zeros responses returning `K` instead of `K0`. This causes some things to break, including `blackmagic -t -H`. Unfortunately, BMDA wasn't adjusted to expect it. It's probably easiest to revert to the previous protocol.
@ -51,62 +54,69 @@ uint64_t remotehston(const uint32_t max, const char *const str)
}
tlyu (Migrated from github.com) commented 2023年02月26日 00:05:53 +01:00
Copy link

This bound is incorrect; it should be 16.

This bound is incorrect; it should be 16.
tlyu (Migrated from github.com) commented 2023年02月26日 00:11:33 +01:00
Copy link

The original do-while loop produced a different protocol: there was always at least one hex digit in a successful result. Unfortunately, the consuming code in BMDA (remote_swdptap.c) wasn't adjusted, so there were spurious errors whenever an operation produced an all-zeros response. (K instead of K0 like the previous version did)

The original do-while loop produced a different protocol: there was always at least one hex digit in a successful result. Unfortunately, the consuming code in BMDA (`remote_swdptap.c`) wasn't adjusted, so there were spurious errors whenever an operation produced an all-zeros response. (`K` instead of `K0` like the previous version did)
Author
Owner
Copy link

You are entirely correct. Fixed :)

You are entirely correct. Fixed :)
@ -51,62 +54,69 @@ uint64_t remotehston(const uint32_t max, const char *const str)
}
Author
Owner
Copy link

Astute observation, thank you!

Astute observation, thank you!
Author
Owner
Copy link

We are split between adjusting this and changing the remote_swdptap.c code - there are a couple of ways we can adjust the new loop to regain the old behaviour but at the cost of code size; it's probably better to adjust BMDA instead - thoughts?

We are split between adjusting this and changing the remote_swdptap.c code - there are a couple of ways we can adjust the new loop to regain the old behaviour but at the cost of code size; it's probably better to adjust BMDA instead - thoughts?
Author
Owner
Copy link

Hmm.. this is actually screwing inadvertently with the old JTAG scan method too because it makes jtag_proc.jtagtap_next() produce different results to what's expected.. we'll check if idx is 0 after the loop and adjust it in a fixup step. That'll give the old protocol behaviour

Hmm.. this is actually screwing inadvertently with the old JTAG scan method too because it makes jtag_proc.jtagtap_next() produce different results to what's expected.. we'll check if idx is 0 after the loop and adjust it in a fixup step. That'll give the old protocol behaviour
tlyu (Migrated from github.com) reviewed 2023年02月26日 01:24:41 +01:00
tlyu (Migrated from github.com) commented 2023年02月26日 01:24:41 +01:00
Copy link

As long as BMDA still accepts K0, it'll probably remain compatible with older firmware. I prefer to avoid changing existing protocols except where absolutely necessary, lest there be some subtle compatibility issue later on.

(The existing remote_swdptap.c also has an incorrect check in the debug message that caused a swdptap_seq_in_parity failed, error \n instead of usefully saying short response.)

My thoughts about how to adjust the new loop are to either change the condition to idx < 16 && (param || !idx) (which is what I tested) or just move the non-zero check for param to after the shift, and have it break the loop

 param >>= 4U;
 if (!param)
 break;

I haven't inspected the assembly output to compare them, but it's worth noting that the existing proposed change probably already has two test-and-branch sections in the loop already, which is probably more than the previous code did.

As long as BMDA still accepts `K0`, it'll probably remain compatible with older firmware. I prefer to avoid changing existing protocols except where absolutely necessary, lest there be some subtle compatibility issue later on. (The existing `remote_swdptap.c` also has an incorrect check in the debug message that caused a `swdptap_seq_in_parity failed, error \n` instead of usefully saying `short response`.) My thoughts about how to adjust the new loop are to either change the condition to `idx < 16 && (param || !idx)` (which is what I tested) or just move the non-zero check for `param` to after the shift, and have it break the loop ```c param >>= 4U; if (!param) break; ``` I haven't inspected the assembly output to compare them, but it's worth noting that the existing proposed change probably already has two test-and-branch sections in the loop already, which is probably more than the previous code did.
Author
Owner
Copy link

Interestingly, the new code produces overall smaller asm output than the old because the compiler is more readily able to optimise. In this case, we do get to entirely change the protocol if we wish as we bumped the protocol version for that reason - a license to break compatibility - but fixing K meaning K0 in every remote protocol function in BMDA would be far too much work. Please give the current version of this PR a spin as it should have fixed all 3 of the issues you found :)

Interestingly, the new code produces overall smaller asm output than the old because the compiler is more readily able to optimise. In this case, we do get to entirely change the protocol if we wish as we bumped the protocol version for that reason - a license to break compatibility - but fixing `K` meaning `K0` in every remote protocol function in BMDA would be far too much work. Please give the current version of this PR a spin as it should have fixed all 3 of the issues you found :)
tlyu commented 2023年02月26日 02:04:47 +01:00 (Migrated from github.com)
Copy link

Thanks for the updates! I did some testing, and it does fix the regressions that I found with this PR.

Unfortunately, the timeout during error recovery on SWD still happens. I think I failed to test my remote protocol fixes without the "add idle clocks after reads on SWD" changes, and those were preventing the error conditions that were triggering the timeouts.

That was a pre-existing problem on main, though, so I'm happy to defer it for another PR or issue.

Thanks for the updates! I did some testing, and it does fix the regressions that I found with this PR. Unfortunately, the timeout during error recovery on SWD still happens. I think I failed to test my remote protocol fixes without the "add idle clocks after reads on SWD" changes, and those were preventing the error conditions that were triggering the timeouts. That was a pre-existing problem on main, though, so I'm happy to defer it for another PR or issue.
Author
Owner
Copy link

Excellent, and yeah - think we'll deal with the SWD problem on a separate PR as mentioned before.

Thank you for your excellent review! Much appreciated and helpful :)

Excellent, and yeah - think we'll deal with the SWD problem on a separate PR as mentioned before. Thank you for your excellent review! Much appreciated and helpful :)
Author
Owner
Copy link

Apparently we hadn't gone quite far enough fixing the order of operations in adiv5.c's adiv5_dp_init(), with the new order for reading the DPIDR and all that both slightly accelerating the process if high-level functions are in use, and dealing with sticky errors that bit earlier.

Apparently we hadn't gone quite far enough fixing the order of operations in adiv5.c's adiv5_dp_init(), with the new order for reading the DPIDR and all that both slightly accelerating the process if high-level functions are in use, and dealing with sticky errors that bit earlier.
esden (Migrated from github.com) approved these changes 2023年04月03日 02:24:31 +02:00
esden (Migrated from github.com) left a comment
Copy link

Damn, that is a lot of rework. I tested it to the best of my abilities. It does look good otherwise, and it definitely does fix the JTAG regression we had too. More use/testing needed to make sure, but that should happen in main. :)

Damn, that is a lot of rework. I tested it to the best of my abilities. It does look good otherwise, and it definitely does fix the JTAG regression we had too. More use/testing needed to make sure, but that should happen in main. :)
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!1389
Reference in a new issue
blackmagic-debug/blackmagic
No description provided.
Delete branch "fix/arm-jtag-regressions"

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?