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@xilinx.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@xilinx.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@80000000 {

•  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>;
  

•          };
  

•      };
  

•  };
  

• };

+...