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

rockchip64: fix rocks0 patch breaking current compilation #7815

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

paolosabatino
Copy link
Contributor

Description

A patch that reworks rocks0 board has been reworked as well to compile on kernel 6.12.13.
@brentr I just did a quick rework of the patch, but cannot test if it is resulting in a working device or not. Perhaps it would be better, in the future, to rewrite the patch to fit the changes on top of the mainline kernel device tree?

How Has This Been Tested?

  • Compiled rockchip64 current kernel without errors

Checklist:

  • I have performed a self-review of my own code
  • My changes generate no new warnings

@paolosabatino paolosabatino requested review from brentr and a team February 9, 2025 21:00
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Feb 9, 2025
@Kwiboo
Copy link
Contributor

Kwiboo commented Feb 10, 2025

Yes, this patch really shreds the upstream device tree apart. You should probably only keep the audio related acodec-sound, sound, pcm5102a, codec, i2s_8ch_0, i2s_8ch_2, thermal tsadc and board-antenna nodes in your downstream patch.

Also look like you should drop rk3308-0003-pinctrl-io-voltage-domains.patch since the io-domain driver exists upstream and also rk3308-add-gmac-alias.patch as the ethernet-aliases should be defined at board level not soc-level.

@brentr
Copy link
Collaborator

brentr commented Feb 11, 2025

@paolosabatino
I verified that this patch doesn't break anything major.
Still boots, WiFi works.
However, after merging it with upstream/main, the result would not compile due to DTC errors.

brentr
brentr previously requested changes Feb 11, 2025
Copy link
Collaborator

@brentr brentr left a comment

Choose a reason for hiding this comment

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

I've nothing against reorganizing the .dts files.
I am very much a neophyte in the wacky ways of device trees.
Please revise this patch to work with the current mainline.
Be sure to @brentr in your comment so I see it. I'll test try to test it that evening, hopefully before it gets stale.

@igorpecovnik
Copy link
Member

This ready?

@brentr
Copy link
Collaborator

brentr commented Feb 17, 2025

No. It no longer applies atop the current head.
I will change this PR to a "draft" if I can, until @paolosabatino weighs in.

I just verified that the current head builds a working image.
WiFi, Ethernet, 1.3ghz all work.
@igorpecovnik
Strangely, the apt update said a new kernel was available,
but, when I upgraded, it downgraded 6.12.12 OVER the running 6.12.13
Everything still works. Still, I suspect something's not right with the package dependencies.

@brentr brentr marked this pull request as draft February 17, 2025 01:00
@brentr
Copy link
Collaborator

brentr commented Feb 17, 2025

Also verified that RockPI-S image works when build from the main branch.
However, the PI-S has the same the same issue where apt upgrade reverts kernel to 6.12.12

@igorpecovnik
Copy link
Member

Still, I suspect something's not right with the package dependencies.

Oh, that was manual intervention of some other problem and i forgot to bump version of the package. If released image has package v1.0 and repository has package version v1.0, it will reinstall. But package version on the repo could have older kernel ... this anomaly will be fixed asap.

Ignore this upgrade - if build works, lets merge, versioning is unrelated problem.

@paolosabatino
Copy link
Contributor Author

paolosabatino commented Feb 17, 2025

@brentr @igorpecovnik I tried to compile on my local branch and it correctly applies on top of current (kernel 6.12.14):

immagine

and the kernel builds fine too. I rebased my local branch to the latest main branch and now the patch does not apply anymore, with some hunks failing.

It clashes with this other rk3308 patch: https://github.com/armbian/build/blob/main/patch/kernel/archive/rockchip64-6.12/board-rocks0-0001-Revert-arm64-dts-rockchip-Fix-sdmmc-access-on-rk3308.patch

This evening I can fix the clashing, if the timing is ok for you @igorpecovnik. Nonetheless, the patch in this branch requires a major overhaul to correctly integrate the mainline dts, and not tearing it apart like now. I could lend a hand to do it after the 25.02 release.

@igorpecovnik
Copy link
Member

Timing is OK, we are still in the 2025/2 release window.

@paolosabatino paolosabatino marked this pull request as ready for review February 17, 2025 18:20
@paolosabatino
Copy link
Contributor Author

@igorpecovnik ok, this should be good enough to go for 25.02

@igorpecovnik igorpecovnik added Ready to merge Reviewed, tested and ready for merge 02 Milestone: First quarter release 05 Milestone: Second quarter release and removed Needs review Seeking for review labels Feb 17, 2025
@paolosabatino paolosabatino dismissed brentr’s stale review February 17, 2025 20:38

changes have been accomodated to build against latest mainline sources

@paolosabatino
Copy link
Contributor Author

paolosabatino commented Feb 17, 2025

@brentr I "dismissed" the change request from your part since it was blocking the PR from merge and we're a bit tight with timing. Nothing personal, it's just I made the changes and now everything compiles fine against mainline kernel. Please write here if you got any other issue.

@paolosabatino paolosabatino merged commit e2f1c44 into armbian:main Feb 17, 2025
14 checks passed
@paolosabatino paolosabatino deleted the fix-rocks0-patch branch February 17, 2025 20:40
@brentr
Copy link
Collaborator

brentr commented Feb 18, 2025

@paolosabatino @igorpecovnik
Verified that Rock-S0 builds and boots with this patch.
(Both current and edge kernels)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02 Milestone: First quarter release 05 Milestone: Second 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/medium PR with more then 50 and less then 250 lines
Development

Successfully merging this pull request may close these issues.

4 participants