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

CM3588-NAS: Go fully mainline by adopting the latest mainline changes from kernel 6.11 and U-Boot v2024.10 #7082

Merged
merged 5 commits into from
Oct 28, 2024

Conversation

ColorfulRhino
Copy link
Collaborator

@ColorfulRhino ColorfulRhino commented Aug 13, 2024

BEFORE UPDATING YOUR CM3588 BOARD

Update procedure after this PR has been merged:

  1. On your board, edit the file /boot/armbianEnv.txt: Find the line fdtfile="rockchip/rk3588-nanopc-cm3588-nas.dtb" and replace this line with fdtfile="rockchip/rk3588-friendlyelec-cm3588-nas.dtb", DO NOT REBOOT YET
  2. Update your kernel as usual.
  3. Install the new bsp-cli package: apt remove armbian-bsp-cli-nanopc-cm3588-nas* and apt install armbian-bsp-cli-cm3588-nas-(current/vendor/edge depending on which kernel you use)

You only have to do this once, and only on existing installations. New installations and further update will not need any manual intervention.

Description

Important:

Full mainline support for the CM3588 NAS has been added to kernel 6.11 🎉 Make use of this device tree going forward.

In addition, support for mainline U-Boot is also in the pipeline and is available in v2024.10-rc3. Use this as well.

How Has This Been Tested?

  • edge: Successful boot without any obvious errors in dmesg with build command ./compile.sh BOARD=cm3588-nas BRANCH=edge BUILD_DESKTOP=no BUILD_MINIMAL=no EXPERT=yes KERNEL_CONFIGURE=no RELEASE=trixie
  • vendor: Successful boot without any obvious errors in dmesg with build command ./compile.sh BOARD=cm3588-nas BRANCH=vendor BUILD_DESKTOP=no BUILD_MINIMAL=no EXPERT=yes KERNEL_CONFIGURE=no RELEASE=trixie

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@github-actions github-actions bot added size/large PR with 250 lines or more Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Aug 13, 2024
@ColorfulRhino ColorfulRhino added the 08 Milestone: Third quarter release label Aug 13, 2024
@Kwiboo
Copy link
Contributor

Kwiboo commented Aug 13, 2024

@ColorfulRhino looks like my U-Boot rk3xxx-2024.10 branch is a bit out of date, changed the U-Boot defconfig name from friendlyelec-cm3588-nas-rk3588_defconfig to cm3588-nas-rk3588_defconfig just before submitting patches upstream.
That name / build target was just merged into U-Boot master u-boot/u-boot@06dceeb the other day, and will be included in v2024.10-rc3 that is planned to be released on Monday next week.

Will push an updated rk3xxx-2024.10 branch to my U-Boot repo that include the merged defconfig name, sorry if this cause any inconvenience.

@ColorfulRhino
Copy link
Collaborator Author

@ColorfulRhino looks like my U-Boot rk3xxx-2024.10 branch is a bit out of date, changed the U-Boot defconfig name from friendlyelec-cm3588-nas-rk3588_defconfig to cm3588-nas-rk3588_defconfig just before submitting patches upstream. That name / build target was just merged into U-Boot master u-boot/u-boot@06dceeb the other day, and will be included in v2024.10-rc3 that is planned to be released on Monday next week.

Will push an updated rk3xxx-2024.10 branch to my U-Boot repo that include the merged defconfig name, sorry if this cause any inconvenience.

Oh, no worries! Thanks for the heads-up and thanks for your work!

I'll wait for the rc3 and change the defconfig name.

@ColorfulRhino ColorfulRhino added the Work in progress Unfinished / work in progress label Aug 13, 2024
@ColorfulRhino ColorfulRhino force-pushed the cm3588-v6.11 branch 2 times, most recently from 8ac7bf6 to 6da2289 Compare August 19, 2024 20:44
@ColorfulRhino ColorfulRhino added Needs review Seeking for review and removed Work in progress Unfinished / work in progress labels Aug 19, 2024
@ColorfulRhino
Copy link
Collaborator Author

ColorfulRhino commented Aug 19, 2024

This is now rebased on the latest main branch where #7015 has been merged. U-Boot tag v2024.10-rc3 is not out as of now though. v2024.10-rc3 is out now.
Can be reviewed, but only merge after checking that rc3 is available https://github.com/u-boot/u-boot/tags

@ColorfulRhino
Copy link
Collaborator Author

Since the dts name changed: As far as I know the FDT file location is saved in the board's /boot/armbianEnv.txt. Will this be able to be updated on existing installations when the kernel is updated? Or do we need to change the kernel update script for this to work? Or is there another method? Does anybody know? @igorpecovnik

@igorpecovnik
Copy link
Member

Will this be able to be updated on existing installations when the kernel is updated?

No.

Or is there another method?

I developed "live patch service", but is not in function.

Two options:

  • add command to BSP postinst script so it runs sed when bsp package is updated
  • ignore and do nothing as this is not a supported target yet

@ColorfulRhino
Copy link
Collaborator Author

ColorfulRhino commented Aug 20, 2024

Will this be able to be updated on existing installations when the kernel is updated?

No.

Or is there another method?

I developed "live patch service", but is not in function.

Two options:

  • add command to BSP postinst script so it runs sed when bsp package is updated
  • ignore and do nothing as this is not a supported target yet

Thanks! This board does not have a BSP yet (neither does rk3588 unless there's something I don't know yet), so I'd rather not make it more complex. This is only a one time change anyway.
In this case, you're probably right, let's ignore this.

I'll update the main post in this PR to make users aware of the change. Edit: done!

@ColorfulRhino
Copy link
Collaborator Author

ColorfulRhino commented Aug 20, 2024

I have added a temprary help for people with existing installations, a pointer from the old dts to the new one, so they can migrate their /boot/armbianEnv.txt. This is just a temporary workaround though, will be finally removed in 6.12. This will make migration smoother since it gives people one kernel version to migrate, which should be enough.

@igorpecovnik
Copy link
Member

I have added a temprary help

Updated on download pages https://www.armbian.com/nanopc-cm3588-nas/

@rpardini
Copy link
Member

Hey @ColorfulRhino -- what's your opinion on the quality of the mainline DT? I mean, yours had very nice details like gpio line names, exposed pwm8 for buzzer, working USB & OTG, etc.

Ref the migration, I think that the board file rename is more impacting than the DT change: the bsp-cli pkg name will change, and existing users won't upgrade to the new pkg name. I personally wouldn't change the board file name right now.

@ColorfulRhino
Copy link
Collaborator Author

Updated on download pages https://www.armbian.com/nanopc-cm3588-nas/

Thanks! When someone downloads a fresh image, they will not be impacted by this though, since the fresh image (once this is merged and a new image is on the website).

what's your opinion on the quality of the mainline DT? I mean, yours had very nice details like gpio line names, exposed pwm8 for buzzer, working USB & OTG, etc.

It's good enough and has most things. USB seems to work and your buzzer is also exposed as far as I can see. I don't see any reason not to use it.

Ref the migration, I think that the board file rename is more impacting than the DT change: the bsp-cli pkg name will change, and existing users won't upgrade to the new pkg name. I personally wouldn't change the board file name right now.

Does this mean that with the name change, people won't get the updated kernel? What is shipped with the bsp-cli package?
As far as I understood, when the old dts is missing in the kernel, it won't boot at all after the new kernel update.
The best time for a name change was yesterday and the best time now is today imo. In the future it likely would be more painful 😄 Like @igorpecovnik said, this is in theory not a supported board.

@rpardini
Copy link
Member

rpardini commented Aug 23, 2024

Does this mean that with the name change, people won't get the updated kernel? What is shipped with the bsp-cli package?

No -- with a board name change, the kernel pkg would still be the same.

The bsp-cli contains... stuff I wish it didn't... and essentially family_tweaks_bsp highlander-function and post_family_tweaks_bsp hooks stuff -- that is both $BRANCH and $BOARD specific, so changing BOARD changes the bsp-cli package name -- users will need to apt install the new bsp-cli to keep getting updates to it.

As far as I understood, when the old dts is missing in the kernel, it won't boot at all after the new kernel update.

Yeah but I see you have an #include based placeholder -- so that should be fine for now.

The best time for a name change was yesterday and the best time now is today imo. In the future it likely would be more painful 😄 Like @igorpecovnik said, this is in theory not a supported board.

Indeed. Feel free, I just wanted to point out Armbian's limitations ref package renames. Similar conundrum's happen if we try to change LINUXFAMILY -- that would impact kernel/headers/dtb pkg names and is the reason we have rk35xx and rockchip-rk3588 etc

rpardini
rpardini previously approved these changes Aug 24, 2024
Copy link
Member

@rpardini rpardini left a comment

Choose a reason for hiding this comment

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

Cherry picked, built, tested.
Everything works as expected.
Some quality-of-life improvements needed:

  • mainline u-boot (24.10-rc3) is defaulting to booting from sd/emmc first than nvme (needs a patch, I think)
    • this is really in comparison to the vendor u-boot; the previous mainline u-boot (from t6) didn't boot from nvme at all
  • pwm-beeper on pwm8 needs CONFIG_INPUT_PWM_BEEPER in kernel config
  • compared to @ColorfulRhino 's DT, this one loses the cooling map / trip for the pwm-fan -- not a problem

I'll try to send fixes for those, but really in the meantime this is IMHO good to merge.

@rpardini
Copy link
Member

During testing I've found out that booting the vendor kernel using mainline u-boot doesn't work well. I guess the vendor kernel expects u-boot to do magic that mainline doesn't -- there's no nvme and no network.
I've brought back the vendor u-boot for vendor branch and it works again.

So I'd make mainline u-boot conditional for now.

I was driven to try vendor kernel due to some pcie crazy (nvme2 is detected sometimes, sometimes not).

@igorpecovnik
Copy link
Member

What this needs to completion?

@rpardini
Copy link
Member

we'll need to rebase around 6.11-rc5 that has a large DT Makefile change that affects our "overlay support" patch. That is not specific to this family, I think every family that has overlays will be affected.

Otherwise this is working fine -- can't both enum-pci-in-uboot-and-use-nvme-from-kernel but if booting from eMMC all is fine, at least, that I can detect.

@ColorfulRhino do you remember if the vendor u-boot scanned pci / enabled booting from NVMe? Because if it did, then this bump to mainline would break it; otherwise this is good to merge.

@ColorfulRhino
Copy link
Collaborator Author

Sorry for the delay. I will rebase this onto 6.12 and the latest U-Boot and test NVME and ethernet with the vendor kernel.

@igorpecovnik igorpecovnik added the Work in progress Unfinished / work in progress label Oct 18, 2024
Also add temporary compatibility patch for existing installations,
since existing installations will look for the old dts file.
To make migration smoother, add a pointer to the new file by
including the new dts in the old one.

For migration:
Please edit your `/boot/armbianEnv.txt` as soon as possible to
use BOOT_FDT_FILE="rockchip/rk3588-friendlyelec-cm3588-nas.dtb"!
This workaround will be removed in kernel version 6.14!
Support for the CM3588 NAS has been added in v2024.10-rc3.
Drop vendor U-Boot for this board completely.
@ColorfulRhino
Copy link
Collaborator Author

ColorfulRhino commented Oct 20, 2024

Otherwise this is working fine -- can't both enum-pci-in-uboot-and-use-nvme-from-kernel but if booting from eMMC all is fine, at least, that I can detect.

@ColorfulRhino do you remember if the vendor u-boot scanned pci / enabled booting from NVMe? Because if it did, then this bump to mainline would break it; otherwise this is good to merge.

I did various testing with U-Boot 2024.10 in combination with the vendor kernel, as well as other combinations. My findings:

  • I can confirm that stopping the boot process and using the pci enum command will break pci stuff after booting the vendor kernel. I spent a few hours trying to solve this by changing the board's DT in the vendor kernel, but couldn't fix it with the DT. I believe it's either a bug in U-Boot's pci driver or a bug in vendor kernel's pci driver.
  • Mainline U-Boot 2024.10 + mainline kernel does not have this problem.
  • I did also test to boot from NVMe with U-Boot 2024.10 + vendor kernel (using armbian-install). Works without problems!

To sum up, booting from NVMe on the vendor kernel works. You just can't stop the boot process and do a pci enum, but that's a very edge case scenario that will simply not be supported or may be fixed upstream if someone can figure out where the bug is.


@rpardini

  • mainline u-boot (24.10-rc3) is defaulting to booting from sd/emmc first than nvme (needs a patch, I think)
    • this is really in comparison to the vendor u-boot; the previous mainline u-boot (from t6) didn't boot from nvme at all

Booting from SD/emmc first is a good thing, since e.g. if you moved your main install to your NVMe SSD, it will boot from there. But if you want to test a new image, you can insert the SD card or flash the eMMC without touching or having to physically remove the NVMe drive.

  • pwm-beeper on pwm8 needs CONFIG_INPUT_PWM_BEEPER in kernel config

Is already enable, I checked.


This is ready to merge now, no need to wait for 6.12 since I have included the neccessary board patches in the 6.12 PR #7372

Thanks for your patience!

@ColorfulRhino ColorfulRhino added 11 Milestone: Fourth quarter release and removed Work in progress Unfinished / work in progress 08 Milestone: Third quarter release labels Oct 20, 2024
@konashevich
Copy link

Hi Devs, is there a way to get a realse for a fresh install not upgrade? Sorry, my question may be stupid but I am a noob

@igorpecovnik
Copy link
Member

igorpecovnik commented Oct 23, 2024

Hi Devs, is there a way to get a realse for a fresh install not upgrade? Sorry, my question may be stupid but I am a noob

Once job is done here and code merged, install images are generated once per week:
https://www.armbian.com/nanopc-cm3588-nas/

If you need it faster, or different style as offered for download, you need to build on your own:
https://docs.armbian.com/Developer-Guide_Build-Preparation/

@hbina
Copy link

hbina commented Oct 26, 2024

Hi, I tried the image provided here[1] but it doesn't detect my SSD on the first M.2 slot.

hanif@nanopc-cm3588-nas:~$ lsblk
NAME         MAJ:MIN RM   SIZE RO TYPE MOUNTPOINTS
sda            8:0    1  14.4G  0 disk 
├─sda1         8:1    1  14.4G  0 part /media/hanif/Ventoy
└─sda2         8:2    1    32M  0 part 
mmcblk0      179:0    0  57.6G  0 disk 
├─mmcblk0p1  179:1    0     4M  0 part 
├─mmcblk0p2  179:2    0     4M  0 part 
├─mmcblk0p3  179:3    0     4M  0 part 
├─mmcblk0p4  179:4    0    16M  0 part 
├─mmcblk0p5  179:5    0    40M  0 part 
├─mmcblk0p6  179:6    0    32M  0 part 
├─mmcblk0p7  179:7    0    32M  0 part 
├─mmcblk0p8  179:8    0   4.5G  0 part 
└─mmcblk0p9  179:9    0    53G  0 part 
mmcblk0boot0 179:32   0     4M  1 disk 
mmcblk0boot1 179:64   0     4M  1 disk 
mmcblk1      179:96   0  59.5G  0 disk 
└─mmcblk1p1  179:97   0  58.9G  0 part /var/log.hdd
                                       /
zram0        251:0    0  15.5G  0 disk [SWAP]
zram1        251:1    0    50M  0 disk /var/log
zram2        251:2    0     0B  0 disk 
nvme0n1      259:0    0 931.5G  0 disk 
├─nvme0n1p1  259:1    0   100M  0 part 
├─nvme0n1p2  259:2    0    16M  0 part 
├─nvme0n1p3  259:3    0 930.9G  0 part 
└─nvme0n1p4  259:4    0   522M  0 part 
hanif@nanopc-cm3588-nas:~$ lspci
0000:00:00.0 PCI bridge: Rockchip Electronics Co., Ltd RK3588 (rev 01)
0001:10:00.0 PCI bridge: Rockchip Electronics Co., Ltd RK3588 (rev 01)
0002:20:00.0 PCI bridge: Rockchip Electronics Co., Ltd RK3588 (rev 01)
0002:21:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN580 NVMe SSD (DRAM-less) (rev 01)
0003:30:00.0 PCI bridge: Rockchip Electronics Co., Ltd RK3588 (rev 01)
0004:40:00.0 PCI bridge: Rockchip Electronics Co., Ltd RK3588 (rev 01)
0004:41:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8125 2.5GbE Controller (rev 05)

[1] https://www.armbian.com/nanopc-cm3588-nas/

@ColorfulRhino
Copy link
Collaborator Author

ColorfulRhino commented Oct 27, 2024

Hi, I tried the image provided here[1] but it doesn't detect my SSD on the first M.2 slot.

Hello, your issue is not related to this PR. One NVMe is detected in your log. If you have two, maybe you've got one bad SSD.
Please test again when this PR has been merged and Armbian 2024.11 is out with Kernel 6.12. If the issue still persists, you can ask the community on the Armbian forums😄

@igorpecovnik igorpecovnik added Ready to merge Reviewed, tested and ready for merge and removed Needs review Seeking for review labels Oct 28, 2024
@igorpecovnik igorpecovnik merged commit 29adf3b into armbian:main Oct 28, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
11 Milestone: Fourth quarter release Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/large PR with 250 lines or more
Development

Successfully merging this pull request may close these issues.

6 participants