RE: [PATCH v14 RESEND 1/6] dt-bindings: display: imx: Add i.MX8qxp/qm DPU binding

[Ying Liu]

On Wednesday, August 23, 2023 3:32 PM Maxime Ripard <mripard@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > > ---
> > > > 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@xxxxxxx>
> > > > +
> > > > +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