Hi, Krzysztof.

-----Original Message-----
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: Thursday, February 20, 2025 5:32 PM
To: Nas Chung <nas.chung@chipsnmedia.com>
Cc: mchehab@kernel.org; hverkuil@xs4all.nl; sebastian.fricke@collabora.com;
robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; linux-
media@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-imx@nxp.com; linux-arm-
kernel@lists.infradead.org; jackson.lee [off-list ref];
lafley.kim [off-list ref]
Subject: Re: [PATCH 3/8] dt-bindings: media: nxp: Add Wave6 video codec
device

On 20/02/2025 08:35, Nas Chung wrote:

quoted

Hi, Krzysztof.

quoted

-----Original Message-----
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: Wednesday, February 19, 2025 9:32 PM
To: Nas Chung <nas.chung@chipsnmedia.com>
Cc: mchehab@kernel.org; hverkuil@xs4all.nl;

sebastian.fricke@collabora.com;

quoted

quoted

robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; linux-
media@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-imx@nxp.com; linux-arm-
kernel@lists.infradead.org; jackson.lee [off-list ref];
lafley.kim [off-list ref]
Subject: Re: [PATCH 3/8] dt-bindings: media: nxp: Add Wave6 video codec
device

On 18/02/2025 10:21, Nas Chung wrote:

quoted

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";

Why do you need compatible here? Could it be anything else?

I will update the driver based on the final DT.

quoted

quoted

          reg = <0x30000 0x10000>;

Where is the rest of resources? You created children only for one
resource - address space?

Sorry for the confusion.
I believe the final example looks like the one below.

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>;
          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";
          memory-region = <&vpu_boot>;
          #cooling-cells = <2>;
          sram = <&sram1>;
        };

        vpucore0: vpu-core@00000 {
          compatible = "nxp,imx95-vpu-core";
          reg = <0x00000 0x10000>;
          interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
          clocks = <&scmi_clk 115>,
                  <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;

These are the same resources for every block so for entire device. Why
they are no in top level node?

quoted

          clock-names = "vpu", "vpublk_wave";
          power-domains = <&scmi_devpd 21>;

Same here

Everything else also looks duplicated... All my previous comments - from
first email - stand. Don't design DT to match your Linu driver structure.

I agree with you. Thanks again for your feedback.

I've moved the shared resources to the parent node while keeping the
independent resources within their respective child nodes, as shown below.

vpu: video-codec@4c480000 {
        compatible = "nxp,imx95-vpu";
        reg = <0x0 0x4c480000 0x0 0x50000>;
        ranges = <0x0 0x0 0x4c480000 0x50000>;
        clocks = <&scmi_clk IMX95_CLK_VPU>,
                  <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
        clock-names = "vpu", "vpublk_wave";
        power-domains = <&scmi_devpd IMX95_PD_VPU>;

        vpucore0: vpu-core@00000 {
          compatible = "nxp,imx95-vpu-core";
          reg = <0x00000 0x10000>;
          interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
        };

        vpucore1: vpu-core@10000 {
          compatible = "nxp,imx95-vpu-core";
          reg = <0x10000 0x10000>;
          interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
        };

        vpucore2: vpu-core@20000 {
          compatible = "nxp,imx95-vpu-core";
          reg = <0x20000 0x10000>;
          interrupts = <GIC_SPI 301 IRQ_TYPE_LEVEL_HIGH>;
        };

        vpucore3: vpu-core@30000 {
          compatible = "nxp,imx95-vpu-core";
          reg = <0x30000 0x10000>;
          interrupts = <GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>;
        };

        vpuctrl: vpu-ctrl@40000 {
          compatible = "nxp,imx95-vpu-ctrl";
          reg = <0x40000 0x10000>;
          power-domains = <&scmi_perf IMX95_PERF_VPU>;
          memory-region = <&vpu_boot>;
          #cooling-cells = <2>;
          sram = <&sram1>;
        };
      };

Thanks.
Nas.

Best regards,
Krzysztof