Re: [PATCH v5 13/14] arm64: dts: qcom: glymur: Add iris video node

[Vishnu Reddy <busanna.reddy@oss.qualcomm.com>]

On 5/13/2026 7:25 PM, Dmitry Baryshkov wrote:
> On Sat, May 09, 2026 at 10:26:49PM +0530, Vishnu Reddy wrote:
>> On 5/9/2026 12:57 AM, Dmitry Baryshkov wrote:
>>> On Sat, May 09, 2026 at 12:30:02AM +0530, Vishnu Reddy wrote:
>>>> Add iris video codec to glymur SoC, which comes with significantly
>>>> different powering up sequence than previous platforms, thus different
>>>> clocks and resets.
>>>>
>>>> Reviewed-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
>>>> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/glymur.dtsi | 118 +++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 118 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/glymur.dtsi b/arch/arm64/boot/dts/qcom/glymur.dtsi
>>>> index f23cf81ddb77..c47443174f97 100644
>>>> --- a/arch/arm64/boot/dts/qcom/glymur.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/glymur.dtsi
>>>> @@ -13,6 +13,7 @@
>>>>  #include <dt-bindings/interconnect/qcom,glymur-rpmh.h>
>>>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>  #include <dt-bindings/mailbox/qcom-ipcc.h>
>>>> +#include <dt-bindings/media/qcom,glymur-iris.h>
>>>>  #include <dt-bindings/phy/phy-qcom-qmp.h>
>>>>  #include <dt-bindings/power/qcom,rpmhpd.h>
>>>>  #include <dt-bindings/power/qcom-rpmpd.h>
>>>> @@ -4163,6 +4164,123 @@ usb_mp: usb@a400000 {
>>>>  			status = "disabled";
>>>>  		};
>>>>  
>>>> +		iris: video-codec@aa00000 {
>>>> +			compatible = "qcom,glymur-iris";
>>>> +			reg = <0x0 0xaa00000 0x0 0xf0000>;
>>>> +
>>>> +			clocks = <&gcc GCC_VIDEO_AXI0_CLK>,
>>>> +				 <&videocc VIDEO_CC_MVS0C_CLK>,
>>>> +				 <&videocc VIDEO_CC_MVS0_CLK>,
>>>> +				 <&gcc GCC_VIDEO_AXI0C_CLK>,
>>>> +				 <&videocc VIDEO_CC_MVS0C_FREERUN_CLK>,
>>>> +				 <&videocc VIDEO_CC_MVS0_FREERUN_CLK>,
>>>> +				 <&gcc GCC_VIDEO_AXI1_CLK>,
>>>> +				 <&videocc VIDEO_CC_MVS1_CLK>,
>>>> +				 <&videocc VIDEO_CC_MVS1_FREERUN_CLK>;
>>>> +			clock-names = "iface",
>>>> +				      "core",
>>>> +				      "vcodec0_core",
>>>> +				      "iface1",
>>> I first wrote the comment regarding resets. But the clocks seem to have
>>> the same pattern. It's not just "iface1" clock. It's the clock for one
>>> of the cores. And there is another clock for another core. Please make
>>> that nicely named.
>> In v1, I used iface_ctrl to reflect the clock purpose, but received the
>> feedback [1] to align with the iface1 naming convention used on earlier
>> platforms.
>>
>> [1] https://lore.kernel.org/all/20260414-lush-reindeer-of-storm-bbe918@quoll/
> I'd also dislike the iface_ctrl, it doesn't say anything.
>
> I'd suggest having vcodec0_iface / vcodec1_iface for vcodecs and just
> iface for the core AXI clock.

These changes would also need to be reflected in the DT binding document. The
current names are carried over from earlier platform targets as legacy names,
and in v1 Krzysztof suggested [1] using an earlier series as a base and building
on top of it — so I followed the same approach.
[1] https://lore.kernel.org/all/20260414-lush-reindeer-of-storm-bbe918@quoll/

Krzysztof, do you have any input on renaming these names?

>>>> +				      "core_freerun",
>>>> +				      "vcodec0_core_freerun",
>>>> +				      "iface2",
>>>> +				      "vcodec1_core",
>>>> +				      "vcodec1_core_freerun";
>>>> +
>>>> +			dma-coherent;
>>>> +
>>>> +			interconnects = <&hsc_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
>>>> +					 &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>>>> +					<&mmss_noc MASTER_VIDEO QCOM_ICC_TAG_ALWAYS
>>>> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
>>>> +			interconnect-names = "cpu-cfg",
>>>> +					     "video-mem";
>>>> +
>>>> +			interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
>>>> +
>>>> +			iommus = <&apps_smmu 0x1940 0x0>,
>>>> +				 <&apps_smmu 0x1943 0x0>,
>>>> +				 <&apps_smmu 0x1944 0x0>,
>>>> +				 <&apps_smmu 0x19e0 0x0>;
>>>> +
>>>> +			iommu-map = <IOMMU_FID_IRIS_FIRMWARE &apps_smmu 0x19e2 0x1>;
>>>> +
>>>> +			memory-region = <&video_mem>;
>>>> +
>>>> +			operating-points-v2 = <&iris_opp_table>;
>>>> +
>>>> +			power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>,
>>>> +					<&videocc VIDEO_CC_MVS0_GDSC>,
>>>> +					<&rpmhpd RPMHPD_MXC>,
>>>> +					<&rpmhpd RPMHPD_MMCX>,
>>>> +					<&videocc VIDEO_CC_MVS1_GDSC>;
>>>> +			power-domain-names = "venus",
>>>> +					     "vcodec0",
>>>> +					     "mxc",
>>>> +					     "mmcx",
>>>> +					     "vcodec1";
>>>> +
>>>> +			resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>,
>>>> +				 <&gcc GCC_VIDEO_AXI0C_CLK_ARES>,
>>>> +				 <&videocc VIDEO_CC_MVS0C_FREERUN_CLK_ARES>,
>>>> +				 <&videocc VIDEO_CC_MVS0_FREERUN_CLK_ARES>,
>>>> +				 <&gcc GCC_VIDEO_AXI1_CLK_ARES>,
>>>> +				 <&videocc VIDEO_CC_MVS1_FREERUN_CLK_ARES>;
>>>> +			reset-names = "bus0",
>>>> +				      "bus1",
>>> The names of the resets suggest that there is single "common" reset and
>>> then one reset per each core.
>> Two resets for controller and two resets for each per vcodec core.
> The same, vcodec0_bus, vcodec1_bus, please.
>
>>>> +				      "core",
>>>> +				      "vcodec0_core",
>>>> +				      "bus2",
>>>> +				      "vcodec1_core";
>>> Are there two codecs? Or are there two cores? Your naming suggests the
>>> former case.
>> Two vcodec cores.
>>
>>>> +
>>>> +			/*
>>>> +			 * IRIS firmware is signed by vendors, only
>>>> +			 * enable on boards where the proper signed firmware
>>>> +			 * is available.
>>>> +			 */
>>>> +			status = "disabled";
>>>> +
>>>> +			iris_opp_table: opp-table {
>>>> +				compatible = "operating-points-v2";
>>>> +
>>>> +				opp-240000000 {
>>>> +					opp-hz = /bits/ 64 <240000000 240000000 360000000>;
>>>> +					required-opps = <&rpmhpd_opp_svs>,
>>>> +							<&rpmhpd_opp_low_svs>;
>>>> +				};
>>>> +
>>>> +				opp-338000000 {
>>>> +					opp-hz = /bits/ 64 <338000000 338000000 507000000>;
>>>> +					required-opps = <&rpmhpd_opp_svs>,
>>>> +							<&rpmhpd_opp_svs>;
>>>> +				};
>>>> +
>>>> +				opp-366000000 {
>>>> +					opp-hz = /bits/ 64 <366000000 366000000 549000000>;
>>>> +					required-opps = <&rpmhpd_opp_svs_l1>,
>>>> +							<&rpmhpd_opp_svs_l1>;
>>>> +				};
>>>> +
>>>> +				opp-444000000 {
>>>> +					opp-hz = /bits/ 64 <444000000 444000000 666000000>;
>>>> +					required-opps = <&rpmhpd_opp_svs_l1>,
>>>> +							<&rpmhpd_opp_nom>;
>>>> +				};
>>>> +
>>>> +				opp-533333334 {
>>>> +					opp-hz = /bits/ 64 <533333334 533333334 800000000>;
>>>> +					required-opps = <&rpmhpd_opp_svs_l1>,
>>>> +							<&rpmhpd_opp_turbo>;
>>>> +				};
>>>> +
>>>> +				opp-655000000 {
>>>> +					opp-hz = /bits/ 64 <655000000 655000000 982000000>;
>>>> +					required-opps = <&rpmhpd_opp_nom>,
>>>> +							<&rpmhpd_opp_turbo_l1>;
>>>> +				};
>>>> +			};
>>>> +		};
>>>> +
>>>>  		mdss: display-subsystem@ae00000 {
>>>>  			compatible = "qcom,glymur-mdss";
>>>>  			reg = <0x0 0x0ae00000 0x0 0x1000>;
>>>>
>>>> -- 
>>>> 2.34.1
>>>>