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

Jump to bottom

linker: aarch32: automatic derivation of region names from devicetree nodes #37279

Merged
merged 9 commits into from Dec 9, 2021
Merged

linker: aarch32: automatic derivation of region names from devicetree nodes #37279

merged 9 commits into from Dec 9, 2021

Conversation

JordanYates
Copy link
Collaborator

Update the aarch32 linker file to automatically derive the name of memory regions from devicetree nodes instead of providing hardcoded values. This is required for automatic generation of regions based on compatibles, and also allows chosen and alias nodes to be used to control variable placement (as desired in #36152).

This PR is the first step to enable automatically generating regions from devicetree.

This is a rework of #36365 to address feedback.

@JordanYates
Copy link
Collaborator Author

@galak @mbolivar-nordic @carlescufi

galak
galak reviewed Jul 28, 2021
View reviewed changes
include/devicetree.h Outdated Show resolved Hide resolved
galak
galak requested changes Jul 28, 2021
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

  • Need to move DT_NODE_LINKER_REGION to different header
  • Need to update docs for zephyr,linker-region

mbolivar-nordic
mbolivar-nordic reviewed Jul 29, 2021
View reviewed changes
include/devicetree.h Outdated Show resolved Hide resolved
tests/lib/devicetree/api/app.overlay Outdated Show resolved Hide resolved
@mbolivar-nordic
Copy link
Contributor

Need to update docs for zephyr,linker-region

I wonder if it would be enough just to add this property to the specific compatibles that are expected to use it, with a nice multi-line description in the YAML. That would show up in the DT bindings index.

@JordanYates
Copy link
Collaborator Author

Bindings have been moved to only mmio-sram and fixed-partition, with additional documentation added.

@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch 2 times, most recently from 99fd7ae to 42a4865 Compare August 1, 2021 10:52
@carlocaione carlocaione mentioned this pull request Nov 23, 2021
Closed
@carlocaione
Copy link
Collaborator

On the other hand @JordanYates can you review #40422? I think a lot of the issues are already solved in my PoC so it could be worthwhile taking that into account.

@erwango
Copy link
Member

erwango commented Nov 25, 2021

@galak Would you mind reviewing again ?
@JordanYates Maybe you could have a try to rebase on latest master to see if the reported error is fixed now ?

@JordanYates
Copy link
Collaborator Author

@JordanYates Maybe you could have a try to rebase on latest master to see if the reported error is fixed now ?

Done, I don't think it will make much difference though as the linked issue is still open

tejlmand
tejlmand requested changes Nov 25, 2021
View reviewed changes
Copy link
Collaborator

@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.

Thanks for this.

Please remember to add same support for the CMake based linker script generator.

And then a minor suggestion on naming,
Implementation wise, things looks good.

dts/bindings/arm/arm,dtcm.yaml Outdated Show resolved Hide resolved
dts/bindings/base/linker.yaml Outdated Show resolved Hide resolved
tests/lib/devicetree/api_ext/CMakeLists.txt Outdated Show resolved Hide resolved
include/linker/devicetree_regions.h Outdated
Comment on lines 35 to 36
#define LINKER_DT_NODE_REGION_NAME(node_id) \
DT_PROP_OR(node_id, zephyr_linker_region, DT_NODE_PATH(node_id))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have same behavior here, so that region names are picked up from devicetree if defined regardless of how the linker script is generated.

zephyr/cmake/linker_script/arm/linker.cmake

Lines 37 to 50 in 513517f

# TI CCFG Registers
zephyr_linker_dts_memory(NAME FLASH_CCFG FLAGS rwx NODELABEL ti_ccfg_partition)
# Data & Instruction Tightly Coupled Memory
zephyr_linker_dts_memory(NAME ITCM FLAGS rw CHOSEN "zephyr,itcm")
zephyr_linker_dts_memory(NAME DTCM FLAGS rw CHOSEN "zephyr,dtcm")
zephyr_linker_dts_memory(NAME SRAM1 FLAGS rw NODELABEL sram1)
zephyr_linker_dts_memory(NAME SRAM2 FLAGS rw NODELABEL sram2)
zephyr_linker_dts_memory(NAME SRAM3 FLAGS rw NODELABEL sram3)
zephyr_linker_dts_memory(NAME SRAM4 FLAGS rw NODELABEL sram4)
zephyr_linker_dts_memory(NAME SDRAM1 FLAGS rw NODELABEL sdram1)
zephyr_linker_dts_memory(NAME SDRAM2 FLAGS rw NODELABEL sdram2)
zephyr_linker_dts_memory(NAME BACKUP_SRAM FLAGS rw NODELABEL backup_sram)

which would be in this function where name from region property used, otherwise fallback on node path.

zephyr/cmake/extensions.cmake

Line 3109 in 513517f

function(zephyr_linker_dts_memory)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@carlocaione carlocaione closed this Dec 9, 2021
@carlocaione carlocaione reopened this Dec 9, 2021
Jordan Yates added 5 commits December 9, 2021 20:34
dts: bindings: zephyr,linker-region property
e06bc25 
Introduce optional `zephyr,linker-region` property which signifies that
the node should result in a linker memory region and what the name of
that region should be. Property added to compatibles likely to result
in a linker memory region; 'mmio-sram', 'arm,itcm`, `arm,dtcm`,
`nxp,imx-itcm`, `nxp,imx-dtcm` and `fixed-partitions`.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
scripts: gen_defines: zephyr,linker-region unique
cebc913 
Add checks to ensure that `zephyr,linker-region` property values are
always globally unique.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
devicetree: LINKER_DT_NODE_REGION_NAME added
29f510c 
Adds a macro to convert a devicetree node to the name of a linker memory
region.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
tests: devicetree: test LINKER_DT_NODE_REGION_NAME
fe500bb 
Add a new application for testing non-core devicetree functionality.
Add tests for the default and fallback case of
`LINKER_DT_NODE_REGION_NAME`.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
linker: remove manual name specification
a290d99 
As memory region names are now derived purely from devicetree, remove
the `name` parameter from `DT_REGION_FROM_NODE_STATUS_OKAY`. Name is
`zephyr,linker-region` if it exists, otherwise the node path.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
cmake: zephyr_linker_dts_memory auto-name
a55b6e4 
Update the linker script generator to automatically derive memory
region names from the devicetree node according to the same logic in
`devicetree_regions.h`.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
Jordan Yates added 3 commits December 9, 2021 21:53
cmake: zephyr_linker_dts_memory no name param
773c637 
Remove the `NAME` function argument from `zephyr_linker_dts_memory` as
the name is now automatically derived from the devicetree node.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
dts: linker region properties
4846b0d 
Add `zephyr,linker-region` properties to all nodes sram1, sram2, sram3,
sram4, sdram1, sdram2, backup_sram, ti_ccfg, dtcm and itcm.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
linker: link into derived section names
47646a5 
Link variables into derived section names instead of hardcoded names.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
tejlmand
tejlmand reviewed Dec 9, 2021
View reviewed changes
Copy link
Collaborator

@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.

All changes looks good.

dts/bindings/base/mem-region.yaml
# Common fields for devices resulting in linker memory regions

properties:
zephyr,memory-region:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing wrong on this line but with the latest update you are now using memory-region
👍 to that :)

But the commit message still says e06bc25:

dts: bindings: zephyr,linker-region property

Introduce optional zephyr,linker-region property

Note: same goes for some other commit messages.

cmake/extensions.cmake
Comment on lines +3381 to +3384
if (NOT DEFINED name)
# Fallback to the node path
set(name ${DTS_MEMORY_PATH})
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor nit, it's possible to do:

Suggested change
if (NOT DEFINED name)
# Fallback to the node path
set(name ${DTS_MEMORY_PATH})
endif()
set_ifndef(name ${DTS_MEMORY_PATH})

zephyr/cmake/extensions.cmake

Line 1619 in 92fbd7d

function(set_ifndef variable value)

tejlmand
tejlmand approved these changes Dec 9, 2021
View reviewed changes
Copy link
Collaborator

@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.

Consider cleanup the commit message before merging to avoid later confusion if later doing a bisect.

Approving as code looks good.

@carlescufi carlescufi merged commit f408f42 into zephyrproject-rtos:main Dec 9, 2021
@fabiobaltieri fabiobaltieri mentioned this pull request Dec 9, 2021
Merged
@carlocaione carlocaione mentioned this pull request Dec 15, 2021
Closed
@JordanYates JordanYates deleted the 210728_linker_name_prop branch August 15, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanosio stephanosio stephanosio left review comments

@gmarull gmarull gmarull left review comments

@carlocaione carlocaione carlocaione approved these changes

@MaureenHelm MaureenHelm MaureenHelm approved these changes

@tejlmand tejlmand tejlmand approved these changes

@carlescufi carlescufi carlescufi approved these changes

@erwango erwango erwango approved these changes

@dleach02 dleach02 dleach02 approved these changes

@mbolivar-nordic mbolivar-nordic mbolivar-nordic approved these changes

@ABOSTM ABOSTM Awaiting requested review from ABOSTM ABOSTM is a code owner

@andyross andyross Awaiting requested review from andyross

@bwitherspoon bwitherspoon Awaiting requested review from bwitherspoon

@dcpleung dcpleung Awaiting requested review from dcpleung

@FRASTM FRASTM Awaiting requested review from FRASTM FRASTM is a code owner

@ioannisg ioannisg Awaiting requested review from ioannisg

@mmahadevan108 mmahadevan108 Awaiting requested review from mmahadevan108 mmahadevan108 is a code owner

@nandojve nandojve Awaiting requested review from nandojve nandojve is a code owner

@nashif nashif Awaiting requested review from nashif

@kgugala kgugala Awaiting requested review from kgugala

@pgielda pgielda Awaiting requested review from pgielda

@galak galak Awaiting requested review from galak galak is a code owner

@anangl anangl Awaiting requested review from anangl anangl is a code owner

@bbolen bbolen Awaiting requested review from bbolen bbolen is a code owner

Assignees

@erwango erwango

Labels
area: Boards area: Build System area: Devicetree platform: nRF Nordic nRFx platform: NXP NXP platform: STM32 ST Micro STM32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet