Add initial support for cix-p1 with kernel 7.0 tested and ms-r1 as test device#9842
Closed
EvilOlaf wants to merge 8 commits into
Closed
Add initial support for cix-p1 with kernel 7.0 tested and ms-r1 as test device#9842EvilOlaf wants to merge 8 commits into
EvilOlaf wants to merge 8 commits into
Conversation
Squashed commit from PR armbian#9395. Includes: - Board vendor configuration - Current (6.18) and edge (6.19) kernel support based on Sky1-Linux - Firmware and mesa backports for GPU support - Various fixes and improvements
upstream changes caused them to no longer apply
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive support for the CIX Sky1 SoC to the Armbian Linux distribution and kernel, encompassing board configuration, kernel parameters across three branches, and extensive kernel driver integration across clock, reset, mailbox, SCMI firmware, pin control, USB, and GPU subsystems. ChangesCIX Sky1 SoC Platform Enablement
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
Suggested reviewers
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
config/kernel/linux-cix-p1-vendor.config (1)
1-1209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis should be a minimal kernel-config overlay, not a full defconfig dump.
This vendor config is also committed as a full snapshot instead of a minimal overlay (Line 1 onward). Please regenerate with
./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch>and commit the reduced overlay output.Based on learnings:
config/kernel/files are expected to be minimal overlays regenerated by the Armbian rewrite-kernel-config flow.🤖 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 `@config/kernel/linux-cix-p1-vendor.config` around lines 1 - 1209, The vendor config file currently contains a full defconfig snapshot (starts with CONFIG_SYSVIPC and many other CONFIG_* entries) but must be a minimal kernel-config overlay; run the Armbian rewrite flow to replace it: execute ./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch> to regenerate a reduced overlay for linux-cix-p1-vendor.config, replace the large full dump with the generated minimal overlay, and commit that reduced file.config/kernel/linux-cix-p1-current.config (1)
1-2005:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis should be a minimal kernel-config overlay, not a full defconfig dump.
This file is a full config snapshot, which makes
config/kernel/hard to maintain and diverges from Armbian’s overlay workflow (Line 1 onward). Please regenerate via./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch>and commit the minimized result.Based on learnings: kernel config files under
config/kernel/must be minimal overlays and should be regenerated withrewrite-kernel-config.🤖 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 `@config/kernel/linux-cix-p1-current.config` around lines 1 - 2005, The file contains a full kernel defconfig snapshot (many CONFIG_* entries such as CONFIG_SYSVIPC and CONFIG_PREEMPT) but must be a minimal Armbian kernel-config overlay; regenerate the minimized overlay by running the rewrite tool (./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch>), replace this giant dump with the generated minimal overlay (only the overlayed CONFIG_ entries), verify it no longer contains the full list of options like CONFIG_SYSVIPC/CONFIG_PREEMPT, and commit the minimized result.config/kernel/linux-cix-p1-edge.config (1)
1-2007:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis should be a minimal kernel-config overlay, not a full defconfig dump.
This file is a full config snapshot, which is not the expected format for
config/kernel/overlays (Line 1 onward). Please regenerate with./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch>and keep only the minimized delta.Based on learnings: kernel config files in
config/kernel/should be minimal overlays generated viarewrite-kernel-config.🤖 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 `@config/kernel/linux-cix-p1-edge.config` around lines 1 - 2007, The file currently contains a full kernel defconfig snapshot (starts with CONFIG_SYSVIPC, CONFIG_POSIX_MQUEUE, etc.) but must be a minimal overlay; regenerate a minimized delta by running ./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch> and replace the current content with only the overlay output (the minimal differences), ensuring you remove the full defconfig dump and keep only the rewritten overlay entries (use the generated overlay as the new content for this file).
🧹 Nitpick comments (2)
patch/kernel/archive/cix-7.0/0006-firmware-arm_scmi-Add-ACPI-boot-support-for-CIX-Sky1.patch (1)
448-477: 💤 Low valueDebug prints should use
dev_*variants or be removed.The added
pr_info()andpr_err()calls insmc_chan_setup()should either usedev_info()/dev_err()with the appropriate device pointer for consistency with kernel logging conventions, or be removed before merge if they were only for debugging. Usingpr_*without a device context makes it harder to correlate logs with specific device instances.♻️ Suggested change
- pr_info("smc_chan_setup: tx=%d dev=%s cdev=%s\n", tx, - dev_name(dev), dev_name(cdev)); - pr_info("smc_chan_setup: dev->of_node=%pOF cdev->of_node=%pOF\n", - dev->of_node, cdev->of_node); + dev_dbg(dev, "smc_chan_setup: tx=%d cdev=%s\n", tx, dev_name(cdev)); + dev_dbg(dev, "smc_chan_setup: dev->of_node=%pOF cdev->of_node=%pOF\n", + dev->of_node, cdev->of_node); ... if (IS_ERR(scmi_info->shmem)) { - pr_err("smc_chan_setup: setup_iomap failed: %ld\n", + dev_err(dev, "smc_chan_setup: setup_iomap failed: %ld\n", PTR_ERR(scmi_info->shmem)); ... - pr_err("smc_chan_setup: arm,smc-id read failed: %d\n", ret); + dev_err(dev, "arm,smc-id read failed: %d\n", ret);🤖 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/cix-7.0/0006-firmware-arm_scmi-Add-ACPI-boot-support-for-CIX-Sky1.patch` around lines 448 - 477, The new debug prints in smc_chan_setup (the pr_info/pr_err lines referencing tx, dev_name(dev), dev_name(cdev), dev->of_node, cdev->of_node, and error cases around scmi_info->shmem and of_property_read_u32) should not use global pr_*; replace them with the appropriate dev_* helpers (e.g., dev_info/dev_err) and pass the correct device pointer (dev or cdev) so logs are attributed to the right device, or remove the prints if they were temporary debugging; ensure the error messages around PTR_ERR(scmi_info->shmem) and the of_property_read_u32 return use dev_err(dev, ...) (or dev_err(cdev, ...) where more appropriate) and that dev_name calls are not duplicated if dev_* already includes device context.patch/kernel/archive/cix-7.0/0007-pinctrl-cix-Update-Sky1-pin-controller.patch (1)
2006-2024: ⚡ Quick winUsing
devm_kmalloc_array()in suspend context is unusual.The
devm_kmalloc_array()call withGFP_ATOMICinpinctrl_save_context()during suspend is atypical. Devm allocations are tied to device lifetime and will only be freed when the device is removed, not on resume or error paths. Consider pre-allocating this buffer during probe instead.♻️ Suggested approach
Allocate
saved_valsduring probe when theSKY1_PINCTRL_CONTEXT_LOSS_OFFflag is set:// In sky1_pinctrl_probe() or sky1_base_pinctrl_probe(): + if (info->flags & SKY1_PINCTRL_CONTEXT_LOSS_OFF) { + spctl->saved_vals = devm_kmalloc_array(&pdev->dev, info->npins, + sizeof(*spctl->saved_vals), + GFP_KERNEL); + if (!spctl->saved_vals) + return -ENOMEM; + } // In pinctrl_save_context(): static int pinctrl_save_context(struct device *dev, struct sky1_pinctrl *spctl) { int i; unsigned long *reg; - if (!spctl->saved_vals) { - spctl->saved_vals = devm_kmalloc_array(dev, spctl->info->npins, - sizeof(*reg), - GFP_ATOMIC); - if (!spctl->saved_vals) - return -ENOMEM; - } + if (!spctl->saved_vals) + return -ENOMEM; reg = spctl->saved_vals;🤖 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/cix-7.0/0007-pinctrl-cix-Update-Sky1-pin-controller.patch` around lines 2006 - 2024, pinctrl_save_context currently allocates spctl->saved_vals with devm_kmalloc_array(GFP_ATOMIC) during suspend which is incorrect; instead allocate spctl->saved_vals in probe when SKY1_PINCTRL_CONTEXT_LOSS_OFF is set (e.g. in the driver probe function), using devm_kmalloc_array(dev, spctl->info->npins, sizeof(*spctl->saved_vals), GFP_KERNEL) and handle allocation failure there, then remove the allocation from pinctrl_save_context so that pinctrl_save_context only reads registers into the pre-allocated spctl->saved_vals; keep the existing usage of SKY1_CONF_MASK and the readl loop unchanged.
🤖 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 `@config/boards/cix-acpi.conf`:
- Around line 2-8: Add the missing metadata fields KERNEL_TEST_TARGET and
INTRODUCED to the board config: update the top-level variables alongside
BOARD_NAME/BOARD_VENDOR/KERNEL_TARGET by adding KERNEL_TEST_TARGET as a
comma-separated list of kernel branches used for CI/test targeting (e.g.,
"edge,vendor" style) and INTRODUCED as the first-ship year (four-digit year,
e.g., "2024"); ensure names match exactly (KERNEL_TEST_TARGET, INTRODUCED) and
place them near KERNEL_TARGET for visibility so the board-config validation sees
them.
In `@config/sources/families/cix-p1.conf`:
- Around line 63-65: The script downloads and immediately trusts the Sky1
repository key via run_host_command_logged to
"${SDCARD}/usr/share/keyrings/sky1-linux.asc" without verifying a known
fingerprint; modify the flow so you first download the key to a temp file,
compute its fingerprint (e.g., via gpg --with-fingerprint or similar), compare
it against the expected/pinned fingerprint constant, and only move/install the
key to usr/share/keyrings/sky1-linux.asc if the fingerprint matches; if it does
not match, use display_alert and run_host_command_logged to log and abort the
operation (do not trust the key). Ensure the expected fingerprint is stored as a
clear variable/constant and reference run_host_command_logged, display_alert,
and the target path in your changes.
In
`@patch/kernel/archive/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patch`:
- Around line 199-205: sky1_hwspinlock_unlock currently writes
SKY1_HWSPINLOCK_OWNER_ID back to the lock (in sky1_hwspinlock_unlock using
lock->priv), which reclaims ownership instead of freeing it; change the
implementation to write the hardware's release value (e.g., 0 or a defined
SKY1_HWSPINLOCK_RELEASE_VALUE) to lock_addr to clear the owner and actually
release the hwspinlock, or add and use a named macro for the release value if
one is expected by the hardware.
- Around line 568-574: The devm_ioremap_resource() call in the probe uses a NULL
check and manually iounmap() which is wrong; change the check on cd->pdc_base
(set by devm_ioremap_resource) to use IS_ERR(cd->pdc_base) and return
PTR_ERR(cd->pdc_base) on error, free cd as needed, and remove any manual
iounmap() calls so the devm-managed mapping is not explicitly unmapped; update
the error path around platform_get_resource()/devm_ioremap_resource() and
references to pdc_base and cd accordingly.
- Around line 1669-1671: The probe calls pm_runtime_enable(dev) followed by
pm_runtime_get_sync(dev) but ignores the return value; update the probe to check
the return of pm_runtime_get_sync (or replace both calls with
pm_runtime_resume_and_get(dev)) and handle errors: if the call returns a
negative error, undo runtime enable (pm_runtime_disable(dev)) and return the
error from the probe so we don't continue with an inactive device; reference
pm_runtime_get_sync, pm_runtime_enable, pm_runtime_disable, and
pm_runtime_resume_and_get when making the change.
In
`@patch/kernel/archive/cix-7.0/0004-reset-Add-Sky1-reset-controllers-and-lookup-table-AP.patch`:
- Around line 670-687: The sky1_reset function currently ignores return values
from sky1_reset_set calls; update sky1_reset to capture and check the return of
the first sky1_reset_set(rcdev, id, true) and if it returns a negative error
code return that error immediately (do not proceed to sleep/deassert), then call
the second sky1_reset_set(rcdev, id, false), capture its return and if negative
return that error instead of unconditionally returning 0; ensure existing
usleep_range calls remain only after successful writes so failure paths
propagate the regmap/write errors to the caller.
- Around line 701-710: sky1_reset_status does not check the return value of
regmap_read, so a read failure can leave `value` stale; capture the return value
from regmap_read (e.g., int ret = regmap_read(...)), if ret < 0 return that
error code (propagate the negative errno) and only compute and return !(value &
signal->bit) on success. Update the function body around the regmap_read call in
sky1_reset_status accordingly.
- Around line 337-354: The static lookups[] declared inside the probe function
causes reentrancy/list corruption because entries persist across remove/re-probe
and reset_controller_add_lookup() has no automatic undo; change the registration
so entries are not static per-probe: either move the lookup registration out of
the device probe into module init (register once), or allocate lookup entries
dynamically with devm_kzalloc and store them on the device so you can call
reset_controller_add_lookup() from probe and remove them in the driver's remove
callback, or add a static bool guard to ensure reset_controller_add_lookup()
runs only once; update code references to lookups, reset_controller_add_lookup,
has_acpi_companion, dev_name and implement corresponding cleanup in remove or
module exit as appropriate.
In
`@patch/kernel/archive/cix-7.0/0005-mailbox-cix-Add-ACPI-support-and-channel-lookup.patch`:
- Around line 593-598: The code logs an invalid index for CIX_MBOX_TYPE_FAST in
the MBOX_RX branch but returns the still-initialized-success value ret (from
request_irq()), leaving the IRQ registered; update the error path in the block
guarded by if (priv->dir == MBOX_RX) { ... } to free/unregister the IRQ and
return a proper negative error (e.g., call free_irq(irq, priv) or the equivalent
unregister routine used elsewhere then return -EINVAL or another appropriate
negative errno) instead of returning ret; reference symbols: priv->dir, MBOX_RX,
index, MBOX_FAST_IDX, request_irq(), ret, dev_err to locate and fix the code (or
add a goto-based cleanup like goto err_free_irq that calls free_irq before
returning the negative error).
- Around line 608-613: The default case returns -EINVAL without releasing the
IRQ that was requested earlier, leaking the IRQ resource; change the default
branch in the switch (where dev_err(priv->dev, "Invalid channel type: %d\n",
cp->type); currently returns -EINVAL) to free the IRQ that was allocated by the
earlier request_irq call (using the same IRQ identifier and dev_id used when
requesting it, e.g. free_irq(<irq_var>, <dev_id>)), then return -EINVAL (or jump
to the existing failed cleanup path instead of returning directly) so the IRQ is
properly released; reference the default case, cp->type, priv->dev, the earlier
request_irq call, and the failed cleanup label/ret variable when making the
change.
- Around line 419-432: The FIFO read loop removed the bounds check and can
overflow the local array data[CIX_MBOX_MSG_WORDS]; when handling int_status with
FIFO_FULL_INT|FIFO_WM01_INT (in the block that calls cix_mbox_write(...,
INT_CLEAR)), restore a safety limit: keep an index i and stop reading when i >=
CIX_MBOX_MSG_WORDS (or break out) in addition to checking mbox_fifo_empty(chan)
before calling cix_mbox_read(FIFO_RD), then pass the collected words to
mbox_chan_received_data(chan, data); ensure the same identifiers (int_status,
FIFO_FULL_INT, FIFO_WM01_INT, cix_mbox_write, cix_mbox_read, FIFO_RD,
mbox_fifo_empty, data/CIX_MBOX_MSG_WORDS, mbox_chan_received_data, chan,
INT_CLEAR) are used so the fix is applied to the correct loop.
In
`@patch/kernel/archive/cix-7.0/0007-pinctrl-cix-Update-Sky1-pin-controller.patch`:
- Around line 2026-2034: pinctrl_restore_context currently dereferences
spctl->saved_vals without validating it; add a NULL check at the start of
pinctrl_restore_context (checking spctl->saved_vals) and return early if NULL to
avoid dereferencing, e.g., if (!spctl->saved_vals) return; — make the check use
the same pointer (spctl->saved_vals) referenced later and leave the rest of the
loop that writes saved values to spctl->base + spctl->pin_regs[i] unchanged.
- Around line 15-26: The Makefile currently adds the cix/ directory twice: once
conditionally via "obj-$(CONFIG_PINCTRL_SKY1) += cix/" and again unconditionally
via "obj-y += cix/"; remove the conditional entry "obj-$(CONFIG_PINCTRL_SKY1) +=
cix/" so only the existing "obj-y += cix/" remains to avoid duplicate inclusion
and build errors.
In `@patch/kernel/archive/cix-7.0/0010-usb-Add-CIX-Sky1-USB-support.patch`:
- Around line 1056-1108: The MMIO lookup helpers (sky1_of_get_addr_by_name,
sky1_acpi_get_addr_by_name, sky1_get_addr_by_name) must return error pointers
instead of NULL so callers can detect failures; change all "return NULL" failure
paths to return ERR_PTR(-ENODEV) (or an appropriate -errno for ioremap
failures), and if ioremap returns NULL map that to ERR_PTR(-ENOMEM) (or
propagate ioremap error if using an ERR_PTR return). In
sky1_acpi_get_addr_by_name release resources on all paths: call
fwnode_handle_put(fwnode) after device_get_next_child_node when done, and call
put_device(device) after bus_find_device_by_fwnode on both success and error
paths (before returning). Also update sky1_put_addr to skip iounmap for error
pointers (check IS_ERR(regs) and non-NULL before iounmap). Ensure
sky1_get_addr_by_name simply forwards the ERR_PTR returned by the two helpers.
- Around line 514-533: The suspend path enables wake with enable_irq_wake() in
cdnsp_plat_suspend but cdnsp_plat_resume never disables it, leaving the IRQ wake
refcount elevated; update cdnsp_plat_resume to get the cdns via
dev_get_drvdata(dev) and, if device_may_wakeup(dev) && cdns->wakeup_irq, call
disable_irq_wake(cdns->wakeup_irq) (do this before returning from
cdnsp_plat_resume) so the wake refcount is balanced with the enable in
cdnsp_plat_suspend.
- Around line 1845-1941: rts5453_update_port() currently calls optional helpers
unconditionally and may dereference NULL when providers are absent; wrap calls
to typec_switch_set(rts->sw, ...) and typec_mux_set(rts->mux, &rts->mux_state)
in NULL checks (if (rts->sw) ...; if (rts->mux) ...), and similarly guard
usb_role_switch_set_role(rts->role_sw, role) and the typec_set_data_role(...)
call so they only run when rts->role_sw and rts->port are non-NULL respectively;
leave power-role updates as-is if rts->port is required/always present or guard
typec_set_pwr_role(rts->port, ...) with if (rts->port) if probe can leave it
NULL.
In
`@patch/kernel/archive/cix-7.0/0011-drm-panthor-Add-Sky1-GPU-support-and-ACPI.patch`:
- Around line 1203-1216: The Sky1 harvesting mask is only applied when
of_device_is_compatible(...) is true, which skips ACPI-backed devices even
though panthor_device_init() maps ptdev->sky1_rcsu_reg for ACPI; change the
condition so the mask is applied whenever ptdev->sky1_rcsu_reg is present (e.g.
replace the combined check with a simple if (ptdev->sky1_rcsu_reg) or
equivalent) and leave the masking logic that reads sky1_harvesting_reg_val and
updates ptdev->gpu_info.shader_present intact so fused-off shader cores are
hidden on ACPI systems as well.
- Around line 474-482: The helper panthor_is_sky1 must not treat the generic
"arm,mali-valhall" DT compatible as Sky1; remove the
of_device_is_compatible(dev->of_node, "arm,mali-valhall") branch and instead
detect Sky1 only via ACPI (acpi_dev_hid_uid_match(ACPI_COMPANION(dev),
"CIXH5000", NULL)) and a Sky1-specific DT compatible (e.g., check
of_device_is_compatible(dev->of_node, "cix,sky1") or similar vendor-specific
string), so update the panthor_is_sky1 function to first test ACPI then a
Sky1-specific OF compatible and drop the generic Valhall check.
- Around line 132-226: The SCMI frequency calls in panthor_devfreq_target use
the main GPU device `dev` but must use the perf-domain device stored in
`pdevfreq->opp_dev`; update both calls to panthor_devfreq_scmi_set_freq(...) to
pass `pdevfreq->opp_dev` (the perf-domain/genpd device recorded by
panthor_devfreq_scmi_init) instead of `dev`, keeping the surrounding logic
(locks, backoff, safe_freq path) unchanged.
---
Outside diff comments:
In `@config/kernel/linux-cix-p1-current.config`:
- Around line 1-2005: The file contains a full kernel defconfig snapshot (many
CONFIG_* entries such as CONFIG_SYSVIPC and CONFIG_PREEMPT) but must be a
minimal Armbian kernel-config overlay; regenerate the minimized overlay by
running the rewrite tool (./compile.sh rewrite-kernel-config BOARD=<board>
BRANCH=<branch>), replace this giant dump with the generated minimal overlay
(only the overlayed CONFIG_ entries), verify it no longer contains the full list
of options like CONFIG_SYSVIPC/CONFIG_PREEMPT, and commit the minimized result.
In `@config/kernel/linux-cix-p1-edge.config`:
- Around line 1-2007: The file currently contains a full kernel defconfig
snapshot (starts with CONFIG_SYSVIPC, CONFIG_POSIX_MQUEUE, etc.) but must be a
minimal overlay; regenerate a minimized delta by running ./compile.sh
rewrite-kernel-config BOARD=<board> BRANCH=<branch> and replace the current
content with only the overlay output (the minimal differences), ensuring you
remove the full defconfig dump and keep only the rewritten overlay entries (use
the generated overlay as the new content for this file).
In `@config/kernel/linux-cix-p1-vendor.config`:
- Around line 1-1209: The vendor config file currently contains a full defconfig
snapshot (starts with CONFIG_SYSVIPC and many other CONFIG_* entries) but must
be a minimal kernel-config overlay; run the Armbian rewrite flow to replace it:
execute ./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch> to
regenerate a reduced overlay for linux-cix-p1-vendor.config, replace the large
full dump with the generated minimal overlay, and commit that reduced file.
---
Nitpick comments:
In
`@patch/kernel/archive/cix-7.0/0006-firmware-arm_scmi-Add-ACPI-boot-support-for-CIX-Sky1.patch`:
- Around line 448-477: The new debug prints in smc_chan_setup (the
pr_info/pr_err lines referencing tx, dev_name(dev), dev_name(cdev),
dev->of_node, cdev->of_node, and error cases around scmi_info->shmem and
of_property_read_u32) should not use global pr_*; replace them with the
appropriate dev_* helpers (e.g., dev_info/dev_err) and pass the correct device
pointer (dev or cdev) so logs are attributed to the right device, or remove the
prints if they were temporary debugging; ensure the error messages around
PTR_ERR(scmi_info->shmem) and the of_property_read_u32 return use dev_err(dev,
...) (or dev_err(cdev, ...) where more appropriate) and that dev_name calls are
not duplicated if dev_* already includes device context.
In
`@patch/kernel/archive/cix-7.0/0007-pinctrl-cix-Update-Sky1-pin-controller.patch`:
- Around line 2006-2024: pinctrl_save_context currently allocates
spctl->saved_vals with devm_kmalloc_array(GFP_ATOMIC) during suspend which is
incorrect; instead allocate spctl->saved_vals in probe when
SKY1_PINCTRL_CONTEXT_LOSS_OFF is set (e.g. in the driver probe function), using
devm_kmalloc_array(dev, spctl->info->npins, sizeof(*spctl->saved_vals),
GFP_KERNEL) and handle allocation failure there, then remove the allocation from
pinctrl_save_context so that pinctrl_save_context only reads registers into the
pre-allocated spctl->saved_vals; keep the existing usage of SKY1_CONF_MASK and
the readl loop unchanged.
🪄 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: c401d72e-ff3f-4818-8dfd-4c81bac59b73
📒 Files selected for processing (47)
config/boards/cix-acpi.confconfig/kernel/linux-cix-p1-current.configconfig/kernel/linux-cix-p1-edge.configconfig/kernel/linux-cix-p1-vendor.configconfig/sources/families/cix-p1.confpatch/kernel/archive/cix-7.0/0001-arm64-dts-cix-Add-Sky1-SoC-and-board-device-trees.patchpatch/kernel/archive/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patchpatch/kernel/archive/cix-7.0/0003-clk-cix-Add-ACPI-clock-infrastructure-for-CIX-Sky1.patchpatch/kernel/archive/cix-7.0/0004-reset-Add-Sky1-reset-controllers-and-lookup-table-AP.patchpatch/kernel/archive/cix-7.0/0005-mailbox-cix-Add-ACPI-support-and-channel-lookup.patchpatch/kernel/archive/cix-7.0/0006-firmware-arm_scmi-Add-ACPI-boot-support-for-CIX-Sky1.patchpatch/kernel/archive/cix-7.0/0007-pinctrl-cix-Update-Sky1-pin-controller.patchpatch/kernel/archive/cix-7.0/0008-PCI-cadence-sky1-Add-ACPI-support-and-fixes.patchpatch/kernel/archive/cix-7.0/0009-phy-cix-Add-Sky1-USB-and-PCIe-PHY-drivers.patchpatch/kernel/archive/cix-7.0/0010-usb-Add-CIX-Sky1-USB-support.patchpatch/kernel/archive/cix-7.0/0011-drm-panthor-Add-Sky1-GPU-support-and-ACPI.patchpatch/kernel/archive/cix-7.0/0012-drm-cix-Add-Sky1-display-drivers.patchpatch/kernel/archive/cix-7.0/0013-net-Add-CIX-Sky1-networking-drivers.patchpatch/kernel/archive/cix-7.0/0014-sound-Add-CIX-Sky1-audio-drivers.patchpatch/kernel/archive/cix-7.0/0015-media-cix-Add-Sky1-video-codec-VPU-driver.patchpatch/kernel/archive/cix-7.0/0016-misc-armchina-npu-Add-Zhouyi-NPU-driver-for-CIX-Sky1.patchpatch/kernel/archive/cix-7.0/0017-thermal-cix-Add-Sky1-thermal-power-domain-and-cpufre.patchpatch/kernel/archive/cix-7.0/0018-arm64-cix-Add-Sky1-miscellaneous-peripheral-drivers.patchpatch/kernel/archive/cix-7.0/0019-remoteproc-cix-Add-DSP-remoteproc-and-rpmsg-support.patchpatch/kernel/archive/cix-7.0/0021-arm64-cix-Add-missing-Sky1-firmware-and-pinctrl-ACPI.patchpatch/kernel/archive/cix-7.0/0022-iommu-arm-smmu-v3-Add-ACPI-boot-active-bypass-STEs-f.patchpatch/kernel/archive/cix-7.0/0023-drm-add-sky1-drm-render-node-bridge-for-CIX-Sky1-SoC.patchpatch/kernel/archive/cix-7.0/0024-drm-sky1-switch-from-faux_device-to-platform_device.patchpatch/kernel/archive/cix-7.0/0025-mm-add-Mali-GPU-movable_ops-page-type-support.patchpatch/kernel/archive/cix-7.0/0026-pmdomain-arm-scmi_perf_domain-export-helpers-for-EM-.patchpatch/kernel/archive/cix-7.0/0027-drm-linlon-dp-add-diagnostic-knobs-for-AFBC-and-10bp.patchpatch/kernel/archive/cix-7.0/0028-phy-cix-usbdp-Default-to-DP-mode-for-static-outputs-.patchpatch/kernel/archive/cix-7.0/0029-drm-trilin-dptx-tear-down-DP-core-on-HPD-disconnect-.patchpatch/kernel/archive/cix-7.0/0030-drm-trilin-dptx-reset-active_stream_cnt-on-HPD-disco.patchpatch/kernel/archive/cix-7.0/0031-media-linlon-add-missing-VIDEOBUF2_DMA_SG-and-VIDEOB.patchpatch/kernel/archive/cix-7.0/0032-drm-trilin-dptx-add-CEC-over-DP-AUX-support.patchpatch/kernel/archive/cix-7.0/0033-drm-trilin-dptx-recover-link-on-HPD-bounce-with-degr.patchpatch/kernel/archive/cix-7.0/0034-Kconfig-fix-missing-dependencies-for-sky1-socinfo-an.patchpatch/kernel/archive/cix-7.0/0035-pwm-sky1-fix-NULL-dereference-in-suspend.patchpatch/kernel/archive/cix-7.0/0036-fix-allmodconfig-build-warnings-across-CIX-drivers.patchpatch/kernel/archive/cix-7.0/0037-drm-trilin-dptx-retry-AUX-on-cold-plug-timeout.patchpatch/kernel/archive/cix-7.0/0038-ASoC-cix-skip-HDMI-audio-links-for-missing-I2S-devic.patchpatch/kernel/archive/cix-7.0/0039-drm-trilin-dptx-add-ELD-reporting-and-audio-infofram.patchpatch/kernel/archive/cix-7.0/0040-drm-cix-dptx-add-IEC60958-channel-status-for-extende.patchpatch/kernel/archive/cix-7.0/0041-drm-cix-dptx-properly-disable-audio-hardware-on-shut.patchpatch/kernel/archive/cix-7.0/0042-drm-cix-dptx-suppress-spurious-ELD-warnings-at-boot.patchpatch/kernel/archive/cix-7.0/0043-cix-remove-pre-silicon-EMU-FPGA-dead-code-from-vendo.patch
Comment on lines
+2
to
+8
| BOARD_NAME="Cix-ACPI" | ||
| BOARD_VENDOR="cixtech" | ||
| BOARDFAMILY="cix-p1" | ||
| KERNEL_TARGET="edge,vendor" | ||
| FULL_DESKTOP="yes" | ||
| BOOT_LOGO="desktop" | ||
| BOARD_MAINTAINER="HeyMeco" |
Contributor
There was a problem hiding this comment.
Add recommended board metadata fields (KERNEL_TEST_TARGET, INTRODUCED).
Please add KERNEL_TEST_TARGET and INTRODUCED to this board file to align with board-config validation expectations and improve CI/test targeting metadata (Line 2 to Line 8).
Based on learnings: static check recommends KERNEL_TEST_TARGET as a comma-separated branch list and INTRODUCED as first-ship year.
🤖 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 `@config/boards/cix-acpi.conf` around lines 2 - 8, Add the missing metadata
fields KERNEL_TEST_TARGET and INTRODUCED to the board config: update the
top-level variables alongside BOARD_NAME/BOARD_VENDOR/KERNEL_TARGET by adding
KERNEL_TEST_TARGET as a comma-separated list of kernel branches used for CI/test
targeting (e.g., "edge,vendor" style) and INTRODUCED as the first-ship year
(four-digit year, e.g., "2024"); ensure names match exactly (KERNEL_TEST_TARGET,
INTRODUCED) and place them near KERNEL_TARGET for visibility so the board-config
validation sees them.
Comment on lines
+63
to
+65
| display_alert "Downloading Sky1 repository key" "" "info" | ||
| run_host_command_logged wget -qO "${SDCARD}"/usr/share/keyrings/sky1-linux.asc https://sky1-linux.github.io/apt/key.gpg | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific file and surrounding context
cat -n config/sources/families/cix-p1.conf | sed -n '55,75p'Repository: armbian/build
Length of output: 1189
🏁 Script executed:
# Search for fingerprint or key verification patterns in the codebase
rg -i "fingerprint|expected.*fpr|gpg.*with-colons" --type conf --type sh -A 2 -B 2 | head -100Repository: armbian/build
Length of output: 86
🏁 Script executed:
# Look for similar key fetch patterns in the codebase
rg "wget.*key\.gpg|wget.*\.asc|apt.*key.*add" -A 3 -B 1 | head -150Repository: armbian/build
Length of output: 1377
🏁 Script executed:
# Check if there's any key verification happening in other parts of the config
rg "gpg|fingerprint|verify.*key" config/sources/ -lRepository: armbian/build
Length of output: 129
🏁 Script executed:
# Check jethub.conf to see if similar pattern exists and how it's handled
cat -n config/sources/families/jethub.conf | grep -A 10 -B 5 "key\|fingerprint"Repository: armbian/build
Length of output: 5589
🏁 Script executed:
# Search for any key validation or verification helpers in the codebase
rg "verify.*key|validate.*key|check.*fingerprint" -iRepository: armbian/build
Length of output: 4798
🏁 Script executed:
# Look for where run_host_command_logged is defined and understand the context
rg "run_host_command_logged" --type sh -B 5 | head -50Repository: armbian/build
Length of output: 3475
🏁 Script executed:
# Check if there's any guidance or references about the Sky1 repository key
rg "sky1" -i config/ lib/ --no-headingRepository: armbian/build
Length of output: 5161
🏁 Script executed:
# Look for any comments or documentation about security practices for repository keys
rg -i "apt.*key|repository.*security|gpg.*fingerprint" config/ lib/ -B 2 -A 2 | head -80Repository: armbian/build
Length of output: 4554
Verify and pin the repository key fingerprint before trusting it.
The key is downloaded from https://sky1-linux.github.io/apt/key.gpg and immediately trusted (line 64) without verifying an expected fingerprint. This weakens supply-chain guarantees if key distribution is compromised.
🔧 Suggested hardening
- run_host_command_logged wget -qO "${SDCARD}"/usr/share/keyrings/sky1-linux.asc https://sky1-linux.github.io/apt/key.gpg
+ run_host_command_logged wget -qO "${SDCARD}"/usr/share/keyrings/sky1-linux.asc https://sky1-linux.github.io/apt/key.gpg
+ EXPECTED_FPR="REPLACE_WITH_OFFICIAL_FINGERPRINT"
+ ACTUAL_FPR="$(gpg --show-keys --with-colons "${SDCARD}"/usr/share/keyrings/sky1-linux.asc | awk -F: '$1=="fpr"{print $10; exit}')"
+ [[ "${ACTUAL_FPR}" == "${EXPECTED_FPR}" ]] || exit_with_error "Sky1 repo key fingerprint mismatch"🤖 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 `@config/sources/families/cix-p1.conf` around lines 63 - 65, The script
downloads and immediately trusts the Sky1 repository key via
run_host_command_logged to "${SDCARD}/usr/share/keyrings/sky1-linux.asc" without
verifying a known fingerprint; modify the flow so you first download the key to
a temp file, compute its fingerprint (e.g., via gpg --with-fingerprint or
similar), compare it against the expected/pinned fingerprint constant, and only
move/install the key to usr/share/keyrings/sky1-linux.asc if the fingerprint
matches; if it does not match, use display_alert and run_host_command_logged to
log and abort the operation (do not trust the key). Ensure the expected
fingerprint is stored as a clear variable/constant and reference
run_host_command_logged, display_alert, and the target path in your changes.
Comment on lines
+199
to
+205
| +static void sky1_hwspinlock_unlock(struct hwspinlock *lock) | ||
| +{ | ||
| + void __iomem *lock_addr = lock->priv; | ||
| + | ||
| + /* Release the lock by writing owner ID */ | ||
| + writel(SKY1_HWSPINLOCK_OWNER_ID, lock_addr); | ||
| +} |
Contributor
There was a problem hiding this comment.
unlock appears to re-write owner ID instead of releasing the hwspinlock.
At Line 204, sky1_hwspinlock_unlock() writes SKY1_HWSPINLOCK_OWNER_ID, which looks like it preserves ownership rather than releasing it. This can cause lock starvation/deadlock in multi-core use.
🤖 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/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patch`
around lines 199 - 205, sky1_hwspinlock_unlock currently writes
SKY1_HWSPINLOCK_OWNER_ID back to the lock (in sky1_hwspinlock_unlock using
lock->priv), which reclaims ownership instead of freeing it; change the
implementation to write the hardware's release value (e.g., 0 or a defined
SKY1_HWSPINLOCK_RELEASE_VALUE) to lock_addr to clear the owner and actually
release the hwspinlock, or add and use a named macro for the release value if
one is expected by the hardware.
Comment on lines
+568
to
+574
| + struct resource *res_dp = platform_get_resource(pdev, IORESOURCE_MEM, 0); | ||
| + cd->pdc_base = devm_ioremap_resource(&pdev->dev, res_dp); | ||
| + if (!cd->pdc_base) { | ||
| + pr_err("%pACPI: unable to map pdc registers\n", pdev); | ||
| + kfree(cd); | ||
| + return -ENOMEM; | ||
| + } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat patch/kernel/archive/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patch | head -n 600 | tail -n 50Repository: armbian/build
Length of output: 1219
🌐 Web query:
devm_ioremap_resource Linux kernel API return value ERR_PTR
💡 Result:
The Linux kernel API devm_ioremap_resource returns an ERR_PTR encoded error code on failure, not NULL [1][2][3][4]. Because it returns an ERR_PTR value, you must use the IS_ERR macro to check for failure and the PTR_ERR macro to retrieve the specific error code [1][2][4]. Checking for NULL (e.g., if (!ptr)) is incorrect and will fail to detect errors [4]. Correct usage pattern: void iomem *base; base = devm_ioremap_resource(dev, res); if (IS_ERR(base)) return PTR_ERR(base); // Proceed with base... This API is a managed function that performs validity checking on the resource, requests the memory region, and performs the ioremap operation [1][2][5]. If any of these steps fail, it returns an appropriate error pointer (such as -EINVAL, -EBUSY, or -ENOMEM) [1][6].
Citations:
- 1: http://rtsrv.cs.unc.edu/cgit/cgit.cgi/litmus-rt-imx6.git/plain/lib/devres.c
- 2: https://github.com/petrpavlu/linux/blob/913ee46905ab48e1cae0dbd18de098f572ffa388/lib/devres.c
- 3: https://github.com/torvalds/linux/blob/0466e7e693efe6647f23532529a9c41a1fa5f4ac/lib/devres.c
- 4: http://btrlinux.inria.fr/mailbox-fix-devm_ioremap_resource-error-detection-code/
- 5: https://subscription.packtpub.com/book/cloud_and_networking/9781801079518/4/ch04lvl1sec88/all-in-one-with-the-devm-ioremap-resource-api
- 6: https://android.googlesource.com/kernel/common/+/e8c75a30a23c/lib/devres.c
Fix error handling for devm_ioremap_resource(): use IS_ERR()/PTR_ERR() and remove manual iounmap().
devm_ioremap_resource() returns ERR_PTR on failure, not NULL, so the check on line 570 (if (!cd->pdc_base)) will fail to catch errors. Additionally, line 580 calls iounmap() on a devm-managed mapping, which is incorrect—the devm framework handles cleanup automatically. Replace the NULL check with IS_ERR(), use PTR_ERR() for the error code, and remove the iounmap() call.
Suggested fix
cd->pdc_base = devm_ioremap_resource(&pdev->dev, res_dp);
-if (!cd->pdc_base) {
+if (IS_ERR(cd->pdc_base)) {
pr_err("%pACPI: unable to map pdc registers\n", pdev);
kfree(cd);
- return -ENOMEM;
+ return PTR_ERR(cd->pdc_base);
}
...
if (!domain) {
- iounmap(cd->pdc_base);
kfree(cd);
return -ENOMEM;
}🤖 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/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patch`
around lines 568 - 574, The devm_ioremap_resource() call in the probe uses a
NULL check and manually iounmap() which is wrong; change the check on
cd->pdc_base (set by devm_ioremap_resource) to use IS_ERR(cd->pdc_base) and
return PTR_ERR(cd->pdc_base) on error, free cd as needed, and remove any manual
iounmap() calls so the devm-managed mapping is not explicitly unmapped; update
the error path around platform_get_resource()/devm_ioremap_resource() and
references to pdc_base and cd accordingly.
Comment on lines
+1669
to
+1671
| + pm_runtime_enable(dev); | ||
| + pm_runtime_get_sync(dev); | ||
| + |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patch"Repository: armbian/build
Length of output: 145
🏁 Script executed:
sed -n '1660,1680p' ./patch/kernel/archive/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patchRepository: armbian/build
Length of output: 554
🌐 Web query:
pm_runtime_get_sync return value error checking Linux kernel
💡 Result:
When checking the return value of pm_runtime_get_sync in the Linux kernel, you must only treat negative values as errors [1]. The function returns: - A negative error code (e.g., -EACCES, -EBUSY) if the operation fails [1]. - 0 if the device was successfully resumed [1]. - 1 if the device was already in the active state [1]. A common mistake is to check for any non-zero return value as an error, which incorrectly flags the successful "already active" (1) case as a failure [1]. Additionally, the official Linux kernel documentation notes that pm_runtime_get_sync does not automatically drop the device's usage counter if an error occurs [2][3]. Because of this, it is recommended to use pm_runtime_resume_and_get instead [2][3]. This helper function handles the counter correctly on failure, leading to cleaner and less error-prone code [2][3].
Citations:
- 1: https://patchwork.ozlabs.org/patch/316166/
- 2: https://kernel.org/doc/html/v5.15/power/runtime_pm.html
- 3: https://github.com/torvalds/linux/blob/master/Documentation/power/runtime_pm.rst
🏁 Script executed:
sed -n '1630,1690p' ./patch/kernel/archive/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patchRepository: armbian/build
Length of output: 1392
🌐 Web query:
pm_runtime_put_noidle kernel documentation correct cleanup
💡 Result:
The function pm_runtime_put_noidle is a standard helper in the Linux kernel's Runtime Power Management framework used to decrement a device's usage counter without triggering an immediate idle check [1][2][3]. Correct usage and cleanup: 1. Purpose: Unlike pm_runtime_put, which decrements the counter and automatically invokes pm_request_idle if the counter reaches zero, pm_runtime_put_noidle only performs the decrement [1][4]. It is typically used when you need to release a reference (decrement the counter) but do not want to initiate a runtime suspend operation at that specific moment [1][3]. 2. Common Cleanup Pattern: It is frequently used in error handling paths or during initialization/cleanup sequences where a previous pm_runtime_get* call must be balanced, but an immediate idle transition is either unnecessary, unsafe, or handled elsewhere [5]. 3. Safety: It is safe to execute from interrupt context [4][6]. 4. Modern Alternatives: For cleaner code and to avoid manual reference counting errors, the Linux kernel has introduced auto-cleanup macros (using the cleanup attribute) [5]. When working in modern kernel code, consider using these guard-based approaches instead of manual calls to pm_runtime_put_noidle to ensure the usage counter is always correctly balanced when the variable goes out of scope [5]. In summary, use pm_runtime_put_noidle when you must decrement the usage counter but want to suppress the automatic trigger of the idle/suspend path [1][3]. Always ensure it is paired correctly with a corresponding pm_runtime_get* call to prevent usage counter imbalances [4][3].
Citations:
- 1: https://kernel.org/doc/html/v5.15/power/runtime_pm.html
- 2: https://docs.kernel.org/power/runtime_pm.html
- 3: https://www.kernel.org/doc/Documentation/power/runtime_pm.rst
- 4: https://www.kernel.org/doc/Documentation/power/runtime_pm.txt
- 5: https://patchew.org/linux/12763087.O9o76ZdvQC@rafael.j.wysocki/2399578.ElGaqSPkdT@rafael.j.wysocki/
- 6: https://android.googlesource.com/kernel/msm/+/eed89d0f9d3383851cec634565a6414fae70fe91/Documentation/power/runtime_pm.txt
Check pm_runtime_get_sync() return value before continuing probe.
Line 1670 ignores errors from pm_runtime_get_sync(). If runtime PM resume fails, the probe continues with an inactive device, causing subsequent PM operations (like dev_pm_opp_get_opp_count() on line 1673) to fail or misbehave.
🔧 Suggested fix
pm_runtime_enable(dev);
- pm_runtime_get_sync(dev);
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(dev);
+ pm_runtime_disable(dev);
+ return ret;
+ }Alternatively, use the modern pm_runtime_resume_and_get() helper which handles the usage counter correctly on failure:
pm_runtime_enable(dev);
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0) {
+ pm_runtime_disable(dev);
+ return ret;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| + pm_runtime_enable(dev); | |
| + pm_runtime_get_sync(dev); | |
| + | |
| pm_runtime_enable(dev); | |
| ret = pm_runtime_get_sync(dev); | |
| if (ret < 0) { | |
| pm_runtime_put_noidle(dev); | |
| pm_runtime_disable(dev); | |
| return ret; | |
| } |
🤖 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/cix-7.0/0002-arm64-cix-Add-Sky1-SoC-infrastructure-drivers.patch`
around lines 1669 - 1671, The probe calls pm_runtime_enable(dev) followed by
pm_runtime_get_sync(dev) but ignores the return value; update the probe to check
the return of pm_runtime_get_sync (or replace both calls with
pm_runtime_resume_and_get(dev)) and handle errors: if the call returns a
negative error, undo runtime enable (pm_runtime_disable(dev)) and return the
error from the probe so we don't continue with an inactive device; reference
pm_runtime_get_sync, pm_runtime_enable, pm_runtime_disable, and
pm_runtime_resume_and_get when making the change.
Comment on lines
+1056
to
+1108
| +static void *sky1_of_get_addr_by_name(struct device_node *parent, char *name) | ||
| +{ | ||
| + struct device_node *node; | ||
| + int index; | ||
| + | ||
| + node = of_get_next_child(parent, NULL); | ||
| + if (node) { | ||
| + index = of_property_match_string(node, "reg-names", name); | ||
| + if (index >= 0) | ||
| + return of_iomap(node, index); | ||
| + } | ||
| + | ||
| + return NULL; | ||
| +} | ||
| + | ||
| +static void *sky1_acpi_get_addr_by_name(struct device *dev, char *name) | ||
| +{ | ||
| + struct fwnode_handle *fwnode; | ||
| + struct platform_device *pdev; | ||
| + struct device *device; | ||
| + struct resource *res; | ||
| + | ||
| + fwnode = device_get_next_child_node(dev, NULL); | ||
| + if (!fwnode) | ||
| + return NULL; | ||
| + | ||
| + device = bus_find_device_by_fwnode(&platform_bus_type, fwnode); | ||
| + if (!device) | ||
| + return NULL; | ||
| + | ||
| + pdev = to_platform_device(device); | ||
| + if (!pdev) | ||
| + return NULL; | ||
| + | ||
| + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name); | ||
| + if (!res) | ||
| + return NULL; | ||
| + | ||
| + return ioremap(res->start, resource_size(res)); | ||
| +} | ||
| + | ||
| +static void *sky1_get_addr_by_name(struct device *dev, char *name) | ||
| +{ | ||
| + if (!ACPI_COMPANION(dev)) | ||
| + return sky1_of_get_addr_by_name(dev->of_node, name); | ||
| + else | ||
| + return sky1_acpi_get_addr_by_name(dev, name); | ||
| +} | ||
| + | ||
| +static void sky1_put_addr(void __iomem *regs) | ||
| +{ | ||
| + if (regs) | ||
| + iounmap(regs); |
Contributor
There was a problem hiding this comment.
Return ERR_PTR() from the MMIO lookup helpers instead of NULL.
These helpers currently return bare NULL on lookup failures, and the probe path then does return PTR_ERR(data->xhci_base) / PTR_ERR(data->device_base). When either mapping is missing that evaluates to 0, so probe can succeed with NULL MMIO pointers. The ACPI helper also leaks the child fwnode and platform-device references on both success and failure paths.
Suggested direction
-static void *sky1_acpi_get_addr_by_name(struct device *dev, char *name)
+static void __iomem *sky1_acpi_get_addr_by_name(struct device *dev, char *name)
{
struct fwnode_handle *fwnode;
struct platform_device *pdev;
struct device *device;
struct resource *res;
+ void __iomem *addr;
fwnode = device_get_next_child_node(dev, NULL);
if (!fwnode)
- return NULL;
+ return ERR_PTR(-ENODEV);
device = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
+ fwnode_handle_put(fwnode);
if (!device)
- return NULL;
+ return ERR_PTR(-EPROBE_DEFER);
pdev = to_platform_device(device);
- if (!pdev)
- return NULL;
+ if (!pdev) {
+ put_device(device);
+ return ERR_PTR(-ENODEV);
+ }
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
- if (!res)
- return NULL;
+ if (!res) {
+ put_device(device);
+ return ERR_PTR(-ENODEV);
+ }
- return ioremap(res->start, resource_size(res));
+ addr = ioremap(res->start, resource_size(res));
+ put_device(device);
+ return addr ?: ERR_PTR(-ENOMEM);
}Also applies to: 1256-1262
🤖 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/cix-7.0/0010-usb-Add-CIX-Sky1-USB-support.patch` around
lines 1056 - 1108, The MMIO lookup helpers (sky1_of_get_addr_by_name,
sky1_acpi_get_addr_by_name, sky1_get_addr_by_name) must return error pointers
instead of NULL so callers can detect failures; change all "return NULL" failure
paths to return ERR_PTR(-ENODEV) (or an appropriate -errno for ioremap
failures), and if ioremap returns NULL map that to ERR_PTR(-ENOMEM) (or
propagate ioremap error if using an ERR_PTR return). In
sky1_acpi_get_addr_by_name release resources on all paths: call
fwnode_handle_put(fwnode) after device_get_next_child_node when done, and call
put_device(device) after bus_find_device_by_fwnode on both success and error
paths (before returning). Also update sky1_put_addr to skip iounmap for error
pointers (check IS_ERR(regs) and non-NULL before iounmap). Ensure
sky1_get_addr_by_name simply forwards the ERR_PTR returned by the two helpers.
Comment on lines
+1845
to
+1941
| +static void rts5453_update_port(struct rts5453 *rts) | ||
| +{ | ||
| + enum typec_orientation orientation; | ||
| + enum usb_role role; | ||
| + u32 dp_status = 0; | ||
| + unsigned long mode; | ||
| + | ||
| + /* Determine orientation */ | ||
| + if (!(rts->data_status[1] & RTS5453_USB3_CONN) && | ||
| + !(rts->data_status[2] & RTS5453_DP_CONN)) | ||
| + orientation = TYPEC_ORIENTATION_NONE; | ||
| + else if (rts->data_status[1] & RTS5453_ORIENTATION) | ||
| + orientation = TYPEC_ORIENTATION_REVERSE; | ||
| + else | ||
| + orientation = TYPEC_ORIENTATION_NORMAL; | ||
| + | ||
| + typec_switch_set(rts->sw, orientation); | ||
| + | ||
| + /* Determine USB role */ | ||
| + if (!(rts->data_status[1] & (RTS5453_USB3_CONN | RTS5453_USB2_CONN))) { | ||
| + role = USB_ROLE_NONE; | ||
| + } else if (rts->data_status[1] & RTS5453_DATA_ROLE) { | ||
| + role = USB_ROLE_DEVICE; | ||
| + } else { | ||
| + /* Host mode - check if USB3 or USB2 only */ | ||
| + if (!(rts->data_status[1] & RTS5453_USB3_CONN) && | ||
| + (rts->data_status[2] & RTS5453_DP_CONN)) | ||
| + role = USB_ROLE_HOST; | ||
| + else | ||
| + role = USB_ROLE_HOST; | ||
| + } | ||
| + | ||
| + /* Handle debug accessory mode */ | ||
| + if (rts->data_status[2] & RTS5453_DEBUG_ACCESSORY) { | ||
| + dev_warn(rts->dev, "Debug accessory mode not supported\n"); | ||
| + return; | ||
| + } | ||
| + | ||
| + /* Determine mux state and DP status */ | ||
| + if ((rts->data_status[1] & RTS5453_USB3_CONN) && | ||
| + (rts->data_status[2] & RTS5453_DP_CONN)) { | ||
| + /* USB + DP mode */ | ||
| + mode = TYPEC_DP_STATE_D; | ||
| + dp_status = DP_STATUS_ENABLED; | ||
| + if (rts->data_status[2] & RTS5453_HPD_STATE) | ||
| + dp_status |= DP_STATUS_HPD_STATE; | ||
| + if (rts->data_status[2] & RTS5453_HPD_IRQ) | ||
| + dp_status |= DP_STATUS_IRQ_HPD; | ||
| + | ||
| + rts->dp_data.status = dp_status; | ||
| + rts->dp_data.conf = 0; | ||
| + rts->mux_state.alt = rts->alt_dp; | ||
| + rts->mux_state.mode = mode; | ||
| + rts->mux_state.data = &rts->dp_data; | ||
| + } else if (rts->data_status[2] & RTS5453_DP_CONN) { | ||
| + /* DP only mode */ | ||
| + mode = TYPEC_DP_STATE_E; | ||
| + dp_status = DP_STATUS_ENABLED; | ||
| + if (rts->data_status[2] & RTS5453_HPD_STATE) | ||
| + dp_status |= DP_STATUS_HPD_STATE; | ||
| + if (rts->data_status[2] & RTS5453_HPD_IRQ) | ||
| + dp_status |= DP_STATUS_IRQ_HPD; | ||
| + | ||
| + rts->dp_data.status = dp_status; | ||
| + rts->dp_data.conf = 0; | ||
| + rts->mux_state.alt = rts->alt_dp; | ||
| + rts->mux_state.mode = mode; | ||
| + rts->mux_state.data = &rts->dp_data; | ||
| + } else if (rts->data_status[1] & RTS5453_USB3_CONN) { | ||
| + /* USB only mode */ | ||
| + mode = TYPEC_STATE_USB; | ||
| + rts->mux_state.alt = NULL; | ||
| + rts->mux_state.mode = mode; | ||
| + rts->mux_state.data = NULL; | ||
| + } else { | ||
| + /* Disconnected */ | ||
| + mode = TYPEC_STATE_SAFE; | ||
| + rts->mux_state.alt = NULL; | ||
| + rts->mux_state.mode = mode; | ||
| + rts->mux_state.data = NULL; | ||
| + } | ||
| + | ||
| + typec_mux_set(rts->mux, &rts->mux_state); | ||
| + | ||
| + /* Update power role */ | ||
| + if ((rts->data_status[3] & RTS5453_PWR_MASK) == RTS5453_PWR_SOURCE) | ||
| + typec_set_pwr_role(rts->port, TYPEC_SOURCE); | ||
| + else if ((rts->data_status[3] & RTS5453_PWR_MASK) == RTS5453_PWR_SINK) | ||
| + typec_set_pwr_role(rts->port, TYPEC_SINK); | ||
| + | ||
| + /* Update USB role if changed */ | ||
| + if (rts->role != role) { | ||
| + rts->role = role; | ||
| + usb_role_switch_set_role(rts->role_sw, role); | ||
| + typec_set_data_role(rts->port, | ||
| + role == USB_ROLE_DEVICE ? TYPEC_DEVICE : TYPEC_HOST); | ||
| + } |
Contributor
There was a problem hiding this comment.
Guard the optional Type-C helpers before using them.
Probe explicitly tolerates missing orientation-switch, mux, and USB-role-switch providers by setting those handles to NULL, but rts5453_update_port() unconditionally calls into all three. On any board that omits one of those optional endpoints, the first status update will dereference NULL.
Suggested fix
- typec_switch_set(rts->sw, orientation);
+ if (rts->sw)
+ typec_switch_set(rts->sw, orientation);
@@
- typec_mux_set(rts->mux, &rts->mux_state);
+ if (rts->mux)
+ typec_mux_set(rts->mux, &rts->mux_state);
@@
- usb_role_switch_set_role(rts->role_sw, role);
+ if (rts->role_sw)
+ usb_role_switch_set_role(rts->role_sw, role);🤖 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/cix-7.0/0010-usb-Add-CIX-Sky1-USB-support.patch` around
lines 1845 - 1941, rts5453_update_port() currently calls optional helpers
unconditionally and may dereference NULL when providers are absent; wrap calls
to typec_switch_set(rts->sw, ...) and typec_mux_set(rts->mux, &rts->mux_state)
in NULL checks (if (rts->sw) ...; if (rts->mux) ...), and similarly guard
usb_role_switch_set_role(rts->role_sw, role) and the typec_set_data_role(...)
call so they only run when rts->role_sw and rts->port are non-NULL respectively;
leave power-role updates as-is if rts->port is required/always present or guard
typec_set_pwr_role(rts->port, ...) with if (rts->port) if probe can leave it
NULL.
Comment on lines
+132
to
+226
| static int panthor_devfreq_target(struct device *dev, unsigned long *freq, | ||
| u32 flags) | ||
| { | ||
| + struct panthor_device *ptdev = dev_get_drvdata(dev); | ||
| + struct panthor_devfreq *pdevfreq = ptdev->devfreq; | ||
| struct dev_pm_opp *opp; | ||
| + unsigned long target_freq; | ||
| + unsigned long irqflags; | ||
| + ktime_t now; | ||
| int err; | ||
|
|
||
| opp = devfreq_recommended_opp(dev, freq, flags); | ||
| if (IS_ERR(opp)) | ||
| return PTR_ERR(opp); | ||
| + target_freq = *freq; | ||
| dev_pm_opp_put(opp); | ||
|
|
||
| - err = dev_pm_opp_set_rate(dev, *freq); | ||
| + /* Non-SCMI platforms: pass through unchanged */ | ||
| + if (!pdevfreq->opp_dev) | ||
| + return dev_pm_opp_set_rate(dev, target_freq); | ||
| + | ||
| + now = ktime_get(); | ||
| + spin_lock_irqsave(&pdevfreq->lock, irqflags); | ||
| + | ||
| + /* Suppress calls during error backoff */ | ||
| + if (ktime_before(now, pdevfreq->backoff_until)) { | ||
| + spin_unlock_irqrestore(&pdevfreq->lock, irqflags); | ||
| + return 0; | ||
| + } | ||
| + | ||
| + /* Skip if already at the requested frequency */ | ||
| + if (target_freq == pdevfreq->last_set_freq) { | ||
| + spin_unlock_irqrestore(&pdevfreq->lock, irqflags); | ||
| + return 0; | ||
| + } | ||
| + | ||
| + /* Rate limit: enforce minimum interval between SCMI calls */ | ||
| + if (ktime_us_delta(now, pdevfreq->last_set_time) < | ||
| + PANTHOR_SCMI_RATE_LIMIT_US) { | ||
| + spin_unlock_irqrestore(&pdevfreq->lock, irqflags); | ||
| + return 0; | ||
| + } | ||
| + | ||
| + spin_unlock_irqrestore(&pdevfreq->lock, irqflags); | ||
| + | ||
| + /* Actual SCMI frequency change (may sleep on mailbox) */ | ||
| + err = panthor_devfreq_scmi_set_freq(dev, target_freq); | ||
| + | ||
| + spin_lock_irqsave(&pdevfreq->lock, irqflags); | ||
| + | ||
| + if (!err) { | ||
| + pdevfreq->last_set_freq = target_freq; | ||
| + pdevfreq->last_set_time = now; | ||
| + pdevfreq->scmi_err_count = 0; | ||
| + pdevfreq->backoff_until = 0; | ||
| + } else { | ||
| + unsigned int errcnt = ++pdevfreq->scmi_err_count; | ||
| + unsigned long backoff_us; | ||
| + | ||
| + /* Exponential backoff: 500ms, 1s, 2s, capped at 4s */ | ||
| + backoff_us = PANTHOR_SCMI_BACKOFF_BASE_US << | ||
| + min(errcnt - 1, 3u); | ||
| + if (backoff_us > PANTHOR_SCMI_BACKOFF_MAX_US) | ||
| + backoff_us = PANTHOR_SCMI_BACKOFF_MAX_US; | ||
| + pdevfreq->backoff_until = ktime_add_us(now, backoff_us); | ||
| + | ||
| + drm_warn(&ptdev->base, | ||
| + "GPU DVFS: SCMI error %d (count=%u), backoff %lu ms\n", | ||
| + err, errcnt, backoff_us / 1000); | ||
| + | ||
| + /* After repeated errors, boost to safe mid-range frequency */ | ||
| + if (errcnt >= PANTHOR_SCMI_MAX_ERRORS && pdevfreq->safe_freq && | ||
| + pdevfreq->last_set_freq != pdevfreq->safe_freq) { | ||
| + spin_unlock_irqrestore(&pdevfreq->lock, irqflags); | ||
| + | ||
| + drm_info(&ptdev->base, | ||
| + "GPU DVFS: boosting to safe freq %lu MHz\n", | ||
| + pdevfreq->safe_freq / 1000000); | ||
| + err = panthor_devfreq_scmi_set_freq(dev, | ||
| + pdevfreq->safe_freq); | ||
| + | ||
| + spin_lock_irqsave(&pdevfreq->lock, irqflags); | ||
| + if (!err) { | ||
| + pdevfreq->last_set_freq = pdevfreq->safe_freq; | ||
| + pdevfreq->last_set_time = ktime_get(); | ||
| + } | ||
| + } | ||
| + } | ||
|
|
||
| - return err; | ||
| + spin_unlock_irqrestore(&pdevfreq->lock, irqflags); | ||
| + | ||
| + /* Always return 0 — errors cause devfreq to spam retries */ | ||
| + return 0; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that opp_dev is captured for SCMI DVFS but never used in the actual
# set-frequency call sites inside this patch.
rg -n -C2 'opp_dev|panthor_devfreq_scmi_set_freq\(' patch/kernel/archive/cix-7.0/0011-drm-panthor-Add-Sky1-GPU-support-and-ACPI.patchRepository: armbian/build
Length of output: 2063
Use pdevfreq->opp_dev instead of dev in both SCMI frequency-setting calls.
The panthor_devfreq_scmi_init() function records the perf-domain device in pdevfreq->opp_dev (line 331), but the two panthor_devfreq_scmi_set_freq() calls at lines 179 and 211 still pass the main GPU device. On device-tree multi-domain platforms that bypass the perf-domain/genpd device, this causes the driver to succeed without actually updating the SCMI performance state.
Suggested fix
- err = panthor_devfreq_scmi_set_freq(dev, target_freq);
+ err = panthor_devfreq_scmi_set_freq(pdevfreq->opp_dev, target_freq);
@@
- err = panthor_devfreq_scmi_set_freq(dev,
- pdevfreq->safe_freq);
+ err = panthor_devfreq_scmi_set_freq(pdevfreq->opp_dev,
+ pdevfreq->safe_freq);Also applies to: 254-338
🤖 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/cix-7.0/0011-drm-panthor-Add-Sky1-GPU-support-and-ACPI.patch`
around lines 132 - 226, The SCMI frequency calls in panthor_devfreq_target use
the main GPU device `dev` but must use the perf-domain device stored in
`pdevfreq->opp_dev`; update both calls to panthor_devfreq_scmi_set_freq(...) to
pass `pdevfreq->opp_dev` (the perf-domain/genpd device recorded by
panthor_devfreq_scmi_init) instead of `dev`, keeping the surrounding logic
(locks, backoff, safe_freq path) unchanged.
Comment on lines
+474
to
+482
| +static bool panthor_is_sky1(struct panthor_device *ptdev) | ||
| +{ | ||
| + struct device *dev = ptdev->base.dev; | ||
| + | ||
| + if (of_device_is_compatible(dev->of_node, "arm,mali-valhall")) | ||
| + return true; | ||
| + | ||
| + return acpi_dev_hid_uid_match(ACPI_COMPANION(dev), "CIXH5000", NULL); | ||
| +} |
Contributor
There was a problem hiding this comment.
Don't identify Sky1 from the generic arm,mali-valhall compatible.
That compatible is not CIX-specific, but this helper now drives Sky1-only flows like coherency selection, backup clocks, reset handling, resource layout, and power sequencing. Any DT platform using the generic Valhall fallback will be treated as Sky1.
🤖 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/cix-7.0/0011-drm-panthor-Add-Sky1-GPU-support-and-ACPI.patch`
around lines 474 - 482, The helper panthor_is_sky1 must not treat the generic
"arm,mali-valhall" DT compatible as Sky1; remove the
of_device_is_compatible(dev->of_node, "arm,mali-valhall") branch and instead
detect Sky1 only via ACPI (acpi_dev_hid_uid_match(ACPI_COMPANION(dev),
"CIXH5000", NULL)) and a Sky1-specific DT compatible (e.g., check
of_device_is_compatible(dev->of_node, "cix,sky1") or similar vendor-specific
string), so update the panthor_is_sky1 function to first test ACPI then a
Sky1-specific OF compatible and drop the generic Valhall check.
Comment on lines
+1203
to
+1216
| + /* On CIX Sky1 SoC some shader cores can be disabled via RCSU */ | ||
| + if (of_device_is_compatible(ptdev->base.dev->of_node, "arm,mali-valhall") && | ||
| + ptdev->sky1_rcsu_reg) { | ||
| + u32 sky1_harvesting_reg_val = readl(ptdev->sky1_rcsu_reg + 0x304); | ||
| + u64 sky1_harvesting_core_mask = ~((sky1_harvesting_reg_val & 0xFFFFFF0) >> 4) & 0x550555; | ||
| + | ||
| + ptdev->gpu_info.shader_present &= sky1_harvesting_core_mask; | ||
| + | ||
| + drm_info(&ptdev->base, | ||
| + "sky1 harvesting: reg=0x%x core_mask=0x%llx", | ||
| + sky1_harvesting_reg_val, sky1_harvesting_core_mask); | ||
| + } | ||
| + | ||
| + return 0; |
Contributor
There was a problem hiding this comment.
Apply the Sky1 harvesting mask on ACPI too.
This block only runs for DT-backed devices, but panthor_device_init() already maps sky1_rcsu_reg for ACPI Sky1 as well. On ACPI systems the harvesting mask is skipped, so fused-off shader cores stay visible to the 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/cix-7.0/0011-drm-panthor-Add-Sky1-GPU-support-and-ACPI.patch`
around lines 1203 - 1216, The Sky1 harvesting mask is only applied when
of_device_is_compatible(...) is true, which skips ACPI-backed devices even
though panthor_device_init() maps ptdev->sky1_rcsu_reg for ACPI; change the
condition so the mask is applied whenever ptdev->sky1_rcsu_reg is present (e.g.
replace the combined check with a simple if (ptdev->sky1_rcsu_reg) or
equivalent) and leave the masking logic that reads sky1_harvesting_reg_val and
updates ptdev->gpu_info.shader_present intact so fused-off shader cores are
hidden on ACPI systems as well.
Collaborator
|
Note: There is an official maintained set of patches from cix / amazingfate at https://github.com/cixtech/cix-linux-main. I wanted to revamp my PR at some point based on it as it is even more streamlined. It also makes sense to look at the new canonical x cix collab which is validated on all the devices (Minisforum, Radxa, Opi,…) Sky1-Linux seems to be more of a proof of concept than long term maintained at this point. I think it might already be deprecated given the work that is coming from cix directly |
Collaborator
did you use the new Minisforum bios and have set the scmi setting accordingly? The original bios was 2025Q1, starting with 2025Q3 it should work with the official patches and ms-r1 repo has sources for a new bios too. |
Member
Author
The one which claimed to reduce idle power draw and broke pcie? Or is there a more recent one? |
Collaborator
new as in last week: https://github.com/minisforum-docs/MS-R1-Docs/blob/main/PlayBook/MS-R1-ARMSystemReady.md It’s supposed to fix all the issues from the first ones and bring it up to speed to the other boards |
Member
Author
|
Wasn't aware of that. Well if we use it as it is now it will work with the initial bios. |
Collaborator
|
The new one is system ready approved and seems to be the better way moving forward. It supports current armbian uefi images better and will have a "vendor" kernel we can use for the whole family via ACPI (which was my original intention all along) |
Collaborator
|
Sorry to bring the "bad" news regarding your work for this PR but at least we can maintain all at once finally |
Member
Author
|
I agree doing it the proper way is better. So no worries |
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
2 participants
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
Only
edgeon Trixie as tested. Other combinations orvendorwere not.How Has This Been Tested?
* I noticed hw acceleration was not available immediately after installation of the DE. Not sure if the logout/login fixed it or if installing
libgl1-mesa-drifrom trixie-backports fixed it. Either way both did the trick.Perhaps this works oob when building a desktop image directly? Did not test
Checklist: