* [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node @ 2025-04-24 16:32 Neil Armstrong 2025-04-25 21:49 ` Konrad Dybcio 0 siblings, 1 reply; 10+ messages in thread From: Neil Armstrong @ 2025-04-24 16:32 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-arm-msm, devicetree, linux-kernel, Dmitry Baryshkov, Neil Armstrong Add DT entries for the sm8650 iris decoder. Since the firmware is required to be signed, only enable on Qualcomm development boards where the firmware is available. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- Changes in v2: - removed useless firmware-name - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org --- arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 4 ++ arch/arm64/boot/dts/qcom/sm8650-mtp.dts | 4 ++ arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 4 ++ arch/arm64/boot/dts/qcom/sm8650.dtsi | 94 +++++++++++++++++++++++++++++++++ 4 files changed, 106 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts index d0912735b54e5090f9f213c2c9341e03effbbbff..259649d7dcd768ecf93c9473adc1738e7d715b6c 100644 --- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts +++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts @@ -894,6 +894,10 @@ &ipa { status = "okay"; }; +&iris { + status = "okay"; +}; + &gpu { status = "okay"; diff --git a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts index 76ef43c10f77d8329ccf0a05c9d590a46372315f..8a957adbfb383411153506e46d4c9acfb02e3114 100644 --- a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts +++ b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts @@ -585,6 +585,10 @@ vreg_l7n_3p3: ldo7 { }; }; +&iris { + status = "okay"; +}; + &lpass_tlmm { spkr_1_sd_n_active: spkr-1-sd-n-active-state { pins = "gpio21"; diff --git a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts index 71033fba21b56bc63620dca3e453c14191739675..7552d5d3fb4020e61d47242b447c9ecbec5f8d55 100644 --- a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts +++ b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts @@ -824,6 +824,10 @@ &ipa { status = "okay"; }; +&iris { + status = "okay"; +}; + &gpu { status = "okay"; diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi index c2937f7217943c4ca91a91eadc8259b2d6a01372..9afde0582ec9b8fef44c0af0324bfae9b20d1d60 100644 --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi @@ -4955,6 +4955,100 @@ opp-202000000 { }; }; + iris: video-codec@aa00000 { + compatible = "qcom,sm8650-iris"; + reg = <0 0x0aa00000 0 0xf0000>; + + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; + + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, + <&videocc VIDEO_CC_MVS0_GDSC>, + <&rpmhpd RPMHPD_MXC>, + <&rpmhpd RPMHPD_MMCX>; + power-domain-names = "venus", + "vcodec0", + "mxc", + "mmcx"; + + operating-points-v2 = <&iris_opp_table>; + + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, + <&videocc VIDEO_CC_MVS0C_CLK>, + <&videocc VIDEO_CC_MVS0_CLK>; + clock-names = "iface", + "core", + "vcodec0_core"; + + interconnects = <&gem_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"; + + /* FW load region */ + memory-region = <&video_mem>; + + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, + <&videocc VIDEO_CC_XO_CLK_ARES>, + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; + reset-names = "bus", + "xo", + "core"; + + iommus = <&apps_smmu 0x1940 0>, + <&apps_smmu 0x1947 0>; + + dma-coherent; + + /* + * IRIS firmware is signed by vendors, only + * enable in boards where the proper signed firmware + * is available. + */ + status = "disabled"; + + iris_opp_table: opp-table { + compatible = "operating-points-v2"; + + opp-196000000 { + opp-hz = /bits/ 64 <196000000>; + required-opps = <&rpmhpd_opp_low_svs_d1>, + <&rpmhpd_opp_low_svs_d1>; + }; + + opp-300000000 { + opp-hz = /bits/ 64 <300000000>; + required-opps = <&rpmhpd_opp_low_svs>, + <&rpmhpd_opp_low_svs>; + }; + + opp-380000000 { + opp-hz = /bits/ 64 <380000000>; + required-opps = <&rpmhpd_opp_svs>, + <&rpmhpd_opp_svs>; + }; + + opp-435000000 { + opp-hz = /bits/ 64 <435000000>; + required-opps = <&rpmhpd_opp_svs_l1>, + <&rpmhpd_opp_svs_l1>; + }; + + opp-480000000 { + opp-hz = /bits/ 64 <480000000>; + required-opps = <&rpmhpd_opp_turbo>, + <&rpmhpd_opp_turbo>; + }; + + opp-533333334 { + opp-hz = /bits/ 64 <533333334>; + required-opps = <&rpmhpd_opp_turbo_l1>, + <&rpmhpd_opp_turbo_l1>; + }; + }; + }; + videocc: clock-controller@aaf0000 { compatible = "qcom,sm8650-videocc"; reg = <0 0x0aaf0000 0 0x10000>; --- base-commit: a7dca088884312d607fff89f2666c670cb7073ac change-id: 20250418-topic-sm8x50-upstream-iris-8650-dt-d2c64a59505f Best regards, -- Neil Armstrong <neil.armstrong@linaro.org> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-04-24 16:32 [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node Neil Armstrong @ 2025-04-25 21:49 ` Konrad Dybcio 2025-04-28 8:18 ` Neil Armstrong 0 siblings, 1 reply; 10+ messages in thread From: Konrad Dybcio @ 2025-04-25 21:49 UTC (permalink / raw) To: Neil Armstrong, Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-arm-msm, devicetree, linux-kernel, Dmitry Baryshkov On 4/24/25 6:32 PM, Neil Armstrong wrote: > Add DT entries for the sm8650 iris decoder. > > Since the firmware is required to be signed, only enable > on Qualcomm development boards where the firmware is > available. > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > Changes in v2: > - removed useless firmware-name > - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org > --- [...] > + iris: video-codec@aa00000 { > + compatible = "qcom,sm8650-iris"; > + reg = <0 0x0aa00000 0 0xf0000>; > + > + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; > + > + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, > + <&videocc VIDEO_CC_MVS0_GDSC>, > + <&rpmhpd RPMHPD_MXC>, > + <&rpmhpd RPMHPD_MMCX>; > + power-domain-names = "venus", > + "vcodec0", > + "mxc", > + "mmcx"; > + > + operating-points-v2 = <&iris_opp_table>; > + > + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, > + <&videocc VIDEO_CC_MVS0C_CLK>, > + <&videocc VIDEO_CC_MVS0_CLK>; > + clock-names = "iface", > + "core", > + "vcodec0_core"; > + > + interconnects = <&gem_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"; > + > + /* FW load region */ I don't think this comment brings value > + memory-region = <&video_mem>; > + > + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, > + <&videocc VIDEO_CC_XO_CLK_ARES>, > + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; > + reset-names = "bus", > + "xo", > + "core"; > + > + iommus = <&apps_smmu 0x1940 0>, > + <&apps_smmu 0x1947 0>; I think you may also need 0x1942 0x0 (please also make the second value / SMR mask hex)> + > + dma-coherent; > + > + /* > + * IRIS firmware is signed by vendors, only > + * enable in boards where the proper signed firmware > + * is available. > + */ Here's to another angry media article :( Please keep Iris enabled.. Vikash reassured me this is not an issue until the user attempts to use the decoder [1], and reading the code myself I come to the same conclusion (though I haven't given it a smoke test - please do that yourself, as you seem to have a better set up with these platforms). If the userland is sane, it should throw an error and defer to CPU decoding. This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) would prevent .sync_state [1] https://lore.kernel.org/linux-arm-msm/98a35a51-6351-5ebb-4207-0004e89682eb@quicinc.com/ [...] > + > + opp-480000000 { > + opp-hz = /bits/ 64 <480000000>; > + required-opps = <&rpmhpd_opp_turbo>, > + <&rpmhpd_opp_turbo>; nom (nom nom nom nom nom) > + }; > + > + opp-533333334 { > + opp-hz = /bits/ 64 <533333334>; > + required-opps = <&rpmhpd_opp_turbo_l1>, > + <&rpmhpd_opp_turbo_l1>; turbo Konrad ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-04-25 21:49 ` Konrad Dybcio @ 2025-04-28 8:18 ` Neil Armstrong 2025-04-28 10:48 ` Dmitry Baryshkov 0 siblings, 1 reply; 10+ messages in thread From: Neil Armstrong @ 2025-04-28 8:18 UTC (permalink / raw) To: Konrad Dybcio, Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-arm-msm, devicetree, linux-kernel, Dmitry Baryshkov Hi, On 25/04/2025 23:49, Konrad Dybcio wrote: > On 4/24/25 6:32 PM, Neil Armstrong wrote: >> Add DT entries for the sm8650 iris decoder. >> >> Since the firmware is required to be signed, only enable >> on Qualcomm development boards where the firmware is >> available. >> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> Changes in v2: >> - removed useless firmware-name >> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org >> --- > > [...] > >> + iris: video-codec@aa00000 { >> + compatible = "qcom,sm8650-iris"; >> + reg = <0 0x0aa00000 0 0xf0000>; >> + >> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; >> + >> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, >> + <&videocc VIDEO_CC_MVS0_GDSC>, >> + <&rpmhpd RPMHPD_MXC>, >> + <&rpmhpd RPMHPD_MMCX>; >> + power-domain-names = "venus", >> + "vcodec0", >> + "mxc", >> + "mmcx"; >> + >> + operating-points-v2 = <&iris_opp_table>; >> + >> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, >> + <&videocc VIDEO_CC_MVS0C_CLK>, >> + <&videocc VIDEO_CC_MVS0_CLK>; >> + clock-names = "iface", >> + "core", >> + "vcodec0_core"; >> + >> + interconnects = <&gem_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"; >> + >> + /* FW load region */ > > I don't think this comment brings value Right > >> + memory-region = <&video_mem>; >> + >> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, >> + <&videocc VIDEO_CC_XO_CLK_ARES>, >> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; >> + reset-names = "bus", >> + "xo", >> + "core"; >> + >> + iommus = <&apps_smmu 0x1940 0>, >> + <&apps_smmu 0x1947 0>; > > I think you may also need 0x1942 0x0 (please also make the second value / SMR > mask hex)> + I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? >> + dma-coherent; >> + >> + /* >> + * IRIS firmware is signed by vendors, only >> + * enable in boards where the proper signed firmware >> + * is available. >> + */ > > Here's to another angry media article :( > > Please keep Iris enabled.. Vikash reassured me this is not an > issue until the user attempts to use the decoder [1], and reading > the code myself I come to the same conclusion (though I haven't given > it a smoke test - please do that yourself, as you seem to have a better > set up with these platforms). > > If the userland is sane, it should throw an error and defer to CPU > decoding. > > This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) > would prevent .sync_state Well sync with Bjorn who asked me to only enable on board with available firmware ;-) > > [1] https://lore.kernel.org/linux-arm-msm/98a35a51-6351-5ebb-4207-0004e89682eb@quicinc.com/ > > [...] > >> + >> + opp-480000000 { >> + opp-hz = /bits/ 64 <480000000>; >> + required-opps = <&rpmhpd_opp_turbo>, >> + <&rpmhpd_opp_turbo>; > > nom (nom nom nom nom nom) > >> + }; >> + >> + opp-533333334 { >> + opp-hz = /bits/ 64 <533333334>; >> + required-opps = <&rpmhpd_opp_turbo_l1>, >> + <&rpmhpd_opp_turbo_l1>; > > turbo Ack > > Konrad Thanks, Neil ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-04-28 8:18 ` Neil Armstrong @ 2025-04-28 10:48 ` Dmitry Baryshkov 2025-04-28 21:14 ` Konrad Dybcio 0 siblings, 1 reply; 10+ messages in thread From: Dmitry Baryshkov @ 2025-04-28 10:48 UTC (permalink / raw) To: neil.armstrong Cc: Konrad Dybcio, Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, devicetree, linux-kernel On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > Hi, > > On 25/04/2025 23:49, Konrad Dybcio wrote: > > On 4/24/25 6:32 PM, Neil Armstrong wrote: > >> Add DT entries for the sm8650 iris decoder. > >> > >> Since the firmware is required to be signed, only enable > >> on Qualcomm development boards where the firmware is > >> available. > >> > >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >> --- > >> Changes in v2: > >> - removed useless firmware-name > >> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org > >> --- > > > > [...] > > > >> + iris: video-codec@aa00000 { > >> + compatible = "qcom,sm8650-iris"; > >> + reg = <0 0x0aa00000 0 0xf0000>; > >> + > >> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; > >> + > >> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, > >> + <&videocc VIDEO_CC_MVS0_GDSC>, > >> + <&rpmhpd RPMHPD_MXC>, > >> + <&rpmhpd RPMHPD_MMCX>; > >> + power-domain-names = "venus", > >> + "vcodec0", > >> + "mxc", > >> + "mmcx"; > >> + > >> + operating-points-v2 = <&iris_opp_table>; > >> + > >> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, > >> + <&videocc VIDEO_CC_MVS0C_CLK>, > >> + <&videocc VIDEO_CC_MVS0_CLK>; > >> + clock-names = "iface", > >> + "core", > >> + "vcodec0_core"; > >> + > >> + interconnects = <&gem_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"; > >> + > >> + /* FW load region */ > > > > I don't think this comment brings value > > Right > > > > >> + memory-region = <&video_mem>; > >> + > >> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, > >> + <&videocc VIDEO_CC_XO_CLK_ARES>, > >> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; > >> + reset-names = "bus", > >> + "xo", > >> + "core"; > >> + > >> + iommus = <&apps_smmu 0x1940 0>, > >> + <&apps_smmu 0x1947 0>; > > > > I think you may also need 0x1942 0x0 (please also make the second value / SMR > > mask hex)> + > > I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? > > >> + dma-coherent; > >> + > >> + /* > >> + * IRIS firmware is signed by vendors, only > >> + * enable in boards where the proper signed firmware > >> + * is available. > >> + */ > > > > Here's to another angry media article :( > > > > Please keep Iris enabled.. Vikash reassured me this is not an > > issue until the user attempts to use the decoder [1], and reading > > the code myself I come to the same conclusion (though I haven't given > > it a smoke test - please do that yourself, as you seem to have a better > > set up with these platforms). > > > > If the userland is sane, it should throw an error and defer to CPU > > decoding. > > > > This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) > > would prevent .sync_state > > Well sync with Bjorn who asked me to only enable on board with available firmware ;-) I'd second him here: if there is no firmware, don't enable the device. It's better than the users having cryptic messages in the dmesg, trying to understand why the driver errors out. > > > > > [1] https://lore.kernel.org/linux-arm-msm/98a35a51-6351-5ebb-4207-0004e89682eb@quicinc.com/ > > > > [...] > > > >> + > >> + opp-480000000 { > >> + opp-hz = /bits/ 64 <480000000>; > >> + required-opps = <&rpmhpd_opp_turbo>, > >> + <&rpmhpd_opp_turbo>; > > > > nom (nom nom nom nom nom) > > > >> + }; > >> + > >> + opp-533333334 { > >> + opp-hz = /bits/ 64 <533333334>; > >> + required-opps = <&rpmhpd_opp_turbo_l1>, > >> + <&rpmhpd_opp_turbo_l1>; > > > > turbo > > Ack > > > > > Konrad > > Thanks, > Neil -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-04-28 10:48 ` Dmitry Baryshkov @ 2025-04-28 21:14 ` Konrad Dybcio 2025-04-29 12:22 ` Dmitry Baryshkov 2025-05-06 20:23 ` Bjorn Andersson 0 siblings, 2 replies; 10+ messages in thread From: Konrad Dybcio @ 2025-04-28 21:14 UTC (permalink / raw) To: Dmitry Baryshkov, neil.armstrong Cc: Konrad Dybcio, Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, devicetree, linux-kernel On 4/28/25 12:48 PM, Dmitry Baryshkov wrote: > On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@linaro.org> wrote: >> >> Hi, >> >> On 25/04/2025 23:49, Konrad Dybcio wrote: >>> On 4/24/25 6:32 PM, Neil Armstrong wrote: >>>> Add DT entries for the sm8650 iris decoder. >>>> >>>> Since the firmware is required to be signed, only enable >>>> on Qualcomm development boards where the firmware is >>>> available. >>>> >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> Changes in v2: >>>> - removed useless firmware-name >>>> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org >>>> --- >>> >>> [...] >>> >>>> + iris: video-codec@aa00000 { >>>> + compatible = "qcom,sm8650-iris"; >>>> + reg = <0 0x0aa00000 0 0xf0000>; >>>> + >>>> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; >>>> + >>>> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, >>>> + <&videocc VIDEO_CC_MVS0_GDSC>, >>>> + <&rpmhpd RPMHPD_MXC>, >>>> + <&rpmhpd RPMHPD_MMCX>; >>>> + power-domain-names = "venus", >>>> + "vcodec0", >>>> + "mxc", >>>> + "mmcx"; >>>> + >>>> + operating-points-v2 = <&iris_opp_table>; >>>> + >>>> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, >>>> + <&videocc VIDEO_CC_MVS0C_CLK>, >>>> + <&videocc VIDEO_CC_MVS0_CLK>; >>>> + clock-names = "iface", >>>> + "core", >>>> + "vcodec0_core"; >>>> + >>>> + interconnects = <&gem_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"; >>>> + >>>> + /* FW load region */ >>> >>> I don't think this comment brings value >> >> Right >> >>> >>>> + memory-region = <&video_mem>; >>>> + >>>> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, >>>> + <&videocc VIDEO_CC_XO_CLK_ARES>, >>>> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; >>>> + reset-names = "bus", >>>> + "xo", >>>> + "core"; >>>> + >>>> + iommus = <&apps_smmu 0x1940 0>, >>>> + <&apps_smmu 0x1947 0>; >>> >>> I think you may also need 0x1942 0x0 (please also make the second value / SMR >>> mask hex)> + >> >> I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? I saw it in docs only, maybe Vikash or Dikshita can chime in whether it's necessary. It would have mask 0x0 if so. >> >>>> + dma-coherent; >>>> + >>>> + /* >>>> + * IRIS firmware is signed by vendors, only >>>> + * enable in boards where the proper signed firmware >>>> + * is available. >>>> + */ >>> >>> Here's to another angry media article :( >>> >>> Please keep Iris enabled.. Vikash reassured me this is not an >>> issue until the user attempts to use the decoder [1], and reading >>> the code myself I come to the same conclusion (though I haven't given >>> it a smoke test - please do that yourself, as you seem to have a better >>> set up with these platforms). >>> >>> If the userland is sane, it should throw an error and defer to CPU >>> decoding. >>> >>> This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) >>> would prevent .sync_state >> >> Well sync with Bjorn who asked me to only enable on board with available firmware ;-) > > I'd second him here: if there is no firmware, don't enable the device. > It's better than the users having cryptic messages in the dmesg, > trying to understand why the driver errors out. I don't agree.. the firmware may appear later at boot (e.g. user installs a small rootfs and manually pulls in linux-firmware). Plus without the firmware, we can still power on and off the IP block, particularly achieve sync_state regardless of it Konrad ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-04-28 21:14 ` Konrad Dybcio @ 2025-04-29 12:22 ` Dmitry Baryshkov 2025-05-06 20:23 ` Bjorn Andersson 1 sibling, 0 replies; 10+ messages in thread From: Dmitry Baryshkov @ 2025-04-29 12:22 UTC (permalink / raw) To: Konrad Dybcio Cc: neil.armstrong, Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, devicetree, linux-kernel On Mon, Apr 28, 2025 at 11:14:18PM +0200, Konrad Dybcio wrote: > On 4/28/25 12:48 PM, Dmitry Baryshkov wrote: > > On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> > >> Hi, > >> > >> On 25/04/2025 23:49, Konrad Dybcio wrote: > >>> On 4/24/25 6:32 PM, Neil Armstrong wrote: > >>>> Add DT entries for the sm8650 iris decoder. > >>>> > >>>> Since the firmware is required to be signed, only enable > >>>> on Qualcomm development boards where the firmware is > >>>> available. > >>>> > >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>>> --- > >>>> Changes in v2: > >>>> - removed useless firmware-name > >>>> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org > >>>> --- > >>> > >>> [...] > >>> > >>>> + iris: video-codec@aa00000 { > >>>> + compatible = "qcom,sm8650-iris"; > >>>> + reg = <0 0x0aa00000 0 0xf0000>; > >>>> + > >>>> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; > >>>> + > >>>> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, > >>>> + <&videocc VIDEO_CC_MVS0_GDSC>, > >>>> + <&rpmhpd RPMHPD_MXC>, > >>>> + <&rpmhpd RPMHPD_MMCX>; > >>>> + power-domain-names = "venus", > >>>> + "vcodec0", > >>>> + "mxc", > >>>> + "mmcx"; > >>>> + > >>>> + operating-points-v2 = <&iris_opp_table>; > >>>> + > >>>> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, > >>>> + <&videocc VIDEO_CC_MVS0C_CLK>, > >>>> + <&videocc VIDEO_CC_MVS0_CLK>; > >>>> + clock-names = "iface", > >>>> + "core", > >>>> + "vcodec0_core"; > >>>> + > >>>> + interconnects = <&gem_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"; > >>>> + > >>>> + /* FW load region */ > >>> > >>> I don't think this comment brings value > >> > >> Right > >> > >>> > >>>> + memory-region = <&video_mem>; > >>>> + > >>>> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, > >>>> + <&videocc VIDEO_CC_XO_CLK_ARES>, > >>>> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; > >>>> + reset-names = "bus", > >>>> + "xo", > >>>> + "core"; > >>>> + > >>>> + iommus = <&apps_smmu 0x1940 0>, > >>>> + <&apps_smmu 0x1947 0>; > >>> > >>> I think you may also need 0x1942 0x0 (please also make the second value / SMR > >>> mask hex)> + > >> > >> I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? > > I saw it in docs only, maybe Vikash or Dikshita can chime in whether it's > necessary. It would have mask 0x0 if so. > > >> > >>>> + dma-coherent; > >>>> + > >>>> + /* > >>>> + * IRIS firmware is signed by vendors, only > >>>> + * enable in boards where the proper signed firmware > >>>> + * is available. > >>>> + */ > >>> > >>> Here's to another angry media article :( > >>> > >>> Please keep Iris enabled.. Vikash reassured me this is not an > >>> issue until the user attempts to use the decoder [1], and reading > >>> the code myself I come to the same conclusion (though I haven't given > >>> it a smoke test - please do that yourself, as you seem to have a better > >>> set up with these platforms). > >>> > >>> If the userland is sane, it should throw an error and defer to CPU > >>> decoding. > >>> > >>> This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) > >>> would prevent .sync_state > >> > >> Well sync with Bjorn who asked me to only enable on board with available firmware ;-) > > > > I'd second him here: if there is no firmware, don't enable the device. > > It's better than the users having cryptic messages in the dmesg, > > trying to understand why the driver errors out. > > I don't agree.. the firmware may appear later at boot (e.g. user installs a > small rootfs and manually pulls in linux-firmware). Plus without the firmware, > we can still power on and off the IP block, particularly achieve sync_state > regardless of it Yes. But the firmware should appear at a well-known location (rather than a default one), so we are back to the question of having the firmware at all (even potentially). I really would rather not having users / developers trying to put qvss.mbn firmware into the default location at qcom/vpu. Likewise I don't think we should have users face cryptic errors if the firmware from linux-firmware is not suitable for the device. With all that in mind, let's follow our standard approach and not enable firmware-backed devices by default. Also, if we define the device as disabled, we can reach the sync_state too, can we not? -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-04-28 21:14 ` Konrad Dybcio 2025-04-29 12:22 ` Dmitry Baryshkov @ 2025-05-06 20:23 ` Bjorn Andersson 2025-05-06 22:53 ` Konrad Dybcio 1 sibling, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2025-05-06 20:23 UTC (permalink / raw) To: Konrad Dybcio Cc: Dmitry Baryshkov, neil.armstrong, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, devicetree, linux-kernel On Mon, Apr 28, 2025 at 11:14:18PM +0200, Konrad Dybcio wrote: > On 4/28/25 12:48 PM, Dmitry Baryshkov wrote: > > On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> > >> Hi, > >> > >> On 25/04/2025 23:49, Konrad Dybcio wrote: > >>> On 4/24/25 6:32 PM, Neil Armstrong wrote: > >>>> Add DT entries for the sm8650 iris decoder. > >>>> > >>>> Since the firmware is required to be signed, only enable > >>>> on Qualcomm development boards where the firmware is > >>>> available. > >>>> > >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>>> --- > >>>> Changes in v2: > >>>> - removed useless firmware-name > >>>> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org > >>>> --- > >>> > >>> [...] > >>> > >>>> + iris: video-codec@aa00000 { > >>>> + compatible = "qcom,sm8650-iris"; > >>>> + reg = <0 0x0aa00000 0 0xf0000>; > >>>> + > >>>> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; > >>>> + > >>>> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, > >>>> + <&videocc VIDEO_CC_MVS0_GDSC>, > >>>> + <&rpmhpd RPMHPD_MXC>, > >>>> + <&rpmhpd RPMHPD_MMCX>; > >>>> + power-domain-names = "venus", > >>>> + "vcodec0", > >>>> + "mxc", > >>>> + "mmcx"; > >>>> + > >>>> + operating-points-v2 = <&iris_opp_table>; > >>>> + > >>>> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, > >>>> + <&videocc VIDEO_CC_MVS0C_CLK>, > >>>> + <&videocc VIDEO_CC_MVS0_CLK>; > >>>> + clock-names = "iface", > >>>> + "core", > >>>> + "vcodec0_core"; > >>>> + > >>>> + interconnects = <&gem_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"; > >>>> + > >>>> + /* FW load region */ > >>> > >>> I don't think this comment brings value > >> > >> Right > >> > >>> > >>>> + memory-region = <&video_mem>; > >>>> + > >>>> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, > >>>> + <&videocc VIDEO_CC_XO_CLK_ARES>, > >>>> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; > >>>> + reset-names = "bus", > >>>> + "xo", > >>>> + "core"; > >>>> + > >>>> + iommus = <&apps_smmu 0x1940 0>, > >>>> + <&apps_smmu 0x1947 0>; > >>> > >>> I think you may also need 0x1942 0x0 (please also make the second value / SMR > >>> mask hex)> + > >> > >> I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? > > I saw it in docs only, maybe Vikash or Dikshita can chime in whether it's > necessary. It would have mask 0x0 if so. > > >> > >>>> + dma-coherent; > >>>> + > >>>> + /* > >>>> + * IRIS firmware is signed by vendors, only > >>>> + * enable in boards where the proper signed firmware > >>>> + * is available. > >>>> + */ > >>> > >>> Here's to another angry media article :( > >>> > >>> Please keep Iris enabled.. Vikash reassured me this is not an > >>> issue until the user attempts to use the decoder [1], and reading > >>> the code myself I come to the same conclusion (though I haven't given > >>> it a smoke test - please do that yourself, as you seem to have a better > >>> set up with these platforms). > >>> > >>> If the userland is sane, it should throw an error and defer to CPU > >>> decoding. > >>> > >>> This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) > >>> would prevent .sync_state > >> > >> Well sync with Bjorn who asked me to only enable on board with available firmware ;-) > > > > I'd second him here: if there is no firmware, don't enable the device. > > It's better than the users having cryptic messages in the dmesg, > > trying to understand why the driver errors out. > > I don't agree.. the firmware may appear later at boot (e.g. user installs a > small rootfs and manually pulls in linux-firmware). Plus without the firmware, > we can still power on and off the IP block, particularly achieve sync_state > regardless of it > Not "available during boot", but rather "available for a particular board". We generally avoid enabling device_nodes that depend on vendor-signed firmware until someone has tested the device on such board and specified the proper path to the vendor-specific firmware. Are you suggesting that we should leave this enabled on all boards for some reason (perhaps to ensure that resources are adequately managed)? Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-05-06 20:23 ` Bjorn Andersson @ 2025-05-06 22:53 ` Konrad Dybcio 2025-05-07 0:27 ` Dmitry Baryshkov 0 siblings, 1 reply; 10+ messages in thread From: Konrad Dybcio @ 2025-05-06 22:53 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio Cc: Dmitry Baryshkov, neil.armstrong, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, devicetree, linux-kernel On 5/6/25 10:23 PM, Bjorn Andersson wrote: > On Mon, Apr 28, 2025 at 11:14:18PM +0200, Konrad Dybcio wrote: >> On 4/28/25 12:48 PM, Dmitry Baryshkov wrote: >>> On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>>> >>>> Hi, >>>> >>>> On 25/04/2025 23:49, Konrad Dybcio wrote: >>>>> On 4/24/25 6:32 PM, Neil Armstrong wrote: >>>>>> Add DT entries for the sm8650 iris decoder. >>>>>> >>>>>> Since the firmware is required to be signed, only enable >>>>>> on Qualcomm development boards where the firmware is >>>>>> available. >>>>>> >>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> >>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>> --- >>>>>> Changes in v2: >>>>>> - removed useless firmware-name >>>>>> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org >>>>>> --- >>>>> >>>>> [...] >>>>> >>>>>> + iris: video-codec@aa00000 { >>>>>> + compatible = "qcom,sm8650-iris"; >>>>>> + reg = <0 0x0aa00000 0 0xf0000>; >>>>>> + >>>>>> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; >>>>>> + >>>>>> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, >>>>>> + <&videocc VIDEO_CC_MVS0_GDSC>, >>>>>> + <&rpmhpd RPMHPD_MXC>, >>>>>> + <&rpmhpd RPMHPD_MMCX>; >>>>>> + power-domain-names = "venus", >>>>>> + "vcodec0", >>>>>> + "mxc", >>>>>> + "mmcx"; >>>>>> + >>>>>> + operating-points-v2 = <&iris_opp_table>; >>>>>> + >>>>>> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, >>>>>> + <&videocc VIDEO_CC_MVS0C_CLK>, >>>>>> + <&videocc VIDEO_CC_MVS0_CLK>; >>>>>> + clock-names = "iface", >>>>>> + "core", >>>>>> + "vcodec0_core"; >>>>>> + >>>>>> + interconnects = <&gem_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"; >>>>>> + >>>>>> + /* FW load region */ >>>>> >>>>> I don't think this comment brings value >>>> >>>> Right >>>> >>>>> >>>>>> + memory-region = <&video_mem>; >>>>>> + >>>>>> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, >>>>>> + <&videocc VIDEO_CC_XO_CLK_ARES>, >>>>>> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; >>>>>> + reset-names = "bus", >>>>>> + "xo", >>>>>> + "core"; >>>>>> + >>>>>> + iommus = <&apps_smmu 0x1940 0>, >>>>>> + <&apps_smmu 0x1947 0>; >>>>> >>>>> I think you may also need 0x1942 0x0 (please also make the second value / SMR >>>>> mask hex)> + >>>> >>>> I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? >> >> I saw it in docs only, maybe Vikash or Dikshita can chime in whether it's >> necessary. It would have mask 0x0 if so. >> >>>> >>>>>> + dma-coherent; >>>>>> + >>>>>> + /* >>>>>> + * IRIS firmware is signed by vendors, only >>>>>> + * enable in boards where the proper signed firmware >>>>>> + * is available. >>>>>> + */ >>>>> >>>>> Here's to another angry media article :( >>>>> >>>>> Please keep Iris enabled.. Vikash reassured me this is not an >>>>> issue until the user attempts to use the decoder [1], and reading >>>>> the code myself I come to the same conclusion (though I haven't given >>>>> it a smoke test - please do that yourself, as you seem to have a better >>>>> set up with these platforms). >>>>> >>>>> If the userland is sane, it should throw an error and defer to CPU >>>>> decoding. >>>>> >>>>> This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) >>>>> would prevent .sync_state >>>> >>>> Well sync with Bjorn who asked me to only enable on board with available firmware ;-) >>> >>> I'd second him here: if there is no firmware, don't enable the device. >>> It's better than the users having cryptic messages in the dmesg, >>> trying to understand why the driver errors out. >> >> I don't agree.. the firmware may appear later at boot (e.g. user installs a >> small rootfs and manually pulls in linux-firmware). Plus without the firmware, >> we can still power on and off the IP block, particularly achieve sync_state >> regardless of it >> > > Not "available during boot", but rather "available for a particular > board". I'd argue that if a device is in the hands of users, there already exists some acceptable set of fw binaries.. but most developers aren't in the position to upload them into l-f.. And quite frankly I'm not the biggest fan of having a gigabyte of 99%-the-same files with a dozen lines changed and a different signature prepended to them either.. > We generally avoid enabling device_nodes that depend on vendor-signed > firmware until someone has tested the device on such board and specified > the proper path to the vendor-specific firmware. > > Are you suggesting that we should leave this enabled on all boards for > some reason (perhaps to ensure that resources are adequately managed)? Yes, for that reason indeed. We don't generally need to load firmware to turn something *off*. And most IP blocks don't **actually** need to be presented with firmware at probe time (I can only think of external ICs like no-storage touch controllers that need the fw uploaded on each powerup). Telling the user "hey, this is supported but the firmware file can't be loaded on your device" may also be better sounding than "won't work on your machine" (with the quiet part being: "because someone hasn't copied 5 lines of DTS") Konrad ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-05-06 22:53 ` Konrad Dybcio @ 2025-05-07 0:27 ` Dmitry Baryshkov 2025-05-08 14:49 ` Konrad Dybcio 0 siblings, 1 reply; 10+ messages in thread From: Dmitry Baryshkov @ 2025-05-07 0:27 UTC (permalink / raw) To: Konrad Dybcio Cc: Bjorn Andersson, neil.armstrong, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, devicetree, linux-kernel On Wed, May 07, 2025 at 12:53:27AM +0200, Konrad Dybcio wrote: > On 5/6/25 10:23 PM, Bjorn Andersson wrote: > > On Mon, Apr 28, 2025 at 11:14:18PM +0200, Konrad Dybcio wrote: > >> On 4/28/25 12:48 PM, Dmitry Baryshkov wrote: > >>> On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >>>> > >>>> Hi, > >>>> > >>>> On 25/04/2025 23:49, Konrad Dybcio wrote: > >>>>> On 4/24/25 6:32 PM, Neil Armstrong wrote: > >>>>>> Add DT entries for the sm8650 iris decoder. > >>>>>> > >>>>>> Since the firmware is required to be signed, only enable > >>>>>> on Qualcomm development boards where the firmware is > >>>>>> available. > >>>>>> > >>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > >>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>>>>> --- > >>>>>> Changes in v2: > >>>>>> - removed useless firmware-name > >>>>>> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org > >>>>>> --- > >>>>> > >>>>> [...] > >>>>> > >>>>>> + iris: video-codec@aa00000 { > >>>>>> + compatible = "qcom,sm8650-iris"; > >>>>>> + reg = <0 0x0aa00000 0 0xf0000>; > >>>>>> + > >>>>>> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; > >>>>>> + > >>>>>> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, > >>>>>> + <&videocc VIDEO_CC_MVS0_GDSC>, > >>>>>> + <&rpmhpd RPMHPD_MXC>, > >>>>>> + <&rpmhpd RPMHPD_MMCX>; > >>>>>> + power-domain-names = "venus", > >>>>>> + "vcodec0", > >>>>>> + "mxc", > >>>>>> + "mmcx"; > >>>>>> + > >>>>>> + operating-points-v2 = <&iris_opp_table>; > >>>>>> + > >>>>>> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, > >>>>>> + <&videocc VIDEO_CC_MVS0C_CLK>, > >>>>>> + <&videocc VIDEO_CC_MVS0_CLK>; > >>>>>> + clock-names = "iface", > >>>>>> + "core", > >>>>>> + "vcodec0_core"; > >>>>>> + > >>>>>> + interconnects = <&gem_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"; > >>>>>> + > >>>>>> + /* FW load region */ > >>>>> > >>>>> I don't think this comment brings value > >>>> > >>>> Right > >>>> > >>>>> > >>>>>> + memory-region = <&video_mem>; > >>>>>> + > >>>>>> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, > >>>>>> + <&videocc VIDEO_CC_XO_CLK_ARES>, > >>>>>> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; > >>>>>> + reset-names = "bus", > >>>>>> + "xo", > >>>>>> + "core"; > >>>>>> + > >>>>>> + iommus = <&apps_smmu 0x1940 0>, > >>>>>> + <&apps_smmu 0x1947 0>; > >>>>> > >>>>> I think you may also need 0x1942 0x0 (please also make the second value / SMR > >>>>> mask hex)> + > >>>> > >>>> I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? > >> > >> I saw it in docs only, maybe Vikash or Dikshita can chime in whether it's > >> necessary. It would have mask 0x0 if so. > >> > >>>> > >>>>>> + dma-coherent; > >>>>>> + > >>>>>> + /* > >>>>>> + * IRIS firmware is signed by vendors, only > >>>>>> + * enable in boards where the proper signed firmware > >>>>>> + * is available. > >>>>>> + */ > >>>>> > >>>>> Here's to another angry media article :( > >>>>> > >>>>> Please keep Iris enabled.. Vikash reassured me this is not an > >>>>> issue until the user attempts to use the decoder [1], and reading > >>>>> the code myself I come to the same conclusion (though I haven't given > >>>>> it a smoke test - please do that yourself, as you seem to have a better > >>>>> set up with these platforms). > >>>>> > >>>>> If the userland is sane, it should throw an error and defer to CPU > >>>>> decoding. > >>>>> > >>>>> This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) > >>>>> would prevent .sync_state > >>>> > >>>> Well sync with Bjorn who asked me to only enable on board with available firmware ;-) > >>> > >>> I'd second him here: if there is no firmware, don't enable the device. > >>> It's better than the users having cryptic messages in the dmesg, > >>> trying to understand why the driver errors out. > >> > >> I don't agree.. the firmware may appear later at boot (e.g. user installs a > >> small rootfs and manually pulls in linux-firmware). Plus without the firmware, > >> we can still power on and off the IP block, particularly achieve sync_state > >> regardless of it > >> > > > > Not "available during boot", but rather "available for a particular > > board". > > I'd argue that if a device is in the hands of users, there already exists > some acceptable set of fw binaries.. but most developers aren't in the > position to upload them into l-f.. And quite frankly I'm not the biggest > fan of having a gigabyte of 99%-the-same files with a dozen lines changed > and a different signature prepended to them either.. > > > We generally avoid enabling device_nodes that depend on vendor-signed > > firmware until someone has tested the device on such board and specified > > the proper path to the vendor-specific firmware. > > > > Are you suggesting that we should leave this enabled on all boards for > > some reason (perhaps to ensure that resources are adequately managed)? > > Yes, for that reason indeed. > > We don't generally need to load firmware to turn something *off*. And > most IP blocks don't **actually** need to be presented with firmware at > probe time (I can only think of external ICs like no-storage touch > controllers that need the fw uploaded on each powerup). > > Telling the user "hey, this is supported but the firmware file can't > be loaded on your device" may also be better sounding than "won't work > on your machine" (with the quiet part being: "because someone hasn't > copied 5 lines of DTS") Then we need to make sure _not_ to make a default path useable, so that the users know that there is no proper firmware rather than facing the cryptic error of "firmware something -error". But... I'd rather prefer to keep firmware-backed nodes disabled exactly for the reason of "making someone copy 5 lines of DTS", which usually means that somebody has thought about how to get and where to put the binary. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node 2025-05-07 0:27 ` Dmitry Baryshkov @ 2025-05-08 14:49 ` Konrad Dybcio 0 siblings, 0 replies; 10+ messages in thread From: Konrad Dybcio @ 2025-05-08 14:49 UTC (permalink / raw) To: Dmitry Baryshkov, Konrad Dybcio Cc: Bjorn Andersson, neil.armstrong, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, devicetree, linux-kernel On 5/7/25 2:27 AM, Dmitry Baryshkov wrote: > On Wed, May 07, 2025 at 12:53:27AM +0200, Konrad Dybcio wrote: >> On 5/6/25 10:23 PM, Bjorn Andersson wrote: >>> On Mon, Apr 28, 2025 at 11:14:18PM +0200, Konrad Dybcio wrote: >>>> On 4/28/25 12:48 PM, Dmitry Baryshkov wrote: >>>>> On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 25/04/2025 23:49, Konrad Dybcio wrote: >>>>>>> On 4/24/25 6:32 PM, Neil Armstrong wrote: >>>>>>>> Add DT entries for the sm8650 iris decoder. >>>>>>>> >>>>>>>> Since the firmware is required to be signed, only enable >>>>>>>> on Qualcomm development boards where the firmware is >>>>>>>> available. >>>>>>>> >>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> >>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>>> --- >>>>>>>> Changes in v2: >>>>>>>> - removed useless firmware-name >>>>>>>> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org >>>>>>>> --- >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>> + iris: video-codec@aa00000 { >>>>>>>> + compatible = "qcom,sm8650-iris"; >>>>>>>> + reg = <0 0x0aa00000 0 0xf0000>; >>>>>>>> + >>>>>>>> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>; >>>>>>>> + >>>>>>>> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>, >>>>>>>> + <&videocc VIDEO_CC_MVS0_GDSC>, >>>>>>>> + <&rpmhpd RPMHPD_MXC>, >>>>>>>> + <&rpmhpd RPMHPD_MMCX>; >>>>>>>> + power-domain-names = "venus", >>>>>>>> + "vcodec0", >>>>>>>> + "mxc", >>>>>>>> + "mmcx"; >>>>>>>> + >>>>>>>> + operating-points-v2 = <&iris_opp_table>; >>>>>>>> + >>>>>>>> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, >>>>>>>> + <&videocc VIDEO_CC_MVS0C_CLK>, >>>>>>>> + <&videocc VIDEO_CC_MVS0_CLK>; >>>>>>>> + clock-names = "iface", >>>>>>>> + "core", >>>>>>>> + "vcodec0_core"; >>>>>>>> + >>>>>>>> + interconnects = <&gem_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"; >>>>>>>> + >>>>>>>> + /* FW load region */ >>>>>>> >>>>>>> I don't think this comment brings value >>>>>> >>>>>> Right >>>>>> >>>>>>> >>>>>>>> + memory-region = <&video_mem>; >>>>>>>> + >>>>>>>> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>, >>>>>>>> + <&videocc VIDEO_CC_XO_CLK_ARES>, >>>>>>>> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>; >>>>>>>> + reset-names = "bus", >>>>>>>> + "xo", >>>>>>>> + "core"; >>>>>>>> + >>>>>>>> + iommus = <&apps_smmu 0x1940 0>, >>>>>>>> + <&apps_smmu 0x1947 0>; >>>>>>> >>>>>>> I think you may also need 0x1942 0x0 (please also make the second value / SMR >>>>>>> mask hex)> + >>>>>> >>>>>> I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ? >>>> >>>> I saw it in docs only, maybe Vikash or Dikshita can chime in whether it's >>>> necessary. It would have mask 0x0 if so. >>>> >>>>>> >>>>>>>> + dma-coherent; >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * IRIS firmware is signed by vendors, only >>>>>>>> + * enable in boards where the proper signed firmware >>>>>>>> + * is available. >>>>>>>> + */ >>>>>>> >>>>>>> Here's to another angry media article :( >>>>>>> >>>>>>> Please keep Iris enabled.. Vikash reassured me this is not an >>>>>>> issue until the user attempts to use the decoder [1], and reading >>>>>>> the code myself I come to the same conclusion (though I haven't given >>>>>>> it a smoke test - please do that yourself, as you seem to have a better >>>>>>> set up with these platforms). >>>>>>> >>>>>>> If the userland is sane, it should throw an error and defer to CPU >>>>>>> decoding. >>>>>>> >>>>>>> This is >>unlike venus<< which if lacking firmware at probe (i.e. boot) >>>>>>> would prevent .sync_state >>>>>> >>>>>> Well sync with Bjorn who asked me to only enable on board with available firmware ;-) >>>>> >>>>> I'd second him here: if there is no firmware, don't enable the device. >>>>> It's better than the users having cryptic messages in the dmesg, >>>>> trying to understand why the driver errors out. >>>> >>>> I don't agree.. the firmware may appear later at boot (e.g. user installs a >>>> small rootfs and manually pulls in linux-firmware). Plus without the firmware, >>>> we can still power on and off the IP block, particularly achieve sync_state >>>> regardless of it >>>> >>> >>> Not "available during boot", but rather "available for a particular >>> board". >> >> I'd argue that if a device is in the hands of users, there already exists >> some acceptable set of fw binaries.. but most developers aren't in the >> position to upload them into l-f.. And quite frankly I'm not the biggest >> fan of having a gigabyte of 99%-the-same files with a dozen lines changed >> and a different signature prepended to them either.. >> >>> We generally avoid enabling device_nodes that depend on vendor-signed >>> firmware until someone has tested the device on such board and specified >>> the proper path to the vendor-specific firmware. >>> >>> Are you suggesting that we should leave this enabled on all boards for >>> some reason (perhaps to ensure that resources are adequately managed)? >> >> Yes, for that reason indeed. >> >> We don't generally need to load firmware to turn something *off*. And >> most IP blocks don't **actually** need to be presented with firmware at >> probe time (I can only think of external ICs like no-storage touch >> controllers that need the fw uploaded on each powerup). >> >> Telling the user "hey, this is supported but the firmware file can't >> be loaded on your device" may also be better sounding than "won't work >> on your machine" (with the quiet part being: "because someone hasn't >> copied 5 lines of DTS") > > Then we need to make sure _not_ to make a default path useable, so that > the users know that there is no proper firmware rather than facing the > cryptic error of "firmware something -error". > > But... I'd rather prefer to keep firmware-backed nodes disabled exactly > for the reason of "making someone copy 5 lines of DTS", which usually > means that somebody has thought about how to get and where to put the > binary. Fine, let's keep it disabled Konrad ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-05-08 14:49 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-24 16:32 [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node Neil Armstrong 2025-04-25 21:49 ` Konrad Dybcio 2025-04-28 8:18 ` Neil Armstrong 2025-04-28 10:48 ` Dmitry Baryshkov 2025-04-28 21:14 ` Konrad Dybcio 2025-04-29 12:22 ` Dmitry Baryshkov 2025-05-06 20:23 ` Bjorn Andersson 2025-05-06 22:53 ` Konrad Dybcio 2025-05-07 0:27 ` Dmitry Baryshkov 2025-05-08 14:49 ` Konrad Dybcio
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox