target/nrf91: add mass_erase and recovery probe

[maxd-nordic]

Detailed description

• add nrf91 rescue probe
• add nrf91 mass erase
• add detailed nrf91 identification
• add nrf91 UICR writing
• add UICR APPROTECT initialization
• add UICR overwrite warning (recommend mass erase if value cannot be written)

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 (see Building the firmware)
• It builds as BMDA (see Building the BMDA)
• I've tested it to the best of my ability
• My commit messages provide a useful short description of what the commits do

Closing issues

fixes #1778

[maxd-nordic]

@dragonmux I added most of the missing parts for nrf91. Maybe you could give me pointers on how to handle the TODOs? The UICR APPROTECT part basically needs to do a flash write after each mass-erase to tell the "hardenend APPROTECT" system not to lock down the device after the next reset. This is also relevant for nRF53 support.

[dragonmux]

The post-Flash write and mass-erase UICR fix-up sounds like something that should be a target exit_flash_mode function and then call that at the end of the nRF91 mass erase implementation too - that should take care of the problem.

[dragonmux]

Done an initial review, there are a lot of interesting details here that we're very grateful to get to know about, like the TARGETID revision number being used for once in a part and having bearing on the part number.

Please don't forget to run clang-format on the contribution and fix the few hex constant capitalisation lints - the clang-format lint pass is currently broken but we are still enforcing it.

[dragonmux]

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.

[dragonmux]

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.

[dragonmux]

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

[dragonmux]

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.

[dragonmux]

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.

[dragonmux]

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

[dragonmux]

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.

[dragonmux]

This can be made const, so lets do so.

[dragonmux]

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.

[maxd-nordic]

The post-Flash write and mass-erase UICR fix-up sounds like something that should be a target exit_flash_mode function and then call that at the end of the nRF91 mass erase implementation too - that should take care of the problem.

Thanks! Got it to work now. Just putting it in exit_flash_mode is enough IMO because a blank app will also cause the protection to engage.

[dragonmux]

The reason we said "and at the end of the mass erase function" is that it's up to the target support too override target->mass_erase with its own implementation (it's NULL by default) and the implementation will only do what you put in the function - so if you need to call target->exit_flash_mode then you'll need to include that in the implementation specifically. We'd be concerned for newly mass erased parts winding up protected without it.

[maxd-nordic]

The reason we said "and at the end of the mass erase function" is that it's up to the target support too override target->mass_erase with its own implementation (it's NULL by default) and the implementation will only do what you put in the function - so if you need to call target->exit_flash_mode then you'll need to include that in the implementation specifically. We'd be concerned for newly mass erased parts winding up protected without it.

nrfjprog flashes a placeholder app onto the target when doing recovery. Should I include that here as well? (it would use up quite a bit of space) Just writing UICR is not enough to keep protection off. The way it's implemented now, you can do a mass-erase and UICR is written alongside your app.

[maxd-nordic]

I got to check the flash write function again - we can skip the readynext wait if the page is erased beforehand.

[maxd-nordic]

@dragonmux I managed to make the app image much smaller and include it here - the target is now unprotected after mass-erase. :)

[maxd-nordic]

It might actually be better to implement target commands and show the CTRL-AP in any case. There is also a rarely-used feature that could lock mass-erase with a password.