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

Add OpenSBI compilation to D1 #7077

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Add OpenSBI compilation to D1 #7077

merged 1 commit into from
Aug 21, 2024

Conversation

Randl
Copy link
Contributor

@Randl Randl commented Aug 13, 2024

Description

Building for mangopi mq crashes with the following error:

   Image 'itb' is missing external blobs and is non-functional: opensbi
   
   /binman/itb/fit/images/opensbi/opensbi (fw_dynamic.bin):
      See the documentation for your board. The OpenSBI git repo is at
      https://github.com/riscv/opensbi.git
      You may need to build fw_dynamic.bin first and re-build u-boot with
      OPENSBI=/path/to/fw_dynamic.bin
   
   Some images are invalid
   make: *** [Makefile:1124: .binman_stamp] Error 103
--> (179) ERROR: Error 2 occurred in main shell [ at /armbian/lib/functions/logging/runners.sh:211
       run_host_command_logged_raw() --> lib/functions/logging/runners.sh:211
      run_host_command_logged_long_running() --> lib/functions/logging/runners.sh:188
         do_with_ccache_statistics() --> lib/functions/compilation/ccache.sh:39
              compile_uboot_target() --> lib/functions/compilation/uboot.sh:230
      loop_over_uboot_targets_and_do() --> lib/functions/compilation/uboot.sh:287
                     compile_uboot() --> lib/functions/compilation/uboot.sh:391
                   do_with_logging() --> lib/functions/logging/section-logging.sh:81
      artifact_uboot_build_from_sources() --> lib/functions/artifacts/artifact-uboot.sh:175
       artifact_build_from_sources() --> lib/functions/artifacts/artifacts-obtain.sh:34
          obtain_complete_artifact() --> lib/functions/artifacts/artifacts-obtain.sh:280
          build_artifact_for_image() --> lib/functions/artifacts/artifacts-obtain.sh:392
       main_default_build_packages() --> lib/functions/main/build-packages.sh:108
      full_build_packages_rootfs_and_image() --> lib/functions/main/default-build.sh:31
             do_with_default_build() --> lib/functions/main/default-build.sh:42
            cli_standard_build_run() --> lib/functions/cli/cli-build.sh:25
           armbian_cli_run_command() --> lib/functions/cli/utils-cli.sh:136
                    cli_entrypoint() --> lib/functions/cli/entrypoint.sh:176
                              main() --> compile.sh:50

See, e.g., https://forum.armbian.com/topic/21465-armbian-image-and-build-support-for-risc-v/?do=findComment&comment=190008

The fix is to build OpenSBI and use it to build uboot as suggested in error message, as discussed in https://forum.rvspace.org/t/building-u-boot-from-mainline-repo/3398 for example. This PR adds OpenSBI building stage.

How Has This Been Tested?

  • Built for BOARD=mangopi-m28k

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings

@Randl Randl requested review from a team and igorpecovnik as code owners August 13, 2024 08:34
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines Hardware Hardware related like kernel, U-Boot, ... Framework Framework components labels Aug 13, 2024
@igorpecovnik igorpecovnik added 08 Milestone: Third quarter release Needs review Seeking for review Giveaway 11 Milestone: Fourth quarter release and removed 08 Milestone: Third quarter release labels Aug 13, 2024
@The-going
Copy link
Contributor

@Randl Евгений, Good health.
The information that is written in the Description on the github in the pull request will remain in the history of the github.
A user who looks at the commit history in the build system will see only two identical messages:
Add opensbi compilation

You change the general code, but there is no explanation in the message
to the commit why this is the case.
At the same time, opensbi is being assembled for the spacemit (riscv) family.
config/sources/families/spacemit.conf#L16-L20
Please look carefully. Maybe it should be done by analogy?
Or please provide details in the message to the commits.

@ColorfulRhino
Copy link
Collaborator

ColorfulRhino commented Aug 13, 2024

I fully agree with the previous comment, thanks @The-going !
Please use proper commit message titles which one can see at a glance at least roughly what changed, and provide a description and/or reasoning in the commit message body if the changes are not small or very obvious. Try to use commit message tags where possible.

I picked a few commits as positive examples on how the messsage should optimally look like:

Note to project members: The quality of the commit messages is currently still all over the place. This is less of a problem when there a only a few contributors and the project is small, but will become a bigger problem when the project grows. We should really establish some consistency and some guidance on how to write good commit messages. This is also important for generating changelogs. One should be able to easily see all changes just by scrolling through the recent commits. The message title should say enough to make it at least roughly clear what this commit added or changed.

@github-actions github-actions bot added size/small PR with less then 50 lines and removed size/medium PR with more then 50 and less then 250 lines labels Aug 14, 2024
@Randl
Copy link
Contributor Author

Randl commented Aug 14, 2024

Sure!
I've missed the ATF compilation function. I've forced-pushed the branch with a better comment message and used the existing infrastructure.

@Randl Randl changed the title Add opensbi compilation Add AFT compilation to D1 Aug 14, 2024
@The-going
Copy link
Contributor

This PR adds OpenSBI building stage.

But the message in the commit contains:

 Add ATF settings to D1 family config

Without OpenSPI, the uboot compilation fails.

Perhaps you wanted to write something like this?
Add an opensbi build for d1 SOC

I still have one question.
In what environment was the build done? Or more precisely, which version of the compiler was used?

@Randl
Copy link
Contributor Author

Randl commented Aug 14, 2024

I've used riscv64-linux-gnu-gcc 11.4.0, building locally on my Ubuntu PC in docker.

As for the discrepancy in the commit message, I think the variable naming may be confusing, which also contributes to the confusing commit message. The compilation function is named compile_atf, and all variables have the ATF prefix in them. However, ATF is arm-specific, and its counterpart on RISC-V is OpenSBI. Thus, the PR indeed adds ATF_* variables to D1 config, and uses them to compile opensbi.

So maybe something like firmware_interface would be less confusing than atf for this set of functions/variables?
This falls outside of this PR scope, though.

@Randl Randl changed the title Add AFT compilation to D1 Add OpenSBI compilation to D1 Aug 14, 2024
@The-going
Copy link
Contributor

I've used riscv64-linux-gnu-gcc 11.4.0, building locally on my Ubuntu PC in docker.

Why did I ask?
For the spacemit family, for some reason, opensbi is built without errors by the compiler version 13.

Without OpenSPI, the uboot compilation fails.

OpenSBI ?

@Randl
Copy link
Contributor Author

Randl commented Aug 14, 2024

For the spacemit family, for some reason, opensbi is built without errors by the compiler version 13.

There is no error building opensbi, the problem was it wasn't built for d1 family, and thus uboot build failed.

@Randl
Copy link
Contributor Author

Randl commented Aug 20, 2024

Is something missing to merge this?

@The-going
Copy link
Contributor

Is something missing to merge this?

There is a typo in the message to commit.
Вы написали OpenSPI.

У меня нет претензий к вашему коду. И в предыдущей версии не было. Пожалуйста поймите правильно.
Вместо этой строчки: Without OpenSPI, the uboot compilation fails.
просто напишите пояснения почему именно так. Что то вроде:
Мы используем существующую функцию compile_atf для сборки OpenSBI
потому что она делает тоже самое ...... и это позволяет собрать u-boot без ошибок.

Пояснения к сообщению в фиксации почему сделано именно так,
а не как то по другому помогают быстро понять суть изменений и точку зрения автора.

Мне неудобно об этом писать повторно.
С уважением, Леонид.

Copy link
Member

@igorpecovnik igorpecovnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without OpenSPI, the uboot compilation fails.
->
Without OpenSBI, the uboot compilation fails.

Without OpenSBI, the uboot compilation fails.
@igorpecovnik igorpecovnik removed the Needs review Seeking for review label Aug 21, 2024
@igorpecovnik igorpecovnik added the Ready to merge Reviewed, tested and ready for merge label Aug 21, 2024
@igorpecovnik igorpecovnik merged commit e3990e8 into armbian:main Aug 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
11 Milestone: Fourth quarter release Framework Framework components Hardware Hardware related like kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/small PR with less then 50 lines
Development

Successfully merging this pull request may close these issues.

4 participants