Sunxi64: Update defconfig(s) and disable h616 RTC patches #8368
Conversation
These two patches muddle up Bluetooth which depends on RTC functioning properly. Signed-off-by: Patrick Yavitz <[email protected]>
This was required in order to get the wif-pwrseq and bluetooth functioning correctly on the BPI-M4-Zero. I suspect it will have no negative results on other units. But of course this needs to be tested. Signed-off-by: Patrick Yavitz <[email protected]>
Prevent dmesg spam: brcmf_set_channel set chanspec xxxxxxx fail Signed-off-by: Patrick Yavitz <[email protected]>
pyavitz
requested review from
AGM1968,
JohnTheCoolingFan,
PanderMusubi,
TheSnowfield,
Tonymac32,
igorpecovnik,
janprunk,
krachlatte,
leggewie,
schwar3kat,
sgjava and
teknoid
as code owners
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
You can disable this status message by setting the WalkthroughA new shell function named Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/large
coderabbitai
bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
config/boards/bananapim4zero.conf (1)
31-31: Remove unnecessary sleep operation.The
sleep .25appears unnecessary sincegit cloneis synchronous and will complete before the next command executes.Apply this diff to remove the unnecessary sleep:
- sleep .25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
patch/kernel/archive/sunxi-6.12/series.confis excluded by!patch/**patch/kernel/archive/sunxi-6.15/series.confis excluded by!patch/**
📒 Files selected for processing (1)
config/boards/bananapim4zero.conf(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: EvilOlaf
PR: armbian/build#8328
File: lib/functions/compilation/patch/drivers_network.sh:542-545
Timestamp: 2025-06-24T10:08:40.313Z
Learning: In the Armbian build system, when a PR removes build support for a specific kernel version, version check issues for that removed version become practically irrelevant even if they appear incorrect in isolation. Context about which kernel versions are being deprecated/removed is important for understanding the impact of version-related code changes.
Learnt from: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
config/boards/bananapim4zero.conf (2)
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Shell script analysis
- GitHub Check: Check kernel security options
🔇 Additional comments (2)
config/boards/bananapim4zero.conf (2)
32-32: Add proper variable quoting for consistency.The file path variables should be quoted to prevent word splitting issues.
Apply this diff to improve consistency:
- if [[ -f "$SRC/cache/wireless-regdb/regulatory.db" ]] && [[ -f "$SRC/cache/wireless-regdb/regulatory.db.p7s" ]]; then + if [[ -f "$SRC/cache/wireless-regdb/regulatory.db" ]] && [[ -f "$SRC/cache/wireless-regdb/regulatory.db.p7s" ]]; thenActually, the quoting is already correct here. My mistake.
27-40: post_family_tweaks_bsp__bananapi_wireless_regdb conforms to Armbian conventions
- No other BSP post‐processing functions were found using the same pattern—this new function cleanly implements the intended upstream wireless-regdb installation.
- Connectivity check via
curland the subsequentgit clonestep align with existing board‐level workflows.Approving these changes.
And tick on a few misc bits Signed-off-by: Patrick Yavitz <[email protected]>
Signed-off-by: Patrick Yavitz <[email protected]>
Signed-off-by: Patrick Yavitz <[email protected]>
Signed-off-by: Patrick Yavitz <[email protected]>
This should be installed system wide. Signed-off-by: Patrick Yavitz <[email protected]>
pyavitz
changed the title
igorpecovnik
added
Ready to merge
| CONFIG_CAN_EMS_USB=m | ||
| CONFIG_CAN_ESD_USB=m | ||
| CONFIG_CAN_ETAS_ES58X=m | ||
| CONFIG_CAN_GS_USB=m |
There was a problem hiding this comment.
@pyavitz sorry to comment on an already merged PR, but if I'm not mistaken, this breaks usage of CANBus adapters commonly used in 3D printers.
I haven't seen any reasoning behind this removal, so can this be added again? Alternatively, is there a way for users of this modules to add them back without rebuilding the image?
There was a problem hiding this comment.
@mdziekon It's not an intentional removal. I started fresh, as the defconfigs were so bloated it was difficult for me to pin point the exact issue. Anything you would like added back can be done via PR.
If you are unfamiliar with doing a PR, I can do it for you if you provide the exact modules you need re-added.
There was a problem hiding this comment.
@pyavitz for me personally, I "think" I need the CONFIG_CAN_GS_USB flag, but to be fair I don't know if that's the only flag needed one for my setup to work again, and unfortunately I don't have a second testing device with CAN setup to test it on custom image build. Flags like CONFIG_CAN_VCAN and CONFIG_CAN_VXCAN can also be useful for creating virtual CAN devices for testing purposes.
However... is that really the way to go about this? This PR changes over 2k lines of configs, these changes already break current use cases (so these fixed one thing, and broke at least one other thing, and as you said yourself, it may break other things), and are spread across the entire sunxi64 platform (I'm not even using BPI-M4-Zero yet I have been affected). Some of the changes also seem a bit subjective, like the change of default scaling governor from ondemand to performance (correct me if I'm reading the changes related to CONFIG_CPU_FREQ_DEFAULT_GOV_ incorrectly), but that at least can be configured past build. I'm also not sure about "re-adding" stuff once stuff breaks - won't that eventually lead to the same problem (config bloat) in the end, as soon as other people notice their use cases are broken and can't be easily reconfigured past image build?
I know my comment may sound a bit like https://xkcd.com/1172/ and I do understand that refactors & cleanups are necessary, but breaking stuff for the sake of cleanup sounds wrong, in my opinion at least. I don't want to sound disrespectful, but I think I'd like for you (and maybe @igorpecovnik) to reconsider reverting this huge change and maybe taking different, more focused approach? However, if this is the approved way (I know these sunxi chips are usually "community supported" only, so maybe that's the general approach here that I'm not aware of?), then please ignore my rambling (well, maybe some public communication would be nice, so other people are more aware of this) and bring back the flags I've enumerated above.
There was a problem hiding this comment.
I'm sorry for the inconvenience. The best I can do is work with the current user base and other devs to get what needs to be ticked on for them, ticked on again.
If @igorpecovnik and yourself "and whom ever else" would rather just revert. Fine.
There was a problem hiding this comment.
Cleaning was perhaps too radical, but I don't think we need to revert. Perhaps AI can help to tell what to bring back?
armbian#8368 (review) Signed-off-by: Patrick Yavitz <[email protected]>
#8368 (review) Signed-off-by: Patrick Yavitz <[email protected]>
|
Why remove all PPS clients? please rollback and re-enable them |
YaoFei509
left a comment
YaoFei509
left a comment
There was a problem hiding this comment.
why did you disable all PPS clients? please rollback and re-enable them
Anything else? #8409 |
|
please disable and rollback this terrible PR。 WTF！ |
|
Why did you change so many network functions? Please rollback all of your stuff. |
|
Complains don't change things, contribution does. This is open source. You want something changed, go ahead and do it. |
armbian#8368 (review) Signed-off-by: Patrick Yavitz <[email protected]>
6 participants
NOTE: This PR may or may not break RTC functionality on other units.
On the M4-Zero I use the following in the wifi-pwrseq node.
I've noticed others using:
Sunxi64: Disable Allwinner H616 RTC support patches
These two patches muddle up Bluetooth which depends on RTC
functioning properly.
Update the linux-sunxi64-*.config(s)
This was required in order to get the wif-pwrseq and bluetooth
functioning correctly on the BPI-M4-Zero. I suspect it will
have no negative results on other units. But of course this
needs to be tested. The defconfigs are far more minimal now.
So I'm sure there will be something missing and will need to be
re-added.
https://paste.armbian.com/ibibusewom