diff mbox series

[1/2] dt-bindings: display: Add xylon logicvc bindings documentation

Message ID 20190910153409.111901-2-paul.kocialkowski@bootlin.com
State Changes Requested, archived
Headers show
Series drm: LogiCVC display controller support | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Paul Kocialkowski Sept. 10, 2019, 3:34 p.m. UTC
The Xylon LogiCVC is a display controller implemented as programmable
logic in Xilinx FPGAs.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
 1 file changed, 188 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt

Comments

Rob Herring Sept. 13, 2019, 2:35 p.m. UTC | #1
On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> The Xylon LogiCVC is a display controller implemented as programmable
> logic in Xilinx FPGAs.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
>  1 file changed, 188 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt

Consider converting this to DT schema format. See 
Documentation/devicetree/writing-schema.rst (.md in 5.3).
 
> diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> new file mode 100644
> index 000000000000..eb4b1553888a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> @@ -0,0 +1,188 @@
> +Xylon LogiCVC display controller
> +
> +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. These do not allow configuring the controller
> +differently than synthesis configuration.
> +
> +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. With version 4,
> +framebuffers are configured with a direct memory address instead.
> +
> +Matching synthesis parameters are provided when applicable.
> +
> +Required properties:
> +- compatible: Should be one of:
> +  "xylon,logicvc-3.02.a-display"
> +  "xylon,logicvc-4.01.a-display"
> +- reg: Physical base address and size for the controller registers.
> +- clocks: List of phandle and clock-specifier pairs, one for each entry
> +  in 'clock-names'
> +- clock-names: List of clock names that should at least contain:
> +  - "vclk": The VCLK video clock input.
> +- interrupts: The interrupt to use for VBLANK signaling.
> +- xylon,display-interface: Display interface in use, should be one of:
> +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).

Presumably some of this is determined by the display attached. Isn't it 
safe to assume the IP was configured correctly for the intended display 
and you can just get this from the panel?


> +Optional properties:
> +- memory-region: phandle to a node describing memory, as specified in:
> +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +- clock-names: List of clock names that can optionally contain:
> +  - "vclk2": The VCLK2 doubled-rate video clock input.
> +  - "lvdsclk": The LVDS clock.
> +  - "lvdsclkn": The LVDS clock inverted.

How are these really optional?

> +- xylon,syscon: Syscon phandle representing the logicvc instance.
> +- xylon,dithering: Dithering module is enabled (C_XCOLOR).
> +- xylon,background-layer: The last layer is used to display a black background
> +  (C_USE_BACKGROUND). It must still be registered.
> +- xylon,layers-configurable: Configuration of layers' size, position and offset
> +  is enabled (C_USE_SIZE_POSITION).

I would think this will effectively have to be enabled to make this 
usable with DRM. I'm not sure if a "standard" userspace would use any of 
the layers if all this is fixed.

> +
> +Required sub-nodes:
> +- layers: The description of the display controller layers, containing layer
> +  sub-nodes that each describe a registered layer.
> +- ports: The LogiCVC connection to an encoder input port. The connection
> +  is modeled using the OF graph bindings, as specified in:
> +  Documentation/devicetree/bindings/graph.txt
> +
> +Required layer properties:
> +- reg: Layer index (from front to back, starting at 0).
> +- xylon,layer-depth: Layer depth in use (C_LAYER_0_DATA_WIDTH).
> +- xylon,layer-colorspace: Layer colorspace in use, should be one of:
> + - "rgb": RGB colorspace (C_LAYER_*_TYPE == 0).

Why is this needed if there's only 1?

> +- xylon,layer-alpha-mode: Alpha mode for the layer, should be one of:
> + - "layer": Alpha is configured layer-wide (C_LAYER_*_ALPHA_MODE == 0).
> + - "pixel": Alpha is configured per-pixel (C_LAYER_*_ALPHA_MODE == 1).

Could just be boolean.

> +- xylon,layer-base-offset: offset in number of lines (C_LAYER_*_OFFSET) starting
> +  from the video RAM base (C_VMEM_BASEADDR), only for version 3.
> +- xylon,layer-buffer-offset: offset in number of lines (C_BUFFER_*_OFFSET)
> +  starting from the layer base offset for the second buffer used in
> +  double-buffering.

It might be better to define all this in terms of byte offsets. I'd 
guess that is what CPU accesses are going to need.

> +
> +Optional layer properties:
> +- xylon,layer-primary: Layer should be registered as a primary plane (exactly
> +  one is required).
> +
> +Example:
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		logicvc_cma: cma@1f800000 {
> +			compatible = "shared-dma-pool";
> +			size = <0x800000>;
> +			alloc-ranges = <0x1f800000 0x800000>;
> +			reusable;
> +		};
> +	};
> +
> +	logicvc: logicvc@43c00000 {
> +		compatible = "syscon", "simple-mfd";
> +		reg = <0x43c00000 0x6000>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		logicvc_display: display-engine@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,syscon = <&logicvc>;
> +
> +			xylon,display-interface = "lvds-4bits";
> +			xylon,display-colorspace = "rgb";
> +			xylon,display-depth = <16>;
> +			xylon,row-stride = <1024>;
> +
> +			xylon,layers-configurable;
> +			xylon,layers-count = <5>;
> +
> +			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.23.0
>
Paul Kocialkowski Sept. 13, 2019, 3:58 p.m. UTC | #2
Hi Rob and thanks for the review!

On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > The Xylon LogiCVC is a display controller implemented as programmable
> > logic in Xilinx FPGAs.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> >  1 file changed, 188 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> 
> Consider converting this to DT schema format. See 
> Documentation/devicetree/writing-schema.rst (.md in 5.3).

Oh right, that would certainly be much more future-proof!

> > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > new file mode 100644
> > index 000000000000..eb4b1553888a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > @@ -0,0 +1,188 @@
> > +Xylon LogiCVC display controller
> > +
> > +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. These do not allow configuring the controller
> > +differently than synthesis configuration.
> > +
> > +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. With version 4,
> > +framebuffers are configured with a direct memory address instead.
> > +
> > +Matching synthesis parameters are provided when applicable.
> > +
> > +Required properties:
> > +- compatible: Should be one of:
> > +  "xylon,logicvc-3.02.a-display"
> > +  "xylon,logicvc-4.01.a-display"
> > +- reg: Physical base address and size for the controller registers.
> > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > +  in 'clock-names'
> > +- clock-names: List of clock names that should at least contain:
> > +  - "vclk": The VCLK video clock input.
> > +- interrupts: The interrupt to use for VBLANK signaling.
> > +- xylon,display-interface: Display interface in use, should be one of:
> > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> 
> Presumably some of this is determined by the display attached. Isn't it 
> safe to assume the IP was configured correctly for the intended display 
> and you can just get this from the panel?

Layers are what corresponds to DRM planes, which are not actually indicated
by the panel but are a charasteristic of the display controller. In our case,
this is directly selected at bitstream synthesis time for the controller.

So I'm afraid there is no way we can auto-detect this from the driver.

> 
> > +Optional properties:
> > +- memory-region: phandle to a node describing memory, as specified in:
> > +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > +- clock-names: List of clock names that can optionally contain:
> > +  - "vclk2": The VCLK2 doubled-rate video clock input.
> > +  - "lvdsclk": The LVDS clock.
> > +  - "lvdsclkn": The LVDS clock inverted.
> 
> How are these really optional?

Well, the controller currently only supports LVDS, but more interfaces may be
added later, so the lvdsclk clock will be optional when another interface
is used instead. Maybe I'm mistaken about how to categorize them though.

My understanding is that the need for vclk2 and lvdsclkn depend on the target
FPGA family. I've developped the driver without the need for them, but the
datasheet states that they may be needed (but doesn't provide significant
details about their role though).

> > +- xylon,syscon: Syscon phandle representing the logicvc instance.
> > +- xylon,dithering: Dithering module is enabled (C_XCOLOR).
> > +- xylon,background-layer: The last layer is used to display a black background
> > +  (C_USE_BACKGROUND). It must still be registered.
> > +- xylon,layers-configurable: Configuration of layers' size, position and offset
> > +  is enabled (C_USE_SIZE_POSITION).
> 
> I would think this will effectively have to be enabled to make this 
> usable with DRM. I'm not sure if a "standard" userspace would use any of 
> the layers if all this is fixed.

I was going with the same assumption, but drm_atomic_helper_check_plane_state
has a can_position parameter, which will check that the plane covers the
whole CRTC if set to false. So I guess it is somewhat expected that this can
be the case and some drivers (e.g. arm/hdlcd_crtc.c) also set this to false.

I guess this falls under a more generic discussion of what should be expected
by userspace when it comes to DRM. Since drivers may reject commits because of
any hardware-specific limitation, there is definitely a significant grey area
there and apparently no common rule.

> > +
> > +Required sub-nodes:
> > +- layers: The description of the display controller layers, containing layer
> > +  sub-nodes that each describe a registered layer.
> > +- ports: The LogiCVC connection to an encoder input port. The connection
> > +  is modeled using the OF graph bindings, as specified in:
> > +  Documentation/devicetree/bindings/graph.txt
> > +
> > +Required layer properties:
> > +- reg: Layer index (from front to back, starting at 0).
> > +- xylon,layer-depth: Layer depth in use (C_LAYER_0_DATA_WIDTH).
> > +- xylon,layer-colorspace: Layer colorspace in use, should be one of:
> > + - "rgb": RGB colorspace (C_LAYER_*_TYPE == 0).
> 
> Why is this needed if there's only 1?

The hardware supports more but support is no implemented yet, so the binding
document may be enriched along with the driver in the future.

Should I describe other colorspaces even if they are not currently supported?

> > +- xylon,layer-alpha-mode: Alpha mode for the layer, should be one of:
> > + - "layer": Alpha is configured layer-wide (C_LAYER_*_ALPHA_MODE == 0).
> > + - "pixel": Alpha is configured per-pixel (C_LAYER_*_ALPHA_MODE == 1).
> 
> Could just be boolean.

In this instance too, there are other modes that are not yet implemented but
supported by the hardware, so I did not describe them yet but they may be added
later.

> > +- xylon,layer-base-offset: offset in number of lines (C_LAYER_*_OFFSET) starting
> > +  from the video RAM base (C_VMEM_BASEADDR), only for version 3.
> > +- xylon,layer-buffer-offset: offset in number of lines (C_BUFFER_*_OFFSET)
> > +  starting from the layer base offset for the second buffer used in
> > +  double-buffering.
> 
> It might be better to define all this in terms of byte offsets. I'd 
> guess that is what CPU accesses are going to need.

I agree that it is more convenient from a driver's perspective, but the
rationale is that this allows copying parameters directly from the synthesis
configuration file, where these are expressed as a number of lines.

I would like to keep both delcarations as close to eachother as possible, as to
facilitate integration work for future users of the driver. But maybe this is a
bit too much in that case. What do you think?

Thanks again for the constructive feedback!

Cheers,

Paul

> > +
> > +Optional layer properties:
> > +- xylon,layer-primary: Layer should be registered as a primary plane (exactly
> > +  one is required).
> > +
> > +Example:
> > +
> > +	reserved-memory {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		ranges;
> > +
> > +		logicvc_cma: cma@1f800000 {
> > +			compatible = "shared-dma-pool";
> > +			size = <0x800000>;
> > +			alloc-ranges = <0x1f800000 0x800000>;
> > +			reusable;
> > +		};
> > +	};
> > +
> > +	logicvc: logicvc@43c00000 {
> > +		compatible = "syscon", "simple-mfd";
> > +		reg = <0x43c00000 0x6000>;
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +
> > +		logicvc_display: display-engine@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,syscon = <&logicvc>;
> > +
> > +			xylon,display-interface = "lvds-4bits";
> > +			xylon,display-colorspace = "rgb";
> > +			xylon,display-depth = <16>;
> > +			xylon,row-stride = <1024>;
> > +
> > +			xylon,layers-configurable;
> > +			xylon,layers-count = <5>;
> > +
> > +			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.23.0
> >
Rob Herring Sept. 13, 2019, 7:16 p.m. UTC | #3
On Fri, Sep 13, 2019 at 4:58 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi Rob and thanks for the review!
>
> On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> > On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > > The Xylon LogiCVC is a display controller implemented as programmable
> > > logic in Xilinx FPGAs.
> > >
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> > >  1 file changed, 188 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> >
> > Consider converting this to DT schema format. See
> > Documentation/devicetree/writing-schema.rst (.md in 5.3).
>
> Oh right, that would certainly be much more future-proof!
>
> > > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > new file mode 100644
> > > index 000000000000..eb4b1553888a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > @@ -0,0 +1,188 @@
> > > +Xylon LogiCVC display controller
> > > +
> > > +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. These do not allow configuring the controller
> > > +differently than synthesis configuration.
> > > +
> > > +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. With version 4,
> > > +framebuffers are configured with a direct memory address instead.
> > > +
> > > +Matching synthesis parameters are provided when applicable.
> > > +
> > > +Required properties:
> > > +- compatible: Should be one of:
> > > +  "xylon,logicvc-3.02.a-display"
> > > +  "xylon,logicvc-4.01.a-display"
> > > +- reg: Physical base address and size for the controller registers.
> > > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > > +  in 'clock-names'
> > > +- clock-names: List of clock names that should at least contain:
> > > +  - "vclk": The VCLK video clock input.
> > > +- interrupts: The interrupt to use for VBLANK signaling.
> > > +- xylon,display-interface: Display interface in use, should be one of:
> > > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> >
> > Presumably some of this is determined by the display attached. Isn't it
> > safe to assume the IP was configured correctly for the intended display
> > and you can just get this from the panel?
>
> Layers are what corresponds to DRM planes, which are not actually indicated
> by the panel but are a charasteristic of the display controller. In our case,
> this is directly selected at bitstream synthesis time for the controller.
>
> So I'm afraid there is no way we can auto-detect this from the driver.

Sorry, I referring to the set of properties above. In particular,
xylon,display-interface and xylon,display-colorspace, though I don't
know if the latter is talking in memory format or on the wire format.

Actually for xylon,layers-count, You should just count the child nodes
of 'layers'.

> > > +Optional properties:
> > > +- memory-region: phandle to a node describing memory, as specified in:
> > > +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > +- clock-names: List of clock names that can optionally contain:
> > > +  - "vclk2": The VCLK2 doubled-rate video clock input.
> > > +  - "lvdsclk": The LVDS clock.
> > > +  - "lvdsclkn": The LVDS clock inverted.
> >
> > How are these really optional?
>
> Well, the controller currently only supports LVDS, but more interfaces may be
> added later, so the lvdsclk clock will be optional when another interface
> is used instead. Maybe I'm mistaken about how to categorize them though.
>
> My understanding is that the need for vclk2 and lvdsclkn depend on the target
> FPGA family. I've developped the driver without the need for them, but the
> datasheet states that they may be needed (but doesn't provide significant
> details about their role though).

Not sure what to tell you then. You'll see it becomes a bit messy to
describe in schema. Ideally we define the exact number, order, and
values possible (or sets of those).

> > > +- xylon,syscon: Syscon phandle representing the logicvc instance.
> > > +- xylon,dithering: Dithering module is enabled (C_XCOLOR).
> > > +- xylon,background-layer: The last layer is used to display a black background
> > > +  (C_USE_BACKGROUND). It must still be registered.
> > > +- xylon,layers-configurable: Configuration of layers' size, position and offset
> > > +  is enabled (C_USE_SIZE_POSITION).
> >
> > I would think this will effectively have to be enabled to make this
> > usable with DRM. I'm not sure if a "standard" userspace would use any of
> > the layers if all this is fixed.
>
> I was going with the same assumption, but drm_atomic_helper_check_plane_state
> has a can_position parameter, which will check that the plane covers the
> whole CRTC if set to false. So I guess it is somewhat expected that this can
> be the case and some drivers (e.g. arm/hdlcd_crtc.c) also set this to false.

Certainly atomic can fail on anything not supported. My question is
more whether userspace has some minimum requirements. A cursor
couldn't deal with can_position=false for example.

> I guess this falls under a more generic discussion of what should be expected
> by userspace when it comes to DRM. Since drivers may reject commits because of
> any hardware-specific limitation, there is definitely a significant grey area
> there and apparently no common rule.
>
> > > +
> > > +Required sub-nodes:
> > > +- layers: The description of the display controller layers, containing layer
> > > +  sub-nodes that each describe a registered layer.
> > > +- ports: The LogiCVC connection to an encoder input port. The connection
> > > +  is modeled using the OF graph bindings, as specified in:
> > > +  Documentation/devicetree/bindings/graph.txt
> > > +
> > > +Required layer properties:
> > > +- reg: Layer index (from front to back, starting at 0).
> > > +- xylon,layer-depth: Layer depth in use (C_LAYER_0_DATA_WIDTH).
> > > +- xylon,layer-colorspace: Layer colorspace in use, should be one of:
> > > + - "rgb": RGB colorspace (C_LAYER_*_TYPE == 0).
> >
> > Why is this needed if there's only 1?
>
> The hardware supports more but support is no implemented yet, so the binding
> document may be enriched along with the driver in the future.
>
> Should I describe other colorspaces even if they are not currently supported?

Document what the h/w supports to the extent you can. Then we can make
better decisions.

> > > +- xylon,layer-alpha-mode: Alpha mode for the layer, should be one of:
> > > + - "layer": Alpha is configured layer-wide (C_LAYER_*_ALPHA_MODE == 0).
> > > + - "pixel": Alpha is configured per-pixel (C_LAYER_*_ALPHA_MODE == 1).
> >
> > Could just be boolean.
>
> In this instance too, there are other modes that are not yet implemented but
> supported by the hardware, so I did not describe them yet but they may be added
> later.
>
> > > +- xylon,layer-base-offset: offset in number of lines (C_LAYER_*_OFFSET) starting
> > > +  from the video RAM base (C_VMEM_BASEADDR), only for version 3.
> > > +- xylon,layer-buffer-offset: offset in number of lines (C_BUFFER_*_OFFSET)
> > > +  starting from the layer base offset for the second buffer used in
> > > +  double-buffering.
> >
> > It might be better to define all this in terms of byte offsets. I'd
> > guess that is what CPU accesses are going to need.
>
> I agree that it is more convenient from a driver's perspective, but the
> rationale is that this allows copying parameters directly from the synthesis
> configuration file, where these are expressed as a number of lines.
>
> I would like to keep both delcarations as close to eachother as possible, as to
> facilitate integration work for future users of the driver. But maybe this is a
> bit too much in that case. What do you think?

Fair enough. I'd feel differently if I thought this would be common,
but this seems to be a pretty specific usecase. I guess there could be
other fixed at synthesis h/w.

Rob
Paul Kocialkowski Sept. 23, 2019, 3:33 p.m. UTC | #4
Hi,

On Fri 13 Sep 19, 20:16, Rob Herring wrote:
> On Fri, Sep 13, 2019 at 4:58 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> >
> > Hi Rob and thanks for the review!
> >
> > On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> > > On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > > > The Xylon LogiCVC is a display controller implemented as programmable
> > > > logic in Xilinx FPGAs.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > ---
> > > >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> > > >  1 file changed, 188 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > >
> > > Consider converting this to DT schema format. See
> > > Documentation/devicetree/writing-schema.rst (.md in 5.3).
> >
> > Oh right, that would certainly be much more future-proof!
> >
> > > > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > new file mode 100644
> > > > index 000000000000..eb4b1553888a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > @@ -0,0 +1,188 @@
> > > > +Xylon LogiCVC display controller
> > > > +
> > > > +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. These do not allow configuring the controller
> > > > +differently than synthesis configuration.
> > > > +
> > > > +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. With version 4,
> > > > +framebuffers are configured with a direct memory address instead.
> > > > +
> > > > +Matching synthesis parameters are provided when applicable.
> > > > +
> > > > +Required properties:
> > > > +- compatible: Should be one of:
> > > > +  "xylon,logicvc-3.02.a-display"
> > > > +  "xylon,logicvc-4.01.a-display"
> > > > +- reg: Physical base address and size for the controller registers.
> > > > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > > > +  in 'clock-names'
> > > > +- clock-names: List of clock names that should at least contain:
> > > > +  - "vclk": The VCLK video clock input.
> > > > +- interrupts: The interrupt to use for VBLANK signaling.
> > > > +- xylon,display-interface: Display interface in use, should be one of:
> > > > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > > > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > > > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > > > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > > > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > > > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> > >
> > > Presumably some of this is determined by the display attached. Isn't it
> > > safe to assume the IP was configured correctly for the intended display
> > > and you can just get this from the panel?
> >
> > Layers are what corresponds to DRM planes, which are not actually indicated
> > by the panel but are a charasteristic of the display controller. In our case,
> > this is directly selected at bitstream synthesis time for the controller.
> >
> > So I'm afraid there is no way we can auto-detect this from the driver.
> 
> Sorry, I referring to the set of properties above. In particular,
> xylon,display-interface and xylon,display-colorspace, though I don't
> know if the latter is talking in memory format or on the wire format.

Both of these are about the wire format, which is also "hardcoded" at synthesis
time with no way to be detected afterwards, as far as I know. Memory format is
described in the layer sub-nodes.

> Actually for xylon,layers-count, You should just count the child nodes
> of 'layers'.

Oh that's a good point, thanks!

> > > > +Optional properties:
> > > > +- memory-region: phandle to a node describing memory, as specified in:
> > > > +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > +- clock-names: List of clock names that can optionally contain:
> > > > +  - "vclk2": The VCLK2 doubled-rate video clock input.
> > > > +  - "lvdsclk": The LVDS clock.
> > > > +  - "lvdsclkn": The LVDS clock inverted.
> > >
> > > How are these really optional?
> >
> > Well, the controller currently only supports LVDS, but more interfaces may be
> > added later, so the lvdsclk clock will be optional when another interface
> > is used instead. Maybe I'm mistaken about how to categorize them though.
> >
> > My understanding is that the need for vclk2 and lvdsclkn depend on the target
> > FPGA family. I've developped the driver without the need for them, but the
> > datasheet states that they may be needed (but doesn't provide significant
> > details about their role though).
> 
> Not sure what to tell you then. You'll see it becomes a bit messy to
> describe in schema. Ideally we define the exact number, order, and
> values possible (or sets of those).

I'll try to do my best.

> > > > +- xylon,syscon: Syscon phandle representing the logicvc instance.
> > > > +- xylon,dithering: Dithering module is enabled (C_XCOLOR).
> > > > +- xylon,background-layer: The last layer is used to display a black background
> > > > +  (C_USE_BACKGROUND). It must still be registered.
> > > > +- xylon,layers-configurable: Configuration of layers' size, position and offset
> > > > +  is enabled (C_USE_SIZE_POSITION).
> > >
> > > I would think this will effectively have to be enabled to make this
> > > usable with DRM. I'm not sure if a "standard" userspace would use any of
> > > the layers if all this is fixed.
> >
> > I was going with the same assumption, but drm_atomic_helper_check_plane_state
> > has a can_position parameter, which will check that the plane covers the
> > whole CRTC if set to false. So I guess it is somewhat expected that this can
> > be the case and some drivers (e.g. arm/hdlcd_crtc.c) also set this to false.
> 
> Certainly atomic can fail on anything not supported. My question is
> more whether userspace has some minimum requirements. A cursor
> couldn't deal with can_position=false for example.

Right, so I suppose that using an overlay plane as cursor wouldn't work
in this situation. Well, I haven't found any formal definition of what minimal
requirements are expected from overlay planes. I would expect userspace that
tries to use an overlay plane as a cursor to have a software fallback as soon
as something goes wrong. My feeling is that overlay planes are provided on a
"best-effort" basis, though contradiction is welcome here.

> > I guess this falls under a more generic discussion of what should be expected
> > by userspace when it comes to DRM. Since drivers may reject commits because of
> > any hardware-specific limitation, there is definitely a significant grey area
> > there and apparently no common rule.
> >
> > > > +
> > > > +Required sub-nodes:
> > > > +- layers: The description of the display controller layers, containing layer
> > > > +  sub-nodes that each describe a registered layer.
> > > > +- ports: The LogiCVC connection to an encoder input port. The connection
> > > > +  is modeled using the OF graph bindings, as specified in:
> > > > +  Documentation/devicetree/bindings/graph.txt
> > > > +
> > > > +Required layer properties:
> > > > +- reg: Layer index (from front to back, starting at 0).
> > > > +- xylon,layer-depth: Layer depth in use (C_LAYER_0_DATA_WIDTH).
> > > > +- xylon,layer-colorspace: Layer colorspace in use, should be one of:
> > > > + - "rgb": RGB colorspace (C_LAYER_*_TYPE == 0).
> > >
> > > Why is this needed if there's only 1?
> >
> > The hardware supports more but support is no implemented yet, so the binding
> > document may be enriched along with the driver in the future.
> >
> > Should I describe other colorspaces even if they are not currently supported?
> 
> Document what the h/w supports to the extent you can. Then we can make
> better decisions.

Okay then, I'll include those well-known other possibilities in the description.
Hopefully users will take the description for what it is and refrain from
expecting that the driver currently supports all that is described.

Thanks for the follow-up!

Cheers,

Paul

> > > > +- xylon,layer-alpha-mode: Alpha mode for the layer, should be one of:
> > > > + - "layer": Alpha is configured layer-wide (C_LAYER_*_ALPHA_MODE == 0).
> > > > + - "pixel": Alpha is configured per-pixel (C_LAYER_*_ALPHA_MODE == 1).
> > >
> > > Could just be boolean.
> >
> > In this instance too, there are other modes that are not yet implemented but
> > supported by the hardware, so I did not describe them yet but they may be added
> > later.
> >
> > > > +- xylon,layer-base-offset: offset in number of lines (C_LAYER_*_OFFSET) starting
> > > > +  from the video RAM base (C_VMEM_BASEADDR), only for version 3.
> > > > +- xylon,layer-buffer-offset: offset in number of lines (C_BUFFER_*_OFFSET)
> > > > +  starting from the layer base offset for the second buffer used in
> > > > +  double-buffering.
> > >
> > > It might be better to define all this in terms of byte offsets. I'd
> > > guess that is what CPU accesses are going to need.
> >
> > I agree that it is more convenient from a driver's perspective, but the
> > rationale is that this allows copying parameters directly from the synthesis
> > configuration file, where these are expressed as a number of lines.
> >
> > I would like to keep both delcarations as close to eachother as possible, as to
> > facilitate integration work for future users of the driver. But maybe this is a
> > bit too much in that case. What do you think?
> 
> Fair enough. I'd feel differently if I thought this would be common,
> but this seems to be a pretty specific usecase. I guess there could be
> other fixed at synthesis h/w.
> 
> Rob
Rob Herring Sept. 24, 2019, 2:58 p.m. UTC | #5
On Mon, Sep 23, 2019 at 10:33 AM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Fri 13 Sep 19, 20:16, Rob Herring wrote:
> > On Fri, Sep 13, 2019 at 4:58 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > >
> > > Hi Rob and thanks for the review!
> > >
> > > On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> > > > On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > > > > The Xylon LogiCVC is a display controller implemented as programmable
> > > > > logic in Xilinx FPGAs.
> > > > >
> > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > ---
> > > > >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> > > > >  1 file changed, 188 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > >
> > > > Consider converting this to DT schema format. See
> > > > Documentation/devicetree/writing-schema.rst (.md in 5.3).
> > >
> > > Oh right, that would certainly be much more future-proof!
> > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > new file mode 100644
> > > > > index 000000000000..eb4b1553888a
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > @@ -0,0 +1,188 @@
> > > > > +Xylon LogiCVC display controller
> > > > > +
> > > > > +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. These do not allow configuring the controller
> > > > > +differently than synthesis configuration.
> > > > > +
> > > > > +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. With version 4,
> > > > > +framebuffers are configured with a direct memory address instead.
> > > > > +
> > > > > +Matching synthesis parameters are provided when applicable.
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible: Should be one of:
> > > > > +  "xylon,logicvc-3.02.a-display"
> > > > > +  "xylon,logicvc-4.01.a-display"
> > > > > +- reg: Physical base address and size for the controller registers.
> > > > > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > > > > +  in 'clock-names'
> > > > > +- clock-names: List of clock names that should at least contain:
> > > > > +  - "vclk": The VCLK video clock input.
> > > > > +- interrupts: The interrupt to use for VBLANK signaling.
> > > > > +- xylon,display-interface: Display interface in use, should be one of:
> > > > > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > > > > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > > > > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > > > > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > > > > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > > > > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> > > >
> > > > Presumably some of this is determined by the display attached. Isn't it
> > > > safe to assume the IP was configured correctly for the intended display
> > > > and you can just get this from the panel?
> > >
> > > Layers are what corresponds to DRM planes, which are not actually indicated
> > > by the panel but are a charasteristic of the display controller. In our case,
> > > this is directly selected at bitstream synthesis time for the controller.
> > >
> > > So I'm afraid there is no way we can auto-detect this from the driver.
> >
> > Sorry, I referring to the set of properties above. In particular,
> > xylon,display-interface and xylon,display-colorspace, though I don't
> > know if the latter is talking in memory format or on the wire format.
>
> Both of these are about the wire format, which is also "hardcoded" at synthesis
> time with no way to be detected afterwards, as far as I know. Memory format is
> described in the layer sub-nodes.

You have to attach the controller to something at the other end of the
wire. A panel is only going to support 1 or a few wire formats, so you
do likely know because the panel knows. In the case that a panel
supports multiple wire formats, we do have some standard properties
there. See the LVDS panel binding.

>
> > Actually for xylon,layers-count, You should just count the child nodes
> > of 'layers'.
>
> Oh that's a good point, thanks!
>
> > > > > +Optional properties:
> > > > > +- memory-region: phandle to a node describing memory, as specified in:
> > > > > +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > +- clock-names: List of clock names that can optionally contain:
> > > > > +  - "vclk2": The VCLK2 doubled-rate video clock input.
> > > > > +  - "lvdsclk": The LVDS clock.
> > > > > +  - "lvdsclkn": The LVDS clock inverted.
> > > >
> > > > How are these really optional?
> > >
> > > Well, the controller currently only supports LVDS, but more interfaces may be
> > > added later, so the lvdsclk clock will be optional when another interface
> > > is used instead. Maybe I'm mistaken about how to categorize them though.
> > >
> > > My understanding is that the need for vclk2 and lvdsclkn depend on the target
> > > FPGA family. I've developped the driver without the need for them, but the
> > > datasheet states that they may be needed (but doesn't provide significant
> > > details about their role though).
> >
> > Not sure what to tell you then. You'll see it becomes a bit messy to
> > describe in schema. Ideally we define the exact number, order, and
> > values possible (or sets of those).
>
> I'll try to do my best.
>
> > > > > +- xylon,syscon: Syscon phandle representing the logicvc instance.
> > > > > +- xylon,dithering: Dithering module is enabled (C_XCOLOR).
> > > > > +- xylon,background-layer: The last layer is used to display a black background
> > > > > +  (C_USE_BACKGROUND). It must still be registered.
> > > > > +- xylon,layers-configurable: Configuration of layers' size, position and offset
> > > > > +  is enabled (C_USE_SIZE_POSITION).
> > > >
> > > > I would think this will effectively have to be enabled to make this
> > > > usable with DRM. I'm not sure if a "standard" userspace would use any of
> > > > the layers if all this is fixed.
> > >
> > > I was going with the same assumption, but drm_atomic_helper_check_plane_state
> > > has a can_position parameter, which will check that the plane covers the
> > > whole CRTC if set to false. So I guess it is somewhat expected that this can
> > > be the case and some drivers (e.g. arm/hdlcd_crtc.c) also set this to false.
> >
> > Certainly atomic can fail on anything not supported. My question is
> > more whether userspace has some minimum requirements. A cursor
> > couldn't deal with can_position=false for example.
>
> Right, so I suppose that using an overlay plane as cursor wouldn't work
> in this situation. Well, I haven't found any formal definition of what minimal
> requirements are expected from overlay planes. I would expect userspace that
> tries to use an overlay plane as a cursor to have a software fallback as soon
> as something goes wrong. My feeling is that overlay planes are provided on a
> "best-effort" basis, though contradiction is welcome here.

For sure, there's always a software fallback. While we shouldn't let a
specific OS's requirements dictate DT bindings, I just wonder if some
of the configuration ends up always having to be set a certain way.
Clearly, you could be writing the whole software stack and do a fixed
configuration, but would you still be using DT at that point?

Rob
Paul Kocialkowski Nov. 20, 2019, 2:49 p.m. UTC | #6
Hi,

Circling back to this thread now, sorry for the delay.

On Tue 24 Sep 19, 09:58, Rob Herring wrote:
> On Mon, Sep 23, 2019 at 10:33 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> >
> > Hi,
> >
> > On Fri 13 Sep 19, 20:16, Rob Herring wrote:
> > > On Fri, Sep 13, 2019 at 4:58 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > >
> > > > Hi Rob and thanks for the review!
> > > >
> > > > On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> > > > > On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > > > > > The Xylon LogiCVC is a display controller implemented as programmable
> > > > > > logic in Xilinx FPGAs.
> > > > > >
> > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > ---
> > > > > >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> > > > > >  1 file changed, 188 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > >
> > > > > Consider converting this to DT schema format. See
> > > > > Documentation/devicetree/writing-schema.rst (.md in 5.3).
> > > >
> > > > Oh right, that would certainly be much more future-proof!
> > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > > new file mode 100644
> > > > > > index 000000000000..eb4b1553888a
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > > @@ -0,0 +1,188 @@
> > > > > > +Xylon LogiCVC display controller
> > > > > > +
> > > > > > +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. These do not allow configuring the controller
> > > > > > +differently than synthesis configuration.
> > > > > > +
> > > > > > +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. With version 4,
> > > > > > +framebuffers are configured with a direct memory address instead.
> > > > > > +
> > > > > > +Matching synthesis parameters are provided when applicable.
> > > > > > +
> > > > > > +Required properties:
> > > > > > +- compatible: Should be one of:
> > > > > > +  "xylon,logicvc-3.02.a-display"
> > > > > > +  "xylon,logicvc-4.01.a-display"
> > > > > > +- reg: Physical base address and size for the controller registers.
> > > > > > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > > > > > +  in 'clock-names'
> > > > > > +- clock-names: List of clock names that should at least contain:
> > > > > > +  - "vclk": The VCLK video clock input.
> > > > > > +- interrupts: The interrupt to use for VBLANK signaling.
> > > > > > +- xylon,display-interface: Display interface in use, should be one of:
> > > > > > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > > > > > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > > > > > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > > > > > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > > > > > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > > > > > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> > > > >
> > > > > Presumably some of this is determined by the display attached. Isn't it
> > > > > safe to assume the IP was configured correctly for the intended display
> > > > > and you can just get this from the panel?
> > > >
> > > > Layers are what corresponds to DRM planes, which are not actually indicated
> > > > by the panel but are a charasteristic of the display controller. In our case,
> > > > this is directly selected at bitstream synthesis time for the controller.
> > > >
> > > > So I'm afraid there is no way we can auto-detect this from the driver.
> > >
> > > Sorry, I referring to the set of properties above. In particular,
> > > xylon,display-interface and xylon,display-colorspace, though I don't
> > > know if the latter is talking in memory format or on the wire format.
> >
> > Both of these are about the wire format, which is also "hardcoded" at synthesis
> > time with no way to be detected afterwards, as far as I know. Memory format is
> > described in the layer sub-nodes.
> 
> You have to attach the controller to something at the other end of the
> wire. A panel is only going to support 1 or a few wire formats, so you
> do likely know because the panel knows. In the case that a panel
> supports multiple wire formats, we do have some standard properties
> there. See the LVDS panel binding.

Looking at the LVDS panel binding, I see that the LVDS types that I have
described as lvds-4bits and lvds-3bits are called jeida-24 and jeida-18.

Either way, the controller cannot be dynamically configured to use one or
another: it is configured to support one at synthesis time and this doesn't
change.

I'm not sure exactly what you implied here. Even if we can retreive the
wire format from the lvds-panel's data-mapping property, I don't think it shall
describe what the display controller was configured to. This information could
be used to make sure that both are compatible (in the driver), but that's about
it as far as I can see.

> >
> > > Actually for xylon,layers-count, You should just count the child nodes
> > > of 'layers'.
> >
> > Oh that's a good point, thanks!
> >
> > > > > > +Optional properties:
> > > > > > +- memory-region: phandle to a node describing memory, as specified in:
> > > > > > +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > > +- clock-names: List of clock names that can optionally contain:
> > > > > > +  - "vclk2": The VCLK2 doubled-rate video clock input.
> > > > > > +  - "lvdsclk": The LVDS clock.
> > > > > > +  - "lvdsclkn": The LVDS clock inverted.
> > > > >
> > > > > How are these really optional?
> > > >
> > > > Well, the controller currently only supports LVDS, but more interfaces may be
> > > > added later, so the lvdsclk clock will be optional when another interface
> > > > is used instead. Maybe I'm mistaken about how to categorize them though.
> > > >
> > > > My understanding is that the need for vclk2 and lvdsclkn depend on the target
> > > > FPGA family. I've developped the driver without the need for them, but the
> > > > datasheet states that they may be needed (but doesn't provide significant
> > > > details about their role though).
> > >
> > > Not sure what to tell you then. You'll see it becomes a bit messy to
> > > describe in schema. Ideally we define the exact number, order, and
> > > values possible (or sets of those).
> >
> > I'll try to do my best.
> >
> > > > > > +- xylon,syscon: Syscon phandle representing the logicvc instance.
> > > > > > +- xylon,dithering: Dithering module is enabled (C_XCOLOR).
> > > > > > +- xylon,background-layer: The last layer is used to display a black background
> > > > > > +  (C_USE_BACKGROUND). It must still be registered.
> > > > > > +- xylon,layers-configurable: Configuration of layers' size, position and offset
> > > > > > +  is enabled (C_USE_SIZE_POSITION).
> > > > >
> > > > > I would think this will effectively have to be enabled to make this
> > > > > usable with DRM. I'm not sure if a "standard" userspace would use any of
> > > > > the layers if all this is fixed.
> > > >
> > > > I was going with the same assumption, but drm_atomic_helper_check_plane_state
> > > > has a can_position parameter, which will check that the plane covers the
> > > > whole CRTC if set to false. So I guess it is somewhat expected that this can
> > > > be the case and some drivers (e.g. arm/hdlcd_crtc.c) also set this to false.
> > >
> > > Certainly atomic can fail on anything not supported. My question is
> > > more whether userspace has some minimum requirements. A cursor
> > > couldn't deal with can_position=false for example.
> >
> > Right, so I suppose that using an overlay plane as cursor wouldn't work
> > in this situation. Well, I haven't found any formal definition of what minimal
> > requirements are expected from overlay planes. I would expect userspace that
> > tries to use an overlay plane as a cursor to have a software fallback as soon
> > as something goes wrong. My feeling is that overlay planes are provided on a
> > "best-effort" basis, though contradiction is welcome here.
> 
> For sure, there's always a software fallback. While we shouldn't let a
> specific OS's requirements dictate DT bindings, I just wonder if some
> of the configuration ends up always having to be set a certain way.
> Clearly, you could be writing the whole software stack and do a fixed
> configuration, but would you still be using DT at that point?

From my understanding, all the possible combinations make sense here.
The ability to position layers is of course quite welcome for a generic
use case, but it certainly makes sense to reduce the controller's gate count by
removing the feature if the final use case doesn't need it. So I don't think
some of these properties end having to be set in a specific way to make sense.

Cheers,

Paul
Rob Herring Nov. 20, 2019, 4:02 p.m. UTC | #7
On Wed, Nov 20, 2019 at 8:50 AM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Circling back to this thread now, sorry for the delay.
>
> On Tue 24 Sep 19, 09:58, Rob Herring wrote:
> > On Mon, Sep 23, 2019 at 10:33 AM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri 13 Sep 19, 20:16, Rob Herring wrote:
> > > > On Fri, Sep 13, 2019 at 4:58 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > >
> > > > > Hi Rob and thanks for the review!
> > > > >
> > > > > On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> > > > > > On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > > > > > > The Xylon LogiCVC is a display controller implemented as programmable
> > > > > > > logic in Xilinx FPGAs.
> > > > > > >
> > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > ---
> > > > > > >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> > > > > > >  1 file changed, 188 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > >
> > > > > > Consider converting this to DT schema format. See
> > > > > > Documentation/devicetree/writing-schema.rst (.md in 5.3).
> > > > >
> > > > > Oh right, that would certainly be much more future-proof!
> > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..eb4b1553888a
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > > > @@ -0,0 +1,188 @@
> > > > > > > +Xylon LogiCVC display controller
> > > > > > > +
> > > > > > > +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. These do not allow configuring the controller
> > > > > > > +differently than synthesis configuration.
> > > > > > > +
> > > > > > > +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. With version 4,
> > > > > > > +framebuffers are configured with a direct memory address instead.
> > > > > > > +
> > > > > > > +Matching synthesis parameters are provided when applicable.
> > > > > > > +
> > > > > > > +Required properties:
> > > > > > > +- compatible: Should be one of:
> > > > > > > +  "xylon,logicvc-3.02.a-display"
> > > > > > > +  "xylon,logicvc-4.01.a-display"
> > > > > > > +- reg: Physical base address and size for the controller registers.
> > > > > > > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > > > > > > +  in 'clock-names'
> > > > > > > +- clock-names: List of clock names that should at least contain:
> > > > > > > +  - "vclk": The VCLK video clock input.
> > > > > > > +- interrupts: The interrupt to use for VBLANK signaling.
> > > > > > > +- xylon,display-interface: Display interface in use, should be one of:
> > > > > > > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > > > > > > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > > > > > > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > > > > > > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > > > > > > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > > > > > > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> > > > > >
> > > > > > Presumably some of this is determined by the display attached. Isn't it
> > > > > > safe to assume the IP was configured correctly for the intended display
> > > > > > and you can just get this from the panel?
> > > > >
> > > > > Layers are what corresponds to DRM planes, which are not actually indicated
> > > > > by the panel but are a charasteristic of the display controller. In our case,
> > > > > this is directly selected at bitstream synthesis time for the controller.
> > > > >
> > > > > So I'm afraid there is no way we can auto-detect this from the driver.
> > > >
> > > > Sorry, I referring to the set of properties above. In particular,
> > > > xylon,display-interface and xylon,display-colorspace, though I don't
> > > > know if the latter is talking in memory format or on the wire format.
> > >
> > > Both of these are about the wire format, which is also "hardcoded" at synthesis
> > > time with no way to be detected afterwards, as far as I know. Memory format is
> > > described in the layer sub-nodes.
> >
> > You have to attach the controller to something at the other end of the
> > wire. A panel is only going to support 1 or a few wire formats, so you
> > do likely know because the panel knows. In the case that a panel
> > supports multiple wire formats, we do have some standard properties
> > there. See the LVDS panel binding.
>
> Looking at the LVDS panel binding, I see that the LVDS types that I have
> described as lvds-4bits and lvds-3bits are called jeida-24 and jeida-18.
>
> Either way, the controller cannot be dynamically configured to use one or
> another: it is configured to support one at synthesis time and this doesn't
> change.

Understood, but I was assuming you need to know how it was configured
for some reason?

> I'm not sure exactly what you implied here. Even if we can retreive the
> wire format from the lvds-panel's data-mapping property, I don't think it shall
> describe what the display controller was configured to. This information could
> be used to make sure that both are compatible (in the driver), but that's about
> it as far as I can see.

It's not the kernel's job to validate the DT is correct. Someone could
just as easily define a panel that doesn't match with the configured
format as they could having lvds-?bits set incorrectly.

So get the wire format from the panel driver (either implied or by DT
property) and assume that matches the configuration of the controller.
Though, I guess if the model is each end of the wire should advertise
what it supports and the core picks the best format, then that only
works if you advertise both formats. Or we could allow jeida-{24,18}
property at both ends of the graph.

Rob
Paul Kocialkowski Nov. 20, 2019, 4:45 p.m. UTC | #8
Hi,

On Wed 20 Nov 19, 10:02, Rob Herring wrote:
> On Wed, Nov 20, 2019 at 8:50 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> >
> > Hi,
> >
> > Circling back to this thread now, sorry for the delay.
> >
> > On Tue 24 Sep 19, 09:58, Rob Herring wrote:
> > > On Mon, Sep 23, 2019 at 10:33 AM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Fri 13 Sep 19, 20:16, Rob Herring wrote:
> > > > > On Fri, Sep 13, 2019 at 4:58 PM Paul Kocialkowski
> > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > >
> > > > > > Hi Rob and thanks for the review!
> > > > > >
> > > > > > On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> > > > > > > On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > > > > > > > The Xylon LogiCVC is a display controller implemented as programmable
> > > > > > > > logic in Xilinx FPGAs.
> > > > > > > >
> > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > ---
> > > > > > > >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> > > > > > > >  1 file changed, 188 insertions(+)
> > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > > >
> > > > > > > Consider converting this to DT schema format. See
> > > > > > > Documentation/devicetree/writing-schema.rst (.md in 5.3).
> > > > > >
> > > > > > Oh right, that would certainly be much more future-proof!
> > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..eb4b1553888a
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > > > > > > @@ -0,0 +1,188 @@
> > > > > > > > +Xylon LogiCVC display controller
> > > > > > > > +
> > > > > > > > +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. These do not allow configuring the controller
> > > > > > > > +differently than synthesis configuration.
> > > > > > > > +
> > > > > > > > +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. With version 4,
> > > > > > > > +framebuffers are configured with a direct memory address instead.
> > > > > > > > +
> > > > > > > > +Matching synthesis parameters are provided when applicable.
> > > > > > > > +
> > > > > > > > +Required properties:
> > > > > > > > +- compatible: Should be one of:
> > > > > > > > +  "xylon,logicvc-3.02.a-display"
> > > > > > > > +  "xylon,logicvc-4.01.a-display"
> > > > > > > > +- reg: Physical base address and size for the controller registers.
> > > > > > > > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > > > > > > > +  in 'clock-names'
> > > > > > > > +- clock-names: List of clock names that should at least contain:
> > > > > > > > +  - "vclk": The VCLK video clock input.
> > > > > > > > +- interrupts: The interrupt to use for VBLANK signaling.
> > > > > > > > +- xylon,display-interface: Display interface in use, should be one of:
> > > > > > > > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > > > > > > > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > > > > > > > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > > > > > > > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > > > > > > > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > > > > > > > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> > > > > > >
> > > > > > > Presumably some of this is determined by the display attached. Isn't it
> > > > > > > safe to assume the IP was configured correctly for the intended display
> > > > > > > and you can just get this from the panel?
> > > > > >
> > > > > > Layers are what corresponds to DRM planes, which are not actually indicated
> > > > > > by the panel but are a charasteristic of the display controller. In our case,
> > > > > > this is directly selected at bitstream synthesis time for the controller.
> > > > > >
> > > > > > So I'm afraid there is no way we can auto-detect this from the driver.
> > > > >
> > > > > Sorry, I referring to the set of properties above. In particular,
> > > > > xylon,display-interface and xylon,display-colorspace, though I don't
> > > > > know if the latter is talking in memory format or on the wire format.
> > > >
> > > > Both of these are about the wire format, which is also "hardcoded" at synthesis
> > > > time with no way to be detected afterwards, as far as I know. Memory format is
> > > > described in the layer sub-nodes.
> > >
> > > You have to attach the controller to something at the other end of the
> > > wire. A panel is only going to support 1 or a few wire formats, so you
> > > do likely know because the panel knows. In the case that a panel
> > > supports multiple wire formats, we do have some standard properties
> > > there. See the LVDS panel binding.
> >
> > Looking at the LVDS panel binding, I see that the LVDS types that I have
> > described as lvds-4bits and lvds-3bits are called jeida-24 and jeida-18.
> >
> > Either way, the controller cannot be dynamically configured to use one or
> > another: it is configured to support one at synthesis time and this doesn't
> > change.
> 
> Understood, but I was assuming you need to know how it was configured
> for some reason?

Well, the information that is really useful for the driver is whether it's
configured as LVDS or something else. The detail of 3bit vs 4bit doesn't really
matter for Linux/DRM but I kept it to that precision to stick close to the
hardware description. But yeah, having a single "lvds" choice in the
display-interface property for these two cases would work too and I can do that
if you prefer.

> > I'm not sure exactly what you implied here. Even if we can retreive the
> > wire format from the lvds-panel's data-mapping property, I don't think it shall
> > describe what the display controller was configured to. This information could
> > be used to make sure that both are compatible (in the driver), but that's about
> > it as far as I can see.
> 
> It's not the kernel's job to validate the DT is correct. Someone could
> just as easily define a panel that doesn't match with the configured
> format as they could having lvds-?bits set incorrectly.

So this is a situation that we are supposed to allow?
I assumed it would be in everyone's best interest to detect that there is a
mismatch, but I don't have any strong opinion about that.

> So get the wire format from the panel driver (either implied or by DT
> property) and assume that matches the configuration of the controller.

To be honest, it feels a bit weird to depend on a panel being connected to know
what display interface the driver should register. It might work in practice
since these media bus formats are specific to LVDS, so it can be correctly
deduced, but I don't understand the advantage of doing that.

It will probably also require parsing the panel remote endpoint's dt props as
I don't think we can get the bus format information from DRM before having
registered an encoder/connector on the controller side (so that's a
chicken-and-egg problem).

More generally, I don't see the issue behind describing this in the controller's
bindings: is it because of unwanted redundency or such?

> Though, I guess if the model is each end of the wire should advertise
> what it supports and the core picks the best format, then that only
> works if you advertise both formats. Or we could allow jeida-{24,18}
> property at both ends of the graph.

I don't think there is such negotiation currently implemented in DRM
(and many driver actually have a strong assumption that only one bus format
is provided).

If we want the DRM core to deal with that, we need drivers to report their bus
format abilities for each concerned encoder/connector. I'm not sure this could
be done in a generic way with a generic dt property since the of_graph to
encoder/connector association seems to be quite driver-specific anyway.
This would just be deporting a description of capabilities of the hardware from
the driver to dt, with no particular gain as far as I can see.

But maybe I'm missing the bigger picture here and describing hardware
capabilities in device-tree is something we want to go towards. I must admit
that I find the boundary quite blurry.

Cheers,

Paul
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
new file mode 100644
index 000000000000..eb4b1553888a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
@@ -0,0 +1,188 @@ 
+Xylon LogiCVC display controller
+
+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. These do not allow configuring the controller
+differently than synthesis configuration.
+
+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. With version 4,
+framebuffers are configured with a direct memory address instead.
+
+Matching synthesis parameters are provided when applicable.
+
+Required properties:
+- compatible: Should be one of:
+  "xylon,logicvc-3.02.a-display"
+  "xylon,logicvc-4.01.a-display"
+- reg: Physical base address and size for the controller registers.
+- clocks: List of phandle and clock-specifier pairs, one for each entry
+  in 'clock-names'
+- clock-names: List of clock names that should at least contain:
+  - "vclk": The VCLK video clock input.
+- interrupts: The interrupt to use for VBLANK signaling.
+- xylon,display-interface: Display interface in use, should be one of:
+  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
+- xylon,display-colorspace: Display output colorspace in use, should be one of:
+  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
+- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
+- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
+- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
+
+Optional properties:
+- memory-region: phandle to a node describing memory, as specified in:
+  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+- clock-names: List of clock names that can optionally contain:
+  - "vclk2": The VCLK2 doubled-rate video clock input.
+  - "lvdsclk": The LVDS clock.
+  - "lvdsclkn": The LVDS clock inverted.
+- xylon,syscon: Syscon phandle representing the logicvc instance.
+- xylon,dithering: Dithering module is enabled (C_XCOLOR).
+- xylon,background-layer: The last layer is used to display a black background
+  (C_USE_BACKGROUND). It must still be registered.
+- xylon,layers-configurable: Configuration of layers' size, position and offset
+  is enabled (C_USE_SIZE_POSITION).
+
+Required sub-nodes:
+- layers: The description of the display controller layers, containing layer
+  sub-nodes that each describe a registered layer.
+- ports: The LogiCVC connection to an encoder input port. The connection
+  is modeled using the OF graph bindings, as specified in:
+  Documentation/devicetree/bindings/graph.txt
+
+Required layer properties:
+- reg: Layer index (from front to back, starting at 0).
+- xylon,layer-depth: Layer depth in use (C_LAYER_0_DATA_WIDTH).
+- xylon,layer-colorspace: Layer colorspace in use, should be one of:
+ - "rgb": RGB colorspace (C_LAYER_*_TYPE == 0).
+- xylon,layer-alpha-mode: Alpha mode for the layer, should be one of:
+ - "layer": Alpha is configured layer-wide (C_LAYER_*_ALPHA_MODE == 0).
+ - "pixel": Alpha is configured per-pixel (C_LAYER_*_ALPHA_MODE == 1).
+- xylon,layer-base-offset: offset in number of lines (C_LAYER_*_OFFSET) starting
+  from the video RAM base (C_VMEM_BASEADDR), only for version 3.
+- xylon,layer-buffer-offset: offset in number of lines (C_BUFFER_*_OFFSET)
+  starting from the layer base offset for the second buffer used in
+  double-buffering.
+
+Optional layer properties:
+- xylon,layer-primary: Layer should be registered as a primary plane (exactly
+  one is required).
+
+Example:
+
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		logicvc_cma: cma@1f800000 {
+			compatible = "shared-dma-pool";
+			size = <0x800000>;
+			alloc-ranges = <0x1f800000 0x800000>;
+			reusable;
+		};
+	};
+
+	logicvc: logicvc@43c00000 {
+		compatible = "syscon", "simple-mfd";
+		reg = <0x43c00000 0x6000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		logicvc_display: display-engine@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,syscon = <&logicvc>;
+
+			xylon,display-interface = "lvds-4bits";
+			xylon,display-colorspace = "rgb";
+			xylon,display-depth = <16>;
+			xylon,row-stride = <1024>;
+
+			xylon,layers-configurable;
+			xylon,layers-count = <5>;
+
+			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>;
+					};
+				};
+			};
+		};
+	};