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
