Message-ID: <3a87c37b-b392-598a-736f-bb01e4c311e1@quicinc.com>
Date: Thu, 17 Jul 2025 16:20:57 +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/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@...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.
> 

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