linux-kernel - Re: [PATCH 7/8] media: iris: Introduce vpu ops for vpu4 with necessary hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <68686586-f161-c6c6-cd3f-c5eb87e33954@quicinc.com>
Date: Mon, 29 Sep 2025 11:15:37 +0530
From: Vishnu Reddy <quic_bvisredd@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        Vikash Garodia
	<vikash.garodia@....qualcomm.com>,
        Dikshita Agarwal
	<dikshita.agarwal@....qualcomm.com>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        Bryan O'Donoghue <bod@...nel.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-arm-msm@...r.kernel.org>, <linux-media@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/8] media: iris: Introduce vpu ops for vpu4 with
 necessary hooks



On 9/25/2025 2:48 PM, Konrad Dybcio wrote:
> On 9/25/25 1:14 AM, Vikash Garodia wrote:
>> Add power sequence for vpu4 by reusing from previous generation wherever
>> possible. Hook up vpu4 op with vpu4 specific implemtation or resue from
>> earlier generation wherever feasible, like clock calculation in this
>> case.
>>
>> Co-developed-by: Vishnu Reddy <quic_bvisredd@...cinc.com>
>> Signed-off-by: Vishnu Reddy <quic_bvisredd@...cinc.com>
>> Signed-off-by: Vikash Garodia <vikash.garodia@....qualcomm.com>
>> ---
> 
> [...]
> 
>> +#include <linux/iopoll.h>
>> +#include <linux/reset.h>
>> +#include "iris_instance.h"
>> +#include "iris_vpu_common.h"
>> +#include "iris_vpu_register_defines.h"
>> +
>> +#define WRAPPER_EFUSE_MONITOR			(WRAPPER_BASE_OFFS + 0x08)
>> +#define AON_WRAPPER_MVP_NOC_RESET_SYNCRST	(AON_MVP_NOC_RESET + 0x08)
>> +#define CPU_CS_APV_BRIDGE_SYNC_RESET		(CPU_BASE_OFFS + 0x174)
>> +#define DISABLE_VIDEO_APV_BIT			BIT(27)
>> +#define DISABLE_VIDEO_VPP1_BIT			BIT(28)
>> +#define DISABLE_VIDEO_VPP0_BIT			BIT(29)
>> +#define CORE_CLK_HALT				BIT(0)
>> +#define APV_CLK_HALT				BIT(1)
>> +#define CORE_PWR_ON				BIT(1)
>> +
>> +static int iris_vpu4x_genpd_set_hwmode(struct iris_core *core, bool hw_mode)
>> +{
>> +	u32 value = readl(core->reg_base + WRAPPER_EFUSE_MONITOR);
> 
> I think this could use some explanations.
> 
> I'll go ahead and assume that the eFuse tells us that parts of the
> IP are disables (hopefully not all three at once.. we shouldn't
> advertise the v4l2 device then, probably)
> 
> You read back the fuse register a lot, even though I presume it's not
> supposed to change at runtime. How about we add:
> 
> bool vpp0_fused_off
> bool vpp1_fused_off
> bool apv_fused_off
> 
> instead?
> 

  Hi Konrad, Thanks for your review and suggestion.

  The poweroff and poweron ops will be called in each test. There is no
  ops available that called onetime only to cache these values.
  And, to create any variable, Need to add as static global in this file
  because these are specific to this platform and I feel it's not
  recommended code to add into any common structures as a member.

  Do you have any suggestion from your side how this can be do it in a
  simple way?

> [...]
> 
>> +	if (!(value & DISABLE_VIDEO_VPP0_BIT)) {
>> +		ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs
>> +						[IRIS_VPP0_HW_POWER_DOMAIN]);
> 
> Maybe the iris_en/disable_foo functions could get a wrapper like:
> 
> int iris_enable_power_domains_if(core, pd_devs[IRIS_VPP0_HW_POWER_DOMAIN],
> 				 !foo->vpp0_fused_off)
> 
> I'm not super sure about it, but that's something to consider
> 
> [...]
> 
>> +	readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN, value,
>> +			   value & 0x7103, 2000, 20000);
> 
> That's a nice magic number.. but what does it mean?
> 

  ACK, Will add macro definitions for these numbers.

> [...]
> 
>> +	writel(0x070103, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
>> +	readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
>> +			   value, value == 0x070103, 200, 2000);
> 
> That's a slightly different magic number, but it's oddly similar to
> the one above
> 

  ACK.

  Thanks and regards,
  Vishnu Reddy