adafruit / Adafruit_nRF52_Bootloader

USB-enabled bootloaders for the nRF52 BLE SoC chips
MIT License
445 stars 401 forks source link

Place data from .uicrREGOUT0 into UICR.REGOUT0 flash word. #139

Open geeksville opened 4 years ago

geeksville commented 4 years ago

The REGOUT0 flash register to set VCC voltage upon power-up. This adds support to allow optionally setting that register in the generated hex file (similar to the other UICR words you already support).

If not used, there is no change to the output files.

Example usage from my board's pinconfig.c file:

 __attribute__ ((section(".uicrREGOUT0"))) volatile uint32_t m_uicr_regout0 = 0xfffffff4;

(These are a few commits I thought might be useful after bringing up an open-source radio board - I can send in the changes for that board if you want it (once the next spin comes back and we test it)?)

rafacouto commented 4 years ago

Warning: REGOUT0 also sets the voltage level for GPIO output. It could be problematic when reflashing the MCU by SWD pins...

geeksville commented 4 years ago

@hathach, sure I'll do that.

@rafacouto, yep - this can definitely break things, but it is also absolutely required on some boards because vcc powers other important things. I'll add a warning to not use this unless you are careful (and example usage) in a comment.

reflashing is still okay if the user connects their vref input on their ICE to the VCC rail.

hathach commented 4 years ago

Warning: REGOUT0 also sets the voltage level for GPIO output. It could be problematic when reflashing the MCU by SWD pins...

I ddin't know it is that dangerous. I will double check the manual. As long as the output binary is the same without using the section. It should be still good/safe to go. @geeksville Maybe you could drop a line of comment for warning in the linker script. Since existing boards don't use it at all.

geeksville commented 4 years ago

yep - warning is in that linker script. Yeah - when not populated, nothing is written to that address so the flash stays at the default 0xffffffff.

Nicell commented 3 years ago

@geeksville thanks for adding this! This is immensely useful for boards that run with VDDH but need a higher GPIO logic level than 1.8V (which is the default). I've hacked this together in the past, but this is a much more proper implementation.

I don't really see how this would be dangerous. If anything, the default 1.8V is the dangerous piece. I've spoken with a few people that have been unable to program their boards due to that default logic level. @hathach could you take a second look at this? I think it's a great improvement to this project.

lyusupov commented 3 years ago

This PR functionality is already superseded by merged https://github.com/adafruit/Adafruit_nRF52_Bootloader/pull/177

Just simply add:


#define UICR_REGOUT0_VALUE UICR_REGOUT0_VOUT_3V3

into your board.h file.

But be aware of this particular PR as well: https://github.com/adafruit/Adafruit_nRF52_Bootloader/pull/186

hathach commented 3 years ago

I really like the solution that is implemented to have define in the board.h. Therefore this PR could be safely close by now.

#define UICR_REGOUT0_VALUE UICR_REGOUT0_VOUT_3V3
geeksville commented 3 years ago

@hathach, @nicell and @lyusupov I think you might want to think twice about that other PR. It seems to me that writing UICR later after the first boot (as that other change does) will not work for many applications.

Because some boards have critical components powered by the VCC output rail from the CPU. And without that rail powered at the proper voltage the first boot might not be possible at all. Which is part of why I suspect nordic put this in the flash config registers.

lyusupov commented 3 years ago

There is nothing new in the #177 / #186 method. This is typical solution for developers to use suggested by Nordic DevZone forum reporters. Moreover, Zephyr project is using this method together with Nordic reference design USB dongle

hathach commented 3 years ago

@geeksville would you mind testing to see if that first boot, method work with your board. I kind of like e it more because of its simplicity and configurable right within the board.h . Sorry for late response, I have been occupied by other works

geeksville commented 3 years ago

@hathach alas - my queue is kinda full right now with other things also (and the board I used this on still hasn't been shipped by the mfg). But there is a reason that nordic put this setting in a special flash register (rather than just a normal CPU register), which is "the problem of critical components being powered by VCC." The solution in 177 is sub-optimal (and might make some boards impossible to properly power sequence) because it guarantees that at least one boot (the first one) has incorrect VCC voltage before trying to run code.

Nordic put REGOUT0 in flash so that the processor could properly configure VCC before running even the first instruction.

The other patch is not 'wrong' but I'm just letting you know it might not be sufficient for all boards. IMO a cleaner solution is to let the linker put the correct word into the flash image directly. And it is quite elegant because it shows up as just another optional constant you can put into your C code.

You could also just use it based on a constant from board.h. Without the nasty second boot hack and writing flash from the bootloader.

__attribute__ ((section(".uicrREGOUT0"))) volatile uint32_t m_uicr_regout0 = UICR_REGOUT0_VALUE;
hathach commented 3 years ago

Thanks @geeksville , to be honest, I am not entirely sure on this matter, but didn't have time to test this as well. The pico and other works take higher priority now. I will re-open this, maybe we could come back later when having more time and/or as reference/solution for others that get into trouble that you just mentioned. Thanks again

dotnfc commented 3 years ago

I really like the solution that is implemented to have define in the board.h. Therefore this PR could be safely close by now.

#define UICR_REGOUT0_VALUE UICR_REGOUT0_VOUT_3V3

create a Makefile.user with:

CFLAGS += -DUICR_REGOUT0_VALUE=UICR_REGOUT0_VOUT_3V3

for pca10059, 3.3V output OK.

this will not touch the board.c