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

[Maxime Ripard]

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@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.

If you have a good reason to deviate from that design, then it should be
explicitly discussed and explained.

Maxime

Attachment: signature.asc
Description: PGP signature