Make STM32F412xG system_clock.c functions weak

[boraozgen]

Summary of changes

Another proposal to fix #13635. This preserves the current directory structure and only defines the clock setup functions as weak, so that the custom board integrators can implement (or generate from STM32CubeMX) their own clock setup functions.

Impact of changes

Migration actions required

Documentation

https://os.mbed.com/docs/mbed-os/v6.3/porting/porting-custom-boards.html could be updated to indicate that some boards may have system_clock.c in MCU level directories.

---

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/mbed-os-maintainers please review.

[jeromecoutant]

Just for information, if you "only" need some PLL configuration update because of different source clock value, you can have a look on #13640

[0xc0170]

System_clock.c defines board level behavior, such as clock initialization with external or internal clock. For example we use a custom board with this chip where we provide our own system_clock.c implementation which suits our board. Therefore this file should reside in board level.

How would this look like? I wonder if there is cleaner way to do this overwrite than having these 2 weakly linked.

[boraozgen]

We used STM32CubeMX tool to generate the clock initialization. I think the GUI based tool is very useful for custom target configuration. I know that it is not part of the Mbed workflow, but it could be used for this case.

Furthermore, https://os.mbed.com/docs/mbed-os/v6.3/porting/porting-custom-boards.html states that this file should be provided with the custom board port anyway.

[0xc0170]

@jeromecoutant system_clock is generated ? should it be moved somewhere else but not in the current location? Any derived target should provide own clock config or it is not the case?

This fix looks like it fixes the issue but does not look correct. You got a custom board and overwrite these 2 functions? Why not rather entire system clock function and invoke own functions?

[jeromecoutant]

@jeromecoutant system_clock is generated ? should it be moved somewhere else but not in the current location? Any derived target should provide own clock config or it is not the case?
This fix looks like it fixes the issue but does not look correct. You got a custom board and overwrite these 2 functions? Why not rather entire system clock function and invoke own functions?

Hi

No, system_clock is not generated. ST provides a tool that help customers to generate clock for all STM32.

About proposed patch, see my comments, my preference was to add some correction like #13640 or to add weak only for SetSysClock... But I would not stop any STM32 user to make their own targets, and propose patches :-)

[jeromecoutant]

For my information, I agree that WEAK to SetSysClock or to SetSysClock_PLL_HSE
could be useful, but for this SetSysClock_PLL_HSI, what is the issue ?
This should work as HSI input clock is the same ?

[boraozgen]

You are right, the board configuration should not affect the HSI. Indeed, it works using the default code for this part. In this case, we can remove this weak statement.

[kjbracey]

How would this look like? I wonder if there is cleaner way to do this overwrite than having these 2 weakly linked.

There's no real problem with "weak" except that you can't stack it - you can only override once, which is more limited than the mbed target inheritance system. You have to agree who is allowed to override any given function. Is it the app, Mbed OS core or a derived target? Only 1 can do it.

(I've dealt with that override-once limit in the past by having two hook points for get_default_instance (app override) and get_target_default_instance (target override). Don't think that applies here.)

In this case, my previous proposal for something more extensible than weak may help: #13022 (comment)

Although that's only worth it if you think there might be an issue with multiple layers wanting to override. "mbed_sdk_init" it makes sense for - each derived target extension may have more base peripherals. Other calls it's less clear, but this may be one of them.

[boraozgen]

After reviews we isolated the weak requirement to HSE clock configuration.

As proposed by @jeromecoutant in #13635 (comment), some macro switching between HSE frequencies could also be a more elegant solution. I do not have the resources to test and propose that solution, so it is for the reviewers to decide.

[0xc0170]

CI started

[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_unittests	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️
jenkins-ci/mbed-os-ci_wisun-mesh-test	✔️