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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

@JordanYates
Copy link
Collaborator

@JordanYates JordanYates commented Jul 28, 2021

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

@JordanYates JordanYates commented Jul 28, 2021

Loading

include/devicetree.h Outdated Show resolved Hide resolved
Loading
Copy link
Contributor

@galak galak left a comment

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

Loading

include/devicetree.h Outdated Show resolved Hide resolved
Loading
tests/lib/devicetree/api/app.overlay Outdated Show resolved Hide resolved
Loading
@mbolivar-nordic
Copy link
Member

@mbolivar-nordic mbolivar-nordic commented Jul 29, 2021

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.

Loading

@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch from 8fdfe92 to bb79da9 Aug 1, 2021
@JordanYates
Copy link
Collaborator Author

@JordanYates JordanYates commented Aug 1, 2021

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

Loading

@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch 2 times, most recently from 99fd7ae to 42a4865 Aug 1, 2021
@JordanYates
Copy link
Collaborator Author

@JordanYates JordanYates commented Nov 12, 2021

@JordanYates could you rebase it please?

Done

Loading

@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch from a859161 to efd38c5 Nov 17, 2021
@JordanYates JordanYates requested review from anangl and bbolen as code owners Nov 17, 2021
@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch from efd38c5 to da65c0d Nov 20, 2021
@JordanYates
Copy link
Collaborator Author

@JordanYates JordanYates commented Nov 21, 2021

@ioannisg can you provide any input as to why the kernel.common.stack_protection_arm_fpu_sharing tests are failing on the mps3_an547 board? I'm not sure what this PR could be doing to cause that.

@carlescufi The remaining failures (19 and 20) appear to be because the CI is timing out?

Loading

@erwango
Copy link
Member

@erwango erwango commented Nov 22, 2021

@carlocaione, I see some overlap with #40422. Would you mind having a look ?

Loading

@carlocaione
Copy link
Collaborator

@carlocaione carlocaione commented Nov 22, 2021

@carlocaione, I see some overlap with #40422. Would you mind having a look ?

Oooh, indeed there is overlap. What is missing here is (that is somehow present in my POC) is:

  • a way to automatically generate the LINKER_DT_REGION_FROM_NODE (having to manually modify the linker script every time a new region is added into the DT is not ideal)
  • a way to automatically generate the sections as well (is it really useful to be able to declare regions but not sections?)
  • a way to define the MPU attributes for the memory regions (this is very important IMO)
  • related to the previous point an easy way for user code to cycle through the regions
  • an easy way to actually use and place variables in the regions (for example if we want to place non-cacheable buffers in a non-cacheable region)

Loading

@JordanYates
Copy link
Collaborator Author

@JordanYates JordanYates commented Nov 22, 2021

  • a way to automatically generate the LINKER_DT_REGION_FROM_NODE (having to manually modify the linker script every time a new region is added into the DT is not ideal)
  • a way to automatically generate the sections as well (is it really useful to be able to declare regions but not sections?)

That was meant to be the next step in this PR series. This PR was a step that could exist standalone and still provide value.
Unfortunately it hasn't really progressed quickly :)

  • a way to define the MPU attributes for the memory regions (this is very important IMO)

I would see that as a new common property, zephyr,mpu-attr or similar (maybe in linker.yaml?).

  • related to the previous point an easy way for user code to cycle through the regions

I think this would probably be solved with the first 2, most likely with some form of DT_FOREACH_COMPAT macro.

  • an easy way to actually use and place variables in the regions (for example if we want to place non-cacheable buffers in a non-cacheable region)

This is trivial if the section names derive from devicetree in the same way as the region names do.

#define LINKER_DT_NODE_SECTION_NAME(node)   DT_CAT(LINKER_DT_NODE_REGION_NAME(node), "_section")
#define LINKER_DT_PLACE_IN(node) __attribute__((__section__(LINKER_DT_NODE_SECTION_NAME(node))


#define MY_SECTION DT_NODELABEL(non-cacheable)
uint8_t section_buffer[DT_REG_SIZE(MY_SECTION)] LINKER_DT_PLACE_IN(MY_SECTION);

Loading

@carlocaione
Copy link
Collaborator

@carlocaione carlocaione commented Nov 22, 2021

That was meant to be the next step in this PR series. This PR was a step that could exist standalone and still provide value. Unfortunately it hasn't really progressed quickly :)

Fair enough, glad to hear that this is on the menu then :D

I would see that as a new common property, zephyr,mpu-attr or similar (maybe in linker.yaml?).

Yes, please. In #40422 I put that as property in mmio-sram.yaml but I don't have a strong opinion on this.

I think this would probably be solved with the first 2, most likely with some form of DT_FOREACH_COMPAT macro.

Yup,

DT_INST_FOREACH_STATUS_OKAY_VARGS(_CHECK_ATTR_FN, FN, __VA_ARGS__)

This is trivial if the section names derive from devicetree in the same way as the region names do.

Agree.

Loading

* @param attr region attributes
*/
#define _REGION_DECLARE(node_id, attr) \
LINKER_DT_NODE_REGION_NAME(node_id)(attr) : \
Copy link
Collaborator

@carlocaione carlocaione Nov 22, 2021

Choose a reason for hiding this comment

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

you would probably want to consider removing attr at all (that at least for the GNU linker is optional). The problem is that when the MPU is present the actual attribute of the region (in terms of RWX) is defined by the MPU and by the attributes that we are using to mark the region and not by the linker.

If you think to introduce the mpu-attr we could think of obtaining this attr from the mpu-attr but that is hard.

Loading

Copy link
Collaborator Author

@JordanYates JordanYates Nov 22, 2021

Choose a reason for hiding this comment

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

That can be done in future PR's if needed. This is purely about updating where the names are coming from, I don't want to potentially introduce other problems here.

Loading

Copy link
Collaborator Author

@JordanYates JordanYates Nov 22, 2021

Choose a reason for hiding this comment

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

This PR is large enough as it it

Loading

Copy link
Collaborator

@carlocaione carlocaione Nov 22, 2021

Choose a reason for hiding this comment

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

Ok, I'm only concerned that all the future needed changes (mpu-attr, the FOR_EACH_.. macro, this fix on the _REGION_DECLARE, etc...) will require deep changes to the current state of this PR.

I have no objections on this PR if this is considered a first step.

Loading

@carlocaione
Copy link
Collaborator

@carlocaione carlocaione commented Nov 22, 2021

@JordanYates can you rebase and trigger the CI again?

Loading

@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch from da65c0d to 984fd36 Nov 22, 2021
@carlocaione
Copy link
Collaborator

@carlocaione carlocaione commented Nov 22, 2021

Opened #40569 for the failing test

Loading

@carlocaione
Copy link
Collaborator

@carlocaione carlocaione commented Nov 23, 2021

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.

Loading

@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch from 984fd36 to 1e97b19 Nov 23, 2021
@erwango
Copy link
Member

@erwango 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 ?

Loading

JordanYates added 7 commits Nov 25, 2021
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>
Add checks to ensure that `zephyr,linker-region` property values are
always globally unique.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
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>
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>
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>
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>
Link variables into derived section names instead of hardcoded names.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
@JordanYates JordanYates force-pushed the 210728_linker_name_prop branch from 1e97b19 to 46fd96a Nov 25, 2021
@JordanYates
Copy link
Collaborator Author

@JordanYates JordanYates commented Nov 25, 2021

@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

Loading

Copy link
Collaborator

@tejlmand tejlmand left a comment

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.

Loading

@@ -4,7 +4,7 @@ description: Cortex-M DTCM (Data Tightly Coupled Memory)

compatible: "arm,dtcm"

include: base.yaml
include: [base.yaml, linker.yaml]
Copy link
Collaborator

@tejlmand tejlmand Nov 25, 2021

Choose a reason for hiding this comment

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

I know this is used when generating the linker script, but seen from devicetree point of view, I don't think it should be using name linker.

How would mem-region.yaml sound ?

Not blocking comment, just a suggestion.

Loading

# Common fields for devices resulting in linker memory regions

properties:
zephyr,linker-region:
Copy link
Collaborator

@tejlmand tejlmand Nov 25, 2021

Choose a reason for hiding this comment

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

maybe use memory-region instead of linker-region.

Loading

@@ -0,0 +1,9 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.13.1)
Copy link
Collaborator

@tejlmand tejlmand Nov 25, 2021

Choose a reason for hiding this comment

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

Suggested change
cmake_minimum_required(VERSION 3.13.1)
cmake_minimum_required(VERSION 3.20)

Loading

#define LINKER_DT_NODE_REGION_NAME(node_id) \
DT_PROP_OR(node_id, zephyr_linker_region, DT_NODE_PATH(node_id))
Copy link
Collaborator

@tejlmand tejlmand Nov 25, 2021

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.

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

function(zephyr_linker_dts_memory)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment