Messages in this thread | | | Date | Wed, 12 Aug 2020 02:12:38 +0300 | From | Laurent Pinchart <> | 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@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>; > + }; > + }; > + }; > + }; > + > +...
-- Regards,
Laurent Pinchart
| |