bootloader: Fix LPC55S69 bootloader segmentation #10791

hugueskamba · 2019-06-07T16:07:46Z

Description

As the build tool in mbed-os 5.13 cannot appropriately deal with a segmented
bootloader when combining it with an application, this commit adjusts the
size reserved for interrupts (via the linker file) to avoid a bootloader
segmentation due to an unpopulated ROM area.

The microcontroller has a total of 60 vector interrupts + 16 exception
handlers. The allocated ROM flash for interrupts should be (60 + 16) x word
size in bytes = 76 x 4 = 304 = 0x130.

This commit changes the interrupt reserved space from 0x140 to 0x130.

Pull request type

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

Reviewers

@bridadan @theotherjimmy

Fixes internal issue IOTCORE-1149

ciarmcom · 2019-06-07T17:00:42Z

@hugueskamba, thank you for your changes.
@bridadan @MarceloSalazar @maclobdell @theotherjimmy @ARMmbed/mbed-os-maintainers please review.

evedon · 2019-06-10T11:44:41Z

...sso_MCUS/TARGET_LPC55S69/TARGET_M33_NS/device/TOOLCHAIN_GCC_ARM/LPC55S69_cm33_core0_flash.ld

@@ -57,15 +57,19 @@ __ram_vector_table__ = 1;
    #define MBED_BOOT_STACK_SIZE 0x400
 #endif

+#if !defined(MBED_NVIC_SIZE)
+    #define MBED_NVIC_SIZE 0x130


This should probably be defined as NVIC_NUM_VECTORS * 4 to avoid magic numbers.

See https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_LPC55S69/TARGET_M33_NS/device/cmsis_nvic.h#L43

Excellent. Thanks

There is an 8-byte alignment requirement, if I recall correctly, so just using NVIC_NUM_VECTORS * 4 may not quite be magic enough in general. Not sure if GCC has a handy alignment built-in to help you here. I know the ARM linker would let you use AlignExpr(NVIC_NUM_VECTORS * 4,8).

Sure, it happens to be aligned on this platform, but if someone were to copy the general-looking implementation to another...

There is an 8-byte alignment requirement, if I recall correctly, so just using NVIC_NUM_VECTORS * 4 may not quite be magic enough in general. Not sure if GCC has a handy alignment built-in to help you here. I know the ARM linker would let you use AlignExpr(NVIC_NUM_VECTORS * 4,8).

Sure, it happens to be aligned on this platform, but if someone were to copy the general-looking implementation to another...

@kjbracey-arm
The mention of the image start address alignment I saw in the user manual is a requirement for a 32-bit word alignment (see linked user manual on page 153).
Should it not be AlignExpr(NVIC_NUM_VECTORS * 4, 4) instead for the ARM toolchain?

Individual chips might permit things to be 4-byte aligned, but architecturally regions are supposed to be 8-byte aligned these days. The ARMC6 toolchain is only letting you get away with 4-byte alignment because --legacyalign is being passed.

There are certain instructions that only work or work faster with 8-byte alignment that the compiler may wish to use.

See other PRs #8013 and #10373, correcting platforms to 8-byte alignment.

Thank you @kjbracey-arm.
I will modify the ARM toolchain linker file. However, for GCC I believe it is handled with . = ALIGN(8); which is already present in the section definitions.

Yes, that probably works. Crosscheck how other implementations do it, including in those PRs, there are a couple of common patterns (including hard-coding alignment to 8 by hand in quite a few).

https://github.com/ARMmbed/mbed-os/compare/923c1f21ffed881e36bf90310ce14b2bf0d35401..b0804c4a0d6bc838e33334573c71e274028cb90b

bridadan

Seems reasonable to me! Not an expert on this platform, but your changes seem sane 😄

mmahadevan108 · 2019-06-12T02:20:31Z

...sso_MCUS/TARGET_LPC55S69/TARGET_M33_NS/device/TOOLCHAIN_GCC_ARM/LPC55S69_cm33_core0_flash.ld

@@ -57,15 +57,19 @@ __ram_vector_table__ = 1;
    #define MBED_BOOT_STACK_SIZE 0x400
 #endif

+#if !defined(MBED_NVIC_SIZE)


Why is the define MBED_NVIC_SIZE used only for the GCC linker script?

mmahadevan108

Thank you

adbridge · 2019-06-18T17:25:49Z

@hugueskamba please take a look at Kevin's comments

As the build tool in mbed-os 5.13 cannot appropriately deal with a segmented bootloader when combining it with an application, this commit adjusts the size reserved for interrupts (via the linker file) to avoid a bootloader segmentation due to an unpopulated ROM area. The microcontroller has a total of 60 vector interrupts + 16 exception handlers. The allocated ROM flash for interrupts should be (60 + 16) x word size in bytes = 76 x 4 = 304 = 0x130. This commit changes the interrupt reserved space from 0x140 to 0x130.

hugueskamba · 2019-06-26T12:57:44Z

The last force used the built-in AlignExpr to align the text region at the 8-byte boundary and squashed all commits of this branch.

kjbracey

I think I would have done it by locking in the alignment to the "interrupts_size" define itself, but this is fine.

Not relevant to this change, but I see that the RAM interrupt table size is fixed at 0x200 - there's no reason why that shouldn't be the size as the ROM region. The base address needs to be 0x200 aligned (if the table is 0x1xx-sized), but you could start the static data immediately after it. You could adjust that if it gets revisited at any point.

0xc0170 · 2019-06-26T13:47:13Z

Ci started

hugueskamba · 2019-06-26T14:14:14Z

I think I would have done it by locking in the alignment to the "interrupts_size" define itself, but this is fine.

Not relevant to this change, but I see that the RAM interrupt table size is fixed at 0x200 - there's no reason why that shouldn't be the size as the ROM region. The base address needs to be 0x200 aligned (if the table is 0x1xx-sized), but you could start the static data immediately after it. You could adjust that if it gets revisited at any point.

@kjbracey-arm Could you please raise a ticket so we don't forget this?

mbed-ci · 2019-06-26T17:13:58Z

Test run: SUCCESS

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

ciarmcom requested review from bridadan, maclobdell, MarceloSalazar, theotherjimmy and a team June 7, 2019 17:00

ciarmcom added the needs: review label Jun 7, 2019

0xc0170 requested a review from mmahadevan108 June 9, 2019 17:55

evedon reviewed Jun 10, 2019

View reviewed changes

bridadan approved these changes Jun 11, 2019

View reviewed changes

mmahadevan108 reviewed Jun 12, 2019

View reviewed changes

mmahadevan108 approved these changes Jun 17, 2019

View reviewed changes

evedon approved these changes Jun 18, 2019

View reviewed changes

adbridge added needs: work and removed needs: review labels Jun 18, 2019

hugueskamba force-pushed the fix-lpc55s69-bootloader-segmentation branch from 923c1f2 to 4e735a1 Compare June 24, 2019 13:57

hugueskamba force-pushed the fix-lpc55s69-bootloader-segmentation branch from 923c1f2 to b0804c4 Compare June 26, 2019 12:55

kjbracey approved these changes Jun 26, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Jun 26, 2019

0xc0170 added ready for merge release-version: 5.13.1 and removed needs: CI labels Jun 27, 2019

0xc0170 merged commit 201a264 into ARMmbed:master Jun 27, 2019

0xc0170 removed the ready for merge label Jun 27, 2019

teetak01 mentioned this pull request Jul 4, 2019

Release candidate for mbed-os-5.13.1 #10952

Merged

hugueskamba deleted the fix-lpc55s69-bootloader-segmentation branch July 30, 2019 10:05

bootloader: Fix LPC55S69 bootloader segmentation #10791

bootloader: Fix LPC55S69 bootloader segmentation #10791

Uh oh!

Conversation

hugueskamba commented Jun 7, 2019

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Jun 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmahadevan108 left a comment

Choose a reason for hiding this comment

Uh oh!

adbridge commented Jun 18, 2019

Uh oh!

hugueskamba commented Jun 26, 2019

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 26, 2019

Uh oh!

hugueskamba commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbed-ci commented Jun 26, 2019

Uh oh!

Uh oh!

kjbracey Jun 18, 2019 •

edited

Loading

hugueskamba Jun 26, 2019 •

edited

Loading

hugueskamba Jun 17, 2019 •

edited

Loading

hugueskamba commented Jun 26, 2019 •

edited

Loading