Hi, Krzysztof.
>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>Sent: Thursday, February 13, 2025 5:50 PM
>To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>Cc: mchehab@xxxxxxxxxx; hverkuil@xxxxxxxxx; sebastian.fricke@xxxxxxxxxxxxx;
>robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; 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 Thu, Feb 13, 2025 at 07:50:53AM +0000, Nas Chung wrote:
>> >> + 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.
>
>That's a no. Bindings are for hardware, not drivers.
>Linux driver design is independent of bindings.
>
>>
>> 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)
>
>This could be, but your binding said something completely opposite. Look
>how other bindings do it, first.
>
>>
>> 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)
>
>No, it changes nothing.
>
>>
>> >
>> >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.
>
>OK
>
>
>> By using the compatible name, the same Wave6 driver can distinguish
>> different implementations.
>
>So you tell DT maintainer how DT works, brilliant...
>
>>
>> However, I agree that "imx95" is not strictly necessary in current status.
>> So, using "nxp,wave633c" would be a better choice, right ?
>
>No, NXP did not create wave633c. SoC components must have SoC prefix,
>assuming this is a Soc component.
Ah, I had completely misunderstood the purpose and role of DT.
My apologies for that.
Would it be okay to modify it as follows ?
items:
- enum:
- nxp,imx95-vpu
- const: cnm,wave633c
>
>>
>> >
>> >> +
>> >> + 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.
>
>Then NAK.
>
>Use proper bindings for this, e.g. 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,
>
>No. Splitting into separate files would change nothing - you still would
>have here a phandle, right?
>
>> or that I should structure them in a parent-child hierarchy
>> within the same YAML file ?
>
>You did not try hard enough to find similar devices, which there is a
>ton of.
>
>>
>> I appreciate any guidance on the best approach to structure this in line
>> with existing bindings.
>
>Then look for them.
>
>You have one device. If you have sub-blocks with their own
>distinguishable address space, then they can be children. But you did
>not write it that way. Just look at your example DTS - no children at
>all!
I see that I didn't review similar devices thoroughly enough.
Thanks for pointing this out.
Okay, I will remove the phandle for second device and handle it in
parent node.
I have left the modified structure in the example below.
>
>>
>> >
>> >> +
>> >> + 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 ?
>
>Look how other bindings do this and what property they use. Yes,
>memory-region.
>
>>
>> >
>> >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 {
>
>So this is the parent device...
>
>> >> + 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 {
>
>
>And here you have child which is not a child? Your binding and DTS
>neither match nor make any sense.
Would it be reasonable to modify it as follows ?
For example:
vpu: video-codec@4c480000 {
compatible = "nxp,imx95-vpu";
reg = <0x0 0x4c480000 0x0 0x50000>;
ranges = <0x0 0x0 0x4c480000 0x50000>;
vpuctrl: vpu-ctrl@40000 {
compatible = "nxp,imx95-vpu-ctrl";
reg = <0x40000 0x10000>;
};
vpucore0: vpu-core@00000 {
compatible = "nxp,imx95-vpu-core";
reg = <0x00000 0x10000>;
};
vpucore1: vpu-core@10000 {
compatible = "nxp,imx95-vpu-core";
reg = <0x10000 0x10000>;
};
vpucore2: vpu-core@20000 {
compatible = "nxp,imx95-vpu-core";
reg = <0x20000 0x10000>;
};
vpucore3: vpu-core@30000 {
compatible = "nxp,imx95-vpu-core";
reg = <0x30000 0x10000>;
};
};
>
>> >
>> >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.
>
>They are redundant in this example. Unless you wanted to express
>something else, but you did not.
Got it.
>
>
>>
>> >
>> >
>> >> + 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.
>
>No, just add entry per entry.
I'll add each entry separately in v2.
Thanks.
Nas.
>
>Best regards,
>Krzysztof
