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:
 <SL2P216MB124656A87931B153F815820BFB8AA@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM>
Date: Fri, 9 May 2025 09:59:35 +0000
From: Nas Chung <nas.chung@...psnmedia.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: "mchehab@...nel.org" <mchehab@...nel.org>, "hverkuil@...all.nl"
	<hverkuil@...all.nl>, "sebastian.fricke@...labora.com"
	<sebastian.fricke@...labora.com>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-imx@....com" <linux-imx@....com>,
	"marex@...x.de" <marex@...x.de>, jackson.lee <jackson.lee@...psnmedia.com>,
	lafley.kim <lafley.kim@...psnmedia.com>
Subject: RE: [PATCH v2 2/8] dt-bindings: media: nxp: Add Wave6 video codec
 device

Hi, Krzysztof.

Thanks for your feedback.

>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@...nel.org>
>Sent: Friday, April 25, 2025 7:34 PM
>To: Nas Chung <nas.chung@...psnmedia.com>
>Cc: mchehab@...nel.org; hverkuil@...all.nl; sebastian.fricke@...labora.com;
>robh@...nel.org; krzk+dt@...nel.org; conor+dt@...nel.org; linux-
>media@...r.kernel.org; devicetree@...r.kernel.org; linux-
>kernel@...r.kernel.org; linux-imx@....com; marex@...x.de; jackson.lee
><jackson.lee@...psnmedia.com>; lafley.kim <lafley.kim@...psnmedia.com>
>Subject: Re: [PATCH v2 2/8] dt-bindings: media: nxp: Add Wave6 video codec
>device
>
>On Tue, Apr 22, 2025 at 06:31:13PM GMT, Nas Chung wrote:
>> Add documents for the Wave6 video codec on NXP i.MX SoCs.
>>
>> Signed-off-by: Nas Chung <nas.chung@...psnmedia.com>
>> ---
>>  .../bindings/media/cnm,wave633c.yaml          | 165 ++++++++++++++++++
>>  MAINTAINERS                                   |   7 +
>>  2 files changed, 172 insertions(+)
>>  create mode 100644
>Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>b/Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>> new file mode 100644
>> index 000000000000..5bb572e8ca18
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>> @@ -0,0 +1,165 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/cnm,wave633c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Chips&Media Wave6 Series multi-standard codec IP.
>
>Drop full stop

I see, I will remove it.

>
>> +
>> +maintainers:
>> +  - Nas Chung <nas.chung@...psnmedia.com>
>> +  - Jackson Lee <jackson.lee@...psnmedia.com>
>> +
>> +description:
>> +  The Chips&Media Wave6 codec IP is a multi-standard video
>encoder/decoder.
>> +  On NXP i.MX SoCs, Wave6 codec IP functionality is split between
>
>... this and ...
>
>> +  the VPU control region and the VPU core region.
>> +  The VPU control region manages shared resources such as firmware
>memory,
>> +  while the VPU core region provides encoding and decoding
>> +  capabilities. The VPU core cannot operate independently without
>> +  the VPU control region.
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - nxp,imx95-vpu
>> +      - const: cnm,wave633c
>
>... your drivers seem to use soc specific compatible, so I do not see
>value in generic compatible. It would have to be good enough alone for
>drivers, but it is not.

I agree with you. I will drop the generic compatible name and
change the yaml file name to nxp,imx95-vpu.yaml.

>
>> +
>> +  clocks:
>> +    items:
>> +      - description: VPU clock
>> +      - description: VPU associated block clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: vpu
>> +      - const: vpublk_wave
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +    description: Main VPU power domain
>
>Drop description

Okay. I will remove the description for power-domains.

>
>> +
>> +  "#address-cells": true
>
>instead const
>
>> +
>> +  "#size-cells": true
>
>const

I see, I will change "#address-cells" and "#size-cells" to use const.

>
>> +
>> +  ranges: true
>> +
>> +patternProperties:
>> +  "^video-core@[0-9a-f]+$":
>> +    type: object
>> +
>> +    properties:
>> +      compatible:
>> +        items:
>
>Drop items and keep just enum

Okay, I will use enum directly.

>
>> +          - enum:
>> +              - nxp,imx95-vpu-core
>
>So this is another proof that cnm,wave633c is wrong. How cnm,wave633c
>can come with nxp,imx95-vpu-core child?

I understand your point.
I will address this in my previous comment. Thanks for pointing it out.

>
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +      interrupts:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - interrupts
>> +
>> +    additionalProperties: false
>
>Put this immediately after type:object

Okay.

>
>> +
>> +  "^video-controller@[0-9a-f]+$":
>> +    type: object
>
>Same here goes additionalProps.

Okay.

>
>> +
>> +    properties:
>> +      compatible:
>> +        items:
>> +          - enum:
>> +              - nxp,imx95-vpu-ctrl
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +      memory-region:
>> +        maxItems: 1
>> +
>> +      power-domains:
>> +        maxItems: 1
>> +        description: Performance power domain
>> +
>> +      '#cooling-cells':
>
>Keep consistent quotes, either ' or "

I see, I will use consistent quoting.

>
>> +        const: 2
>> +
>> +      sram:
>> +        $ref: /schemas/types.yaml#/definitions/phandle
>> +        description: phandle of the SRAM memory region node.
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - memory-region
>> +      - power-domains
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - power-domains
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/nxp,imx95-clock.h>
>> +
>> +    soc {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      vpu: video-codec {
>
>Why this device does not have MMIO? Sorry, but makes little sense and if
>you posted and tested your entire DTS you would see why.

I initially thought that if the reg property is declared in the child,
it would not need to be declared in the parent node.
I based this approach on the mediatek,mt8195-jpegenc.yaml binding,
where the parent node does not include MMIO.

But, if this structure is problematic, I will address it in patch v3.

>
>Can we see the entire DTS?

Sure !
Below is the cnm.wave633c.example.dts file created from dt_binding_check.

/dts-v1/;
/plugin/; // silence any missing phandle references

/{
    compatible = "foo";
    model = "foo";
    #address-cells = <1>;
    #size-cells = <1>;


    example-0 {
        #address-cells = <1>;
        #size-cells = <1>;

        
        interrupt-parent = <&fake_intc0>;
        fake_intc0: fake-interrupt-controller {
            interrupt-controller;
            #interrupt-cells = < 3 >;
        };


        #include <dt-bindings/interrupt-controller/arm-gic.h>
        #include <dt-bindings/clock/nxp,imx95-clock.h>
        
        soc {
          #address-cells = <2>;
          #size-cells = <2>;
        
          vpu: video-codec {
            compatible = "nxp,imx95-vpu", "cnm,wave633c";
            clocks = <&scmi_clk 115>,
                     <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
            clock-names = "vpu", "vpublk_wave";
            power-domains = <&scmi_devpd 21>;
            #address-cells = <2>;
            #size-cells = <2>;
            ranges;
        
            vpucore0: video-core@...80000 {
              compatible = "nxp,imx95-vpu-core";
              reg = <0x0 0x4c480000 0x0 0x10000>;
              interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
            };
        
            vpucore1: video-core@...90000 {
              compatible = "nxp,imx95-vpu-core";
              reg = <0x0 0x4c490000 0x0 0x10000>;
              interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
            };
        
            vpucore2: video-core@...a0000 {
              compatible = "nxp,imx95-vpu-core";
              reg = <0x0 0x4c4a0000 0x0 0x10000>;
              interrupts = <GIC_SPI 301 IRQ_TYPE_LEVEL_HIGH>;
            };
        
            vpucore3: video-core@...b0000 {
              compatible = "nxp,imx95-vpu-core";
              reg = <0x0 0x4c4b0000 0x0 0x10000>;
              interrupts = <GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>;
            };
        
            vpuctrl: video-controller@...c0000 {
              compatible = "nxp,imx95-vpu-ctrl";
              reg = <0x0 0x4c4c0000 0x0 0x10000>;
              memory-region = <&vpu_boot>;
              power-domains = <&scmi_perf 10>;
              #cooling-cells = <2>;
              sram = <&sram1>;
            };
          };
        };

    };
};

>
>Best regards,
>Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ