Add support for SM8750 Iris codec with major differences against
previous generation SM8650:

1. New clocks and new resets, thus new power up and power down
   sequences,

2. New WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0 register programmed
   during boot-up

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../platform/qcom/iris/iris_platform_common.h      |   6 +-
 .../media/platform/qcom/iris/iris_platform_gen2.c  |  68 +++++++
 .../platform/qcom/iris/iris_platform_sm8750.h      |  22 +++
 drivers/media/platform/qcom/iris/iris_probe.c      |   4 +
 drivers/media/platform/qcom/iris/iris_vpu3x.c      | 203 +++++++++++++++++++++
 drivers/media/platform/qcom/iris/iris_vpu_common.c |   4 +
 drivers/media/platform/qcom/iris/iris_vpu_common.h |   2 +
 7 files changed, 308 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
index adafdce8a856f9c661aabc5ca28f0faceaa93551..fd5a6e69e01cfd00253f4ffb282d40112b93073b 100644
--- a/drivers/media/platform/qcom/iris/iris_platform_common.h
+++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
@@ -38,11 +38,15 @@ extern struct iris_platform_data qcs8300_data;
 extern struct iris_platform_data sm8250_data;
 extern struct iris_platform_data sm8550_data;
 extern struct iris_platform_data sm8650_data;
+extern struct iris_platform_data sm8750_data;
 
 enum platform_clk_type {
-	IRIS_AXI_CLK,
+	IRIS_AXI_CLK, /* AXI0 in case of platforms with multiple AXI clocks */
 	IRIS_CTRL_CLK,
 	IRIS_HW_CLK,
+	IRIS_AXI1_CLK,
+	IRIS_CTRL_FREERUN_CLK,
+	IRIS_HW_FREERUN_CLK,
 };
 
 struct platform_clk_data {
diff --git a/drivers/media/platform/qcom/iris/iris_platform_gen2.c b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
index d3026b2bcb708c7ec31f134f628df7e57b54af4f..795efe2226228c4d7155ce18ff42ba9cb74b4af2 100644
--- a/drivers/media/platform/qcom/iris/iris_platform_gen2.c
+++ b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2025 Linaro Ltd
  */
 
 #include "iris_core.h"
@@ -12,6 +13,7 @@
 
 #include "iris_platform_qcs8300.h"
 #include "iris_platform_sm8650.h"
+#include "iris_platform_sm8750.h"
 
 #define VIDEO_ARCH_LX 1
 
@@ -463,6 +465,72 @@ struct iris_platform_data sm8650_data = {
 	.dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl),
 };
 
+struct iris_platform_data sm8750_data = {
+	.get_instance = iris_hfi_gen2_get_instance,
+	.init_hfi_command_ops = iris_hfi_gen2_command_ops_init,
+	.init_hfi_response_ops = iris_hfi_gen2_response_ops_init,
+	.vpu_ops = &iris_vpu35_ops,
+	.set_preset_registers = iris_set_sm8550_preset_registers,
+	.icc_tbl = sm8550_icc_table,
+	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
+	.clk_rst_tbl = sm8750_clk_reset_table,
+	.clk_rst_tbl_size = ARRAY_SIZE(sm8750_clk_reset_table),
+	.bw_tbl_dec = sm8550_bw_table_dec,
+	.bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec),
+	.pmdomain_tbl = sm8550_pmdomain_table,
+	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
+	.opp_pd_tbl = sm8550_opp_pd_table,
+	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
+	.clk_tbl = sm8750_clk_table,
+	.clk_tbl_size = ARRAY_SIZE(sm8750_clk_table),
+	/* Upper bound of DMA address range */
+	.dma_mask = 0xe0000000 - 1,
+	.fwname = "qcom/vpu/vpu35_4v.mbn",
+	.pas_id = IRIS_PAS_ID,
+	.inst_caps = &platform_inst_cap_sm8550,
+	.inst_fw_caps = inst_fw_cap_sm8550,
+	.inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550),
+	.tz_cp_config_data = &tz_cp_config_sm8550,
+	.core_arch = VIDEO_ARCH_LX,
+	.hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE,
+	.ubwc_config = &ubwc_config_sm8550,
+	.num_vpp_pipe = 4,
+	.max_session_count = 16,
+	.max_core_mbpf = NUM_MBS_8K * 2,
+	.input_config_params_default =
+		sm8550_vdec_input_config_params_default,
+	.input_config_params_default_size =
+		ARRAY_SIZE(sm8550_vdec_input_config_params_default),
+	.input_config_params_hevc =
+		sm8550_vdec_input_config_param_hevc,
+	.input_config_params_hevc_size =
+		ARRAY_SIZE(sm8550_vdec_input_config_param_hevc),
+	.input_config_params_vp9 =
+		sm8550_vdec_input_config_param_vp9,
+	.input_config_params_vp9_size =
+		ARRAY_SIZE(sm8550_vdec_input_config_param_vp9),
+	.output_config_params =
+		sm8550_vdec_output_config_params,
+	.output_config_params_size =
+		ARRAY_SIZE(sm8550_vdec_output_config_params),
+	.dec_input_prop = sm8550_vdec_subscribe_input_properties,
+	.dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties),
+	.dec_output_prop_avc = sm8550_vdec_subscribe_output_properties_avc,
+	.dec_output_prop_avc_size =
+		ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_avc),
+	.dec_output_prop_hevc = sm8550_vdec_subscribe_output_properties_hevc,
+	.dec_output_prop_hevc_size =
+		ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_hevc),
+	.dec_output_prop_vp9 = sm8550_vdec_subscribe_output_properties_vp9,
+	.dec_output_prop_vp9_size =
+		ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_vp9),
+
+	.dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl,
+	.dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl),
+	.dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl,
+	.dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl),
+};
+
 /*
  * Shares most of SM8550 data except:
  * - inst_caps to platform_inst_cap_qcs8300
diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8750.h b/drivers/media/platform/qcom/iris/iris_platform_sm8750.h
new file mode 100644
index 0000000000000000000000000000000000000000..719056656a5baf48a7bced634d2582629333cf5c
--- /dev/null
+++ b/drivers/media/platform/qcom/iris/iris_platform_sm8750.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2025 Linaro Ltd
+ */
+
+#ifndef __MEDIA_IRIS_PLATFORM_SM8750_H__
+#define __MEDIA_IRIS_PLATFORM_SM8750_H__
+
+static const char * const sm8750_clk_reset_table[] = {
+	"bus0", "bus1", "core", "vcodec0_core"
+};
+
+static const struct platform_clk_data sm8750_clk_table[] = {
+	{IRIS_AXI_CLK,		"iface"			},
+	{IRIS_CTRL_CLK,		"core"			},
+	{IRIS_HW_CLK,		"vcodec0_core"		},
+	{IRIS_AXI1_CLK,		"iface1"		},
+	{IRIS_CTRL_FREERUN_CLK,	"core_freerun"		},
+	{IRIS_HW_FREERUN_CLK,	"vcodec0_core_freerun"	},
+};
+
+#endif
diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
index 4e6e92357968d7419f114cc0ffa9b571bad19e46..5fb936a04155e72f4298cd6760eff6e9d1da6310 100644
--- a/drivers/media/platform/qcom/iris/iris_probe.c
+++ b/drivers/media/platform/qcom/iris/iris_probe.c
@@ -353,6 +353,10 @@ static const struct of_device_id iris_dt_match[] = {
 		.compatible = "qcom,sm8650-iris",
 		.data = &sm8650_data,
 	},
+	{
+		.compatible = "qcom,sm8750-iris",
+		.data = &sm8750_data,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, iris_dt_match);
diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
index c235112057aa7b7eab1995737541b7a8276ff18b..b00702a4d6c23258550a77373eb34740e785ef22 100644
--- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
+++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2025 Linaro Ltd
  */
 
 #include <linux/iopoll.h>
@@ -19,8 +20,11 @@
 #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_POWER_CONTROL		(WRAPPER_BASE_OFFS + 0x84)
 #define WRAPPER_CORE_CLOCK_CONFIG		(WRAPPER_BASE_OFFS + 0x88)
 #define CORE_CLK_RUN				0x0
+/* VPU v3.5 */
+#define WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0	(WRAPPER_BASE_OFFS + 0x78)
 
 #define WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG		(WRAPPER_TZ_BASE_OFFS + 0x14)
 #define CTL_AXI_CLK_HALT			BIT(0)
@@ -52,6 +56,8 @@
 #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 AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL	(AON_BASE_OFFS + 0x2C)
+#define AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_STATUS	(AON_BASE_OFFS + 0x30)
 
 static bool iris_vpu3x_hw_power_collapsed(struct iris_core *core)
 {
@@ -225,6 +231,194 @@ static int iris_vpu33_power_off_controller(struct iris_core *core)
 	return 0;
 }
 
+static int iris_vpu35_power_on_hw(struct iris_core *core)
+{
+	int ret;
+	u32 val;
+
+	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
+	if (ret)
+		return ret;
+
+	/* Switch GDSC to SW control */
+	writel(0x0, core->reg_base + WRAPPER_CORE_POWER_CONTROL);
+	ret = readl_poll_timeout(core->reg_base + WRAPPER_CORE_POWER_STATUS,
+				 val, val & BIT(1), 200, 2000);
+	if (ret)
+		goto err_disable_power;
+
+	ret = iris_prepare_enable_clock(core, IRIS_AXI_CLK);
+	if (ret)
+		goto err_gdsc;
+
+	ret = iris_prepare_enable_clock(core, IRIS_HW_FREERUN_CLK);
+	if (ret)
+		goto err_disable_axi_clk;
+
+	ret = iris_prepare_enable_clock(core, IRIS_HW_CLK);
+	if (ret)
+		goto err_disable_hw_free_clk;
+
+	ret = dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], true);
+	if (ret)
+		goto err_disable_hw_clk;
+
+	return 0;
+
+err_disable_hw_clk:
+	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
+err_disable_hw_free_clk:
+	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
+err_disable_axi_clk:
+	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
+err_gdsc:
+	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
+err_disable_power:
+	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
+
+	return ret;
+}
+
+static void iris_vpu35_power_off_hw(struct iris_core *core)
+{
+	u32 val = 0, value, i;
+	int ret;
+
+	if (iris_vpu3x_hw_power_collapsed(core))
+		goto disable_power;
+
+	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
+	if (value)
+		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
+
+	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
+		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
+					 val, val & 0x400000, 2000, 20000);
+		if (ret)
+			goto disable_power;
+	}
+
+	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
+				 val, val & BIT(0), 200, 2000);
+	if (ret)
+		goto disable_power;
+
+	/* set MNoC to low power, set PD_NOC_QREQ (bit 0) */
+	writel(BIT(0), core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
+
+	writel(CORE_BRIDGE_SW_RESET | CORE_BRIDGE_HW_RESET_DISABLE,
+	       core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
+	writel(CORE_BRIDGE_HW_RESET_DISABLE, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
+	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
+
+disable_power:
+	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
+	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
+	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
+	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
+
+	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
+	/*
+	 * Do not wait for power-down, because hardware might delay it (it
+	 * always timeouts).
+	 */
+
+	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
+}
+
+static int iris_vpu35_power_off_controller(struct iris_core *core)
+{
+	u32 xo_rst_tbl_size = core->iris_platform_data->controller_rst_tbl_size;
+	u32 clk_rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
+	u32 val = 0;
+	int ret;
+
+	writel(MSK_SIGNAL_FROM_TENSILICA | MSK_CORE_POWER_ON, core->reg_base + CPU_CS_X2RPMH);
+
+	writel(REQ_POWER_DOWN_PREP, core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_CONTROL);
+
+	ret = readl_poll_timeout(core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_STATUS,
+				 val, val & BIT(0), 200, 2000);
+	if (ret)
+		goto disable_power;
+
+	writel(0x0, core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_CONTROL);
+
+	writel(REQ_POWER_DOWN_PREP, core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL);
+	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_STATUS,
+				 val, val & (BIT(0) | BIT(1) | BIT(2)), 15, 1000);
+	if (ret)
+		goto disable_power;
+
+	writel(0x0, core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL);
+
+	writel(0x0, core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_CONTROL);
+
+	ret = readl_poll_timeout(core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_STATUS,
+				 val, val == 0, 200, 2000);
+	if (ret)
+		goto disable_power;
+
+disable_power:
+	iris_disable_unprepare_clock(core, IRIS_CTRL_CLK);
+	iris_disable_unprepare_clock(core, IRIS_CTRL_FREERUN_CLK);
+	iris_disable_unprepare_clock(core, IRIS_AXI1_CLK);
+
+	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
+
+	reset_control_bulk_reset(clk_rst_tbl_size, core->resets);
+
+	reset_control_bulk_assert(xo_rst_tbl_size, core->controller_resets);
+
+	usleep_range(400, 500);
+
+	reset_control_bulk_deassert(xo_rst_tbl_size, core->controller_resets);
+
+	return 0;
+}
+
+static int iris_vpu35_power_on_controller(struct iris_core *core)
+{
+	u32 rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
+	int ret;
+
+	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
+	if (ret)
+		return ret;
+
+	ret = reset_control_bulk_reset(rst_tbl_size, core->resets);
+	if (ret)
+		goto err_disable_power;
+
+	ret = iris_prepare_enable_clock(core, IRIS_AXI1_CLK);
+	if (ret)
+		goto err_disable_power;
+
+	ret = iris_prepare_enable_clock(core, IRIS_CTRL_FREERUN_CLK);
+	if (ret)
+		goto err_disable_axi1_clk;
+
+	ret = iris_prepare_enable_clock(core, IRIS_CTRL_CLK);
+	if (ret)
+		goto err_disable_ctrl_free_clk;
+
+	return 0;
+
+err_disable_ctrl_free_clk:
+	iris_disable_unprepare_clock(core, IRIS_CTRL_FREERUN_CLK);
+err_disable_axi1_clk:
+	iris_disable_unprepare_clock(core, IRIS_AXI1_CLK);
+err_disable_power:
+	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
+
+	return ret;
+}
+
+static void iris_vpu35_program_bootup_registers(struct iris_core *core)
+{
+	writel(0x1, core->reg_base + WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0);
+}
+
 static u64 iris_vpu3x_calculate_frequency(struct iris_inst *inst, size_t data_size)
 {
 	struct platform_inst_caps *caps = inst->core->iris_platform_data->inst_caps;
@@ -277,3 +471,12 @@ const struct vpu_ops iris_vpu33_ops = {
 	.power_on_controller = iris_vpu_power_on_controller,
 	.calc_freq = iris_vpu3x_calculate_frequency,
 };
+
+const struct vpu_ops iris_vpu35_ops = {
+	.power_off_hw = iris_vpu35_power_off_hw,
+	.power_on_hw = iris_vpu35_power_on_hw,
+	.power_off_controller = iris_vpu35_power_off_controller,
+	.power_on_controller = iris_vpu35_power_on_controller,
+	.program_bootup_registers = iris_vpu35_program_bootup_registers,
+	.calc_freq = iris_vpu3x_calculate_frequency,
+};
diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
index 6c51002f72ab3d9e16d5a2a50ac712fac91ae25c..bb98950e018fadf69ac4f41b3037f7fd6ac33c5b 100644
--- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
+++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
@@ -84,6 +84,7 @@ static void iris_vpu_interrupt_init(struct iris_core *core)
 static void iris_vpu_setup_ucregion_memory_map(struct iris_core *core)
 {
 	u32 queue_size, value;
+	const struct vpu_ops *vpu_ops = core->iris_platform_data->vpu_ops;
 
 	/* Iris hardware requires 4K queue alignment */
 	queue_size = ALIGN(sizeof(struct iris_hfi_queue_table_header) +
@@ -105,6 +106,9 @@ static void iris_vpu_setup_ucregion_memory_map(struct iris_core *core)
 		value = (u32)core->sfr_daddr + core->iris_platform_data->core_arch;
 		writel(value, core->reg_base + SFR_ADDR);
 	}
+
+	if (vpu_ops->program_bootup_registers)
+		vpu_ops->program_bootup_registers(core);
 }
 
 int iris_vpu_boot_firmware(struct iris_core *core)
diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
index d95b305ca5a89ba8f08aefb6e6acd9ea4a721a8b..d636e287457adf0c44540af5c85cfa69decbca8b 100644
--- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
+++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
@@ -11,12 +11,14 @@ struct iris_core;
 extern const struct vpu_ops iris_vpu2_ops;
 extern const struct vpu_ops iris_vpu3_ops;
 extern const struct vpu_ops iris_vpu33_ops;
+extern const struct vpu_ops iris_vpu35_ops;
 
 struct vpu_ops {
 	void (*power_off_hw)(struct iris_core *core);
 	int (*power_on_hw)(struct iris_core *core);
 	int (*power_off_controller)(struct iris_core *core);
 	int (*power_on_controller)(struct iris_core *core);
+	void (*program_bootup_registers)(struct iris_core *core);
 	u64 (*calc_freq)(struct iris_inst *inst, size_t data_size);
 };
 

-- 
2.43.0

On 7/14/2025 7:11 PM, Krzysztof Kozlowski wrote:
> Add support for SM8750 Iris codec with major differences against
> previous generation SM8650:
> 
> 1. New clocks and new resets, thus new power up and power down
>    sequences,
> 
> 2. New WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0 register programmed
>    during boot-up
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../platform/qcom/iris/iris_platform_common.h      |   6 +-
>  .../media/platform/qcom/iris/iris_platform_gen2.c  |  68 +++++++
>  .../platform/qcom/iris/iris_platform_sm8750.h      |  22 +++
>  drivers/media/platform/qcom/iris/iris_probe.c      |   4 +
>  drivers/media/platform/qcom/iris/iris_vpu3x.c      | 203 +++++++++++++++++++++
>  drivers/media/platform/qcom/iris/iris_vpu_common.c |   4 +
>  drivers/media/platform/qcom/iris/iris_vpu_common.h |   2 +
>  7 files changed, 308 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> index adafdce8a856f9c661aabc5ca28f0faceaa93551..fd5a6e69e01cfd00253f4ffb282d40112b93073b 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> @@ -38,11 +38,15 @@ extern struct iris_platform_data qcs8300_data;
>  extern struct iris_platform_data sm8250_data;
>  extern struct iris_platform_data sm8550_data;
>  extern struct iris_platform_data sm8650_data;
> +extern struct iris_platform_data sm8750_data;
>  
>  enum platform_clk_type {
> -	IRIS_AXI_CLK,
> +	IRIS_AXI_CLK, /* AXI0 in case of platforms with multiple AXI clocks */
>  	IRIS_CTRL_CLK,
>  	IRIS_HW_CLK,
> +	IRIS_AXI1_CLK,
> +	IRIS_CTRL_FREERUN_CLK,
> +	IRIS_HW_FREERUN_CLK,
>  };
>  
>  struct platform_clk_data {
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_gen2.c b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
> index d3026b2bcb708c7ec31f134f628df7e57b54af4f..795efe2226228c4d7155ce18ff42ba9cb74b4af2 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_gen2.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
>   * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2025 Linaro Ltd
>   */
>  
>  #include "iris_core.h"
> @@ -12,6 +13,7 @@
>  
>  #include "iris_platform_qcs8300.h"
>  #include "iris_platform_sm8650.h"
> +#include "iris_platform_sm8750.h"
>  
>  #define VIDEO_ARCH_LX 1
>  
> @@ -463,6 +465,72 @@ struct iris_platform_data sm8650_data = {
>  	.dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl),
>  };
>  
> +struct iris_platform_data sm8750_data = {
> +	.get_instance = iris_hfi_gen2_get_instance,
> +	.init_hfi_command_ops = iris_hfi_gen2_command_ops_init,
> +	.init_hfi_response_ops = iris_hfi_gen2_response_ops_init,
> +	.vpu_ops = &iris_vpu35_ops,
> +	.set_preset_registers = iris_set_sm8550_preset_registers,
> +	.icc_tbl = sm8550_icc_table,
> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
> +	.clk_rst_tbl = sm8750_clk_reset_table,
> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8750_clk_reset_table),
> +	.bw_tbl_dec = sm8550_bw_table_dec,
> +	.bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec),
> +	.pmdomain_tbl = sm8550_pmdomain_table,
> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
> +	.opp_pd_tbl = sm8550_opp_pd_table,
> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
> +	.clk_tbl = sm8750_clk_table,
> +	.clk_tbl_size = ARRAY_SIZE(sm8750_clk_table),
> +	/* Upper bound of DMA address range */
> +	.dma_mask = 0xe0000000 - 1,
> +	.fwname = "qcom/vpu/vpu35_4v.mbn",
Could you clarify where this firmware has been merged? Also, it appears
that the naming convention hasn't been followed.
> +	.pas_id = IRIS_PAS_ID,
> +	.inst_caps = &platform_inst_cap_sm8550,
> +	.inst_fw_caps = inst_fw_cap_sm8550,
> +	.inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550),
> +	.tz_cp_config_data = &tz_cp_config_sm8550,
> +	.core_arch = VIDEO_ARCH_LX,
> +	.hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE,
> +	.ubwc_config = &ubwc_config_sm8550,
> +	.num_vpp_pipe = 4,
> +	.max_session_count = 16,
> +	.max_core_mbpf = NUM_MBS_8K * 2,
> +	.input_config_params_default =
> +		sm8550_vdec_input_config_params_default,
> +	.input_config_params_default_size =
> +		ARRAY_SIZE(sm8550_vdec_input_config_params_default),
> +	.input_config_params_hevc =
> +		sm8550_vdec_input_config_param_hevc,
> +	.input_config_params_hevc_size =
> +		ARRAY_SIZE(sm8550_vdec_input_config_param_hevc),
> +	.input_config_params_vp9 =
> +		sm8550_vdec_input_config_param_vp9,
> +	.input_config_params_vp9_size =
> +		ARRAY_SIZE(sm8550_vdec_input_config_param_vp9),
> +	.output_config_params =
> +		sm8550_vdec_output_config_params,
> +	.output_config_params_size =
> +		ARRAY_SIZE(sm8550_vdec_output_config_params),
> +	.dec_input_prop = sm8550_vdec_subscribe_input_properties,
> +	.dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties),
> +	.dec_output_prop_avc = sm8550_vdec_subscribe_output_properties_avc,
> +	.dec_output_prop_avc_size =
> +		ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_avc),
> +	.dec_output_prop_hevc = sm8550_vdec_subscribe_output_properties_hevc,
> +	.dec_output_prop_hevc_size =
> +		ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_hevc),
> +	.dec_output_prop_vp9 = sm8550_vdec_subscribe_output_properties_vp9,
> +	.dec_output_prop_vp9_size =
> +		ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_vp9),
> +
> +	.dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl,
> +	.dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl),
> +	.dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl,
> +	.dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl),
> +};
> +
>  /*
>   * Shares most of SM8550 data except:
>   * - inst_caps to platform_inst_cap_qcs8300
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8750.h b/drivers/media/platform/qcom/iris/iris_platform_sm8750.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..719056656a5baf48a7bced634d2582629333cf5c
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8750.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2025 Linaro Ltd
> + */
> +
> +#ifndef __MEDIA_IRIS_PLATFORM_SM8750_H__
> +#define __MEDIA_IRIS_PLATFORM_SM8750_H__
> +
> +static const char * const sm8750_clk_reset_table[] = {
> +	"bus0", "bus1", "core", "vcodec0_core"
> +};
> +
> +static const struct platform_clk_data sm8750_clk_table[] = {
> +	{IRIS_AXI_CLK,		"iface"			},
> +	{IRIS_CTRL_CLK,		"core"			},
> +	{IRIS_HW_CLK,		"vcodec0_core"		},
> +	{IRIS_AXI1_CLK,		"iface1"		},
> +	{IRIS_CTRL_FREERUN_CLK,	"core_freerun"		},
> +	{IRIS_HW_FREERUN_CLK,	"vcodec0_core_freerun"	},
> +};
> +
> +#endif
> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
> index 4e6e92357968d7419f114cc0ffa9b571bad19e46..5fb936a04155e72f4298cd6760eff6e9d1da6310 100644
> --- a/drivers/media/platform/qcom/iris/iris_probe.c
> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
> @@ -353,6 +353,10 @@ static const struct of_device_id iris_dt_match[] = {
>  		.compatible = "qcom,sm8650-iris",
>  		.data = &sm8650_data,
>  	},
> +	{
> +		.compatible = "qcom,sm8750-iris",
> +		.data = &sm8750_data,
> +	},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, iris_dt_match);
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index c235112057aa7b7eab1995737541b7a8276ff18b..b00702a4d6c23258550a77373eb34740e785ef22 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
>   * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2025 Linaro Ltd
>   */
>  
>  #include <linux/iopoll.h>
> @@ -19,8 +20,11 @@
>  #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_POWER_CONTROL		(WRAPPER_BASE_OFFS + 0x84)
>  #define WRAPPER_CORE_CLOCK_CONFIG		(WRAPPER_BASE_OFFS + 0x88)
>  #define CORE_CLK_RUN				0x0
> +/* VPU v3.5 */
> +#define WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0	(WRAPPER_BASE_OFFS + 0x78)
>  
>  #define WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG		(WRAPPER_TZ_BASE_OFFS + 0x14)
>  #define CTL_AXI_CLK_HALT			BIT(0)
> @@ -52,6 +56,8 @@
>  #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 AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL	(AON_BASE_OFFS + 0x2C)
> +#define AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_STATUS	(AON_BASE_OFFS + 0x30)
>  
>  static bool iris_vpu3x_hw_power_collapsed(struct iris_core *core)
>  {
> @@ -225,6 +231,194 @@ static int iris_vpu33_power_off_controller(struct iris_core *core)
>  	return 0;
>  }
>  
> +static int iris_vpu35_power_on_hw(struct iris_core *core)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> +	if (ret)
> +		return ret;
> +
> +	/* Switch GDSC to SW control */
> +	writel(0x0, core->reg_base + WRAPPER_CORE_POWER_CONTROL);
GDSCs have been transitioned from HW_CTRL to HW_CTRL_TRIGGER, placing them
under software control by default, what is the need of doing this?
> +	ret = readl_poll_timeout(core->reg_base + WRAPPER_CORE_POWER_STATUS,
> +				 val, val & BIT(1), 200, 2000);
> +	if (ret)
> +		goto err_disable_power;
> +
> +	ret = iris_prepare_enable_clock(core, IRIS_AXI_CLK);
> +	if (ret)
> +		goto err_gdsc;
> +
> +	ret = iris_prepare_enable_clock(core, IRIS_HW_FREERUN_CLK);
> +	if (ret)
> +		goto err_disable_axi_clk;
> +
> +	ret = iris_prepare_enable_clock(core, IRIS_HW_CLK);
> +	if (ret)
> +		goto err_disable_hw_free_clk;
> +
> +	ret = dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], true);
> +	if (ret)
> +		goto err_disable_hw_clk;
> +
> +	return 0;
> +
> +err_disable_hw_clk:
> +	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
> +err_disable_hw_free_clk:
> +	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
> +err_disable_axi_clk:
> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
> +err_gdsc:
> +	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
> +err_disable_power:
> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> +
> +	return ret;
> +}
> +
> +static void iris_vpu35_power_off_hw(struct iris_core *core)
> +{
> +	u32 val = 0, value, i;
> +	int ret;
> +
> +	if (iris_vpu3x_hw_power_collapsed(core))
> +		goto disable_power;
> +
> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +	if (value)
> +		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +
> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
> +					 val, val & 0x400000, 2000, 20000);
> +		if (ret)
> +			goto disable_power;
> +	}
> +
> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
> +				 val, val & BIT(0), 200, 2000);
what are you polling here for?
> +	if (ret)
> +		goto disable_power;
> +
> +	/* set MNoC to low power, set PD_NOC_QREQ (bit 0) */
Could you share the reference for this sqeunece, this looks half-cooked.
Would recommend following Hardware programmin guide(HPG) for this.
> +	writel(BIT(0), core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +
> +	writel(CORE_BRIDGE_SW_RESET | CORE_BRIDGE_HW_RESET_DISABLE,
> +	       core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(CORE_BRIDGE_HW_RESET_DISABLE, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +
> +disable_power:
> +	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
> +	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
> +
> +	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
> +	/*
> +	 * Do not wait for power-down, because hardware might delay it (it
> +	 * always timeouts).
> +	 */
> +
> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> +}
> +
> +static int iris_vpu35_power_off_controller(struct iris_core *core)
> +{
> +	u32 xo_rst_tbl_size = core->iris_platform_data->controller_rst_tbl_size;
where is controller_rst_tbl_size defined for this SOC?
> +	u32 clk_rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
> +	u32 val = 0;
> +	int ret;
> +
> +	writel(MSK_SIGNAL_FROM_TENSILICA | MSK_CORE_POWER_ON, core->reg_base + CPU_CS_X2RPMH);
> +
> +	writel(REQ_POWER_DOWN_PREP, core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_CONTROL);
> +
> +	ret = readl_poll_timeout(core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_STATUS,
> +				 val, val & BIT(0), 200, 2000);
> +	if (ret)
> +		goto disable_power;
> +
> +	writel(0x0, core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_CONTROL);
> +
> +	writel(REQ_POWER_DOWN_PREP, core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL);
> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_STATUS,
> +				 val, val & (BIT(0) | BIT(1) | BIT(2)), 15, 1000);
doesn't seems right and missing retry logic. would recommend referring HPG.
> +	if (ret)
> +		goto disable_power;
> +
> +	writel(0x0, core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL);
> +
> +	writel(0x0, core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_CONTROL);
> +
> +	ret = readl_poll_timeout(core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_STATUS,
> +				 val, val == 0, 200, 2000);
> +	if (ret)
> +		goto disable_power;
> +
> +disable_power:
> +	iris_disable_unprepare_clock(core, IRIS_CTRL_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_CTRL_FREERUN_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_AXI1_CLK);
> +
> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
> +
> +	reset_control_bulk_reset(clk_rst_tbl_size, core->resets);
> +
> +	reset_control_bulk_assert(xo_rst_tbl_size, core->controller_resets);
??
> +
> +	usleep_range(400, 500);
> +
> +	reset_control_bulk_deassert(xo_rst_tbl_size, core->controller_resets);
> +
> +	return 0;
> +}
> +
> +static int iris_vpu35_power_on_controller(struct iris_core *core)
> +{
> +	u32 rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
> +	int ret;
> +
> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_bulk_reset(rst_tbl_size, core->resets);
what is the reference for this?

Thanks,
Dikshita
> +	if (ret)
> +		goto err_disable_power;
> +
> +	ret = iris_prepare_enable_clock(core, IRIS_AXI1_CLK);
> +	if (ret)
> +		goto err_disable_power;
> +
> +	ret = iris_prepare_enable_clock(core, IRIS_CTRL_FREERUN_CLK);
> +	if (ret)
> +		goto err_disable_axi1_clk;
> +
> +	ret = iris_prepare_enable_clock(core, IRIS_CTRL_CLK);
> +	if (ret)
> +		goto err_disable_ctrl_free_clk;
> +
> +	return 0;
> +
> +err_disable_ctrl_free_clk:
> +	iris_disable_unprepare_clock(core, IRIS_CTRL_FREERUN_CLK);
> +err_disable_axi1_clk:
> +	iris_disable_unprepare_clock(core, IRIS_AXI1_CLK);
> +err_disable_power:
> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
> +
> +	return ret;
> +}
> +
> +static void iris_vpu35_program_bootup_registers(struct iris_core *core)
> +{
> +	writel(0x1, core->reg_base + WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0);
> +}
> +
>  static u64 iris_vpu3x_calculate_frequency(struct iris_inst *inst, size_t data_size)
>  {
>  	struct platform_inst_caps *caps = inst->core->iris_platform_data->inst_caps;
> @@ -277,3 +471,12 @@ const struct vpu_ops iris_vpu33_ops = {
>  	.power_on_controller = iris_vpu_power_on_controller,
>  	.calc_freq = iris_vpu3x_calculate_frequency,
>  };
> +
> +const struct vpu_ops iris_vpu35_ops = {
> +	.power_off_hw = iris_vpu35_power_off_hw,
> +	.power_on_hw = iris_vpu35_power_on_hw,
> +	.power_off_controller = iris_vpu35_power_off_controller,
> +	.power_on_controller = iris_vpu35_power_on_controller,
> +	.program_bootup_registers = iris_vpu35_program_bootup_registers,
> +	.calc_freq = iris_vpu3x_calculate_frequency,
> +};
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 6c51002f72ab3d9e16d5a2a50ac712fac91ae25c..bb98950e018fadf69ac4f41b3037f7fd6ac33c5b 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -84,6 +84,7 @@ static void iris_vpu_interrupt_init(struct iris_core *core)
>  static void iris_vpu_setup_ucregion_memory_map(struct iris_core *core)
>  {
>  	u32 queue_size, value;
> +	const struct vpu_ops *vpu_ops = core->iris_platform_data->vpu_ops;
>  
>  	/* Iris hardware requires 4K queue alignment */
>  	queue_size = ALIGN(sizeof(struct iris_hfi_queue_table_header) +
> @@ -105,6 +106,9 @@ static void iris_vpu_setup_ucregion_memory_map(struct iris_core *core)
>  		value = (u32)core->sfr_daddr + core->iris_platform_data->core_arch;
>  		writel(value, core->reg_base + SFR_ADDR);
>  	}
> +
> +	if (vpu_ops->program_bootup_registers)
> +		vpu_ops->program_bootup_registers(core);
>  }
>  
>  int iris_vpu_boot_firmware(struct iris_core *core)
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> index d95b305ca5a89ba8f08aefb6e6acd9ea4a721a8b..d636e287457adf0c44540af5c85cfa69decbca8b 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> @@ -11,12 +11,14 @@ struct iris_core;
>  extern const struct vpu_ops iris_vpu2_ops;
>  extern const struct vpu_ops iris_vpu3_ops;
>  extern const struct vpu_ops iris_vpu33_ops;
> +extern const struct vpu_ops iris_vpu35_ops;
>  
>  struct vpu_ops {
>  	void (*power_off_hw)(struct iris_core *core);
>  	int (*power_on_hw)(struct iris_core *core);
>  	int (*power_off_controller)(struct iris_core *core);
>  	int (*power_on_controller)(struct iris_core *core);
> +	void (*program_bootup_registers)(struct iris_core *core);
>  	u64 (*calc_freq)(struct iris_inst *inst, size_t data_size);
>  };
>  
>

On 16/07/2025 11:10, Dikshita Agarwal wrote:
> 
> 
> On 7/14/2025 7:11 PM, Krzysztof Kozlowski wrote:
>> Add support for SM8750 Iris codec with major differences against
>> previous generation SM8650:
>>
>> 1. New clocks and new resets, thus new power up and power down
>>    sequences,
>>
>> 2. New WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0 register programmed
>>    during boot-up
>>



Please kindly trim the replies from unnecessary context. It makes it
much easier to find new content.


>> +struct iris_platform_data sm8750_data = {
>> +	.get_instance = iris_hfi_gen2_get_instance,
>> +	.init_hfi_command_ops = iris_hfi_gen2_command_ops_init,
>> +	.init_hfi_response_ops = iris_hfi_gen2_response_ops_init,
>> +	.vpu_ops = &iris_vpu35_ops,
>> +	.set_preset_registers = iris_set_sm8550_preset_registers,
>> +	.icc_tbl = sm8550_icc_table,
>> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
>> +	.clk_rst_tbl = sm8750_clk_reset_table,
>> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8750_clk_reset_table),
>> +	.bw_tbl_dec = sm8550_bw_table_dec,
>> +	.bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec),
>> +	.pmdomain_tbl = sm8550_pmdomain_table,
>> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
>> +	.opp_pd_tbl = sm8550_opp_pd_table,
>> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
>> +	.clk_tbl = sm8750_clk_table,
>> +	.clk_tbl_size = ARRAY_SIZE(sm8750_clk_table),
>> +	/* Upper bound of DMA address range */
>> +	.dma_mask = 0xe0000000 - 1,
>> +	.fwname = "qcom/vpu/vpu35_4v.mbn",
> Could you clarify where this firmware has been merged? Also, it appears
> that the naming convention hasn't been followed.


I mentioned in the DTS patchset but not here, so I will add it in the
cover letter - firmware is not released. About the name I cannot
comment, that's the name I got from qcom. Happy to use whatever name you
prefer.



>> +static int iris_vpu35_power_on_hw(struct iris_core *core)
>> +{
>> +	int ret;
>> +	u32 val;
>> +
>> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Switch GDSC to SW control */
>> +	writel(0x0, core->reg_base + WRAPPER_CORE_POWER_CONTROL);
> GDSCs have been transitioned from HW_CTRL to HW_CTRL_TRIGGER, placing them
> under software control by default, what is the need of doing this?
>> +	ret = readl_poll_timeout(core->reg_base + WRAPPER_CORE_POWER_STATUS,
>> +				 val, val & BIT(1), 200, 2000);


The need comes from differences between this and previous generation,
mostly based on downstream sources. I think the hardware just did not
boot up without it.

You need to fix your email client to add line breaks around your
replies, because it is very difficult to spot them. It's close to
impossible...


>> +	if (ret)
>> +		goto err_disable_power;
>> +
>> +	ret = iris_prepare_enable_clock(core, IRIS_AXI_CLK);
>> +	if (ret)
>> +		goto err_gdsc;
>> +
>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_FREERUN_CLK);
>> +	if (ret)
>> +		goto err_disable_axi_clk;
>> +
>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_CLK);
>> +	if (ret)
>> +		goto err_disable_hw_free_clk;
>> +
>> +	ret = dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], true);
>> +	if (ret)
>> +		goto err_disable_hw_clk;
>> +
>> +	return 0;
>> +
>> +err_disable_hw_clk:
>> +	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
>> +err_disable_hw_free_clk:
>> +	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
>> +err_disable_axi_clk:
>> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
>> +err_gdsc:
>> +	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
>> +err_disable_power:
>> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>> +
>> +	return ret;
>> +}
>> +
>> +static void iris_vpu35_power_off_hw(struct iris_core *core)
>> +{
>> +	u32 val = 0, value, i;
>> +	int ret;
>> +
>> +	if (iris_vpu3x_hw_power_collapsed(core))
>> +		goto disable_power;
>> +
>> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>> +	if (value)
>> +		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>> +
>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>> +					 val, val & 0x400000, 2000, 20000);
>> +		if (ret)
>> +			goto disable_power;
>> +	}
>> +
>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>> +				 val, val & BIT(0), 200, 2000);
> what are you polling here for?


This is not different than existing code. I don't understand why you are
commenting on something which is already there.

>> +	if (ret)
>> +		goto disable_power;
>> +
>> +	/* set MNoC to low power, set PD_NOC_QREQ (bit 0) */
> Could you share the reference for this sqeunece, this looks half-cooked.
> Would recommend following Hardware programmin guide(HPG) for this.


Why? Look at existing code. It's the same.

I think I responded to all your comments - it barely possible to spot
them in the quote.

Best regards,
Krzysztof

On 7/16/2025 2:58 PM, Krzysztof Kozlowski wrote:
> On 16/07/2025 11:10, Dikshita Agarwal wrote:
>>
>>
>> On 7/14/2025 7:11 PM, Krzysztof Kozlowski wrote:
>>> Add support for SM8750 Iris codec with major differences against
>>> previous generation SM8650:
>>>
>>> 1. New clocks and new resets, thus new power up and power down
>>>    sequences,
>>>
>>> 2. New WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0 register programmed
>>>    during boot-up
>>>
> 
> 
> 
> Please kindly trim the replies from unnecessary context. It makes it
> much easier to find new content.
> 
> 
>>> +struct iris_platform_data sm8750_data = {
>>> +	.get_instance = iris_hfi_gen2_get_instance,
>>> +	.init_hfi_command_ops = iris_hfi_gen2_command_ops_init,
>>> +	.init_hfi_response_ops = iris_hfi_gen2_response_ops_init,
>>> +	.vpu_ops = &iris_vpu35_ops,
>>> +	.set_preset_registers = iris_set_sm8550_preset_registers,
>>> +	.icc_tbl = sm8550_icc_table,
>>> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
>>> +	.clk_rst_tbl = sm8750_clk_reset_table,
>>> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8750_clk_reset_table),
>>> +	.bw_tbl_dec = sm8550_bw_table_dec,
>>> +	.bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec),
>>> +	.pmdomain_tbl = sm8550_pmdomain_table,
>>> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
>>> +	.opp_pd_tbl = sm8550_opp_pd_table,
>>> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
>>> +	.clk_tbl = sm8750_clk_table,
>>> +	.clk_tbl_size = ARRAY_SIZE(sm8750_clk_table),
>>> +	/* Upper bound of DMA address range */
>>> +	.dma_mask = 0xe0000000 - 1,
>>> +	.fwname = "qcom/vpu/vpu35_4v.mbn",
>> Could you clarify where this firmware has been merged? Also, it appears
>> that the naming convention hasn't been followed.
> 
> 
> I mentioned in the DTS patchset but not here, so I will add it in the
> cover letter - firmware is not released. About the name I cannot
> comment, that's the name I got from qcom. Happy to use whatever name you
> prefer.
> 


You can name it vpu35_p4.mbn to maintain consistency with the current
naming convention.


> 
> 
>>> +static int iris_vpu35_power_on_hw(struct iris_core *core)
>>> +{
>>> +	int ret;
>>> +	u32 val;
>>> +
>>> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Switch GDSC to SW control */
>>> +	writel(0x0, core->reg_base + WRAPPER_CORE_POWER_CONTROL);
>> GDSCs have been transitioned from HW_CTRL to HW_CTRL_TRIGGER, placing them
>> under software control by default, what is the need of doing this?
>>> +	ret = readl_poll_timeout(core->reg_base + WRAPPER_CORE_POWER_STATUS,
>>> +				 val, val & BIT(1), 200, 2000);
> 
> 
> The need comes from differences between this and previous generation,


which previous generation you’re referring to?
HW_CTRL_TRIGGER is supported on SM8550 and all later SOCs, and if you look
at videocc changes, same applies to SM8750 as well.



> mostly based on downstream sources. I think the hardware just did not
> boot up without it.


That shouldn’t be the case. The downstream design is different, which is
why the driver requires the above code to move the GDSC to software control
before enabling the clock. With HW_CTRL_TRIGGER, this step isn’t needed, so
the above code is unnecessary.


> 
> You need to fix your email client to add line breaks around your
> replies, because it is very difficult to spot them. It's close to
> impossible...
> 
> 
>>> +	if (ret)
>>> +		goto err_disable_power;
>>> +
>>> +	ret = iris_prepare_enable_clock(core, IRIS_AXI_CLK);
>>> +	if (ret)
>>> +		goto err_gdsc;
>>> +
>>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_FREERUN_CLK);
>>> +	if (ret)
>>> +		goto err_disable_axi_clk;
>>> +
>>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_CLK);
>>> +	if (ret)
>>> +		goto err_disable_hw_free_clk;
>>> +
>>> +	ret = dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], true);
>>> +	if (ret)
>>> +		goto err_disable_hw_clk;
>>> +
>>> +	return 0;
>>> +
>>> +err_disable_hw_clk:
>>> +	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
>>> +err_disable_hw_free_clk:
>>> +	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
>>> +err_disable_axi_clk:
>>> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
>>> +err_gdsc:
>>> +	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
>>> +err_disable_power:
>>> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void iris_vpu35_power_off_hw(struct iris_core *core)
>>> +{
>>> +	u32 val = 0, value, i;
>>> +	int ret;
>>> +
>>> +	if (iris_vpu3x_hw_power_collapsed(core))
>>> +		goto disable_power;
>>> +
>>> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>> +	if (value)
>>> +		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>> +
>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>> +					 val, val & 0x400000, 2000, 20000);
>>> +		if (ret)
>>> +			goto disable_power;
>>> +	}
>>> +
>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>> +				 val, val & BIT(0), 200, 2000);
>> what are you polling here for?
> 
> 
> This is not different than existing code. I don't understand why you are
> commenting on something which is already there.

Which code are you referring to?

You are not setting AON_WRAPPER_MVP_NOC_LPI_CONTROL and polling for its status.

The current code is incomplete and missing several steps.
Please review and provide a corrected version.


> 
>>> +	if (ret)
>>> +		goto disable_power;
>>> +
>>> +	/* set MNoC to low power, set PD_NOC_QREQ (bit 0) */
>> Could you share the reference for this sqeunece, this looks half-cooked.
>> Would recommend following Hardware programmin guide(HPG) for this.
> 
> 
> Why? Look at existing code. It's the same.


Which existing code? Please be specific.
I don't think you referred to downstream code for this, because I see a lot
of missing pieces here.


> 
> I think I responded to all your comments - it barely possible to spot
> them in the quote.
> 


No, you have missed some of the later comments. Since the code is snipped,
I can’t point out those comments here.


Thanks,
Dikshita

> Best regards,
> Krzysztof

On 17/07/2025 09:37, Dikshita Agarwal wrote:
> 
> 
>> mostly based on downstream sources. I think the hardware just did not
>> boot up without it.
> 
> 
> That shouldn’t be the case. The downstream design is different, which is
> why the driver requires the above code to move the GDSC to software control
> before enabling the clock. With HW_CTRL_TRIGGER, this step isn’t needed, so
> the above code is unnecessary.

Ack.

Best regards,
Krzysztof

On 17/07/2025 09:37, Dikshita Agarwal wrote:
>>>> +	.clk_tbl_size = ARRAY_SIZE(sm8750_clk_table),
>>>> +	/* Upper bound of DMA address range */
>>>> +	.dma_mask = 0xe0000000 - 1,
>>>> +	.fwname = "qcom/vpu/vpu35_4v.mbn",
>>> Could you clarify where this firmware has been merged? Also, it appears
>>> that the naming convention hasn't been followed.
>>
>>
>> I mentioned in the DTS patchset but not here, so I will add it in the
>> cover letter - firmware is not released. About the name I cannot
>> comment, that's the name I got from qcom. Happy to use whatever name you
>> prefer.
>>
> 
> 
> You can name it vpu35_p4.mbn to maintain consistency with the current
> naming convention.


Sure.

> 
> 
>>
>>
>>>> +static int iris_vpu35_power_on_hw(struct iris_core *core)
>>>> +{
>>>> +	int ret;
>>>> +	u32 val;
>>>> +
>>>> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Switch GDSC to SW control */
>>>> +	writel(0x0, core->reg_base + WRAPPER_CORE_POWER_CONTROL);
>>> GDSCs have been transitioned from HW_CTRL to HW_CTRL_TRIGGER, placing them
>>> under software control by default, what is the need of doing this?
>>>> +	ret = readl_poll_timeout(core->reg_base + WRAPPER_CORE_POWER_STATUS,
>>>> +				 val, val & BIT(1), 200, 2000);
>>
>>
>> The need comes from differences between this and previous generation,
> 
> 
> which previous generation you’re referring to?


The one I mentioned in the commit msg - SM8650.

> HW_CTRL_TRIGGER is supported on SM8550 and all later SOCs, and if you look
> at videocc changes, same applies to SM8750 as well.
> 
> 
> 
>> mostly based on downstream sources. I think the hardware just did not
>> boot up without it.
> 
> 
> That shouldn’t be the case. The downstream design is different, which is
> why the driver requires the above code to move the GDSC to software control
> before enabling the clock. With HW_CTRL_TRIGGER, this step isn’t needed, so
> the above code is unnecessary.
> 
> 
>>
>> You need to fix your email client to add line breaks around your
>> replies, because it is very difficult to spot them. It's close to
>> impossible...
>>
>>
>>>> +	if (ret)
>>>> +		goto err_disable_power;
>>>> +
>>>> +	ret = iris_prepare_enable_clock(core, IRIS_AXI_CLK);
>>>> +	if (ret)
>>>> +		goto err_gdsc;
>>>> +
>>>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_FREERUN_CLK);
>>>> +	if (ret)
>>>> +		goto err_disable_axi_clk;
>>>> +
>>>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_CLK);
>>>> +	if (ret)
>>>> +		goto err_disable_hw_free_clk;
>>>> +
>>>> +	ret = dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], true);
>>>> +	if (ret)
>>>> +		goto err_disable_hw_clk;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err_disable_hw_clk:
>>>> +	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
>>>> +err_disable_hw_free_clk:
>>>> +	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
>>>> +err_disable_axi_clk:
>>>> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
>>>> +err_gdsc:
>>>> +	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
>>>> +err_disable_power:
>>>> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void iris_vpu35_power_off_hw(struct iris_core *core)
>>>> +{
>>>> +	u32 val = 0, value, i;
>>>> +	int ret;
>>>> +
>>>> +	if (iris_vpu3x_hw_power_collapsed(core))
>>>> +		goto disable_power;
>>>> +
>>>> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>>> +	if (value)
>>>> +		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>>> +
>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>> +					 val, val & 0x400000, 2000, 20000);
>>>> +		if (ret)
>>>> +			goto disable_power;
>>>> +	}
>>>> +
>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>> +				 val, val & BIT(0), 200, 2000);
>>> what are you polling here for?
>>
>>
>> This is not different than existing code. I don't understand why you are
>> commenting on something which is already there.
> 
> Which code are you referring to?

To the existing vpu33 which had Reviewed-by: Vikash Garodia
<quic_vgarodia@quicinc.com>

You understand that everything here is the same, everything is a copy
while adding just few more things?

My patch is not doing in this respect anything different that what you
reviewed.


> 
> You are not setting AON_WRAPPER_MVP_NOC_LPI_CONTROL and polling for its status.

True, neither old reviewed code has done. I am not changing or fixing
any existing logic, I am only adding new clocks and resets.

> 
> The current code is incomplete and missing several steps.

Current you mean what was:
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
?

> Please review and provide a corrected version.
> 
> 
>>
>>>> +	if (ret)
>>>> +		goto disable_power;
>>>> +
>>>> +	/* set MNoC to low power, set PD_NOC_QREQ (bit 0) */
>>> Could you share the reference for this sqeunece, this looks half-cooked.
>>> Would recommend following Hardware programmin guide(HPG) for this.
>>
>>
>> Why? Look at existing code. It's the same.
> 
> 
> Which existing code? Please be specific.


Existing upstream VPU33 which this builts on top of. And that existing
upstream VPU33 was Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>

> I don't think you referred to downstream code for this, because I see a lot
> of missing pieces here.
> 
> 
>>
>> I think I responded to all your comments - it barely possible to spot
>> them in the quote.
>>
> 
> 
> No, you have missed some of the later comments. Since the code is snipped,
> I can’t point out those comments here.


It's impossible to find them in the original response.


Best regards,
Krzysztof

On 7/17/2025 3:04 PM, Krzysztof Kozlowski wrote:
> On 17/07/2025 09:37, Dikshita Agarwal wrote:
>>>>> +	.clk_tbl_size = ARRAY_SIZE(sm8750_clk_table),
>>>>> +	/* Upper bound of DMA address range */
>>>>> +	.dma_mask = 0xe0000000 - 1,
>>>>> +	.fwname = "qcom/vpu/vpu35_4v.mbn",
>>>> Could you clarify where this firmware has been merged? Also, it appears
>>>> that the naming convention hasn't been followed.
>>>
>>>
>>> I mentioned in the DTS patchset but not here, so I will add it in the
>>> cover letter - firmware is not released. About the name I cannot
>>> comment, that's the name I got from qcom. Happy to use whatever name you
>>> prefer.
>>>
>>
>>
>> You can name it vpu35_p4.mbn to maintain consistency with the current
>> naming convention.
> 
> 
> Sure.
> 
>>
>>
>>>
>>>
>>>>> +static int iris_vpu35_power_on_hw(struct iris_core *core)
>>>>> +{
>>>>> +	int ret;
>>>>> +	u32 val;
>>>>> +
>>>>> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	/* Switch GDSC to SW control */
>>>>> +	writel(0x0, core->reg_base + WRAPPER_CORE_POWER_CONTROL);
>>>> GDSCs have been transitioned from HW_CTRL to HW_CTRL_TRIGGER, placing them
>>>> under software control by default, what is the need of doing this?
>>>>> +	ret = readl_poll_timeout(core->reg_base + WRAPPER_CORE_POWER_STATUS,
>>>>> +				 val, val & BIT(1), 200, 2000);
>>>
>>>
>>> The need comes from differences between this and previous generation,
>>
>>
>> which previous generation you’re referring to?
> 
> 
> The one I mentioned in the commit msg - SM8650.
> 
>> HW_CTRL_TRIGGER is supported on SM8550 and all later SOCs, and if you look
>> at videocc changes, same applies to SM8750 as well.
>>
>>
>>
>>> mostly based on downstream sources. I think the hardware just did not
>>> boot up without it.
>>
>>
>> That shouldn’t be the case. The downstream design is different, which is
>> why the driver requires the above code to move the GDSC to software control
>> before enabling the clock. With HW_CTRL_TRIGGER, this step isn’t needed, so
>> the above code is unnecessary.
>>
>>
>>>
>>> You need to fix your email client to add line breaks around your
>>> replies, because it is very difficult to spot them. It's close to
>>> impossible...
>>>
>>>
>>>>> +	if (ret)
>>>>> +		goto err_disable_power;
>>>>> +
>>>>> +	ret = iris_prepare_enable_clock(core, IRIS_AXI_CLK);
>>>>> +	if (ret)
>>>>> +		goto err_gdsc;
>>>>> +
>>>>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_FREERUN_CLK);
>>>>> +	if (ret)
>>>>> +		goto err_disable_axi_clk;
>>>>> +
>>>>> +	ret = iris_prepare_enable_clock(core, IRIS_HW_CLK);
>>>>> +	if (ret)
>>>>> +		goto err_disable_hw_free_clk;
>>>>> +
>>>>> +	ret = dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], true);
>>>>> +	if (ret)
>>>>> +		goto err_disable_hw_clk;
>>>>> +
>>>>> +	return 0;
>>>>> +
>>>>> +err_disable_hw_clk:
>>>>> +	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
>>>>> +err_disable_hw_free_clk:
>>>>> +	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
>>>>> +err_disable_axi_clk:
>>>>> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
>>>>> +err_gdsc:
>>>>> +	writel(BIT(0), core->reg_base + WRAPPER_CORE_POWER_CONTROL);
>>>>> +err_disable_power:
>>>>> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static void iris_vpu35_power_off_hw(struct iris_core *core)
>>>>> +{
>>>>> +	u32 val = 0, value, i;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (iris_vpu3x_hw_power_collapsed(core))
>>>>> +		goto disable_power;
>>>>> +
>>>>> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>>>> +	if (value)
>>>>> +		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>>>> +
>>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>>> +					 val, val & 0x400000, 2000, 20000);
>>>>> +		if (ret)
>>>>> +			goto disable_power;
>>>>> +	}
>>>>> +
>>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>>> +				 val, val & BIT(0), 200, 2000);
>>>> what are you polling here for?
>>>
>>>
>>> This is not different than existing code. I don't understand why you are
>>> commenting on something which is already there.
>>
>> Which code are you referring to?
> 
> To the existing vpu33 which had Reviewed-by: Vikash Garodia
> <quic_vgarodia@quicinc.com>
> 
> You understand that everything here is the same, everything is a copy
> while adding just few more things?
> 
> My patch is not doing in this respect anything different that what you
> reviewed.
> 

It seems to have been missed in vpu33 power off sequence as well and should
be fixed.

Still, as mentioned earlier as well, your reference should be
HPG/downstream driver of SM8750 not the previous generation (SM8650).

Thanks,
Dikshita

> 
>>
>> You are not setting AON_WRAPPER_MVP_NOC_LPI_CONTROL and polling for its status.
> 
> True, neither old reviewed code has done. I am not changing or fixing
> any existing logic, I am only adding new clocks and resets.
> 
>>
>> The current code is incomplete and missing several steps.
> 
> Current you mean what was:
> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ?
> 
>> Please review and provide a corrected version.
>>
>>
>>>
>>>>> +	if (ret)
>>>>> +		goto disable_power;
>>>>> +
>>>>> +	/* set MNoC to low power, set PD_NOC_QREQ (bit 0) */
>>>> Could you share the reference for this sqeunece, this looks half-cooked.
>>>> Would recommend following Hardware programmin guide(HPG) for this.
>>>
>>>
>>> Why? Look at existing code. It's the same.
>>
>>
>> Which existing code? Please be specific.
> 
> 
> Existing upstream VPU33 which this builts on top of. And that existing
> upstream VPU33 was Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> 
>> I don't think you referred to downstream code for this, because I see a lot
>> of missing pieces here.
>>
>>
>>>
>>> I think I responded to all your comments - it barely possible to spot
>>> them in the quote.
>>>
>>
>>
>> No, you have missed some of the later comments. Since the code is snipped,
>> I can’t point out those comments here.
> 
> 
> It's impossible to find them in the original response.
> 
> 
> Best regards,
> Krzysztof

On 17/07/2025 12:50, Dikshita Agarwal wrote:
>>>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>>>> +					 val, val & 0x400000, 2000, 20000);
>>>>>> +		if (ret)
>>>>>> +			goto disable_power;
>>>>>> +	}
>>>>>> +
>>>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>>>> +				 val, val & BIT(0), 200, 2000);
>>>>> what are you polling here for?
>>>>
>>>>
>>>> This is not different than existing code. I don't understand why you are
>>>> commenting on something which is already there.
>>>
>>> Which code are you referring to?
>>
>> To the existing vpu33 which had Reviewed-by: Vikash Garodia
>> <quic_vgarodia@quicinc.com>
>>
>> You understand that everything here is the same, everything is a copy
>> while adding just few more things?
>>
>> My patch is not doing in this respect anything different that what you
>> reviewed.
>>
> 
> It seems to have been missed in vpu33 power off sequence as well and should
> be fixed.
> 
> Still, as mentioned earlier as well, your reference should be
> HPG/downstream driver of SM8750 not the previous generation (SM8650).

Yes and partially no, because we write upstream code matching or
extending existing upstream driver. As you said earlier, downstream is
not the truth always:

"That shouldn’t be the case. The downstream design is different, which
is why the driver requires the above code to move the GDSC"

so here I built on top of SM8650 and re-iterate whatever mistakes are
there. The best if someone fixes VPU33 and then I rebase on top,
re-using fixed code as my base.

Best regards,
Krzysztof

On 7/17/2025 4:24 PM, Krzysztof Kozlowski wrote:
> On 17/07/2025 12:50, Dikshita Agarwal wrote:
>>>>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>>>>> +					 val, val & 0x400000, 2000, 20000);
>>>>>>> +		if (ret)
>>>>>>> +			goto disable_power;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>>>>> +				 val, val & BIT(0), 200, 2000);
>>>>>> what are you polling here for?
>>>>>
>>>>>
>>>>> This is not different than existing code. I don't understand why you are
>>>>> commenting on something which is already there.
>>>>
>>>> Which code are you referring to?
>>>
>>> To the existing vpu33 which had Reviewed-by: Vikash Garodia
>>> <quic_vgarodia@quicinc.com>
>>>
>>> You understand that everything here is the same, everything is a copy
>>> while adding just few more things?
>>>
>>> My patch is not doing in this respect anything different that what you
>>> reviewed.
>>>
>>
>> It seems to have been missed in vpu33 power off sequence as well and should
>> be fixed.
>>
>> Still, as mentioned earlier as well, your reference should be
>> HPG/downstream driver of SM8750 not the previous generation (SM8650).
> 
> Yes and partially no, because we write upstream code matching or
> extending existing upstream driver. As you said earlier, downstream is
> not the truth always:

You're writing the power sequence for a new generation, so referencing the
previous generation is totally wrong. Power sequences can vary between
generations — that's precisely why HPG exists.

I've already pointed this out multiple times, but let me reiterate one last
time:
The current power sequence code is incomplete.
Copying the SM8650 code to SM8750 is not appropriate — it's the wrong
reference.

Regards,
Dikshita

> 
> "That shouldn’t be the case. The downstream design is different, which
> is why the driver requires the above code to move the GDSC"
> 
> so here I built on top of SM8650 and re-iterate whatever mistakes are
> there. The best if someone fixes VPU33 and then I rebase on top,
> re-using fixed code as my base.
> > Best regards,
> Krzysztof

On 17/07/2025 14:22, Dikshita Agarwal wrote:
> 
> 
> On 7/17/2025 4:24 PM, Krzysztof Kozlowski wrote:
>> On 17/07/2025 12:50, Dikshita Agarwal wrote:
>>>>>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>>>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>>>>>> +					 val, val & 0x400000, 2000, 20000);
>>>>>>>> +		if (ret)
>>>>>>>> +			goto disable_power;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>>>>>> +				 val, val & BIT(0), 200, 2000);
>>>>>>> what are you polling here for?
>>>>>>
>>>>>>
>>>>>> This is not different than existing code. I don't understand why you are
>>>>>> commenting on something which is already there.
>>>>>
>>>>> Which code are you referring to?
>>>>
>>>> To the existing vpu33 which had Reviewed-by: Vikash Garodia
>>>> <quic_vgarodia@quicinc.com>
>>>>
>>>> You understand that everything here is the same, everything is a copy
>>>> while adding just few more things?
>>>>
>>>> My patch is not doing in this respect anything different that what you
>>>> reviewed.
>>>>
>>>
>>> It seems to have been missed in vpu33 power off sequence as well and should
>>> be fixed.
>>>
>>> Still, as mentioned earlier as well, your reference should be
>>> HPG/downstream driver of SM8750 not the previous generation (SM8650).
>>
>> Yes and partially no, because we write upstream code matching or
>> extending existing upstream driver. As you said earlier, downstream is
>> not the truth always:
> 
> You're writing the power sequence for a new generation, so referencing the
> previous generation is totally wrong. Power sequences can vary between


No. I am extending existing source code in hopefully compatible or
matching style.

We do not follow here downstream approaches of reimplementing everything
from scratch on every new generation.

> generations — that's precisely why HPG exists.
> 
> I've already pointed this out multiple times, but let me reiterate one last
> time:
> The current power sequence code is incomplete.
> Copying the SM8650 code to SM8750 is not appropriate — it's the wrong
> reference.
You only pointed out missing AON_WRAPPER_MVP_NOC_LPI_CONTROL, so
"multiple times" is not accurate.


Best regards,
Krzysztof

On 7/17/2025 4:24 PM, Krzysztof Kozlowski wrote:
> On 17/07/2025 12:50, Dikshita Agarwal wrote:
>>>>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>>>>> +					 val, val & 0x400000, 2000, 20000);
>>>>>>> +		if (ret)
>>>>>>> +			goto disable_power;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>>>>> +				 val, val & BIT(0), 200, 2000);
>>>>>> what are you polling here for?
>>>>>
>>>>>
>>>>> This is not different than existing code. I don't understand why you are
>>>>> commenting on something which is already there.
>>>>
>>>> Which code are you referring to?
>>>
>>> To the existing vpu33 which had Reviewed-by: Vikash Garodia
>>> <quic_vgarodia@quicinc.com>
>>>
>>> You understand that everything here is the same, everything is a copy
>>> while adding just few more things?
>>>
>>> My patch is not doing in this respect anything different that what you
>>> reviewed.
>>>
>>
>> It seems to have been missed in vpu33 power off sequence as well and should
>> be fixed.
>>
>> Still, as mentioned earlier as well, your reference should be
>> HPG/downstream driver of SM8750 not the previous generation (SM8650).
> 
> Yes and partially no, because we write upstream code matching or
> extending existing upstream driver. As you said earlier, downstream is
> not the truth always:
> 
> "That shouldn’t be the case. The downstream design is different, which
> is why the driver requires the above code to move the GDSC"
> 
> so here I built on top of SM8650 and re-iterate whatever mistakes are
> there. The best if someone fixes VPU33 and then I rebase on top,
> re-using fixed code as my base.

You have mixed different comments made earlier.

1. Downstream GDSCs are still in HW_CTRL mode, while upstream GDSCs are migrated
to HW_CTRL_TRIGGER. This does not need a fix in SM8650, but in the
"iris_vpu35_power_on_hw" which you have added in this patch for SM8750.

2. Register write "AON_WRAPPER_MVP_NOC_LPI_CONTROL" with 0x1 is needed on both
SM8650 and SM8750, before polling AON_WRAPPER_MVP_NOC_LPI_STATUS in
"iris_vpu35_power_off_hw" function.

I can soon submit a patch to fix SM8650 with the missing register write, but i
do not see a need to wait for it to continue your development on SM8750.

Regards,
Vikash

On 17/07/2025 14:02, Vikash Garodia wrote:
> 
> On 7/17/2025 4:24 PM, Krzysztof Kozlowski wrote:
>> On 17/07/2025 12:50, Dikshita Agarwal wrote:
>>>>>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>>>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>>>>>> +					 val, val & 0x400000, 2000, 20000);
>>>>>>>> +		if (ret)
>>>>>>>> +			goto disable_power;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>>>>>> +				 val, val & BIT(0), 200, 2000);
>>>>>>> what are you polling here for?
>>>>>>
>>>>>>
>>>>>> This is not different than existing code. I don't understand why you are
>>>>>> commenting on something which is already there.
>>>>>
>>>>> Which code are you referring to?
>>>>
>>>> To the existing vpu33 which had Reviewed-by: Vikash Garodia
>>>> <quic_vgarodia@quicinc.com>
>>>>
>>>> You understand that everything here is the same, everything is a copy
>>>> while adding just few more things?
>>>>
>>>> My patch is not doing in this respect anything different that what you
>>>> reviewed.
>>>>
>>>
>>> It seems to have been missed in vpu33 power off sequence as well and should
>>> be fixed.
>>>
>>> Still, as mentioned earlier as well, your reference should be
>>> HPG/downstream driver of SM8750 not the previous generation (SM8650).
>>
>> Yes and partially no, because we write upstream code matching or
>> extending existing upstream driver. As you said earlier, downstream is
>> not the truth always:
>>
>> "That shouldn’t be the case. The downstream design is different, which
>> is why the driver requires the above code to move the GDSC"
>>
>> so here I built on top of SM8650 and re-iterate whatever mistakes are
>> there. The best if someone fixes VPU33 and then I rebase on top,
>> re-using fixed code as my base.
> 
> You have mixed different comments made earlier.


I did not mix. I used them here to show how pointless arguments you keep
making instead of focusing on technical aspects.

Once you say that, other place you say something else.

> 
> 1. Downstream GDSCs are still in HW_CTRL mode, while upstream GDSCs are migrated
> to HW_CTRL_TRIGGER. This does not need a fix in SM8650, but in the
> "iris_vpu35_power_on_hw" which you have added in this patch for SM8750.

No one discusses this.

> 
> 2. Register write "AON_WRAPPER_MVP_NOC_LPI_CONTROL" with 0x1 is needed on both
> SM8650 and SM8750, before polling AON_WRAPPER_MVP_NOC_LPI_STATUS in
> "iris_vpu35_power_off_hw" function.
> 
> I can soon submit a patch to fix SM8650 with the missing register write, but i

Great!

> do not see a need to wait for it to continue your development on SM8750.


I am not sure if you both understand how upstream development works. We
reduce to a reasonable minimum duplicated codes and different solutions,
so that's why my code is a copy of existing code plus new things for SM8750.

The goal of upstream is not to implement SM8750 completely different.
Please switch downstream approach to above re-usage approach.

And that's why your fix is important because I am going to copy exactly
that part of code and I should not come with different code.


Best regards,
Krzysztof