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

RFC: memory regions from devicetree & explicit code locations #33656

Closed

Conversation

JordanYates
Copy link
Collaborator

@JordanYates JordanYates commented Mar 24, 2021

** Update **
Please refer to #34167 for a walkthrough of the issue reasoning. This PR may reasonably be closed in favor of following the more independent steps outlined there.

** Original**
This patch series is prototyping a solution to several problems related to the linker and memory layout. A minimal solution is developed and applied to the nrf52840dk_nrf52840 board to demonstrate its use.
The primary problem can be stated as:

  • The flash memory on a SoC is partitioned in devicetree (settings partition, bootloaders, etc), but that partitioning is not automatically applied by the linker.

The biggest problem that appears as a result is that for standard applications, the linker makes the assumption that the entirety of flash is available for code (unless Kconfig options are set separately). This results in problems like #27471.

The assumption underlying this solution is that different code layouts of applications running on a board (standalone, chain-loadable, etc) is roughly analogous to minor hardware revisions (https://docs.zephyrproject.org/latest/guides/porting/board_porting.html#multiple-board-revisions).

The broad idea is to describe different code layouts in devicetree overlay files in the board directory. Partitions are then instantiated as linker memory regions, which lets the linker place code at defined locations, and detect when partitions have been overrun.

The first step is to extend the board revision syntax board@revision to support different code layouts board%layout/board@revision%layout. This extension is proposed as manually specifying devicetree overlays on the command line is verbose and error prone (-- -DDTC_OVERLAY_FILE=zephyr/boards/arm/board/board_mcuboot.overlay). If a layout is not specified, the default layout is selected.

The second step automatically adds the layout overlay (and an optional Kconfig settings file). The Kconfig file is to allow for configurations such as board%mcuboot to automatically enable relevant symbols like CONFIG_BOOTLOADER_MCUBOOT.

The final step automatically creates a linker memory region for all partitions under the devicetree chosen node zephyr,flash.
The partition pointed to by the chosen node zephyr,code-partition is used as the output memory region for TEXT/DATA.
Additional memory regions can be created by pointing the linker directly at a devicetree node, which can clean up the linker scripts considerably.

Example outputs

west build -b nrf52840dk_nrf52840 zephyr\samples\basic\minimal
...
[139/139] Linking C executable zephyr\zephyr.elf
Memory region         Used Size  Region Size  %age Used
            SRAM:        5600 B       256 KB      2.14%
         storage:          0 GB        32 KB      0.00%
            code:       16956 B       992 KB      1.67%
        IDT_LIST:          0 GB         2 KB      0.00%
west build -b nrf52840dk_nrf52840%mcuboot zephyr\samples\basic\minimal
...
[139/139] Linking C executable zephyr\zephyr.elf
Memory region         Used Size  Region Size  %age Used
            SRAM:        5600 B       256 KB      2.14%
         storage:          0 GB        32 KB      0.00%
         mcuboot:          0 GB        48 KB      0.00%
         image-0:       17628 B       412 KB      4.18%
         image-1:          0 GB       412 KB      0.00%
   image-scratch:          0 GB       120 KB      0.00%
        IDT_LIST:          0 GB         2 KB      0.00%
west build -b nrf52840dk_nrf52840%bootloader zephyr\samples\basic\minimal
...
[139/139] Linking C executable zephyr\zephyr.elf
Memory region         Used Size  Region Size  %age Used
            SRAM:        5600 B       256 KB      2.14%
         storage:          0 GB        32 KB      0.00%
         mcuboot:       16956 B        48 KB     34.50%
         image-0:          0 GB       412 KB      0.00%
         image-1:          0 GB       412 KB      0.00%
   image-scratch:          0 GB       120 KB      0.00%
        IDT_LIST:          0 GB         2 KB      0.00%

@JordanYates
Copy link
Collaborator Author

Feedback on this reworked proposal would be appreciated @Laczen @ioannisg @galak

@carlescufi carlescufi added the dev-review To be discussed in dev-review meeting label Apr 1, 2021
Jordan Yates added 2 commits April 1, 2021 21:30
Add a second modifier to the board string that allows for specifying a
code layout. Both board revision and board layout can be specified
independently, or together. If specified together, the layout must
follow the revision.

A board indicates that it supports multiple code layouts by the presence
of the `layouts` folder in the board folder. If this folder exists and
no layout is specified, the `default` layout is selected. The selected
layout must exactly match an overlay file in that folder.

An optional Kconfig configuration file may also exist.

For example, for a layout of `mcuboot` on board `test`:
```
test
 - layouts
   - default.overlay
   - mcuboot.overlay
   - mcuboot.conf
  -test.dts
  -...
```

The valid board specifiers are now:
`board`
`board@revision`
`board%layout`
`board@revision%layout`

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
Apply board dts overlays and kconfig configurations when `BOARD_LAYOUT`
is specified.

Fixes #31487.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
Jordan Yates added 4 commits April 1, 2021 21:33
Convert the nrf52840dk_nrf52840 board to use the new board layouts as
a demonstration of the changes required.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
Adds a public macro for creating memory regions from devicetree
partitions. All partitions under `chosen/zephyr,flash` result in an
output region.

`DT_PARTITION_REGIONS` declares the memory regions for consumption by
ld. While macro limitations mean that region names are quoted and there
are no seperators between regions, ld appears to handle this fine.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
Generate linker memory regions from flash partitions that exist under
the chosen node `zephyr,flash`. This makes the linker aware of the
various flash memory regions. If code overflows from one region to
another, the linker will complain. This additionally enables variables
to be placed at absolute memory addresses by placing sections inside
the generated memory regions.

Fixes #31073, #27471.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
Simplify bespoke memory region definitions by declaring the regions
directly from devicetree nodes.

Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
@galak
Copy link
Collaborator

galak commented Apr 8, 2021

A highly level comment is I think it would be good to split this PR in two. One is the linker.ld changes and use of devicetree and the other PR the introduction of the layout concept.

@gregshue
Copy link

gregshue commented Apr 8, 2021

@JordanYates
The build system already can pick up boards and overlays (among other things) from application modules. The capability is illustrated here and in general demonstrated here.

The zephyrproject-rtos/example-application repository scope includes demonstrating how to configure application repositories so the build system sees DTS overlays from the application repository tree.

Can these be used to solve your problem? If not, what is lacking?

@JordanYates
Copy link
Collaborator Author

The build system already can pick up boards and overlays (among other things) from application modules. The capability is illustrated here and in general demonstrated here.

This is not sufficient as the core functionality that this PR enables is dynamically generating the "correct" flash partitions depending on what type of application is being built. These flash partitions are then used to generate the memory regions supplied to the linker.

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.

The first step is to extend the board revision syntax board@revision to support different code layouts board%layout/board@revision%layout.

This really don't belong with the BOARD argument.

Flash layout is a (user) configuration that is depending directly on the sample you are building and for which we have DTC_OVERLAY_FILE and CONFIG_FILE / OVERLAY_CONFIG.

Note, if we have a <board>_mcuboot.overlay, users can easily copy that file to their sample, for example:
cp nrf52840dk_nrf52840.overlay <sample>/boards/nrf52840dk_nrf52840.overlay

This extension is proposed as manually specifying devicetree overlays on the command line is verbose and error prone (-- -DDTC_OVERLAY_FILE=zephyr/boards/arm/board/board_mcuboot.overlay).

If this is not sufficient and Zephyr would benefit from an enhanced way of looking up files, I think a dedicated issue should be raised for further discussions.
https://docs.zephyrproject.org/latest/guides/dts/howtos.html#set-devicetree-overlays

@@ -162,7 +147,7 @@ SECTIONS

GROUP_START(ROMABLE_REGION)

_image_rom_start = ROM_ADDR;
_image_rom_start = .;
Copy link
Collaborator

@tejlmand tejlmand Apr 12, 2021

Choose a reason for hiding this comment

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

This is changing the value of _image_rom_start from a calculated address to 0x0.

This means that code that refers to _image_rom_start might get a wrong value.

This also results in:

_flash_used = LOADADDR(.last_section) - _image_rom_start;

being calculated wrongly.

As example, building hello world for cc3235sf_launchxl with CONFIG_XIP=n on master gives me:

$ objdump -x zephyr/zephyr.elf |grep _image_rom_start
20000000 g       *ABS*  00000000 _image_rom_start
$ objdump -x build_xip_ref/zephyr/zephyr.elf |grep _flash_used
00004158 g       *ABS*  00000000 _flash_used

but with this PR:

$ objdump -x build_xip/zephyr/zephyr.elf |grep _image_rom_start
00000000 g       rom_start      00000000 _image_rom_start
$ objdump -x build_xip/zephyr/zephyr.elf |grep _flash_used
20004158 g       *ABS*  00000000 _flash_used

so any code relying on those two symbols will fail.

#define CCFG_SIZE 88
#define ROM_SIZE (CONFIG_FLASH_SIZE*1K - CONFIG_FLASH_LOAD_OFFSET - \
CCFG_SIZE)
#define CCFG_ADDR (ROM_ADDR + ROM_SIZE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

CCFG_ADDR is still being used in the code below:

#ifdef CONFIG_HAS_TI_CCFG
    FLASH_CCFG            (rwx): ORIGIN = CCFG_ADDR, LENGTH = CCFG_SIZE
#endif

causing:

invalid origin for memory region FLASH_CCFG
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to be corrected in: #34185
So assume this is now pending a rebase after #34185 is merged.

@mbolivar-nordic
Copy link
Contributor

Removing dev-review as this was already discussed there; please add the label again if there's more to discuss at the meeting.

@mbolivar-nordic mbolivar-nordic removed the dev-review To be discussed in dev-review meeting label Apr 29, 2021
@carlescufi carlescufi requested a review from de-nordic May 11, 2021 09:54
@de-nordic
Copy link
Collaborator

I am not in favor of adding the % thing,, so I let myself drop some thoughts on alternative I have been considering.
(I am aware of #34167.)

I think the zephyr,code-partition and CONFIG_USE_DT_CODE_PARTITION could be dropped.
The zephyr,code-partition is problematic because if you have 2 or more partitions that are can be used for image then you will have to change that thing every time when you select app to be build for different partition.
This causes problems, for example, when you try to test Direct-XIP capability of mcuboot (execution of image directly from slot without moving it to first slot as it happens in "swap" mode).

Instead we should introduce compatible that will be set to entire partitioning schema, for example "mcuboot-schema". "zephyr-single" and one for marking the zephyr image, for example "zephyr,app-image" (these are examples, just to be able to present the general idea).
The "zephyr-single" does not have (probably) to be used because we only need to know about alternatives, and this would be default.

Having the "zephyr,app-image" compatible fixes one of problems with Direct-XIP because it probably could allow, with a lot of cmake magic, to iterate through the partitioning scheme, get info on all possible image positions and build Zephyr apps for all the slots at once.

But getting back to the partitioning: we can attach any number of "fixed-partitions" configurations to same flash, only the node labels need to be different (@galak, correct me here). Which means that it should be possible to have something like this:

&flash0 {
    mcuboot_schema: partitions {
         compatible = "fixed-partitions", "mcuboot-schema";
         ....
         boot_partition: partition@0 {
             compatible = "bootloader-mcuboot";
             label = "mcuboot";
             ....
         };
        slot0_partition: partition@c000 {
            compatible = "zephyr,app-image";
            ...
         };
         slot1_partition: partition@67000 {
            compatible = "zephyr,app-image";
            ...
         };
         ....
    };
};

&flash0 {
    single_schema: partitions {
         compatible = "fixed-partitions", "zephyr-single";
         ....
         single_boot_partition: partition@0 {
             compatible = "zephyr,app-image";
             ....
         };
        /* some other partition */
         ....
    };
};

withing single DTS configuration. This means that at compilation time Several partitioning schema should be able to co-exist (@galak ?)

We could probably provide more independent configurations for other bootloaders without no need for overlays or switching layouts.
There will be issue with flash map API, which will require changes to macros, so that the macros would properly select images, as it might be desired to give label property the same value (for example "storage') regardless of schema, which is not allowed now.

After DTS processing phase all listed schema are available, if provided, for board, at the same time and we can decide which will be picked basing on Kconfig options, for example: now we can even decide, basing on existence of "mcuboot-*" compatibles whether the CONFIG_MCUBOOT appears in Kconfig.

Having spaces for Zephyr app images with compatible set should also allow to, while processing DTS file, warn user that for example images have different sizes or warn that the smallest size defined by "zephyr,app-image" will be used as limit for linker.

User does not have to overlay both schema, for example if only single is used then user removes the single one and provides own configuration, same with mcuboot schema.
Not all devices have to provide all schema:, these that will not provide MCUBOOT schema will not have such option within Kconfig, these that do not provide the "zephyr-single" will have such option always enabled and not configurable; and configurations that provide neither end up in error during compilation.

What do you think about such approach @JordanYates , @mrrosen ?

@JordanYates
Copy link
Collaborator Author

I am not in favor of adding the % thing

This has been soundly rejected at this point, but it was never the important part of the proposal, simply a shorthand to specify a devicetree overlay.

The zephyr,code-partition is problematic because if you have 2 or more partitions that are can be used for image then you will have to change that thing every time when you select app to be build for different partition.

This is only problematic if we have no simple way of applying common devicetree overlays from the command line.
A CMake variable that specifies a particular layout (which can be easily set via west) eliminates this as a pain point IMO.

west build -b nrf52840dk_nrf52840 zephyr/samples/minimal # Overlays ${board_dir}/layouts/default.overlay
west build -b nrf52840dk_nrf52840 -l bootloader zephyr/samples/minimal # Overlays ${board_dir}/layouts/bootloader.overlay
west build -b nrf52840dk_nrf52840 -l xip_im1 zephyr/samples/minimal # Overlays ${board_dir}/layouts/xip_im1.overlay

You therefore setup ${board_dir}/board.dts with the 'common' flash partitions (NVS partition, etc), while each of the overlays setup the relevant partition layout and zephyr,code-partition.

But getting back to the partitioning: we can attach any number of "fixed-partitions" configurations to same flash

This is possible, but I don't believe this is the correct path forward. IMO zephyr.dts should contain the 'correct' flash partitioning for the current application, and nothing else (Standalone application should not include mcuboot partitions, and vice versa). Failure to do so bloats the flash map table (as I think you allude to), and makes it much harder to automatically generate linker sections (as demonstrated in #34537).

I am not convinced by the proposed compatibles and keeping the existing devicetree/Kconfig dance to configure the linker. Under my preferred scenario (where zephyr.dts doesn't contain extra information), the solution is reduced to automatically generating a linker region for all children of certain devicetree nodes, both Flash and RAM partitions. The solution in #34537 can be trivially extended to support this.

@mrrosen
Copy link
Contributor

mrrosen commented May 12, 2021

I agree with @JordanYates that limiting the devicetree to only include the set of partitions actually used by the application is the right approach and that by using overlays, a user can select the desired set of partitions from a few commons ones provided by the board or provide their own in their application.

But @de-nordic does raise a good issue that I also ran into in my own project: once you have a nice way of creating the desired partitions in devicetree and provide this information to the linker in the form of memory regions (addressed in this MR and discussed more generally in #34167), how do you instruct Zephyr on how to link all the unspecified code/data and what artifacts do you want out of the build.

The current solution for the first part is to use chosen DT nodes (zephyr,code-partition for read-only data like text/rodata and zephyr,sram for read-write data like data/bss), but Im not sure if thats the correct approach. The main issue being that while its pretty easy to redirect the linker by providing a different overlay, it does require the entire code be rebuilt (this is a potential solution to the Direct-XIP just using different overlays for slot1 and slot2 that just change the code partition, but again, its not ideal the code has to be completely rebuilt when the only step that needs to change is the linking).

Another somewhat related issue is that for certain partitions/memory regions, it would be good to generate separate binaries for them instead of a single monolithic hex/binary at the end (for example, you have a separate flash in your system that needs separate binaries to program but needs to be linked with the application); this is a problem I ran into recently and had to somewhat hack a solution using a second configuration file parsed by CMake.

In both cases, while devicetree is providing the build system and code with what memories are available in the hardware and how they are partitioned (ie, spelling out all the linkable memory in the system), Im not sure its the right place to actually instruct the build system where the link the code and data and what artifacts to generate during the build (I dont think Kconfig is the right place either as it runs into the same issues as devicetree in terms of rebuilding unnecessarily and isnt as flexible). Im sure this is a much larger discussion than just this MR but I did want to raise it in response to @de-nordic 's comments on Direct-XIP; while devicetree can solve that problem (somewhat), Im not sure its the right strategy moving forward.

@de-nordic
Copy link
Collaborator

I agree with @JordanYates that limiting the devicetree to only include the set of partitions actually used by the application is the right approach and that by using overlays, a user can select the desired set of partitions from a few commons ones provided by the board or provide their own in their application.

There is actually is good argument for having mcuboot layout as separate layout overlay, so that user could include it into own dts of own board to bootstrap work: we support mcuboot so having basic description of fixed partition layout that gives support to mcuboot is desired as a working base, cut for a specific chip (or a family); although the user may finally even override that when it would seem that the app would not required that much flash yet storage or other flash area would be desired.
But this is useful for out-of-tree, non-dev boards.

The dev-boards, that are "known" by the Zephyr code are different case, as they serve different purpose. Heaving all configurations in one file allows you to switch, with the same DTS, without re-parsing it, to build mcuboot and single app conf just by adding or removing CONFIG_MCUBOOT to config overlay.
In call cases to build different layouts you will probably have to have different prj.conf or proper overlays to do so (for example to set CONFIG_MCUBOOT), in case when you have both layouts available at once, all you change is config overlay; when you have them separate then you end up changing project overlay and build invocation.
When both layouts are included, You may even pick generated zephyr.dts and use it as a ready hardware description for various prj.conf.

The existence of all layouts supported out-of-tree for a board in single DTS is just demonstratpion that these are alternatives you may choose from, user may remove or replace any of them with overlays. as it may do with any other dev board peripheral.

We provide generic configurations for dev boards to make them work with samples and tests, although sometimes overlays are required, adding all partitions' layouts that are supported at once would be in line with that approach. The nrf52840dk_nrf52840, as an example, gives configuration arduino header and mx25 in board dts, even though neither of these may be in use by an application or a sample.

The provided DTS may only support what we know is possible and that is limited to layout of dev board and samples that run on them.

(...) it does require the entire code be rebuilt (this is a potential solution to the Direct-XIP just using different overlays for slot1 and slot2 that just change the code partition, but again, its not ideal the code has to be completely rebuilt when the only step that needs to change is the linking).

I have used that approach, although I do not like it because it basically means that I had to change hardware description to move software to address, and restart built from the bottom (Direct-XIP scripts that builds "other-slot" application #29974).
You have a board, designed for specific task and described via DTS, all the mem regions, etc: why do you have to alter this (via overlay) while trying to move app around in mem? You are not altering the board.

Shouldn't that be done by compiler and linker basing on decision by user where to put the application?

I know that "fixed-partition" definitions can not be strictly affiliated with either software or hardware configuration: they usually describe constrains for the application (software) placement in regard of a given board (hardware), but also may gather different flash devices into single map of storage devices.

Another somewhat related issue is that for certain partitions/memory regions, it would be good to generate separate binaries for them instead of a single monolithic hex/binary at the end (for example, you have a separate flash in your system that needs separate binaries to program but needs to be linked with the application); this is a problem I ran into recently and had to somewhat hack a solution using a second configuration file parsed by CMake.

Yes, that would probably be desired, this is "move problem to linker" approach. When this would be supported you could pre-build app and then decide which image you want to link it to: that would be very nice because moving app from single/no-mcuboot to slot1 or slot2 (or other) would be just a matter of re-linking it.

In both cases, while devicetree is providing the build system and code with what memories are available in the hardware and how they are partitioned (ie, spelling out all the linkable memory in the system), Im not sure its the right place to actually instruct the build system where the link the code and data and what artifacts to generate during the build (I dont think Kconfig is the right place either as it runs into the same issues as devicetree in terms of rebuilding unnecessarily and isnt as flexible).

I agree that both the zephyr,code-partition and "fixed-partitions table both cross the line from hardware description to instructing build where to link code. In case of the later it is more about giving constrains on regions where you may put the code, the first one is directly stating what will be executed, and then it is possible to override this with Kconfig with no warning which may render you confused when you change the zephyr,code-partition yet your app does not move.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@de-nordic
Copy link
Collaborator

@JordanYates Why are you closing the issue?

@JordanYates
Copy link
Collaborator Author

I've closed the PR because every aspect of the approach has changed in response to feedback.
It was also too ambitious, so future PR's will address one aspect or another.

@de-nordic
Copy link
Collaborator

I've closed the PR because every aspect of the approach has changed in response to feedback.
It was also too ambitious, so future PR's will address one aspect or another.

Thanks for explaining. I hope that the idea on switching layout via command line will come back at some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants