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

Dmitry Baryshkov
2025-10-08T19:25:45Z

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