imxrt: ungate boot_mode even on non-hosted

[xobs]

Detailed description

When building debug mode for hardware, the boot_mode flag is consulted. However, the boot_mode flag is only present when debug mode is enabled on PC_HOSTED.

Remove the requirement for this to be built on PC_HOSTED in order to fix compiling when built on native hardware with debug mode enabled.

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

[dragonmux]

This macro should resolve to a noop on not-BMDA so should cause boot_mode to be considered unused (which is a -Werror'd warning). That suggests the #if check should still check for PC_HOSTED == 1 as all DEBUG_TARGET() output only actually exists in BMDA.

[xobs]

On Farpatch we send DEBUG_TARGET() to the ESP-IDF logging stream, which is helpful.

Is it not desirable to enable debug prints on a build that is not PC_HOSTED?

[dragonmux]

Ah, that's actually very helpful if you have the Flash space, yes. The platforms that are in-repo do not because we do not have sufficient Flash available on the majority of the targets, so DEBUG_ERROR(), DEBUG_WARN() and DEBUG_INFO() are the only 3 that get included in an ENABLE_DEBUG=1 firmware binary usually as even that uses vast amounts of space and requires disabling targets to get to fit.

This is also how the in-repo machinery in src/include/general.h works:

blackmagic/src/include/general.h

Lines 74 to 76 in 799a408

	#define PRINT_NOOP(...) \
	do { \
	} while (false)

blackmagic/src/include/general.h

Lines 86 to 90 in 799a408

	#define DEBUG_GDB(...) PRINT_NOOP(__VA_ARGS__)
	#define DEBUG_TARGET(...) PRINT_NOOP(__VA_ARGS__)
	#define DEBUG_PROTO(...) PRINT_NOOP(__VA_ARGS__)
	#define DEBUG_PROBE(...) PRINT_NOOP(__VA_ARGS__)
	#define DEBUG_WIRE(...) PRINT_NOOP(__VA_ARGS__)

We'll need to include some more complicated #if logic to allow Farpatch to continue including DEBUG_TARGET() in firmware and we weren't aware it was doing that to avoid breaking it.

[xobs]

This is how it's done in Farpatch:

https://github.com/farpatch/farpatch/blob/835ba05bcf92e42cfa0ccbb28bf6a0fa6be5f232/components/blackmagic/general.h#L71-L86

(I have to reimplement the ESP macros here but without LF since the BMP logging system always appends \n as part of their logging macros, but aside from that it works well.)

How would you like this patch to change?

[dragonmux]

Because they need to expand as block statements, we suggest https://github.com/farpatch/farpatch/blob/835ba05bcf92e42cfa0ccbb28bf6a0fa6be5f232/components/blackmagic/general.h#L89-L92 gets modified to use PRINT_NOOP(), as an aside.

Regarding this patch, we're going to suggest as the guard:

#if defined(ENABLE_DEBUG) && (PC_HOSTED == 1 || defined(ESP_LOGW_BMP))

We think that'll prevent problems in building the firmware on the in-repo platforms while preserving the behaviour Farpatch needs.

[xobs]

ESP_LOGW_BMP is somewhat of an implementation detail, and I hope it will go away when a new logging mechanism is introduced in BMP.

I've changed it to ESP_LOGD, which is the macro for debugging, and exists in all ESP builds.

[dragonmux]

LGTM, sorry for having broken Farpatch quite so flagrantly!

[ALTracer]

when built on native hardware

Is your platform supported in-tree by blackmagic?
In Blackmagic org terms, native is a very specific family of hardware platform variants historically sold by 1BitSquared, these are based on 128k Flash parts and usually can't fit all the code pulled in by ENABLE_DEBUG=1, that's why a) this makeflag option exists; b) different loglevels are routed to PLATFORM_PRINTF() or PRINT_NOOP() because apparently devs don't want DEBUG_TARGET in their probe firmwares (but you do).

However, when testing on HEAD~1 of main I see only (172+352)=524 bytes Flash increase from enabling TARGET_DEBUG. I could dedupe some (16*4-2=78) bytes from 5 similar strings in imxrt.c, if deemed necessary, see my previous PRs, they are few.

.text=105184 .rodata=33248 .data=432 .bss=3928 ENABLE_DEBUG=1 TARGET_DEBUG=1 (PLATFORM_PRINTF)
.text=105012 .rodata=32896 .data=432 .bss=3928 ENABLE_DEBUG=1 without TARGET
.text= 97200 .rodata=21356 .data=392 .bss=3628 (NO DEBUG)

To @dragonmux: reconsider enabling DEBUG_TARGET loglevel for probes because after ENABLE_DEBUG=1 it requires 8k .text + 11k .rodata = 19k of Flash anyways, and in light of this another 524 bytes shouldn't be a problem. It's used in imxrt, rp, but also spi (in mass_erase) outside of hot paths, usually in flash & detection code. This alternative fixes debug builds.

To @xobs: when writing #1517 I kept in mind only the official/supported/"foreign" in-tree platforms covered by make all_platforms, and I didn't know Farpatch existed, apparently it's a thing with ESP32-based wireless SoCs. Maybe I should be apologizing to you for breaking your debug builds, but you decided to go a separate way about debug loglevels selection, and all this situation frankly can't be resolved neatly without refactoring all the logging.

And I won't be apologizing because you just broke probe ENABLE_DEBUG=1 builds for me which I have been using very frequently for the last month or so on blackpill-f4 hardware with 512k Flash (so fits) during implementing new target support, enhancing platform support (DFU, SPI) and other general tinkering. Your diff should be saying

diff --git a/src/target/imxrt.c b/src/target/imxrt.c
index 7d4eab1d..e17c933c 100644
--- a/src/target/imxrt.c
+++ b/src/target/imxrt.c
@@ -172,7 +172,7 @@ bool imxrt_probe(target_s *const target)
        target->target_options |= CORTEXM_TOPT_INHIBIT_NRST;
        target->driver = "i.MXRT10xx";
 
-#if defined(ENABLE_DEBUG) && PC_HOSTED == 1
+#if defined(ENABLE_DEBUG) && (PC_HOSTED == 1 || defined(ESP_LOGD))
        const uint8_t boot_mode = (target_mem_read32(target, IMXRT_SRC_BOOT_MODE2) >> 24U) & 3U;
 #endif
        DEBUG_TARGET("i.MXRT boot mode is %x\n", boot_mode);

so it's enabled either on BMA/hosted or on your ESP hardware but never on BMD platforms, which don't like TARGET_DEBUG as of now. You seem to have introduced a typo mismatching the review feedback.

PR CI doesn't catch this because it's not easy simultaneously passing a makeflag and dropping a bunch of target files from GH Actions build, but it could be implemented for bigger flash platforms, like the ENABLE_RTT=1 case. I had similar problems with NuttX where they merge fixes without testing under CONFIGDEBUG settings._

Also ESP_LOGD is mentioned zero times in BMF codebase, maybe leave a comment relating this to Farpatch taking different design decisions.

[dragonmux]

@ALTracer, Farpatch is a product that xobs designed and which is out of tree but that we try, where possible, not to break - especially as he's a much appreciated long-time contributor. The reason you don't find the log macro in-tree is because it's not in tree. It's in the Farpatch tree which is based on BMD.

This doesn't enable DEBUG_TARGET in-tree other than BMDA for in-tree platforms. This only makes it possible for Farpatch to.