riscv: add flashstub framework and helper functions (riscv32 only)

[mean00]

Detailed description

Hi
This is a temptative patch to add some flashstub helper functions for the rv32 targets.
It tries to be very similar to the cortexm version. i.e.
Call riscv32_run_stub( target, loadaddr, param1, param2, param3, param4)
and the flashstub must end up with riscv_stub_exit(errorcode)

The main difference in behavior is the code saves and restore PC & MIE to avoid letting things dangling in ram.
Not completely sure this is needed, just to be on the safe side.

I've tested it with a CH32V307 board using the upcoming board & rvswd protocol support with a ~2x speedup.

As a sidenote and for the record, my original version was providing a temporary stack for the stub to avoid strict compiler rules about not using stack. Dont know if it is a valid idea. Nonetheless, this MR is assuming the stack is not used at all by the stub.

Thank you.

Your checklist for this pull request

• I've read the Code of Conduct
• I've read the guidelines for contributing to this repository
• [Builds till linking then out of flash ] It builds for hardware native (see Building the firmware)
• It builds as BMDA (see Building the BMDA)
• I've tested it to the best of my ability
  Tested with a BMP derivative using CH32V3xx chips. Cant test (yet!) with vanilla bmp
• My commit messages provide a useful short description of what the commits do

[dragonmux]

We'll dive in and review this tomorrow, but as for the stack thing - the stub can use stack, it just has to set one up itself - see the RP2040 stub in #1609 which does this to improve code density and size. It's not really viable for the stub runner to do this itself, and with the lui instruction in RISC-V, it would be trivial to load a 4KiB-aligned stack pointer in the stub.

[mean00]

Hi,
This MR exhibits an issue related to the CSR register management introduced earlier (my bad) because it manipulates some CSR directly.
The CSR registers are offset by RV_CSR_GDB_OFFSET to present them as regular register to gdb
It means that also when used internally, something like
t->reg_write(t, RV_CSR_MIE, &zero, 4);
should actually be
t->reg_write(t, RV_CSR_MIE+RV_CSR_GDB_OFFSET, &zero, 4);

This looks ugly and a bit error prone. Not sure what is the best way to deal with that.

[dragonmux]

Given that going through the target API there is just a convoluted way to access riscv_csr_read/riscv_csr_write - we would suggest that the logic use these functions directly, solving the need to deal with the GDB offset and specify the register width that way (you can use RV_CSR_FORCE_32_BIT with RV_CSR_MIE to ensure the right width access will always happen, or just assume because it's a RISC-V 32 part that it's the right width)

[mean00]

Indeed, thank you for the pointer, that's way better.

[mean00]

Updated accordingly.
Thank you.

[dragonmux]

This is looking good, though there are some items to address and talk about before this can be considered ready for merge. Most of the items are about formatting and code style though.

[dragonmux]

Please use target for the target_s * parameter name, we'd like not to introduce more clang-tidy lints when we've been trying to fix these.

Perhaps use loadaddr rather than codeexec, or at least codeaddr to more clearly indicate what this parameter is?

[dragonmux]

Please define each of these separately and with initialisation values. This is a clang-tidy lint.

[dragonmux]

This condition and the next look merge-able, and more importantly, look to be able to be simplified such that it removes the gotos (we'd really like not to introduce any, they make for horrible codegen and a horrid time understanding code flow.

For example:

if (reason == TARGET_HALT_REQUEST)
   ret = true;
target->halt_request(target);

Now, we have questions about ret as well and its (re)use here as that logic doesn't make sense right now, but that should also simplify the while loop too so as to just need a break; statement from that check for timeout expiry.

Please write comments detailing your intent with this code - it's very difficult to tell from the plain text of this code what you are intending to have happen here and why, vs what this actually does.

[dragonmux]

Please remove this trailing comment.

[dragonmux]

Given how this gets used and how small a function this is, but with it not marked inline, we expect this to be contributing a significant amount to the codegen for riscv32_run_stub() - if you grab the hart at the top of that function, you can then call the underlying read and write routines directly, which should result in leaner code generation. Same with the read function.

[dragonmux]

These need suffixing with U to make them unsigned. It looks like these should really belong in the riscv_debug.h header so all RISC-V debug code can benefit from them.

[dragonmux]

The braces here can be (should be) removed. Please also run clang-format as this is not formatted properly for this code base.

[mean00]

I think i've taken all or about all of the comments into account.

I've left the two helper functions as inline to have a simpler code. I guess they might be helpful later on.
The generated codesize should be the same.

Similarily i've let the error handling a bit detailed as to allow further more detailed use cases. It should not cause binary bloat.

( On a side note, clang-tidy 17 does not like much the .clang-tidy due to varying indentation and after fixing it complains really a lot )