Conversation
WalkthroughThe PR introduces hardware isolation mode support to HSLink-Pro by adding conditional build configurations, separate binary variants (HSLink-Pro vs HSLink-Iso), and isolation-aware firmware logic spanning pin I/O, SPI transfers, and clock management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
tms也分输入输出吗？怎么有个tmsin |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/HSLinkPro-build.yml (1)
5-13: Workflow trigger paths do not match this file's name.Lines 6 and 12 reference
.github/workflows/HPM-build.yml, but this workflow is namedHSLinkPro-build.yml. This means changes to this file won't trigger the workflow itself. If intentional, no action needed; otherwise, update the trigger paths.push: paths: - - '.github/workflows/HPM-build.yml' + - '.github/workflows/HSLinkPro-build.yml' - 'projects/HSLink-Pro/**' pull_request: paths: - - '.github/workflows/HPM-build.yml' + - '.github/workflows/HSLinkPro-build.yml' - 'projects/HSLink-Pro/**'
🧹 Nitpick comments (5)
projects/HSLink-Pro/src/DAP_config.h (1)
536-561: Re-check pin-controller “restore” choice in PIN_SWDIO_OUT_DISABLE (gpiom_soc_gpio0 vs PIN_GPIOM).
PIN_SWDIO_OUT_ENABLE()usesgpiom_configure_pin_control_setting(PIN_TMS)(→PIN_GPIOM), butPIN_SWDIO_OUT_DISABLE()restores controller togpiom_soc_gpio0. If downstream code assumesPIN_GPIOMownership for fast IO, this could cause subtle behavior differences.projects/HSLink-Pro/src/CMakeLists.txt (1)
6-11: Good: variant name +-DHSLINK_ISOLATE={0,1}wiring is straightforward.
Optional: considerSTREQUAL "1"(vsEQUAL "1") since these config values often come in as strings/cached options.Also applies to: 40-45
projects/HSLink-Pro/src/dp_common.c (1)
125-132: Clock clamp makes sense, but use unsigned literals + fix comment typos.
Minor: prefer20000000Uand fixolny→onlyto avoid warnings/ambiguity.#if HSLINK_ISOLATE - // the isolator olny works below 20MHz + // the isolator only works below 20 MHz // so we set a limit avoid communication failure - if (clock > 20000000) + if (clock > 20000000U) { - clock = 20000000; + clock = 20000000U; } #endifprojects/HSLink-Pro/src/SW_DP/SW_DP_IO.c (1)
88-100: Good: SWD IO setup now explicitly configures PIN_TMS_IN + SWDIO_DIR.
Optional hardening: explicitlygpio_disable_pin_interrupt()forPIN_TMS_INhere as well (to avoid any unexpected IRQ configuration inherited from earlier states).projects/HSLink-Pro/src/SW_DP/SW_DP_SPI.c (1)
239-339: Replace TRANSFMT “magic numbers” with named constants + verify bitfields.
The isolate/non-isolate framing hinges on values like0x1F08/0x1F18and0x0008/0x0018. Please define them (with comments mapping to register fields) to reduce the chance of future accidental breakage—and verify against the HPM SPI TRANSFMT bit definitions for your SDK/SoC.+// TRANSFMT presets (verify against SoC TRM / hpm_spi_drv.h bitfields) +#define SWD_TRANSFMT_32_MOSI_BIDIR_0_LSB1 0x1F08U +#define SWD_TRANSFMT_32_MOSI_BIDIR_1_LSB1 0x1F18U +#define SWD_TRANSFMT_1_MOSI_BIDIR_0_LSB1 0x0008U +#define SWD_TRANSFMT_1_MOSI_BIDIR_1_LSB1 0x0018U ... #if HSLINK_ISOLATE - SWD_SPI_BASE->TRANSFMT = 0x1F08; /* datalen = 32bit, mosibidir = 0, lsb=1 */ + SWD_SPI_BASE->TRANSFMT = SWD_TRANSFMT_32_MOSI_BIDIR_0_LSB1; #else - SWD_SPI_BASE->TRANSFMT = 0x1F18; /* datalen = 32bit, mosibidir = 1, lsb=1 */ + SWD_SPI_BASE->TRANSFMT = SWD_TRANSFMT_32_MOSI_BIDIR_1_LSB1; #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/HSLinkPro-build.yml(1 hunks)projects/HSLink-Pro/src/CMakeLists.txt(2 hunks)projects/HSLink-Pro/src/DAP_config.h(6 hunks)projects/HSLink-Pro/src/HID_COMM/hid_comm.cpp(1 hunks)projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_IO.c(2 hunks)projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_SPI.c(1 hunks)projects/HSLink-Pro/src/SW_DP/SW_DP_IO.c(1 hunks)projects/HSLink-Pro/src/SW_DP/SW_DP_SPI.c(9 hunks)projects/HSLink-Pro/src/dp_common.c(1 hunks)
🔇 Additional comments (9)
.github/workflows/HSLinkPro-build.yml (2)
68-85: Artifact copying assumes both build outputs exist.The merge and copy steps assume both
HSLink-Pro.binandHSLink-Iso.binare present after the build phase. If the shared build directory (discussed above) causes the first build's output to be lost, these steps will fail silently or copy incorrect/missing files.Ensure the build directory separation issue is resolved before these steps can reliably execute.
63-66: Build configuration properly differentiates output binaries via APP_NAME variable.The concern about shared build directory is mitigated: CMakeLists.txt correctly sets
APP_NAMEto either "HSLink-Iso" or "HSLink-Pro" based on theCONFIG_HARDWARE_ISOLATEDflag (lines 6–10), ensuring output binaries are named distinctly and coexist in the samesrc/build/output/directory. No separate build directories are needed.projects/HSLink-Pro/src/HID_COMM/hid_comm.cpp (1)
136-141: Isolation-aware “model” field looks correct (HSLink-Iso vs HSLink-Pro).
Nice, low-risk way to surface the build variant without changing the rest of the payload.projects/HSLink-Pro/src/DAP_config.h (2)
384-424: Good: PORT_OFF now fully tri-states/configures PIN_TMS_IN too.
This reduces the chance of leaving the extra input pin floating in an unintended mode.
480-521: Ensure PIN_TMS_IN is always correctly muxed before isolate-mode reads.
You now read SWDIO/TMS fromPIN_TMS_INunderHSLINK_ISOLATE; that’s fine, but it increases sensitivity to pad FUNC_CTL / pinmux defaults in every SWD/JTAG setup path (not just PORT_OFF).projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_SPI.c (1)
68-80: Good: SWDIO_DIR pad FUNC_CTL initialized alongside other pads.
Helps avoid relying on reset defaults for a direction-control pin.projects/HSLink-Pro/src/SW_DP/SW_DP_SPI.c (2)
426-435: Good: mosi_bidir matches isolate vs non-isolate behavior.
Tyingformat_config.common_config.mosi_bidirto the isolate mode aligns with the TRANSFMT switching in the transfer path.
40-47: No action needed — PIN_TMS_IN pinmux is properly configured.The FUNC_CTL for PIN_TMS_IN (PA28) is correctly set to SPI1_MISO in
board_init_spi_pins()called at line 42. The PAD_CTL configuration at line 46 for ISOLATE mode sets electrical properties (slew rate, speed, pull-enable, pull-select) and is the appropriate place for these settings.projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_IO.c (1)
265-316: Good: SWDIO_DIR is now explicitly initialized/controlled in JTAG IO setup.
Optional: add a short comment near the PAD_CTL/FUNC_CTL block explaining SWDIO_DIR’s electrical role (direction/isolator control) to prevent future regressions.
对于HSLink-Iso是需要区分输入和输出的，因为隔离器只能过单向信号，因此输入和输出做了拆分 
HSLink-Pro的输入输出并联了，原始设计是通过TMS脚单线半双工复用，将TMS_IN设置为输入模式就好 （实际上这种设计在GPIO模拟模式下不仅需要手动切换DIR引脚方向电平，以控制外部电平转换电路方向，还需要手动切换TMS方向，因此速度更慢一些 |
|
哦这一会SWDIO DIR一会TMS的让我以为TMS是时钟了。。。 |
|
感觉还行，虽然 |
实际用了，这玩意接的是1T45的方向控制 
|
| if (DAP_Data.swd_conf.data_phase && ((request & DAP_TRANSFER_RnW) == 0U)) { | ||
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(SWDIO_DIR), GPIO_GET_PIN_INDEX(SWDIO_DIR), 1); | ||
| SWD_SPI_BASE->TRANSCTRL = 0x01000000; /* only write mode*/ | ||
| #if HSLINK_ISOLATE |
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(SWDIO_DIR), GPIO_GET_PIN_INDEX(SWDIO_DIR), 0); | ||
| SWD_SPI_BASE->TRANSCTRL = 0x02000000; /* only read mode*/ | ||
| #if HSLINK_ISOLATE | ||
| SWD_SPI_BASE->TRANSFMT = 0x0008; /* datalen = 1bit, mosibidir = 0, lsb=1 */ |
There was a problem hiding this comment.
为什么ISO版本不需要开启MOSIBIDIR呢
There was a problem hiding this comment.
看原理图，ISO版本是双线双向，Pro版是单线双向（硬件22r电阻太小了，没法双线驱动）
| gpiom_configure_pin_control_setting(PIN_TCK); | ||
| gpiom_configure_pin_control_setting(PIN_TMS); | ||
| gpiom_configure_pin_control_setting(PIN_TDI); | ||
| gpiom_configure_pin_control_setting(PIN_TDO); |
|
|
||
| writer.Key("model"); | ||
| #if HSLINK_ISOLATE | ||
| writer.String("HSLink-Iso"); |
|
@kaidegit 目前compat板子是用的动态读取版本号，ISO用的是静态的方式加上宏定义控制特性，这两者你觉得有必要统一一下吗 |
因为我看不同的地方蛮多的就觉得宏或许更明确？当然我肯定觉得一个固件做完最好。。。。只是我也感觉有点奇怪怎么有这么多不同的。。。 |
|
我也觉得用一个固件能搞定最好，要不然ISO固件刷到普通板上也可能会有奇怪的反应 |
|
不考虑增加这么多其他版本的patch，只保留hslink pro。其次，改动太多，比如swdio dir不是main里面配了吗，为什么又重新配，tdo为什么去掉，tms in又是哪来的？ |
4 participants
for hardware design, please refer to HSLink-Iso 1.2.2 hardware repo.
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.