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

ubootOlimexA64Teres1: init #259077

Merged
merged 1 commit into from Feb 4, 2024
Merged

ubootOlimexA64Teres1: init #259077

merged 1 commit into from Feb 4, 2024

Conversation

Kreyren
Copy link
Contributor

@Kreyren Kreyren commented Oct 4, 2023

Description of changes

Add U-Boot Package Declaration for OLIMEX Teres-I

Things done

  • Built on platform(s)
    • x86_64-linux (binfmt)
    • aarch64-linux
    • [-] x86_64-darwin
    • [-] aarch64-darwin
  • [-] For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@samueldr
Copy link
Member

samueldr commented Oct 6, 2023

Hi,

Can you describe the steps you used to test this?

Thanks,

Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

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

(To prevent well-meaning merges)

@Kreyren
Copy link
Contributor Author

Kreyren commented Oct 7, 2023

Can you describe the steps you used to test this? -- @samueldr (Can you describe the steps you used to test this?)

It was flashed on SDCard with known working AerithForge image to verify that the bootloader works as expected, didn't find any unexpected issues with it.

@samueldr
Copy link
Member

samueldr commented Oct 7, 2023

It was flashed on SDCard with known working AerithForge image to verify that the bootloader works as expected, didn't find any unexpected issues with it.

Can you describe the commands you have used to build this?

As it is, this change has at least two fundamental issues meaning this change as it is was not tested.

@K900
Copy link
Contributor

K900 commented Oct 7, 2023

@ofborg build ubootOlimexA64Teres1

@Kreyren
Copy link
Contributor Author

Kreyren commented Oct 7, 2023

Can you describe the commands you have used to build this? -- @samueldr (#259077 (comment))

Used:

  • $ nix build ".#ubootOlimexA64Teres1" on teres
  • $ nix build .#ubootOlimexA64Teres1 --system aarch64-linux on a x86_64 host with binfmt

Both of them built and seem to work without issues

As it is, this change has at least two fundamental issues meaning this change as it is was not tested. -- @samueldr (#259077 (comment))

None that i can see beyond crust which is not a subject to this merge request elaborate?


EDIT: After writing that reply i noticed that pkgs/top-level/all-packages.nix was not staged, it seems that my file syncing service is messing with git directories i disabled it for the time being and will monitor it, thanks for catching it.
Also noticed that on binfmt it's asking for SCP and fails if it's not provided, which doesn't seem to do that on teres itself so added SCP=/dev/null per docs to build without the SCP firmware.

The contribution was re-tested and seems to work.

@Kreyren
Copy link
Contributor Author

Kreyren commented Jan 17, 2024

@ofborg build ubootOlimexA64Teres1

Copy link
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

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

@Kreyren Can you just leave a brief comment in the code explaining the SCP = "/dev/null"; part, and then squash everything into one commit?

@Kreyren
Copy link
Contributor Author

Kreyren commented Feb 3, 2024

@Kreyren Can you just leave a brief comment in the code explaining the SCP = "/dev/null"; part, and then squash everything into one commit? -- @ElvishJerricco (#259077 (review))

It seems that by default u-boot will expect the SCP variable declared with value pointing to the scp.bin directory from crust-firmware to build the support for power management.
According to the u-boot docs the crust-firmware integration can be skipped by using /dev/null which is set as it's currently not packaged in NixOS (i will try to follow up on this merge request with crust package to set the variable and make power management functional as teres won't be able to successfully wake up from suspension without it which is important for it's practical use).

squashing..

@ElvishJerricco
Copy link
Contributor

@Kreyren Sorry, I meant I'd like to see a sentence or two added in a comment in the code, so that future readers of it know why it's there.

@Kreyren
Copy link
Contributor Author

Kreyren commented Feb 3, 2024

@Kreyren Sorry, I meant I'd like to see a sentence or two added in a comment in the code, so that future readers of it know why it's there. -- @ElvishJerricco (#259077 (comment))

Understood, i will add the comment as soon as my infra stops building (i had scheduled a big compilation for today that can't be paused and currently makes it impossible to submit changes as the scheduler is overwhelmed and git timesout)

pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
Add U-Boot Package Declaration for OLIMEX Teres-I
@ElvishJerricco ElvishJerricco merged commit c45d87c into NixOS:master Feb 4, 2024
23 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.

None yet

4 participants