driver-power-meson64-reset.patch: adapt and re-enable for Linux 7.1#9780
Open
EvilOlaf wants to merge 1 commit into
Open
driver-power-meson64-reset.patch: adapt and re-enable for Linux 7.1#9780EvilOlaf wants to merge 1 commit into
EvilOlaf wants to merge 1 commit into
Conversation
EvilOlaf
requested review from
NicoD-SBC,
SteeManMI,
Tonymac32,
adeepn,
igorpecovnik,
pyavitz,
rpardini and
teknoid
as code owners
Contributor
📝 WalkthroughWalkthroughThis pull request disables a Meson64 power/reset driver patch that was added to support system restart and power-off on Amlogic G12A, G12B, and SM1 SoCs. The patch is disabled because it fails to compile on kernel 7.1 due to the upstream removal of the ChangesMeson64 Power/Reset Driver Disabling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
github-actions
Bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch (1)
177-180: ⚡ Quick winHardcoded magic number
0x82000042needs a comment or named constant.The value
0x82000042is an undocumented bare hex literal with no explanation of what SiP/vendor service it invokes, what argument1means, or why it must be called beforepsci_function_id_poweroff. This will be completely opaque to anyone maintaining the driver later.🔧 Suggested fix
+/* Amlogic SiP service: set shutdown reason before PSCI SYSTEM_OFF */ +#define MESON64_SIP_SET_POWEROFF_REASON 0x82000042 +#define MESON64_POWEROFF_REASON_NORMAL 1 + static void do_meson64_poweroff(void) { meson64_card_reset(); - __invoke_psci_fn_smc(0x82000042, 1, 0, 0); + meson64_invoke_smc(MESON64_SIP_SET_POWEROFF_REASON, + MESON64_POWEROFF_REASON_NORMAL, 0, 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch` around lines 177 - 180, Replace the bare hex literal 0x82000042 with a well-named constant and add a concise comment explaining the vendor/SiP SMC being invoked, the meaning of the argument '1', and why it must be called before __invoke_psci_fn_smc(psci_function_id_poweroff,...); specifically, define a constant (e.g., MESON_SIP_PRE_POWEROFF_SMC or MESON_SIP_SYSTEM_CONTROL_SMC) and use that constant in the call to __invoke_psci_fn_smc, and add a one- to two-line comment above the call describing the SMC function ID, the vendor providing it, the effect of the '1' parameter, and why it precedes psci_function_id_poweroff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch`:
- Around line 54-65: The driver source drivers/power/reset/meson64-reboot.c is
missing a MODULE_LICENSE declaration causing the kernel to be tainted; add a
MODULE_LICENSE("GPL") (or another appropriate SPDX-compatible license string)
near the top of the file (after includes or the header comment) so the kernel
recognizes the module license; you can also add optional MODULE_AUTHOR and
MODULE_DESCRIPTION for completeness, but ensure MODULE_LICENSE is present in
meson64-reboot.c.
- Around line 95-108: The functions __invoke_psci_fn_smc and meson64_card_reset
currently have external linkage; make both functions static to limit their
scope: rename __invoke_psci_fn_smc to meson64_invoke_smc (update all local call
sites to use meson64_invoke_smc) and change its signature to static noinline int
meson64_invoke_smc(...), and mark meson64_card_reset as static void
meson64_card_reset(void); ensure you update every reference in this file to the
new meson64_invoke_smc symbol so callers compile after the rename.
- Around line 196-213: Replace the immediate-drive acquisitions with preserving
acquisitions and proper error handling: acquire sd_vqsw_desc, sd_vmmc_desc and
sd_vqen_desc using gpiod_get_optional(&pdev->dev, "name", GPIOD_ASIS) (not
GPIOD_OUT_LOW) and if any gpiod_get_optional returns ERR_PTR(-EPROBE_DEFER)
return that error so the probe defers instead of silently dropping GPIO support;
treat other ERR_PTRs by setting the corresponding descriptor to NULL. Add a
platform remove callback (e.g., meson64_restart_remove) that calls gpiod_put()
for sd_vqen_desc, sd_vmmc_desc, sd_vqsw_desc if non-NULL and clears pm_power_off
if it equals do_meson64_poweroff, and wire that remove into the platform_driver
struct for meson64_restart_driver so resources are released on unload.
---
Nitpick comments:
In `@patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch`:
- Around line 177-180: Replace the bare hex literal 0x82000042 with a well-named
constant and add a concise comment explaining the vendor/SiP SMC being invoked,
the meaning of the argument '1', and why it must be called before
__invoke_psci_fn_smc(psci_function_id_poweroff,...); specifically, define a
constant (e.g., MESON_SIP_PRE_POWEROFF_SMC or MESON_SIP_SYSTEM_CONTROL_SMC) and
use that constant in the call to __invoke_psci_fn_smc, and add a one- to
two-line comment above the call describing the SMC function ID, the vendor
providing it, the effect of the '1' parameter, and why it precedes
psci_function_id_poweroff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f01d145-dbed-414d-aaea-a8e65bd24faf
📒 Files selected for processing (2)
patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patchpatch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch.disabled
💤 Files with no reviewable changes (1)
- patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch.disabled
Comment on lines
+54
to
+65
| +/* | ||
| + * drivers/power/reset/meson64-reboot.c | ||
| + * | ||
| + * Copyright (C) 2017 Amlogic, Inc. All rights reserved. | ||
| + * Updated for Linux 7.1 | ||
| + * | ||
| + * This program is free software; you can redistribute it and/or modify | ||
| + * it under the terms of the GNU General Public License as published by | ||
| + * the Free Software Foundation; either version 2 of the License, or | ||
| + * (at your option) any later version. | ||
| + * | ||
| + */ |
Contributor
There was a problem hiding this comment.
Missing MODULE_LICENSE — will taint the kernel.
There is no MODULE_LICENSE declaration anywhere in the driver. Without it the kernel marks itself as tainted with TAINT_PROPRIETARY_MODULE at boot and emits a loud warning. Even built-in drivers (registered via device_initcall) must declare a license.
🔧 Proposed fix
device_initcall(meson64_restart_init);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Meson64 reboot/power-off driver");
+MODULE_AUTHOR("Patrick Yavitz <pyavitz@gmail.com>");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch` around
lines 54 - 65, The driver source drivers/power/reset/meson64-reboot.c is missing
a MODULE_LICENSE declaration causing the kernel to be tainted; add a
MODULE_LICENSE("GPL") (or another appropriate SPDX-compatible license string)
near the top of the file (after includes or the header comment) so the kernel
recognizes the module license; you can also add optional MODULE_AUTHOR and
MODULE_DESCRIPTION for completeness, but ensure MODULE_LICENSE is present in
meson64-reboot.c.
Comment on lines
+95
to
+108
| +static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64 arg1, | ||
| + u64 arg2) | ||
| +{ | ||
| + struct arm_smccc_res res; | ||
| + | ||
| + arm_smccc_smc((unsigned long)function_id, | ||
| + (unsigned long)arg0, | ||
| + (unsigned long)arg1, | ||
| + (unsigned long)arg2, | ||
| + 0, 0, 0, 0, &res); | ||
| + return res.a0; | ||
| +} | ||
| + | ||
| +void meson64_card_reset(void) |
Contributor
There was a problem hiding this comment.
Both __invoke_psci_fn_smc and meson64_card_reset must be static.
Neither function is declared static, so both are exported into the kernel's global symbol namespace. __invoke_psci_fn_smc collides in name with the function in drivers/firmware/psci/psci.c (currently static __always_inline there, so no linker error today — but that could change). meson64_card_reset is called only within this file and has no EXPORT_SYMBOL, so there is no reason for external linkage.
🔧 Proposed fix
-static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64 arg1,
+static noinline int meson64_invoke_smc(u64 function_id, u64 arg0, u64 arg1,
u64 arg2)
{
...
}
-void meson64_card_reset(void)
+static void meson64_card_reset(void)
{Update all call sites of __invoke_psci_fn_smc → meson64_invoke_smc accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch` around
lines 95 - 108, The functions __invoke_psci_fn_smc and meson64_card_reset
currently have external linkage; make both functions static to limit their
scope: rename __invoke_psci_fn_smc to meson64_invoke_smc (update all local call
sites to use meson64_invoke_smc) and change its signature to static noinline int
meson64_invoke_smc(...), and mark meson64_card_reset as static void
meson64_card_reset(void); ensure you update every reference in this file to the
new meson64_invoke_smc symbol so callers compile after the rename.
Comment on lines
+196
to
+213
| + if (!of_property_read_u32(of_node, "sys_poweroff", &id)) { | ||
| + psci_function_id_poweroff = id; | ||
| + pm_power_off = do_meson64_poweroff; | ||
| + } | ||
| + | ||
| + sd_vqsw_desc = gpiod_get(&pdev->dev, "sd-vqsw", GPIOD_OUT_LOW); | ||
| + if (IS_ERR(sd_vqsw_desc)) | ||
| + sd_vqsw_desc = NULL; | ||
| + | ||
| + sd_vmmc_desc = gpiod_get(&pdev->dev, "sd-vmmc", GPIOD_OUT_LOW); | ||
| + if (IS_ERR(sd_vmmc_desc)) | ||
| + sd_vmmc_desc = NULL; | ||
| + | ||
| + sd_vqen_desc = gpiod_get(&pdev->dev, "sd-vqen", GPIOD_OUT_LOW); | ||
| + if (IS_ERR(sd_vqen_desc)) | ||
| + sd_vqen_desc = NULL; | ||
| + | ||
| + return 0; |
Contributor
There was a problem hiding this comment.
GPIOD_OUT_LOW at probe drives SD power GPIOs low immediately, corrupting SD state.
gpiod_get(&pdev->dev, "sd-vmmc", GPIOD_OUT_LOW) and the other GPIOD_OUT_LOW acquisitions immediately configure and assert the GPIOs as driven-low at driver probe time. sd-vmmc is an SD card power rail and sd-vqsw is a voltage switch — pulling them low during normal operation will cut SD card power and break any mounted SD filesystem before the first reboot.
The GPIOs should be acquired with GPIOD_ASIS (preserve firmware-set state) and only toggled during the actual reset/poweroff path.
Additionally:
gpiod_get(notgpiod_get_optional) is used, thenIS_ERRresults are silently suppressed. This swallows-EPROBE_DEFER: if the GPIO controller has not yet probed, the driver silently continues with no GPIO support instead of deferring.- There is no
platform_driver.removecallback, sogpiod_putis never called for any acquired GPIO, andpm_power_offis set but never cleared on unload/remove.
🔧 Proposed fix
- sd_vqsw_desc = gpiod_get(&pdev->dev, "sd-vqsw", GPIOD_OUT_LOW);
- if (IS_ERR(sd_vqsw_desc))
- sd_vqsw_desc = NULL;
-
- sd_vmmc_desc = gpiod_get(&pdev->dev, "sd-vmmc", GPIOD_OUT_LOW);
- if (IS_ERR(sd_vmmc_desc))
- sd_vmmc_desc = NULL;
-
- sd_vqen_desc = gpiod_get(&pdev->dev, "sd-vqen", GPIOD_OUT_LOW);
- if (IS_ERR(sd_vqen_desc))
- sd_vqen_desc = NULL;
+ sd_vqsw_desc = gpiod_get_optional(&pdev->dev, "sd-vqsw", GPIOD_ASIS);
+ if (IS_ERR(sd_vqsw_desc))
+ return PTR_ERR(sd_vqsw_desc);
+
+ sd_vmmc_desc = gpiod_get_optional(&pdev->dev, "sd-vmmc", GPIOD_ASIS);
+ if (IS_ERR(sd_vmmc_desc)) {
+ ret = PTR_ERR(sd_vmmc_desc);
+ goto err_put_vqsw;
+ }
+
+ sd_vqen_desc = gpiod_get_optional(&pdev->dev, "sd-vqen", GPIOD_ASIS);
+ if (IS_ERR(sd_vqen_desc)) {
+ ret = PTR_ERR(sd_vqen_desc);
+ goto err_put_vmmc;
+ }
+
return 0;
+
+err_put_vmmc:
+ gpiod_put(sd_vmmc_desc);
+err_put_vqsw:
+ gpiod_put(sd_vqsw_desc);
+ return ret;Add a remove callback to meson64_restart_driver:
static int meson64_restart_remove(struct platform_device *pdev)
{
if (sd_vqen_desc) gpiod_put(sd_vqen_desc);
if (sd_vmmc_desc) gpiod_put(sd_vmmc_desc);
if (sd_vqsw_desc) gpiod_put(sd_vqsw_desc);
if (pm_power_off == do_meson64_poweroff)
pm_power_off = NULL;
return 0;
}And wire it in:
static struct platform_driver meson64_restart_driver = {
.probe = meson64_restart_probe,
+ .remove = meson64_restart_remove,
.driver = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patch/kernel/archive/meson64-7.1/driver-power-meson64-reset.patch` around
lines 196 - 213, Replace the immediate-drive acquisitions with preserving
acquisitions and proper error handling: acquire sd_vqsw_desc, sd_vmmc_desc and
sd_vqen_desc using gpiod_get_optional(&pdev->dev, "name", GPIOD_ASIS) (not
GPIOD_OUT_LOW) and if any gpiod_get_optional returns ERR_PTR(-EPROBE_DEFER)
return that error so the probe defers instead of silently dropping GPIO support;
treat other ERR_PTRs by setting the corresponding descriptor to NULL. Add a
platform remove callback (e.g., meson64_restart_remove) that calls gpiod_put()
for sd_vqen_desc, sd_vmmc_desc, sd_vqsw_desc if non-NULL and clears pm_power_off
if it equals do_meson64_poweroff, and wire that remove into the platform_driver
struct for meson64_restart_driver so resources are released on unload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #9777
thx @pyavitz
How Has This Been Tested?
Checklist:
Summary by CodeRabbit