Skip to content

Add framework for configuring boot stack size #8039

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

Merged
merged 2 commits into from
Oct 8, 2018

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Sep 7, 2018

Description

Add the target config option "boot-stack-size" which is passed to the linker as the define "MBED_BOOT_STACK_SIZE" so the linker can adjust the stack accordingly. On mbed 2 the boot stack becomes the main stack after boot. On mbed 5 the boot stack becomes the ISR stack after boot. Because of these different uses the stack size for mbed 2 is set to 4K by default while on mbed 5 it is set to 1k.

Additionally, the NRF5X family requires a larger interrupt stack sizedue to the softdevice so the size is increased to 2k on mbed 5 builds.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Breaking change

Add the target config option "boot-stack-size" which is passed to the
linker as the define "MBED_BOOT_STACK_SIZE" so the linker can
adjust the stack accordingly. On mbed 2 the boot stack becomes the
main stack after boot.  On mbed 5 the boot stack becomes the
ISR stack after boot. Because of these different uses  the stack size
for mbed 2 is set to 4K by default while on mbed 5 it is set to 1k.

Additionally, the NRF5X family requires a larger interrupt stack size
due to the softdevice so the size  is increased to 2k on mbed 5 builds.
Update the K64F linker scripts to make use of the newly added stack
size target config.
define symbol MBED_BOOT_STACK_SIZE = 0x400;
}

define symbol __stack_size__=MBED_BOOT_STACK_SIZE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@theotherjimmy does the name MBED_BOOT_STACK_SIZE seems reasonable to you? Alternatively I could set this to the target define that is set - MBED_CONF_TARGET_BOOT_STACK_SIZE. Let me know what you think.

if stack_param in params:
define_string = self.make_ld_define("MBED_BOOT_STACK_SIZE", int(params[stack_param].value, 0))
self.ld.append(define_string)
self.flags["ld"].append(define_string)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@theotherjimmy this is how the target define is getting passed to the linker script. Is this the right way to do it or would you like any changes made here?

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks fine.

@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 7, 2018

This should unblock #7238 as this mechanism allows the boot stack size to be correctly configured for all 3 toolchains in both mbed 2 and mbed 5.

Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

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

This is great. Well done :-)

@@ -3,5 +3,22 @@
"config": {
"present": 1
},
"macros": ["_RTE_"]
"macros": ["_RTE_"],
"target_overrides": {
Copy link
Contributor

Choose a reason for hiding this comment

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

In targets/targets.json file boot-stack-size is set to 4kB and here is modified and set to 1kB with some exceptions (like for NRF boards).
Is this done to distinguish stack size for builds with and without RTOS (4kB for non RTOS and typically 1kB for builds with RTOS)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, by default stack sizes are 4K and they are overridden to 1K when the RTOS is present. Targets with special requirements on stack size, such as the nrf51, can further override for both the RTOS and non-RTOS configuration.

* the stack where main runs is determined via the RTOS. */
__stack_size__ = MBED_BOOT_STACK_SIZE;

__heap_size__ = 0x6000;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like __heap_size__ and HEAP_SIZE symbols now are unused and can be removed.
It would be enough to define STACK_SIZE = MBED_BOOT_STACK_SIZE or even remove also STACK_SIZE and use MBED_BOOT_STACK_SIZE instead.

"target.boot-stack-size": "0x400"
},
"MCU_NRF51": {
"target.boot-stack-size": "0x800"

Choose a reason for hiding this comment

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

Will it be good to move these overrides in targets.json instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The size in targets.json is the size for mbed 2 - 0x1000, while rtos/mbed_lib.json is for mbed 5 - 0x400. In the case of the nrf5x the interrupt stack needs to be customized for mbed 5 only so it is set to 0x800 here.

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

Tools and config changes look fine.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

LGTM would be good to add some info to the porting guide.

@NirSonnenschein
Copy link
Contributor

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 3, 2018

Build : SUCCESS

Build number : 3223
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8039/

Triggering tests

/morph test
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Oct 3, 2018

@studavekar
Copy link
Contributor

/morph test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Oct 7, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 7, 2018

@NirSonnenschein
Copy link
Contributor

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Oct 7, 2018

@cmonr cmonr merged commit b7cf1ab into ARMmbed:master Oct 8, 2018
@JaniSuonpera
Copy link
Contributor

@cmonr Why only for target K64F? I think that same kind changes are needed other target e.g. K66F.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 25, 2018

@cmonr Why only for target K64F? I think that same kind changes are needed other target e.g. K66F.

@c1728p9 Review the latest question - This new configuration is applied to set of targets - should be done for all?

@JaniSuonpera
Copy link
Contributor

Yes, I think so.

@c1728p9
Copy link
Contributor Author

c1728p9 commented Oct 25, 2018

@JaniSuonpera the rest of the targets were going to be updated in #7238. I think @mprse is making a new PR for this.

@c1728p9 c1728p9 deleted the stack_size_framework branch October 25, 2018 15:40
@jeromecoutant
Copy link
Collaborator

the rest of the targets were going to be updated in #7238

Now this PR has been closed, who is in charge to update all the targets ?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 13, 2018

@mprse Can you review?

@mprse
Copy link
Contributor

mprse commented Dec 13, 2018

Now this PR has been closed, who is in charge to update all the targets ?

I'm working on it. PR should be ready today or tomorrow.

mprse added a commit to mprse/mbed-os that referenced this pull request Feb 1, 2019
The following commits: ARMmbed#8039, ARMmbed#9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.
deepikabhavnani pushed a commit to deepikabhavnani/mbed-os that referenced this pull request Feb 11, 2019
The following commits: ARMmbed#8039, ARMmbed#9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.
deepikabhavnani pushed a commit to mprse/mbed-os that referenced this pull request Feb 19, 2019
The following commits: ARMmbed#8039, ARMmbed#9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.