Date:   Tue, 14 Sep 2021 21:23:01 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Rob Herring <robh@...nel.org>
Cc:     dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Lee Jones <lee.jones@...aro.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v8 1/4] dt-bindings: display: Document the Xylon LogiCVC
 display controller

Hi Rob,

I just found out as I'm about to send a new revision that I had not yet
responded to your concerns here.

On Tue 12 Jan 21, 09:17, Rob Herring wrote:
> On Wed, Dec 23, 2020 at 10:29:44PM +0100, Paul Kocialkowski wrote:
> > The Xylon LogiCVC is a display controller implemented as programmable
> > logic in Xilinx FPGAs.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> > Acked-by: Rob Herring <robh@...nel.org>
> > ---
> >  .../display/xylon,logicvc-display.yaml        | 313 ++++++++++++++++++
> >  1 file changed, 313 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc-display.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc-display.yaml b/Documentation/devicetree/bindings/display/xylon,logicvc-display.yaml
> > new file mode 100644
> > index 000000000000..aca78334ad2c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc-display.yaml
> > @@ -0,0 +1,313 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2019 Bootlin
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/display/xylon,logicvc-display.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Xylon LogiCVC display controller
> > +
> > +maintainers:
> > +  - Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> > +
> > +description: |
> > +  The Xylon LogiCVC is a display controller that supports multiple layers.
> > +  It is usually implemented as programmable logic and was optimized for use
> > +  with Xilinx Zynq-7000 SoCs and Xilinx FPGAs.
> > +
> > +  Because the controller is intended for use in a FPGA, most of the
> > +  configuration of the controller takes place at logic configuration bitstream
> > +  synthesis time. As a result, many of the device-tree bindings are meant to
> > +  reflect the synthesis configuration and must not be configured differently.
> > +  Matching synthesis parameters are provided when applicable.
> > +
> > +  Layers are declared in the "layers" sub-node and have dedicated configuration.
> > +  In version 3 of the controller, each layer has fixed memory offset and address
> > +  starting from the video memory base address for its framebuffer. In version 4,
> > +  framebuffers are configured with a direct memory address instead.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - xylon,logicvc-3.02.a-display
> > +      - xylon,logicvc-4.01.a-display
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 4
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 4
> > +    items:
> > +      # vclk is required and must be provided as first item.
> > +      - const: vclk
> > +      # Other clocks are optional and can be provided in any order.
> > +      - enum:
> > +          - vclk2
> > +          - lvdsclk
> > +          - lvdsclkn
> > +      - enum:
> > +          - vclk2
> > +          - lvdsclk
> > +          - lvdsclkn
> > +      - enum:
> > +          - vclk2
> > +          - lvdsclk
> > +          - lvdsclkn
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +
> > +  xylon,display-interface:
> > +    enum:
> > +      # Parallel RGB interface (C_DISPLAY_INTERFACE == 0)
> > +      - parallel-rgb
> > +      # ITU-T BR656 interface (C_DISPLAY_INTERFACE == 1)
> > +      - bt656
> > +      # 4-bit LVDS interface (C_DISPLAY_INTERFACE == 2)
> > +      - lvds-4bits
> > +      # 3-bit LVDS interface (C_DISPLAY_INTERFACE == 4)
> > +      - lvds-3bits
> > +      # DVI interface (C_DISPLAY_INTERFACE == 5)
> > +      - dvi
> > +    description: Display output interface (C_DISPLAY_INTERFACE).
> 
> As I mentioned before, we have standard properties for these or you know 
> the setting based on the panel/bridge attached. 

The point here is to indicate how the bitstream was configured and not
to indicate what some run-time configuration should be. The bottomline
is to let the driver have knowledge of the bitstream configuration,
like it's done for the other properties.

This could typically be used by the driver to check if a connected panel/bridge
is compatible or not with the bitstream configuration.

As a result, it doesn't reflect the general "bus configuration" for which
we may have a generic property, but a bitstream configuration property that
is specific to our hardware. Hence why I think it makes sense to have it
described that way.

> > +
> > +  xylon,display-colorspace:
> > +    enum:
> > +      # RGB colorspace (C_DISPLAY_COLOR_SPACE == 0)
> > +      - rgb
> > +      # YUV 4:2:2 colorspace (C_DISPLAY_COLOR_SPACE == 1)
> > +      - yuv422
> > +      # YUV 4:4:4 colorspace (C_DISPLAY_COLOR_SPACE == 2)
> > +      - yuv444
> > +    description: Display output colorspace (C_DISPLAY_COLOR_SPACE).
> > +
> > +  xylon,display-depth:
> > +    $ref: "/schemas/types.yaml#/definitions/uint32"
> > +    description: Display output depth (C_PIXEL_DATA_WIDTH).
> > +
> > +  xylon,row-stride:
> > +    $ref: "/schemas/types.yaml#/definitions/uint32"
> > +    description: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > +
> > +  xylon,syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: |
> > +      Syscon phandle representing the top-level logicvc instance, useful when
> > +      the parent node is not the top-level logicvc instance.
> 
> Why do you need to support both ways? Drop this and require it to be the 
> parent node.

Fair enough, I guess I had seen similar things around but there's no real
use-case as far as I know.

> > +
> > +  xylon,dithering:
> > +    $ref: "/schemas/types.yaml#/definitions/flag"
> > +    description: Dithering module is enabled (C_XCOLOR)
> > +
> > +  xylon,background-layer:
> > +    $ref: "/schemas/types.yaml#/definitions/flag"
> > +    description: |
> > +      The last layer is used to display a black background (C_USE_BACKGROUND).
> > +      The layer must still be registered.
> > +
> > +  xylon,layers-configurable:
> > +    $ref: "/schemas/types.yaml#/definitions/flag"
> > +    description: |
> > +      Configuration of layers' size, position and offset is enabled
> > +      (C_USE_SIZE_POSITION).
> > +
> > +  layers:
> > +    type: object
> > +
> > +    properties:
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^layer@[0-9]+$":
> > +        type: object
> > +
> > +        properties:
> > +          reg:
> > +            maxItems: 1
> > +
> > +          xylon,layer-depth:
> > +            $ref: "/schemas/types.yaml#/definitions/uint32"
> > +            description: Layer depth (C_LAYER_X_DATA_WIDTH).
> > +
> > +          xylon,layer-colorspace:
> > +            enum:
> > +              # RGB colorspace (C_LAYER_X_TYPE == 0)
> > +              - rgb
> > +              # YUV packed colorspace (C_LAYER_X_TYPE == 0)
> > +              - yuv
> > +            description: Layer colorspace (C_LAYER_X_TYPE).
> > +
> > +          xylon,layer-alpha-mode:
> > +            enum:
> > +              # Alpha is configured layer-wide (C_LAYER_X_ALPHA_MODE == 0)
> > +              - layer
> > +              # Alpha is configured per-pixel (C_LAYER_X_ALPHA_MODE == 1)
> > +              - pixel
> > +            description: Alpha mode for the layer (C_LAYER_X_ALPHA_MODE).
> > +
> > +          xylon,layer-base-offset:
> > +            $ref: "/schemas/types.yaml#/definitions/uint32"
> > +            description: |
> > +              Offset in number of lines (C_LAYER_X_OFFSET) starting from the
> > +              video RAM base (C_VMEM_BASEADDR), only for version 3.
> > +
> > +          xylon,layer-buffer-offset:
> > +            $ref: "/schemas/types.yaml#/definitions/uint32"
> > +            description: |
> > +              Offset in number of lines (C_BUFFER_*_OFFSET) starting from the
> > +              layer base offset for the second buffer used in double-buffering.
> > +
> > +          xylon,layer-primary:
> > +            $ref: "/schemas/types.yaml#/definitions/flag"
> > +            description: |
> > +              Layer should be registered as a primary plane (exactly one is
> > +              required).
> > +
> > +        additionalProperties: false
> > +
> > +        required:
> > +          - reg
> > +          - xylon,layer-depth
> > +          - xylon,layer-colorspace
> > +          - xylon,layer-alpha-mode
> > +
> > +    required:
> > +      - "#address-cells"
> > +      - "#size-cells"
> > +      - layer@0
> > +
> > +    additionalProperties: false
> > +
> > +    description: |
> > +      The description of the display controller layers, containing layer
> > +      sub-nodes that each describe a registered layer.
> > +
> > +  ports:
> > +    type: object
> 
> You have to define what each port is. If only 1, then use just 'port'.
> 
> We now have graph.yaml (in dt-schema). You need to reference that here. 
> See drm-misc-next as everything has been converted.

I will definitely switch to a single port and describe its purpose.

Thanks!

Paul

> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - xylon,display-interface
> > +  - xylon,display-colorspace
> > +  - xylon,display-depth
> > +  - xylon,row-stride
> > +  - layers
> > +  - ports
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    logicvc: logicvc@...00000 {
> > +      compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
> > +      reg = <0x43c00000 0x6000>;
> > +
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +
> > +      logicvc_display: display@0 {
> > +        compatible = "xylon,logicvc-3.02.a-display";
> > +        reg = <0x0 0x6000>;
> > +
> > +        memory-region = <&logicvc_cma>;
> > +
> > +        clocks = <&logicvc_vclk 0>, <&logicvc_lvdsclk 0>;
> > +        clock-names = "vclk", "lvdsclk";
> > +
> > +        interrupt-parent = <&intc>;
> > +        interrupts = <0 34 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +        xylon,display-interface = "lvds-4bits";
> > +        xylon,display-colorspace = "rgb";
> > +        xylon,display-depth = <16>;
> > +        xylon,row-stride = <1024>;
> > +
> > +        xylon,layers-configurable;
> > +
> > +        layers {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          layer@0 {
> > +            reg = <0>;
> > +            xylon,layer-depth = <16>;
> > +            xylon,layer-colorspace = "rgb";
> > +            xylon,layer-alpha-mode = "layer";
> > +            xylon,layer-base-offset = <0>;
> > +            xylon,layer-buffer-offset = <480>;
> > +            xylon,layer-primary;
> > +          };
> > +
> > +          layer@1 {
> > +            reg = <1>;
> > +            xylon,layer-depth = <16>;
> > +            xylon,layer-colorspace = "rgb";
> > +            xylon,layer-alpha-mode = "layer";
> > +            xylon,layer-base-offset = <2400>;
> > +            xylon,layer-buffer-offset = <480>;
> > +          };
> > +
> > +          layer@2 {
> > +            reg = <2>;
> > +            xylon,layer-depth = <16>;
> > +            xylon,layer-colorspace = "rgb";
> > +            xylon,layer-alpha-mode = "layer";
> > +            xylon,layer-base-offset = <960>;
> > +            xylon,layer-buffer-offset = <480>;
> > +          };
> > +
> > +          layer@3 {
> > +            reg = <3>;
> > +            xylon,layer-depth = <16>;
> > +            xylon,layer-colorspace = "rgb";
> > +            xylon,layer-alpha-mode = "layer";
> > +            xylon,layer-base-offset = <480>;
> > +            xylon,layer-buffer-offset = <480>;
> > +          };
> > +
> > +          layer@4 {
> > +            reg = <4>;
> > +            xylon,layer-depth = <16>;
> > +            xylon,layer-colorspace = "rgb";
> > +            xylon,layer-alpha-mode = "layer";
> > +            xylon,layer-base-offset = <8192>;
> > +            xylon,layer-buffer-offset = <480>;
> > +          };
> > +        };
> > +
> > +        ports {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          logicvc_out: port@1 {
> > +            reg = <1>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            logicvc_output: endpoint@0 {
> > +              reg = <0>;
> > +              remote-endpoint = <&panel_input>;
> > +            };
> > +          };
> > +        };
> > +      };
> > +    };
> > -- 
> > 2.29.2
> > 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists