Setup MPU for memory regions defined into the DT

[carlocaione]

Let's try this again from scratch.

What this is about

In #42008 we introduced the zephyr,memory-region compatible that can be used in the DT to automate the creation of memory regions in the linker script.

We want now to extend this a bit so that we can also leverage the same compatible to set the MPU attributes for the region.

The problem and the current situation

Currently the only way for a SoC (or a board) to configure at compile time the MPU regions is to add, in a soc-specific file, the mpu_config struct adding static entries for the new regions with the needed attributes (cacheable, non-cacheable, etc...). This exported struct is then read by the MPU driver at boot time and used to properly setup the MPU regions.

See for example nuvoton_npcx/npcx7/mpu_regions.c or arm/st_stm32/stm32h7/mpu_regions.c that are derived from the general file for cortex-M arm/common/cortex_m/arm_mpu_regions.c.

This is of course creating a lot of duplicate code

The proposal

With this patch it is now possible to declare the memory regions in the DT and define the MPU attributes for the regions using the zephyr,memory-region-mpu property. When this new property is present together with the zephyr,memory-region property and a the zephyr,memory-region compatible, the mpu_config struct is automatically extended at compile-time to host the DT defined regions with the correct MPU attributes.

So for example in the DT we can now have:

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

and a new region will be created called SRAM_CACHE and this new MPU region will be configured at boot time with the attribute REGION_RAM_ATTR.

Is this mandatory?

No. Each SoC or board can keep using the mpu_regions.c file to do as it pleases. Also the nodes that are not using the zephyr,memory-region-mpu are not configured in the MPU.

What is exactly 'zephyr,memory-region-mpu`?

This is actually a string passed to the MPU code as token, this string token is then interpreted by the MPU code (it could be a macro or a define for example).

[pdgendt]

PR #40422 also introduced a linker section label to place variables in the corresponding memory, will this be added in a future PR as well?

[carlocaione]

PR #40422 also introduced a linker section label to place variables in the corresponding memory, will this be added in a future PR as well

Possibly but I'm trying to approach things very gradually (but steadily :) so that is matter for another PR if this will be merged.

[andyross]

A little out of my depth with dts critique, but I'll make it anyway:

I'm not completely understanding the value of "zephyr,memory-region-mpu". I get that it's for platform-specific tuning/configuration. But... why not just add new dts attributes for that? So you could have, I dunno, "cortex_m_uncached = 1" or whatever just sitting in the device node and available via the standard macros to the driver in the standard way (though maybe we'd need to plumb this device node down into the MMU driver).

Arbitrary dts extension attributions are allowed by our schema layer, aren't they? I swear I've seen this done somewhere.

[carlocaione]

A little out of my depth with dts critique, but I'll make it anyway:

Every comment is welcome :)

I'm not completely understanding the value of "zephyr,memory-region-mpu". I get that it's for platform-specific tuning/configuration. But... why not just add new dts attributes for that? So you could have, I dunno, "cortex_m_uncached = 1" or whatever just sitting in the device node and available via the standard macros to the driver in the standard way (though maybe we'd need to plumb this device node down into the MMU driver).

Arbitrary dts extension attributions are allowed by our schema layer, aren't they? I swear I've seen this done somewhere.

Uhm, I'm not sure I have ever seen something like that. @mbolivar-nordic can you shed some light on this?

[mbolivar-nordic]

Arbitrary dts extension attributions are allowed by our schema layer, aren't they? I swear I've seen this done somewhere.

Uhm, I'm not sure I have ever seen something like that. @mbolivar-nordic can you shed some light on this?

I don't know what "extension attributions" means in this context. But there are 3 cases where a DTS property's value is available to C code:

1. It's a standard property (e.g. reg, interrupts, ranges) whose meaning is defined by the DT specification itself
2. There is a devicetree binding associated with the node's compatible property which defines the property's type and therefore how it should be represented in C
3. It's a property of the special /zephyr,user node that has "inferred" bindings and is reserved for application developer convenience

Gory details in https://docs.zephyrproject.org/latest/guides/dts/bindings.html#dt-bindings.

But TL;DR is that neither 1. nor 3. apply, so the value of the compatible is so that case 2 applies.

[andyross]

I was thinking I'd seen somewhere some handling where it was possible for derived code (app, whatever) to add bindings to compatibles that are defined elsewhere. Maybe that was a proposal or something?

Basically: this use of an "scratch" property for derived code seems clumsy to me. The point of devicetree as a syntax is that the nodes can have arbitrarily complicated configuration in a (sorta) human-readable syntax. It seems like sort of a wart if we can't use that here.

[mbolivar-nordic]

I was thinking I'd seen somewhere some handling where it was possible for derived code (app, whatever) to add bindings to compatibles that are defined elsewhere. Maybe that was a proposal or something?

That's not possible, no. You can extend a binding on a bus-specific basis, e.g. add properties when the node appears on a SPI bus that aren't present when the node appears on an I2C bus. But you can have at most one binding file per (compatible, bus) pair in the current tooling, where the bus may be "null" if there's a "global" or bus-independent binding.

Basically: this use of an "scratch" property for derived code seems clumsy to me. The point of devicetree as a syntax is that the nodes can have arbitrarily complicated configuration in a (sorta) human-readable syntax. It seems like sort of a wart if we can't use that here.

I don't really disagree that this is clumsy. But as a design decision, DT has to be zero overhead from the C programmer's point of view. So we have to know how to generate macros for every single node at build time, and we do that based on compatible properties in order to have some type checking and other validation in the mix. So there isn't a mechanism for general-purpose sprinkling of additional properties throughout the tree in arbitrary nodes in a way that is accessible to C.

We could debate whether it's worthwhile to add something like that, perhaps for a special predefined set of properties prefixed with zephyr, or so. But that's not possible today.

[carlocaione]

@andyross @mbolivar-nordic having now discussed on the DT usage, do you have any actionable on this patch?

[mbolivar-nordic]

@andyross @mbolivar-nordic having now discussed on the DT usage, do you have any actionable on this patch?

I'm fine with this approach and I appreciate the time you've invested in the PR description.

[JordanYates]

No complaints otherwise. DT properties that resolve to macros looks like fun :)

[JordanYates]

Looks good, thanks for the updates

[mbolivar-nordic]

@gmarull it's weird that this is passing the docs build check; there are various issues with it, such as this:

+* All the memory regions defined into the DT with the compatible
+  `zephyr,memory-region` and at least the attribute `zephyr,memory-region-mpu`
+  defining the MPU permissions for the memory region. See the next section for
+  more details.

Note the uses of the any role in the second line that don't look like they should resolve to anything...

[mbolivar-nordic]

I appreciate the documentation effort! Not sure if I missed it before, but it has a few issues. Please take the time to build and inspect the HTML. I know it's a pain since it takes a few minutes to build, but there are some things like code formatting that aren't coming out right, among other things.

I've fixed those and made some other suggested changes here; please review and squash, if you don't mind:

https://github.com/mbolivar-nordic/zephyr/tree/pull/43119

Specific notes:

• In the devicetree, let's please use "property", not "attribute", to be precise
• It's probably more confusing for beginners to use an acronym like "DT" without defining it first, so I expanded to "devicetree"
• Always use :dtcompatible: whenever referring to devicetree compatibles with bindings, so users get a link to the relevant page in the bindings index (https://docs.zephyrproject.org/latest/reference/devicetree/bindings.html). That way you don't have to document the currently legal token values: the bindings index has that same information, straight from YAML, so the documentation can't ever go stale.
• .rst is not like markdown; you need double-backticks for inline code.
• I think the devicetree_regions.h mention should be in the docs page for how cortex M uses this header, since that's above the DT bindings layer.
• If you can remember to use :zephyr_file: when referring to in-tree paths you think people should look at, that's nice as it links straight to the file on GitHub from the HTML.

[carlocaione]

I've fixed those and made some other suggested changes here; please review and squash, if you don't mind:

Thank you for having spent time on this, really appreciated. I'll be more careful in the future.

[JiafeiPan]

Hi, @carlocaione, may I know do you have a plan to extend this feature to arm64? thanks.

[carlocaione]

Hi, @carlocaione, may I know do you have a plan to extend this feature to arm64? thanks.

I'm not planning it but that should be pretty trivial. It should be enough to extend:

zephyr/arch/arm64/core/mmu.c

Lines 649 to 654 in c5dc184

	static const struct arm_mmu_flat_range mmu_zephyr_ranges[] = {
	/* Mark the zephyr execution regions (data, bss, noinit, etc.)
	* cacheable, read-write
	* Note: read-write region is marked execute-never internally
	*/

with LINKER_DT_REGION_MPU()

[JiafeiPan]

Hi, @carlocaione, may I know do you have a plan to extend this feature to arm64? thanks.

I'm not planning it but that should be pretty trivial. It should be enough to extend:

zephyr/arch/arm64/core/mmu.c

Lines 649 to 654 in c5dc184

	static const struct arm_mmu_flat_range mmu_zephyr_ranges[] = {
	/* Mark the zephyr execution regions (data, bss, noinit, etc.)
	* cacheable, read-write
	* Note: read-write region is marked execute-never internally
	*/

with LINKER_DT_REGION_MPU()

okay, I will have a try and let's keep in update, thanks.