Ensure the RTC is enabled and also do not mess with other bits in RCC_BDCR (especially for those in the Backup domain)

[Zoey-SunflowerLabs]

I found this when reading the code. It looks like a typo to me.

There are also other bits in the register RCC_BDCR which would be cleared after RCC_BDCR &= RCC_BDCR_RTCEN . This can be problematic. However, I did not see it caused any problem so far.

[Zoey-SunflowerLabs]

My comment was a bit misleading. Resetting other bits should be ok.

The original code:

static void
board_set_rtc_signature(uint32_t sig)
{
    /* enable the backup registers */
    PWR_CR |= PWR_CR_DBP;
    RCC_BDCR |= RCC_BDCR_RTCEN;

    BOOT_RTC_REG = sig;

    /* disable the backup registers */
    RCC_BDCR &= RCC_BDCR_RTCEN;
    PWR_CR &= ~PWR_CR_DBP;
}

My understanding is that before BOOT_RTC_REG = sig;, RTCEN bit is set to be able to write to BOOT_RTC_REG. After that, the intent is to reverse the RTCEN set operation. So, it should be RCC_BDCR &= ~RCC_BDCR_RTCEN. It is similar to the operation on PWR_CR: PWR_CR_DBP is set to be able to write to BOOT_RTC_REG and later is reset.

[davids5]

I do not think the comment explained the intent of the original code well

I was referring to the original comment in the code.

The goal was not to reset the bit but set the complete register.

[Zoey-SunflowerLabs]

I do not think the comment explained the intent of the original code well

I was referring to the original comment in the code.

The goal was not to reset the bit but set the complete register.

@davids5 could you explain a bit more?

What is the purpose of having RCC_BDCR set to RCC_BDCR_RTCEN after the backup register write is done?

And if the goal is to set RCC_BDCRto RCC_BDCR_RTCEN when returning from these functions, then why does it need RCC_BDCR |= RCC_BDCR_RTCEN and RCC_BDCR &= RCC_BDCR_RTCEN to achieve it? Why not simply use RCC_BDCR = RCC_BDCR_RTCEN directly before BOOT_RTC_REG = sig.

On the other hand, the benefit of RCC_BDCR &= ~RCC_BDCR_RTCEN is to protect backup registers against possible unwanted write accesses.

Also I just found reading backup registers actually doesn't have to change bits on RCC_BDCR and PWR_CR to enable read operation.

[davids5]

Hi @Zoey-SunflowerLabs - thank you for being persistent.

RTCEN: RTC clock enable will disable the clock, that is not desirable.

I think deleting the line altogether is the correct thing to do. This way we ensure the RTC is enabled. We also do not mess with the LSE or bypass. The application might have turned on the LSE and needs the RTC to keep running while power is off.

[davids5]

Thank you @Zoey-SunflowerLabs - I squashed and re-titled this PR here https://github.com/PX4/PX4-Bootloader/pull/188