Message-ID:
 <SL2P216MB12469B8A2DD5B2F6EA2F04DCFB06A@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM>
Date: Tue, 2 Sep 2025 05:45:31 +0000
From: Nas Chung <nas.chung@...psnmedia.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, "mchehab@...nel.org"
	<mchehab@...nel.org>, "hverkuil@...all.nl" <hverkuil@...all.nl>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"shawnguo@...nel.org" <shawnguo@...nel.org>, "s.hauer@...gutronix.de"
	<s.hauer@...gutronix.de>
CC: "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 v3 2/9] dt-bindings: media: nxp: Add Wave6 video codec
 device

Hi, Krzysztof.

>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@...nel.org>
>Sent: Friday, August 29, 2025 10:57 PM
>To: Nas Chung <nas.chung@...psnmedia.com>; mchehab@...nel.org;
>hverkuil@...all.nl; robh@...nel.org; krzk+dt@...nel.org;
>conor+dt@...nel.org; shawnguo@...nel.org; s.hauer@...gutronix.de
>Cc: linux-media@...r.kernel.org; devicetree@...r.kernel.org; linux-
>kernel@...r.kernel.org; linux-imx@....com; linux-arm-
>kernel@...ts.infradead.org; jackson.lee <jackson.lee@...psnmedia.com>;
>lafley.kim <lafley.kim@...psnmedia.com>
>Subject: Re: [PATCH v3 2/9] dt-bindings: media: nxp: Add Wave6 video codec
>device
>
>On 29/08/2025 10:46, Nas Chung wrote:
>> Add documents for the Wave6 video codec on NXP i.MX SoCs.
>Pretty incomplete commit msg. Nothing explaining hardware, nothing
>documenting resolution of previous discussions (where is all this
>chip&media?).

I see,  I'll improve the commit message in v4 to include hardware details.

>
>...
>
>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - nxp,imx95-vpu
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  memory-region:
>> +    maxItems: 1
>> +
>> +  sram:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: phandle of the SRAM memory region node.
>> +
>> +  "#cooling-cells":
>> +    const: 2
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 2
>> +
>> +  ranges: true
>> +
>> +patternProperties:
>> +  "^video-core@[0-9a-f]+$":
>> +    type: object
>
>Missing description.

I'll add a description in v4.

>
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - nxp,imx95-vpu-core
>
>Why do you need here compatible? Can this child be anything else? Can it
>be re-used? Is it actually a separate block?
>
>Your example suggests that the only distinctive resource are the
>interrupt and address space and that's on the edge of calling it a
>separate device.
>
>There is some tendency to call such "pseudo-cores" a separate devices in
>case of video codec bindings and experience shows these are usually
>fake. It's not the same as DP or HDMI sub-block of display pipeline.
>
>That's why you should come here with strong argument what separate piece
>of hardware this is.

Thanks for your feedback.

As you mentioned, I wanted to represent the interrupts and address space
as separate "cores". This is because, from an external perspective (e.g. multi-VM),
each of these resources is a VPU interface and can be accessed independently
to operate the VPU.

However, there is indeed only one actual VPU processing engine.
I understand your point about "pseudo-cores".

I would appreciate any guidance on the preferred way to represent
these resources in the device tree.

>
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +      clocks:
>> +        maxItems: 1
>> +
>> +      power-domains:
>> +        maxItems: 1
>> +
>> +      interrupts:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - clocks
>> +      - power-domains
>> +      - interrupts
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - power-domains
>> +  - memory-region
>> +
>> +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@...c0000 {
>
>Unused label, drop

Okay. I'll drop the unused label.

>
>> +        compatible = "nxp,imx95-vpu";
>> +        reg = <0x0 0x4c4c0000 0x0 0x10000>;
>> +        clocks = <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
>> +        power-domains = <&scmi_perf 10>;
>> +        memory-region = <&vpu_boot>;
>> +        sram = <&sram1>;
>> +        #cooling-cells = <2>;
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        ranges;
>> +
>> +        vpucore0: video-core@...80000 {
>
>None of these labels are used, drop.

I'll drop it.

Thanks,
Nas.

>
>> +          compatible = "nxp,imx95-vpu-core";
>> +          reg = <0x0 0x4c480000 0x0 0x10000>;
>> +          clocks = <&scmi_clk 115>;
>> +          power-domains = <&scmi_devpd 21>;
>> +          interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
>> +        };
>> +
>> +        vpucore1: video-core@...90000 {
>> +          compatible = "nxp,imx95-vpu-core";
>> +          reg = <0x0 0x4c490000 0x0 0x10000>;
>> +          clocks = <&scmi_clk 115>;
>> +          power-domains = <&scmi_devpd 21>;
>> +          interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
>> +        };
>> +
>
>
>
>Best regards,
>Krzysztof

Powered by blists - more mailing lists