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

nixos/hardware.deviceTree: new module #60422

Merged
merged 3 commits into from Aug 16, 2019
Merged

Conversation

kwohlfahrt
Copy link
Contributor

@kwohlfahrt kwohlfahrt commented Apr 29, 2019

Motivation for this change

Add support for custom device-tree files, as well as configuring overlays applied to device-trees. These are needed in embedded devices to expose non-discoverable hardware to the kernel, and are common on Raspberry Pi and similar.

This does not include support for Raspberry Pi style device-tree parameters (dtparam), as there does not seem to be a command-line tool available to apply these parameters to an overlay (if there is one in nixpkgs, I'm happy to extend this PR to include support).

@samueldr was particularly interested on IRC.

An example configuration might be:

hardware.deviceTree = {
  enable = true;
  base = pkgs.deviceTree.raspberryPiDtbs;
  overlays = [ "${pkgs.deviceTree.raspberryPiOverlays}/w1-gpio.dtbo" ];
};
Things done

This can be used to build a sd-image, that contains appropriately modified device-tree files. I'll test the result over the next few days, but would appreciate some comments on the API in the meantime.

I'm particularly not sure about how things should be split between nixos and nixpkgs, and whether I've understood the options interface correctly.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

'';
});

raspberryPiDtbs = stdenvNoCC.mkDerivation {
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to put the raspberry pi overlays here? This package could grow a bit large if you start adding overlays for other boards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I'm not at all sure. Where would you suggest they go?

Copy link
Member

Choose a reason for hiding this comment

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

I would probably put them in separate nix files in pkgs/os-specific/linux/overlays (or something like that), but I'd like a second opinion before I suggest you do that.

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've moved them - does it look better now?

@@ -0,0 +1,28 @@
{ stdenvNoCC, dtc, findutils, raspberrypifw }:
Copy link
Member

Choose a reason for hiding this comment

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

Somewhat related to @artemist's comment:

Nothing in pkgs/os-specific/linux/ is directly at the root, everything ends up in sub-folder. The generic tooling implementation (applyOverlays) would be fine in default.nix in that sub-folder.

Additionally, the kernel folder can be used as an inspiration, where linux-rpi.nix provides the kernel for the raspberry pi family. The raspberry pi foundation's device tree things could be found under pkgs/os-specific/linux/device-tree/raspberrypi.nix for example.

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've moved these files as well.

@@ -159,6 +159,8 @@ in

demoit = callPackage ../servers/demoit { };

deviceTree = callPackage ../os-specific/linux/device-tree.nix {};
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what's the better implementation for the "API" in all-packages.nix for providing device trees.

I'm thinking that yes, helpers like applyOverlays is fine under deviceTree, but thinking that the raspberry pi blobs (and possibly others in the future) shouldn't be found under that name. Just like we don't have linux.v4_4 but linux_4_4. raspberryPiDtbs and raspberryPiOverlays might be better names, but suggestions welcome.

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've moved these packages, does the new organization look better?

@samueldr
Copy link
Member

samueldr commented Apr 29, 2019

👋 Hi! Amazing contribution.

Some minor changes asked, and some questions left unanswered, but at a glance this looks good.

I guess I need to set myself up to test soon :).

@kwohlfahrt
Copy link
Contributor Author

kwohlfahrt commented Apr 30, 2019

Spoiler alert: the example suggested doesn't work as-is.

The initial problem is that the .dtb file in raspberrypifw is not named according to the uboot convention. I will fix this for the raspberryPiDtbs derivation (see pkgs/os-specific/linux/kernel/linux-rpi.nix for similar). EDIT: In fact, this should probably be fixed in uboot-builder.nix, not linux-rpi.nix, since it's a uboot specific problem.

Secondly, once the w1-gpio overlay is enabled, the boot process hangs after loading the kernel. This could be a hardware issue though, I haven't confirmed that my 1-wire device works on stock raspbian.

@samueldr
Copy link
Member

Mixing raspberry pi foundation device trees with mainline kernels may not work. I am not 100% sure though. From what I understand, while they describe the same hardware, the way they are described in the targeted Linux kernel versions is different.

I know that, at least with a dtbo, there were changes required for it to begin kind of working (it failed due to what I assume were other issues to check).

@kwohlfahrt
Copy link
Contributor Author

Interesting, I thought I'd compared the .dts files in both kernels and found them to be identical. And then compiled them to .dtb and found them to also be identical to raspberrypifw with the exception of the __symbols__ section. But I'll check that again, and also try with a RPi kerne.


config = mkIf (cfg.enable) {
hardware.deviceTree.package = if (cfg.overlays != [])
then pkgs.deviceTree.applyOverlays cfg.base cfg.overlays else cfg.base;
Copy link
Contributor

Choose a reason for hiding this comment

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

This prevents you from setting package manually. I believe that if if package needs to be settable as an option there probably needs to be some kind of logic like

mkIf (cfg.enable) 
  (mkIf (cfg.overlays != [] || cfg.base != null) 
    { hardware.deviceTree.package = ...; })

I.e. With just package set I'm getting

The unique option `hardware.deviceTree.package' is defined multiple times, in `...'  and `/nix/store/.../nixos/modules/hardware/device-tree.nix'

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 don't think package needs to bet settable - if overlays is empty, then base just becomes package. Is making internal = true; the correct way of making it not settable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is making internal = true; the correct way of making it not settable?

I'm not sure, possibly? Should it be exposed via mkOption at all? I imagined that it would have just been a simple let binding if it wasn't intended to be exposed as a settable option, but I could be wrong about that.

let 
  package = if (cfg.overlays != [])
       then pkgs.deviceTree.applyOverlays cfg.base cfg.overlays else cfg.base;
in
   ...

@kwohlfahrt
Copy link
Contributor Author

I have moved the packages containing device-tree files. I'll next test exactly what the compatibility is between linux kernels, rpi kernels and raspberrypifw. If there are significant issues, we may want to add a deviceTrees to kernelPackages_xyz, so we can have easy access to .dtbs matching a particular kernel?

@@ -159,6 +159,10 @@ in

demoit = callPackage ../servers/demoit { };

deviceTree = callPackage ../os-specific/linux/device-tree {};
Copy link
Member

Choose a reason for hiding this comment

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

No camelCase in package attributes please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deviceTree is not a package, it currently only contains the applyOverlays attribute, which is for applying overlays to derivations containing device-tree files. Should this still be renamed?

deviceTree_rpi is a package, I will rename this to device-tree_rpi (to parallel linux_rpi) unless somebody suggests a better name.

@kwohlfahrt
Copy link
Contributor Author

kwohlfahrt commented May 15, 2019

I've tested that the following configurations work on my Raspberry Pi (with the u-boot loader):

With linuxPackages_4_19:

  • defaults
  • deviceTree.enable = true;
  • linuxPackages_4_19 does not work with deviceTree.base = pkgs.device-tree_rpi. Should there be any checks for compatibility, or should we just document that setting deviceTree.base may give an unbootable system?

With linuxPackages_rpi:

  • defaults
  • deviceTree.enable = true
  • deviceTree.base = pkgs.device-tree_rpi seems to be OK.
  • the w1-gpio1 overlay works, and my device is available (yay! that was the point of this for me)

Edit: The first time I tried deviceTree.base it I couldn't get SSH access, but when it restarted plugged into my monitor & keyboard it came up fine. Moved back to it's usual headless location it has worked since, without any changes, so I'm putting that one down to a network issue.

@kwohlfahrt
Copy link
Contributor Author

@samueldr could you have another look at this? I think I have addressed all the comments and it is ready for testing/review now that it is working for me.

I would maybe like to move the renaming bcm*.dtb from linux-rpi.nix and device-tree/raspberrypi.nix into a shared place, maybe in the device-tree folder?

kwohlfahrt and others added 3 commits August 7, 2019 13:51
Add support for custom device-tree files, and applying overlays to them.
This is useful for supporting non-discoverable hardware, such as sensors
attached to GPIO pins on a Raspberry Pi.
In response to comments, create a sub-folder for deviceTree packages
(starting with rpi), and a top-level package for helpers.
This is just as messy as it is for the kernel files. Maybe it should be
done in the uboot boot-loader?
@kwohlfahrt
Copy link
Contributor Author

@infinisil and @samueldr, is anything preventing this from being merged? I'd like to have it in for 19.09...

I've been running this on my rpi3 with a temperature sensor for several months with no issues.

@samueldr
Copy link
Member

Nothing, except being unexpectedly busy lately. Thanks for the contribution!

@samueldr samueldr merged commit b750ebf into NixOS:master Aug 16, 2019
@kwohlfahrt kwohlfahrt deleted the device-tree branch August 19, 2019 19:31
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/enabling-spi-on-the-raspberry-pi-3/9122/3

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

6 participants