soc: nxp: imrt: clean section definitions in boot header #85634
Conversation
zephyrbot
requested review from
butok,
decsny,
DerekSnell,
dleach02,
EmilioCBen,
mmahadevan108 and
nashif
|
Verified on RT1180 and RT1170 EVK. |
thanks for the feedback. I tried to enable this feature on RT1170 evk by adding these lines in Helloworld prj.conf but building failed, CONFIG_XIP=n ########################################################################### error: Aborting due to Kconfig warnings CMake Error at C:/workspace/zephyr_github_nxp_upstream/zephyr/cmake/modules/kconfig.cmake:396 (message): -- Configuring incomplete, errors occurred! |
AFAIK, ramloader feature has been removed from main, please double check |
It's not removed as far as I know, I dont know why would it be, did you change the zephyr,flash |
yes, I changed Zephyr,flash, the 1st warning disappeared but the error still there. NXP_IMX_RT_ROM_RAMLOADER can't be found. I searched this macro in the whole project, it is not there. |
I'm not sure where you got that name, I think it's called NXP_FLEXSPI_ROM_RAMLOADER |
thanks, I think I got the wrong macro name from a commit history. |
Clean up redundancy boot header sections configuration in cmake file as they have been defined in boot_header.ld very well and already included. Add new section name macro definitions, use these macro instead of hard code the section name. Signed-off-by: Raymond Lei <raymond.lei@nxp.com>
Raymond0225
force-pushed
the
clean_boot_header_section_name
branch
from
3416b93 to
48825c2
Compare
There was a problem hiding this comment.
This will break NXP on IAR and ARMCLANG and using CONFIG_CMAKE_LINKER_GENERATOR=y
Please don't remove things just because you don't think they are being used.
We really need to communicate our strategy for linker scripts moving forward @tejlmand
There was a problem hiding this comment.
This is being used in CMAKE_LINKER_GENERATOR
I do not agree with this statement. Mistakes might be made but reducing maintenance surface is not a bad thing to try to do. If it had caused a regression it could just be reverted. We shouldn't discourage contributors from trying to do things like this because they are clearly just trying to improve the quality and maintainability of the project. When I reviewed this PR I tried also to figure out why the linking description was duplicated in 2 places because something seemed weird about that and I was suspicious at first. But inspecting the git history made it look like it was on accident, and I really couldn't figure out from looking around the tree what it was for. So you've answered now, which is good. |
|
@RobinKastberg Now my question is do we actually need the .ld file too or is the cmake sufficient in all cases? I also don't want two different ways of doing the same thing if we can avoid it, but I'm not an expert on the build system here, you seem to have strong opinions about this. Having two places where we need to say to do the same thing is a recipe for someone missing one of them when doing a change and having a bug |
100% this is really bad, and we are aware. For now. It would be interesting if NXP would start testing with |
Sorry for being unfriendly. We just got the cmake and ld scripts enough in sync to be able to merge basic IAR support. The fact that we have two parallel linking systems without a single source of truth, I really don't want to discourage anyone from cleaning up and fixing things, we really need those things. But I would encourage figuring out why it was like that in the first place. Reverting regressions is easy, but finding out about them can be really hard in these lesser tested toolchains/configurations/boards. |
@hakehuang isn't this something we could do against our HW range? Do a twister run with |
and how to obtain the BEARER TOKEN? |
|
@hakehuang NXP via @dleach02 should have received alicense token. |
@RobinKastberg , mimxrt1060_evk does not work for me, and the whole boards does not enabled for iar, see below steps
|
|
@hakehuang I have not understood why toolchain needs to be enabled per test. Since we are aiming for gnu compatiblity I have gone the other way around and blacklisted failing tests instead. Maybe testing WG can help us with proper twister integration. So far we have just used We also have not cleaned up warnings completely yet, see #86002. So we need |
it is not enabled per test, it is a logic that every platform need specify its supported tool-chain. I tries with your command which works basically, and let me do more test with. Thanks a lot for your supporting, @RobinKastberg mimxrt11xx series have below issue: also there are issues with S32 paltform in cmsis-build |
As these duplicated linking scripts are used for different tool-chains, is there a way to make it more clear: |
|
@Raymond0225 @RobinKastberg What should we do with this PR? |
| #define _IMX_BOOT_DCD_SECTION_NAME .boot_hdr.dcd_data | ||
| #define _IMX_BOOT_CONTAINER_SECTION_NAME .boot_hdr.container | ||
| #define _IMX_BOOT_FLASH_CONF_SECTION_NAME .flash_conf | ||
| #define _IMX_BOOT_XMCD_SECTION_NAME .boot_hdr.xmcd_data |
There was a problem hiding this comment.
nit: you changed the whitespace practice used in the rest of this file from tabs to spaces.
@RobinKastberg is there any update from IAR side on this? |
Yes. I am working on a cmake linker generator refactoring right now. As a part of this I will investigate how hard this kind of snippet generation would be. If anyone wants to help I will gladly accept it. Also I should create a separate issue for this. |
|
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. |
Clean up redundancy boot header sections configuration in cmake file as they have been defined in boot_header.ld very well and already included.
Add new section name macro definitions, use these macro instead of hard code the section name.