-
Notifications
You must be signed in to change notification settings - Fork 8.4k
boards: st: stm32h7s78_dk: add Ethernet support #99296
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
base: main
Are you sure you want to change the base?
boards: st: stm32h7s78_dk: add Ethernet support #99296
Conversation
|
The #99505 should be resolved first before proceeding with the merge (if it even gets to that point), since the changes to |
|
Regarding the failed checks:
So I think this isn't relevant to the PR, but I might be wrong. Note: next commit will also add |
etienne-lms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you squash to 2 commits?
2fdc201 to
dc13beb
Compare
|
Squashed upcoming commit into 2. commit and added the requested changes. |
|
Regarding the failed Compliance Check: I don't see the trailing whitespace that is being reported, neither locally nor in GitHub Web Editor, so I can't really fix this. |
Your first commit introduces a trailing whitespace, which is then fixed by your second commit. This isn't allowed. |
dc13beb to
473226c
Compare
|
@pdgendt I edited the first commit and removed the trailing whitespace there. Hopefully this fixes the CI check error. |
This unique commit as-is makes sense to me.
Ok for another P-R. |
f24dfb6 to
a491c3a
Compare
|
@etienne-lms Amended the last commit to include the requested changes. Question: do I need to add a (second) Copyright line when I edit an existing file or only when I create a new file? |
39967ac to
9884696
Compare
9884696 to
a54f308
Compare
|
@etienne-lms I kindly ask again that you review my comments to your changes to see why the twister-builds fail (and how to fix the fail → revert to the |
a54f308 to
99ec211
Compare
| include: st,stm32-ethernet-common.yaml | ||
|
|
||
| properties: | ||
| memory-region: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe have it consistent between the stm ethernet macs, and do it like in dts/bindings/ethernet/st,stm32mp13-ethernet.yaml and use memory-regions (with an s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed the s. Will update.
But if we are trying to be consistent, shouldn't the property also include the required: true as in st,stm32mp13-ethernet.yaml?
zephyr/dts/bindings/ethernet/st,stm32mp13-ethernet.yaml
Lines 16 to 22 in a1970eb
| properties: | |
| memory-regions: | |
| required: true | |
| description: | | |
| Memory region in which the Ethernet DMA lists and data buffers will be | |
| placed. The memory region has to be non-cacheable and accessible by the | |
| Ethernet peripheral. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, the soc/st/stm32/stm32h7x doesn't use it and we still want to be able to have it on the main ram by not setting it.
Implied by
BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)),
"Remove property memory-region from 'mac' node "
"to locate ethernet buffers in system RAM.")
But that's something the ST people (@etienne-lms )should decide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the s across the commit.
99ec211 to
1f3a8e3
Compare
| compatible: "st,stm32h7-ethernet" | ||
|
|
||
| include: st,stm32-ethernet-common.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
include:
- "st,stm32-ethernet-common.yaml"
- "memory-region.yaml"
and remove type: phandle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do that, then I get the following error:
devicetree error: <Node /memory@30000000 in /zephyr/dts/arm/st/h7rs/stm32h7rs.dtsi:60> lacks #memory-region-cells
That line corresponds to sram1:
zephyr/dts/arm/st/h7rs/stm32h7rs.dtsi
Lines 60 to 65 in 923ece1
| sram1: memory@30000000 { | |
| reg = <0x30000000 DT_SIZE_K(16)>; | |
| compatible = "zephyr,memory-region", "mmio-sram"; | |
| zephyr,memory-region = "SRAM1"; | |
| zephyr,memory-attr = <DT_MEM_ARM(ATTR_MPU_RAM)>; | |
| }; |
The memory region stm32mp series uses for Ethernet is custom defined region and not predefined region as sram1 or sram2. And in that custom defined region the attribute #memory-region-cells is defined:
zephyr/boards/st/stm32mp135f_dk/stm32mp135f_dk.dts
Lines 78 to 83 in 923ece1
| eth_ram: sram@2fffc000 { | |
| compatible = "zephyr,memory-region", "mmio-sram"; | |
| reg = <0x2fffc000 DT_SIZE_K(16)>; | |
| #memory-region-cells = <0>; | |
| zephyr,memory-region = "ETH_SRAM"; | |
| }; |
What do you suggest here?
Note that I tried to get Ethernet working with custom region but wasn't successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add #memory-region-cells = <0>; to all possible memory regions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding it fixes the mentioned error and doesn't break anything else with this build (http_server sample). I added it to the sram0, sram1 and sram2 nodes.
However, I can't guarantee that this addition doesn't break something else unrelated to Ethernet. I can add it to the commit and see if it triggers any twister-build errors. Would that work?
| 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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could do
#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node) && !DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram))
here and in the sections.ld file to get rid of the build assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing assert and replacing it with a similar check works. For example, I already had this:
#if DT_NODE_HAS_COMPAT(sram_eth_node, zephyr_sram)
#error "Invalid Ethernet memory-region: cannot use zephyr,sram. \
Use a SRAM region like sram1 or sram2 for Ethernet."
but @etienne-lms suggested BUILD_ASSERT instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then leave it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. But what do I do about the failing twister-builds caused by the BUILD_ASSERT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply:
diff --git a/soc/st/stm32/stm32h7rsx/mpu_regions.c b/soc/st/stm32/stm32h7rsx/mpu_regions.c
index 5384d8bb2a6..f28fdf8b5b4 100644
--- a/soc/st/stm32/stm32h7rsx/mpu_regions.c
+++ b/soc/st/stm32/stm32h7rsx/mpu_regions.c
@@ -34,10 +34,6 @@ static const struct arm_mpu_region mpu_regions[] = {
DT_NODE_HAS_PROP(DT_NODELABEL(mac), memory_regions)
#define sram_eth_node DT_PHANDLE(DT_NODELABEL(mac), memory_regions)
-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.")
-
#if DT_NODE_HAS_STATUS_OKAY(sram_eth_node)
/* Region 5 - Ethernet DMA buffer RAM */
MPU_REGION_ENTRY("SRAM_ETH_BUF", DT_REG_ADDR(sram_eth_node),
@@ -48,6 +44,12 @@ BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)),
#endif
};
+#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
+
const struct arm_mpu_config mpu_config = {
.num_regions = ARRAY_SIZE(mpu_regions),
.mpu_regions = mpu_regions,
you can't do a BUILD_ASSERT inside a
static const struct arm_mpu_region mpu_regions[] = {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Add Ethernet configuration for mac and mdio nodes. Define the SRAM region for Ethernet descriptor and buffer using memory-regions = <...> in the mac node for this board and for nucleo_h7s3l8, since they both use the same mpu_regions.c and sections.ld files. Add memory-regions property to st,stm32h7-ethernet.yaml file. Update mpu_regions.c and sections.ld to use the region defined in board's .dtsi file. Add an assert to mpu_regions.c to check if the memory-region node refers to the zephyr,sram region. Disable mac node in .overlay files for tests/drivers/memc/ram since ethernet drivers that get enabled by it consume sram1 node and thus fail the test. Update the yaml files to include Ethernet tag and update documentation to explain Ethernet usage. Signed-off-by: Benjamin Klaric <benjamin.klaric01@gmail.com>
1f3a8e3 to
cd00b2c
Compare
|
You don't need to add a copyright line at source/header files entry when you modify them to have your copyright implicitly taken into account. What drives the copyright owners list is the Git reversion history, which tracks all contributions, not the file entry inline comment. |
That is what I thought, just wanted to be sure. Thanks for the explanation. Noted for future PRs. |
| }; | ||
|
|
||
| #ifdef sram_eth_node | ||
| BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation
| BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)), | |
| BUILD_ASSERT(!DT_SAME_NODE(sram_eth_node, DT_CHOSEN(zephyr_sram)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| reg = <0x24000000 DT_SIZE_K(456)>; | ||
| compatible = "zephyr,memory-region", "mmio-sram"; | ||
| zephyr,memory-region = "SRAM0"; | ||
| #memory-region-cells = <0>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed. We don't expect one to reference &sram0 as a memory-region phandle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| compatible = "zephyr,memory-region", "mmio-sram"; | ||
| zephyr,memory-region = "SRAM1"; | ||
| #memory-region-cells = <0>; | ||
| zephyr,memory-attr = <DT_MEM_ARM(ATTR_MPU_RAM)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the change made in #97731, won't sram1 MPU attributes here now conflicts with the one defined by mpu_regions.c?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the change made in #97731, won't
sram1MPU attributes here now conflicts with the one defined by mpu_regions.c?
Yes it will create 2 overlaying mpu regions for sram1 and the one from zephyr,memory-attr has the higher priority and should cause problems with caching on the dma region, but for whatever reason it seems to work.
| pinctrl-names = "default"; | ||
| phy-connection-type = "rmii"; | ||
| phy-handle = <ð_phy>; | ||
| memory-regions = <&sram1>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use sram2 to preserve to previous configuration?
Otherwise, the commit message should tell that h7rs based device ethnet buffers are now in sram1 (AXI SRAM1) by default instead of sram2 (AXI SRAM2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sram1 and sram2 are AHB, sram0 is AXI sram (called sram 1-4 in the datasheet/rm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sram2 is disabled by default. That is why I opted to use sram1.
I can switch to using sram2 but that requires the node to be enabled by default. Otherwise an introduction of a fallback is needed (to sram1).
What should I do here?
I will update the commit message in any case. Thanks @TD-JBL for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the sram2 is disabled to have users enable it on purpose:
- Use as normal ram: enabled it in your board and set
zephyr,memory-attr = <DT_MEM_ARM(ATTR_MPU_RAM)> - Use it as dma buffer for ethernet, enable it in your board and let
mpu_regions.cconfigure the mpu-region for it.
I don't mind changing this behaviour, maybe have it already enabled... not sure whats be best way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that preserving behavior is that important for a dev board
erwango
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment for equivalent change on H7.
Please apply the same for H7RS (configure memory in .dtsi files)



Changes
macandmdioconfiguration tostm32h7s78_dk-common.dtsiand enabled them. The RMII Ethernet pin configuration follows this schematic from ST: MB1736.Added fallback option for definition ofDefined the SRAM region for Ethernet descriptor and buffersram_eth_nodeinmpu_regions.candsections.ld. This definition earlier defaulted tosram2, but now hassram1as fallback option.using
memory-regions = <...>in themacnode (as discussed) and added the updated definition ofsram_eth_nodetompu_regions.candsections.ld. Also added the needed property indts/bindings/ethernet/st,stm32h7-ethernet.yaml.memory-regions = <...>toboards/st/nucleo_h7s3l8/nucleo_h7s3l8.dts, since they use the samempu_regions.candsections.ld.stm32h7s78_dk.yamlandstm32h7s78_dk_stm32h7s7xx_ext_flash_app.yamlto include the Ethernet tag.index.rstto include the Ethernet feature and documented the steps needed in order to use Ethernet for this board.tests/drivers/memc/ram/boards/stm32h7s78_dk_stm32h7s7xx_ext_flash_app.overlayand updatedtests/drivers/memc/ram/boards/stm32h7s78_dk.overlaysince Ethernet drivers fully consumesram1node and thus fail the test without the additions.#memory-region-cells = <0>;to all possible memory regions (sram1andsram2nodes) indts/arm/st/h7rs/stm32h7rs.dtsiTests
I tested these changes with the following examples:
I could ping the board after flashing both samples and also open the website in the browser. Everything worked as expected.
Notes
I opened #99164 in order to discuss how to implement the fallback for
sram2and whether this approach is correct. Taking a look at the discussion should answer some questions about the implementation.Closes #99164