u-boot: ROCK64 RAM init improvements #191538
Conversation
lorenz
force-pushed
the
rock64-uboot-improvements
branch
from
c9deb72 to
687d795
Compare
ofborg
bot
added
10.rebuild-darwin: 0
There was a problem hiding this comment.
This looks good and I'm glad to finally see the removal of the binary blob.
I tested ubootRock64v2 on my ROCK64 v2 and it boots, although not completely reliably. I don't have physical or UART access to the board right now (just a remote power switch), so I don't have any idea where it is getting stuck (it might not be U-Boot).
| # frequency. If your ROCK64 is unstable you can try this u-boot variant to | ||
| # see if it works better for you. The only disadvantage is lowered memory | ||
| # bandwidth. | ||
| ubootRock64v2 = buildUBoot { |
There was a problem hiding this comment.
minor: how about converting this to a function that accepts either { memFreq = "666"; } or { memFreq = "1600"; } as argument and calling it from all-packages with { memFreq = "666"; }?
There was a problem hiding this comment.
Not sure, the memory frequency is not just a tunable (it is also half, i.e. 333MHz / 800MHz), there is a whole host of timing parameters that need to be configured to make it work. There are only two specification in the u-boot tree, the "regular" 1600MT/s and the 666MT/s. I've actually reverse-engineered most of the memory controller, but I don't have the time currently to write a driver which would allow us to do something like that.
There was a problem hiding this comment.
I think @wamserma meant just templating the memFreq value into the .dtsi name to select the correct one without duplicating the whole buildUBoot invocation.
There was a problem hiding this comment.
If you want to further improve eval-time robustness, it could be checked with either an attrset of valid values, or couple of ifs. Otherwise it would fail during a needless attempt at building, which is still probably fine.
There was a problem hiding this comment.
Yes, it was just about templating it into the name because it is more readable (and thus clearer) than the replacement expression.
There was a problem hiding this comment.
I had a try at implementing this, but I struggled with where to put the function. There is a single callPackage in all-packages for these, which then get inherited up. So the only place I can see to put this is to put the function at the top together with buildUBoot and then call it twice. But that IMO makes the file relatively hard to read.
I've actually never had it fail at the u-boot stage already (with the full 1600MT/s), always after a few minutes to hours of operation. The special v2 version has been running on my V2s for ~3 months without any incident. Would be interesting to know if yours actually fails to start u-boot. |
lorenz
force-pushed
the
rock64-uboot-improvements
branch
from
687d795 to
1aec824
Compare
|
@lopsided98 Please retest this, I figured out that the patch hook for the reduced-clock version applied too late. Before I PRed this, I used a patch which applied earlier and it worked. |
|
That explains why my Rock64 locked up after a few days as well. I just tested the new version and it seems to boot reliably. |
|
Pinging @lopsided98 @dezgeg @samueldr |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/1392 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/1945 |
| # see if it works better for you. The only disadvantage is lowered memory | ||
| # bandwidth. | ||
| ubootRock64v2 = buildUBoot { | ||
| prePatch = '' |
There was a problem hiding this comment.
| prePatch = '' | |
| postPatch = '' |
There was a problem hiding this comment.
I think this would require a change to buildUBoot to avoid overwriting the postPatch defined there.
There was a problem hiding this comment.
Yup, this can be done, but would require adding a new argument to buildUBoot.
This switches the ROCK64 over to the open-source RAM init as it now works flawlessly. It also removes the HDCP flag from the ATF for the RK3328 as it cannot use it, it is only used in the RK3399. This makes the ROCK64 u-boot now fully open. There is also an issue with the ROCK64 v2 revision where the DRAM routing is marginal, making some of them unstable. So also package a variant which uses a lower-speed DDR3 timing configuration which is stable on these boards.
lorenz
force-pushed
the
rock64-uboot-improvements
branch
from
1aec824 to
6568016
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/installing-nixos-on-rock64-2024-experience/40954/1 |
Description of changes
This switches the ROCK64 over to the open-source RAM init as it now works flawlessly. It also removes the HDCP flag from the ATF for the RK3328 as it cannot use it, it is only used in the RK3399. This makes the ROCK64 u-boot now fully open.
There is also an issue with the ROCK64 v2 revision where the DRAM routing is marginal, making some of them unstable. So also package a variant which uses a lower-speed DDR3 timing configuration which is stable on these boards.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes