Re: [PATCH v8 07/12] media: iris: Add power sequence for glymur

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Direct writel() to shared CPU_CS_AHB_BRIDGE_SYNC_RESET register overwrites the adjacent core's state, asserting hardware reset prematurely.
--

commit 0a8584c1c055cb2958fb73836b1a4ff132552257
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Add power sequence for glymur

This commit adds power sequencing for the glymur platform, which features
two video codec cores (vcodec0 and vcodec1). It ensures both cores vote for
the clock and power rails based on the combined workload of both cores,
since they share the same power rails.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 7cda946015558..afd2a0e2a3a54 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
[ ... ]
> @@ -221,6 +221,98 @@ static void iris_vpu35_power_off_hw(struct iris_core *core)
>  	iris_vpu33_power_off_hardware(core);
>  }
>  
> +static void iris_vpu36_power_off_vcodec(struct iris_core *core, u32 core_id)
> +{
> +	u32 bridge_hw_reset[] = {CORE_BRIDGE_HW_RESET_DISABLE, VCODEC1_BRIDGE_HW_RESET_DISABLE};
> +	u32 power_down_prep[] = {REQ_POWER_DOWN_PREP, REQ_VCODEC1_POWER_DOWN_PREP};
> +	u32 bridge_sw_reset[] = {CORE_BRIDGE_SW_RESET, VCODEC1_BRIDGE_SW_RESET};
> +	u32 idle_status[] = {VCODEC_SS_IDLE_STATUSN, VCODEC1_SS_IDLE_STATUSN};
> +	u32 lpi_status[] = {NOC_LPI_STATUS_DONE, NOC_LPI_VCODEC1_STATUS_DONE};
> +	u32 power_status[] = {VCODEC0_POWER_STATUS, VCODEC1_POWER_STATUS};
> +	u32 value, i;
> +	int ret;
[ ... ]
> +	writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +
> +	writel(bridge_sw_reset[core_id] | bridge_hw_reset[core_id],
> +	       core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(bridge_hw_reset[core_id], core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);

[Severity: High]
Will this writel(0x0) overwrite the adjacent core's state and cause an AHB
bus stall?

CPU_CS_AHB_BRIDGE_SYNC_RESET appears to be a shared register containing
configuration bits for both vcodec0 and vcodec1. By using a direct writel()
instead of a read-modify-write, writing 0x0 unconditionally clears the
HW reset disable bits for both cores (e.g., VCODEC1_BRIDGE_HW_RESET_DISABLE).

When iris_vpu36_power_off_hw() shuts down core 0, this immediately forces
core 1 into a hardware reset while it is still fully powered on. During the
next loop iteration, iris_vpu36_power_off_vcodec(core, 1) attempts to read
core 1's VCODEC1_SS_IDLE_STATUSN. Because the AHB bridge to VCODEC1 was
already thrown into reset, won't this read crash the kernel due to a
synchronous external abort?

Should this sequence preserve the adjacent core's active state bits when
writing to this shared register?

> +
> +disable_power:
> +	iris_genpd_set_hwmode(&core->vcodec[core_id], false);
> +	iris_disable_power_domain_and_clocks(core, &core->vcodec[core_id]);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-glymur-v8-0-1c79b9d51fc0@oss.qualcomm.com?part=7