[v3,01/29] dt-bindings: media: Add sm8550 dt schema

  From: Dikshita Agarwal <quic_dikshita@quicinc.com>

Add a schema description for the iris video encoder/decoder
on sm8550.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 .../bindings/media/qcom,sm8550-iris.yaml           | 162 +++++++++++++++++++++
 1 file changed, 162 insertions(+)
  

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