Skip to content

arch: common: Make nocache region optionally loadable #87826

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

Conversation

jachatzi
Copy link
Contributor

@jachatzi jachatzi commented Mar 28, 2025

The nocache region is not loadable, thus data stored therein cannot be initialized by the startup code. This might be needed in special cases. E.g. One might have a buffer which one wants to DMA into, and which is a member of a struct. Other members of the struct one may want to have initialized by the startup code.
The buffer thus should be placed in the nocache region, but for the other members of the struct to be initialized by the startup code, the nocache region needs to be loadable.

Fix it by adding a KConfig symbol NOCACHE_REGION_IS_LOADABLE, which if set to y makes the nocache region loadable. It defaults to n so that existing code does not get affected by it.

Fixes #52637, #85236

@jachatzi
Copy link
Contributor Author

To add a bit more background: I created #52637 when I was working on a project where we modified the modbus stack to use DMA. The solution then existed as a patch in said project and never was cleaned up for a PR. Wrote a clean one now.

The question, however, is if anyone needs it. I.e. no one commented on #52637 so we could also close the issue and this PR. But if not than here is a solution ;)

One further note: soc/aspeed/ast10x0/nocache.ld might also have to be adjusted accordingly.

@jachatzi jachatzi force-pushed the fix_52637_make_nocache_optionally_loadable branch from 288ab2f to 7c5e337 Compare March 28, 2025 14:51
@cfriedt
Copy link
Member

cfriedt commented Mar 29, 2025

@jachatzi - thanks for following up with this - a 2yo feature request.

Personally, I don't see why nocache regions should be excluded from loading. I wonder if we can just enable that behaviour by default without being opt-in.

@jachatzi
Copy link
Contributor Author

@cfriedt Normally I'd say so too. The only thing which made me disable it by default is that it would slightly increase the boot time of existing products, when they update to a newer zephyr version. Maybe a bit over cautious...

@peter-mitsis
Copy link
Collaborator

@jachatzi - Would this also fix #85236 ?

@jachatzi
Copy link
Contributor Author

jachatzi commented Apr 8, 2025

@peter-mitsis Yes indeed, it would fix #85236

@jachatzi
Copy link
Contributor Author

jachatzi commented Apr 8, 2025

@cfriedt So how do we go one? Should I remove the option and make it loadable by default?
Maybe @aurel32 can tell us why it was set to NOLOAD?

@cfriedt
Copy link
Member

cfriedt commented Apr 12, 2025

@jachatzi - it fixes a bug. I say make it the default.

Technically speaking, if there are no initialized objects in a no-cache region, they won't get loaded.

I wonder if maybe the no-load was a bit of a hack to preserve the contents of memory across soft-resets, but IIRC, there are other ways of doing that.

@aurel32
Copy link
Collaborator

aurel32 commented Apr 12, 2025

@cfriedt So how do we go one? Should I remove the option and make it loadable by default? Maybe @aurel32 can tell us why it was set to NOLOAD?

The NOCACHE region has been initially designed to hold data for DMA operations where cache operations can't not be used (e.g. due to alignment issues). Support for loading it on boot was not implemented as it was not needed for that usage.

@cfriedt
Copy link
Member

cfriedt commented Apr 13, 2025

@jachatzi - IIUC you can assume that the Kconfig option you have added is always true, so I think it makes sense to remove it and hard-code the resulting bits.

This would only affect initialized objects in nocache regions, which would typically be what one wants anyway with a C runtime.

There is a separate option / attribute to skip zero-initializing bss, but I don't think that's in play here because we are specifically looking at data regions / initialized objects.

@jachatzi jachatzi force-pushed the fix_52637_make_nocache_optionally_loadable branch from 7c5e337 to 438739b Compare April 15, 2025 18:08
@github-actions github-actions bot requested a review from andyross April 15, 2025 18:09
@jachatzi
Copy link
Contributor Author

@cfriedt - Removed the KConfig symbol and made it always loadable.

@jachatzi jachatzi force-pushed the fix_52637_make_nocache_optionally_loadable branch from 438739b to 650ea4f Compare April 15, 2025 18:13
@jachatzi
Copy link
Contributor Author

@cfriedt So how do we go one? Should I remove the option and make it loadable by default? Maybe @aurel32 can tell us why it was set to NOLOAD?

The NOCACHE region has been initially designed to hold data for DMA operations where cache operations can't not be used (e.g. due to alignment issues). Support for loading it on boot was not implemented as it was not needed for that usage.

@aurel32 - Thanks for clarifying. Wanted to make sure that there isn't some weird edge case which speaks against making it loadable. As this is not the case we could go on with this PR.

@cfriedt
Copy link
Member

cfriedt commented Apr 15, 2025

@jachatzi - this might need a rebase on the latest from main

The `nocache` is not loadable, thus data stored therein cannot be
initialized by the startup code. This might be needed in special
cases. E.g. One might have a buffer which one wants to DMA into,
and which is a member of a struct. Other members of the struct one
may want to have initialized by the startup code.
The buffer thus should be placed in the `nocache` region, but for
the other members of the buffer to be initialized by the startup
code, the `nocache` region needs to be loadable.

Fix it by making the `nocache` region loadable. Adding a KConfig
symbol to do this optionally was considered, but deemed unnecessary
during the PR.

Signed-off-by: Julian Achatzi <[email protected]>
@jachatzi jachatzi force-pushed the fix_52637_make_nocache_optionally_loadable branch from 650ea4f to 2aa9ca9 Compare April 16, 2025 09:05
@jachatzi
Copy link
Contributor Author

@jachatzi - this might need a rebase on the latest from main

@cfriedt - done

@kartben kartben merged commit f9168ae into zephyrproject-rtos:main Apr 21, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a loadable NOCACHE linker section
7 participants