[<prev] [next>] [<thread-prev] [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
Powered by blists - more mailing lists