| 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
| ||
|
Message-ID: <20250213-imaginary-shrimp-of-merriment-6ccb6f@krzk-bin>
Date: Thu, 13 Feb 2025 09:49:50 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nas Chung <nas.chung@...psnmedia.com>
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>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "jackson.lee" <jackson.lee@...psnmedia.com>,
"lafley.kim" <lafley.kim@...psnmedia.com>
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.
>
> >
> >> +
> >> + 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!
>
> >
> >> +
> >> + 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@...c0000 {
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@...80000 {
And here you have child which is not a child? Your binding and DTS
neither match nor make any sense.
> >
> >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@...90000 {
> >> + 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.
>
> >
> >
> >> + 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@...a0000 {
> >> + 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@...b0000 {
> >> + 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@...psnmedia.com>
> >> +M: Jackson Lee <jackson.lee@...psnmedia.com>
> >> +L: linux-media@...r.kernel.org
> >> +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.
Best regards,
Krzysztof
Powered by blists - more mailing lists