linux-kernel - Re: [PATCH 1/8] media: dt-bindings: qcom-kaanapali-iris: Add kaanapali video codec binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <82c0cc6d-90e9-493f-4e09-32158a707c35@oss.qualcomm.com>
Date: Fri, 26 Sep 2025 00:53:51 +0530
From: Vikash Garodia <vikash.garodia@....qualcomm.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Dikshita Agarwal <dikshita.agarwal@....qualcomm.com>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        Bryan O'Donoghue <bod@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
 <conor+dt@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>, linux-arm-msm@...r.kernel.org,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Vishnu Reddy <quic_bvisredd@...cinc.com>
Subject: Re: [PATCH 1/8] media: dt-bindings: qcom-kaanapali-iris: Add
 kaanapali video codec binding


On 9/25/2025 2:38 PM, Krzysztof Kozlowski wrote:
> On Thu, 25 Sept 2025 at 08:16, Vikash Garodia
> <vikash.garodia@....qualcomm.com> wrote:
>>
>> Kaanapali SOC brings in the new generation of video IP i.e iris4. When
>> compared to previous generation, iris3x, it has,
>> - separate power domains for stream and pixel processing hardware blocks
>>   (bse and vpp).
>> - additional power domain for apv codec.
>> - power domains for individual pipes (VPPx).
>> - different clocks and reset lines.
>>
>> There are variants of this hardware, where only a single VPP pipe would
>> be functional (VPP0), and APV may not be present. In such case, the
>> hardware can be enabled without those 2 related power doamins, and
>> corresponding clocks. This explains the min entries for power domains
>> and clocks.
>> Iommus include all the different stream-ids which can be possibly
>> generated by vpu4 video hardware in both secure and non secure
>> execution mode.
>>
>> This patch depends on following patches
> 
> No, it cannot.
> 
> Don't send such patches then. We gave you already clear guidance how
> this is supposed to be solved.

The dependencies would be removed now in next revision.

> 
> 
>> https://lore.kernel.org/all/20250924-knp-interconnect-v1-1-4c822a72141c@oss.qualcomm.com/
>> https://lore.kernel.org/all/20250924-knp-clk-v1-3-29b02b818782@oss.qualcomm.com/
>>
>> Signed-off-by: Vikash Garodia <vikash.garodia@....qualcomm.com>
>> ---
>>  .../bindings/media/qcom,kaanapali-iris.yaml        | 236 +++++++++++++++++++++
>>  1 file changed, 236 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,kaanapali-iris.yaml b/Documentation/devicetree/bindings/media/qcom,kaanapali-iris.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..f3528d514fe29771227bee5f156962fedb1ea9cd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/qcom,kaanapali-iris.yaml
>> @@ -0,0 +1,236 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/qcom,kaanapali-iris.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm kaanapali iris video encode and decode accelerators
>> +
>> +maintainers:
>> +  - Vikash Garodia <vikash.garodia@....qualcomm.com>
>> +  - Dikshita Agarwal <dikshita.agarwal@....qualcomm.com>
>> +
>> +description:
>> +  The iris video processing unit is a video encode and decode accelerator
>> +  present on Qualcomm platforms.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,kaanapali-iris
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  power-domains:
>> +    minItems: 5
>> +    maxItems: 7
>> +
>> +  power-domain-names:
> 
> Wrong constraints, see writing bindings.

Got it, please review if below is fine

  power-domains:
    minItems: 5
    maxItems: 7
    description:
      Some of the power domains are optional(vpp1 and apv) and depends on the
hardware variants having them or not.

  power-domain-names:
    minItems: 5
    items:
      - const: venus
      - const: vcodec0
      - const: mxc
      - const: mmcx
      - const: vpp0
      - enum: [vpp1, apv]
      - enum: [vpp1, apv]

> 
>> +    items:
>> +      - const: venus
>> +      - const: vcodec0
>> +      - const: vpp0
>> +      - const: vpp1
>> +      - const: apv
>> +      - const: mxc
>> +      - const: mmcx
>> +
>> +  clocks:
>> +    minItems: 8
>> +    maxItems: 10
> 
> also wrong

Please review if below looks good

  clocks:
    minItems: 8
    maxItems: 10
    description:
      Some of the clocks are optional(vcodec_vpp1 and vcodec_apv) and depends on
the hardware variants having them or not.

  clock-names:
    minItems: 8
    items:
      - const: iface
      - const: core
      - const: vcodec0_core
      - const: iface1
      - const: core_freerun
      - const: vcodec0_core_freerun
      - const: vcodec_bse
      - const: vcodec_vpp0
      - enum: [vcodec_vpp1, vcodec_apv]
      - enum: [vcodec_vpp1, vcodec_apv]
> 
>> +
>> +  clock-names:
>> +    items:
>> +      - const: iface
>> +      - const: core
>> +      - const: vcodec0_core
>> +      - const: iface1
>> +      - const: core_freerun
>> +      - const: vcodec0_core_freerun
>> +      - const: vcodec_bse
>> +      - const: vcodec_vpp0
>> +      - const: vcodec_vpp1
>> +      - const: vcodec_apv
>> +
>> +  interconnects:
>> +    maxItems: 2
>> +
>> +  interconnect-names:
>> +    items:
>> +      - const: cpu-cfg
>> +      - const: video-mem
>> +
>> +  resets:
>> +    maxItems: 4
>> +
>> +  reset-names:
>> +    items:
>> +      - const: bus0
>> +      - const: bus1
>> +      - const: core_freerun_reset
>> +      - const: vcodec0_core_freerun_reset
>> +
>> +  iommus:
>> +    minItems: 3
>> +    maxItems: 8
> 
> No, you need to list the items.

Could you please elaborate how this is expected ? any reference would be
appreciated here.

> 
> You've been told that already in that discussion.
> 
> 
>> +
>> +  memory-region:
>> +    maxItems: 1
>> +
>> +  dma-coherent: true
>> +
>> +  operating-points-v2: true
>> +
>> +  opp-table:
>> +    type: object
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - power-domains
>> +  - power-domain-names
>> +  - clocks
>> +  - clock-names
>> +  - interconnects
>> +  - interconnect-names
>> +  - resets
>> +  - reset-names
>> +  - iommus
>> +  - dma-coherent
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interconnect/qcom,kaanapali-rpmh.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,kaanapali-gcc.h>
>> +    #include <dt-bindings/interconnect/qcom,icc.h>
>> +    #include <dt-bindings/power/qcom,rpmhpd.h>
>> +
>> +    video-codec@...0000 {
>> +      compatible = "qcom,kaanapali-iris";
>> +
> 
> Please read dts coding style and look how dts for Qualcomm is written.

made the changes w.r.t spacing alignment and alphabetical order, following the
reference of sm8750 iris yaml. It can be reviewed in next revision.

Regards,
Vikash
> 
>> +      reg = <0x02000000 0xf0000>;
>> +
>> +      interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +      power-domains = <&video_cc_mvs0c_gdsc>,
> ,
> Krzysztof