| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200811231238.GA23600@pendragon.ideasonboard.com>
Date: Wed, 12 Aug 2020 02:12:38 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Venkateshwar Rao Gannavarapu
<venkateshwar.rao.gannavarapu@...inx.com>
Cc: hyun.kwon@...inx.com, dri-devel@...ts.freedesktop.org,
airlied@...ux.ie, daniel@...ll.ch, linux-kernel@...r.kernel.org,
sandipk@...inx.com, vgannava@...inx.com
Subject: Re: [RFC PATCH V2 1/2] dt-bindings: display: xlnx: dsi: This add a
DT binding for Xilinx DSI TX subsystem.
Hi GVRao,
Thank you for the patch.
On Tue, Aug 11, 2020 at 06:16:16AM +0530, Venkateshwar Rao Gannavarapu wrote:
> The Xilinx MIPI DSI (Display Serial Interface) Transmitter subsystem
> implements the Mobile Industry Processor Interface (MIPI) based display
> interface. It supports the interface with programmable logic (FPGA).
>
> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@...inx.com>
> ---
> .../devicetree/bindings/display/xlnx/xlnx,dsi.yaml | 147 +++++++++++++++++++++
> 1 file changed, 147 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.yaml
> new file mode 100644
> index 0000000..73da0d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.yaml
> @@ -0,0 +1,147 @@
> +# SPDX-License-Identifier: GPL-2.0
New bindings must be licensed as (GPL-2.0-only OR BSD-2-Clause).
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/xlnx/xlnx,dsi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Programmable DSI-TX Subsystem
> +
> +description: |
> + The programmable MIPI DSI controller of Xilinx implements display pipeline
> + based on DSI v1.3 specification. The subsystem includes multiple functional
> + blocks as below
> +
> + +---------------+ +-----------------------------------------------+
> + | | | +------------------+ |
> + | | | v----------->+AXI CROSBAR |XXX |
> + |FRAME BUFFER | AXI STREAM | | | X X |
> + |(DMA) | +------->+ +------------+ +------------------+ XX |
> + | +<------+ | |MIPI | X |
> + | | | |DSI-TX | X |
> + | | | |Controller | +------------+ |
> + | | | | +------------->D-PHY | |
> + +---------------+ | | | | | |
> + S_AXIS_ACLK | +-------------<------------+ | |
> + +---------------->+ | | |
> + | | | |
> + DPHY_CLK_200M | | | |
> + +---------------->+ | | |
> + + | +------------+ |
> + | |
> + | MIPI DSI TX SUBSYSTEM |
> + +-----------------------------------------------+
> +
> + The DSI TX controller consists of multiple layers such as lane management layer,
> + low-level protocol and pixel-to-byte conversion. The DSI TX controller core
> + receives stream of image data through an input stream interface. The subsystem
> + driver supports upto 4 lane support and generates PPI trasfers towards DPHY
DT bindings shouldn't mention drivers, you can just say "The subsystem
supports up to 4 data lanes ...".
s/trasfers/transfers/
> + with continuous clock. It supports Burst, non-burst modes and command modes.
> +
> +maintainers:
> + - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@...inx.com>
> +
> +properties:
> + compatible:
> + const: xlnx,dsi-1.0
I think calling it just "dsi-1.0" isn't specific enough, as it could
also be a DSI RX. "xlnx,dsi-tx-1.0" would be a better value.
Is this binding for v1.0 of the IP core ? There's a v2.0 too. Only v2.0
supports command mode as far as I can tell, so with the
xlnx,dsi-cmd-mode property below, this seems to match v2.0.
> +
> + reg:
> + maxItems: 1
This should specify in the description if only the DSI TX registers are
covered, or if the D-PHY registers are included too.
> +
> + clocks:
> + description: List of clock specifiers
> + items:
> + - description: AXI Lite clock
> + - description: Video DPHY clock
> +
> + clock-names:
> + items:
> + - const: s_axis_aclk
> + - const: dphy_clk_200M
> +
> + xlnx,dsi-num-lanes:
> + description: Maximum number of lanes that IP configured with.
> + possible values are 1, 2, 4.
You can drop the second sentence as it's implied by the enum below.
> +
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - enum: [1, 2, 4]
Do we need this property ? The protocol configuration register has an
Active Lanes field that reports the number of lanes. Could we read the
information from the register, and drop this property ?
> +
> + xlnx,dsi-data-type:
> + description: MIPI DSI pixel format.
> + possible values are 0, 1, 2, 3.
Same here.
> +
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - enum: [0, 1, 2, 3]
Same comment as above, should this be read from the Pixel Format field
instead of being specified in DT ?
> +
> + xlnx,dsi-cmd-mode:
> + description: denotes command mode support
> +
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - enum: [0, 1]
I don't see a command mode user parameter in the datasheet, it seems
that command mode is a runtime configuration parameter (bit 3 in the
Core Configuration Register). It shouldn't be specified in DT, but
queried at runtime from the connected panel.
> +
> + ports:
> + type: object
> +
> + properties:
> + port@0:
> + type: object
> + description: |
> + output / source port node, endpoint describing modules
> + connected the DSI TX subsystem
> +
> + properties:
> + reg:
> + const: 0
> +
> + endpoint:
> + type: object
> +
> + properties:
> +
> + remote-endpoint: true
> +
> + required:
> + - remote-endpoint
> +
> + additionalProperties: false
> +
> + additionalProperties: false
There should also be a port for the AXI4-Stream interface. The common
practice is to number the input port@0 and the output port@1.
Shouldn't ports specify
required:
- port@0
- port@1
?
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - xlnx,dsi-num-lanes
> + - xlnx,dsi-data-type
> + - xlnx,dsi-cmd-mode
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + mipi_dsi_tx_subsystem@...00000 {
> + compatible = "xlnx,dsi-1.0";
> + reg = <0x0 0x80000000 0x0 0x10000>;
DT examples are compiled in a context where #address-cells and
#size-cells are both set to 1. The reg property should thus be
<0x80000000 0x10000>; in the example, even if it doesn't match the
hardware.
Could you please validate the bindings with
make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.yaml
as explained in Documentation/devicetree/writing-schema.rst ? This will
also compile the example to make sure that everything is right.
> + clocks = <&misc_clk_0>, <&misc_clk_1>;
> + clock-names = "s_axis_aclk", "dphy_clk_200M";
> + xlnx,dsi-num-lanes = <4>;
> + xlnx,dsi-data-type = <1>;
> + xlnx,dsi-cmd-mode = <0>;
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0x0>;
> + dsi_tx_out: endpoint {
> + remote-endpoint = <&panel_in>;
> + };
> + };
> + };
> + };
> +
> +...
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists