Increase performance of SWD and JTAG bitbanging #1688

We'll have to do some captures and testing with this attribute to make sure it's not shooting the timings in the foot - as you note in the PR description. We do note that this particular function signature still has the old ticks nomenclature in it, so please switch that to clock_cycles.

This is a great shout - if you could please move the comment explaining what this is doing up to this, you can then eliminate the preprocessor guarding and the old code.

Of these two forms, we prefer this one. Please see our notes in timing_stm32.c's platform_delay_busy on the matter of __asm__("nop") vs continue. The same query and notes apply here as there.

Please include a comment on what this is doing - we don't want to assume that the reader has seen the other uses of this and knows what's going on. Is there any particular reason for the inserting of a no-op rather than only a reordering barrier?

The cppreference page on C's volatile does not mention anything about forced stack usage and we've never seen it imply any such - are we sure that is what the compiler's doing here? Either way, we would strongly prefer you to use a for loop not a do-while here. This is a for loop in disguise.

The disappearance of the loop when using continue; without volatile can be entirely explained not so much by DCE (it's the result, not the cause) but by there being no observable side effects thus allowing the compiler to assume that nothing of value is done by the loop. This is the reason for using volatile in the jtagtap code. Perhaps we can use volatile register to force the compiler to consider the operation to have observable side-effects but also keep the loop counter register-bound?

Same comments as in jtagtap.c

As this is only used in the one place and is bitwise masked, perhaps lets make this const uint8_t parity and put the bit mask on the end, lifting it from the gpio_set_val() call below? Failing that, we should make it uint32_t so we don't do a signed+bitwise oops below.

Please insert a note here about this being a reordering barrier - same as in other places the trick is used.

Indeed volatile register was the first thing I tried, only to be yelled at by the compiler to swap qualifiers... and identical codegen. I specifically wanted to avoid extraneous ldr/str of loop counters, so I monitored Puncover and objdump -CSd. Maybe Compiler Explorer can shed some light on why this codegens this way.

Oh how interesting - ok, that'll be an interesting session in CE then as yes, that might well provide some insight into how and why the compiler's choosing to do what it does.

Probably qualifying a variable as volatile forces the compiler to assume that it should be observable elsewhere, and hence needs to be pinned to SRAM (if only on the stack). This directly contradicts with the intent of register-allocated counters (which debuggers don't display due to "optimized away" but you can always dump the gp_regs). Something "dead store elimination".
And the for-loop form is longer and has an extra jump, I didn't subjectively like it; objectively it has to fit into Flash prefetch buffer which is a few (4) 64-bit lines on F103 (?) to not incur wait-states of 72MHz MCU against 24MHz flash on random jumps.

From our perspective re the for loop part, the for loop form should code gen to the same or very near the same as the do-while (perhaps needs the condition expressed as --counter > 0 and --counter in the update step removed to do it) while being a much clearer expression of your intent. Think this is very much going to be "take it to Compiler Explorer and tinker for an hour" to find the right thing to do and the right way to express this.

Zooming out: do we actually care about 2..32 number of bits, or just the SWD parity per IHI0031 B4.1.6? The same 4698a26ba introduced the two builtin calls, which was a better idea than spinning the parity manually. And I see an opportunity to eliminate either __popcountsi2() or __paritysi2(), currently both are used, only once. They are 32+40 bytes, (36+52 depending on platform Thumb2 availability; popcount is bigger). GCC sees we don't care in seq_out and substitutes parity in place of popcount (because of the LSBit masking), but doesn't see in seq_in and leaves popcount alone.

750 kHz of tdi_tdo_seq on native was unremarkable, I hope performance improves -- please note, during capture/analysis, whether a given hardware relies on ancient FLITF Prefetch (2x64-bit lines) or runs from "0-wait-state" shadow SRAM.
Updated jtagtap_tms_seq() declaration and definition.

Your suggestion of --counter > 0 gets the codegen I want and the C idiomatic form you look to preserve, thanks.
Updated the three places accordingly.
m3/m4/m7 still get the SUBS+BNE.N, m0 (f072) gets subs; cmp; bne.n so its divisor timings will be off.

Updated as such. Note that it would be ADIV5_SWD_TO_JTAG_SELECT_SEQUENCE if the unit included adiv5.h -- this magic number also occurs in hosted/ftdi_jtag. That one will use _next().
I assume the jtagtap_cycle() method is a somewhat recent addition -- it's not used widely across the codebase, if only in BMPremote. I can even eliminate it as dead code for ADIv5 and RISC-V DTM targets, breaking the v0/v2 remote contract; unsure about AVR PDI. It's also too fast in comparison to other methods.

We only care about the single bit of parity (__builtin_parity(tms_states)), not the rest of the value of popcount.

This kind of thing is why we've been thinking on how on earth to lift these builtin calls into their own function in one of the buffer_utils.h or maths_utils.h headers, as we should really provide: versions for each supported compiler; a fallback implementation for compilers that do not have the required builtin (just as with ulog2 which uses the exact paradigm we're wanting to switch to).

As we've got a #define for it in that header, include the header - that's fine. We should look to reduce the number of places this constant is defined down to the one, documented, and global place ideally.

Correct, jtagtap_cycle() was introduced in v1.8 and the BMP remote protocol implementation may need tweaking to provide a jtagtap_next()-based fallback, but we're principally fine with that as a pivot as this is exactly the kind of thing it was introduced for from what we can tell from Uwe's PR and commit for it.

Thread resolved by merging (and rebasing over) PR1700.

Updated delay loops to --counter > 0 form.

Annotated for the readers. This routine is too fast in comparison to others, because it has nothing to do besides toggle the jTCK pin, and it certainly does not move data into (or out of) BMP. I may even want to add more nops or something similar to tone down this one.

Updated the delay loop to --counter > 0U;) form.

Added a comment. Both swdptap_seq_in_no_delay() and swdptap_seq_out_no_delay() need careful consideration in code influencing bitbanged timings, including effect of gpio_set()/clear() or gpio_set_val() and gpio_get().
I would like to see this entire routine fit into ART cache of STM32F4, or at least not jump around thanks to all the gpio_* bits inlined.

We went checking and the remote protocol has this fallback - apparently we thought about this when rewriting BMDA's implementation. We should verify fallbacks for the other adaptors are present, but as most of them outright replace this init code we don't see that being a problem as such - though it is a target for potential future dedupe in the backends.