Hi, Krzysztof. >-----Original Message----- >From: Krzysztof Kozlowski <krzk@xxxxxxxxxx> >Sent: Tuesday, February 11, 2025 2:31 AM >To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx; >hverkuil@xxxxxxxxx; sebastian.fricke@xxxxxxxxxxxxx; robh@xxxxxxxxxx; >krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx >Cc: linux-media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx; linux-imx@xxxxxxx; linux-arm- >kernel@xxxxxxxxxxxxxxxxxxx; jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; >lafley.kim <lafley.kim@xxxxxxxxxxxxxxx> >Subject: Re: [PATCH 3/8] dt-bindings: media: nxp: Add Wave6 video codec >device > >On 10/02/2025 10:07, Nas Chung wrote: >> Add documents for the Wave6 video codec on NXP i.MX SoCs. >> >> Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx> >> --- >> .../bindings/media/nxp,wave633c.yaml | 202 ++++++++++++++++++ >> MAINTAINERS | 8 + >> 2 files changed, 210 insertions(+) >> create mode 100644 >Documentation/devicetree/bindings/media/nxp,wave633c.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/nxp,wave633c.yaml >b/Documentation/devicetree/bindings/media/nxp,wave633c.yaml >> new file mode 100644 >> index 000000000000..99c3008314c5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/nxp,wave633c.yaml > >Filename matching compatible. > >> @@ -0,0 +1,202 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/nxp,wave633c.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Chips&Media Wave6 Series multi-standard codec IP on NXP i.MX SoCs. >> + >> +maintainers: >> + - Nas Chung <nas.chung@xxxxxxxxxxxxxxx> >> + - Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx> >> + >> +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 the >VPU control device >> + (vpu-ctrl) and the VPU device (vpu). The VPU control device manages >shared resources >> + such as firmware access and power domains, while the VPU device >provides encoding >> + and decoding capabilities. The VPU devie cannot operate independently > >Typo I'll fix it in v2. > >> + without the VPU control device. >> + > >Please wrap code according to the preferred limit expressed in Kernel >coding style (checkpatch is not a coding style description, but only a >tool). Bindings use strict rule here. I'll address this in v2. > > > >> +properties: >> + compatible: >> + items: >> + - enum: >> + - nxp,imx95-wave633c-ctrl >> + - nxp,imx95-wave633c > >I don't understand why you duplicated compatibles. You split this for >driver? That's a no. There are no two hardwares. Yes, I want to introduce two different devices and drivers, even though there is only one hardware. Wave6 IP has five independent register regions: One register region is dedicated to the control device, which manages shared resources such as firmware loading and power domains. The remaining four register regions are assigned to four independent VPU devices, each accessing its own dedicated region. (to support 4 vms) Would it be reasonable to split the YAML into separate files for the VPU control device and the VPU device ? (like nxp,wave633c-ctrl.yaml) > >These compatibles are anyway weird - why imx95 is in chipmedia product? >Is this part of a SoC? I want to represent that the Wave633 is part of the i.MX95. Chips&Media's Wave633 can also be integrated into SoCs from other vendors. By using the compatible name, the same Wave6 driver can distinguish different implementations. However, I agree that "imx95" is not strictly necessary in current status. So, using "nxp,wave633c" would be a better choice, right ? > >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: VPU clock >> + - description: VPU associated block clock >> + >> + clock-names: >> + items: >> + - const: vpu >> + - const: vpublk_wave >> + >> + power-domains: >> + minItems: 1 >> + items: >> + - description: Main VPU power domain >> + - description: Performance power domain >> + >> + power-domain-names: >> + items: >> + - const: vpumix >> + - const: vpuperf >> + >> + cnm,ctrl: > >What is this prefix about? Is this nxp or something else? Yes, using "nxp" as the prefix seems more appropriate. > >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: phandle of the VPU control device node. Required for VPU >operation. > >Explain - required for what. Operation is too generic. phandle of the VPU control device node, which manages shared resources such as firmware access and power domains. > >If this is phandle to second device, then it's proof your split is >really wrong. Are you suggesting that I should separate them into two YAML files, or that I should structure them in a parent-child hierarchy within the same YAML file ? I appreciate any guidance on the best approach to structure this in line with existing bindings. > >> + >> + boot: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: phandle of the boot memory region node for the VPU >control device. > >No clue what is this... if memory region then use existing bindings. Boot is a memory region used for firmware upload. Only the control device can access this region. By "existing bindings," do you mean I should use "memory-region" instead ? > >Anyway, wrap your code correctly. Okay. > >> + >> + sram: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: phandle of the SRAM memory region node for the VPU >control device. >> + >> + '#cooling-cells': >> + const: 2 >> + >> + support-follower: >> + type: boolean >> + description: Indicates whether the VPU domain power always on. > >You cannot add new common properties in random way. Missing vendor >prefix but more important: does not look at all as hardware property but >OS policy. I agree with you. I'll remove it in v2. > >> + >> +patternProperties: >> + "^vpu-ctrl@[0-9a-f]+$": >> + type: object >> + properties: >> + compatible: >> + items: >> + - enum: >> + - nxp,imx95-wave633c-ctrl > >Really, what? How nxp,imx95-wave633c-ctrl node can have a child with >nxp,imx95-wave633c-ctrl compatible? > >NAK. Apologies, I misunderstood the meaning of 'patternProperties'. I'll remove it in v2. > > >> + reg: true >> + clocks: true >> + clock-names: true >> + power-domains: >> + items: >> + - description: Main VPU power domain >> + - description: Performance power domain >> + power-domain-names: >> + items: >> + - const: vpumix >> + - const: vpuperf >> + sram: true >> + boot: true >> + '#cooling-cells': true >> + support-follower: true >> + required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - power-domains >> + - power-domain-names >> + - sram >> + - boot >> + >> + additionalProperties: false >> + >> + "^vpu@[0-9a-f]+$": >> + type: object >> + properties: >> + compatible: >> + items: >> + - enum: >> + - nxp,imx95-wave633c >> + reg: true >> + interrupts: true >> + clocks: true >> + clock-names: true >> + power-domains: >> + maxItems: 1 >> + description: Main VPU power domain >> + cnm,ctrl: true >> + required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + - clock-names >> + - power-domains >> + - cnm,ctrl > >All this is just incorrect. I'll remove it in v2. > >> + >> + additionalProperties: false >> + >> +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>; >> + >> + vpuctrl: vpu-ctrl@4c4c0000 { >> + compatible = "nxp,imx95-wave633c-ctrl"; >> + reg = <0x0 0x4c4c0000 0x0 0x10000>; >> + clocks = <&scmi_clk 115>, >> + <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>; >> + clock-names = "vpu", "vpublk_wave"; >> + power-domains = <&scmi_devpd 21>, <&scmi_perf 10>; >> + power-domain-names = "vpumix", "vpuperf"; >> + #cooling-cells = <2>; >> + boot = <&vpu_boot>; >> + sram = <&sram1>; >> + }; >> + >> + vpu0: vpu@4c480000 { > >Node names should be generic. See also an explanation and list of >examples (not exhaustive) in DT specification: >https://devicetree-specification.readthedocs.io/en/latest/chapter2- >devicetree-basics.html#generic-names-recommendation Thanks for sharing the link. I'll use "video-codec" as the node name in v2. > > >> + compatible = "nxp,imx95-wave633c"; >> + reg = <0x0 0x4c480000 0x0 0x10000>; >> + interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&scmi_clk 115>, >> + <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>; >> + clock-names = "vpu", "vpublk_wave"; >> + power-domains = <&scmi_devpd 21>; >> + cnm,ctrl = <&vpuctrl>; >> + }; >> + >> + vpu1: vpu@4c490000 { >> + compatible = "nxp,imx95-wave633c"; > >Drop all duplicated examples. Wave6 HW is designed for simultaneous access, as each VPU device has its own separate register space. Therefore, I defined the 4 VPU devices as independent nodes in the example to reflect this. > > >> + reg = <0x0 0x4c490000 0x0 0x10000>; >> + interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&scmi_clk 115>, >> + <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>; >> + clock-names = "vpu", "vpublk_wave"; >> + power-domains = <&scmi_devpd 21>; >> + cnm,ctrl = <&vpuctrl>; >> + }; >> + >> + vpu2: vpu@4c4a0000 { >> + compatible = "nxp,imx95-wave633c"; >> + reg = <0x0 0x4c4a0000 0x0 0x10000>; >> + interrupts = <GIC_SPI 301 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&scmi_clk 115>, >> + <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>; >> + clock-names = "vpu", "vpublk_wave"; >> + power-domains = <&scmi_devpd 21>; >> + cnm,ctrl = <&vpuctrl>; >> + }; >> + >> + vpu3: vpu@4c4b0000 { >> + compatible = "nxp,imx95-wave633c"; >> + reg = <0x0 0x4c4b0000 0x0 0x10000>; >> + interrupts = <GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&scmi_clk 115>, >> + <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>; >> + clock-names = "vpu", "vpublk_wave"; >> + power-domains = <&scmi_devpd 21>; >> + cnm,ctrl = <&vpuctrl>; >> + }; >> + }; >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 896a307fa065..5ff5b1f1ced2 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -25462,6 +25462,14 @@ S: Maintained >> F: Documentation/devicetree/bindings/media/cnm,wave521c.yaml >> F: drivers/media/platform/chips-media/wave5/ >> >> +WAVE6 VPU CODEC DRIVER >> +M: Nas Chung <nas.chung@xxxxxxxxxxxxxxx> >> +M: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx> >> +L: linux-media@xxxxxxxxxxxxxxx >> +S: Maintained >> +F: Documentation/devicetree/bindings/media/nxp,wave633c.yaml >> +F: drivers/media/platform/chips-media/wave6/ > >There is no such file/directory. Understood. I'll move the MAINTAINERS update to the last patch in v2. > >Best regards, >Krzysztof
