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

On 7/26/2026 11:33 AM, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

iris_vpu36_ops is assigned to glymur_data as .vpu_ops = &iris_vpu36_ops in
patch-10. The Glymur platform data and of_match_table entry are added there,
so this is not 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?

Platforms that require 0x0 simply override it via program_bootup_registers.
The overwrite is harmless since firmware hasn't started yet at this point.