Re: [PATCH v10 09/14] media: iris: Handle CPU_CS_SCIACMDARG3 register write via program bootup registers hook

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added `iris_vpu36_ops` structure, which contains the fix for the Glymur platform, is never assigned to any platform data.
- [Low] Bootup register programming is improperly placed inside a memory map setup function, creating an awkward double-write pattern.
--

--- Patch [9]: [PATCH v10 09/14] media: iris: Handle CPU_CS_SCIACMDARG3 register write via program bootup registers hook ---
commit 5b1dcdb0f86534dd18f11d0f09a2cbbcb73f7014
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Handle CPU_CS_SCIACMDARG3 register write via program bootup registers hook

This commit adds a platform-specific hook to handle the CPU_CS_SCIACMDARG3
register during boot. It aims to fix the VM count on the Glymur platform by
writing 0x0 instead of the default 0x1.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 1b08461ee4d70..5212f7d4e606c 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
[ ... ]
> @@ -361,6 +366,7 @@ const struct vpu_ops iris_vpu36_ops = {
>  	.power_on_hw = iris_vpu36_power_on_hw,
>  	.power_off_controller = iris_vpu35_vpu4x_power_off_controller,
>  	.power_on_controller = iris_vpu35_vpu4x_power_on_controller,
> +	.program_bootup_registers = iris_vpu36_program_bootup_registers,
>  	.calc_freq = iris_vpu3x_vpu4x_calculate_frequency,
>  	.set_hwmode = iris_vpu36_set_hwmode,
>  };

[Severity: High]
Is iris_vpu36_ops assigned to any platform data?

Looking through the driver, it appears this new vpu_ops structure is
never referenced, nor is the Glymur platform added to the driver's
of_match_table in this patch series.

Does this mean the promised firmware boot fix for the Glymur platform
is currently unreachable dead code?

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index e690c566c8751..72eb8f6240b70 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
[ ... ]
> @@ -67,6 +66,8 @@ static void iris_vpu_setup_ucregion_memory_map(struct iris_core *core)
>  		writel(value, core->reg_base + SFR_ADDR);
>  	}
>  
> +	writel(0x1, core->reg_base + CPU_CS_SCIACMDARG3);
> +
>  	if (vpu_ops->program_bootup_registers)
>  		vpu_ops->program_bootup_registers(core);
>  }

[Severity: Low]
Does this create an unnecessary double-write pattern?

By placing this inside iris_vpu_setup_ucregion_memory_map(), the code first
writes a default 0x1 to CPU_CS_SCIACMDARG3 (which is a boot argument
unrelated to memory mapping), and then immediately calls the hook.

On platforms using the workaround, this means writing 0x1 and then
immediately overwriting it with 0x0 before the firmware has started.

Could the default write and the hook be moved out of the memory map setup
and organized to avoid writing to this register twice?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-glymur-v10-0-de451559e88b@oss.qualcomm.com?part=9