Skip to content

Conversation

@carlocaione
Copy link
Contributor

@carlocaione carlocaione commented Jan 20, 2022

Introduce a new zephyr,memory-region compatible to be used when a new memory region must be created in the linker script from the devicetree node using the compatible.

Remove also the LINKER_DT_REGION_FROM_NODE macro and add a new LINKER_DT_REGION macro to cycle through all the compatible regions.

This is a preparatory work to be integrated with #41223 so that under one single compatible we have all the properties to create a new region in the linker script and set the MPU properties of the region using the info coming from the DT.

@JordanYates
Copy link
Contributor

AFAICT we've lost the rw or rwx attributes on the memory regions with this PR.
I suspect we'll need an additional property called zephyr,memory-region-attr or similar

gmarull
gmarull previously approved these changes Jan 26, 2022
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

One minor documentation comment

@carlocaione
Copy link
Contributor Author

@tejlmand can you take a look?

@carlescufi carlescufi removed the request for review from erwango January 27, 2022 12:46
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.

Great work.

We need to be able to be able to fetch all zephyr,memory-region compatibles in same way here:

# TI CCFG Registers
zephyr_linker_dts_memory(NODELABEL ti_ccfg_partition FLAGS rwx)
# Data & Instruction Tightly Coupled Memory
zephyr_linker_dts_memory(CHOSEN "zephyr,itcm" FLAGS rw)
zephyr_linker_dts_memory(CHOSEN "zephyr,dtcm" FLAGS rw)
zephyr_linker_dts_memory(NODELABEL sram1 FLAGS rw)
zephyr_linker_dts_memory(NODELABEL sram2 FLAGS rw)
zephyr_linker_dts_memory(NODELABEL sram3 FLAGS rw)
zephyr_linker_dts_memory(NODELABEL sram4 FLAGS rw)
zephyr_linker_dts_memory(NODELABEL sdram1 FLAGS rw)
zephyr_linker_dts_memory(NODELABEL sdram2 FLAGS rw)
zephyr_linker_dts_memory(NODELABEL backup_sram FLAGS rw)

so that code can use same principle.

Everything else looks good.
Let me know if you would like me to take a look at it.

@carlocaione
Copy link
Contributor Author

carlocaione commented Feb 2, 2022

@tejlmand I gave it a shot (added 2 commits to implement what you requested).

@mbolivar-nordic you want probably take a look at this again.

@carlocaione carlocaione requested review from tejlmand and removed request for ABOSTM February 7, 2022 09:31
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.

Looks very good, thanks for putting this together.

A couple of very minor nits.

Copy link
Contributor

Choose a reason for hiding this comment

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

When testing with arty_a7_arm_designstart_m3 this code results in DTCM and ITCM being defined on the same line like this:

MEMORY
    {
    FLASH (rx) : ORIGIN = (0x0 + 0x0), LENGTH = (32*1K - 0x0)
    SRAM (wx) : ORIGIN = 0x20000000, LENGTH = (32 * 1K)
    "ITCM" : ORIGIN = 0, LENGTH = 32768 "DTCM" : ORIGIN = 536870912, LENGTH = 32768
    IDT_LIST (wx) : ORIGIN = 0xFFFFF7FF, LENGTH = 2K
    }

it's not invalid code, but would be nice if each could have their own line.

This looks very nice when using -DCONFIG_CMAKE_LINKER_GENERATOR=y 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeeeeah, problem is that adding a new line in a macro using the preprocessor is not trivial (or even feasible?). I'll try to see if there is any way to achieve that but IIRC I already tried in the past and failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, not easy at all and I can't think of a solution, just wanted to mention it.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

I'm happy with the devicetree portions; @tejlmand should decide about the build system parts.

One nonblocking suggestion for the script.

Thanks for your persistence, @carlocaione.

Add a new cmake extension function:

  dt_comp_path(<var> COMPATIBLE <compatible> [INDEX <idx>])

to get a list of paths for the nodes with the given <compatible>. This
is useful when we have to cycle through several nodes with the same
compatible.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Introduce a new "zephyr,memory-region" compatible to be used when a new
memory region must be created in the linker script from the devicetree
nodes using the compatible.

Remove also the LINKER_DT_REGION_FROM_NODE macro and add a new
LINKER_DT_REGIONS macro to cycle through all the compatible regions.

In the same PR modify the DTS files and the linker scripts.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
As already done for the regular linker script, dinamically generates the
memory regions with the 'zephyr,memory-region' compatible also when
using the cmake linker generator.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
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.

Looks good.

Thanks for the effort.

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: Boards area: Build System area: Devicetree area: RISCV RISCV Architecture (32-bit & 64-bit) platform: nRF Nordic nRFx platform: NXP NXP platform: STM32 ST Micro STM32

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants