Platform behaviour unification (wrt VTref, GPIOs, DFU) #1868

ALTracer commented 2024年07月13日 15:19:27 +02:00 (Migrated from github.com)

There are lots of inconsistently implemented features in all in-tree platforms which should obey the same internal API.
The actually-invoking-UB one is platform_target_voltage(), see #1866. But also I have doubts about platform_nrst_set_val() (and get_val()), proposals on platform_target_clk_output_enable() and major gripes with platform_request_boot() (and entire bootloader-related workflow).

First, Target voltage: ABSENT!\n in GDB interface, or even Target voltage: ABSENT! Volt\n in BMDA CLI.
github.com/blackmagic-debug/blackmagic@117028d3e2/src/platforms/f4discovery/platform.c (L118-L121)
Some platforms return a NULL char pointer: f4discovery, blackpill-f4, hydrabus, also launchpad-icdi.
Some platforms return a fixed string: 96b_carbon, f3, f072.
Some platforms implement sampling internal ADC: native (PB0), swlink, stlink, stlinkv3 (all PA0).
For this point I suggest 1) unifying the returned value accross the 7 platforms which don't do ADC, 2) catching that in command.c and remote.c and suppressing the output of unknown voltage.

Second, cortexm_reset() may have arbitrarily short pulse length.
github.com/blackmagic-debug/blackmagic@117028d3e2/src/target/cortexm.c (L928-L934)
Some platforms do a small busy loop on asserting NRST pin: native, stlinkv3, launchpad-icdi.
Some platforms call a couple libopencm3 functions and return immediately: 96b_carbon, blackpill-f4, f3, f0, stlink.
Some platforms do nothing (and return false): f4discovery, hydrabus.
swlink checks its TRST pin (because that's what on STM8S JTAG pin header) GPIO_IDR value (because it's unbuffered?) in either output open-drain mode or input pull-up. This is not protected by any timeouts.
Also, TRST is driven by a completely different function:
github.com/blackmagic-debug/blackmagic@117028d3e2/src/platforms/common/jtagtap.c (L59-L70)
For this point I suggest 1) unifying all implementations to do an open-drain pin-clear for assert, and input-mode pull-up on deassert, especially when nRST pin is not buffered; 2) either allocating some pins or saving bool state on f407 and hydrabus; 3) moving the delays out into cortexm_reset(), riscv_reset(), cortexar_reset(), zynq7_reset(). I don't believe platform support code should be blocking like that.

Third, platform_target_clk_output_enable() is a feature required to use small-package STM32G0 chips which share their SWCLK and UART_TX pins; on buffered adapters it's accomplished by disabling the corresponding OEn signal.
github.com/blackmagic-debug/blackmagic@117028d3e2/src/platforms/native/platform.c (L438-L450)
On unbuffered directly-wired adapters I propose adding libopencm3 calls to set input mode (possibly with some pull-up/down) and output mode. It will behave very similarly to SWDIO_MODE_DRIVE() and SWDIO_MODE_INPUT() multiline macros. While I don't have said DUTs, the "disable SWDIO/SWCLK drive when inactive" feature implemented will let me keep multiple BMPs connected to a single target, or a BMP and a 3rd party debugger, which is very useful when I'm comparing target support behaviour to fix BMD bugs.
On buffered adapters like genuine STLINK/V2 without SWCLK_OEn capability I'm not sure what to do. STLINK/V2-ISOL can switch directions of SWDIO line (it has to), but unsure about SWCLK (maybe PA4 disables drive?).

Fourth, I have rewritten the f4discovery "enter ST MaskROM" reboot flow for blackpill-f4 and cleaned up a significant amount of nolints and pragmas, but also the latter now fully supports the faster BMD bootloader (F4 flash driver) with USB DFU capability. There are other platforms which are not cleaned up like so, and when I'm comparing platform behaviour etc. this brings a lot of confusion. Which of these actions are required and possible on them: disable SYSCFG memory remap, relocate SCB VTOR, deinit every peripheral or just reboot and check a flag, reset system or reset core, set flags in GPIO registers or use a normal noinit variable, blink differently or light up a different LED, and so on. The booloader with F1 flash driver is successfully used on native, stlink, swlink, and is required to do USB DFU because F103 does not have the MaskROM (or OTGFS) of F107.

There are lots of inconsistently implemented features in all in-tree platforms which should obey the same internal API. The actually-invoking-UB one is `platform_target_voltage()`, see #1866. But also I have doubts about `platform_nrst_set_val()` (and get_val()), proposals on `platform_target_clk_output_enable()` and major gripes with `platform_request_boot()` (and entire bootloader-related workflow). First, `Target voltage: ABSENT!\n` in GDB interface, or even `Target voltage: ABSENT! Volt\n` in BMDA CLI. https://github.com/blackmagic-debug/blackmagic/blob/117028d3e2fafbbb930ae09ddd4e1f2ef3a849a4/src/platforms/f4discovery/platform.c#L118-L121 Some platforms return a NULL char pointer: f4discovery, blackpill-f4, hydrabus, also launchpad-icdi. Some platforms return a fixed string: 96b_carbon, f3, f072. Some platforms implement sampling internal ADC: native (PB0), swlink, stlink, stlinkv3 (all PA0). For this point I suggest 1) unifying the returned value accross the 7 platforms which don't do ADC, 2) catching that in command.c and remote.c and suppressing the output of unknown voltage. Second, `cortexm_reset()` may have arbitrarily short pulse length. https://github.com/blackmagic-debug/blackmagic/blob/117028d3e2fafbbb930ae09ddd4e1f2ef3a849a4/src/target/cortexm.c#L928-L934 Some platforms do a small busy loop on asserting NRST pin: native, stlinkv3, launchpad-icdi. Some platforms call a couple libopencm3 functions and return immediately: 96b_carbon, blackpill-f4, f3, f0, stlink. Some platforms do nothing (and return false): f4discovery, hydrabus. swlink checks its TRST pin (because that's what on STM8S JTAG pin header) GPIO_IDR value (because it's unbuffered?) in either output open-drain mode or input pull-up. This is not protected by any timeouts. Also, TRST is driven by a completely different function: https://github.com/blackmagic-debug/blackmagic/blob/117028d3e2fafbbb930ae09ddd4e1f2ef3a849a4/src/platforms/common/jtagtap.c#L59-L70 For this point I suggest 1) unifying all implementations to do an open-drain pin-clear for assert, and input-mode pull-up on deassert, especially when nRST pin is not buffered; 2) either allocating some pins or saving bool state on f407 and hydrabus; 3) moving the delays out into cortexm_reset(), riscv_reset(), cortexar_reset(), zynq7_reset(). I don't believe platform support code should be blocking like that. Third, `platform_target_clk_output_enable()` is a feature required to use small-package STM32G0 chips which share their SWCLK and UART_TX pins; on buffered adapters it's accomplished by disabling the corresponding OEn signal. https://github.com/blackmagic-debug/blackmagic/blob/117028d3e2fafbbb930ae09ddd4e1f2ef3a849a4/src/platforms/native/platform.c#L438-L450 On unbuffered directly-wired adapters I propose adding libopencm3 calls to set input mode (possibly with some pull-up/down) and output mode. It will behave very similarly to SWDIO_MODE_DRIVE() and SWDIO_MODE_INPUT() multiline macros. While I don't have said DUTs, the "disable SWDIO/SWCLK drive when inactive" feature implemented will let me keep multiple BMPs connected to a single target, or a BMP and a 3rd party debugger, which is very useful when I'm comparing target support behaviour to fix BMD bugs. On buffered adapters like genuine STLINK/V2 without SWCLK_OEn capability I'm not sure what to do. STLINK/V2-ISOL can switch directions of SWDIO line (it has to), but unsure about SWCLK (maybe PA4 disables drive?). Fourth, I have rewritten the f4discovery "enter ST MaskROM" reboot flow for blackpill-f4 and cleaned up a significant amount of nolints and pragmas, but also the latter now fully supports the faster BMD bootloader (F4 flash driver) with USB DFU capability. There are other platforms which are not cleaned up like so, and when I'm comparing platform behaviour etc. this brings a lot of confusion. Which of these actions are required and possible on them: disable SYSCFG memory remap, relocate SCB VTOR, deinit every peripheral or just reboot and check a flag, reset system or reset core, set flags in GPIO registers or use a normal noinit variable, blink differently or light up a different LED, and so on. The booloader with F1 flash driver is successfully used on native, stlink, swlink, and is required to do USB DFU because F103 does not have the MaskROM (or OTGFS) of F107.

dragonmux commented 2024年07月13日 16:27:22 +02:00

There's a lot to unpack here, so we'll answer the point most relevant to PR #1866 and come back to the rest after.

Some platforms return a NULL char pointer: f4discovery, blackpill-f4, hydrabus, also launchpad-icdi.
Some platforms return a fixed string: 96b_carbon, f3, f072.
Some platforms implement sampling internal ADC: native (PB0), swlink, stlink, stlinkv3 (all PA0).
For this point I suggest 1) unifying the returned value accross the 7 platforms which don't do ADC, 2) catching that in command.c and remote.c and suppressing the output of unknown voltage.

As mentioned before on Discord, for the platforms that do not implement this feature, we would prefer them to return the string "Unknown" as they literally do not know what the target's voltage is. This creates the grammatically and linguistically correct display of "Target voltage: Unknown" and allows all the NULL handling in command.c around this function to be dropped. Additionally, then add a comment in platform_support.h against the function of something like /* Detect the current voltage the target is running at, or return the string "Unknown" if unimplemented in a given platform */ to document this as the intended API contract.

This option keeps the overheads down because any NULL handling the firmware has to do costs Flash space that will not get reclaimed by the compiler when it can see the checks aren't necessary. Detection for this string can then be compiled in to the scan machinery (optionally only for BMDA) to neaten things up if it winds up displaying as "Target voltage: Unknown Volt", so as to drop the " Volt" term. Because it would be a fixed string, comparing voltage == "Unknown" would /technically/ be ok.. though this is probably an ill-advised optimisation for BMDA as we can't guarantee the compiler will de-dupe strings there [eg: MSVC, where it's a compilation flag for link].

There's a lot to unpack here, so we'll answer the point most relevant to PR #1866 and come back to the rest after. > Some platforms return a NULL char pointer: f4discovery, blackpill-f4, hydrabus, also launchpad-icdi. > Some platforms return a fixed string: 96b_carbon, f3, f072. > Some platforms implement sampling internal ADC: native (PB0), swlink, stlink, stlinkv3 (all PA0). > For this point I suggest 1) unifying the returned value accross the 7 platforms which don't do ADC, 2) catching that in command.c and remote.c and suppressing the output of unknown voltage. As mentioned before on Discord, for the platforms that do not implement this feature, we would prefer them to return the string "Unknown" as they literally do not know what the target's voltage is. This creates the grammatically and linguistically correct display of "Target voltage: Unknown" and allows all the NULL handling in command.c around this function to be dropped. Additionally, then add a comment in platform_support.h against the function of something like `/* Detect the current voltage the target is running at, or return the string "Unknown" if unimplemented in a given platform */` to document this as the intended API contract. This option keeps the overheads down because any NULL handling the firmware has to do costs Flash space that will not get reclaimed by the compiler when it can see the checks aren't necessary. Detection for this string can then be compiled in to the scan machinery (optionally only for BMDA) to neaten things up if it winds up displaying as "Target voltage: Unknown Volt", so as to drop the " Volt" term. Because it would be a fixed string, comparing `voltage == "Unknown"` would /technically/ be ok.. though this is probably an ill-advised optimisation for BMDA as we can't guarantee the compiler will de-dupe strings there [eg: MSVC, where it's a compilation flag for link].

ALTracer commented 2024年09月03日 19:31:32 +02:00 (Migrated from github.com)

First problem dealt with in PR1866 and the corresponding ctxLink follow-up (those happened somewhat in parallel independently). Other problems still stand. Notably onboard stlinks reflashed to BMF will drive their jtagtaps in push-pull output mode, interfering with external debuggers (unless held in reset). The DFU flow minimally needs an answer with technical explanations for every applicable platform (no PR code editing action).

First problem dealt with in PR1866 and the corresponding ctxLink follow-up (those happened somewhat in parallel independently). Other problems still stand. Notably onboard stlinks reflashed to BMF will drive their jtagtaps in push-pull output mode, interfering with external debuggers (unless held in reset). The DFU flow minimally needs an answer with technical explanations for every applicable platform (no PR code editing action).