Message-ID: <1c5df071-7000-ab45-dbc6-4384d883ba24@quicinc.com>
Date: Thu, 17 Jul 2025 13:07:14 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Vikash Garodia
	<quic_vgarodia@...cinc.com>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        "Bryan
 O'Donoghue" <bryan.odonoghue@...aro.org>,
        Mauro Carvalho Chehab
	<mchehab@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Philipp Zabel
	<p.zabel@...gutronix.de>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] media: iris: Add support for SM8750 (VPU v3.5)



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

Powered by blists - more mailing lists