Skip to content
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

cv_bsp_generator: correct CONFIG macro prefixes to CFG #14

Open
wants to merge 1 commit into
base: socfpga_v2023.04
Choose a base branch
from

Conversation

LRitzdorf
Copy link

This PR fixes several cases of mis-named macros in the Cyclone V BSP generator scripts. Specifically, many macros use the prefix CONFIG_ when they should use CFG_.

This issue was discovered while following RocketBoards' bootloader tutorial. Running cv_bsp_generator.py creates configuration header files with, at present, incorrectly-named macros. This is obvious when examining a diff of the changes, and also causes U-Boot compilation to fail.

I've basically just replaced relevant instances of the incorrect CONFIG_ prefix with the expected CFG_, and have tested the results on my own system. However, this is my first time contributing to anything remotely U-Boot-related, so I may have missed something — please let me know if this is the case! I'm quite familiar with Git-based development in general, and would be happy to make any relevant updates to this PR.

(Also, the current PR templates includes a note about using mailing-list-based development instead of PRs, but this seems to be an artifact of the original U-Boot project. If this is indeed the case, it may be worthwhile to amend the template.)

@truhy
Copy link

truhy commented Dec 8, 2023

I think there is a mistake in the changes for hps.py. The handoff file hps.xml actually contains these keywords:
CONFIG_HPS_QSPI_CS3
CONFIG_HPS_QSPI_CS2
CONFIG_HPS_QSPI_CS1
CONFIG_HPS_QSPI_CS0
CONFIG_HPS_UART0_TX
CONFIG_HPS_UART0_CTS
CONFIG_HPS_UART0_RTS
CONFIG_HPS_UART0_RX
CONFIG_HPS_UART1_TX
CONFIG_HPS_UART1_CTS
CONFIG_HPS_UART1_RTS
CONFIG_HPS_UART1_RX

Only the right hand-side needs changing, so for example:
p['CONFIG_HPS_QSPI_CS3'] = { 'name': 'CFG_HPS_QSPI_CS3', 'used': 0 }

@LRitzdorf
Copy link
Author

Thanks, @truhy — good catch. Verified and fixed.

@ernstp
Copy link

ernstp commented Feb 8, 2024

Thanks, this was useful. Hope this can be merged!

@0x000029A
Copy link

I've encountered problems I think they are related to this.
When I run the QTS filter on my project, the files produced by it in "u-boot-socfpga/board/altera/cyclone5-socdk/qts" are empty, because the files in "myproject/software/spl_bsp/generated" use CONFIG prefix, while qts-filter.sh search for CFG prefix.

And definitely, the problem propagates to the compiling process of u-boot using this command "make -j$(nproc)." The command generates errors about the CFG/CONFIG mismatch in files like wrap_pll_config.c, wrap_iocsr_config.c, and wrap_sdram_config.c.

@LRitzdorf
Copy link
Author

This has been partially patched in 195035a, but that commit makes the same mistake which I initially did (and which @truhy pointed out, thanks again!). This PR will be updated to address this momentarily.

@LRitzdorf
Copy link
Author

LRitzdorf commented May 28, 2024

@zamrimuh-intel Since you made the commit mentioned above, maybe you're the right person to review this?

As noted in #14 (comment), the handoff files generated by Platform Designer (QSYS) still use the CONFIG_ prefix, so a blind search-and-replace in the Python script is slightly too heavy-handed. As an example, in one of my own projects:

$ grep '_HPS_UART1_TX' my-quartus-project/hps_isw_handoff/soc_system_hps
my-quartus-project/hps_isw_handoff/soc_system_hps/hps.xml
95:      <config name='CONFIG_HPS_UART1_TX' value='0' />

Notice that the CONFIG_ prefix is still present, and thus the Python script in this repo still needs to look for it.

Since you've already addressed most of what this PR originally aimed to do, I've just pushed an update here which builds on your work to fix the above issue.

Edit: I notice that there's now a v2023.10 branch, and have verified that this PR applies cleanly there. I'm not sure how Intel's branch policies work, so please feel free to rebase and change the PR target if appropriate!

@zamrimuh-intel
Copy link
Contributor

I might overlooked this changes. Thanks to @truhy for mentioning the correct changes it should make in first comment here.

@LRitzdorf , are you able to run the bsp generation successfully after this changes? If yes then this can be verified and merged.

About the rebase on the other branch, yes I will make necessary implementation to it.

@LRitzdorf
Copy link
Author

Yep, BSP generation works as expected. This should also be confirmed by a few of the comments above.

For what it's worth, on my test project, I don't see any differences in the resulting header files. Presumably, this is because my project uses settings that are already the defaults?

@LRitzdorf
Copy link
Author

@zamrimuh-intel Quick reminder that this still exists, and is ready for merge whenever you feel like doing so!

@LRitzdorf
Copy link
Author

@zamrimuh-intel Hello again — as a reminder, the remaining part of this PR still needs to be merged, even on the current 2024.1 branch. Until this happens, the Cyclone V BSP generator remains partially broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants