Re: [PATCH v5 12/14] media: iris: Add platform data for glymur

[Dmitry Baryshkov]

On Wed, May 13, 2026 at 10:31:55PM +0530, Vikash Garodia wrote:
> 
> On 5/13/2026 9:33 PM, Dmitry Baryshkov wrote:
> > On Wed, May 13, 2026 at 08:00:39PM +0530, Vikash Garodia wrote:
> > > 
> > > 
> > > On 5/13/2026 7:47 PM, Dmitry Baryshkov wrote:
> > > > On Mon, May 11, 2026 at 09:45:01PM +0530, Vishnu Reddy wrote:
> > > > > 
> > > > > On 5/9/2026 2:35 AM, Dmitry Baryshkov wrote:
> > > > > > On Sat, May 09, 2026 at 12:30:01AM +0530, Vishnu Reddy wrote:
> > > > > > > On glymur platform, the iris core shares most properties with the
> > > > > > > iris core on the SM8550 platform. The major difference is that glymur
> > > > > > > integrates two codec cores (vcodec0 and vcodec1), while SM8550 has only
> > > > > > > one. Add glymur specific platform data, reusing SM8550 definitions
> > > > > > > wherever applicable.
> > > > > > This leave me in confusion. Having two cores, each with its own set of
> > > > > > clocks and pm domains, I'd have expected that each core scales
> > > > > > independently. I.e. if the load is pushed to the core0, it requires
> > > > > > core0 clocks to go higher (while core1 clocks can stay at the low freq).
> > > > > > Or, at least, the clocks would be set to the frequency corresponding to
> > > > > > the max of the workloads (if for some reason the cores should stay in
> > > > > > sync).
> > > > > > 
> > > > > > However, I don't see it in the code. All clocks and all power domains
> > > > > > seem do be scaled using the common workload. If my assumptions were not
> > > > > > correct, please explain it in the commit message.
> > > > > 
> > > > > The OPP core logic sets the rpmhpd level and clock rate based on the OPP table
> > > > > defined in the DT node, where the clock frequency and power rail level are
> > > > > tightly coupled together. Since vcodec0 and vcodec1 share the same power rails,
> > > > > independently scaling one clock high while keeping the other low is not
> > > > > straightforward within this OPP framework.
> > > > > 
> > > > > Do you have any suggestion on how best to handle per core independent clock
> > > > > scaling within these constraints?
> > > > 
> > > > This would require more plumbing and driver changes, but:
> > > > 
> > > > 	iris: video-codec@foo {
> > > > 		compatible = "qcom,glymur-iris",
> > > > 		clocks = <only-core-clocks>;
> > > > 		resets = <only-core-resets>;
> > > > 
> > > > 		/* or core@0 */
> > > > 		codec@0 {
> > > > 			clocks, resets, power-domains;
> > > > 			operating-points-v2 = <&iris_opp_table>
> > > > 		};
> > > > 
> > > > 		/* or core@1 */
> > > > 		codec@1 {
> > > > 			clocks, resets, power-domains;
> > > > 			operating-points-v2 = <&iris_opp_table>
> > > > 		};
> > > > 
> > > > 		iris_opp_table: opp-table {
> > > > 			compatible = "operating-points-v2"
> > > > 		};
> > > > 	};
> > > > 
> > > 
> > > clock source "video_cc_mvs0_clk_src" is common for both the cores. It would
> > > not matter if core0 is scaled for a specific workload and core1 is scaled
> > > for different (let say lower), as the common PLL would always generate the
> > > higher of them.
> > > 
> > > Infact, going with the approach of exclusive scaling would be an issue here.
> > > The later core scaling command would bring down/up the corner for other
> > > core, and could lead to under/over-voting.
> > 
> > Are the dividers between mvs0_clk_src and the branch clocks really R/O
> > in the hardware? Can they be scaled to account for the different
> > workloads?
> 
> dividers div ratio is pre-fixed in nature, does not vary with workload.
> Again, you need to look at the source clock, rather than the ones at
> different core. Even if they are scaled differently, either in software or
> hardware (assume for now, hw does), the source would always pick the higher
> of the scaled frequency corner.
> 
> > The commit message should capture the details of the
> > interaction between cores:
> > - Can either of them be turned off, while retaining the other one
> >    running?
> > - Can either of them run at a different frequency than the other one?
> > - etc.
> 
> Lets capture these details in the patch which enables the power sequence for
> the dual core.

Agree, thank you.

> 
> > 
> > > > > 
> > > > > > > Reviewed-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >    drivers/media/platform/qcom/iris/Makefile          |  1 +
> > > > > > >    .../platform/qcom/iris/iris_platform_common.h      |  5 ++
> > > > > > >    .../media/platform/qcom/iris/iris_platform_gen2.c  | 99 ++++++++++++++++++++++
> > > > > > >    .../platform/qcom/iris/iris_platform_glymur.c      | 97 +++++++++++++++++++++
> > > > > > >    .../platform/qcom/iris/iris_platform_glymur.h      | 17 ++++
> > > > > > >    drivers/media/platform/qcom/iris/iris_probe.c      |  4 +
> > > > > > >    6 files changed, 223 insertions(+)
> > > > > > > 
> > > > 
> > > 
> > 
> 

-- 
With best wishes
Dmitry