Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions dts/bindings/base/mpu-region.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Copyright (c) 2021, Carlo Caione <[email protected]>
# SPDX-License-Identifier: Apache-2.0

description: MPU region

compatible: "mpu-region"

include: [base.yaml, mem-region.yaml]

properties:
zephyr,memory-region:
required: true

zephyr,memory-region-mpu:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reasoning behind adding this as a new compatible instead of adding the zephyr,memory-region-mpu property to mem-region.yaml? Is it solely to enable the usage of DT_FOREACH_STATUS_OKAY_VARGS to iterate over usages of zephyr,memory-region-mpu?

If so I wonder if it would make more sense to add a DT macro for iterating over all nodes that share a given property (Maybe limited to the zephyr, prefix).
This would also enable automatic generation of the linker regions as well:

LINKER_DT_REGION_FROM_NODE(DT_NODELABEL(ti_ccfg_partition), rwx)
/* Data & Instruction Tightly Coupled Memory */
LINKER_DT_REGION_FROM_NODE(DT_CHOSEN(zephyr_itcm), rw)
LINKER_DT_REGION_FROM_NODE(DT_CHOSEN(zephyr_dtcm), rw)
/* STM32 Core Coupled Memory */
LINKER_DT_REGION_FROM_NODE(DT_CHOSEN(zephyr_ccm), rw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the reasoning behind adding this as a new compatible instead of adding the zephyr,memory-region-mpu property to mem-region.yaml? Is it solely to enable the usage of DT_FOREACH_STATUS_OKAY_VARGS to iterate over usages of zephyr,memory-region-mpu?

Yes. In the first version of this PR it was done like that but that was changed when the usage of the additional compatible was proposed, see #41223 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. In the first version of this PR it was done like that but that was changed when the usage of the additional compatible was proposed, see #41223 (comment)

Right, that's where the compatible was proposed, but there was no discussion of its merits vs alternatives.
The iteration you really want is over anything that sets zephyr,memory-region-mpu, with the compatible being a bit of a hack to get that. The code I linked wants to do a similar thing eventually, iterate over all zephyr,memory-region.

Given that both systems want to do the same thing, their solution should probably be the same. A new compatible could be added to every SRAM node so that it does the same thing as this PR, or we can introduce a way to do DT_FOREACH_NODE_WITH_PROP(zephyr_memory_...) directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbolivar-nordic what's your opinion on this?

@JordanYates the problem i see with the DT_FOREACH_NODE_WITH_PROP() macro is that you have no control over the properties of the nodes you are cycling through. At least when using an additional compatible you can use a yaml file to exactly define what properties you expect (or you need) to find and use.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll need some more time to look at this, but I'm firing off a quick response in case it's helpful.

The zephyr DT tooling supports multiple compatibles in a single node, and unless I am mistaken, DT_FOREACH_STATUS_OKAY will work on a node that has multiple compatibles, even for compatibles that aren't the "main" compatible that matches a binding.

So for example if you have

foo { compatible = "a", "x"; };
bar { compatible = "b", "x"; };

Then you should be able to do DT_FOREACH_STATUS_OKAY(x, my_fn) even if there isn't a binding for x.

So maybe you want to have a 'marker' compatible that you can put on the nodes you are interested in, even if they have their own individual bindings?

If that's tedious or nonsense, feel free to ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #42008 to move to the multiple compatibles solution more gradually.

type: string
required: true
description: |
Signify that this node should result in a dedicated MPU region. The
region address and size are taken from the <reg> property, while the MPU
attribute is the value of this property.
71 changes: 71 additions & 0 deletions include/linker/devicetree_regions_mpu.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright (c) 2021, Carlo Caione <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <devicetree.h>
#include <linker/devicetree_regions.h>
#include <sys/util_macro.h>

/**
* @cond INTERNAL_HIDDEN
*/

#define _DT_COMPATIBLE mpu_region

/**
* Check that the node_id has both properties:
* - zephyr,memory-region-mpu
* - zephyr,memory-region
*
* and call the EXPAND_MPU_FN() macro
*/
#define _CHECK_ATTR_FN(node_id, EXPAND_MPU_FN, ...) \
COND_CODE_1(UTIL_AND(DT_NODE_HAS_PROP(node_id, zephyr_memory_region_mpu), \
DT_NODE_HAS_PROP(node_id, zephyr_memory_region)), \
(EXPAND_MPU_FN(node_id, __VA_ARGS__)), \
())

/**
* Call the user-provided MPU_FN() macro passing the expected arguments
*/
#define _EXPAND_MPU_FN(node_id, MPU_FN, ...) \
MPU_FN(LINKER_DT_NODE_REGION_NAME(node_id), \
DT_REG_ADDR(node_id), \
DT_REG_SIZE(node_id), \
DT_STRING_TOKEN(node_id, zephyr_memory_region_mpu)),

/**
* Call _CHECK_ATTR_FN() for each enabled node passing EXPAND_MPU_FN() as
* explicit argument and the user-provided MPU_FN() macro in __VA_ARGS__
*/
#define _CHECK_APPLY_FN(compat, EXPAND_MPU_FN, ...) \
DT_FOREACH_STATUS_OKAY_VARGS(compat, _CHECK_ATTR_FN, EXPAND_MPU_FN, __VA_ARGS__)

/**
* @endcond
*/

/**
* Helper macro to apply an MPU_FN macro to all the memory regions declared
* using the 'zephyr,memory-region-mpu' property and the 'mpu-region'
* compatible.
*
* @p mpu_fn must take the form:
*
* @code{.c}
* #define MPU_FN(name, base, size, attr) ...
* @endcode
*
* Example:
*
* @code{.c}
* static const struct arm_mpu_region mpu_regions[] = {
* ...
* DT_REGION_MPU(MPU_FN)
* ...
* };
* @endcode
*/
#define DT_REGION_MPU(mpu_fn) _CHECK_APPLY_FN(_DT_COMPATIBLE, _EXPAND_MPU_FN, mpu_fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

The DT_ namespace is reserved for the devicetree.h header and its sub-headers.

42 changes: 42 additions & 0 deletions soc/arm/common/cortex_m/arm_mpu_mem_cfg.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,48 @@
#error "Unsupported sram size configuration"
#endif

#define MPU_REGION_SIZE_32 REGION_32B
#define MPU_REGION_SIZE_64 REGION_64B
#define MPU_REGION_SIZE_128 REGION_128B
#define MPU_REGION_SIZE_256 REGION_256B
#define MPU_REGION_SIZE_512 REGION_512B
#define MPU_REGION_SIZE_1024 REGION_1K
#define MPU_REGION_SIZE_2048 REGION_2K
#define MPU_REGION_SIZE_4096 REGION_4K
#define MPU_REGION_SIZE_8192 REGION_8K
#define MPU_REGION_SIZE_16384 REGION_16K
#define MPU_REGION_SIZE_32768 REGION_32K
#define MPU_REGION_SIZE_65536 REGION_64K
#define MPU_REGION_SIZE_131072 REGION_128K
#define MPU_REGION_SIZE_262144 REGION_256K
#define MPU_REGION_SIZE_524288 REGION_512K
#define MPU_REGION_SIZE_1048576 REGION_1M
#define MPU_REGION_SIZE_2097152 REGION_2M
#define MPU_REGION_SIZE_4194304 REGION_4M
#define MPU_REGION_SIZE_8388608 REGION_8M
#define MPU_REGION_SIZE_16777216 REGION_16M
#define MPU_REGION_SIZE_33554432 REGION_32M
#define MPU_REGION_SIZE_67108864 REGION_64M
#define MPU_REGION_SIZE_134217728 REGION_128M
#define MPU_REGION_SIZE_268435456 REGION_256M
#define MPU_REGION_SIZE_536870912 REGION_512M

#define MPU_REGION_SIZE(x) MPU_REGION_SIZE_ ## x

#define ARM_MPU_REGION_INIT(p_name, p_base, p_size, p_attr) \
{ .name = p_name, \
.base = p_base, \
.attr = p_attr(MPU_REGION_SIZE(p_size)), \
}

#else

#define ARM_MPU_REGION_INIT(p_name, p_base, p_size, p_attr) \
{ .name = p_name, \
.base = p_base, \
.attr = p_attr(p_base, p_size), \
}

#endif /* !ARMV8_M_BASELINE && !ARMV8_M_MAINLINE */

#endif /* _ARM_CORTEX_M_MPU_MEM_CFG_H_ */
4 changes: 4 additions & 0 deletions soc/arm/common/cortex_m/arm_mpu_regions.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <sys/slist.h>
#include <arch/arm/aarch32/mpu/arm_mpu.h>
#include <linker/devicetree_regions_mpu.h>

#include "arm_mpu_mem_cfg.h"

Expand All @@ -28,6 +29,9 @@ static const struct arm_mpu_region mpu_regions[] = {
#else
REGION_RAM_ATTR(REGION_SRAM_SIZE)),
#endif

/* DT-defined regions */
DT_REGION_MPU(ARM_MPU_REGION_INIT)
};

const struct arm_mpu_config mpu_config = {
Expand Down
8 changes: 8 additions & 0 deletions tests/misc/arm_mpu_regions/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.20.0)

find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(arm_mpu_regions)

target_sources(app PRIVATE src/main.c)
35 changes: 35 additions & 0 deletions tests/misc/arm_mpu_regions/boards/mps2_an385.overlay
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright (c) 2021 Carlo Caione <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/

/ {
/delete-node/ memory@20000000;

sram0: memory@20000000 {
compatible = "mmio-sram";
reg = <0x20000000 0x200000>;
};

sram_cache: memory@20200000 {
compatible = "mpu-region", "mmio-sram";
reg = <0x20200000 0x100000>;
zephyr,memory-region = "SRAM_CACHE";
zephyr,memory-region-mpu = "REGION_RAM_ATTR";
};

sram_no_cache: memory@20300000 {
compatible = "mpu-region", "mmio-sram";
reg = <0x20300000 0x100000>;
zephyr,memory-region = "SRAM_NO_CACHE";
zephyr,memory-region-mpu = "REGION_RAM_NOCACHE_ATTR";
};

sram_dtcm_fake: memory@abcdabcd {
compatible = "mpu-region", "arm,dtcm";
reg = <0xabcdabcd 0x100000>;
zephyr,memory-region = "SRAM_DTCM_FAKE";
zephyr,memory-region-mpu = "REGION_RAM_ATTR";
};
};
22 changes: 22 additions & 0 deletions tests/misc/arm_mpu_regions/linker_sram_regions.ld
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright (c) Carlo Caione <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <autoconf.h>
#include <linker/sections.h>
#include <devicetree.h>

#include <linker/linker-defs.h>
#include <linker/linker-tool.h>
#include <linker/devicetree_regions.h>

MEMORY
{
LINKER_DT_REGION_FROM_NODE(DT_NODELABEL(sram_cache), rw)
LINKER_DT_REGION_FROM_NODE(DT_NODELABEL(sram_no_cache), rw)
LINKER_DT_REGION_FROM_NODE(DT_NODELABEL(sram_dtcm_fake), rw)
}

#include <arch/arm/aarch32/cortex_m/scripts/linker.ld>
3 changes: 3 additions & 0 deletions tests/misc/arm_mpu_regions/prj.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
CONFIG_ZTEST=y
CONFIG_HAVE_CUSTOM_LINKER_SCRIPT=y
CONFIG_CUSTOM_LINKER_SCRIPT="linker_sram_regions.ld"
56 changes: 56 additions & 0 deletions tests/misc/arm_mpu_regions/src/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright (c) 2021 Carlo Caione <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr.h>
#include <linker/linker-defs.h>
#include <sys/slist.h>
#include <arch/arm/aarch32/mpu/arm_mpu.h>
#include <ztest.h>
#include <string.h>

extern const struct arm_mpu_config mpu_config;

static arm_mpu_region_attr_t cacheable = REGION_RAM_ATTR(REGION_1M);
static arm_mpu_region_attr_t noncacheable = REGION_RAM_NOCACHE_ATTR(REGION_1M);

static void test_regions(void)
{
int cnt = 0;

for (size_t i = 0; i < mpu_config.num_regions; i++) {
const struct arm_mpu_region *r = &mpu_config.mpu_regions[i];

if (!strcmp(r->name, "SRAM_CACHE")) {
zassert_equal(r->base, 0x20200000, "Wrong base");
zassert_equal(r->attr.rasr, cacheable.rasr,
"Wrong attr for SRAM_CACHE");
cnt++;
} else if (!strcmp(r->name, "SRAM_NO_CACHE")) {
zassert_equal(r->base, 0x20300000, "Wrong base");
zassert_equal(r->attr.rasr, noncacheable.rasr,
"Wrong attr for SRAM_NO_CACHE");
cnt++;
} else if (!strcmp(r->name, "SRAM_DTCM_FAKE")) {
zassert_equal(r->base, 0xabcdabcd, "Wrong base");
zassert_equal(r->attr.rasr, cacheable.rasr,
"Wrong attr for SRAM_DTCM_FAKE");
cnt++;
}
}

if (cnt != 3) {
ztest_test_fail();
}
}

void test_main(void)
{
ztest_test_suite(test_c_arm_mpu_regions,
ztest_unit_test(test_regions)
);

ztest_run_test_suite(test_c_arm_mpu_regions);
}
4 changes: 4 additions & 0 deletions tests/misc/arm_mpu_regions/testcase.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
tests:
misc.arm_mpu_regions:
platform_allow: mps2_an385
tags: sample board sram mpu