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.

Sign up for GitHub

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

OLIMEX Teres-I: Init #750

Merged
merged 2 commits into from
Oct 6, 2023
Merged

OLIMEX Teres-I: Init #750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09a45e6
OLIMEX Teres-I: Init
Kreyren Oct 4, 2023
c97d27d
Merge branch 'master' into teres_init
mergify[bot] Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@
omen-15-en1007sa = import ./omen/15-en1007sa;
omen-en00015p = import ./omen/en00015p;
onenetbook-4 = import ./onenetbook/4;
olimex-teres_i = import ./olimex/teres_i;
pcengines-apu = import ./pcengines/apu;
pine64-pinebook-pro = import ./pine64/pinebook-pro;
pine64-rockpro64 = import ./pine64/rockpro64;
Expand Down
30 changes: 30 additions & 0 deletions olimex/teres_i/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{ config, lib, pkgs, ...}:

{
hardware.deviceTree = {
name = lib.mkDefault "allwinner/sun50i-a64-teres-i.dts";
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed, as it is used to pick a different dtb filename from the default in the extlinux-compatible boot flow.

Setting it could (but probably won't) break things as it would not naturally change if the platform firmware + kernel pairing of DTB filenames change for the board.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See rationale in #750 (comment)

tldr: it's needed as the bootloader likes to sometimes prefer generic a64 sunxi dtb over the one specific to teres

Copy link
Member

Choose a reason for hiding this comment

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

Only mainline U-Boot is aimed to be supported, unless specified otherwise.

Mainline U-Boot will use the appropriate dtb filename here.

If it does not, you are not using mainline U-Boot, or have it misconfigured. (There is the unlikely possibility of a bug upstream, but if it did, every board at least using the same SoC would be broken, if not all boards using a dtb).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i do more tests on this and get back to you

enable = lib.mkDefault true;
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed.

in my tests it was sometimes using the sun50i-a64.dtsi so this is an attempt to mitigate that behavior (also observed on armbian).

It doesn't seem to be using the DTB if hardware.deviceTree.enable is set to zero.

Additionally i tested other kernels and they don't seem to work correctly if this is set so expecting the user to use the mainline, ideally it should have an option for linux-libre, but that seems to need an upstream changes to work correctly.

Copy link
Member

@samueldr samueldr Oct 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't seem to be using the DTB if hardware.deviceTree.enable is set to zero.

It simply can't be zero. It's either true or false, and on aarch64-linux pkgs.stdenv.hostPlatform.linux-kernel.DTB is always true, since it's a platform value.

So hardware.deviceTree.enable is always true for AArch64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

misspoke about the zero meant false

I do more tests on this and get back to you

};

boot = {
consoleLogLevel = lib.mkDefault 7;
Copy link
Member

Choose a reason for hiding this comment

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

Why change the default console log level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why change the default console log level? -- @samueldr (#750 (comment))

Other boards seems to change that so it's an attempt to follow the coding standard


kernelPackages = lib.mkDefault pkgs.linuxPackages_latest;
kernelParams = lib.mkDefault ["console=ttyS0,115200n8"];
Copy link
Member

@Mic92 Mic92 Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This will make serial the main way of seeing boot logs (other ttys will only see a getty later on). Is this the preferred way to access it? Does the board have other means like HDMI output that might be used? I.e. if someone doesn't have a serial adapter.

Copy link
Member

Choose a reason for hiding this comment

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

Also see #745 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will make serial the main way of seeing boot logs (other ttys will only see a getty later on). Is this the preferred way to access it? Does the board have other means like HDMI output that might be used? I.e. if someone doesn't have a serial adapter. -- @Mic92 (#750 (comment))

it has micro-hdmi and eDP both of which seem to show the full initialization of the OS.

Serial adapter is essential to be able to use the system as far as i know every teres user has one.

Also see #745 (comment) -- @Mic92 (#750 (comment))

Afaik it's not relevant, the initialization of rockchip differs from sunxi.

Copy link
Member

Choose a reason for hiding this comment

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

Allwinner and Rockchip are both irrelevant here. It's a kernel feature.

The reason this "works" is because this is a no-op, functionally speaking.

lib.mkDefault on a list that is being used by the modules system in any other way is extremely likely to mean the value is not being used. As boot.kernelParams is being used for many other reason, this line does nothing.

extraModulePackages = lib.mkDefault [];
Copy link
Member

Choose a reason for hiding this comment

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

This likely does nothing. See previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

boot.kernelPackages defaults to pkgs.linuxPackages where we expect to use latest kernel

kernelParams is important to provide the serial console, it was tested to not do that without the setting.

extraModulePackages is to ensure that it's empty by default

Copy link
Member

Choose a reason for hiding this comment

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

There is a fundamental misunderstanding of how the modules system work in here.

boot.kernelParams with lib.mkDefault will not do what you expect. I suspect what you mean here is that you have tested boot.kernelParams = ["console=ttyS0,115200n8"]; in your own config, then attempted to put it in here without validating that it was in use otherwise.

Here's a quick proof, this is evaluating a bare entirely unconfigured system, with your "configuration.nix" being only what is in the configuration arg.

~ $ nix repl --system aarch64-linux --arg configuration '{ lib, ... }: { boot.kernelParams = lib.mkDefault [ "THIS_IS_A_TEST" ]; }' '<nixpkgs/nixos>'

nix-repl> config.boot.kernelParams                              
[ "loglevel=4" ]

You will see that it will never end-up being used, as the moment anything sets boot.kernelParams, the priority is higher than the priority at mkDefault, and thus the whole list gets ignored.

Doing so for boot.extraModulePackages is also entirely unnecessary because NixOS modules system values need to be set to be used, and since it's always used, it is already set to a default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested both through my config and sourcing the fork, but i didn't notice different behavior

i will investigate and get back to you


initrd = {
availableKernelModules = lib.mkDefault ["usbhid"];
kernelModules = lib.mkDefault [];
Comment on lines +17 to +18
Copy link
Member

@samueldr samueldr Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These likely do nothing. See previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These likely do nothing. See previous comment. -- @samueldr (#750 (comment))

Link "the previous comment" next time, it makes it hard to follow plz.

Assuming you meant this one: #750 (comment)

usbhid is what generates nixos-generate-config when called so i am trying to follow that.

the unsets for empty lists are an attempt to highlight to the end-user that these are relevant for hardware modifications (very common practice among teres users) and to keep them empty by default.

Copy link
Member

Choose a reason for hiding this comment

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

the unsets for empty lists are an attempt to highlight to the end-user that these are relevant for hardware modifications (very common practice among teres users) and to keep them empty by default.

See this comment.

TLDR: it won't do anything as those are set with higher priority. Setting any list with lib.mkDefault, will not do what you expect here, and "setting" an "empty" list would not do what you expect here even if the priority was the same, with the merge semantics of lists.

};

loader = {
grub.enable = lib.mkDefault false;
generic-extlinux-compatible.enable = lib.mkDefault true;
};
};

nixpkgs.hostPlatform = lib.mkDefault "aarch64-linux";

powerManagement.cpuFreqGovernor = lib.mkDefault "ondemand";
}