Feature: add new targets: hpm53xx,hpm67xx,hpm68xx,hpm6pxx #2228

Please drop the parens around all these single item constants - nothing else in the code base does this, so please don't cargo cult you own style in here.

OK, I have removed the parentheses for the single constants.

NB to previous note: parens in situations like this are fine - there are two things with an operator in the define - that's what the parens are there to ensure order of ops for when substituted.. but doing it for all constants is the thing we don't do in BMD.

As mentioned before, the suggested changes have been implemented.

Please use the code base multi-line comment style:

/*
 * A multi-line
 * comment
 * looks like this
 */

Please note the start of line *'s aligned to the start and end of comment ones. This and the next comment should just b one signle comment with some sort of explainer as to what this table means and why it's here - we can infer it's probably because of the constants below, but there's no clear through line to them and thus what those constants actually mean and so why this block is here.

In the new code, the format of multiple lines of comments has been changed.

Same note about multi-line comment style.

As mentioned before, the suggested changes have been implemented.

Please don't define anonymously named structs - struct hpm_chip_id - please name it.. this matches the typedef. Structures in BMD are typedef'd with _s suffixing their name, making the typedef for this hpm_chip_id_s please.

Thank you. This change has been implemented.

The more commonly used term for this is sentinel - also please include the trailing , after the closing } for this struct instance so clang-format doesn't reformat this badly.

Thank you. This change has been implemented.

This inner if is considered a single-line statement, so please drop its braces ({}) - however the for loop outside is not because there are two statement lines within the {} (yes, we know technically the if counts as one syntactically.. we're talking visually) so its braces remain.

OK, it has been changed.

Same thing about anonymous structs, and please put all struct definitions together before the first function.

OK, it has been changed.

size, not sz please - total_size_bytes, or just total_size is more than fine for a name here. Cutting vowels out is not useful and just leads to confusion for other readers unfamiliar with your choice of vowels to drop.

Changed

Rather than embedding a giant blob of code, please use the flashstub directory and mechanism and use that to provide the data to embed here. Source for this blob is not optional for it to be able to be included in BMD.

Thank you for reminding me of the existence of the flashstub mechanism. I have now begun to implement it in the initial stage.

Please move this down to where the variable is first used - line 370.

This part of the code has been restructured.

Please move this down to the loop it's used in. Please prefer for looping than do-while'ing.. the for version of the loop below has the more comprehensible form:
for (uint32_t timeout = 0U; timeout < HPM_ALGO_CMD_TIMEOUT; ++timeout) which is much easier to understand than a do-while as written. If timeout is absolutely required outside that loop, then define the variable right above the loop, leaving the init section of the for blank.

Part of the reason for this is that x++ has side-effects, thus x++ < value has side-effects and this winds up being surprising to the reader - we want to avoid that kind of susprise as much as possible.

This part of the code has been restructured and is written in accordance with the similar code in the repository.

Rather than putting out what feels like machine names etc.. please output a header line such as Config:, and use more huan names such as XPI Header:, Flash Base:, etc.. do not worry about aligning everything.. it's a use of Flash space that is better avoided, especially with the fact all these strings wind up with extra spaces on the front of them. Given they're all PRIx32, please add 08 to the format specifer, as in %08 so they print at a fixed width.

I have cancelled this command because it should be user input. If there is a need to add it again in the future, I will re-enable it.

This builds on the previous variable, so just += - there's no point fully reloading the variable like this.

This part of the code has been refactored into hpm_run_stub.

Same comment about aligning and using hex outputs that aren't formatted to a fixed width.

I have cancelled this command

This run-and-loop pattern appears in at least two places - please refactor into a function to be called so it can be named, and maintained just once.

This part of the code has been refactored into hpm_run_stub.thank you for your reminder.

Extra space at the end of the message. Please simplify the message down a touch to something like Invalid arguments - the user already knows what monitor command the typed to get here.

Changed

This return is impossible to hit because of teh above if check.. so, just leave it an empty default block.

Changed

This monitor command list should be defined nearer the top of the file - forward declaring the monitor functions is not only recomended, but preferred, with monitor functions appearing after everything else in the file.

Changed

This and the function above should be the two first functions to appear in this file. Please use forward declarations of functions to make that happen. See how something like stm32h5.c or similar is organised.

OK, it has been changed.

It's impossible to get here and for this check to fail - riscv_hart_struct() cannot fail at this point. There's no need to check for NULL on the result.

OK, it has been changed.

Please drop the braces - it's a single-statement if.

OK, it has been changed.