On 8/27/2024 4:12 PM, Krzysztof Kozlowski wrote: > On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote: >> From: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx> >> >> Add a schema description for the iris video encoder/decoder >> on sm8550. > > A nit, subject: drop second/last, redundant "dt sche,a". The > "dt-bindings" prefix is already stating that these are bindings/dt schema. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > And subject is neither correct nor complete. You did not add here SM8550 > SoC, but SM8550 Iris. Plus what is SM8550? TI SM8550? Samsung SM8550? > > You have entire commit subject to say briefly such details without > repeating obvious "dt schema". > Sure, will update the commit text with better description in next patch. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx> >> --- >> .../bindings/media/qcom,sm8550-iris.yaml | 162 +++++++++++++++++++++ >> 1 file changed, 162 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml >> new file mode 100644 >> index 000000000000..a7aa6a6190cf >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml >> @@ -0,0 +1,162 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/qcom,sm8550-iris.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm IRIS video encode and decode accelerators > > IRIS or Iris? Why it is every time written differently? > > If IRIS then explain the acronym in description. > It should be iris, will make consistent throughout the file. >> + >> +maintainers: >> + - Vikash Garodia <quic_vgarodia@xxxxxxxxxxx> >> + - Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx> >> + >> +description: | > > Do not need '|' unless you need to preserve formatting. > Ok. >> + The Iris video processing unit is a video encode and decode accelerator >> + present on Qualcomm platforms. >> + >> +allOf: >> + - $ref: qcom,venus-common.yaml# >> + >> +properties: >> + compatible: >> + oneOf: > > Drop oneOf > This was added so that in future we can add new compatible to the list where the same driver supports a different SOC with different compatible. is this not the correct way of doing it? >> + - enum: >> + - qcom,sm8550-iris >> + >> + power-domains: >> + maxItems: 4 >> + >> + power-domain-names: >> + oneOf: > > Drop oneOf > Sure >> + - items: >> + - const: venus >> + - const: vcodec0 >> + - const: mxc >> + - const: mmcx >> + >> + clocks: >> + maxItems: 3 >> + >> + clock-names: >> + items: >> + - const: iface >> + - const: core >> + - const: vcodec0_core >> + >> + interconnects: >> + maxItems: 2 >> + >> + interconnect-names: >> + items: >> + - const: cpu-cfg >> + - const: video-mem >> + >> + resets: >> + maxItems: 1 >> + >> + reset-names: >> + items: >> + - const: bus >> + >> + iommus: >> + maxItems: 2 >> + >> + dma-coherent: true >> + >> + operating-points-v2: true >> + >> + opp-table: >> + type: object >> + >> +required: >> + - compatible >> + - power-domain-names >> + - interconnects >> + - interconnect-names >> + - resets >> + - reset-names >> + - iommus >> + - dma-coherent >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/qcom,rpmh.h> >> + #include <dt-bindings/clock/qcom,sm8550-gcc.h> >> + #include <dt-bindings/clock/qcom,sm8450-videocc.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/interconnect/qcom,icc.h> >> + #include <dt-bindings/interconnect/qcom,sm8550-rpmh.h> >> + #include <dt-bindings/power/qcom-rpmpd.h> >> + #include <dt-bindings/power/qcom,rpmhpd.h> >> + >> + iris: video-codec@aa00000 { > > Drop unused label > This was referred from existing driver, if its not valid, can remove the iris label. >> + compatible = "qcom,sm8550-iris"; >> + > > No blank line here Ok, will remove. > >> + reg = <0x0aa00000 0xf0000>; >> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>; >> + >> + 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"; >> + >> + 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_ALWAYS >> + &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ALWAYS>, >> + <&mmss_noc MASTER_VIDEO QCOM_ICC_TAG_ALWAYS >> + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>; >> + interconnect-names = "cpu-cfg", "video-mem"; >> + >> + memory-region = <&video_mem>; >> + >> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>; >> + reset-names = "bus"; >> + >> + iommus = <&apps_smmu 0x1940 0x0000>, >> + <&apps_smmu 0x1947 0x0000>; >> + dma-coherent; >> + >> + operating-points-v2 = <&iris_opp_table>; >> + >> + iris_opp_table: opp-table { >> + compatible = "operating-points-v2"; >> + >> + opp-240000000 { >> + opp-hz = /bits/ 64 <240000000>; >> + required-opps = <&rpmhpd_opp_svs>, >> + <&rpmhpd_opp_low_svs>; >> + }; >> + >> + opp-338000000 { >> + opp-hz = /bits/ 64 <338000000>; >> + required-opps = <&rpmhpd_opp_svs>, >> + <&rpmhpd_opp_svs>; >> + }; >> + >> + opp-366000000 { >> + opp-hz = /bits/ 64 <366000000>; >> + required-opps = <&rpmhpd_opp_svs_l1>, >> + <&rpmhpd_opp_svs_l1>; >> + }; >> + >> + opp-444000000 { >> + opp-hz = /bits/ 64 <444000000>; >> + 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>; >> + }; > > Fix the indentation above. Sure, will fix. > >> + }; >> + }; >> +... >> > > Best regards, > Krzysztof > > Thanks, Dikshita
