STM32: Add HSE support to RTC

[boraozgen]

Summary of changes

Hey folks, especially @jeromecoutant,

We have a requirement to supply the RTC from HSE. This is currently not possible in Mbed. I tinkered a little bit with the RTC port to enable this.

I used the reference manuals of STM32F411/412 and tested on the respective MCUs. I am not sure if this applies to all of the STM32 family, would be great if someone checks this.

According to the STM32F411 reference manual, the requirements are:

• RTC clock must be 1 MHz if supplied from HSE (p.98, p.107), although I found out that it also works if this is set lower than 1 MHz.
• ck_spre should be set to 1 Hz using the PREDIV_A and PREDIV_S prescalers (p.435).

My implementation:

• RTC_FROM_HSE is defined as a macro, could be set in mbed_app.json or in the target definition.
• PeriphClkInitStruct.RTCClockSelection prescaler calculated automatically from HSE_VALUE.
• RTC_CLOCK is set to 1000000U (per requirement)
• PREDIV_A_VALUE is set to 124 for PREDIV_A_VALUE + 1 to be a divisor of 1 MHz.

I am not sure if lp_ticker is affected by this, or any changes are needed there...

Impact of changes

Migration actions required

Documentation

Needs to be amended.

---

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[X] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

@jeromecoutant

---

[ciarmcom]

@boraozgen, thank you for your changes.
@jeromecoutant @ARMmbed/team-st-mcd @ARMmbed/mbed-os-maintainers please review.

[jeromecoutant]

Hi

Need also to test, but as a first comment, I think that HSE is stopped during deepsleep (and not LSE)
So if you clock RTC with HSE, you should need to lock deepsleep as well?

@LMESTM

[jeromecoutant]

Compilation failed for STM32L4

Tests failed for STM32F429 for ex:
| NUCLEO_F429ZI-ARMC6 | NUCLEO_F429ZI | drivers-tests-tests-mbed_drivers-rtc | FAIL | 17.02 | default |
| NUCLEO_F429ZI-ARMC6 | NUCLEO_F429ZI | hal-tests-tests-mbed_hal-rtc | TIMEOUT | 72.04 | default |
| NUCLEO_F429ZI-ARMC6 | NUCLEO_F429ZI | hal-tests-tests-mbed_hal-rtc_reset | FAIL | 17.08 | default |

[boraozgen]

Need also to test, but as a first comment, I think that HSE is stopped during deepsleep (and not LSE)
So if you clock RTC with HSE, you should need to lock deepsleep as well?

I believe this should be left to the application developer. One can lock the deep sleep in the main function.

[jeromecoutant]

Need also to test, but as a first comment, I think that HSE is stopped during deepsleep (and not LSE)
So if you clock RTC with HSE, you should need to lock deepsleep as well?

I believe this should be left to the application developer. One can lock the deep sleep in the main function.

Not so sure... Or you should add explicit warnings in some comments somewhere...

[boraozgen]

Should we add the switch into targets.json, where we can provide a comment as well? I'm not sure which is the correct place as I am not very familiar with the document. For example here:

mbed-os/targets/targets.json

Line 1137 in c153880

"config": {

[jeromecoutant]

Should we add the switch into targets.json?

NO... targets.json is defining the default configuration. And default and working configuration is RTC with LSE.

You can change this default configuration in your local mbed_app.json file.
Like this in your case:
{
"macros": [
"RTC_FROM_HSE=1"
],
}

Note that I still couldn't make tests working with this configuration...

[boraozgen]

I did not mean to set the switch, but to define it, similar to how gpio_reset_at_init is defined but not set by default. There we could assign a help text which indicates its limitations. It would allow setting it in target_overrides instead of defining a macro.

What is blocking the tests? It should give an preprocessor error in case it is enabled with an invalid target (any target except F2/F3/F4).

[jeromecoutant]

I did not mean to set the switch, but to define it, similar to how gpio_reset_at_init is defined but not set by default. There we could assign a help text which indicates its limitations. It would allow setting it in target_overrides instead of defining a macro.

Nice way could be to add a config for RTC clock, with default value LSE, and sometimes LSI when there is no LSE.
Then you could add HSE.
When you add a config, the generated macro name is something like MBED_CONF_xxx

[jeromecoutant]

What is blocking the tests? It should give an preprocessor error in case it is enabled with an invalid target (any target except F2/F3/F4).

See above:
Tests failed for STM32F429 for ex:
| NUCLEO_F429ZI-ARMC6 | NUCLEO_F429ZI | drivers-tests-tests-mbed_drivers-rtc | FAIL | 17.02 | default |
| NUCLEO_F429ZI-ARMC6 | NUCLEO_F429ZI | hal-tests-tests-mbed_hal-rtc | TIMEOUT | 72.04 | default |
| NUCLEO_F429ZI-ARMC6 | NUCLEO_F429ZI | hal-tests-tests-mbed_hal-rtc_reset | FAIL | 17.08 | default |

[boraozgen]

I just ran the tests on my side. The first one seems to pass for me. Is there something else in your configuration?

| NUCLEO_F429ZI-GCC_ARM | NUCLEO_F429ZI | mbed-os-drivers-tests-tests-mbed_drivers-rtc | OK     | 26.86              | default     |

The others fail, and the reasons are clear. hal-tests-tests-mbed_hal-rtc fails on the 'RTC - sleep' case, which tries to deep sleep. This can be overcome by disabling LPTICKER (this would be another limitation to be documented). Indeed, when I add "target.device_has_remove": ["LPTICKER"] to the configuration the test passes.

I don't exactly understand why mbed-os-hal-tests-tests-mbed_hal-rtc_reset fails but it probably has something to do with HSE being reconfigured on reset. Any ideas on this?

Nice way could be to add a config for RTC clock, with default value LSE, and sometimes LSI when there is no LSE.
Then you could add HSE.
When you add a config, the generated macro name is something like MBED_CONF_xxx

This is a good idea, I will suggest an implementation when the tests issue is cleared.

[jeromecoutant]

I don't exactly understand why mbed-os-hal-tests-tests-mbed_hal-rtc_reset fails but it probably has something to do with HSE being reconfigured on reset. Any ideas on this?

Maybe you should have a look on RTC init after reset:
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/mbed_overrides.c#L297-L314
With LSE, it is "automatic".

[boraozgen]

Maybe you should have a look on RTC init after reset:
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/mbed_overrides.c#L297-L314
With LSE, it is "automatic".

Good clue, I managed to pass the reset test when I added the same peripheral clock initialization with HAL_RCCEx_PeriphCLKConfig here. However there is something I don't understand. How is this "automatic" for other clock sources (LSI and LSE)? Why is HAL_RCCEx_PeriphCLKConfig not called for those cases?

I will soon update the PR with the suggested changes in the configuration.

[boraozgen]

I wanted to add the setting to the configuration and thought of keeping the same structure as the LPUART clock source as in here:

mbed-os/targets/targets.json

Lines 1142 to 1145 in 2660621

	"lpuart_clock_source": {
	"help": "Define the LPUART clock source. Mask values: USE_LPUART_CLK_LSE, USE_LPUART_CLK_PCLK1, USE_LPUART_CLK_HSI",
	"value": "USE_LPUART_CLK_LSE|USE_LPUART_CLK_PCLK1"
	},

I then realized that there is a collision between that and the lse_available setting. For example in serial_api.c there are no checks if lse_available is set or not. Should I do the same, ignoring the lse_available setting and letting the user to make sure LSE is available before setting the RTC clock to LSE?

[ciarmcom]

This pull request has automatically been marked as stale because it has had no recent activity. @boraozgen, please carry out any necessary work to get the changes merged. Thank you for your contributions.

[boraozgen]

@jeromecoutant could you please comment on my last question?

[adbridge]

@jeromecoutant this is awaiting your feedback...

Pull request has been modified.

[mergify]

This PR cannot be merged due to conflicts. Please rebase to resolve them.

[boraozgen]

Finally I have come up with a solution, which in my opinion is too much preprocessor magic, but this is how Mbed works :)

I first considered the masking logic similar to the lpuart_clock_source, but it leads to undefined situations, such as what happens if all options are masked. I believe the lpuart_clock_source has therefore some problems but it is out of scope here.

I finally decided on choosing between the options USE_RTC_CLK_LSE_OR_LSI, USE_RTC_CLK_LSI or USE_RTC_CLK_HSE. This leaves no undefined conditions and allows a fallback to LSI in case USE_RTC_CLK_LSE_OR_LSI is selected and LSE is not available. This is the default option, so it does not change the current behavior.

Each option passes the following tests with GCC and F429ZI. For HSE to pass, LPTICKER must be disabled using target.device_has_remove": ["LPTICKER"]. This is documented in the targets.json file.

• drivers-tests-tests-mbed_drivers-rtc
• hal-tests-tests-mbed_hal-rtc
• hal-tests-tests-mbed_hal-rtc_reset

[mbed-ci]

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️

[0xc0170]

@jeromecoutant Can this be merged now?

[jeromecoutant]

@0xc0170 not tested yet,
Will be for next release

[0xc0170]

@jeromecoutant As I understood we can merge this, tests can be run on your side and if anything found we can fix or revert this? Let me know

[boraozgen]

@0xc0170 can we use this PR for the review changes or do I have to open a new one?