diff mbox series

[02/10] dt-bindings: display: imx: Add i.MX8qxp Display Controller display engine

Message ID 20240705090932.1880496-3-victor.liu@nxp.com (mailing list archive)
State Superseded
Headers show
Series Add Freescale i.MX8qxp Display Controller support | expand

Commit Message

Liu Ying July 5, 2024, 9:09 a.m. UTC
i.MX8qxp Display Controller display engine consists of all processing units
that operate in a display clock domain.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 .../imx/fsl,imx8qxp-dc-display-engine.yaml    | 166 ++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml

Comments

Krzysztof Kozlowski July 7, 2024, 1:59 p.m. UTC | #1
On 05/07/2024 11:09, Liu Ying wrote:
> i.MX8qxp Display Controller display engine consists of all processing units
> that operate in a display clock domain.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  .../imx/fsl,imx8qxp-dc-display-engine.yaml    | 166 ++++++++++++++++++
>  1 file changed, 166 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
> new file mode 100644
> index 000000000000..dc9579897b76
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
> @@ -0,0 +1,166 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dc-display-engine.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX8qxp Display Controller Display Engine
> +
> +description:
> +  All Processing Units that operate in a display clock domain. Pixel pipeline
> +  is driven by a video timing and cannot be stalled. Implements all display
> +  specific processing.
> +
> +maintainers:
> +  - Liu Ying <victor.liu@nxp.com>
> +
> +properties:
> +  compatible:
> +    const: fsl,imx8qxp-dc-display-engine
> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: top
> +      - const: cfg
> +
> +  resets:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 3
> +
> +  interrupt-names:
> +    items:
> +      - const: shdload
> +      - const: framecomplete
> +      - const: seqcomplete
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges: true
> +
> +  fsl,dc-de-id:
> +    description: Display Engine instance number
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

No, drop. For the same reason as earlier patch.

> +
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +    description: video output

Eh, mixing children with and without addresses is considered poor design.

> +
> +patternProperties:
> +  "^dither@[0-9a-f]+$":
> +    type: object
> +    additionalProperties: true
> +
> +    properties:
> +      compatible:
> +        const: fsl,imx8qxp-dc-dither
> +
> +  "^framegen@[0-9a-f]+$":
> +    type: object
> +    additionalProperties: true
> +
> +    properties:
> +      compatible:
> +        const: fsl,imx8qxp-dc-framegen
> +
> +  "^gammacor@[0-9a-f]+$":

This looks like you are organizing bindings per your driver architecture.

Best regards,
Krzysztof
Liu Ying July 8, 2024, 6:40 a.m. UTC | #2
On 07/07/2024, Krzysztof Kozlowski wrote:
> On 05/07/2024 11:09, Liu Ying wrote:
>> i.MX8qxp Display Controller display engine consists of all processing units
>> that operate in a display clock domain.
>>
>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>> ---
>>  .../imx/fsl,imx8qxp-dc-display-engine.yaml    | 166 ++++++++++++++++++
>>  1 file changed, 166 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
>> new file mode 100644
>> index 000000000000..dc9579897b76
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
>> @@ -0,0 +1,166 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dc-display-engine.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Freescale i.MX8qxp Display Controller Display Engine
>> +
>> +description:
>> +  All Processing Units that operate in a display clock domain. Pixel pipeline
>> +  is driven by a video timing and cannot be stalled. Implements all display
>> +  specific processing.
>> +
>> +maintainers:
>> +  - Liu Ying <victor.liu@nxp.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: fsl,imx8qxp-dc-display-engine
>> +
>> +  reg:
>> +    maxItems: 2
>> +
>> +  reg-names:
>> +    items:
>> +      - const: top
>> +      - const: cfg
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 3
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: shdload
>> +      - const: framecomplete
>> +      - const: seqcomplete
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 1
>> +
>> +  ranges: true
>> +
>> +  fsl,dc-de-id:
>> +    description: Display Engine instance number
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1]
> 
> No, drop. For the same reason as earlier patch.

Call of_alias_get_id() from the driver instead?

> 
>> +
>> +  port:
>> +    $ref: /schemas/graph.yaml#/properties/port
>> +    description: video output
> 
> Eh, mixing children with and without addresses is considered poor design.

Will move this to the tcon child node as it is the
last processing unit in the display engine.

> 
>> +
>> +patternProperties:
>> +  "^dither@[0-9a-f]+$":
>> +    type: object
>> +    additionalProperties: true
>> +
>> +    properties:
>> +      compatible:
>> +        const: fsl,imx8qxp-dc-dither
>> +
>> +  "^framegen@[0-9a-f]+$":
>> +    type: object
>> +    additionalProperties: true
>> +
>> +    properties:
>> +      compatible:
>> +        const: fsl,imx8qxp-dc-framegen
>> +
>> +  "^gammacor@[0-9a-f]+$":
> 
> This looks like you are organizing bindings per your driver architecture.

As I mentioned in cover letter, this series addresses Maxime's
comment for the previous series - split the display controller
into multiple internal devices.  Maxime insisted on doing this.

> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski July 8, 2024, 2:04 p.m. UTC | #3
On 08/07/2024 08:40, Liu Ying wrote:
>>> +
>>> +  "^framegen@[0-9a-f]+$":
>>> +    type: object
>>> +    additionalProperties: true
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        const: fsl,imx8qxp-dc-framegen
>>> +
>>> +  "^gammacor@[0-9a-f]+$":
>>
>> This looks like you are organizing bindings per your driver architecture.
> 
> As I mentioned in cover letter, this series addresses Maxime's
> comment for the previous series - split the display controller
> into multiple internal devices.  Maxime insisted on doing this.

But these are not separate devices. Look:
1. parent DC:
reg = <0x56180000 0x40000>;

2. child interrupt controller:
reg = <0x56180040 0x60>;

That address is within parent.

3. Then we go to things like:
reg = <0x5618b400 0x14>, <0x5618b800 0x1c00>;

Still within parent's range and just few words in address range. That's
a clear indication that you choose few registers and call it a "device".

Best regards,
Krzysztof
Maxime Ripard July 8, 2024, 2:52 p.m. UTC | #4
On Mon, Jul 08, 2024 at 04:04:21PM GMT, Krzysztof Kozlowski wrote:
> On 08/07/2024 08:40, Liu Ying wrote:
> >>> +
> >>> +  "^framegen@[0-9a-f]+$":
> >>> +    type: object
> >>> +    additionalProperties: true
> >>> +
> >>> +    properties:
> >>> +      compatible:
> >>> +        const: fsl,imx8qxp-dc-framegen
> >>> +
> >>> +  "^gammacor@[0-9a-f]+$":
> >>
> >> This looks like you are organizing bindings per your driver architecture.
> > 
> > As I mentioned in cover letter, this series addresses Maxime's
> > comment for the previous series - split the display controller
> > into multiple internal devices.  Maxime insisted on doing this.
> 
> But these are not separate devices. Look:
> 1. parent DC:
> reg = <0x56180000 0x40000>;
> 
> 2. child interrupt controller:
> reg = <0x56180040 0x60>;
> 
> That address is within parent.
> 
> 3. Then we go to things like:
> reg = <0x5618b400 0x14>, <0x5618b800 0x1c00>;
> 
> Still within parent's range and just few words in address range. That's
> a clear indication that you choose few registers and call it a "device".

That's never really been a metric though?

If not, one could just create a "soc" device node covering the entire
register map, and since it would overlap despite clearly defined
features, you would claim it's a single device?

Maxime
Krzysztof Kozlowski July 9, 2024, 6:50 a.m. UTC | #5
On 08/07/2024 16:52, Maxime Ripard wrote:
> On Mon, Jul 08, 2024 at 04:04:21PM GMT, Krzysztof Kozlowski wrote:
>> On 08/07/2024 08:40, Liu Ying wrote:
>>>>> +
>>>>> +  "^framegen@[0-9a-f]+$":
>>>>> +    type: object
>>>>> +    additionalProperties: true
>>>>> +
>>>>> +    properties:
>>>>> +      compatible:
>>>>> +        const: fsl,imx8qxp-dc-framegen
>>>>> +
>>>>> +  "^gammacor@[0-9a-f]+$":
>>>>
>>>> This looks like you are organizing bindings per your driver architecture.
>>>
>>> As I mentioned in cover letter, this series addresses Maxime's
>>> comment for the previous series - split the display controller
>>> into multiple internal devices.  Maxime insisted on doing this.
>>
>> But these are not separate devices. Look:
>> 1. parent DC:
>> reg = <0x56180000 0x40000>;
>>
>> 2. child interrupt controller:
>> reg = <0x56180040 0x60>;
>>
>> That address is within parent.
>>
>> 3. Then we go to things like:
>> reg = <0x5618b400 0x14>, <0x5618b800 0x1c00>;
>>
>> Still within parent's range and just few words in address range. That's
>> a clear indication that you choose few registers and call it a "device".
> 
> That's never really been a metric though?
> 
> If not, one could just create a "soc" device node covering the entire
> register map, and since it would overlap despite clearly defined
> features, you would claim it's a single device?

Since I do not create such one-address-soc devices, I claim I have
separate devices in the SoC. Here is not the case: there is a device
covering entire address space.

Soc is a good example, because components/blocks of the SoC are being
re-used among different SoCs. Is the case here?

BTW, it could be that some of the sub-devices here are worth to be
devices, I agree.

Best regards,
Krzysztof
Maxime Ripard July 18, 2024, 3:53 p.m. UTC | #6
On Tue, Jul 09, 2024 at 08:50:35AM GMT, Krzysztof Kozlowski wrote:
> On 08/07/2024 16:52, Maxime Ripard wrote:
> > On Mon, Jul 08, 2024 at 04:04:21PM GMT, Krzysztof Kozlowski wrote:
> >> On 08/07/2024 08:40, Liu Ying wrote:
> >>>>> +
> >>>>> +  "^framegen@[0-9a-f]+$":
> >>>>> +    type: object
> >>>>> +    additionalProperties: true
> >>>>> +
> >>>>> +    properties:
> >>>>> +      compatible:
> >>>>> +        const: fsl,imx8qxp-dc-framegen
> >>>>> +
> >>>>> +  "^gammacor@[0-9a-f]+$":
> >>>>
> >>>> This looks like you are organizing bindings per your driver architecture.
> >>>
> >>> As I mentioned in cover letter, this series addresses Maxime's
> >>> comment for the previous series - split the display controller
> >>> into multiple internal devices.  Maxime insisted on doing this.
> >>
> >> But these are not separate devices. Look:
> >> 1. parent DC:
> >> reg = <0x56180000 0x40000>;
> >>
> >> 2. child interrupt controller:
> >> reg = <0x56180040 0x60>;
> >>
> >> That address is within parent.
> >>
> >> 3. Then we go to things like:
> >> reg = <0x5618b400 0x14>, <0x5618b800 0x1c00>;
> >>
> >> Still within parent's range and just few words in address range. That's
> >> a clear indication that you choose few registers and call it a "device".
> > 
> > That's never really been a metric though?
> > 
> > If not, one could just create a "soc" device node covering the entire
> > register map, and since it would overlap despite clearly defined
> > features, you would claim it's a single device?
> 
> Since I do not create such one-address-soc devices, I claim I have
> separate devices in the SoC. Here is not the case: there is a device
> covering entire address space.
> 
> Soc is a good example, because components/blocks of the SoC are being
> re-used among different SoCs. Is the case here?
> 
> BTW, it could be that some of the sub-devices here are worth to be
> devices, I agree.

This was the binding of the previous version:
https://lore.kernel.org/dri-devel/20230822085949.816844-2-victor.liu@nxp.com/

To me, the duplication of interrupts, clocks and power domains with
different indices kind of proves that it's all separate devices

Maxime
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
new file mode 100644
index 000000000000..dc9579897b76
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml
@@ -0,0 +1,166 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dc-display-engine.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX8qxp Display Controller Display Engine
+
+description:
+  All Processing Units that operate in a display clock domain. Pixel pipeline
+  is driven by a video timing and cannot be stalled. Implements all display
+  specific processing.
+
+maintainers:
+  - Liu Ying <victor.liu@nxp.com>
+
+properties:
+  compatible:
+    const: fsl,imx8qxp-dc-display-engine
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: top
+      - const: cfg
+
+  resets:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 3
+
+  interrupt-names:
+    items:
+      - const: shdload
+      - const: framecomplete
+      - const: seqcomplete
+
+  power-domains:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  ranges: true
+
+  fsl,dc-de-id:
+    description: Display Engine instance number
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description: video output
+
+patternProperties:
+  "^dither@[0-9a-f]+$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: fsl,imx8qxp-dc-dither
+
+  "^framegen@[0-9a-f]+$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: fsl,imx8qxp-dc-framegen
+
+  "^gammacor@[0-9a-f]+$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: fsl,imx8qxp-dc-gammacor
+
+  "^matrix@[0-9a-f]+$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: fsl,imx8qxp-dc-matrix
+
+  "^signature@[0-9a-f]+$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: fsl,imx8qxp-dc-signature
+
+  "^tcon@[0-9a-f]+$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: fsl,imx8qxp-dc-tcon
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - power-domains
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+  - fsl,dc-de-id
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8-lpcg.h>
+    #include <dt-bindings/firmware/imx/rsrc.h>
+
+    display-engine@5618b400 {
+        compatible = "fsl,imx8qxp-dc-display-engine";
+        reg = <0x5618b400 0x14>, <0x5618b800 0x1c00>;
+        reg-names = "top", "cfg";
+        interrupt-parent = <&dc0_intc>;
+        interrupts = <15>, <16>, <17>;
+        interrupt-names = "shdload", "framecomplete", "seqcomplete";
+        power-domains = <&pd IMX_SC_R_DC_0_PLL_0>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+        fsl,dc-de-id = <0>;
+
+        framegen@5618b800 {
+            compatible = "fsl,imx8qxp-dc-framegen";
+            reg = <0x5618b800 0x400>;
+            clocks = <&dc0_disp_lpcg IMX_LPCG_CLK_0>;
+            interrupt-parent = <&dc0_intc>;
+            interrupts = <18>, <19>, <20>, <21>, <41>, <42>, <43>, <44>;
+            interrupt-names = "int0", "int1", "int2", "int3",
+                              "primsync_on", "primsync_off",
+                              "secsync_on", "secsync_off";
+            fsl,dc-fg-id = <0>;
+        };
+
+        tcon@5618c800 {
+            compatible = "fsl,imx8qxp-dc-tcon";
+            reg = <0x5618c800 0x800>;
+            fsl,dc-tc-id = <0>;
+        };
+
+        port {
+            dc0_disp0_dc0_pixel_combiner_ch0: endpoint {
+                remote-endpoint = <&dc0_pixel_combiner_ch0_dc0_disp0>;
+            };
+        };
+    };