boards: st: h7: add memory-regions property to mac node #100910

bklaric1 · 2025-12-11T16:22:43Z

Changes

Added the memory-regions property to h7 series boards (to the ones supporting ethernet) in order to explicitly locate ethernet buffer and descriptor to an sram region inside the DT. Since sram3 was the first option and sram2 the fallback (in mpu_regions.c and sections.ld), I tried to use sram3 when possible and used sram2 otherwise.
Added #memory-region-cells to sram3 and sram2 nodes in dts/arm/st/h7 since it is needed by the memory-regions property.
Added the same check and definition of the sram_eth_node in mpu_regions.c and sections.ld as in boards: st: stm32h7s78_dk: add Ethernet support #99296.
Added memory-regions property to dts/bindings/ethernet/st,stm32h7-ethernet.yaml so twister-builds can run (will be added by the PR listed under Dependencies and can be removed later).

Dependencies

boards: st: stm32h7s78_dk: add Ethernet support #99296 (due to the addition of the memory-regions property to dts/bindings/ethernet/st,stm32h7-ethernet.yaml)

Notes

@etienne-lms suggested this change in #99296.

bklaric1 · 2025-12-11T18:43:59Z

Failed twister-builds have nothing to do with the changes this PR introduces, as far as I can say.

juickar

@bklaric1 try to rebase on main. A fix has been pushed for the CI failed test

bklaric1 · 2025-12-14T14:20:32Z

@juickar they seem to be queued now for almost a day. Was there another fix in the meantime?

juickar · 2025-12-15T08:08:40Z

@juickar they seem to be queued now for almost a day. Was there another fix in the meantime?

@bklaric1 No, but from what I witnessed CI also fails to launch on other PRs

bklaric1 · 2025-12-15T08:52:20Z

@juickar the first run before the rebase seemed to pass, at least the part that runs the build with code changes. So I guess I'll wait for the CI fix and/or review to update this branch to rerun the checks.

erwango

Thanks for the change, but in order to avoid impacting existing users, I'd prefer you keep the changes out of the boards.

So please assign the memory regions (memory-regions = <&sram2/3>;) in .dtsi files and keep existing pattern of using sram3 when available otherwise sram2.
This was, out of tree users won't see any difference.

erwango · 2025-12-15T14:04:46Z

soc/st/stm32/stm32h7x/mpu_regions.c

+#ifdef sram_eth_node
+BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)),
+	     "Remove property memory-regions from 'mac' node "
+	     "to locate ethernet buffers in system RAM.");
+#endif


Since this is not an option today, I'd suggest to remove

This was suggested by @etienne-lms. But I can remove it if wanted.

I think we should prevent one to set memory-regions property to the zephyr,sram phandle.
I'd suggest:

#include <zephyr/devicetree.h> #include <zephyr/arch/arm/mpu/arm_mpu_mem_cfg.h> +#if DT_NODE_HAS_STATUS_OKAY(DT_NODELABEL(mac)) +#define sram_eth_node DT_PHANDLE(DT_NODELABEL(mac), memory_regions) + +BUILD_ASSERT(DT_NODE_HAS_PROP(DT_NODELABEL(mac), memory_regions) && + DT_NODE_HAS_STATUS_OKAY(sram_eth_node), + "Ethernet MAC node must provide an enabled memory region for ethernet buffers"); +BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)), + "Ethernet buffer memory-region cannot be located in Zephyr system RAM."); +#endif /* mac node enabled */ + static const struct arm_mpu_region mpu_regions[] = { (...) #ifdef sram_eth_node MPU_REGION_ENTRY("SRAM_ETH_BUF", DT_REG_ADDR(sram_eth_node), REGION_RAM_NOCACHE_ATTR(REGION_16K)), MPU_REGION_ENTRY("SRAM_ETH_DESC", DT_REG_ADDR(sram_eth_node), REGION_PPB_ATTR(REGION_256B)), #endif }; (...)

I guess DT_NODE_HAS_PROP(DT_NODELABEL(mac), memory_regions) assertion can be removed if the property is tagged required.

bklaric1 · 2025-12-15T14:48:34Z

Thanks for the change, but in order to avoid impacting existing users, I'd prefer you keep the changes out of the boards.

So please assign the memory regions (memory-regions = <&sram2/3>;) in .dtsi files and keep existing pattern of using sram3 when available otherwise sram2. This was, out of tree users won't see any difference.

@erwango If I understand this correctly, this is what you're suggesting:

Remove memory-regions from all boards dts/dtsi files under boards/st/stm32h7*
Define memory-regions once in dts/arm/st/h7/stm32h7.dtsi with sram3 as default
Override sram3 with sram2 for boards that don't have sram3 in dts/arm/st/h7 dtsi files directly (for example: stm32h723.dtsi)
Restore the previous check/definition in mpu_regions.c and sections.ld

Eveything else stays the same (pending BUILD_ASSERT change).
Is that correct? If yes, then the addition of memory-regions doesn't do anything, since it gets ignored by check/definition in mpu_regions.c. Maybe we consider a hybrid approach?

erwango · 2025-12-15T16:28:00Z

@erwango If I understand this correctly, this is what you're suggesting:

Remove memory-regions from all boards dts/dtsi files under boards/st/stm32h7*

Yes

Define memory-regions once in dts/arm/st/h7/stm32h7.dtsi with sram3 as default

Yes

Override sram3 with sram2 for boards that don't have sram3 in dts/arm/st/h7 dtsi files directly (for example: stm32h723.dtsi)

I assume you're talking about SoC instead of "boards".
I'd prefer to use sram2 as default and overwrite with sram3 when available. Otherwise, for a out of tree user of a SoC not supported in tree that has no sram3, it would break.

Restore the previous check/definition in mpu_regions.c and sections.ld

Not required. Keep what is proposed in the PR, except the build assert proposing a fallback on System RAM.
My goal is to stay as close as possible to existing behavior for all users, including out of tree users.

etienne-lms · 2025-12-15T16:50:59Z

soc/st/stm32/stm32h7x/mpu_regions.c

+#ifdef sram_eth_node
+BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)),
+	     "Remove property memory-regions from 'mac' node "
+	     "to locate ethernet buffers in system RAM.");
+#endif


I think we should prevent one to set memory-regions property to the zephyr,sram phandle.
I'd suggest:

#include <zephyr/devicetree.h> #include <zephyr/arch/arm/mpu/arm_mpu_mem_cfg.h> +#if DT_NODE_HAS_STATUS_OKAY(DT_NODELABEL(mac)) +#define sram_eth_node DT_PHANDLE(DT_NODELABEL(mac), memory_regions) + +BUILD_ASSERT(DT_NODE_HAS_PROP(DT_NODELABEL(mac), memory_regions) && + DT_NODE_HAS_STATUS_OKAY(sram_eth_node), + "Ethernet MAC node must provide an enabled memory region for ethernet buffers"); +BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)), + "Ethernet buffer memory-region cannot be located in Zephyr system RAM."); +#endif /* mac node enabled */ + static const struct arm_mpu_region mpu_regions[] = { (...) #ifdef sram_eth_node MPU_REGION_ENTRY("SRAM_ETH_BUF", DT_REG_ADDR(sram_eth_node), REGION_RAM_NOCACHE_ATTR(REGION_16K)), MPU_REGION_ENTRY("SRAM_ETH_DESC", DT_REG_ADDR(sram_eth_node), REGION_PPB_ATTR(REGION_256B)), #endif }; (...)

I guess DT_NODE_HAS_PROP(DT_NODELABEL(mac), memory_regions) assertion can be removed if the property is tagged required.

dts/bindings/ethernet/st,stm32h7-ethernet.yaml

bklaric1 · 2025-12-16T12:06:49Z

@erwango @etienne-lms I amended the commit to add the requested changes, update the property to be required, and update the commit message.
Now the stm32h7.dtsi defines memory-regions and sets it to sram2 by default.
stm32h7a3.dtsi has sram3 override for a and b series (these series don't support Ethernet as of now, but the addition of memory-regions to stm32h7.dtsi requires the addition of this).
stm32h743.dtsi has sram3 override for 43 and 53 series.
stm32h745.dtsi has sram3 override for 45, 47, 55 and 57 series.
I hope I didn't forget anything/understood something wrong. In any case, I am open to feedback.

Use the memory-regions property in stm32h7.dtsi to explicitly locate ethernet buffer and descriptor to an sram region. Set sram2 as default and override with sram3 when available, in soc level .dtsi files. Add #memory-region-cells to sram2 and sram3 nodes in specific .dtsi files in dts/arm/st/h7, since it is needed by the memory-regions property. Also add the memory-regions property to stm32h563.dtsi since ethernet defined there uses the same .yaml file and thus requires the same addition as in stm32h7.dtsi. Also add #memory-region-cells to sram2 and sram3 nodes in stm32h562.dtsi. Use the same check and definition of the sram_eth_node in mpu_regions.c and sections.ld as in soc/st/stm32/h7rsx. Signed-off-by: Benjamin Klaric <benjamin.klaric01@gmail.com>

bklaric1 · 2025-12-16T13:31:58Z

Note: I also needed to add the memory-regions to the mac node in stm32h563.dtsi, since it uses the same .yaml file for ethernet
properties. Otherwise this error is reported:

devicetree error: 'memory-regions' is marked as required in 'properties:' in /__w/zephyr/zephyr/dts/bindings/ethernet/st,stm32h7-ethernet.yaml, but does not appear in <Node /soc/ethernet@40028000/ethernet in /__w/zephyr/zephyr/dts/arm/st/h5/stm32h563.dtsi:29>

Also note that I didn't override the mac region with sram3 here. The reason is that for other SOCs, the sram3 is smaller than sram2 but here it's the other way around.
Let me know if I need to add the override.

sonarqubecloud · 2025-12-16T13:45:46Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mathieuchopstm · 2025-12-16T13:52:45Z

dts/arm/st/h5/stm32h562.dtsi

 			compatible = "zephyr,memory-region", "mmio-sram";
 			reg = <0x20040000 DT_SIZE_K(64)>;
 			zephyr,memory-region = "SRAM2";
+			#memory-region-cells = <0>;


This should be declared in zephyr,memory-region.yml or somewhere else; IIRC edtlib has special casing for #xxx-cells properties so this works, but it should still be listed and documented as part of a binding.

As far as I can tell, this is needed because memory-regions is defined as phandle-array here:

zephyr/dts/bindings/reserved-memory/memory-region.yaml

Lines 7 to 10 in e6fd157

properties:

memory-regions:

type: phandle-array

description: List of memory region phandles

I don't know if it's possible to define it in memory-region.yaml directly (as <0>), and if it's wanted there, since <0> might not always be the use case.

I hope I didn't miss the point of your comment.

mathieuchopstm · 2025-12-16T13:53:56Z

dts/arm/st/h7/stm32h743.dtsi

 };
+
+&mac {
+	memory-regions = <&sram3>;


Nit: maybe memory-region-names should be used?

memory-region-names is not phandle but rather string-array:

zephyr/dts/bindings/reserved-memory/memory-region.yaml

Lines 12 to 14 in e6fd157

memory-region-names:

type: string-array

description: A list of names, one for each corresponding phandle in memory-region

Since we need to pass a referance to sram node, I am unsure if it will work with memory-region-names, tho I might be wrong.

Names used in memory-region-names are rather identifiers for the phandle indices used in a memory-regions property. It's useful when there can be several phandles, not a single one, see for examples clocks = ... and clock-names.
Since here we only expect one, I don't think it's really useful.

mathieuchopstm · 2025-12-16T13:56:03Z

soc/st/stm32/stm32h7x/sections.ld

-#endif
-
-#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node)
+#if defined(sram_eth_node) && DT_NODE_HAS_STATUS_OKAY(sram_eth_node)


defined(sram_eth_node) will always evaluate to 1: it is defined literally one line above

Either the check is not necessary:

Suggested change

#if defined(sram_eth_node) && DT_NODE_HAS_STATUS_OKAY(sram_eth_node)

#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node)

Or this is a mistake and you were looking for another macro (DT_NODE_EXISTS? DT_NODE_HAS_PROP on &mac?)

This was a copy&paste mistake I made from mpu_regions.c. The idea will be explained in the comment regarding mpu_regions.c. Will remove it here.

mathieuchopstm · 2025-12-16T13:56:16Z

soc/st/stm32/stm32h7x/mpu_regions.c

-#endif
-
-#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node)
+#if defined(sram_eth_node) && DT_NODE_HAS_STATUS_OKAY(sram_eth_node)


ditto wrt defined(sram_eth_node)

The idea here was, disregarding the mistake made in sections.ld, to check if sram_eth_node has status = "okay", meaning we check if the sram node passed with memory-regions has status = "okay", to avoid the case of passing sram node which is disabled. This is the case with the current implementation of this check here:

zephyr/soc/st/stm32/stm32h7rsx/mpu_regions.c

Lines 33 to 35 in e6fd157

#if DT_NODE_HAS_STATUS_OKAY(DT_NODELABEL(mac))

#define sram_eth_node DT_NODELABEL(sram2)

#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node)

sram2 is passed but it is disabled by default in DT.

Ah, right, I see now that (unlike in the linker script) here sram_eth_node is only defined if the ETH MAC node is enabled. So indeed here it does make sense.

mathieuchopstm · 2025-12-16T14:00:56Z

soc/st/stm32/stm32h7x/mpu_regions.c

 					 REGION_RAM_NOCACHE_ATTR(REGION_16K)),
 	MPU_REGION_ENTRY("SRAM_ETH_DESC",
 					 DT_REG_ADDR(sram_eth_node),
 					 REGION_PPB_ATTR(REGION_256B)),


Nit, outside PR scope: (ab)using REGION_PPB as a REGION_STRONGLY_ORDERED is really ugly.

As you said, it's outside the scope of PR and also beyond my current knowledge on the topic, but taking a guess here, could it be defined as REGION_RAM_NOCACHE_ATTR() instead? As far as I can tell, the goal is to make the region non-cacheable.

I assume there is a reason for which PPB was used rather than RAM_NOCACHE as done above, but I don't know which. So let's leave it as is for now...

bklaric1 · 2025-12-16T14:03:59Z

The failed twister-build (5) is related to missing memory-regions from stm32h7rs.dtsi. As originally stated, this PR depends on #99296, but maybe the dependency should be the other way around.
Should I then add everything related to the memory-regions with this PR and then just use it in another PR?

bklaric1 force-pushed the feat-stm32h7-memory_region_ethernet branch from f1b20a2 to 60a4695 Compare December 11, 2025 17:12

bklaric1 marked this pull request as ready for review December 11, 2025 18:44

zephyrbot added area: Ethernet area: Devicetree Bindings platform: STM32 ST Micro STM32 area: Boards/SoCs labels Dec 11, 2025

zephyrbot requested review from ClaCodes, FRASTM, GeorgeCGV, decsny, djiatsaf-st, erwango, etienne-lms, gautierg-st, juickar, lmajewski, maass-hamburg, mathieuchopstm and pdgendt December 11, 2025 18:45

zephyrbot assigned erwango Dec 11, 2025

juickar reviewed Dec 12, 2025

View reviewed changes

bklaric1 force-pushed the feat-stm32h7-memory_region_ethernet branch from 60a4695 to 81c21ce Compare December 13, 2025 16:04

erwango requested changes Dec 15, 2025

View reviewed changes

erwango mentioned this pull request Dec 15, 2025

boards: st: stm32h7s78_dk: add Ethernet support #99296

Open

etienne-lms reviewed Dec 15, 2025

View reviewed changes

bklaric1 force-pushed the feat-stm32h7-memory_region_ethernet branch from 81c21ce to 4161046 Compare December 16, 2025 12:01

bklaric1 force-pushed the feat-stm32h7-memory_region_ethernet branch from 4161046 to f6e1669 Compare December 16, 2025 12:30

bklaric1 force-pushed the feat-stm32h7-memory_region_ethernet branch from f6e1669 to f57c012 Compare December 16, 2025 13:28

mathieuchopstm reviewed Dec 16, 2025

View reviewed changes

	properties:
	memory-regions:
	type: phandle-array
	description: List of memory region phandles

	memory-region-names:
	type: string-array
	description: A list of names, one for each corresponding phandle in memory-region

	#if defined(sram_eth_node) && DT_NODE_HAS_STATUS_OKAY(sram_eth_node)
	#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node)

	#if DT_NODE_HAS_STATUS_OKAY(DT_NODELABEL(mac))
	#define sram_eth_node DT_NODELABEL(sram2)
	#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node)

boards: st: h7: add memory-regions property to mac node #100910

Are you sure you want to change the base?

boards: st: h7: add memory-regions property to mac node #100910

Conversation

bklaric1 commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Dependencies

Notes

Uh oh!

bklaric1 commented Dec 11, 2025

Uh oh!

juickar left a comment

Choose a reason for hiding this comment

Uh oh!

bklaric1 commented Dec 14, 2025

Uh oh!

juickar commented Dec 15, 2025

Uh oh!

bklaric1 commented Dec 15, 2025

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bklaric1 commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erwango commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bklaric1 commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bklaric1 commented Dec 16, 2025

Uh oh!

sonarqubecloud bot commented Dec 16, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bklaric1 Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bklaric1 Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bklaric1 commented Dec 16, 2025

Uh oh!

Reviewers

bklaric1 commented Dec 11, 2025 •

edited

Loading

bklaric1 commented Dec 15, 2025 •

edited

Loading

erwango commented Dec 15, 2025 •

edited

Loading

bklaric1 commented Dec 16, 2025 •

edited

Loading

bklaric1 Dec 16, 2025 •

edited

Loading

bklaric1 Dec 16, 2025 •

edited

Loading