[PATCH v14 RESEND 1/6] dt-bindings: display: imx: Add i.MX8qxp/qm DPU binding
Ying Liu
victor.liu at nxp.com
Wed Aug 23 08:47:51 UTC 2023
On Wednesday, August 23, 2023 3:32 PM Maxime Ripard <mripard at 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 at 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 at kernel.org>
> > > > Signed-off-by: Liu Ying <victor.liu at 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 at 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 at 0:
> > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > + description: The DPU output port node from display stream0.
> > > > +
> > > > + port at 1:
> > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > + description: The DPU output port node from display stream1.
> > > > +
> > > > + required:
> > > > + - port at 0
> > > > + - port at 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
More information about the dri-devel
mailing list