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

Improve device-tree overlay support #79370

Merged
merged 1 commit into from Sep 9, 2020
Merged

Improve device-tree overlay support #79370

merged 1 commit into from Sep 9, 2020

Conversation

@sorki
Copy link
Member

@sorki sorki commented Feb 6, 2020

Now allows applying external overlays either in form of
.dts file, literal dts contents added to store or precompiled .dtbo file.

If overlays are defined, kernel device-trees are compiled with -@
so the .dtb files contain symbols which we can reference in our
overlays.

Since fdtoverlay doesn't respect / compatible by itself
we query compatible strings of both dtb and dtbo(verlay)
and apply only if latter is substring of the former.

Also adds support for filtering .dtb files (as there are now nearly 1k
dtbs).

Mashup of

Introduces a breaking change for users of hardware.deviceTree.base which is now renamed to hardware.deviceTree.kernelPackage and requires actual kernel package for dtc recompilation to work.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Member

@samueldr samueldr left a comment

Introduces a breaking change for users of hardware.deviceTree.base which is now renamed to hardware.deviceTree.kernelPackage and requires actual kernel package for dtc recompilation to work.

We'll need the release notes to show the breaking change.

@samueldr
Copy link
Member

@samueldr samueldr commented Feb 6, 2020

I don't have the setup to test, exactly, but I do have some hardware I should setup to test.

Otherwise, at a glance this looks fine, though not super thrilled about any breaking change, but the change is for the best considering it allows multiple representations of device trees to be used here.

@sorki sorki force-pushed the sorki:dtoverlays branch from 815595b to 3ea8c80 Feb 7, 2020
@sorki
Copy link
Member Author

@sorki sorki commented Feb 7, 2020

You can test on rPi with https://gist.github.com/sorki/c72dfbf1af51f7a42693622fbd020f07

I think that the previous hardware.deviceTree.base package was used mainly with deviceTree_rpi (which should probably be removed as well as in #67989). Provided you don't need to apply overlays you can still set hardware.deviceTree.kernelPackage to custom derivation. I guess that not many people are using their custom dtbs instead of one provided by kernel + overlays as that would require a lot of maintenance.

@sorki sorki force-pushed the sorki:dtoverlays branch from 3ea8c80 to 84daf7a Feb 7, 2020
@sorki
Copy link
Member Author

@sorki sorki commented Feb 7, 2020

We'll need the release notes to show the breaking change.

Release note added

@samueldr
Copy link
Member

@samueldr samueldr commented Feb 7, 2020

cc @bennofs who played with out of tree DTBs recently. Might have some thoughts about this.

@samueldr
Copy link
Member

@samueldr samueldr commented Feb 7, 2020

When I said "hardware to test" I meant I have a raspberry pi touch screen I can test this with, still hadn't looked at how to make it work. My main issue is that I would prefer the mainline kernel, but there's a dire lack of documentation about "raspberry pi special hardware" and mainline.

@bennofs
bennofs approved these changes Feb 7, 2020
Copy link
Contributor

@bennofs bennofs left a comment

I like it, especially the recompiling the kernel dts with symbols part 👍 Because I didn't have the patience to figure out how that works, I just compiled a completely new device tree for my own use case.

I added two review comments, but I would also be fine with the current state of the PR.

nixos/modules/hardware/device-tree.nix Show resolved Hide resolved
installPhase = ''
make dtbs_install INSTALL_DTBS_PATH=$out/dtbs
'';
};

This comment has been minimized.

@bennofs

bennofs Feb 7, 2020
Contributor

Would it make sense to move this function, and maybe also filterDTBs and compileDTS to nixpkgs (next to applyOverlays)? I think these functions might sometimes be useful for users to build their own device tree packages, and having them inside the module makes it hard to reuse.

This comment has been minimized.

@sorki

sorki Feb 8, 2020
Author Member

compileDTS sounds good, not sure about filterDTBs as that is a pretty generic store path filter which resembles filterSource but for store paths..

This comment has been minimized.

@sorki

sorki May 2, 2020
Author Member

For now I'm keeping this internal, if you would like to have this exposed from pkgs let me know and I'll try to fit it somewhere but I'm bad at namespaces and naming :) It also needs a better name in that case, something like pkgs.compileDeviceTreeOverlays.

Copy link
Contributor

@kwohlfahrt kwohlfahrt left a comment

I won't be able to test this until Monday, but at first glance I like the overlayType and filterDtbs.

I don't like the testing for /compatible, I feel like trying to apply an overlay to an incompatible device-tree should give a visible error rather than being silently ignored. Any thoughts from others?

@sorki
Copy link
Member Author

@sorki sorki commented Feb 8, 2020

I don't like the testing for /compatible, I feel like trying to apply an overlay to an incompatible device-tree should give a visible error rather than being silently ignored. Any thoughts from others?

At first I've tried applying overlays to all devices but that results in tons of errors on the output as most are incompatible (I've thought fdtoverlay respects compatible automatically but that's not the case).

Then my second version had overlay.match POSIX regex for find but that looked needlessly complicated and redundant to compatible strings in dts itself so I've stripped that and used fdtget instead to query both. It now results in following log output:

Applying overlay spi to bcm2835-rpi-zero-w.dtb
Applying overlay pps to bcm2835-rpi-zero-w.dtb
Applying overlay spi to bcm2836-rpi-2-b.dtb
Applying overlay pps to bcm2836-rpi-2-b.dtb
...

Which I think is better than seeing bunch of errors due to incompatibilities.

@kwohlfahrt
Copy link
Contributor

@kwohlfahrt kwohlfahrt commented Feb 8, 2020

Would it make sense to do the filtering in filterDtbs? Something like:

filterDtbs = {
  name = "...";
  compatible = "...";
}

And then only copying based on fdtget output?

Pros:

  • Lets you avoid errors on applying overlays to device-trees you don't mean to
  • Gives explicit errors when overlays unexpectedly don't apply due to compatibility

Cons:

  • Need to specify compatibility twice (once in overlay, once in nixpkgs)
  • Doesn't work if you have multiple overlays that are meant to apply to distinct subsets of your .dtb collection. I'm not sure if this is a common use-case?

I'm not 100% opposed to the current solution, it is very flexible.

@sorki sorki force-pushed the sorki:dtoverlays branch from 84daf7a to fcc0c72 Feb 9, 2020
@sorki
Copy link
Member Author

@sorki sorki commented Feb 9, 2020

Would it make sense to do the filtering in filterDtbs? Something like:

Not sure if that would be an improvement - often you want to patch only few files but preserve the rest, filterDTBs is good for saving some CPU time or if you only need DTBs for one specific board and don't care about the rest. I'm not a big fan of duplicating compatible + another filter which is why I've scraped that attempt - the implementation was also becoming too complicated compared to current state.

@sorki
Copy link
Member Author

@sorki sorki commented Feb 9, 2020

Removed option is now addressed via mkRemovedOptionModule.

@samueldr samueldr dismissed their stale review Feb 9, 2020

Now I want to test it, to be sure

@kwohlfahrt
Copy link
Contributor

@kwohlfahrt kwohlfahrt commented Feb 9, 2020

OK, I'm convinced by the current approach for compatible. I'll try and test it tomorrow.

Copy link
Contributor

@georgewhewell georgewhewell left a comment

LGTM

@samueldr
Copy link
Member

@samueldr samueldr commented Feb 13, 2020

Hmm, this is a bit annoying...

Either:

  • We have the authorization by @NixOS/nixos-release-managers to backport this mildly-breaking change to 20.03.
  • The release notes are moved to 20.09.

Otherwise looks fine to me.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 8, 2020

I'm cool with this being brought into alpha since we haven't reached the beta freeze just yet (and it was supposed to be in the last release anyways)

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 8, 2020

Be sure to direct requests to releases managers via the team @NixOS/nixos-release-managers

@jonringer jonringer added this to the 20.09 milestone Sep 8, 2020
@sorki sorki force-pushed the sorki:dtoverlays branch from f2a392d to 081782b Sep 9, 2020
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 9, 2020

Ah wait, I think this commit should be
nixos/device-tree: improve support

Now allows applying external overlays either in form of
.dts file, literal dts context added to store or precompiled .dtbo.

If overlays are defined, kernel device-trees are compiled with '-@'
so the .dtb files contain symbols which we can reference in our
overlays.

Since `fdtoverlay` doesn't respect `/ compatible` by itself
we query compatible strings of both `dtb` and `dtbo(verlay)`
and apply only if latter is substring of the former.

Also adds support for filtering .dtb files (as there are now nearly 1k
dtbs).

Co-authored-by: georgewhewell <georgerw@gmail.com>
Co-authored-by: Kai Wohlfahrt <kai.wohlfahrt@gmail.com>
@sorki sorki force-pushed the sorki:dtoverlays branch from 081782b to 6c9df40 Sep 9, 2020
@sorki
Copy link
Member Author

@sorki sorki commented Sep 9, 2020

Ah wait, I think this commit should be
nixos/device-tree: improve support

Changed to nixos/device-tree: improve overlays support, thanks!

@worldofpeace worldofpeace merged commit f0f88be into NixOS:master Sep 9, 2020
2 of 4 checks passed
2 of 4 checks passed
tests tests
Details
action
Details
Wait for ofborg This failed status will be cleared when ofborg finishes eval.
Details
grahamcofborg-eval Checking original out paths
Details
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 9, 2020

@sorki Okay, merged.

@timclassic
Copy link
Contributor

@timclassic timclassic commented Sep 19, 2020

I believe the merge of this PR has broken the "With GPU" instructions on the wiki (https://nixos.wiki/wiki/NixOS_on_ARM/Raspberry_Pi_4#With_GPU). I get the following error now:

# nixos-rebuild switch
building Nix...
building the system configuration...
error:
Failed assertions:
- The option definition `hardware.deviceTree.base' in `/etc/nixos/configuration.nix' no longer has any effect; please remove it.
Use hardware.deviceTree.kernelPackage instead

(use '--show-trace' to show detailed location information)

I tried deleting the hardware.deviceTree.base option but now X complains that it can't find /dev/dri/card0 and fails to start. What should I should provide for the hardware.deviceTree.kernelPackage option to produce the same behavior provided by base = pkgs.device-tree_rpi;?

@jonringer
Copy link
Contributor

@jonringer jonringer commented Sep 20, 2020

@lovesegfault
Copy link
Contributor

@lovesegfault lovesegfault commented Sep 20, 2020

I too am struggling with these changes. It's not clear to me how the wiki instructions for using the RPi4 GPU should be now, and separately I'm trying to apply a dtbo file but finding it hard.

  hardware.deviceTree.overlays = [{
    name = "hyperpixel4";
    dtboFile = "${pkgs.hyperpixel4}/share/overlays/hyperpixel4.dtbo";
  }];

I thought this would be sufficient (with hardware.deviceTree.enable = true) but the dtbo is not applied.

@sorki
Copy link
Member Author

@sorki sorki commented Sep 21, 2020

During build, you should see a line saying Applying overlay xyz to .... If that's not the case decompile the .dtbo file with dtc -I dtb -O dts < some.dtbo (using dtc from nix-shell -p dtc) and check its toplevel compatible directive which should say something like compatible = "brcm,bcm2835"; or compatible = "raspberrypi".

vc4-fkms-v3d.dtbo looks good to me, not sure about the hyperpixel one.

Setting a kernel package shouldn't be needed but I have some trouble reproducing this (wiki vp4 instructions) with linuxPackages_rpi4 as it gives me an error during make ... dtbs cross compilation (kernel package itself seems to cross fine):

gcc: error: unrecognized argument in option '-mabi=aapcs-linux'
gcc: note: valid arguments to '-mabi=' are: ms sysv
gcc: error: unrecognized command line option '-mlittle-endian'
gcc: error: unrecognized command line option '-mfpu=vfp'
make[1]: *** [Kbuild:21: kernel/bounds.s] Error 1

I'll try native build later.

@tmplt
Copy link
Contributor

@tmplt tmplt commented Sep 24, 2020

I'm trying to use this module on a Raspberry Pi 3B to mirror the effect of a dtparam=spi=on (if I were on Raspbian), but I'm shooting a bit in the dark to be honest. On f0f88be with

# Unkown if needed, but read in some relevant thread that RPI overlays do not merge with mainline.
boot.kernelPackages = pkgs.linuxPackages_rpi3;

hardware.deviceTree = {
  enable = true;
  filter = "*-rpi-*.dtb";
  overlays = [{
    name = "spi";
    dtboFile = "${pkgs.device-tree_rpi.overlays}/spi0-hw-cs.dtbo";
}];

the image from sd-image-aarch64.nix builds and boots, but the build output did not contain any mention of overlay application, and I have no /dev/spi* files. The dtb file I presume is being used is bcm2837-rpi-3-b.dtb which contains compatible = "raspberrypi,3-model-b\0brcm,bcm2837"; while the dtbo contains compatible = "brcm,bcm2835";. But after boot dmesg contains some bcm2835 entries. Is there some minor revision mismatch only in the DT files? Is that why it didn't apply?

@sorki
Copy link
Member Author

@sorki sorki commented Sep 24, 2020

@tmplt If there's no specific reason to use linuxPackages_rpi3 you can use my SPI overlay with upstream kernel:

If you switch compatible of the overlay to "raspberrypi" or "brcm,bcm2837" it would apply it.
I'm now a bit worried we need some fallback way to handle these unfortunate rpi foundation kernels and overlays.

@tmplt
Copy link
Contributor

@tmplt tmplt commented Sep 25, 2020

@sorki I'm trying to build with that overlay using nixpkgs.crossSystem.system = "aarch64-linux", but it seems dtbs-with-symbols-aarch64-* fails to build with gcc: error: unrecognized command line option '-mlittle-endian' (see below). Do you use armv7l-linux in the configuration you linked? Does this derivation only build for a set of supported archs?

I also have a aarch64-linux cross-compilation in progress via binfmt, but it's taking ages to build on my local system. I don't know if it works yet, but it did not stumble upon the error mentioned above.


these derivations will be built:
  /nix/store/lwc5gw8grp40cchv5xvq01602v0m7yp6-dtbs-with-symbols-aarch64-unknown-linux-gnu.drv
  /nix/store/nh3dk24r1hyv6ma5k016zs8hphw6vvs0-dtbs-filtered.drv
  /nix/store/fpf45n6g6wq78ngvzlqmxh59b0wn1p6l-device-tree-overlays.drv
  /nix/store/r7vr2w4k243v9vkg4y8faskz373h10jn-nixos-system-nixos-21.03pre-git.drv
  /nix/store/3lbafcis8zabrwf9mqb2ffv6yaiw4s8i-closure-info.drv
  /nix/store/mjlbs80iy0abn5psxhcvyn22ffvr8bvm-ext4-fs.img.zst-aarch64-unknown-linux-gnu.drv
  /nix/store/jj6r2kjspl95z939spwc4b3wqncp36h8-nixos-sd-image-21.03pre-git-aarch64-linux.img-aarch64-unknown-linux-gnu.drv
[...]
  WRAP    arch/arm64/include/generated/asm/trace_clock.h
  WRAP    arch/arm64/include/generated/asm/unaligned.h
  WRAP    arch/arm64/include/generated/asm/user.h
  WRAP    arch/arm64/include/generated/asm/vga.h
  WRAP    arch/arm64/include/generated/asm/xor.h
  UPD     include/generated/uapi/linux/version.h
  UPD     include/generated/utsrelease.h
  CC      kernel/bounds.s
gcc: error: unrecognized command line option '-mlittle-endian'
make[1]: *** [Kbuild:21: kernel/bounds.s] Error 1
make: *** [Makefile:1103: prepare0] Error 2
builder for '/nix/store/lwc5gw8grp40cchv5xvq01602v0m7yp6-dtbs-with-symbols-aarch64-unknown-linux-gnu.drv' failed with exit code 2
error: build of '/nix/store/lwc5gw8grp40cchv5xvq01602v0m7yp6-dtbs-with-symbols-aarch64-unknown-linux-gnu.drv' on 'ssh://builder@tmplt.dev' failed: builder for '/nix/store/lwc5gw8grp40cchv5xvq01602v0m7yp6-dtbs-with-symbols-aarch64-unknown-linux-gnu.drv' failed with exit code 2
builder for '/nix/store/lwc5gw8grp40cchv5xvq01602v0m7yp6-dtbs-with-symbols-aarch64-unknown-linux-gnu.drv' failed with exit code 1
cannot build derivation '/nix/store/nh3dk24r1hyv6ma5k016zs8hphw6vvs0-dtbs-filtered.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/fpf45n6g6wq78ngvzlqmxh59b0wn1p6l-device-tree-overlays.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/r7vr2w4k243v9vkg4y8faskz373h10jn-nixos-system-nixos-21.03pre-git.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/3lbafcis8zabrwf9mqb2ffv6yaiw4s8i-closure-info.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/mjlbs80iy0abn5psxhcvyn22ffvr8bvm-ext4-fs.img.zst-aarch64-unknown-linux-gnu.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/jj6r2kjspl95z939spwc4b3wqncp36h8-nixos-sd-image-21.03pre-git-aarch64-linux.img-aarch64-unknown-linux-gnu.drv': 1 dependencies couldn't be built
error: build of '/nix/store/jj6r2kjspl95z939spwc4b3wqncp36h8-nixos-sd-image-21.03pre-git-aarch64-linux.img-aarch64-unknown-linux-gnu.drv' failed
@sorki
Copy link
Member Author

@sorki sorki commented Sep 25, 2020

@tmplt It contains cross compilation parts and it works for me with mainline kernels (yes I'm mostly using armv7l, will test on aarch64 as well). Do you use it with mainline kernel or _rpi one? I've seen similar errors with _rpi kernel.

@tmplt
Copy link
Contributor

@tmplt tmplt commented Sep 25, 2020

@sorki Mainline linuxPackages_4_19. I just tried with linuxPackages_latest which passes the build, but it does not boot: uboot reports ERROR: Did not find a cmdline Flattened Device Tree after which a Staring kernel... but stops there. I do not know if this is caused by the 5.8 kernel or the DT overlay.

EDIT: just tried a build without the overlay and got the same error. Is support for older kernel versions possible?

@tmplt
Copy link
Contributor

@tmplt tmplt commented Sep 25, 2020

I also have a aarch64-linux cross-compilation in progress via binfmt

It finished, and it works: spidev and spi_bcm2835 are loaded and /dev/spi0.{0,1} exists. There is some differences between the expessions used for the binfmt build vis-a-vis the nixpkgs.crossSystem.system = "aarch64-linux" one. I'll dig them up if you need them.

tmplt added a commit to tmplt/ed7039e that referenced this pull request Sep 25, 2020
This commit pins nixpkgs for a recent modification of the deviceTree
module [0] that is needed to properly apply an overlay that enables
SPI for communication with the BrickPi3.

Additionally, all required libs are installed into a modified Python
environment.

Resolves #9.

[0] NixOS/nixpkgs#79370
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Sep 25, 2020

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/4

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 1, 2020

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

https://discourse.nixos.org/t/cross-compiling-problem-with-devicetree-on-armv6/9254/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.