target/nrf91: add mass_erase and recovery probe #1785

Please prefix these with NRF91_ to maintain the macro prefixing for the file.

This uses int for a boolean return value, please use bool instead. We're in C11 here, so <stdbool.h> functions properly, giving access to clean error returns. Note, we would strongly prefer true to mean success otherwise the caller side logic becomes cumbersome and difficult to comprehend, which is error-prone.

Please use PRIx32 for the format specifier, not x, as idr is a uint32_t and so this doesn't work on Xtensa or several desktop platforms.

eg: DEBUG_ERROR("nRF91: AHB-AP IDR is 0x%08" PRIx32 ", expected 0x84770001\n", ahb_ap.idr);

It would also be better for Flash space if the expected value were formatted in the same way.

This would be fine as a DEBUG_* statement, but should be avoided in the scan output as it's not guaranteed where exactly this will end up in the information display, while also duplicating the information to the user from the target->driver strings.

Same comment as below on uint32_t needing PRIx32.

Please avoid a magic number here - 0x90U should ideally be #define'd above as, eg, ID_NRF91 so it's clearer what it is and where it comes from. Ideally a reference to the TRM section and page this information can be found from would be great, but we know that's not always possible.

Please avoid single letter variable naming for this, please use target instead. Likewise for nrf91_rescue_probe() on return from target_new().

This is the style we've been gradually moving the whole code base to.

This can be made const, so lets do so.

const here will be ignored by the compiler. This should be a clang-tidy lint. Having it in the function definitions is correct though.

Easiest way to fix this would be to gdb_outf() a message like "Skipping UICR erase, operation requires mass erase". If there's a good way to blank check the UICR that can be used to have this routine return true with no message, and then return false if the UICR is not blank.

These are missing their U suffixes to ensure they're defined as unsigned values. Because the AP IDR value (as an example) sets the sign bit on 32-bit platforms, if this value is then widened for any reason, then the upper bytes of the new value will be all set high, and if it is narrowed, this invokes UB (due to changing the value of the sign bit). Neither of these problems occur with unsigned constants.

Please do not use raw types. uint8_t if you want to make this unsigned (and then U-suffix every value in the array initialisation so they're unsigned and won't get sign extended).

Please use ARRAY_LENGTH() and make this a #define. This is not a constant expression in C and so uses Flash space unnecessarily to store a full variable. something like #define NRF91_EMPTY_APP_LEN ARRAY_LENGTH(empty_app) would be appropriate.

The braces are superfluous, please drop them.

Please drop these braces as they are unnecessary. The code base style is for single statement bodies of conditional block to not be enclosed in braces.

This wants to be #ifndef DEBUG_INFO_IS_NOOP to correctly guard these memory read-backs.

Please note that the choice of suggesting ID_NRF91 is for consistency with all the other target support implementations - while this is at least now a define that describes what the magic number is, it would be best to have it use the same style as the other target implementations.