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

Dmitry Baryshkov
2025-10-09T16:20:35Z

On Thu, Oct 09, 2025 at 05:49:45PM +0200, Konrad Dybcio wrote:
> On 10/9/25 5:00 PM, Dmitry Baryshkov wrote:
> > On Thu, Oct 09, 2025 at 11:19:31AM +0200, Konrad Dybcio wrote:
> >> On 10/8/25 9:25 PM, Dmitry Baryshkov wrote:
> >>> 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?
> >>
> >> [...]
> >>
> >>>>> + 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?
> >>
> >> Digging in techpack/video, I found:
> >>
> >> commit c543f70aca8d40c593b8ad342d42e913a422c552
> >> Author: Priyanka Gujjula <pgujjula@codeaurora.org>
> >> Date: Fri Feb 14 13:38:31 2020 +0530
> >>
> >> msm: vidc: Skip AON register programming for lagoon
> >>
> >> AON register programming is used to set NOC to low
> >> power mode during IRIS2 power off sequence. However
> >> AON register memory map is not applicable and hence
> >> skipping AON register programming for lagoon.
> >>
> >> Change-Id: Ib63248d118ed9fecfa5fa87925e8f69625dc1ba8
> >> Signed-off-by: Priyanka Gujjula <pgujjula@codeaurora.org>
> >>
> >>
> >> lagoon being a downstream codename of the aforementioned sm6350
> >>
> >> Meaning yeah it's bus topology.. so I think an if-statement within
> >> a common flow would be what we want here..
> >>
> >> perhaps
> >>
> >> if (core->iris_platform_data->num_vpp_pipe == 1)
> >>
> >> just like venus and downstream do for the most part, and kick the
> >> can down the road.. In an unlikely event someone decides to implement
> >> IRIS2_1 on a brand new SoC, we can delay our worries..
> >
> > But this function is being used for VPU3 devices too, if I'm not
> > mistaken. So it becomes a bit ugly... Also I'm not sure if this is
> > really related to a num of VPP pipes or the CVP.
>
> Oh.. hm.. maybe we can add a .aon_shutdown op? I'm not sure how
> to proceed either

I think, if you want to merge those codepaths, I can add .no_aon flag to
platform data, unless maintainers disagree with this proposeal.

--
With best wishes
Dmitry