Skip to content
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

Fix RTC backup register indexing, add target-specific register and boot command #42

Merged
merged 2 commits into from
Sep 11, 2021

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Sep 6, 2021

Continuation of my bug hunting and feature implementation round started in #41.

According to ST's RM0008 STM32F103xx reference manual section 6.4.5 table 17 the backup registers are 32 bits wide with only the lower 16 bits being used for storing values. In addition to this, the "first" register slot is entirely reserved and there is no backup register zero, meaning that BKP_DR1 starts from an offset of 0x04. This was mostly correctly reflected in backup.c, but the 2-multiplier for the register enum throws everything off by a lot, which results in the boot command check failing to work correctly when using anything but the first backup register. BKP0 has also been removed since as described by RM0008 indexing starts at one (BKP1 is now fully equivalent to the old BKP0).

I've also gone ahead and implemented support for specifying the backup register and boot command to use per-target. If not defined, options equivalent to the old behavior are used to not break existing applications.

…ot command

According to ST's RM0008 STM32F103xx reference manual section 6.4.5 table 17
the backup registers are 32 bits wide with only the lower 16 bits being used
for storing values. In addition to this, the "first" register slot is entirely
reserved and there is no backup register zero, meaning that BKP_DR1 starts from
an offset of 0x04. This was mostly correctly reflected in backup.c, but the
2-multiplier for the register enum throws everything off by a lot, which
results in the boot command check failing to work correctly when using anything
but the first backup register. BKP0 has also been removed since as described by
RM0008 indexing starts at one (BKP1 is now fully equivalent to the old BKP0).

I've also gone ahead and implemented support for specifying the backup register
and boot command to use per-target. If not defined, options equivalent to the
old behavior are used to not break existing applications.
@devanlai
Copy link
Owner

devanlai commented Sep 7, 2021

I probably should have better documented what I was doing with the backup registers here, sorry about that.

I originally started using the backup registers for DAP42 on an STM32F042 target (where the application instead uses the backup registers to check whether to trigger the ROM bootloader). When I later ported DAP42 to the STM32F103 and started work on dapboot, I wanted to make the backup register control for the STM32F103 look just like the backup register control for the STM32F042. On the STM32F0 series, the backup registers are 32-bits wide, so the backup implementation for the STM32F103 mimics that, using two physical 16-bit backup registers to implement one logical 32-bit backup register. Thus there are five logical backup registers 0-4 rather than the 10 physical backup registers 1-10 that actually exist on the STM32F103.

As far as I can tell, this already worked okay, or at least it worked for me when testing with logical backup registers 0, 1, and 4 (using physical registers 1 and 2, 3 and 4, and 9 and 10 respectively).

I like the idea of allowing for selecting other backup registers to use, though I had originally figured that anyone that wanted to customize the bootloader backup registers would also probably want to change it to use a single physical backup register instead of two, anyways.

@twelho
Copy link
Contributor Author

twelho commented Sep 10, 2021

The STM32F103xx series actually has 42 backup registers, but that would quite severely bloat the BackupRegister enum if I would write them all out. I went up to 10 since that is what BTTSKRMINIE3V2 in #44 requires, and I think if someone needs more they can open a new PR to add them later.

Since the bootloader targets the STM32F103xx series MCUs it only makes sense to use their register naming IMO. If both the bootloader and the application refer to a different memory location with e.g. BKP10 it will only cause needless debugging of why the MCU didn't stay in the bootloader when the application told it to (as happened in my case).

Do you want me to change stm32f103/backup.c to use a single physical backup register instead of two? The bootloader using two registers is a bit unexpected for many applications targeting the STM32F103xx series. Moving to a single backup register should technically not break backwards compatibility either, the second register written by the application will simply not be considered by the bootloader anymore, but functionality will (in most cases) remain the same.

This is behavior is more akin to what most STM32F103xx applications wanting to
enter the bootloader are accustomed to. Should retain backwards compatibility
with existing applications writing to both registers, the higher one just won't
be checked by the bootloader anymore.
@twelho
Copy link
Contributor Author

twelho commented Sep 10, 2021

30e54d0 now contains the change to only use one (the first) backup register. LMK if this is acceptable or if it's better to keep the old behavior.

Copy link
Owner

@devanlai devanlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these changes, it looks generally okay to me, though I'll want to do some testing tomorrow before merging it.

@devanlai devanlai merged commit d1428ca into devanlai:master Sep 11, 2021
@twelho twelho deleted the fix-rtc-bkp-register-indexing branch September 11, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants