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

OLIMEX Teres-I: Init #750

Merged
merged 2 commits into from
Oct 6, 2023
Merged

OLIMEX Teres-I: Init #750

merged 2 commits into from
Oct 6, 2023

Conversation

Kreyren
Copy link
Contributor

@Kreyren Kreyren commented Oct 4, 2023

Description of changes

Initial declaration of the device

Things done
  • Tested the changes in your own NixOS Configuration
  • [-] Tested the changes end-to-end by using your fork of nixos-hardware and
    importing it via <nixos-hardware> or Flake input

Initial declaration of the device
consoleLogLevel = lib.mkDefault 7;

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

@Mic92 Mic92 Oct 6, 2023

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.

@Mic92
Copy link
Member

Mic92 commented Oct 6, 2023

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2023

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 93fcc5f

@mergify mergify bot merged commit 93fcc5f into NixOS:master Oct 6, 2023
3 checks passed
};

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.


kernelPackages = lib.mkDefault pkgs.linuxPackages_latest;
kernelParams = lib.mkDefault ["console=ttyS0,115200n8"];
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

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

@samueldr samueldr Oct 6, 2023

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.

{
hardware.deviceTree = {
name = lib.mkDefault "allwinner/sun50i-a64-teres-i.dts";
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.

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

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


{
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

@Kreyren Kreyren deleted the teres_init branch March 14, 2024 06:10
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.

3 participants