Re: [PATCH 3/3] media: iris: Add support for SM8750 (VPU v3.5)

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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