| Message ID | 20230822085949.816844-2-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/imx: Introduce i.MX8qm/qxp DPU DRM | expand |
Hi, On Tue, Aug 22, 2023 at 04:59:44PM +0800, Liu Ying wrote: > This patch adds bindings for i.MX8qxp/qm Display Processing Unit. > > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v7->v14: > * No change. > > v6->v7: > * Add Rob's R-b tag back. > > v5->v6: > * Use graph schema. So, drop Rob's R-b tag as review is needed. > > v4->v5: > * No change. > > v3->v4: > * Improve compatible property by using enum instead of oneOf+const. (Rob) > * Add Rob's R-b tag. > > v2->v3: > * No change. > > v1->v2: > * Fix yamllint warnings. > * Require bypass0 and bypass1 clocks for both i.MX8qxp and i.MX8qm, as the > display controller subsystem spec does say that they exist. > * Use new dt binding way to add clocks in the example. > * Trivial tweaks for the example. > > .../bindings/display/imx/fsl,imx8qxp-dpu.yaml | 387 ++++++++++++++++++ > 1 file changed, 387 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml > > diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml > new file mode 100644 > index 000000000000..6b05c586cd9d > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml > @@ -0,0 +1,387 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dpu.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Freescale i.MX8qm/qxp Display Processing Unit > + > +maintainers: > + - Liu Ying <victor.liu@nxp.com> > + > +description: | > + The Freescale i.MX8qm/qxp Display Processing Unit(DPU) is comprised of two > + main components that include a blit engine for 2D graphics accelerations > + and a display controller for display output processing, as well as a command > + sequencer. > + > +properties: > + compatible: > + enum: > + - fsl,imx8qxp-dpu > + - fsl,imx8qm-dpu > + > + reg: > + maxItems: 1 > + > + interrupts: > + items: > + - description: | > + store9 shadow load interrupt(blit engine) > + - description: | > + store9 frame complete interrupt(blit engine) > + - description: | > + store9 sequence complete interrupt(blit engine) > + - description: | > + extdst0 shadow load interrupt > + (display controller, content stream 0) > + - description: | > + extdst0 frame complete interrupt > + (display controller, content stream 0) > + - description: | > + extdst0 sequence complete interrupt > + (display controller, content stream 0) > + - description: | > + extdst4 shadow load interrupt > + (display controller, safety stream 0) > + - description: | > + extdst4 frame complete interrupt > + (display controller, safety stream 0) > + - description: | > + extdst4 sequence complete interrupt > + (display controller, safety stream 0) > + - description: | > + extdst1 shadow load interrupt > + (display controller, content stream 1) > + - description: | > + extdst1 frame complete interrupt > + (display controller, content stream 1) > + - description: | > + extdst1 sequence complete interrupt > + (display controller, content stream 1) > + - description: | > + extdst5 shadow load interrupt > + (display controller, safety stream 1) > + - description: | > + extdst5 frame complete interrupt > + (display controller, safety stream 1) > + - description: | > + extdst5 sequence complete interrupt > + (display controller, safety stream 1) > + - description: | > + disengcfg0 shadow load interrupt > + (display controller, display stream 0) > + - description: | > + disengcfg0 frame complete interrupt > + (display controller, display stream 0) > + - description: | > + disengcfg0 sequence complete interrupt > + (display controller, display stream 0) > + - description: | > + framegen0 programmable interrupt0 > + (display controller, display stream 0) > + - description: | > + framegen0 programmable interrupt1 > + (display controller, display stream 0) > + - description: | > + framegen0 programmable interrupt2 > + (display controller, display stream 0) > + - description: | > + framegen0 programmable interrupt3 > + (display controller, display stream 0) > + - description: | > + signature0 shadow load interrupt > + (display controller, display stream 0) > + - description: | > + signature0 measurement valid interrupt > + (display controller, display stream 0) > + - description: | > + signature0 error condition interrupt > + (display controller, display stream 0) > + - description: | > + disengcfg1 shadow load interrupt > + (display controller, display stream 1) > + - description: | > + disengcfg1 frame complete interrupt > + (display controller, display stream 1) > + - description: | > + disengcfg1 sequence complete interrupt > + (display controller, display stream 1) > + - description: | > + framegen1 programmable interrupt0 > + (display controller, display stream 1) > + - description: | > + framegen1 programmable interrupt1 > + (display controller, display stream 1) > + - description: | > + framegen1 programmable interrupt2 > + (display controller, display stream 1) > + - description: | > + framegen1 programmable interrupt3 > + (display controller, display stream 1) > + - description: | > + signature1 shadow load interrupt > + (display controller, display stream 1) > + - description: | > + signature1 measurement valid interrupt > + (display controller, display stream 1) > + - description: | > + signature1 error condition interrupt > + (display controller, display stream 1) > + - description: | > + command sequencer error condition interrupt(command sequencer) > + - description: | > + common control software interrupt0(common control) > + - description: | > + common control software interrupt1(common control) > + - description: | > + common control software interrupt2(common control) > + - description: | > + common control software interrupt3(common control) > + - description: | > + framegen0 synchronization status activated interrupt > + (display controller, safety stream 0) > + - description: | > + framegen0 synchronization status deactivated interrupt > + (display controller, safety stream 0) > + - description: | > + framegen0 synchronization status activated interrupt > + (display controller, content stream 0) > + - description: | > + framegen0 synchronization status deactivated interrupt > + (display controller, content stream 0) > + - description: | > + framegen1 synchronization status activated interrupt > + (display controller, safety stream 1) > + - description: | > + framegen1 synchronization status deactivated interrupt > + (display controller, safety stream 1) > + - description: | > + framegen1 synchronization status activated interrupt > + (display controller, content stream 1) > + - description: | > + framegen1 synchronization status deactivated interrupt > + (display controller, content stream 1) > + > + interrupt-names: > + items: > + - const: store9_shdload > + - const: store9_framecomplete > + - const: store9_seqcomplete > + - const: extdst0_shdload > + - const: extdst0_framecomplete > + - const: extdst0_seqcomplete > + - const: extdst4_shdload > + - const: extdst4_framecomplete > + - const: extdst4_seqcomplete > + - const: extdst1_shdload > + - const: extdst1_framecomplete > + - const: extdst1_seqcomplete > + - const: extdst5_shdload > + - const: extdst5_framecomplete > + - const: extdst5_seqcomplete > + - const: disengcfg_shdload0 > + - const: disengcfg_framecomplete0 > + - const: disengcfg_seqcomplete0 > + - const: framegen0_int0 > + - const: framegen0_int1 > + - const: framegen0_int2 > + - const: framegen0_int3 > + - const: sig0_shdload > + - const: sig0_valid > + - const: sig0_error > + - const: disengcfg_shdload1 > + - const: disengcfg_framecomplete1 > + - const: disengcfg_seqcomplete1 > + - const: framegen1_int0 > + - const: framegen1_int1 > + - const: framegen1_int2 > + - const: framegen1_int3 > + - const: sig1_shdload > + - const: sig1_valid > + - const: sig1_error > + - const: cmdseq_error > + - const: comctrl_sw0 > + - const: comctrl_sw1 > + - const: comctrl_sw2 > + - const: comctrl_sw3 > + - const: framegen0_primsync_on > + - const: framegen0_primsync_off > + - const: framegen0_secsync_on > + - const: framegen0_secsync_off > + - const: framegen1_primsync_on > + - const: framegen1_primsync_off > + - const: framegen1_secsync_on > + - const: framegen1_secsync_off > + > + clocks: > + maxItems: 8 > + > + clock-names: > + items: > + - const: axi > + - const: cfg > + - const: pll0 > + - const: pll1 > + - const: bypass0 > + - const: bypass1 > + - const: disp0 > + - const: disp1 > + > + power-domains: > + items: > + - description: DC power-domain > + - description: PLL0 power-domain > + - description: PLL1 power-domain > + > + power-domain-names: > + items: > + - const: dc > + - const: pll0 > + - const: pll1 > + > + fsl,dpr-channels: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: | > + List of phandle which points to DPR channels associated with > + this DPU instance. > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/properties/port > + description: The DPU output port node from display stream0. > + > + port@1: > + $ref: /schemas/graph.yaml#/properties/port > + description: The DPU output port node from display stream1. > + > + required: > + - port@0 > + - port@1 Generally speaking, and looking at the main KMS drivers patch, it really looks like it's multiple device glued as one, with the driver un-gluing them and creating devices and their resources based on what actual devices you have in there. It's especially obvious for the CRTCs, and to some extent the embedded interrupt controller you have in your driver. This is *very* far from the usual way of describing things in the device tree, and you would usually have a driver that doesn't take care of creating the devices, because they are properly described in the device tree. If you have a good reason to deviate from that design, then it should be explicitly discussed and explained. Maxime
On Tuesday, August 22, 2023 7:47 PM Maxime Ripard <mripard@kernel.org> wrote: > > Hi, Hi Maxime, Thanks for your review. > > On Tue, Aug 22, 2023 at 04:59:44PM +0800, Liu Ying wrote: > > This patch adds bindings for i.MX8qxp/qm Display Processing Unit. > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > v7->v14: > > * No change. > > > > v6->v7: > > * Add Rob's R-b tag back. > > > > v5->v6: > > * Use graph schema. So, drop Rob's R-b tag as review is needed. > > > > v4->v5: > > * No change. > > > > v3->v4: > > * Improve compatible property by using enum instead of oneOf+const. > (Rob) > > * Add Rob's R-b tag. > > > > v2->v3: > > * No change. > > > > v1->v2: > > * Fix yamllint warnings. > > * Require bypass0 and bypass1 clocks for both i.MX8qxp and i.MX8qm, as > the > > display controller subsystem spec does say that they exist. > > * Use new dt binding way to add clocks in the example. > > * Trivial tweaks for the example. > > > > .../bindings/display/imx/fsl,imx8qxp-dpu.yaml | 387 ++++++++++++++++++ > > 1 file changed, 387 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml > > > > diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > dpu.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > dpu.yaml > > new file mode 100644 > > index 000000000000..6b05c586cd9d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > dpu.yaml > > @@ -0,0 +1,387 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dpu.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Freescale i.MX8qm/qxp Display Processing Unit > > + > > +maintainers: > > + - Liu Ying <victor.liu@nxp.com> > > + > > +description: | > > + The Freescale i.MX8qm/qxp Display Processing Unit(DPU) is comprised of > two > > + main components that include a blit engine for 2D graphics accelerations > > + and a display controller for display output processing, as well as a > command > > + sequencer. > > + > > +properties: > > + compatible: > > + enum: > > + - fsl,imx8qxp-dpu > > + - fsl,imx8qm-dpu > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + items: > > + - description: | > > + store9 shadow load interrupt(blit engine) > > + - description: | > > + store9 frame complete interrupt(blit engine) > > + - description: | > > + store9 sequence complete interrupt(blit engine) > > + - description: | > > + extdst0 shadow load interrupt > > + (display controller, content stream 0) > > + - description: | > > + extdst0 frame complete interrupt > > + (display controller, content stream 0) > > + - description: | > > + extdst0 sequence complete interrupt > > + (display controller, content stream 0) > > + - description: | > > + extdst4 shadow load interrupt > > + (display controller, safety stream 0) > > + - description: | > > + extdst4 frame complete interrupt > > + (display controller, safety stream 0) > > + - description: | > > + extdst4 sequence complete interrupt > > + (display controller, safety stream 0) > > + - description: | > > + extdst1 shadow load interrupt > > + (display controller, content stream 1) > > + - description: | > > + extdst1 frame complete interrupt > > + (display controller, content stream 1) > > + - description: | > > + extdst1 sequence complete interrupt > > + (display controller, content stream 1) > > + - description: | > > + extdst5 shadow load interrupt > > + (display controller, safety stream 1) > > + - description: | > > + extdst5 frame complete interrupt > > + (display controller, safety stream 1) > > + - description: | > > + extdst5 sequence complete interrupt > > + (display controller, safety stream 1) > > + - description: | > > + disengcfg0 shadow load interrupt > > + (display controller, display stream 0) > > + - description: | > > + disengcfg0 frame complete interrupt > > + (display controller, display stream 0) > > + - description: | > > + disengcfg0 sequence complete interrupt > > + (display controller, display stream 0) > > + - description: | > > + framegen0 programmable interrupt0 > > + (display controller, display stream 0) > > + - description: | > > + framegen0 programmable interrupt1 > > + (display controller, display stream 0) > > + - description: | > > + framegen0 programmable interrupt2 > > + (display controller, display stream 0) > > + - description: | > > + framegen0 programmable interrupt3 > > + (display controller, display stream 0) > > + - description: | > > + signature0 shadow load interrupt > > + (display controller, display stream 0) > > + - description: | > > + signature0 measurement valid interrupt > > + (display controller, display stream 0) > > + - description: | > > + signature0 error condition interrupt > > + (display controller, display stream 0) > > + - description: | > > + disengcfg1 shadow load interrupt > > + (display controller, display stream 1) > > + - description: | > > + disengcfg1 frame complete interrupt > > + (display controller, display stream 1) > > + - description: | > > + disengcfg1 sequence complete interrupt > > + (display controller, display stream 1) > > + - description: | > > + framegen1 programmable interrupt0 > > + (display controller, display stream 1) > > + - description: | > > + framegen1 programmable interrupt1 > > + (display controller, display stream 1) > > + - description: | > > + framegen1 programmable interrupt2 > > + (display controller, display stream 1) > > + - description: | > > + framegen1 programmable interrupt3 > > + (display controller, display stream 1) > > + - description: | > > + signature1 shadow load interrupt > > + (display controller, display stream 1) > > + - description: | > > + signature1 measurement valid interrupt > > + (display controller, display stream 1) > > + - description: | > > + signature1 error condition interrupt > > + (display controller, display stream 1) > > + - description: | > > + command sequencer error condition interrupt(command sequencer) > > + - description: | > > + common control software interrupt0(common control) > > + - description: | > > + common control software interrupt1(common control) > > + - description: | > > + common control software interrupt2(common control) > > + - description: | > > + common control software interrupt3(common control) > > + - description: | > > + framegen0 synchronization status activated interrupt > > + (display controller, safety stream 0) > > + - description: | > > + framegen0 synchronization status deactivated interrupt > > + (display controller, safety stream 0) > > + - description: | > > + framegen0 synchronization status activated interrupt > > + (display controller, content stream 0) > > + - description: | > > + framegen0 synchronization status deactivated interrupt > > + (display controller, content stream 0) > > + - description: | > > + framegen1 synchronization status activated interrupt > > + (display controller, safety stream 1) > > + - description: | > > + framegen1 synchronization status deactivated interrupt > > + (display controller, safety stream 1) > > + - description: | > > + framegen1 synchronization status activated interrupt > > + (display controller, content stream 1) > > + - description: | > > + framegen1 synchronization status deactivated interrupt > > + (display controller, content stream 1) > > + > > + interrupt-names: > > + items: > > + - const: store9_shdload > > + - const: store9_framecomplete > > + - const: store9_seqcomplete > > + - const: extdst0_shdload > > + - const: extdst0_framecomplete > > + - const: extdst0_seqcomplete > > + - const: extdst4_shdload > > + - const: extdst4_framecomplete > > + - const: extdst4_seqcomplete > > + - const: extdst1_shdload > > + - const: extdst1_framecomplete > > + - const: extdst1_seqcomplete > > + - const: extdst5_shdload > > + - const: extdst5_framecomplete > > + - const: extdst5_seqcomplete > > + - const: disengcfg_shdload0 > > + - const: disengcfg_framecomplete0 > > + - const: disengcfg_seqcomplete0 > > + - const: framegen0_int0 > > + - const: framegen0_int1 > > + - const: framegen0_int2 > > + - const: framegen0_int3 > > + - const: sig0_shdload > > + - const: sig0_valid > > + - const: sig0_error > > + - const: disengcfg_shdload1 > > + - const: disengcfg_framecomplete1 > > + - const: disengcfg_seqcomplete1 > > + - const: framegen1_int0 > > + - const: framegen1_int1 > > + - const: framegen1_int2 > > + - const: framegen1_int3 > > + - const: sig1_shdload > > + - const: sig1_valid > > + - const: sig1_error > > + - const: cmdseq_error > > + - const: comctrl_sw0 > > + - const: comctrl_sw1 > > + - const: comctrl_sw2 > > + - const: comctrl_sw3 > > + - const: framegen0_primsync_on > > + - const: framegen0_primsync_off > > + - const: framegen0_secsync_on > > + - const: framegen0_secsync_off > > + - const: framegen1_primsync_on > > + - const: framegen1_primsync_off > > + - const: framegen1_secsync_on > > + - const: framegen1_secsync_off > > + > > + clocks: > > + maxItems: 8 > > + > > + clock-names: > > + items: > > + - const: axi > > + - const: cfg > > + - const: pll0 > > + - const: pll1 > > + - const: bypass0 > > + - const: bypass1 > > + - const: disp0 > > + - const: disp1 > > + > > + power-domains: > > + items: > > + - description: DC power-domain > > + - description: PLL0 power-domain > > + - description: PLL1 power-domain > > + > > + power-domain-names: > > + items: > > + - const: dc > > + - const: pll0 > > + - const: pll1 > > + > > + fsl,dpr-channels: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + description: | > > + List of phandle which points to DPR channels associated with > > + this DPU instance. > > + > > + ports: > > + $ref: /schemas/graph.yaml#/properties/ports > > + > > + properties: > > + port@0: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: The DPU output port node from display stream0. > > + > > + port@1: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: The DPU output port node from display stream1. > > + > > + required: > > + - port@0 > > + - port@1 > > Generally speaking, and looking at the main KMS drivers patch, it really > looks like it's multiple device glued as one, with the driver un-gluing > them and creating devices and their resources based on what actual > devices you have in there. > > It's especially obvious for the CRTCs, and to some extent the embedded > interrupt controller you have in your driver. > > This is *very* far from the usual way of describing things in the device > tree, and you would usually have a driver that doesn't take care of > creating the devices, because they are properly described in the device > tree. The DPU core driver(dpu-core.c) creates platform devices only for CRTCs, no other device is created. The CRTC devices, as components, are bound together with the DPU DRM master device. i.MX8qm SoC embeds two DPU IPs, while i.MX8qxp SoC embeds one. Each DPU supports two CRTCs. So, e.g., for i.MX8qm, there could be at most four CRTCs under the imx8-dpu umbrella. > > If you have a good reason to deviate from that design, then it should be > explicitly discussed and explained. The DPU is one single IP which cannot be split into separate devices. The "IPIdentifer" register in DPU register map kind of provides version information for the IP. This dt-binding just follows generic dt-binding rule to describe the DPU IP hardware, not the software implementation. DPU internal units do not constitute separate devices. Regards, Liu Ying > > Maxime
On Wed, Aug 23, 2023 at 02:45:53AM +0000, Ying Liu wrote: > On Tuesday, August 22, 2023 7:47 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > Hi, > > Hi Maxime, > > Thanks for your review. > > > > > On Tue, Aug 22, 2023 at 04:59:44PM +0800, Liu Ying wrote: > > > This patch adds bindings for i.MX8qxp/qm Display Processing Unit. > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > --- > > > v7->v14: > > > * No change. > > > > > > v6->v7: > > > * Add Rob's R-b tag back. > > > > > > v5->v6: > > > * Use graph schema. So, drop Rob's R-b tag as review is needed. > > > > > > v4->v5: > > > * No change. > > > > > > v3->v4: > > > * Improve compatible property by using enum instead of oneOf+const. > > (Rob) > > > * Add Rob's R-b tag. > > > > > > v2->v3: > > > * No change. > > > > > > v1->v2: > > > * Fix yamllint warnings. > > > * Require bypass0 and bypass1 clocks for both i.MX8qxp and i.MX8qm, as > > the > > > display controller subsystem spec does say that they exist. > > > * Use new dt binding way to add clocks in the example. > > > * Trivial tweaks for the example. > > > > > > .../bindings/display/imx/fsl,imx8qxp-dpu.yaml | 387 ++++++++++++++++++ > > > 1 file changed, 387 insertions(+) > > > create mode 100644 > > Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > > dpu.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > > dpu.yaml > > > new file mode 100644 > > > index 000000000000..6b05c586cd9d > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > > dpu.yaml > > > @@ -0,0 +1,387 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dpu.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Freescale i.MX8qm/qxp Display Processing Unit > > > + > > > +maintainers: > > > + - Liu Ying <victor.liu@nxp.com> > > > + > > > +description: | > > > + The Freescale i.MX8qm/qxp Display Processing Unit(DPU) is comprised of > > two > > > + main components that include a blit engine for 2D graphics accelerations > > > + and a display controller for display output processing, as well as a > > command > > > + sequencer. > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - fsl,imx8qxp-dpu > > > + - fsl,imx8qm-dpu > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + items: > > > + - description: | > > > + store9 shadow load interrupt(blit engine) > > > + - description: | > > > + store9 frame complete interrupt(blit engine) > > > + - description: | > > > + store9 sequence complete interrupt(blit engine) > > > + - description: | > > > + extdst0 shadow load interrupt > > > + (display controller, content stream 0) > > > + - description: | > > > + extdst0 frame complete interrupt > > > + (display controller, content stream 0) > > > + - description: | > > > + extdst0 sequence complete interrupt > > > + (display controller, content stream 0) > > > + - description: | > > > + extdst4 shadow load interrupt > > > + (display controller, safety stream 0) > > > + - description: | > > > + extdst4 frame complete interrupt > > > + (display controller, safety stream 0) > > > + - description: | > > > + extdst4 sequence complete interrupt > > > + (display controller, safety stream 0) > > > + - description: | > > > + extdst1 shadow load interrupt > > > + (display controller, content stream 1) > > > + - description: | > > > + extdst1 frame complete interrupt > > > + (display controller, content stream 1) > > > + - description: | > > > + extdst1 sequence complete interrupt > > > + (display controller, content stream 1) > > > + - description: | > > > + extdst5 shadow load interrupt > > > + (display controller, safety stream 1) > > > + - description: | > > > + extdst5 frame complete interrupt > > > + (display controller, safety stream 1) > > > + - description: | > > > + extdst5 sequence complete interrupt > > > + (display controller, safety stream 1) > > > + - description: | > > > + disengcfg0 shadow load interrupt > > > + (display controller, display stream 0) > > > + - description: | > > > + disengcfg0 frame complete interrupt > > > + (display controller, display stream 0) > > > + - description: | > > > + disengcfg0 sequence complete interrupt > > > + (display controller, display stream 0) > > > + - description: | > > > + framegen0 programmable interrupt0 > > > + (display controller, display stream 0) > > > + - description: | > > > + framegen0 programmable interrupt1 > > > + (display controller, display stream 0) > > > + - description: | > > > + framegen0 programmable interrupt2 > > > + (display controller, display stream 0) > > > + - description: | > > > + framegen0 programmable interrupt3 > > > + (display controller, display stream 0) > > > + - description: | > > > + signature0 shadow load interrupt > > > + (display controller, display stream 0) > > > + - description: | > > > + signature0 measurement valid interrupt > > > + (display controller, display stream 0) > > > + - description: | > > > + signature0 error condition interrupt > > > + (display controller, display stream 0) > > > + - description: | > > > + disengcfg1 shadow load interrupt > > > + (display controller, display stream 1) > > > + - description: | > > > + disengcfg1 frame complete interrupt > > > + (display controller, display stream 1) > > > + - description: | > > > + disengcfg1 sequence complete interrupt > > > + (display controller, display stream 1) > > > + - description: | > > > + framegen1 programmable interrupt0 > > > + (display controller, display stream 1) > > > + - description: | > > > + framegen1 programmable interrupt1 > > > + (display controller, display stream 1) > > > + - description: | > > > + framegen1 programmable interrupt2 > > > + (display controller, display stream 1) > > > + - description: | > > > + framegen1 programmable interrupt3 > > > + (display controller, display stream 1) > > > + - description: | > > > + signature1 shadow load interrupt > > > + (display controller, display stream 1) > > > + - description: | > > > + signature1 measurement valid interrupt > > > + (display controller, display stream 1) > > > + - description: | > > > + signature1 error condition interrupt > > > + (display controller, display stream 1) > > > + - description: | > > > + command sequencer error condition interrupt(command sequencer) > > > + - description: | > > > + common control software interrupt0(common control) > > > + - description: | > > > + common control software interrupt1(common control) > > > + - description: | > > > + common control software interrupt2(common control) > > > + - description: | > > > + common control software interrupt3(common control) > > > + - description: | > > > + framegen0 synchronization status activated interrupt > > > + (display controller, safety stream 0) > > > + - description: | > > > + framegen0 synchronization status deactivated interrupt > > > + (display controller, safety stream 0) > > > + - description: | > > > + framegen0 synchronization status activated interrupt > > > + (display controller, content stream 0) > > > + - description: | > > > + framegen0 synchronization status deactivated interrupt > > > + (display controller, content stream 0) > > > + - description: | > > > + framegen1 synchronization status activated interrupt > > > + (display controller, safety stream 1) > > > + - description: | > > > + framegen1 synchronization status deactivated interrupt > > > + (display controller, safety stream 1) > > > + - description: | > > > + framegen1 synchronization status activated interrupt > > > + (display controller, content stream 1) > > > + - description: | > > > + framegen1 synchronization status deactivated interrupt > > > + (display controller, content stream 1) > > > + > > > + interrupt-names: > > > + items: > > > + - const: store9_shdload > > > + - const: store9_framecomplete > > > + - const: store9_seqcomplete > > > + - const: extdst0_shdload > > > + - const: extdst0_framecomplete > > > + - const: extdst0_seqcomplete > > > + - const: extdst4_shdload > > > + - const: extdst4_framecomplete > > > + - const: extdst4_seqcomplete > > > + - const: extdst1_shdload > > > + - const: extdst1_framecomplete > > > + - const: extdst1_seqcomplete > > > + - const: extdst5_shdload > > > + - const: extdst5_framecomplete > > > + - const: extdst5_seqcomplete > > > + - const: disengcfg_shdload0 > > > + - const: disengcfg_framecomplete0 > > > + - const: disengcfg_seqcomplete0 > > > + - const: framegen0_int0 > > > + - const: framegen0_int1 > > > + - const: framegen0_int2 > > > + - const: framegen0_int3 > > > + - const: sig0_shdload > > > + - const: sig0_valid > > > + - const: sig0_error > > > + - const: disengcfg_shdload1 > > > + - const: disengcfg_framecomplete1 > > > + - const: disengcfg_seqcomplete1 > > > + - const: framegen1_int0 > > > + - const: framegen1_int1 > > > + - const: framegen1_int2 > > > + - const: framegen1_int3 > > > + - const: sig1_shdload > > > + - const: sig1_valid > > > + - const: sig1_error > > > + - const: cmdseq_error > > > + - const: comctrl_sw0 > > > + - const: comctrl_sw1 > > > + - const: comctrl_sw2 > > > + - const: comctrl_sw3 > > > + - const: framegen0_primsync_on > > > + - const: framegen0_primsync_off > > > + - const: framegen0_secsync_on > > > + - const: framegen0_secsync_off > > > + - const: framegen1_primsync_on > > > + - const: framegen1_primsync_off > > > + - const: framegen1_secsync_on > > > + - const: framegen1_secsync_off > > > + > > > + clocks: > > > + maxItems: 8 > > > + > > > + clock-names: > > > + items: > > > + - const: axi > > > + - const: cfg > > > + - const: pll0 > > > + - const: pll1 > > > + - const: bypass0 > > > + - const: bypass1 > > > + - const: disp0 > > > + - const: disp1 > > > + > > > + power-domains: > > > + items: > > > + - description: DC power-domain > > > + - description: PLL0 power-domain > > > + - description: PLL1 power-domain > > > + > > > + power-domain-names: > > > + items: > > > + - const: dc > > > + - const: pll0 > > > + - const: pll1 > > > + > > > + fsl,dpr-channels: > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > + description: | > > > + List of phandle which points to DPR channels associated with > > > + this DPU instance. > > > + > > > + ports: > > > + $ref: /schemas/graph.yaml#/properties/ports > > > + > > > + properties: > > > + port@0: > > > + $ref: /schemas/graph.yaml#/properties/port > > > + description: The DPU output port node from display stream0. > > > + > > > + port@1: > > > + $ref: /schemas/graph.yaml#/properties/port > > > + description: The DPU output port node from display stream1. > > > + > > > + required: > > > + - port@0 > > > + - port@1 > > > > Generally speaking, and looking at the main KMS drivers patch, it really > > looks like it's multiple device glued as one, with the driver un-gluing > > them and creating devices and their resources based on what actual > > devices you have in there. > > > > It's especially obvious for the CRTCs, and to some extent the embedded > > interrupt controller you have in your driver. > > > > This is *very* far from the usual way of describing things in the device > > tree, and you would usually have a driver that doesn't take care of > > creating the devices, because they are properly described in the device > > tree. > > The DPU core driver(dpu-core.c) creates platform devices only for CRTCs, > no other device is created. The CRTC devices, as components, are bound > together with the DPU DRM master device. i.MX8qm SoC embeds two > DPU IPs, while i.MX8qxp SoC embeds one. Each DPU supports two CRTCs. > So, e.g., for i.MX8qm, there could be at most four CRTCs under the imx8-dpu > umbrella. Yeah, and that's fine. It should all be separate devices in the device tree though. > > If you have a good reason to deviate from that design, then it should be > > explicitly discussed and explained. > > The DPU is one single IP which cannot be split into separate devices. Sure it can, your driver does so already by splitting it into several devices and accessing registers based on their stream_id. > The "IPIdentifer" register in DPU register map kind of provides version > information for the IP. That's fine too, just read the version register in the main KMS driver, every other component will then have access to it. > This dt-binding just follows generic dt-binding rule to describe the DPU IP > hardware, not the software implementation. DPU internal units do not > constitute separate devices. I mean, your driver does split them into separate devices so surely it constitutes separate devices. Maxime
On Wednesday, August 23, 2023 3:32 PM Maxime Ripard <mripard@kernel.org> wrote: > > On Wed, Aug 23, 2023 at 02:45:53AM +0000, Ying Liu wrote: > > On Tuesday, August 22, 2023 7:47 PM Maxime Ripard > <mripard@kernel.org> wrote: > > > > > > Hi, > > > > Hi Maxime, > > > > Thanks for your review. > > > > > > > > On Tue, Aug 22, 2023 at 04:59:44PM +0800, Liu Ying wrote: > > > > This patch adds bindings for i.MX8qxp/qm Display Processing Unit. > > > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > --- > > > > v7->v14: > > > > * No change. > > > > > > > > v6->v7: > > > > * Add Rob's R-b tag back. > > > > > > > > v5->v6: > > > > * Use graph schema. So, drop Rob's R-b tag as review is needed. > > > > > > > > v4->v5: > > > > * No change. > > > > > > > > v3->v4: > > > > * Improve compatible property by using enum instead of oneOf+const. > > > (Rob) > > > > * Add Rob's R-b tag. > > > > > > > > v2->v3: > > > > * No change. > > > > > > > > v1->v2: > > > > * Fix yamllint warnings. > > > > * Require bypass0 and bypass1 clocks for both i.MX8qxp and i.MX8qm, > as > > > the > > > > display controller subsystem spec does say that they exist. > > > > * Use new dt binding way to add clocks in the example. > > > > * Trivial tweaks for the example. > > > > > > > > .../bindings/display/imx/fsl,imx8qxp-dpu.yaml | 387 > ++++++++++++++++++ > > > > 1 file changed, 387 insertions(+) > > > > create mode 100644 > > > Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml > > > > > > > > diff --git > a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > > > dpu.yaml > b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > > > dpu.yaml > > > > new file mode 100644 > > > > index 000000000000..6b05c586cd9d > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp- > > > dpu.yaml > > > > @@ -0,0 +1,387 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp- > dpu.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Freescale i.MX8qm/qxp Display Processing Unit > > > > + > > > > +maintainers: > > > > + - Liu Ying <victor.liu@nxp.com> > > > > + > > > > +description: | > > > > + The Freescale i.MX8qm/qxp Display Processing Unit(DPU) is > comprised of > > > two > > > > + main components that include a blit engine for 2D graphics > accelerations > > > > + and a display controller for display output processing, as well as a > > > command > > > > + sequencer. > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - fsl,imx8qxp-dpu > > > > + - fsl,imx8qm-dpu > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + items: > > > > + - description: | > > > > + store9 shadow load interrupt(blit engine) > > > > + - description: | > > > > + store9 frame complete interrupt(blit engine) > > > > + - description: | > > > > + store9 sequence complete interrupt(blit engine) > > > > + - description: | > > > > + extdst0 shadow load interrupt > > > > + (display controller, content stream 0) > > > > + - description: | > > > > + extdst0 frame complete interrupt > > > > + (display controller, content stream 0) > > > > + - description: | > > > > + extdst0 sequence complete interrupt > > > > + (display controller, content stream 0) > > > > + - description: | > > > > + extdst4 shadow load interrupt > > > > + (display controller, safety stream 0) > > > > + - description: | > > > > + extdst4 frame complete interrupt > > > > + (display controller, safety stream 0) > > > > + - description: | > > > > + extdst4 sequence complete interrupt > > > > + (display controller, safety stream 0) > > > > + - description: | > > > > + extdst1 shadow load interrupt > > > > + (display controller, content stream 1) > > > > + - description: | > > > > + extdst1 frame complete interrupt > > > > + (display controller, content stream 1) > > > > + - description: | > > > > + extdst1 sequence complete interrupt > > > > + (display controller, content stream 1) > > > > + - description: | > > > > + extdst5 shadow load interrupt > > > > + (display controller, safety stream 1) > > > > + - description: | > > > > + extdst5 frame complete interrupt > > > > + (display controller, safety stream 1) > > > > + - description: | > > > > + extdst5 sequence complete interrupt > > > > + (display controller, safety stream 1) > > > > + - description: | > > > > + disengcfg0 shadow load interrupt > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + disengcfg0 frame complete interrupt > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + disengcfg0 sequence complete interrupt > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + framegen0 programmable interrupt0 > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + framegen0 programmable interrupt1 > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + framegen0 programmable interrupt2 > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + framegen0 programmable interrupt3 > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + signature0 shadow load interrupt > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + signature0 measurement valid interrupt > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + signature0 error condition interrupt > > > > + (display controller, display stream 0) > > > > + - description: | > > > > + disengcfg1 shadow load interrupt > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + disengcfg1 frame complete interrupt > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + disengcfg1 sequence complete interrupt > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + framegen1 programmable interrupt0 > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + framegen1 programmable interrupt1 > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + framegen1 programmable interrupt2 > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + framegen1 programmable interrupt3 > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + signature1 shadow load interrupt > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + signature1 measurement valid interrupt > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + signature1 error condition interrupt > > > > + (display controller, display stream 1) > > > > + - description: | > > > > + command sequencer error condition interrupt(command > sequencer) > > > > + - description: | > > > > + common control software interrupt0(common control) > > > > + - description: | > > > > + common control software interrupt1(common control) > > > > + - description: | > > > > + common control software interrupt2(common control) > > > > + - description: | > > > > + common control software interrupt3(common control) > > > > + - description: | > > > > + framegen0 synchronization status activated interrupt > > > > + (display controller, safety stream 0) > > > > + - description: | > > > > + framegen0 synchronization status deactivated interrupt > > > > + (display controller, safety stream 0) > > > > + - description: | > > > > + framegen0 synchronization status activated interrupt > > > > + (display controller, content stream 0) > > > > + - description: | > > > > + framegen0 synchronization status deactivated interrupt > > > > + (display controller, content stream 0) > > > > + - description: | > > > > + framegen1 synchronization status activated interrupt > > > > + (display controller, safety stream 1) > > > > + - description: | > > > > + framegen1 synchronization status deactivated interrupt > > > > + (display controller, safety stream 1) > > > > + - description: | > > > > + framegen1 synchronization status activated interrupt > > > > + (display controller, content stream 1) > > > > + - description: | > > > > + framegen1 synchronization status deactivated interrupt > > > > + (display controller, content stream 1) > > > > + > > > > + interrupt-names: > > > > + items: > > > > + - const: store9_shdload > > > > + - const: store9_framecomplete > > > > + - const: store9_seqcomplete > > > > + - const: extdst0_shdload > > > > + - const: extdst0_framecomplete > > > > + - const: extdst0_seqcomplete > > > > + - const: extdst4_shdload > > > > + - const: extdst4_framecomplete > > > > + - const: extdst4_seqcomplete > > > > + - const: extdst1_shdload > > > > + - const: extdst1_framecomplete > > > > + - const: extdst1_seqcomplete > > > > + - const: extdst5_shdload > > > > + - const: extdst5_framecomplete > > > > + - const: extdst5_seqcomplete > > > > + - const: disengcfg_shdload0 > > > > + - const: disengcfg_framecomplete0 > > > > + - const: disengcfg_seqcomplete0 > > > > + - const: framegen0_int0 > > > > + - const: framegen0_int1 > > > > + - const: framegen0_int2 > > > > + - const: framegen0_int3 > > > > + - const: sig0_shdload > > > > + - const: sig0_valid > > > > + - const: sig0_error > > > > + - const: disengcfg_shdload1 > > > > + - const: disengcfg_framecomplete1 > > > > + - const: disengcfg_seqcomplete1 > > > > + - const: framegen1_int0 > > > > + - const: framegen1_int1 > > > > + - const: framegen1_int2 > > > > + - const: framegen1_int3 > > > > + - const: sig1_shdload > > > > + - const: sig1_valid > > > > + - const: sig1_error > > > > + - const: cmdseq_error > > > > + - const: comctrl_sw0 > > > > + - const: comctrl_sw1 > > > > + - const: comctrl_sw2 > > > > + - const: comctrl_sw3 > > > > + - const: framegen0_primsync_on > > > > + - const: framegen0_primsync_off > > > > + - const: framegen0_secsync_on > > > > + - const: framegen0_secsync_off > > > > + - const: framegen1_primsync_on > > > > + - const: framegen1_primsync_off > > > > + - const: framegen1_secsync_on > > > > + - const: framegen1_secsync_off > > > > + > > > > + clocks: > > > > + maxItems: 8 > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: axi > > > > + - const: cfg > > > > + - const: pll0 > > > > + - const: pll1 > > > > + - const: bypass0 > > > > + - const: bypass1 > > > > + - const: disp0 > > > > + - const: disp1 > > > > + > > > > + power-domains: > > > > + items: > > > > + - description: DC power-domain > > > > + - description: PLL0 power-domain > > > > + - description: PLL1 power-domain > > > > + > > > > + power-domain-names: > > > > + items: > > > > + - const: dc > > > > + - const: pll0 > > > > + - const: pll1 > > > > + > > > > + fsl,dpr-channels: > > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > > + description: | > > > > + List of phandle which points to DPR channels associated with > > > > + this DPU instance. > > > > + > > > > + ports: > > > > + $ref: /schemas/graph.yaml#/properties/ports > > > > + > > > > + properties: > > > > + port@0: > > > > + $ref: /schemas/graph.yaml#/properties/port > > > > + description: The DPU output port node from display stream0. > > > > + > > > > + port@1: > > > > + $ref: /schemas/graph.yaml#/properties/port > > > > + description: The DPU output port node from display stream1. > > > > + > > > > + required: > > > > + - port@0 > > > > + - port@1 > > > > > > Generally speaking, and looking at the main KMS drivers patch, it really > > > looks like it's multiple device glued as one, with the driver un-gluing > > > them and creating devices and their resources based on what actual > > > devices you have in there. > > > > > > It's especially obvious for the CRTCs, and to some extent the embedded > > > interrupt controller you have in your driver. > > > > > > This is *very* far from the usual way of describing things in the device > > > tree, and you would usually have a driver that doesn't take care of > > > creating the devices, because they are properly described in the device > > > tree. > > > > The DPU core driver(dpu-core.c) creates platform devices only for CRTCs, > > no other device is created. The CRTC devices, as components, are bound > > together with the DPU DRM master device. i.MX8qm SoC embeds two > > DPU IPs, while i.MX8qxp SoC embeds one. Each DPU supports two CRTCs. > > So, e.g., for i.MX8qm, there could be at most four CRTCs under the imx8- > dpu > > umbrella. > > Yeah, and that's fine. It should all be separate devices in the device > tree though. There are 50+ individual DPU internal units and 20+ unit types. Do you really mean that each unit should be a separate device in device tree and each unit type should have it's own compatible string ? Almost all units have input/output ports to connect with each other. Some units have multiple input/output options. Should we use OF graph ports to tell the connections ? > > > > If you have a good reason to deviate from that design, then it should be > > > explicitly discussed and explained. > > > > The DPU is one single IP which cannot be split into separate devices. > > Sure it can, your driver does so already by splitting it into several > devices and accessing registers based on their stream_id. I would call them DPU internal units instead of devices. > > > The "IPIdentifer" register in DPU register map kind of provides version > > information for the IP. > > That's fine too, just read the version register in the main KMS driver, > every other component will then have access to it. I meant that "IPIdentifer" register hints that DPU is one single IP/hardware. And, we don't have to describe all DPU internal details in dt-binding. > > > This dt-binding just follows generic dt-binding rule to describe the DPU IP > > hardware, not the software implementation. DPU internal units do not > > constitute separate devices. > > I mean, your driver does split them into separate devices so surely it > constitutes separate devices. My driver treats them as DPU internal units, especially not Linux devices. Let's avoid Linuxisms when implementing this dt-binding and just be simple to describe necessary stuff exposing to DPU's embodying system/SoC, like reg, interrupts, clocks and power-domains. Regards, Liu Ying > > Maxime
On Wed, Aug 23, 2023 at 08:47:51AM +0000, Ying Liu wrote: > > > This dt-binding just follows generic dt-binding rule to describe the DPU IP > > > hardware, not the software implementation. DPU internal units do not > > > constitute separate devices. > > > > I mean, your driver does split them into separate devices so surely it > > constitutes separate devices. > > My driver treats them as DPU internal units, especially not Linux devices. > > Let's avoid Linuxisms when implementing this dt-binding and just be simple > to describe necessary stuff exposing to DPU's embodying system/SoC, like > reg, interrupts, clocks and power-domains. Let's focus the conversation here, because it's redundant with the rest. Your driver registers two additional devices, that have a different register space, different clocks, different interrupts, different power domains, etc. That has nothing to do with Linux, it's hardware properties. That alone is a very good indication to me that these devices should be modeled as such. And your driver agrees. Whether or not the other internal units need to be described as separate devices, I can't really tell, I don't have the datasheet. But at least the CRTC and the interrupt controller should be split away, or explained and detailed far better than "well it's just convenient". Maxime
On Thursday, August 24, 2023 5:48 PM, Maxime Ripard <mripard@kernel.org> wrote: > On Wed, Aug 23, 2023 at 08:47:51AM +0000, Ying Liu wrote: > > > > This dt-binding just follows generic dt-binding rule to describe the DPU > IP > > > > hardware, not the software implementation. DPU internal units do not > > > > constitute separate devices. > > > > > > I mean, your driver does split them into separate devices so surely it > > > constitutes separate devices. > > > > My driver treats them as DPU internal units, especially not Linux devices. > > > > Let's avoid Linuxisms when implementing this dt-binding and just be simple > > to describe necessary stuff exposing to DPU's embodying system/SoC, like > > reg, interrupts, clocks and power-domains. > > Let's focus the conversation here, because it's redundant with the rest. > > Your driver registers two additional devices, that have a different > register space, different clocks, different interrupts, different power > domains, etc. That has nothing to do with Linux, it's hardware > properties. > > That alone is a very good indication to me that these devices should be > modeled as such. And your driver agrees. > > Whether or not the other internal units need to be described as separate > devices, I can't really tell, I don't have the datasheet. i.MX8qxp and i.MX8qm SoC reference manuals can be found at(I think registration is needed first): https://www.nxp.com/webapp/Download?colCode=IMX8DQXPRM https://www.nxp.com/webapp/Download?colCode=IMX8QMRM Sorry for putting this in a short way, but the DPU is one IP, so one dt-binding. > > But at least the CRTC and the interrupt controller should be split away, > or explained and detailed far better than "well it's just convenient". CRTC is Linuxisms, which cannot be referenced to determine dt-binding. DPU as Display Controller is listed as a standalone module/IP in RM. This is how the IP is designed in the first place, not for any convenient purpose. Regards, Liu Ying > > Maxime
On Tue, Sep 05, 2023 at 03:44:23AM +0000, Ying Liu wrote: > On Thursday, August 24, 2023 5:48 PM, Maxime Ripard <mripard@kernel.org> wrote: > > On Wed, Aug 23, 2023 at 08:47:51AM +0000, Ying Liu wrote: > > > > > This dt-binding just follows generic dt-binding rule to describe the DPU > > IP > > > > > hardware, not the software implementation. DPU internal units do not > > > > > constitute separate devices. > > > > > > > > I mean, your driver does split them into separate devices so surely it > > > > constitutes separate devices. > > > > > > My driver treats them as DPU internal units, especially not Linux devices. > > > > > > Let's avoid Linuxisms when implementing this dt-binding and just be simple > > > to describe necessary stuff exposing to DPU's embodying system/SoC, like > > > reg, interrupts, clocks and power-domains. > > > > Let's focus the conversation here, because it's redundant with the rest. > > > > Your driver registers two additional devices, that have a different > > register space, different clocks, different interrupts, different power > > domains, etc. That has nothing to do with Linux, it's hardware > > properties. > > > > That alone is a very good indication to me that these devices should be > > modeled as such. And your driver agrees. > > > > Whether or not the other internal units need to be described as separate > > devices, I can't really tell, I don't have the datasheet. > > i.MX8qxp and i.MX8qm SoC reference manuals can be found at(I think > registration is needed first): > https://www.nxp.com/webapp/Download?colCode=IMX8DQXPRM > https://www.nxp.com/webapp/Download?colCode=IMX8QMRM I tried, but the registration is buggy. The email takes longer than the timeout to be sent. > Sorry for putting this in a short way, but the DPU is one IP, so one dt-binding. > > > > > But at least the CRTC and the interrupt controller should be split away, > > or explained and detailed far better than "well it's just convenient". > > CRTC is Linuxisms, which cannot be referenced to determine dt-binding. > > DPU as Display Controller is listed as a standalone module/IP in RM. > This is how the IP is designed in the first place, not for any convenient > purpose. Sure, but pushing that argument further, the entire SoC has been designed as a single entity. Every vendor out there designs its display pipeline in its entirety and not block by block. This doesn't mean that it isn't composed of several mostly discrete components. If it has a separate address space, clock and interrupt, it's a different device, no matter how it was designed or what the intent was. Maxime
diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml new file mode 100644 index 000000000000..6b05c586cd9d --- /dev/null +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dpu.yaml @@ -0,0 +1,387 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dpu.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Freescale i.MX8qm/qxp Display Processing Unit + +maintainers: + - Liu Ying <victor.liu@nxp.com> + +description: | + The Freescale i.MX8qm/qxp Display Processing Unit(DPU) is comprised of two + main components that include a blit engine for 2D graphics accelerations + and a display controller for display output processing, as well as a command + sequencer. + +properties: + compatible: + enum: + - fsl,imx8qxp-dpu + - fsl,imx8qm-dpu + + reg: + maxItems: 1 + + interrupts: + items: + - description: | + store9 shadow load interrupt(blit engine) + - description: | + store9 frame complete interrupt(blit engine) + - description: | + store9 sequence complete interrupt(blit engine) + - description: | + extdst0 shadow load interrupt + (display controller, content stream 0) + - description: | + extdst0 frame complete interrupt + (display controller, content stream 0) + - description: | + extdst0 sequence complete interrupt + (display controller, content stream 0) + - description: | + extdst4 shadow load interrupt + (display controller, safety stream 0) + - description: | + extdst4 frame complete interrupt + (display controller, safety stream 0) + - description: | + extdst4 sequence complete interrupt + (display controller, safety stream 0) + - description: | + extdst1 shadow load interrupt + (display controller, content stream 1) + - description: | + extdst1 frame complete interrupt + (display controller, content stream 1) + - description: | + extdst1 sequence complete interrupt + (display controller, content stream 1) + - description: | + extdst5 shadow load interrupt + (display controller, safety stream 1) + - description: | + extdst5 frame complete interrupt + (display controller, safety stream 1) + - description: | + extdst5 sequence complete interrupt + (display controller, safety stream 1) + - description: | + disengcfg0 shadow load interrupt + (display controller, display stream 0) + - description: | + disengcfg0 frame complete interrupt + (display controller, display stream 0) + - description: | + disengcfg0 sequence complete interrupt + (display controller, display stream 0) + - description: | + framegen0 programmable interrupt0 + (display controller, display stream 0) + - description: | + framegen0 programmable interrupt1 + (display controller, display stream 0) + - description: | + framegen0 programmable interrupt2 + (display controller, display stream 0) + - description: | + framegen0 programmable interrupt3 + (display controller, display stream 0) + - description: | + signature0 shadow load interrupt + (display controller, display stream 0) + - description: | + signature0 measurement valid interrupt + (display controller, display stream 0) + - description: | + signature0 error condition interrupt + (display controller, display stream 0) + - description: | + disengcfg1 shadow load interrupt + (display controller, display stream 1) + - description: | + disengcfg1 frame complete interrupt + (display controller, display stream 1) + - description: | + disengcfg1 sequence complete interrupt + (display controller, display stream 1) + - description: | + framegen1 programmable interrupt0 + (display controller, display stream 1) + - description: | + framegen1 programmable interrupt1 + (display controller, display stream 1) + - description: | + framegen1 programmable interrupt2 + (display controller, display stream 1) + - description: | + framegen1 programmable interrupt3 + (display controller, display stream 1) + - description: | + signature1 shadow load interrupt + (display controller, display stream 1) + - description: | + signature1 measurement valid interrupt + (display controller, display stream 1) + - description: | + signature1 error condition interrupt + (display controller, display stream 1) + - description: | + command sequencer error condition interrupt(command sequencer) + - description: | + common control software interrupt0(common control) + - description: | + common control software interrupt1(common control) + - description: | + common control software interrupt2(common control) + - description: | + common control software interrupt3(common control) + - description: | + framegen0 synchronization status activated interrupt + (display controller, safety stream 0) + - description: | + framegen0 synchronization status deactivated interrupt + (display controller, safety stream 0) + - description: | + framegen0 synchronization status activated interrupt + (display controller, content stream 0) + - description: | + framegen0 synchronization status deactivated interrupt + (display controller, content stream 0) + - description: | + framegen1 synchronization status activated interrupt + (display controller, safety stream 1) + - description: | + framegen1 synchronization status deactivated interrupt + (display controller, safety stream 1) + - description: | + framegen1 synchronization status activated interrupt + (display controller, content stream 1) + - description: | + framegen1 synchronization status deactivated interrupt + (display controller, content stream 1) + + interrupt-names: + items: + - const: store9_shdload + - const: store9_framecomplete + - const: store9_seqcomplete + - const: extdst0_shdload + - const: extdst0_framecomplete + - const: extdst0_seqcomplete + - const: extdst4_shdload + - const: extdst4_framecomplete + - const: extdst4_seqcomplete + - const: extdst1_shdload + - const: extdst1_framecomplete + - const: extdst1_seqcomplete + - const: extdst5_shdload + - const: extdst5_framecomplete + - const: extdst5_seqcomplete + - const: disengcfg_shdload0 + - const: disengcfg_framecomplete0 + - const: disengcfg_seqcomplete0 + - const: framegen0_int0 + - const: framegen0_int1 + - const: framegen0_int2 + - const: framegen0_int3 + - const: sig0_shdload + - const: sig0_valid + - const: sig0_error + - const: disengcfg_shdload1 + - const: disengcfg_framecomplete1 + - const: disengcfg_seqcomplete1 + - const: framegen1_int0 + - const: framegen1_int1 + - const: framegen1_int2 + - const: framegen1_int3 + - const: sig1_shdload + - const: sig1_valid + - const: sig1_error + - const: cmdseq_error + - const: comctrl_sw0 + - const: comctrl_sw1 + - const: comctrl_sw2 + - const: comctrl_sw3 + - const: framegen0_primsync_on + - const: framegen0_primsync_off + - const: framegen0_secsync_on + - const: framegen0_secsync_off + - const: framegen1_primsync_on + - const: framegen1_primsync_off + - const: framegen1_secsync_on + - const: framegen1_secsync_off + + clocks: + maxItems: 8 + + clock-names: + items: + - const: axi + - const: cfg + - const: pll0 + - const: pll1 + - const: bypass0 + - const: bypass1 + - const: disp0 + - const: disp1 + + power-domains: + items: + - description: DC power-domain + - description: PLL0 power-domain + - description: PLL1 power-domain + + power-domain-names: + items: + - const: dc + - const: pll0 + - const: pll1 + + fsl,dpr-channels: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: | + List of phandle which points to DPR channels associated with + this DPU instance. + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + description: The DPU output port node from display stream0. + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: The DPU output port node from display stream1. + + required: + - port@0 + - port@1 + +required: + - compatible + - reg + - interrupts + - interrupt-names + - clocks + - clock-names + - power-domains + - power-domain-names + - fsl,dpr-channels + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/imx8-lpcg.h> + #include <dt-bindings/firmware/imx/rsrc.h> + dpu@56180000 { + compatible = "fsl,imx8qxp-dpu"; + reg = <0x56180000 0x40000>; + interrupt-parent = <&dc0_irqsteer>; + interrupts = <448>, <449>, <450>, <64>, + <65>, <66>, <67>, <68>, + <69>, <70>, <193>, <194>, + <195>, <196>, <197>, <72>, + <73>, <74>, <75>, <76>, + <77>, <78>, <79>, <80>, + <81>, <199>, <200>, <201>, + <202>, <203>, <204>, <205>, + <206>, <207>, <208>, <0>, + <1>, <2>, <3>, <4>, + <82>, <83>, <84>, <85>, + <209>, <210>, <211>, <212>; + interrupt-names = "store9_shdload", + "store9_framecomplete", + "store9_seqcomplete", + "extdst0_shdload", + "extdst0_framecomplete", + "extdst0_seqcomplete", + "extdst4_shdload", + "extdst4_framecomplete", + "extdst4_seqcomplete", + "extdst1_shdload", + "extdst1_framecomplete", + "extdst1_seqcomplete", + "extdst5_shdload", + "extdst5_framecomplete", + "extdst5_seqcomplete", + "disengcfg_shdload0", + "disengcfg_framecomplete0", + "disengcfg_seqcomplete0", + "framegen0_int0", + "framegen0_int1", + "framegen0_int2", + "framegen0_int3", + "sig0_shdload", + "sig0_valid", + "sig0_error", + "disengcfg_shdload1", + "disengcfg_framecomplete1", + "disengcfg_seqcomplete1", + "framegen1_int0", + "framegen1_int1", + "framegen1_int2", + "framegen1_int3", + "sig1_shdload", + "sig1_valid", + "sig1_error", + "cmdseq_error", + "comctrl_sw0", + "comctrl_sw1", + "comctrl_sw2", + "comctrl_sw3", + "framegen0_primsync_on", + "framegen0_primsync_off", + "framegen0_secsync_on", + "framegen0_secsync_off", + "framegen1_primsync_on", + "framegen1_primsync_off", + "framegen1_secsync_on", + "framegen1_secsync_off"; + clocks = <&dc0_dpu_lpcg IMX_LPCG_CLK_5>, + <&dc0_dpu_lpcg IMX_LPCG_CLK_4>, + <&clk IMX_SC_R_DC_0_PLL_0 IMX_SC_PM_CLK_PLL>, + <&clk IMX_SC_R_DC_0_PLL_1 IMX_SC_PM_CLK_PLL>, + <&clk IMX_SC_R_DC_0_VIDEO0 IMX_SC_PM_CLK_BYPASS>, + <&clk IMX_SC_R_DC_0_VIDEO1 IMX_SC_PM_CLK_BYPASS>, + <&dc0_disp_lpcg IMX_LPCG_CLK_0>, + <&dc0_disp_lpcg IMX_LPCG_CLK_1>; + clock-names = "axi", "cfg", + "pll0", "pll1", "bypass0", "bypass1", + "disp0", "disp1"; + power-domains = <&pd IMX_SC_R_DC_0>, + <&pd IMX_SC_R_DC_0_PLL_0>, + <&pd IMX_SC_R_DC_0_PLL_1>; + power-domain-names = "dc", "pll0", "pll1"; + fsl,dpr-channels = <&dc0_dpr1_channel1>, + <&dc0_dpr1_channel2>, + <&dc0_dpr1_channel3>, + <&dc0_dpr2_channel1>, + <&dc0_dpr2_channel2>, + <&dc0_dpr2_channel3>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + dpu0_disp0_pixel_combiner0_ch0: endpoint { + remote-endpoint = <&pixel_combiner0_ch0_dpu0_disp0>; + }; + }; + + port@1 { + reg = <1>; + dpu0_disp1_pixel_combiner0_ch1: endpoint { + remote-endpoint = <&pixel_combiner0_ch1_dpu0_disp1>; + }; + }; + }; + };