| Message ID | 20240827-iris_v3-v3-1-c5fdbbe65e70@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Qualcomm iris video decoder driver | expand |
On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote: > From: Dikshita Agarwal <quic_dikshita@quicinc.com> > > 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". > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- > .../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. > + > +maintainers: > + - Vikash Garodia <quic_vgarodia@quicinc.com> > + - Dikshita Agarwal <quic_dikshita@quicinc.com> > + > +description: | Do not need '|' unless you need to preserve formatting. > + 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 > + - enum: > + - qcom,sm8550-iris > + > + power-domains: > + maxItems: 4 > + > + power-domain-names: > + oneOf: Drop oneOf > + - 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 > + compatible = "qcom,sm8550-iris"; > + No blank line here > + 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. > + }; > + }; > +... > Best regards, Krzysztof
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@quicinc.com> >> >> 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@quicinc.com> >> --- >> .../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@quicinc.com> >> + - Dikshita Agarwal <quic_dikshita@quicinc.com> >> + >> +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
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 + +maintainers: + - Vikash Garodia <quic_vgarodia@quicinc.com> + - Dikshita Agarwal <quic_dikshita@quicinc.com> + +description: | + 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: + - enum: + - qcom,sm8550-iris + + power-domains: + maxItems: 4 + + power-domain-names: + oneOf: + - 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 { + compatible = "qcom,sm8550-iris"; + + 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>; + }; + }; + }; +...