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

Vikash Garodia
2024-09-06T11:27:56Z

Hi Bryan,

On 9/5/2024 6:04 PM, Bryan O'Donoghue wrote:
> 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)
Yes, we should expand the bits being set to provide better context to it. There
are few other places as well, primarily the power on/off sequences, will update it.

>
> 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

Regards,
Vikash