STM32: add weak TargetBSP_Init function

[jeromecoutant]

Summary of changes

Hi

With this commit,
ecaa5fe

user is able to initialize some extra components like external RAM.

I propose to enable this for all STM32 targets

@AGlass0fMilk
Is the new place OK for you ?

@LMESTM

Impact of changes

Migration actions required

Documentation

---

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

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

---

Reviewers

---

[ciarmcom]

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

Pull request has been modified.

[ciarmcom]

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

[AGlass0fMilk]

I'll have to test it on my hardware. What concerns me is that mbed_sdk_init is called after RAM initialization:

https://github.com/jeromecoutant/mbed/blob/76135d082022f57f78229215da3e57c5e7fd3908/targets/TARGET_STM/mbed_overrides.c#L50-L57

So if I include the external RAM region in my linker script it may cause issues when the toolchain code attempts to initialize (zero-out?) the unconfigured external RAM.

However, this is a much cleaner implementation. Perhaps Mbed itself should provide an additional boot hook for the application to do BSP initialization from the reset handler.

I probably don't have to include the external RAM in my linker script however. I mainly use it for a framebuffer and in that case I know where I'm putting things ahead of time.

I like the idea of extending this to all STM targets and don't mind changing my code to make it work.

I will test later.

[0xc0170]

Can't we make this generic, or what step is missing in our boot sequence - that this pull request is fixing?

[AGlass0fMilk]

@0xc0170
The problem here is:

Let's say I have a target based on an STM32H745 (which I do). The STM32H745 target files already implement SystemInit for me:

mbed-os/targets/TARGET_STM/TARGET_STM32H7/TARGET_STM32H745xI/system_stm32h7xx.c

Lines 164 to 173 in c7759fe

	void SystemInit(void)
	{
	/* FPU settings ------------------------------------------------------------*/
	#if (__FPU_PRESENT == 1) && (__FPU_USED == 1)
	SCB->CPACR |= ((3UL << (10 * 2)) | (3UL << (11 * 2))); /* set CP10 and CP11 Full Access */
	#endif
	/*SEVONPEND enabled so that an interrupt coming from the CPU(n) interrupt signal is
	detectable by the CPU after a WFI/WFE instruction.*/
	SCB->SCR |= SCB_SCR_SEVONPEND_Pos;

My custom board has external SDRAM that must be initialized before it's able to be used, or a hardfault could happen during boot if an access to the external RAM address range is attempted. So it's critical this SDRAM initialization happen as early as possible in the boot sequence.

Now I have a dilemma. Do I throw away all the existing files implementing support for my target MCU just so I can add a few lines to initialize the external SDRAM at start up? Now I have to separately maintain a critical part of my MCU's Mbed OS implementation.

It would be nice to have some sort of generic hook called during or immediately after SystemInit so board-level hardware and peripherals can be initialized before any RAM initialization, constructors, etc are called.

I don't think the current Mbed-OS boot sequence allows for this.

[kjbracey]

I would say external RAM initialisation would have to be done from SystemInit, in target-specific code. Only place in the current start-up sequence that would make the external RAM general-purpose - usable for static data,

https://os.mbed.com/docs/mbed-os/v5.15/reference/bootstrap.html

Many existing STM targets already have a hook present there - SystemInit_ExtMemCtl. That's call is conditionally made based on a macro which a derived target could define.

I'm not sure there's anything much generic that Mbed OS could do to help here. I think SystemInit_ExtMemCtl could be a universal convention, but even SystemInit is not really Mbed OS's business.

[kjbracey]

Do I throw away all the existing files implementing support for my target MCU just so I can add a few lines to initialize the external SDRAM at start up?

You always have the option to intercept via a wrapper, like malloc tracing and minimal-printf too. There are toolchain-specific ways of hijacking any global symbol, even if not weak.

[kjbracey]

Can't we make this generic, or what step is missing in our boot sequence - that this pull request is fixing?

We have a general "inheritance" mechanism for targets, so it would be logical to have an inheritance chain for the mbed_sdk_init. At the minute it's a singleton.

As ever, I can think of lots of too-clever solutions. But here's something vaguely simple.

As a derived target, you know what you derived from. It's reasonable to assume you have fairly detailed knowledge of it, and it's more stable than you.

My suggestion then is that basic targets continue to provide mbed_sdk_init.

We add one new target config setting - an option in the target.json that lets you specify an alternative name for the target initialise call for Mbed OS to call.

So derived target sets that, provides its own init function, Mbed OS calls that, and that init function calls the base target's mbed_sdk_init internally at the appropriate point. (Probably first, but maybe it wants to do a little before).

If someone then derives from that, they can override the initialise call again. Their own would call the previous target's one (hard-coded, as they know what it's initialise call is called).

However, that is not solving the "RAM init" problem, that would be an "initialise extra peripherals" hook - a better version of what this PR is giving.

[kjbracey]

Thinking a bit further, maybe similar inheritance logic to the mbed_sdk_init "extra peripherals" would work for "extra RAM". It would be chaining of init before calling __main or whatever. But it would have to be coded into the targets, rather than core , as they go BL SystemInit; BL __main at the moment. But they could do

BL     MBED_CONF_TARGET_SYSTEM_INIT_FN   // Default is SystemInit
BL     __main

Assuming the same idea of derived target having DerivedSystemInit which does SystemInit(); ExtRamInit();, and that there's no real need to put the external RAM init in the middle of the base SystemInit. SystemInit_ExtRAMCtl lets it be in the middle, but not sure it's required.

[0xc0170]

I'm not sure there's anything much generic that Mbed OS could do to help here. I think SystemInit_ExtMemCtl could be a universal convention, but even SystemInit is not really Mbed OS's business.

Reading some older issues around SystemInit in CMSIS repo, they state it is a user provided function - it is provided by MCU family. I would question this as in our space, it is framework provided that a user should be able to extend (I guess it is our responsibility to add that hook). Then a question is how. CMSIS does not state, they leave it to a user to do its job in the SystemInit.

Would this extension be worth adding to CMSIS or rather we handle it as being proposed above? We would need to change a startup for each target to have it available :/

[kjbracey]

CMSIS doesn't have any concept of inheritance, so a simple SystemInit is sufficient.

But does CMSIS even care itself? As I understand it, the basic CMSIS model is that you have "startup_XXX.s" which is toolchain-and-device-specific, which then (conventionally) calls SystemInit which is device-specific, which then calls into non-device-specific startup code.

For CMSIS SystemInit is just a convention, isn't it? I'm not immediately seeing what relies on that outside the device-specific code.

In a generic CMSIS startup.S, you could certainly have something like

#ifdef ALTERNATIVE_SYSTEM_INIT
BL      ALTERNATIVE_SYSTEM_INIT
#else
BL      SystemInit
#endif

If the macro name could be made a convention, then that's what the Mbed OS target config could set.

[kjbracey]

The simpler form of

BL SystemInit
BL UserEarlyInit   // default WEAK empty implementtion
BL __main

which I can imagine being a counter-proposal, doesn't fully cut it for an Mbed OS multi-level device inheritance chain. Maybe that's not a big deal though. Can't really imagine 3 layers of RAM addition.

[0xc0170]

I shall not block this PR, it is fine as it is. However we have more generic problem that should be taken separately.

@jeromecoutant @AGlass0fMilk Can you create an issue for this to track?

[0xc0170]

CI started

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

[mergify]

This PR does not contain release version label after merging.

[AGlass0fMilk]

I shall not block this PR, it is fine as it is. However we have more generic problem that should be taken separately.

@jeromecoutant @AGlass0fMilk Can you create an issue for this to track?

This seems more like what they want us to use the forums for... but I'd rather continue the conversation on GitHub where linking issues/PRs is easier. Let me know if you still want me to create an issue or forum post.