Re: [PATCH 7/8] media: iris: Introduce vpu ops for vpu4 with necessary hooks

Konrad Dybcio
2025-09-25T09:18:49Z

On 9/25/25 1:14 AM, Vikash Garodia wrote:
> Add power sequence for vpu4 by reusing from previous generation wherever
> possible. Hook up vpu4 op with vpu4 specific implemtation or resue from
> earlier generation wherever feasible, like clock calculation in this
> case.
>
> Co-developed-by: Vishnu Reddy <quic_bvisredd@quicinc.com>
> Signed-off-by: Vishnu Reddy <quic_bvisredd@quicinc.com>
> Signed-off-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
> ---

[...]

> +#include <linux/iopoll.h>
> +#include <linux/reset.h>
> +#include "iris_instance.h"
> +#include "iris_vpu_common.h"
> +#include "iris_vpu_register_defines.h"
> +
> +#define WRAPPER_EFUSE_MONITOR (WRAPPER_BASE_OFFS + 0x08)
> +#define AON_WRAPPER_MVP_NOC_RESET_SYNCRST (AON_MVP_NOC_RESET + 0x08)
> +#define CPU_CS_APV_BRIDGE_SYNC_RESET (CPU_BASE_OFFS + 0x174)
> +#define DISABLE_VIDEO_APV_BIT BIT(27)
> +#define DISABLE_VIDEO_VPP1_BIT BIT(28)
> +#define DISABLE_VIDEO_VPP0_BIT BIT(29)
> +#define CORE_CLK_HALT BIT(0)
> +#define APV_CLK_HALT BIT(1)
> +#define CORE_PWR_ON BIT(1)
> +
> +static int iris_vpu4x_genpd_set_hwmode(struct iris_core *core, bool hw_mode)
> +{
> + u32 value = readl(core->reg_base + WRAPPER_EFUSE_MONITOR);

I think this could use some explanations.

I'll go ahead and assume that the eFuse tells us that parts of the
IP are disables (hopefully not all three at once.. we shouldn't
advertise the v4l2 device then, probably)

You read back the fuse register a lot, even though I presume it's not
supposed to change at runtime. How about we add:

bool vpp0_fused_off
bool vpp1_fused_off
bool apv_fused_off

instead?

[...]

> + if (!(value & DISABLE_VIDEO_VPP0_BIT)) {
> + ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs
> + [IRIS_VPP0_HW_POWER_DOMAIN]);

Maybe the iris_en/disable_foo functions could get a wrapper like:

int iris_enable_power_domains_if(core, pd_devs[IRIS_VPP0_HW_POWER_DOMAIN],
!foo->vpp0_fused_off)

I'm not super sure about it, but that's something to consider

[...]

> + readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN, value,
> + value & 0x7103, 2000, 20000);

That's a nice magic number.. but what does it mean?

[...]

> + writel(0x070103, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
> + readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
> + value, value == 0x070103, 200, 2000);

That's a slightly different magic number, but it's oddly similar to
the one above

Konrad