Feature/wch link support #2061
Feature/wch link support #2061
Conversation
Since spec v0.13 there is an explicit version value (15) to identify dm/dmi's that do not conform with any available specs
Identify but allow non-standard DMI versions but only process standard explicitly know DM versions
| libbmd_core_args += ['-DENABLE_RVSWD=1'] | ||
| bmd_core_args += ['-DENABLE_RVSWD=1'] |
There was a problem hiding this comment.
We suggest making this CONFIG_RVSWD to be consistent with the revamped macro naming style. The if just above needs checking as we think the condition is wrong - think it should be checking rvswd_support, not rtt_support.
There was a problem hiding this comment.
You are correct, I noticed after pushing
| if (dmi->version == RISCV_DEBUG_UNKNOWN) | ||
| return; | ||
| if (dmi->version == RISCV_DEBUG_0_11) { | ||
| DEBUG_INFO("RISC-V debug v0.11 not presently supported\n"); | ||
| DEBUG_INFO("RISC-V v0.11 DMI is not presently supported\n"); |
There was a problem hiding this comment.
The inclusion of the term "debug" here is very intentional as the specification is the RISC-V Debug specification, not the ISA specification. For this piece of technical clarity in case we wind up needing to specify ISA specification conformance too, we would like to keep including this extra word in all messages involving the DTM, DMI, and DM versioning. Adding this is about the DMI version is good though 👍🏼
There was a problem hiding this comment.
Hm, that does make sense, I'll reword it again
| @@ -586,18 +613,21 @@ static riscv_debug_version_e riscv_dm_version(const uint32_t status) | |||
| case 0: | |||
| return RISCV_DEBUG_UNIMPL; | |||
| case 1: | |||
| DEBUG_INFO("RISC-V debug v0.11 DM\n"); | |||
| DEBUG_INFO("RISC-V v0.11 Debug Module\n"); | |||
There was a problem hiding this comment.
Same for these strings as above.
| } | ||
|
|
||
| /* Reads the 96 bit unique id */ | ||
| static bool ch32vx_uid_cmd(target_s *const target, const int argc, const char **const argv) |
There was a problem hiding this comment.
Is this able to use the stm32_common.c stm32_uid() function? Would be neat if it can.
There was a problem hiding this comment.
Maybe, a lot of things can be shared with stm32, that's what #1642 was about, but that's a mess now, I'll look but I might push sharing stuff for later
| } | ||
|
|
||
| if (!scan_result) { | ||
| platform_target_clk_output_enable(false); |
There was a problem hiding this comment.
This line and its identical counterpart below can be lifted out together just above the check for the scan result. That reduces repetition.
| if (!scan_result) | ||
| if (!scan_result) { | ||
| gdb_out("SWD scan found no devices.\n"); | ||
| #if defined(ENABLE_RVSWD) && defined(PLATFORM_HAS_RVSWD) | ||
| #if CONFIG_BMDA == 1 | ||
| scan_result = bmda_rvswd_scan(); | ||
| #else | ||
| scan_result = false; | ||
| #endif | ||
| if (!scan_result) | ||
| gdb_out("RVSWD scan found no devices.\n"); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Given how nested this is getting, perhaps lets invert this logic by extracting the post-scan actions into their own function (this allows all scan routines to stop duplicating those same actions) so we can check if (scan_result) and then return via that post-scan routine?
There was a problem hiding this comment.
I have some ideas to help with that and make it more future proof too, but for now some minor refactor in that direction wouldn't hurt
Detailed description
This is the first part of #1399 which will be broken up into smaller PR's
This adds support for the WCH-Link debug probe by WCH, it's a naive implementation based on a rough reverse engineer of the protocol, which is not publicly documented
Your checklist for this pull request
Closing issues