target/nrf91: add mass_erase and recovery probe #1785

maxd-nordic · 2024-03-15T12:17:31Z

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

maxd-nordic · 2024-03-15T12:21:22Z

@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 · 2024-03-15T12:54:15Z

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.

src/target/nrf91.c

src/target/target_probe.h

src/target/nrf91.c

dragonmux · 2024-03-15T13:09:34Z

src/target/nrf91.c

 }

 bool nrf91_probe(target_s *target)
 {
 	adiv5_access_port_s *ap = cortex_ap(target);

-	if (ap->dp->version < 2U)
+	if (ap->dp->version < 2U || ap->dp->target_partno != 0x90U)


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 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.

src/target/nrf91.c

maxd-nordic · 2024-03-18T10:17:00Z

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 · 2024-03-19T08:55:54Z

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 · 2024-03-19T09:55:53Z

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 · 2024-03-19T11:55:01Z

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

maxd-nordic · 2024-03-22T14:21:34Z

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

Signed-off-by: Maximilian Deubel <[email protected]>

maxd-nordic · 2024-03-25T15:52:53Z

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.

dragonmux

Apologies this PR got neglected and the code review we'd started some time back in March didn't get posted. If you could please rebase this PR on main and fix the lint pass issues along with this new review, that would be greatly appreciated as we would like to be able to land this before v2.0

dragonmux · 2024-03-25T02:25:05Z

src/target/nrf91.c

+  0x5a, 0x02, 0xc3, 0xf8, 0x10, 0x2e, 0x03, 0x4b, 0x4f, 0xf0, 0x5a, 0x02,
+  0xc3, 0xf8, 0x00, 0x2e, 0xfe, 0xe7, 0x00, 0x00, 0x00, 0x90, 0x03, 0x50
+};
+unsigned int empty_app_len = 36;


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.

dragonmux · 2024-03-25T02:26:06Z

src/target/nrf91.c

+#define NRF91_UICR_APPROTECT_UNPROTECT_VAL 0x50FA50FAU
+#define NRF91_UICR_ERASED_VAL              0xFFFFFFFFU
+
+unsigned char empty_app[] = {


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).

dragonmux · 2024-03-25T02:27:38Z

src/target/nrf91.c

+		.apsel = 0x4U,
+	};
+
+	if (!nrf91_ctrl_ap_mass_erase(&ctrl_ap)) {


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.

dragonmux · 2024-10-24T02:13:19Z

src/target/nrf91.c

 		return false;

-	switch (ap->dp->target_partno) {
-	case 0x90:
+#ifndef ENABLE_DEBUG


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

dragonmux · 2024-10-24T02:16:54Z

src/target/nrf91.c

+#define NRF91_CTRL_IDR_EXPECTED      0x12880000
+#define NRF91_AHB_AP_IDR_EXPECTED    0x84770001


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.

dragonmux · 2024-10-24T02:19:48Z

src/target/nrf91.c

+	if (erase_needed) {
+		gdb_out("Skipping UICR erase, mass erase might be needed\n");
+	}


The braces are superfluous, please drop them.

dragonmux added this to the v2.0 release milestone Mar 15, 2024

dragonmux added the Enhancement General project improvement label Mar 15, 2024

dragonmux reviewed Mar 15, 2024

View reviewed changes

src/target/nrf91.c Outdated Show resolved Hide resolved

maxd-nordic force-pushed the nrf91-recover branch 3 times, most recently from 2d81846 to b1391c6 Compare March 18, 2024 15:57

maxd-nordic force-pushed the nrf91-recover branch from b1391c6 to a7a9b63 Compare March 22, 2024 14:19

target/nrf91: add mass_erase and recovery probe

8941178

Signed-off-by: Maximilian Deubel <[email protected]>

maxd-nordic force-pushed the nrf91-recover branch from a7a9b63 to 8941178 Compare March 22, 2024 14:30

dragonmux reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target/nrf91: add mass_erase and recovery probe #1785

target/nrf91: add mass_erase and recovery probe #1785

maxd-nordic commented Mar 15, 2024 •

edited

Loading

maxd-nordic commented Mar 15, 2024

dragonmux commented Mar 15, 2024

dragonmux left a comment

dragonmux Mar 15, 2024

dragonmux Oct 24, 2024

maxd-nordic commented Mar 18, 2024

dragonmux commented Mar 19, 2024

maxd-nordic commented Mar 19, 2024

maxd-nordic commented Mar 19, 2024

maxd-nordic commented Mar 22, 2024

maxd-nordic commented Mar 25, 2024

dragonmux left a comment

dragonmux Mar 25, 2024

dragonmux Mar 25, 2024

dragonmux Mar 25, 2024

dragonmux Oct 24, 2024

dragonmux Oct 24, 2024

dragonmux Oct 24, 2024

		#define NRF91_CTRL_IDR_EXPECTED 0x12880000
		#define NRF91_AHB_AP_IDR_EXPECTED 0x84770001

target/nrf91: add mass_erase and recovery probe #1785

Are you sure you want to change the base?

target/nrf91: add mass_erase and recovery probe #1785

Conversation

maxd-nordic commented Mar 15, 2024 • edited Loading

Detailed description

Your checklist for this pull request

Closing issues

maxd-nordic commented Mar 15, 2024

dragonmux commented Mar 15, 2024

dragonmux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxd-nordic commented Mar 18, 2024

dragonmux commented Mar 19, 2024

maxd-nordic commented Mar 19, 2024

maxd-nordic commented Mar 19, 2024

maxd-nordic commented Mar 22, 2024

maxd-nordic commented Mar 25, 2024

dragonmux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxd-nordic commented Mar 15, 2024 •

edited

Loading