Message-ID: <a6dbca7e-4d49-49a6-987c-8cd587501c98@linaro.org>
Date: Thu, 17 Jul 2025 11:34:11 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Dikshita Agarwal <quic_dikshita@...cinc.com>,
 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 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@...cinc.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@...cinc.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@...cinc.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

Powered by blists - more mailing lists