| Message ID | 20240705090932.1880496-3-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Freescale i.MX8qxp Display Controller support | expand |
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
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 >
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
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
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
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 --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>; + }; + }; + };
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