Re: [PATCH 3/4] media: platform: qcom/iris: add support for vpu33

[Philipp Zabel]

On Di, 2025-02-25 at 10:05 +0100, Neil Armstrong wrote:
> The IRIS acceleration found in the SM8650 platforms uses the vpu33
> hardware version, and requires a slighly different reset and power off
> sequences in order to properly get out of runtime suspend.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> ---
>  drivers/media/platform/qcom/iris/Makefile          |   1 +
>  drivers/media/platform/qcom/iris/iris_vpu33.c      | 315 +++++++++++++++++++++
>  drivers/media/platform/qcom/iris/iris_vpu_common.h |   1 +
>  3 files changed, 317 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/Makefile b/drivers/media/platform/qcom/iris/Makefile
> index 35390534534e93f4617c1036a05ca0921567ba1d..6b64c9988505afd9707c704449d60bb53209229f 100644
> --- a/drivers/media/platform/qcom/iris/Makefile
> +++ b/drivers/media/platform/qcom/iris/Makefile
> @@ -21,6 +21,7 @@ qcom-iris-objs += \
>               iris_vdec.o \
>               iris_vpu2.o \
>               iris_vpu3.o \
> +             iris_vpu33.o \
>               iris_vpu_buffer.o \
>               iris_vpu_common.o \
>  
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu33.c b/drivers/media/platform/qcom/iris/iris_vpu33.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..128a050f206f99ec0d43b97ff995fa50d5684150
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_vpu33.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#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_TZ_BASE_OFFS			0x000C0000
> +#define AON_BASE_OFFS				0x000E0000
> +#define AON_MVP_NOC_RESET			0x0001F000
> +
> +#define WRAPPER_DEBUG_BRIDGE_LPI_CONTROL	(WRAPPER_BASE_OFFS + 0x54)
> +#define WRAPPER_DEBUG_BRIDGE_LPI_STATUS		(WRAPPER_BASE_OFFS + 0x58)
> +#define WRAPPER_IRIS_CPU_NOC_LPI_CONTROL	(WRAPPER_BASE_OFFS + 0x5C)
> +#define REQ_POWER_DOWN_PREP			BIT(0)
> +#define WRAPPER_IRIS_CPU_NOC_LPI_STATUS		(WRAPPER_BASE_OFFS + 0x60)
> +#define WRAPPER_CORE_CLOCK_CONFIG		(WRAPPER_BASE_OFFS + 0x88)
> +#define CORE_CLK_RUN				0x0
> +
> +#define WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG		(WRAPPER_TZ_BASE_OFFS + 0x14)
> +#define CTL_AXI_CLK_HALT			BIT(0)
> +#define CTL_CLK_HALT				BIT(1)
> +
> +#define WRAPPER_TZ_QNS4PDXFIFO_RESET		(WRAPPER_TZ_BASE_OFFS + 0x18)
> +#define RESET_HIGH				BIT(0)
> +
> +#define CPU_CS_AHB_BRIDGE_SYNC_RESET		(CPU_CS_BASE_OFFS + 0x160)
> +#define CORE_BRIDGE_SW_RESET			BIT(0)
> +#define CORE_BRIDGE_HW_RESET_DISABLE		BIT(1)
> +
> +#define CPU_CS_X2RPMH				(CPU_CS_BASE_OFFS + 0x168)
> +#define MSK_SIGNAL_FROM_TENSILICA		BIT(0)
> +#define MSK_CORE_POWER_ON			BIT(1)
> +
> +#define AON_WRAPPER_MVP_NOC_RESET_REQ		(AON_MVP_NOC_RESET + 0x000)
> +#define VIDEO_NOC_RESET_REQ			(BIT(0) | BIT(1))
> +
> +#define AON_WRAPPER_MVP_NOC_RESET_ACK		(AON_MVP_NOC_RESET + 0x004)
> +
> +#define VCODEC_SS_IDLE_STATUSN			(VCODEC_BASE_OFFS + 0x70)
> +
> +#define AON_WRAPPER_MVP_NOC_LPI_CONTROL		(AON_BASE_OFFS)
> +#define AON_WRAPPER_MVP_NOC_LPI_STATUS		(AON_BASE_OFFS + 0x4)
> +
> +#define AON_WRAPPER_MVP_NOC_CORE_SW_RESET	(AON_BASE_OFFS + 0x18)
> +#define SW_RESET				BIT(0)
> +#define AON_WRAPPER_MVP_NOC_CORE_CLK_CONTROL	(AON_BASE_OFFS + 0x20)
> +#define NOC_HALT				BIT(0)
> +#define AON_WRAPPER_SPARE			(AON_BASE_OFFS + 0x28)
> +
> +#define VCODEC_DMA_SPARE_3 0x87B8
> +
> +static int reset_control_bulk_assert_id(int num_rstcs,
> +					struct reset_control_bulk_data *rstcs,
> +					char *id)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_rstcs; ++i) {
> +		if (!strcmp(rstcs[i].id, id))
> +			return reset_control_assert(rstcs[i].rstc);
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int reset_control_bulk_deassert_id(int num_rstcs,
> +					  struct reset_control_bulk_data *rstcs,
> +					  char *id)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_rstcs; ++i) {
> +		if (!strcmp(rstcs[i].id, id))
> +			return reset_control_deassert(rstcs[i].rstc);
> +	}
> +
> +	return -ENODEV;
> +}

Please adapt the abstractions instead of working around them. If the
driver isn't suited for a single reset_control_bulk_data in iris_core,
split it into multiple groups, or store the resets individually.

At the very least, this could use constant indices instead of linear
search with string compares.

regards
Philipp