Skip to content

Conversation

@tari
Copy link
Contributor

@tari tari commented Aug 24, 2022

The name of the default RAM region for the linker varies somewhat between architectures, generally either being RAM or SRAM. Since it's useful at a higher level to have predictable memory region names (especially for the CODE_DATA_RELOCATION feature), this renames SRAM regions to RAM for architectures where that makes sense.

RAM is chosen as the unified name because SRAM may not be technically correct for some systems. Some SoCs and architectures have more complex memory layouts so they remain unchanged: they do not currently support CODE_DATA_RELOCATION which provides the motivation.

This is a follow-up from #49262 to clean up some of the build configuration.

@nashif nashif requested a review from edersondisouza August 24, 2022 01:39
@tari tari force-pushed the arm-sram-region-rename branch from 55fafc7 to fbb28f3 Compare August 24, 2022 23:22
@tari tari marked this pull request as ready for review August 24, 2022 23:23
@zephyrbot zephyrbot added area: Build System area: ARM ARM (32-bit) Architecture area: Toolchains Toolchains area: ARM64 ARM (64-bit) Architecture area: NIOS2 NIOS2 Architecture labels Aug 24, 2022
edersondisouza
edersondisouza previously approved these changes Aug 25, 2022
carlocaione
carlocaione previously approved these changes Aug 25, 2022
Copy link
Contributor

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

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

LGTM

@tari tari dismissed stale reviews from carlocaione and edersondisouza via 8c8367d August 26, 2022 00:07
@tari tari force-pushed the arm-sram-region-rename branch from fbb28f3 to 8c8367d Compare August 26, 2022 00:07
@tari tari requested a review from carlocaione August 26, 2022 00:08
@tari tari force-pushed the arm-sram-region-rename branch 3 times, most recently from 00e0ee5 to 07c7609 Compare August 30, 2022 02:51
@tari tari requested a review from nvlsianpu as a code owner August 30, 2022 02:51
@tejlmand
Copy link
Contributor

tejlmand commented Sep 6, 2022

That's a symptom of referring to a region that doesn't exist, so it's fixed by the cmake change to that sample.

I know, it was just an extra piece of info for you 😉

tejlmand
tejlmand previously approved these changes Sep 6, 2022
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

lgtm

SgrrZhf
SgrrZhf previously approved these changes Sep 7, 2022
It's useful for RAMABLE_REGION to have a uniform name when
CODE_DATA_RELOCATION is supported, because otherwise the build system
needs to be aware of how the region name differs between architectures.
Since architectures tend to prefer one of 'SRAM' or 'RAM' for that
region, prefer to use 'RAM' as the more general term.

Signed-off-by: Peter Marheine <[email protected]>
As with 32-bit ARM, it is useful to make the RAMABLE_REGION name uniform
across architectures so the build system does not need to be aware of
the differences when CODE_DATA_RELOCATION is supported. Generalize the
name to just 'RAM' for uniformity.

Signed-off-by: Peter Marheine <[email protected]>
It's useful for the default RAM region to have a uniform name across
architectures so higher-level build processes do not need to have their
own awareness of the names. Since NIOS2 uniformly uses 'SRAM', change it
to 'RAM' to match most other architectures.

Signed-off-by: Peter Marheine <[email protected]>
This was copied from the ARM version of the test's linker script, which
in turn seems to have been copied from another linker script at some
point. Remove the incorrect comment.

Signed-off-by: Peter Marheine <[email protected]>
@tari tari force-pushed the arm-sram-region-rename branch from 07c7609 to 856e402 Compare September 14, 2022 00:23
@fabiobaltieri fabiobaltieri dismissed stale reviews from SgrrZhf and tejlmand via 856e402 September 14, 2022 09:30
@fabiobaltieri
Copy link
Member

Should this be considered a bugfix? (would queue for v3.3.otherwise since we are in feature freeze)

@fabiobaltieri fabiobaltieri added this to the v3.3.0 milestone Sep 16, 2022
@tari
Copy link
Contributor Author

tari commented Sep 16, 2022

I wouldn't call this a bugfix, since it doesn't affect normal usage. It's even a sort of breaking change, since users referring to SRAM in a zephyr_code_relocate will need to change to RAM on changed platforms.

@carlescufi carlescufi merged commit 34ca734 into zephyrproject-rtos:main Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ARM ARM (32-bit) Architecture area: ARM64 ARM (64-bit) Architecture area: Build System area: Flash area: NIOS2 NIOS2 Architecture area: Toolchains Toolchains platform: NXP NXP

Projects

None yet

Development

Successfully merging this pull request may close these issues.