Re: [PATCH v3 08/29] media: iris: implement boot sequence of the firmware

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 27/08/2024 11:05, Dikshita Agarwal via B4 Relay wrote:
> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
> 
> Set memory region to firmware and implement boot sequence.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>   drivers/media/platform/qcom/iris/Makefile          |  1 +
>   drivers/media/platform/qcom/iris/iris_core.c       |  7 ++
>   .../platform/qcom/iris/iris_platform_common.h      |  1 +
>   .../platform/qcom/iris/iris_platform_sm8550.c      |  3 +
>   drivers/media/platform/qcom/iris/iris_vpu_common.c | 87 ++++++++++++++++++++++
>   drivers/media/platform/qcom/iris/iris_vpu_common.h | 13 ++++
>   6 files changed, 112 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/Makefile b/drivers/media/platform/qcom/iris/Makefile
> index ddd4c994a0b9..95f4e92fe085 100644
> --- a/drivers/media/platform/qcom/iris/Makefile
> +++ b/drivers/media/platform/qcom/iris/Makefile
> @@ -8,5 +8,6 @@ iris-objs += iris_core.o \
>                iris_probe.o \
>                iris_resources.o \
>                iris_vidc.o \
> +             iris_vpu_common.o \
>   
>   obj-$(CONFIG_VIDEO_QCOM_IRIS) += iris.o
> diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
> index 8c7d53c57086..5ad66ac113ae 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.c
> +++ b/drivers/media/platform/qcom/iris/iris_core.c
> @@ -6,6 +6,7 @@
>   #include "iris_core.h"
>   #include "iris_firmware.h"
>   #include "iris_state.h"
> +#include "iris_vpu_common.h"
>   
>   void iris_core_deinit(struct iris_core *core)
>   {
> @@ -39,10 +40,16 @@ int iris_core_init(struct iris_core *core)
>   	if (ret)
>   		goto error_queue_deinit;
>   
> +	ret = iris_vpu_boot_firmware(core);
> +	if (ret)
> +		goto error_unload_fw;
> +
>   	mutex_unlock(&core->lock);
>   
>   	return 0;
>   
> +error_unload_fw:
> +	iris_fw_unload(core);
>   error_queue_deinit:
>   	iris_hfi_queues_deinit(core);
>   error:
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> index 9c919367f9d7..47fdebd8135c 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> @@ -45,6 +45,7 @@ struct iris_platform_data {
>   	const char *fwname;
>   	u32 pas_id;
>   	struct tz_cp_config *tz_cp_config_data;
> +	u32 core_arch;
>   };
>   
>   #endif
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> index 1bb34c3e6e18..a559e095fefc 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> @@ -8,6 +8,8 @@
>   #include "iris_platform_common.h"
>   #include "iris_resources.h"
>   
> +#define VIDEO_ARCH_LX 1
> +
>   static const struct icc_info sm8550_icc_table[] = {
>   	{ "cpu-cfg",    1000, 1000     },
>   	{ "video-mem",  1000, 15000000 },
> @@ -48,4 +50,5 @@ struct iris_platform_data sm8550_data = {
>   	.fwname = "qcom/vpu/vpu30_p4.mbn",
>   	.pas_id = IRIS_PAS_ID,
>   	.tz_cp_config_data = &tz_cp_config_sm8550,
> +	.core_arch = VIDEO_ARCH_LX,
>   };
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> new file mode 100644
> index 000000000000..df87b1b719a9
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/iopoll.h>
> +
> +#include "iris_core.h"
> +#include "iris_vpu_common.h"
> +
> +#define CPU_BASE_OFFS				0x000A0000
> +
> +#define CPU_CS_BASE_OFFS			(CPU_BASE_OFFS)
> +
> +#define CTRL_INIT				(CPU_CS_BASE_OFFS + 0x48)
> +#define CTRL_STATUS				(CPU_CS_BASE_OFFS + 0x4C)
> +
> +#define CTRL_ERROR_STATUS__M			0xfe
> +
> +#define QTBL_INFO				(CPU_CS_BASE_OFFS + 0x50)
> +#define QTBL_ADDR				(CPU_CS_BASE_OFFS + 0x54)
> +#define CPU_CS_SCIACMDARG3			(CPU_CS_BASE_OFFS + 0x58)
> +#define SFR_ADDR				(CPU_CS_BASE_OFFS + 0x5C)
> +#define UC_REGION_ADDR				(CPU_CS_BASE_OFFS + 0x64)
> +#define UC_REGION_SIZE				(CPU_CS_BASE_OFFS + 0x68)
> +
> +#define CPU_CS_H2XSOFTINTEN			(CPU_CS_BASE_OFFS + 0x148)
> +#define CPU_CS_X2RPMH				(CPU_CS_BASE_OFFS + 0x168)
> +
> +static void iris_vpu_setup_ucregion_memory_map(struct iris_core *core)
> +{
> +	u32 queue_size, value;
> +
> +	/* Iris hardware requires 4K queue alignment */
> +	queue_size = ALIGN(sizeof(struct iris_hfi_queue_table_header) +
> +		(IFACEQ_QUEUE_SIZE * IFACEQ_NUMQ), SZ_4K);
> +
> +	value = (u32)core->iface_q_table_daddr;
> +	writel(value, core->reg_base + UC_REGION_ADDR);
> +
> +	/* Iris hardware requires 1M queue alignment */
> +	value = ALIGN(SFR_SIZE + queue_size, SZ_1M);
> +	writel(value, core->reg_base + UC_REGION_SIZE);
> +
> +	value = (u32)core->iface_q_table_daddr;
> +	writel(value, core->reg_base + QTBL_ADDR);
> +
> +	writel(0x01, core->reg_base + QTBL_INFO);

A general comment I have is instead of writing hard-coded values to 
registers we should define at a minimum the bit-fields we use if not the 
entire set of bits-fields for the register.

The only exception to this is when we don't know what those values are - 
for example receiving a magic write sequence for a camera sensor.

In this case though we have full access to enumerate the register 
bit-fields.

Without looking at the register descriptions I guess this bit is an 
enable or a startup bit =>

#define QTBL_INFO_EN BIT(0)

I'll not go through this series reiterating this comment but, it 
certainly applies to any bit-field/register in the same => please define 
at least the bits used if not the full set of bits for register writes 
instead of using magic numbers.

---
bod