Skip to content
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

Add target definition: nrf51 16k on MCU_NRF51_UNIFIED platform #6178

Merged
merged 3 commits into from
Jun 11, 2018

Conversation

andrewleech
Copy link
Contributor

@andrewleech andrewleech commented Feb 23, 2018

The flash base address in the linker (APP_CODE_BASE) for nrf51_16k_s130 is incorrect, as per the official documentation for S130: http://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.s130.sds%2Fdita%2Fsoftdevices%2Fs130%2Fmem_usage%2Fmem_resource_reqs.html

Without this change applications do not boot when running on a 16K nrf51 (not very common any more) as the softdevice jumps to the wrong location to start the application.

I've been running with this fix in place using a Raytac MDBT40 module with only 16K ram.

Pull request type

[x] Fix
[ ] Refactor
[ ] New Target
[ ] Feature
[ ] Breaking Change

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2018

@marcuschangarm Ping.

@marcuschangarm
Copy link
Contributor

@cmonr I'm not familiar with the NRF51 code base anymore.

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2018

@marcuschangarm Thanks for the update. Feel free to remove yourself from the reviewers list (oddly enough, not sure how to do that on my end...).

@cmonr cmonr requested a review from donatieng March 6, 2018 20:31
@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2018

@donatieng If you're not the right person to review, we're open to suggestions.

Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

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

Looks like there is no 16K version of the "TARGET_MCU_NRF51822_UNIFIED" target in targets.json.

The other NRF51 16K targets use TARGET_MCU_NRF51822 which uses the S130 v1 Softdevice for which the linker scripts are correct.

Although there's quite a bit of cleanup that should be done around NRF51 targets.

@cmonr
Copy link
Contributor

cmonr commented Mar 21, 2018

@donatieng So what are the side effects of this PR noy being able to go in? Does this mean that the NRF51 cannot support Flash properly, or that it can not use mbed-os bootloader features?

@andrewleech
Copy link
Contributor Author

Yes I didn't entirely realise there were no targets currently configured that use these linker files.
I'm running with a custom_targets.json to create nRF51 targets using nrf unified platform, which then hit this linker file.
I currently have to .mbedignore the built in ones and have this fixed copy locally in my project.

This does not relate to bootloader, but to regular apps running on nrf51 16k with unified platform.

@andrewleech andrewleech changed the title nrf51_16k_s130 Fix flash origin for APP_CODE_BASE Add target definition: nrf51 16k on MCU_NRF51_UNIFIED platform Mar 21, 2018
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 9, 2018

Yes I didn't entirely realise there were no targets currently configured that use these linker files.
I'm running with a custom_targets.json to create nRF51 targets using nrf unified platform, which then hit this linker file.
I currently have to .mbedignore the built in ones and have this fixed copy locally in my project.

This does not relate to bootloader, but to regular apps running on nrf51 16k with unified platform.

@donatieng What do you think? Please review

@cmonr
Copy link
Contributor

cmonr commented May 8, 2018

@donatieng Any thoughts?

@andrewleech Sorry, but this now needs a rebase...

Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

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

Thanks @andrewleech, please see my comments. We're also missing linker files for IAR and ARMCC. Update: (sorry those are already in and correct)

@@ -2,7 +2,7 @@

MEMORY
{
FLASH (rx) : ORIGIN = 0x0001C000, LENGTH = 0x24000
FLASH (rx) : ORIGIN = 0x0001B000, LENGTH = 0x24000
Copy link
Contributor

Choose a reason for hiding this comment

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

The length should be updated too, to 0x25000 to use the full 256K of flash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not on mine, no. I've only got 128K flash.
Yes we probably should have linker files for all different variants of the nrf51 and a convenient way to choose which one is in use (I guess it's just a different target for each).

Copy link
Contributor

@donatieng donatieng May 16, 2018

Choose a reason for hiding this comment

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

Indeed it'd be much better to have separate targets definitions - if we're targeting a device with 128K of flash, length should be 0x5000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, I must have the 256K part. After you take off the softdevice's chunk the 128K part only has 20K left for user code... on an arm that's nothing. I doubt there's many people actually using them.

I'll fix the linker for 256K, if anyone really needs the 128K part that could be a separate commit (with new target definitions and a new full set of linker scripts)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, still need updating to 0x25000 for the 256K part :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh what, I did fix that. Must have rebased an old copy or something :-(

@@ -2,7 +2,7 @@

MEMORY
{
FLASH (rx) : ORIGIN = 0x0001C000, LENGTH = 0x24000
FLASH (rx) : ORIGIN = 0x0001B000, LENGTH = 0x24000
RAM (rwx) : ORIGIN = 0x20002800, LENGTH = 0x1800
Copy link
Contributor

@donatieng donatieng May 8, 2018

Choose a reason for hiding this comment

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

RAM start should be 0x20002ef8 and length adjusted accordingly - this will need to be tested, do we still have enough RAM to run a program reliably? (already in for ARMCC and IAR so assuming that works!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where did you get 0x20002ef8 from?

The minimum is 0x200013C8 according to the official memory requirements:
http://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.s130.sds%2Fdita%2Fsoftdevices%2Fs130%2Fmem_usage%2Fmem_resource_reqs.html&anchor=mem_resource_reqs__table_n5z_2lp_3r

We don't have the configuration at minimum so more is needed that this, but I'm not sure if it does need to be as high as 0x20002ef8?

Obviously this all changes with my other PR #5980 but I haven't finished rebasing / trimming
/ re-testing that and I've well and truly run out of work time to keep adding more than taking back on that one.

And no there's not much ram to work with (hence wanting to keep the linker value low). You really can't fit the rtos but it's working well to build an interrupt driven custom uart <-> ble bridge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've done some more testing at default softdevice settings and the reported required ram start is 0x20002EF0 which is pretty much the same as 0x20002ef8

@cmonr
Copy link
Contributor

cmonr commented May 21, 2018

@andrewleech Please take a look at the travis issues.

…orm: MCU_NRF51_16K_UNIFIED_S130

Add target definition for Raytac MDBT40 module to use the MCU_NRF51_16K_UNIFIED_S130 target.
@cmonr
Copy link
Contributor

cmonr commented May 22, 2018

@donatieng @pan- @marcuschangarm Would y'all mind re-reviewing?

@andrewleech
Copy link
Contributor Author

@cmonr, @donatieng did (in hidden comment thread above) my rebase managed to lose my previous change fixing the flash end address. I'll have to fix that again first

@cmonr
Copy link
Contributor

cmonr commented May 22, 2018

@andrewleech Thanks for the heads up!

@cmonr
Copy link
Contributor

cmonr commented May 31, 2018

@donatieng A quick re-review?
@pan- @marcuschangarm Thoughts?

Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks @andrewleech!

@cmonr
Copy link
Contributor

cmonr commented Jun 1, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 1, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jun 2, 2018

@mbed-ci
Copy link

mbed-ci commented Jun 2, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 7, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 7, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@cmonr
Copy link
Contributor

cmonr commented Jun 7, 2018

Halting CI builds until RC3 PRs are completed. Will resume after.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 9, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 9, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jun 9, 2018

@mbed-ci
Copy link

mbed-ci commented Jun 9, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 10, 2018

/morph mbed2-build

@cmonr cmonr merged commit b989afa into ARMmbed:master Jun 11, 2018
0xc0170 added a commit to 0xc0170/mbed-os that referenced this pull request Jul 10, 2018
No files to build - should not be in targets
Reverts part of the ARMmbed#6178
@0xc0170 0xc0170 mentioned this pull request Jul 10, 2018
cmonr pushed a commit that referenced this pull request Jul 15, 2018
No files to build - should not be in targets
Reverts part of the #6178
cmonr pushed a commit that referenced this pull request Jul 16, 2018
No files to build - should not be in targets
Reverts part of the #6178
cmonr pushed a commit that referenced this pull request Jul 16, 2018
No files to build - should not be in targets
Reverts part of the #6178
pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018
No files to build - should not be in targets
Reverts part of the ARMmbed#6178
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.

8 participants