Delay relocation of data/bss init after MEMC init is completed

[talih0]

Currently, when relocating to external SRAM regions via MEMC drivers, the relocation/zero initialization is done before the MEMC drivers are initialized.

See issue #68884

This patch delays initialization after PRE_KERNEL_1 level.
The initialization is only delayed on devicetree memory regions with delay-relocation property set.

Note, MEMC drivers (i.e. Atmel SAM SMC) use POST_KERNEL level, so their initialization levels would have to be updated.

[talih0]

@pdgendt, @nandojve, would you mind having a look please, as I believe it also applies to Atmel SAM SMC drivers.

[erwango]

I guess this would fix #43555

[cfriedt]

I guess this would fix #43555

I had a similar issue #39598 a couple of years ago. That would also be fixed by this (with the addition of a memc driver)

[erwango]

@talih0 Can you fix compliance issue reported by automated checks ?

[pdgendt]

CODE_DATA_RELOCATION already depends on ARCH_HAS_CODE_DATA_RELOCATION, no need to add it again.

Suggested change

	depends on ARCH_HAS_CODE_DATA_RELOCATION
	depends on CODE_DATA_RELOCATION
	depends on CODE_DATA_RELOCATION

[danieldegrasse]

I wonder if we should have some form of device_is_ready check done before the relocation calls are made- since the code relocation script now has access to the devicetree, perhaps there is some way we can verify the MEMC device defining the memory region was initialized correctly?

The reason I ask is that if we simply blindly copy data into memory regions that may be set up by MEMC devices, we run the risk of crashing the chip. I have seen this with iMX.RT parts (which use external SDRAM setup by the bootloader). In this case, the chip will simply crash at initialization since the SRAM region is not accessible.

I'm not sure how we would best handle this (perhaps simply triggering a fault). I just would like to avoid the case where an external memory device fails to initialize, and the application simply crashes rather than printing some form of error message

[swift-tk]

Is cache handled at this point? I see most of cache enables are in the level of PRE_KERNEL_1. The SoCs may have a default cacheable memory map for those regions, so it is best to perform a whole cache clean operation at the end of copy.

[swift-tk]

There is another issue I just thought about. Since stimer init call is in INIT_LEVEL_PRE_KERNEL_2, if MEMC drivers have to be initialized in INIT_LEVEL_PRE_KERNEL_1, this means that the device init can not use any timer related kernel function like k_sleep, k_busy_wait.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[jhol]

There is another issue I just thought about. Since stimer init call is in INIT_LEVEL_PRE_KERNEL_2, if MEMC drivers have to be initialized in INIT_LEVEL_PRE_KERNEL_1, this means that the device init can not use any timer related kernel function like k_sleep, k_busy_wait.

I transition my memc driver to use k_busy_wait everywhere. This was necessary because otherwise it is not possible to use the SDRAM in a CONFIG_MULTITHREADING=n application.

In my opinion, this should be the policy across all the memc drivers. I can't think of anything useful the system could do with the time, because there's not going to be any other thread the memc initialization could yield to.

In which case your PR would be a welcome improvement to Zephyr IMO.

[jhol]

If you really want to handle the file not existing, it would be better instead to handle FileNotFoundError exceptions from open() so that there is no change of a race condition between the check and the use of the file.

But I think it's wrong to set edt = None if the file is not found. It should be set to None only if the args.edt_pickle is not None.

If the --edt_pickle option is given but the file fails to open, then it would probably best to just let the exception go out unhandled.

[erwango]

In which case your PR would be a welcome improvement to Zephyr IMO.

@talih0 Any plans to resurect this ? Otherwise @jhol would you be ready to taking it over ?

[jhol]

I did some work on amending the patch yesterday, and it does indeed solve the problem now. I have an SDRAM that is initialized POST_KERNEL, and the revocations are performed immediately after entering this run-level.

The main issue outstanding with the patch from my perspective is the way it reads from the EDT pickle. This design seems a little ugly to me. I would expect the information to have to somehow feed through Kconfig and CMake instead.

[jhol]

One issue I encountered doing relocations POST_KERNEL, is that certain static initialization features fail. For example, K_HEAP_DEFINE will fail because it will attempt to initialize a heap before the data has been loaded.

[talih0]

@jh

In which case your PR would be a welcome improvement to Zephyr IMO.

@talih0 Any plans to resurect this ? Otherwise @jhol would you be ready to taking it over ?

@jhol, feel free to take over the patch. Thanks!

[jhol]

Happy to take it over, though it will be a few weeks before I have time to finish it off

[jhol]

One issue I encountered doing relocations POST_KERNEL, is that certain static initialization features fail. For example, K_HEAP_DEFINE will fail because it will attempt to initialize a heap before the data has been loaded.

Same problem applies to K_MEM_SLAB_DEFINE_STATIC. init_mem_slab_obj_core_list is called at PRE_KERNEL_1 initialization. The function uses STRUCT_SECTION_FOREACH to initialize memory slabs that may be not yet be available, and so causes a crash in early boot.

[VynDragon]

Looking forward to this 👍

[jhol]

Looking forward to this 👍

I'll publish my latest attempt at the patch, but I'm not sure how we get around some of these issues. In the current version we can delay relocation until after memc device initialization. In practice though, the developer needs to ensure that none of the relocated memory is used before relocation occurs otherwise there will be early-boot crashes - this includes many device drivers, memory allocators and other system initialization code. In the current structure of Zephyr, this is extremely difficult and error-prone to ensure, and makes the patch rather useless.

So for my project, I abandoned the approach and instead divided my application into a Zephyr-powered bootloader which loads a main Zephyr application into RAM.

[jhol]

Note I've been doing some other work on the relocation code:

• Tidy-ups to gen_relocate_app.py #90229
• code_relocation: Handle relocation of noinit sections #90405

[VynDragon]

Having the ability to delay the relocation is enough and not useless at all, it enables using the memc-setup memory for main app code at least, and other things as well if the user setups their priorities correctly, it's a very important capability.

[jhol]

Here is my current version of the patch: b3df801

The main complaint about it is that parsing the EDT pickle in gen_relocate_app.py is a rather sketchy design. I'm not sure how this could be handled better. I'm wondering if it would be better to replace the delay-relocation device-tree property with a Kconfig-driven approach.

Having the ability to delay the relocation is enough and not useless at all, it enables using the memc-setup memory for main app code at least, and other things as well if the user setups their priorities correctly, it's a very important capability.

It's certainly a capability many would like to have.