-
-
Notifications
You must be signed in to change notification settings - Fork 770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
target/nrf91: add mass_erase and recovery probe #1785
base: main
Are you sure you want to change the base?
Conversation
@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. |
The post-Flash write and mass-erase UICR fix-up sounds like something that should be a target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
|
||
return true; | ||
} | ||
|
||
int nrf91_dp_prepare(adiv5_debug_port_s *const dp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/target/nrf91.c
Outdated
ctrl_ap.idr = adiv5_ap_read(&ctrl_ap, ADIV5_AP_IDR); | ||
|
||
if (ahb_ap.idr != AHB_AP_IDR_EXPECTED) { | ||
DEBUG_ERROR("nRF91: AHB-AP IDR is 0x%08x, expected 0x84770001\n", ahb_ap.idr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/target/target_probe.h
Outdated
@@ -98,5 +98,6 @@ bool stm32mp15_cm4_probe(target_s *target); | |||
bool zynq7_probe(target_s *target); | |||
|
|||
void lpc55_dp_prepare(adiv5_debug_port_s *dp); | |||
int nrf91_dp_prepare(adiv5_debug_port_s *const dp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
here will be ignored by the compiler. This should be a clang-tidy
lint. Having it in the function definitions is correct though.
src/target/nrf91.c
Outdated
|
||
ap->dp->mem_read(ap, buf, 0x00FF0140, 3*sizeof(uint32_t)); | ||
|
||
gdb_outf("nRF%04x %4s%4s detected!\n", buf[0], (const char *)&buf[2], (const char *)&buf[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/target/nrf91.c
Outdated
} | ||
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/target/nrf91.c
Outdated
#define CTRL_AP_RESET ADIV5_AP_REG(0x000) | ||
#define CTRL_AP_ERASEALL ADIV5_AP_REG(0x004) | ||
#define CTRL_IDR_EXPECTED 0x12880000 | ||
#define AHB_AP_IDR_EXPECTED 0x84770001 | ||
#define CTRL_AP_ERASEALLSTATUS ADIV5_AP_REG(0x008) | ||
|
||
#define UICR_APPROTECT 0x00FF8000U | ||
#define UICR_SECUREAPPROTECT 0x00FF802CU | ||
#define UICR_APPROTECT_UNPROTECT_VAL 0x50FA50FAU | ||
#define UICR_ERASED_VAL 0xFFFFFFFFU |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prefix these with NRF91_
to maintain the macro prefixing for the file.
src/target/nrf91.c
Outdated
|
||
|
||
|
||
static bool nrf91_rescue_do_recover(target_s *t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/target/nrf91.c
Outdated
//check if CSW DEVICEEN is set | ||
struct adiv5_access_port ahb_ap = *ap; | ||
ahb_ap.apsel = 0x0U; | ||
uint32_t csw = ap->dp->ap_read(&ahb_ap, ADIV5_AP_CSW); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be made const
, so lets do so.
src/target/nrf91.c
Outdated
static bool nrf91_uicr_flash_erase(target_flash_s *flash, target_addr_t addr, size_t len) | ||
{ | ||
/* can't erase UICR without a mass-erase */ | ||
/* TODO: this will silently fail if you actually need to erase */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
2d81846
to
b1391c6
Compare
The reason we said "and at the end of the mass erase function" is that it's up to the target support too override |
|
I got to check the flash write function again - we can skip the readynext wait if the page is erased beforehand. |
b1391c6
to
a7a9b63
Compare
@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]>
a7a9b63
to
8941178
Compare
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. |
Detailed description
Your checklist for this pull request
Closing issues
fixes #1778