linux-kernel - Re: [PATCH 8/8] media: iris: enable support for SC7280 platform

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <gdui5coiazt5ynuz6c6mr5slau33vuewr55vx53m5zzyi537at@cs7dbgirijuw>
Date: Wed, 8 Oct 2025 22:25:39 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: 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>,
        linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 8/8] media: iris: enable support for SC7280 platform

On Wed, Oct 08, 2025 at 10:26:02AM +0200, Konrad Dybcio wrote:
> On 10/8/25 6:33 AM, Dmitry Baryshkov wrote:
> > As a part of migrating code from the old Venus driver to the new Iris
> > one, add support for the SC7280 platform. It is very similar to SM8250,
> > but it (currently) uses no reset controls (there is an optional
> > GCC-generated reset, it will be added later) and no AON registers
> > region. The Venus driver names this platform "IRIS2_1", so the ops in
> 
> Which we've learnt in the past is "IRIS2, 1-pipe"

Well, I'm open for better suggestions. iris_vpu2_no_aon_ops?

> 
> > the driver are also now called iris_vpu21_ops.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
> > ---
> >  .../platform/qcom/iris/iris_platform_common.h      |   3 +
> >  .../media/platform/qcom/iris/iris_platform_gen1.c  |  66 +++++++++++
> >  drivers/media/platform/qcom/iris/iris_probe.c      |   4 +
> >  drivers/media/platform/qcom/iris/iris_vpu2.c       | 130 +++++++++++++++++++++
> >  drivers/media/platform/qcom/iris/iris_vpu_common.h |   1 +
> >  5 files changed, 204 insertions(+)
> > 
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> > index 104ff38219e30e6d52476d44b54338c55ef2ca7b..36e33eb05a6918de590feca37b41c07a92e9c434 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> > @@ -42,6 +42,7 @@ enum pipe_type {
> >  };
> >  
> >  extern const struct iris_platform_data qcs8300_data;
> > +extern const struct iris_platform_data sc7280_data;
> >  extern const struct iris_platform_data sm8250_data;
> >  extern const struct iris_platform_data sm8550_data;
> >  extern const struct iris_platform_data sm8650_data;
> > @@ -50,7 +51,9 @@ extern const struct iris_platform_data sm8750_data;
> >  enum platform_clk_type {
> >  	IRIS_AXI_CLK, /* AXI0 in case of platforms with multiple AXI clocks */
> >  	IRIS_CTRL_CLK,
> > +	IRIS_AHB_CLK,
> 
> Interestingly, 8250 also has an AHB clock, but the clock driver keeps it
> always-on..
> 
> >  	IRIS_HW_CLK,
> > +	IRIS_HW_AXI_CLK,
> 
> This exists on SC7280 and SM6350, perhaps as a result of the bus topology

These parts are still a bit unclear to me (as well as resets usage for
sc7280).

> 
> >  	IRIS_AXI1_CLK,
> >  	IRIS_CTRL_FREERUN_CLK,
> >  	IRIS_HW_FREERUN_CLK,
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_gen1.c b/drivers/media/platform/qcom/iris/iris_platform_gen1.c
> > index 2b3b8bd00a6096acaae928318d9231847ec89855..d5288a71a6a8289e5ecf69b6f38231500f2bf8b4 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_gen1.c
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_gen1.c
> > @@ -364,3 +364,69 @@ const struct iris_platform_data sm8250_data = {
> >  	.enc_ip_int_buf_tbl = sm8250_enc_ip_int_buf_tbl,
> >  	.enc_ip_int_buf_tbl_size = ARRAY_SIZE(sm8250_enc_ip_int_buf_tbl),
> >  };
> > +
> > +static const struct bw_info sc7280_bw_table_dec[] = {
> > +	{ ((3840 * 2160) / 256) * 60, 1896000, },
> > +	{ ((3840 * 2160) / 256) * 30,  968000, },
> > +	{ ((1920 * 1080) / 256) * 60,  618000, },
> > +	{ ((1920 * 1080) / 256) * 30,  318000, },
> > +};
> > +
> > +static const char * const sc7280_opp_pd_table[] = { "cx" };
> 
> Wonder why this is different..
> 
> Oh, I can bet good money SM8250's Venus isn't fed off of MX alone..
> 
> Let's check the sauce..
> 
> It was always supposed to be M*MC*X with MX just for the VIDEO_CC
> PLLs..
> 
> [...]
> 
> > +/*
> > + * This is the same as iris_vpu_power_off_controller except
> > + * AON_WRAPPER_MVP_NOC_LPI_CONTROL / AON_WRAPPER_MVP_NOC_LPI_STATUS programming
> > + * and with added IRIS_AHB_CLK handling
> > + */
> > +static int iris_vpu21_power_off_controller(struct iris_core *core)
> 
> This is 1 : 1 the existing sm8250 code except...> +{
> > +	u32 val = 0;
> > +	int ret;
> > +
> > +	writel(MSK_SIGNAL_FROM_TENSILICA | MSK_CORE_POWER_ON, core->reg_base + CPU_CS_X2RPMH);
> > +
> > +	writel(REQ_POWER_DOWN_PREP, core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_CONTROL);
> > +
> > +	ret = readl_poll_timeout(core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_STATUS,
> > +				 val, val & BIT(0), 200, 2000);
> > +	if (ret)
> > +		goto disable_power;
> > +
> > +	writel(0x0, core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_CONTROL);
> > +
> > +	ret = readl_poll_timeout(core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_STATUS,
> > +				 val, val == 0, 200, 2000);
> > +	if (ret)
> > +		goto disable_power;
> > +
> > +	writel(CTL_AXI_CLK_HALT | CTL_CLK_HALT,
> > +	       core->reg_base + WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG);
> > +	writel(RESET_HIGH, core->reg_base + WRAPPER_TZ_QNS4PDXFIFO_RESET);
> > +	writel(0x0, core->reg_base + WRAPPER_TZ_QNS4PDXFIFO_RESET);
> > +	writel(0x0, core->reg_base + WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG);
> > +
> > +disable_power:
> > +	iris_disable_unprepare_clock(core, IRIS_AHB_CLK);
> 
> ..for this line

Not only. You missed the absense of AON_WRAPPER_MVP_NOC_LPI_CONTROL /
AON_WRAPPER_MVP_NOC_LPI_STATUS. Which in theory can become a flag in
iris_platform_data.

> 
> but this could be added to that one instead, since both clk APIs and the
> Iris wrappers around it are happy to consume a null pointer (funnily
> enough this one returns !void and is never checked)
> 
> similar story for other func additions

In fact, initially I had them merged, but then I couldn't find an
elegant way to handle AON regs. I can squash them back, if that's the
consensus. Any idea regarding AON regs?

-- 
With best wishes
Dmitry