eth: stm32h7: use SRAM3 / MPU configuration

[mjaun]

Fixes: #29915

Changes are based on these recommendations.

Notes:

• Putting the descriptors in a separate MPU region and configuring them as device memory is what actually fixed the issue. I am not sure why ST recommends using SRAM3, perhaps there are performance reasons but maybe also other potential issues.
• The Cortex M linker script should not contain STM32 specific definitions. There should be a more generic way to define the required sections.
• Same applies for the static MPU configuration. Ideally both could be done in the device tree as proposed by @Nukersson in the issue.
• There is already a PR with another approach: ethernet: stm32h7: fix tx semaphore timeout bug #29944

[KozhinovAlexander]

Thank you very much for this very useful PR.

Now my ideas on it:

1. @emillindq Already provided solution and test with ST's driver fix. My suggestion here would be push it anyway, cause it does only __DSB() after writing eth descriptors and therefore should not have impact on ethernet throughput or driver performance.

2.@reloZid Very elegant fix from you here! But we can't just change linker scripts every time we need specific memory regions configuration. I think here we need memory reallocation and MPU configuration possibilities in DT

@gmarull @erwango @galak Now we seems to have proper solution for solide bug fix with future improvents for the whole Zephyr project. What do you think? Especially, what do you think about MPU configuration and data regions definition in DT?

[gmarull]

Thanks for this draft, will be useful to trigger discussions on how to find a proper fix for this problem and also future similar requirements.

I see some other projects have a sort of MPU API, e.g. https://github.com/u-boot/u-boot/blob/master/arch/arm/mach-stm32/soc.c

In Zephyr I think something similar can be done if using CONFIG_CPU_HAS_CUSTOM_FIXED_SOC_MPU_REGIONS (see NXP k6x/k8x for example) and provide a soc level MPU configuration. In this case, as ST recommends SRAM3 for I/O Ethernet/USB:

SRAM3 could be configured entirely for that purpose at soc level, and just provide DT definitions for sram3 together with linker scripts facilities so that Ethernet buffers can easily be placed there. I'm not sure if that's the best approach, though.

I'm not really familiar with Zephyr MPU implementation, it would be good to have input from maintainers @ioannisg @galak @MaureenHelm @stephanosio @carlocaione

[KozhinovAlexander]

We need a possibility to configure MPU and SRAM sections over DT or KConfig for Zephyr. And then we need a possibility to use configured SRAM sections in our code.

[galak]

dev-review (Dec 3, 2020):

Suggestion to utilize CONFIG_NOCACHE_MEMORY and look at adding one more section for the SRAM specific case in arch/arm/core/aarch32/cortex_m/mpu/arm_core_mpu.c.

@ioannisg would like to see over time cleaning up of the linker script and mpu tables to move into SoC specific code.

[KozhinovAlexander]

dev-review (Dec 3, 2020):

Suggestion to utilize CONFIG_NOCACHE_MEMORY and look at adding one more section for the SRAM specific case in arch/arm/core/aarch32/cortex_m/mpu/arm_core_mpu.c.

I would prefer not to add half-solutions. We can better define amount of work and split it here, providing proper and solid solution.

My idea is to add mpu_attributes section to each sram section. Something like this for h74x:

sram3: memory@30000000 {
		reg = <0x30000000 DT_SIZE_K(128)>;
		compatible = "mmio-sram";
                mpu_config {
                    attr = NOCACHE && SHAREABLE;
                };
	};

This needs changes in DT parser or at least an additional preprocessing step before original DT. In this preprocessing step west/zephyr specifig artifacts (like mpu_config) should be splitted from DT syntax and classing DT file should be generated. Additionally the MPU config should be saved and added after DT step to linker. Everything can be done inside of west or Zephyr specific west processing.

Any ideas to my proposal?

@ioannisg would like to see over time cleaning up of the linker script and mpu tables to move into SoC specific code.

[KozhinovAlexander]

The idea behind the changes is very good. But we need more scalable solution.

[erwango]

The idea behind the changes is very good. But we need more scalable solution.

@Nukersson, this point was discussed during dev-review meeting. Of course this proposal lacks scalability. Though w/o alternative proposal we find it acceptable for now with few tweaks.
To define a more scalable alternative we'd need more variety of usage examples. A rework of the whole way to instantiate memory partitions from DT is something we'd like to get, but it has a lot of intrications so it was proposed to postpone post LTS (not before 6 monthes).

[KozhinovAlexander]

The idea behind the changes is very good. But we need more scalable solution.

@Nukersson, this point was discussed during dev-review meeting. Of course this proposal lacks scalability. Though w/o alternative proposal we find it acceptable for now with few tweaks.
To define a more scalable alternative we'd need more variety of usage examples. A rework of the whole way to instantiate memory partitions from DT is something we'd like to get, but it has a lot of intrications so it was proposed to postpone post LTS (not before 6 monthes).

Okay. I understand it. But then we need to plan proper solution within the roadmap for next year or two.

[ioannisg]

Looks acceptable to me, but I would insist on trying using DT for RAM region definition and use the DT macros in the linker script and other locations. And use the SOC_SPECIFIC MPU region definition option and define the MPU region at the SoC level (not in the arm mpu driver)

Future:
I would really like to clean this up; in particular, the soc could add hooks to the mpu driver at run time, to define static MPU partitions that are SoC-specific, not feature-specific.

[ioannisg]

This could also be an #ifdef directive based on DT.

[mjaun]

Done.

[ioannisg]

This won't be needed, imho, if we use device tree for getting the RAM3 address and size.

[mjaun]

Done.

[ioannisg]

This is fine. I wonder if you can add the declaration for the ARMv8-M architecture as well.
Optional.

[mjaun]

No longer needed, therefore removed.

[ioannisg]

nit: see if you could have it in a single line

[mjaun]

No longer needed, therefore removed.

[ioannisg]

This section is

• SoC specific, not feature specific, and,
• outside the image boundaries (image_ram_start, image_ram_end)

So I think we should try to define this MPU region at boot. See soc/arm/common/arm_mpu_regions.c. As discussed in the dev-review meeting there is a Kconfig option that allows to override the default SoC MPU region definition, and add a SoC-specific one that includes this region as well.

[mjaun]

Thanks for this hint! Added SoC specific MPU region definitions.

[mjaun]

Should there still be a Kconfig option to enable this for the driver?

[ioannisg]

@erwango ^^

[mjaun]

Aligned this with the H745 device tree which should have the same memory architecture. Anyone can confirm this is correct?

[erwango]

Please have a check to #30530.
I'm reworking that part a bit.

[KozhinovAlexander]

Aligned this with the H745 device tree which should have the same memory architecture. Anyone can confirm this is correct?

Yes. They have same memory architecture. They also have same ReferenceManual

[mjaun]

Copied this from soc/arm/common/cortex_m/arm_mpu_mem_cfg.h. Can we share this somehow?

[KozhinovAlexander]

@erwango ?

[erwango]

Isn't it possible to include soc/arm/common/cortex_m/arm_mpu_mem_cfg.h directly ?

[mjaun]

Included it directly. Wasn't sure because it seems only possible using a relative path:

#include "../../common/cortex_m/arm_mpu_mem_cfg.h"

[erwango]

@reloZid it might be interested for you to rebase on top of latest master, following the merge of #30530.
Also, please remove the initial commits, so we only have to review the last version of the proposed solution. This should help in the review process

[erwango]

@reloZid would you mind rebasing ?

[mjaun]

@erwango Sorry for the late reaction. I rebased the changes.

[erwango]

@ioannisg can you have a new look ?