linux-kernel - Re: [PATCH v2 2/8] dt-bindings: media: nxp: Add Wave6 video codec device

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f1073f21-0885-486f-80c8-00f91dfd7448@kernel.org>
Date: Fri, 9 May 2025 12:11:48 +0200
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>, "marex@...x.de" <marex@...x.de>,
 "jackson.lee" <jackson.lee@...psnmedia.com>,
 "lafley.kim" <lafley.kim@...psnmedia.com>
Subject: Re: [PATCH v2 2/8] dt-bindings: media: nxp: Add Wave6 video codec
 device

On 09/05/2025 11:59, Nas Chung wrote:
>>> +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 {
>>
>> Why this device does not have MMIO? Sorry, but makes little sense and if
>> you posted and tested your entire DTS you would see why.
> 
> I initially thought that if the reg property is declared in the child,
> it would not need to be declared in the parent node.
> I based this approach on the mediatek,mt8195-jpegenc.yaml binding,
> where the parent node does not include MMIO.

Do you have access to mt8195 datasheet? What is the memory/register
layout there?

If you do not have access, why do you think these are similar devices?

> 
> But, if this structure is problematic, I will address it in patch v3.
> 
>>
>> Can we see the entire DTS?
> 
> Sure !
> Below is the cnm.wave633c.example.dts file created from dt_binding_check.

This is not the entire DTS.

> 
> /dts-v1/;
> /plugin/; // silence any missing phandle references

This is bindings example. I want to see entire DTS or DTSI of the SoC.
Once you see entire DTS, you will notice that your current split is just
not correct - it should be pretty obvious.

And that's why we should keep rejecting such works which do not bring
any DTS user, because the no one - neither we nor the contributors - see
big picture and if someone saw the big picture then immediately would
notice - it's just bollocks.

What you claim is:

soc@0 {
  // MMIO bus

  video-codec {
    // which is a non-MMIO device and clearly a no-go.

Just look how DTS is organized and learn from it.

> 
> /{
>     compatible = "foo";
>     model = "foo";
>     #address-cells = <1>;
>     #size-cells = <1>;
> 
> 
>     example-0 {
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         
>         interrupt-parent = <&fake_intc0>;
>         fake_intc0: fake-interrupt-controller {
>             interrupt-controller;
>             #interrupt-cells = < 3 >;
>         };
> 

All of above are wrong for the SoC...

> 
>         #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 {
>             compatible = "nxp,imx95-vpu", "cnm,wave633c";

What does this device represent? It is not "ctrl", because you made ctrl
separate device node. Your binding description suggests that is the VPU
control region.

>             clocks = <&scmi_clk 115>,
>                      <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;

For which sub devices these clocks are valid? For all of them?

>             clock-names = "vpu", "vpublk_wave";
>             power-domains = <&scmi_devpd 21>;
>             #address-cells = <2>;
>             #size-cells = <2>;
>             ranges;
>         
>             vpucore0: video-core@...80000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c480000 0x0 0x10000>;
>               interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpucore1: video-core@...90000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c490000 0x0 0x10000>;
>               interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpucore2: video-core@...a0000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c4a0000 0x0 0x10000>;
>               interrupts = <GIC_SPI 301 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpucore3: video-core@...b0000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c4b0000 0x0 0x10000>;
>               interrupts = <GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpuctrl: video-controller@...c0000 {
>               compatible = "nxp,imx95-vpu-ctrl";
>               reg = <0x0 0x4c4c0000 0x0 0x10000>;
>               memory-region = <&vpu_boot>;
>               power-domains = <&scmi_perf 10>;
>               #cooling-cells = <2>;
>               sram = <&sram1>;
>             };
>           };
>         };
> 
>     };
> };
> 
>>
>> Best regards,
>> Krzysztof
> 


Best regards,
Krzysztof