[RFC PATCH 0/2] Add Xilinx DSI-TX DRM driver

[RFC PATCH 0/2] Add Xilinx DSI-TX DRM driver

[Venkateshwar Rao Gannavarapu]

MIPI DSI TX subsystem allows you to quickly create systems based on the
MIPI protocol. It interfaces between the video processing subsystems and
MIPI-based displays. An internal high-speed physical layer design, D-PHY,
is provided to allow direct connection to display peripherals.

The subsystem consists of the following sub-blocks:
- MIPI D-PHY
- MIPI DSI TX Controller
- AXI Crossbar

Please refer pg238 [1].

The DSI TX Controller receives stream of image data through an input stream
interface. At design time, this subsystem can be configured to number of lanes
and command mode support.

This patch series adds the dt-binding and DRM driver for Xilinx DSI-TX soft IP.
patch is created on git://linuxtv.org/pinchartl/media.git drm/dpsub/next

References:
[1]: https://www.xilinx.com/support/documentation/ip_documentation/mipi_dsi_tx_subsystem/v2_0/pg238-mipi-dsi-tx.pdf

Venkateshwar Rao Gannavarapu (2):
  dt-bindings: display: xlnx: Add Xilinx DSI TX subsystem bindings
  drm: xlnx: driver for Xilinx DSI TX Subsystem

 .../devicetree/bindings/display/xlnx/xlnx,dsi.txt  |  68 ++
 drivers/gpu/drm/xlnx/Kconfig                       |  11 +
 drivers/gpu/drm/xlnx/Makefile                      |   2 +
 drivers/gpu/drm/xlnx/xlnx_dsi.c                    | 755 +++++++++++++++++++++
 4 files changed, 836 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
 create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

--
2.7.4

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

[RFC PATCH 1/2] dt-bindings: display: xlnx: Add Xilinx DSI TX subsystem bindings

[Venkateshwar Rao Gannavarapu]

This add a dt binding for Xilinx DSI TX subsystem.

The Xilinx MIPI DSI (Display serial interface) Transmitter Subsystem
implements the Mobile Industry Processor Interface (MIPI) based display
interface. It supports the interface with the programmable logic (FPGA).

Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
---
 .../devicetree/bindings/display/xlnx/xlnx,dsi.txt  | 68 ++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt

diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
new file mode 100644
index 0000000..ef69729
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
@@ -0,0 +1,68 @@
+Device-Tree bindings for Xilinx MIPI DSI Tx IP core
+
+The IP core supports transmission of video data in MIPI DSI protocol.
+
+Required properties:
+ - compatible: Should be "xlnx-dsi".
+ - reg: physical base address and length of the registers set for the device.
+ - xlnx,dsi-num-lanes: Possible number of DSI lanes for the Tx controller.
+   The values should be 1, 2, 3 or 4. Based on xlnx,dsi-num-lanes and
+   line rate for the MIPI D-PHY core in Mbps, the AXI4-stream received by
+   Xilinx MIPI DSI Tx IP core adds markers as per DSI protocol and the packet
+   thus framed is convered to serial data by MIPI D-PHY core. Please refer
+   Xilinx pg238 for more details. This value should be equal to the number
+   of lanes supported by the connected DSI panel. Panel has to support this
+   value or has to be programmed to the same value that DSI Tx controller is
+   configured to.
+ - xlnx,dsi-datatype: Color format. The value should be one of "MIPI_DSI_FMT_RGB888",
+  "MIPI_DSI_FMT_RGB666", "MIPI_DSI_FMT_RGB666_PACKED" or "MIPI_DSI_FMT_RGB565".
+ - #address-cells, #size-cells: should be set respectively to <1> and <0>
+   according to DSI host bindings (see MIPI DSI bindings [1])
+ - clock-names: Must contain "s_axis_aclk" and "dphy_clk_200M" in same order as
+   clocks listed in clocks property.
+ - clocks: List of phandles to Video and 200Mhz DPHY clocks.
+ - port: Logical block can be used / connected independently with
+   external device. In the display controller port nodes, topology
+   for entire pipeline should be described using the DT bindings defined in
+   Documentation/devicetree/bindings/graph.txt.
+ - simple_panel: The subnode for connected panel. This represents the
+   DSI peripheral connected to the DSI host node. Please refer to
+   Documentation/devicetree/bindings/display/mipi-dsi-bus.txt. The
+   simple-panel driver has auo,b101uan01 panel timing parameters added along
+   with other existing panels. DSI driver derive the required Tx IP controller
+   timing values from the panel timing parameters.
+
+Required simple_panel properties:
+ - compatible: Value should be one of the panel names in
+   Documentation/devicetree/bindings/display/panel/. e.g. "auo,b101uan01".
+   For available panel compatible strings, please refer to bindings in
+   Documentation/devicetree/bindings/display/panel/
+
+Optional properties:
+ - xlnx,dsi-cmd-mode: denotes command mode enable.
+
+[1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
+[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+       mipi_dsi_tx_subsystem@80000000 {
+               compatible = "xlnx,dsi";
+               reg = <0x0 0x80000000 0x0 0x10000>;
+               xlnx,dsi-num-lanes = <4>;
+               xlnx,dsi-data-type = <MIPI_DSI_FMT_RGB888>;
+               #address-cells = <1>;
+               #size-cells = <0>;
+               clock-names = "dphy_clk_200M", "s_axis_aclk";
+               clocks = <&misc_clk_0>, <&misc_clk_1>;
+               encoder_dsi_port: port@0 {
+                       reg = <0>;
+                       dsi_encoder: endpoint {
+                               remote-endpoint = <&xdsi_ep>;
+                       };
+               };
+               simple_panel: simple-panel@0 {
+                       compatible = "auo,b101uan01";
+                       reg = <0>;
+                       };
+               };
--
2.7.4

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [RFC PATCH 1/2] dt-bindings: display: xlnx: Add Xilinx DSI TX subsystem bindings

[Sam Ravnborg]

Hi Venkateshwar

On Tue, Apr 21, 2020 at 02:50:55AM +0530, Venkateshwar Rao Gannavarapu wrote:
> This add a dt binding for Xilinx DSI TX subsystem.
> 
> The Xilinx MIPI DSI (Display serial interface) Transmitter Subsystem
> implements the Mobile Industry Processor Interface (MIPI) based display
> interface. It supports the interface with the programmable logic (FPGA).
> 
> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> ---
>  .../devicetree/bindings/display/xlnx/xlnx,dsi.txt  | 68 ++++++++++++++++++++++

Sorry, but new bindings in DT Schema format (.yaml) please.
We are working on migrating all bindings to DT Schema and do not want
to add new bindings in the old format.


>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
> new file mode 100644
> index 0000000..ef69729
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
> @@ -0,0 +1,68 @@
> +Device-Tree bindings for Xilinx MIPI DSI Tx IP core
> +
> +The IP core supports transmission of video data in MIPI DSI protocol.
> +
> +Required properties:
> + - compatible: Should be "xlnx-dsi".
> + - reg: physical base address and length of the registers set for the device.
> + - xlnx,dsi-num-lanes: Possible number of DSI lanes for the Tx controller.
> +   The values should be 1, 2, 3 or 4. Based on xlnx,dsi-num-lanes and
> +   line rate for the MIPI D-PHY core in Mbps, the AXI4-stream received by
> +   Xilinx MIPI DSI Tx IP core adds markers as per DSI protocol and the packet
> +   thus framed is convered to serial data by MIPI D-PHY core. Please refer
> +   Xilinx pg238 for more details. This value should be equal to the number
> +   of lanes supported by the connected DSI panel. Panel has to support this
> +   value or has to be programmed to the same value that DSI Tx controller is
> +   configured to.
> + - xlnx,dsi-datatype: Color format. The value should be one of "MIPI_DSI_FMT_RGB888",
> +  "MIPI_DSI_FMT_RGB666", "MIPI_DSI_FMT_RGB666_PACKED" or "MIPI_DSI_FMT_RGB565".
> + - #address-cells, #size-cells: should be set respectively to <1> and <0>
> +   according to DSI host bindings (see MIPI DSI bindings [1])
> + - clock-names: Must contain "s_axis_aclk" and "dphy_clk_200M" in same order as
> +   clocks listed in clocks property.
> + - clocks: List of phandles to Video and 200Mhz DPHY clocks.
> + - port: Logical block can be used / connected independently with
> +   external device. In the display controller port nodes, topology
> +   for entire pipeline should be described using the DT bindings defined in
> +   Documentation/devicetree/bindings/graph.txt.

> + - simple_panel: The subnode for connected panel. This represents the
> +   DSI peripheral connected to the DSI host node. Please refer to
> +   Documentation/devicetree/bindings/display/mipi-dsi-bus.txt. The
> +   simple-panel driver has auo,b101uan01 panel timing parameters added along
> +   with other existing panels. DSI driver derive the required Tx IP controller
> +   timing values from the panel timing parameters.A
Please always use either a port or a ports node.

	Sam

> +
> +Required simple_panel properties:
> + - compatible: Value should be one of the panel names in
> +   Documentation/devicetree/bindings/display/panel/. e.g. "auo,b101uan01".
> +   For available panel compatible strings, please refer to bindings in
> +   Documentation/devicetree/bindings/display/panel/
> +
> +Optional properties:
> + - xlnx,dsi-cmd-mode: denotes command mode enable.
> +
> +[1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +Example:
> +
> +       mipi_dsi_tx_subsystem@80000000 {
> +               compatible = "xlnx,dsi";
> +               reg = <0x0 0x80000000 0x0 0x10000>;
> +               xlnx,dsi-num-lanes = <4>;
> +               xlnx,dsi-data-type = <MIPI_DSI_FMT_RGB888>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               clock-names = "dphy_clk_200M", "s_axis_aclk";
> +               clocks = <&misc_clk_0>, <&misc_clk_1>;
> +               encoder_dsi_port: port@0 {
> +                       reg = <0>;
> +                       dsi_encoder: endpoint {
> +                               remote-endpoint = <&xdsi_ep>;
> +                       };
> +               };
> +               simple_panel: simple-panel@0 {
> +                       compatible = "auo,b101uan01";
> +                       reg = <0>;
> +                       };
> +               };
> --
> 2.7.4
> 
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [RFC PATCH 1/2] dt-bindings: display: xlnx: Add Xilinx DSI TX subsystem bindings

[Venkateshwar Rao Gannavarapu]

Hi Sam, thanks for your comments.

>-----Original Message-----
>From: Sam Ravnborg <sam@ravnborg.org>
>Sent: Sunday, April 26, 2020 1:59 AM
>To: Venkateshwar Rao Gannavarapu <VGANNAVA@xilinx.com>
>Cc: Hyun Kwon <hyunk@xilinx.com>; laurent.pinchart@ideasonboard.com; dri-
>devel@lists.freedesktop.org; Sandip Kothari <sandipk@xilinx.com>;
>airlied@linux.ie; linux-kernel@vger.kernel.org; Venkateshwar Rao Gannavarapu
><VGANNAVA@xilinx.com>
>Subject: Re: [RFC PATCH 1/2] dt-bindings: display: xlnx: Add Xilinx DSI TX
>subsystem bindings
>
>Hi Venkateshwar
>
>On Tue, Apr 21, 2020 at 02:50:55AM +0530, Venkateshwar Rao Gannavarapu
>wrote:
>> This add a dt binding for Xilinx DSI TX subsystem.
>>
>> The Xilinx MIPI DSI (Display serial interface) Transmitter Subsystem
>> implements the Mobile Industry Processor Interface (MIPI) based
>> display interface. It supports the interface with the programmable logic
>(FPGA).
>>
>> Signed-off-by: Venkateshwar Rao Gannavarapu
>> <venkateshwar.rao.gannavarapu@xilinx.com>
>> ---
>>  .../devicetree/bindings/display/xlnx/xlnx,dsi.txt  | 68
>> ++++++++++++++++++++++
>
>Sorry, but new bindings in DT Schema format (.yaml) please.
>We are working on migrating all bindings to DT Schema and do not want to add
>new bindings in the old format.
>
I will address bindings in YAML format in V2 patch.
>
>>  1 file changed, 68 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
>> b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
>> new file mode 100644
>> index 0000000..ef69729
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
>> @@ -0,0 +1,68 @@
>> +Device-Tree bindings for Xilinx MIPI DSI Tx IP core
>> +
>> +The IP core supports transmission of video data in MIPI DSI protocol.
>> +
>> +Required properties:
>> + - compatible: Should be "xlnx-dsi".
>> + - reg: physical base address and length of the registers set for the device.
>> + - xlnx,dsi-num-lanes: Possible number of DSI lanes for the Tx controller.
>> +   The values should be 1, 2, 3 or 4. Based on xlnx,dsi-num-lanes and
>> +   line rate for the MIPI D-PHY core in Mbps, the AXI4-stream received by
>> +   Xilinx MIPI DSI Tx IP core adds markers as per DSI protocol and the packet
>> +   thus framed is convered to serial data by MIPI D-PHY core. Please refer
>> +   Xilinx pg238 for more details. This value should be equal to the number
>> +   of lanes supported by the connected DSI panel. Panel has to support this
>> +   value or has to be programmed to the same value that DSI Tx controller is
>> +   configured to.
>> + - xlnx,dsi-datatype: Color format. The value should be one of
>> +"MIPI_DSI_FMT_RGB888",
>> +  "MIPI_DSI_FMT_RGB666", "MIPI_DSI_FMT_RGB666_PACKED" or
>"MIPI_DSI_FMT_RGB565".
>> + - #address-cells, #size-cells: should be set respectively to <1> and <0>
>> +   according to DSI host bindings (see MIPI DSI bindings [1])
>> + - clock-names: Must contain "s_axis_aclk" and "dphy_clk_200M" in same
>order as
>> +   clocks listed in clocks property.
>> + - clocks: List of phandles to Video and 200Mhz DPHY clocks.
>> + - port: Logical block can be used / connected independently with
>> +   external device. In the display controller port nodes, topology
>> +   for entire pipeline should be described using the DT bindings defined in
>> +   Documentation/devicetree/bindings/graph.txt.
>
>> + - simple_panel: The subnode for connected panel. This represents the
>> +   DSI peripheral connected to the DSI host node. Please refer to
>> +   Documentation/devicetree/bindings/display/mipi-dsi-bus.txt. The
>> +   simple-panel driver has auo,b101uan01 panel timing parameters added
>along
>> +   with other existing panels. DSI driver derive the required Tx IP controller
>> +   timing values from the panel timing parameters.A
>Please always use either a port or a ports node.
>
OK.
>	Sam
>
>> +
>> +Required simple_panel properties:
>> + - compatible: Value should be one of the panel names in
>> +   Documentation/devicetree/bindings/display/panel/. e.g. "auo,b101uan01".
>> +   For available panel compatible strings, please refer to bindings in
>> +   Documentation/devicetree/bindings/display/panel/
>> +
>> +Optional properties:
>> + - xlnx,dsi-cmd-mode: denotes command mode enable.
>> +
>> +[1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
>> +
>> +Example:
>> +
>> +       mipi_dsi_tx_subsystem@80000000 {
>> +               compatible = "xlnx,dsi";
>> +               reg = <0x0 0x80000000 0x0 0x10000>;
>> +               xlnx,dsi-num-lanes = <4>;
>> +               xlnx,dsi-data-type = <MIPI_DSI_FMT_RGB888>;
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               clock-names = "dphy_clk_200M", "s_axis_aclk";
>> +               clocks = <&misc_clk_0>, <&misc_clk_1>;
>> +               encoder_dsi_port: port@0 {
>> +                       reg = <0>;
>> +                       dsi_encoder: endpoint {
>> +                               remote-endpoint = <&xdsi_ep>;
>> +                       };
>> +               };
>> +               simple_panel: simple-panel@0 {
>> +                       compatible = "auo,b101uan01";
>> +                       reg = <0>;
>> +                       };
>> +               };
>> --
>> 2.7.4
>>
>> This email and any attachments are intended for the sole use of the named
>recipient(s) and contain(s) confidential information that may be proprietary,
>privileged or copyrighted under applicable law. If you are not the intended
>recipient, do not read, copy, or forward this email message or any attachments.
>Delete this email message and any attachments immediately.
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/2] dt-bindings: display: xlnx: Add Xilinx DSI TX subsystem bindings

[Laurent Pinchart]

Hi GVRao,

On Sat, Apr 25, 2020 at 10:29:19PM +0200, Sam Ravnborg wrote:
> On Tue, Apr 21, 2020 at 02:50:55AM +0530, Venkateshwar Rao Gannavarapu wrote:
> > This add a dt binding for Xilinx DSI TX subsystem.
> > 
> > The Xilinx MIPI DSI (Display serial interface) Transmitter Subsystem
> > implements the Mobile Industry Processor Interface (MIPI) based display
> > interface. It supports the interface with the programmable logic (FPGA).
> > 
> > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > ---
> >  .../devicetree/bindings/display/xlnx/xlnx,dsi.txt  | 68 ++++++++++++++++++++++
> 
> Sorry, but new bindings in DT Schema format (.yaml) please.
> We are working on migrating all bindings to DT Schema and do not want
> to add new bindings in the old format.
> 
> >  1 file changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
> > new file mode 100644
> > index 0000000..ef69729
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt
> > @@ -0,0 +1,68 @@
> > +Device-Tree bindings for Xilinx MIPI DSI Tx IP core
> > +
> > +The IP core supports transmission of video data in MIPI DSI protocol.
> > +
> > +Required properties:
> > + - compatible: Should be "xlnx-dsi".

The compatible value should start with the vendor name, followed by a
comma, and then the device identifier. This should thus be "xlnx,dsi".
However, I think calling it just "dsi" isn't specific enough, as it
could also be a DSI RX. "xlnx,dsi-tx" would be a better value. As the IP
core is versioned, I would make it "xlnx,dsi-tx-v2.0" (assuming that's
the version you want to support). If you want to support v1.0 as well,
you can add "xlnx,dsi-tx-v1.0" as an option.

> > + - reg: physical base address and length of the registers set for the device.

Does this cover both the DSI TX registers and the D-PHY registers, or
only the DSI TX registers ? It should be specified.

> > + - xlnx,dsi-num-lanes: Possible number of DSI lanes for the Tx controller.
> > +   The values should be 1, 2, 3 or 4. Based on xlnx,dsi-num-lanes and
> > +   line rate for the MIPI D-PHY core in Mbps, the AXI4-stream received by
> > +   Xilinx MIPI DSI Tx IP core adds markers as per DSI protocol and the packet
> > +   thus framed is convered to serial data by MIPI D-PHY core. Please refer
> > +   Xilinx pg238 for more details. This value should be equal to the number
> > +   of lanes supported by the connected DSI panel. Panel has to support this
> > +   value or has to be programmed to the same value that DSI Tx controller is
> > +   configured to.

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-datatype: Color format. The value should be one of "MIPI_DSI_FMT_RGB888",
> > +  "MIPI_DSI_FMT_RGB666", "MIPI_DSI_FMT_RGB666_PACKED" or "MIPI_DSI_FMT_RGB565".

The example below (and the driver) use "xlnx,dsi-data-type".

Same comment as above, should this be read from the Pixel Format field
instead of being specified in DT ?

> > + - #address-cells, #size-cells: should be set respectively to <1> and <0>
> > +   according to DSI host bindings (see MIPI DSI bindings [1])
> > + - clock-names: Must contain "s_axis_aclk" and "dphy_clk_200M" in same order as
> > +   clocks listed in clocks property.
> > + - clocks: List of phandles to Video and 200Mhz DPHY clocks.
> > + - port: Logical block can be used / connected independently with
> > +   external device. In the display controller port nodes, topology
> > +   for entire pipeline should be described using the DT bindings defined in
> > +   Documentation/devicetree/bindings/graph.txt.

I think you should put the "port" node in a "ports" node, as there's
also a panel subnode. Otherwise both the port and panel will compete for
#address-cells and #size-cells. It happens that both need those
properties to be 1 and 0 respectively, but isolating the two would be
cleaner.

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.

> > + - simple_panel: The subnode for connected panel. This represents the

This should be panel, not simple_panel.

> > +   DSI peripheral connected to the DSI host node. Please refer to
> > +   Documentation/devicetree/bindings/display/mipi-dsi-bus.txt. The
> > +   simple-panel driver has auo,b101uan01 panel timing parameters added along
> > +   with other existing panels. DSI driver derive the required Tx IP controller
> > +   timing values from the panel timing parameters.

You can drop the last two sentences, DT bindings shouldn't mention
driver-specific implementations.

> Please always use either a port or a ports node.
> 
> > +
> > +Required simple_panel properties:
> > + - compatible: Value should be one of the panel names in
> > +   Documentation/devicetree/bindings/display/panel/. e.g. "auo,b101uan01".
> > +   For available panel compatible strings, please refer to bindings in
> > +   Documentation/devicetree/bindings/display/panel/

This should be dropped too, it's out of scope.

> > +
> > +Optional properties:
> > + - xlnx,dsi-cmd-mode: denotes command mode enable.

I think this should be queried at runtime from the panel (if I'm not
mistaken it's reported through the mode_flags field of struct
mipi_dsi_device), and not specified in DT.

> > +
> > +[1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> > +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > +Example:
> > +
> > +       mipi_dsi_tx_subsystem@80000000 {
> > +               compatible = "xlnx,dsi";
> > +               reg = <0x0 0x80000000 0x0 0x10000>;

DT example nodes, when using YAML, are put in a bus node that has
#address-cells and #size-cells both set to 1, so this needs to be

               reg = <0x80000000 0x10000>;

> > +               xlnx,dsi-num-lanes = <4>;
> > +               xlnx,dsi-data-type = <MIPI_DSI_FMT_RGB888>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +               clock-names = "dphy_clk_200M", "s_axis_aclk";
> > +               clocks = <&misc_clk_0>, <&misc_clk_1>;
> > +               encoder_dsi_port: port@0 {
> > +                       reg = <0>;
> > +                       dsi_encoder: endpoint {
> > +                               remote-endpoint = <&xdsi_ep>;
> > +                       };
> > +               };
> > +               simple_panel: simple-panel@0 {

You can drop unused labels.

> > +                       compatible = "auo,b101uan01";
> > +                       reg = <0>;
> > +                       };

This line doesn't appear to be needed.

> > +               };

-- 
Regards,

Laurent Pinchart

[RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Venkateshwar Rao Gannavarapu]

The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
data from AXI-4 stream interface.

It supports upto 4 lanes, optional register interface for the DPHY,
multiple RGB color formats, command mode and video mode.
This is a MIPI-DSI host driver and provides DSI bus for panels.
This driver also helps to communicate with its panel using panel
framework.

Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
---
 drivers/gpu/drm/xlnx/Kconfig    |  11 +
 drivers/gpu/drm/xlnx/Makefile   |   2 +
 drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 768 insertions(+)
 create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
index aa6cd88..73873cf 100644
--- a/drivers/gpu/drm/xlnx/Kconfig
+++ b/drivers/gpu/drm/xlnx/Kconfig
@@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
          This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
          this option if you have a Xilinx ZynqMP SoC with DisplayPort
          subsystem.
+
+config DRM_XLNX_DSI
+        tristate "Xilinx DRM DSI Subsystem Driver"
+        select DRM_MIPI_DSI
+        select DRM_PANEL
+        select DRM_PANEL_SIMPLE
+        help
+         This enables support for Xilinx MIPI-DSI.
+         This is a DRM/KMS driver for Xilinx programmable DSI controller.
+         Choose this option if you have a Xilinx MIPI DSI-TX controller
+         subsytem.
diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
index 2b844c6..b7ee6ef 100644
--- a/drivers/gpu/drm/xlnx/Makefile
+++ b/drivers/gpu/drm/xlnx/Makefile
@@ -1,2 +1,4 @@
 zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
 obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
+
+obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
new file mode 100644
index 0000000..b8cae59
--- /dev/null
+++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
@@ -0,0 +1,755 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx FPGA MIPI DSI Tx Controller driver
+ *
+ * Copyright (C) 2017 - 2019 Xilinx, Inc.
+ *
+ * Authors:
+ * - Saurabh Sengar <saurabhs@xilinx.com>
+ * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_device.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/device.h>
+#include <linux/iopoll.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/phy/phy.h>
+
+#include <video/mipi_display.h>
+#include <video/videomode.h>
+
+/* DSI Tx IP registers */
+#define XDSI_CCR                       0x00
+#define XDSI_CCR_COREENB               BIT(0)
+#define XDSI_CCR_SOFTRST               BIT(1)
+#define XDSI_CCR_CRREADY               BIT(2)
+#define XDSI_CCR_CMDMODE               BIT(3)
+#define XDSI_CCR_DFIFORST              BIT(4)
+#define XDSI_CCR_CMDFIFORST            BIT(5)
+#define XDSI_PCR                       0x04
+#define XDSI_PCR_VIDEOMODE(x)          (((x) & 0x3) << 3)
+#define XDSI_PCR_VIDEOMODE_MASK                (0x3 << 3)
+#define XDSI_PCR_VIDEOMODE_SHIFT       3
+#define XDSI_PCR_BLLPTYPE(x)           ((x) << 5)
+#define XDSI_PCR_BLLPMODE(x)           ((x) << 6)
+#define XDSI_PCR_EOTPENABLE(x)         ((x) << 13)
+#define XDSI_GIER                      0x20
+#define XDSI_ISR                       0x24
+#define XDSI_IER                       0x28
+#define XDSI_STR                       0x2C
+#define XDSI_STR_RDY_SHPKT             BIT(6)
+#define XDSI_STR_RDY_LNGPKT            BIT(7)
+#define XDSI_STR_DFIFO_FULL            BIT(8)
+#define XDSI_STR_DFIFO_EMPTY           BIT(9)
+#define XDSI_STR_WAITFR_DATA           BIT(10)
+#define XDSI_STR_CMD_EXE_PGS           BIT(11)
+#define XDSI_STR_CCMD_PROC             BIT(12)
+#define XDSI_STR_LPKT_MASK             (0x5 << 7)
+#define XDSI_CMD                       0x30
+#define XDSI_CMD_QUEUE_PACKET(x)       ((x) & GENMASK(23, 0))
+#define XDSI_DFR                       0x34
+#define XDSI_TIME1                     0x50
+#define XDSI_TIME1_BLLP_BURST(x)       ((x) & GENMASK(15, 0))
+#define XDSI_TIME1_HSA(x)              (((x) & GENMASK(15, 0)) << 16)
+#define XDSI_TIME2                     0x54
+#define XDSI_TIME2_VACT(x)             ((x) & GENMASK(15, 0))
+#define XDSI_TIME2_HACT(x)             (((x) & GENMASK(15, 0)) << 16)
+#define XDSI_HACT_MULTIPLIER           GENMASK(1, 0)
+#define XDSI_TIME3                     0x58
+#define XDSI_TIME3_HFP(x)              ((x) & GENMASK(15, 0))
+#define XDSI_TIME3_HBP(x)              (((x) & GENMASK(15, 0)) << 16)
+#define XDSI_TIME4                     0x5c
+#define XDSI_TIME4_VFP(x)              ((x) & GENMASK(7, 0))
+#define XDSI_TIME4_VBP(x)              (((x) & GENMASK(7, 0)) << 8)
+#define XDSI_TIME4_VSA(x)              (((x) & GENMASK(7, 0)) << 16)
+#define XDSI_LTIME                     0x60
+#define XDSI_BLLP_TIME                 0x64
+/*
+ * XDSI_NUM_DATA_T represents number of data types in the
+ * enum mipi_dsi_pixel_format in the MIPI DSI part of DRM framework.
+ */
+#define XDSI_NUM_DATA_T                        4
+#define XDSI_VIDEO_MODE_SYNC_PULSE     0x0
+#define XDSI_VIDEO_MODE_SYNC_EVENT     0x1
+#define XDSI_VIDEO_MODE_BURST          0x2
+
+#define XDSI_DPHY_CLK_MIN      197000000000UL
+#define XDSI_DPHY_CLK_MAX      203000000000UL
+#define XDSI_DPHY_CLK_REQ      200000000000UL
+
+/* command timeout in usec */
+#define XDSI_CMD_TIMEOUT_VAL   (3000)
+
+/**
+ * struct xlnx_dsi - Xilinx DSI-TX core
+ * @encoder: DRM encoder structure
+ * @dsi_host: DSI host device
+ * @connector: DRM connector structure
+ * @panel_node: MIPI DSI device panel node
+ * @panel:  DRM panel structure
+ * @dev: device structure
+ * @iomem: Base address of DSI subsystem
+ * @lanes: number of active data lanes supported by DSI controller
+ * @cmdmode: command mode support
+ * @mode_flags: DSI operation mode related flags
+ * @format: pixel format for video mode of DSI controller
+ * @vm: videomode data structure
+ * @mul_factor: multiplication factor for HACT timing parameter
+ * @video_aclk: Video clock
+ * @dphy_clk_200M: 200MHz DPHY clock and AXI Lite clock
+ */
+struct xlnx_dsi {
+       struct drm_encoder encoder;
+       struct mipi_dsi_host dsi_host;
+       struct drm_connector connector;
+       struct device_node *panel_node;
+       struct drm_panel *panel;
+       struct device *dev;
+       void __iomem *iomem;
+       u32 lanes;
+       bool cmdmode;
+       u32 mode_flags;
+       enum mipi_dsi_pixel_format format;
+       struct videomode vm;
+       u32 mul_factor;
+       struct clk *video_aclk;
+       struct clk *dphy_clk_200M;
+};
+
+#define host_to_dsi(host) container_of(host, struct xlnx_dsi, dsi_host)
+#define connector_to_dsi(c) container_of(c, struct xlnx_dsi, connector)
+#define encoder_to_dsi(e) container_of(e, struct xlnx_dsi, encoder)
+
+static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val)
+{
+       writel(val, base + offset);
+}
+
+static inline u32 xlnx_dsi_readl(void __iomem *base, int offset)
+{
+       return readl(base + offset);
+}
+
+/**
+ * xlnx_dsi_set_display_mode - Configure DSI timing registers
+ * @dsi: xilinx DSI structure
+ *
+ * This function writes the timing parameters of DSI IP which are
+ * retrieved from panel timing values.
+ */
+static void xlnx_dsi_set_display_mode(struct xlnx_dsi *dsi)
+{
+       struct videomode *vm = &dsi->vm;
+       u32 reg, video_mode;
+
+       reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
+       video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >>
+                     XDSI_PCR_VIDEOMODE_SHIFT;
+
+       /* configure the HSA value only if non_burst_sync_pluse video mode */
+       if (!video_mode &&
+           (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
+               reg = XDSI_TIME1_HSA(vm->hsync_len);
+               xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
+       }
+
+       reg = XDSI_TIME4_VFP(vm->vfront_porch) |
+             XDSI_TIME4_VBP(vm->vback_porch) |
+             XDSI_TIME4_VSA(vm->vsync_len);
+       xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
+
+       reg = XDSI_TIME3_HFP(vm->hfront_porch) |
+             XDSI_TIME3_HBP(vm->hback_porch);
+       xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
+
+       dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
+               (dsi->mul_factor) / 100);
+       /*
+        * The HACT parameter received from panel timing values should be
+        * divisible by 4. The reason for this is, the word count given as
+        * input to DSI controller is HACT * mul_factor. The mul_factor is
+        * 3, 2.25, 2.25, 2 respectively for RGB888, RGB666_L, RGB666_P and
+        * RGB565.
+        * e.g. for RGB666_L color format and 1080p, the word count is
+        * 1920*2.25 = 4320 which is divisible by 4 and it is a valid input
+        * to DSI controller. Based on this 2.25 mul factor, we come up with
+        * the division factor of (XDSI_HACT_MULTIPLIER) as 4 for checking
+        */
+       if ((vm->hactive & XDSI_HACT_MULTIPLIER) != 0)
+               dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
+
+       reg = XDSI_TIME2_HACT((vm->hactive) * (dsi->mul_factor) / 100) |
+             XDSI_TIME2_VACT(vm->vactive);
+       xlnx_dsi_writel(dsi->iomem, XDSI_TIME2, reg);
+
+       dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive);
+}
+
+/**
+ * xlnx_dsi_set_display_enable - Enables the DSI Tx IP core enable
+ * register bit
+ * @dsi: Xilinx DSI structure
+ *
+ * This function takes the DSI strucure and enables the core enable bit
+ * of core configuration register.
+ */
+static void xlnx_dsi_set_display_enable(struct xlnx_dsi *dsi)
+{
+       u32 reg;
+
+       reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
+       reg |= XDSI_CCR_COREENB;
+
+       xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
+       dev_dbg(dsi->dev, "MIPI DSI Tx controller is enabled.\n");
+}
+
+/**
+ * xlnx_dsi_set_display_disable - Disable the DSI Tx IP core enable
+ * register bit
+ * @dsi: Xilinx DSI structure
+ *
+ * This function takes the DSI strucure and disables the core enable bit
+ * of core configuration register.
+ */
+static void xlnx_dsi_set_display_disable(struct xlnx_dsi *dsi)
+{
+       u32 reg;
+
+       reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
+       reg &= ~XDSI_CCR_COREENB;
+
+       xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
+       dev_dbg(dsi->dev, "DSI Tx is disabled. reset regs to default values\n");
+}
+
+/**
+ * xlnx_dsi_host_transfer - transfer command to panel
+ * @host: mipi dsi host structure
+ * @msg: mipi dsi msg with type, length and data
+ *
+ * This function is valid only in command mode.
+ * It checks the command fifo empty status and writes into
+ * data or cmd register and waits for the completion status.
+ *
+ * Return:     number of bytes, on success and error number on failure
+ */
+static ssize_t xlnx_dsi_host_transfer(struct mipi_dsi_host *host,
+                                     const struct mipi_dsi_msg *msg)
+{
+       struct xlnx_dsi *dsi = host_to_dsi(host);
+       u32 data0, data1, cmd0, status, val;
+       const char *tx_buf = msg->tx_buf;
+
+       if (!(xlnx_dsi_readl(dsi->iomem, XDSI_CCR) & (XDSI_CCR_COREENB |
+                                                     XDSI_CCR_CMDMODE))) {
+               dev_err(dsi->dev, "dsi command mode not enabled\n");
+               return -EINVAL;
+       }
+
+       if (msg->type == MIPI_DSI_DCS_LONG_WRITE) {
+               status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
+                                           ((val & XDSI_STR_LPKT_MASK) ==
+                                            XDSI_STR_LPKT_MASK), 1,
+                                           XDSI_CMD_TIMEOUT_VAL);
+               if (status) {
+                       dev_err(dsi->dev, "long cmd fifo not empty!\n");
+                       return -ETIMEDOUT;
+               }
+               data0 = tx_buf[0] | (tx_buf[1] << 8) | (tx_buf[2] << 16) |
+                       (tx_buf[3] << 24);
+               data1 = tx_buf[4] | (tx_buf[5] << 8);
+               cmd0 = msg->type | (MIPI_DSI_DCS_READ << 8);
+
+               xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data0);
+               xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data1);
+               xlnx_dsi_writel(dsi->iomem, XDSI_CMD, cmd0);
+       } else {
+               data0 = tx_buf[0];
+               if (msg->type == MIPI_DSI_DCS_SHORT_WRITE_PARAM)
+                       data0 = MIPI_DSI_DCS_SHORT_WRITE_PARAM |
+                               (tx_buf[0] << 8) | (tx_buf[1] << 16);
+               else
+                       data0 = MIPI_DSI_DCS_SHORT_WRITE | (tx_buf[0] << 8);
+
+               status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
+                                           ((val & XDSI_STR_RDY_SHPKT) ==
+                                            XDSI_STR_RDY_SHPKT), 1,
+                                           XDSI_CMD_TIMEOUT_VAL);
+               if (status) {
+                       dev_err(dsi->dev, "short cmd fifo not empty\n");
+                       return -ETIMEDOUT;
+               }
+               xlnx_dsi_writel(dsi->iomem, XDSI_CMD, data0);
+       }
+
+       status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
+                                   (!(val & XDSI_STR_CMD_EXE_PGS)), 1,
+                                   XDSI_CMD_TIMEOUT_VAL);
+       if (status) {
+               dev_err(dsi->dev, "cmd time out\n");
+               return -ETIMEDOUT;
+       }
+
+       return msg->tx_len;
+}
+
+static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
+                               struct mipi_dsi_device *device)
+{
+       u32 panel_lanes;
+       struct xlnx_dsi *dsi = host_to_dsi(host);
+
+       panel_lanes = device->lanes;
+       dsi->mode_flags = device->mode_flags;
+       dsi->panel_node = device->dev.of_node;
+
+       if (panel_lanes != dsi->lanes) {
+               dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
+                       panel_lanes, dsi->lanes);
+               return -EINVAL;
+       }
+
+       if (device->format != dsi->format) {
+               dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
+                       device->format, dsi->format);
+               return -EINVAL;
+       }
+
+       if (dsi->connector.dev)
+               drm_helper_hpd_irq_event(dsi->connector.dev);
+
+       return 0;
+}
+
+static int xlnx_dsi_host_detach(struct mipi_dsi_host *host,
+                               struct mipi_dsi_device *device)
+{
+       struct xlnx_dsi *dsi = host_to_dsi(host);
+
+       if (dsi->panel) {
+               drm_panel_detach(dsi->panel);
+               dsi->panel = NULL;
+       }
+
+       if (dsi->connector.dev)
+               drm_helper_hpd_irq_event(dsi->connector.dev);
+
+       return 0;
+}
+
+static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
+       .attach = xlnx_dsi_host_attach,
+       .detach = xlnx_dsi_host_detach,
+       .transfer = xlnx_dsi_host_transfer,
+};
+
+static int xlnx_dsi_connector_dpms(struct drm_connector *connector,
+                                  int mode)
+{
+       struct xlnx_dsi *dsi = connector_to_dsi(connector);
+       int ret;
+
+       dev_dbg(dsi->dev, "connector dpms state: %d\n", mode);
+
+       switch (mode) {
+       case DRM_MODE_DPMS_ON:
+               ret = drm_panel_prepare(dsi->panel);
+               if (ret < 0) {
+                       dev_err(dsi->dev, "DRM panel not found\n");
+                       return ret;
+               }
+
+               ret = drm_panel_enable(dsi->panel);
+               if (ret < 0) {
+                       drm_panel_unprepare(dsi->panel);
+                       dev_err(dsi->dev, "DRM panel not enabled\n");
+                       return ret;
+               }
+               break;
+       default:
+               drm_panel_disable(dsi->panel);
+               drm_panel_unprepare(dsi->panel);
+               break;
+       }
+
+       return drm_helper_connector_dpms(connector, mode);
+}
+
+static enum drm_connector_status
+xlnx_dsi_detect(struct drm_connector *connector, bool force)
+{
+       struct xlnx_dsi *dsi = connector_to_dsi(connector);
+
+       if (!dsi->panel) {
+               dsi->panel = of_drm_find_panel(dsi->panel_node);
+               if (dsi->panel) {
+                       drm_panel_attach(dsi->panel, &dsi->connector);
+                       if (dsi->cmdmode) {
+                               xlnx_dsi_writel(dsi->iomem, XDSI_CCR,
+                                               XDSI_CCR_CMDMODE |
+                                               XDSI_CCR_COREENB);
+                               drm_panel_prepare(dsi->panel);
+                               xlnx_dsi_writel(dsi->iomem, XDSI_CCR, 0);
+                       }
+               }
+       } else if (!dsi->panel_node) {
+               xlnx_dsi_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+               drm_panel_detach(dsi->panel);
+               dsi->panel = NULL;
+       }
+
+       if (dsi->panel)
+               return connector_status_connected;
+
+       return connector_status_disconnected;
+}
+
+static void xlnx_dsi_connector_destroy(struct drm_connector *connector)
+{
+       drm_connector_unregister(connector);
+       drm_connector_cleanup(connector);
+       connector->dev = NULL;
+}
+
+static const struct drm_connector_funcs xlnx_dsi_connector_funcs = {
+       .detect = xlnx_dsi_detect,
+       .fill_modes = drm_helper_probe_single_connector_modes,
+       .destroy = xlnx_dsi_connector_destroy,
+       .reset                  = drm_atomic_helper_connector_reset,
+       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+       .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
+       .dpms = xlnx_dsi_connector_dpms,
+};
+
+static int xlnx_dsi_get_modes(struct drm_connector *connector)
+{
+       struct xlnx_dsi *dsi = connector_to_dsi(connector);
+
+       if (dsi->panel)
+               return drm_panel_get_modes(dsi->panel, connector);
+
+       return 0;
+}
+
+static struct drm_encoder *
+xlnx_dsi_best_encoder(struct drm_connector *connector)
+{
+       return &(connector_to_dsi(connector)->encoder);
+}
+
+static const struct drm_connector_helper_funcs
+xlnx_dsi_connector_helper_funcs = {
+       .get_modes = xlnx_dsi_get_modes,
+       .best_encoder = xlnx_dsi_best_encoder,
+};
+
+static int xlnx_dsi_create_connector(struct drm_encoder *encoder)
+{
+       struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
+       struct drm_connector *connector = &dsi->connector;
+       struct drm_device *drm = encoder->dev;
+       int ret;
+
+       connector->polled = DRM_CONNECTOR_POLL_HPD;
+
+       ret = drm_connector_init(encoder->dev, connector,
+                                &xlnx_dsi_connector_funcs,
+                                DRM_MODE_CONNECTOR_DSI);
+       if (ret) {
+               dev_err(dsi->dev, "Failed to initialize connector with drm\n");
+               return ret;
+       }
+
+       drm_connector_helper_add(connector, &xlnx_dsi_connector_helper_funcs);
+       drm_connector_attach_encoder(connector, encoder);
+       if (!drm->registered)
+               return 0;
+
+       drm_connector_register(connector);
+
+       return 0;
+}
+
+/**
+ * xlnx_dsi_atomic_mode_set -  derive the DSI timing parameters
+ *
+ * @encoder: pointer to Xilinx DRM encoder
+ * @crtc_state: Pointer to drm core crtc state
+ * @connector_state: DSI connector drm state
+ *
+ * This function derives the DSI IP timing parameters from the timing
+ * values given in the attached panel driver.
+ */
+static void
+xlnx_dsi_atomic_mode_set(struct drm_encoder *encoder,
+                        struct drm_crtc_state *crtc_state,
+                                struct drm_connector_state *connector_state)
+{
+       struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
+       struct videomode *vm = &dsi->vm;
+       struct drm_display_mode *m = &crtc_state->adjusted_mode;
+
+       vm->hactive = m->hdisplay;
+       vm->vactive = m->vdisplay;
+       vm->vfront_porch = m->vsync_start - m->vdisplay;
+       vm->vback_porch = m->vtotal - m->vsync_end;
+       vm->vsync_len = m->vsync_end - m->vsync_start;
+       vm->hfront_porch = m->hsync_start - m->hdisplay;
+       vm->hback_porch = m->htotal - m->hsync_end;
+       vm->hsync_len = m->hsync_end - m->hsync_start;
+       xlnx_dsi_set_display_mode(dsi);
+}
+
+static void xlnx_dsi_disable(struct drm_encoder *encoder)
+{
+       struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
+
+       xlnx_dsi_set_display_disable(dsi);
+}
+
+static void xlnx_dsi_enable(struct drm_encoder *encoder)
+{
+       struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
+
+       xlnx_dsi_set_display_enable(dsi);
+}
+
+static const struct drm_encoder_helper_funcs
+xlnx_dsi_encoder_helper_funcs = {
+       .atomic_mode_set = xlnx_dsi_atomic_mode_set,
+       .enable = xlnx_dsi_enable,
+       .disable = xlnx_dsi_disable,
+};
+
+static const struct drm_encoder_funcs xlnx_dsi_encoder_funcs = {
+       .destroy = drm_encoder_cleanup,
+};
+
+static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi)
+{
+       struct device *dev = dsi->dev;
+       struct device_node *node = dev->of_node;
+       int ret;
+       u32 datatype;
+       static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225, 200};
+
+       dsi->dphy_clk_200M = devm_clk_get(dev, "dphy_clk_200M");
+       if (IS_ERR(dsi->dphy_clk_200M)) {
+               ret = PTR_ERR(dsi->dphy_clk_200M);
+               dev_err(dev, "failed to get dphy_clk_200M %d\n", ret);
+               return ret;
+       }
+
+       dsi->video_aclk = devm_clk_get(dev, "s_axis_aclk");
+       if (IS_ERR(dsi->video_aclk)) {
+               ret = PTR_ERR(dsi->video_aclk);
+               dev_err(dev, "failed to get video_clk %d\n", ret);
+               return ret;
+       }
+
+       ret = of_property_read_u32(node, "xlnx,dsi-num-lanes", &dsi->lanes);
+       if (ret < 0) {
+               dev_err(dsi->dev, "missing xlnx,dsi-num-lanes property\n");
+               return ret;
+       }
+       if (dsi->lanes > 4 || dsi->lanes < 1) {
+               dev_err(dsi->dev, "%d lanes : invalid lanes\n", dsi->lanes);
+               return -EINVAL;
+       }
+       ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);
+       if (ret < 0) {
+               dev_err(dsi->dev, "missing xlnx,dsi-data-type property\n");
+               return ret;
+       }
+       dsi->format = datatype;
+       if (datatype > MIPI_DSI_FMT_RGB565) {
+               dev_err(dsi->dev, "Invalid xlnx,dsi-data-type string\n");
+               return -EINVAL;
+       }
+
+       /*
+        * multiplication factor used for HACT, based on data type.
+        *
+        * e.g. for RGB666_L datatype and 1920x1080 resolution,
+        * the Hact (WC) would be as follows -
+        * 1920 pixels * 18 bits per pixel / 8 bits per byte
+        * = 1920 pixels * 2.25 bytes per pixel = 4320 bytes.
+        *
+        * Data Type - Multiplication factor
+        * RGB888    - 3
+        * RGB666_L  - 2.25
+-       * RGB666_P  - 2.25
+        * RGB565    - 2
+        *
+        * Since the multiplication factor maybe a floating number,
+        * a 100x multiplication factor is used.
+        */
+       dsi->mul_factor = xdsi_mul_fact[datatype];
+
+       dsi->cmdmode = of_property_read_bool(node, "xlnx,dsi-cmd-mode");
+
+       dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
+       dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
+       dev_dbg(dsi->dev, "DSI controller cmd mode = %d\n", dsi->cmdmode);
+
+       return 0;
+}
+
+static int xlnx_dsi_bind(struct device *dev, struct device *master,
+                        void *data)
+{
+       struct xlnx_dsi *dsi = dev_get_drvdata(dev);
+       struct drm_encoder *encoder = &dsi->encoder;
+       struct drm_device *drm_dev = data;
+       int ret;
+
+       drm_encoder_init(drm_dev, encoder, &xlnx_dsi_encoder_funcs,
+                        DRM_MODE_ENCODER_DSI, NULL);
+       drm_encoder_helper_add(encoder, &xlnx_dsi_encoder_helper_funcs);
+
+       encoder->possible_crtcs = 1;
+
+       ret = xlnx_dsi_create_connector(encoder);
+       if (ret) {
+               dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret);
+               drm_encoder_cleanup(encoder);
+               return ret;
+       }
+
+       ret = mipi_dsi_host_register(&dsi->dsi_host);
+       if (ret < 0) {
+               dev_err(dsi->dev, "failed to register DSI host: %d\n", ret);
+               xlnx_dsi_connector_destroy(&dsi->connector);
+               drm_encoder_cleanup(encoder);
+               return ret;
+       }
+
+       return 0;
+}
+
+static void xlnx_dsi_unbind(struct device *dev, struct device *master,
+                           void *data)
+{
+       struct xlnx_dsi *dsi = dev_get_drvdata(dev);
+
+       xlnx_dsi_disable(&dsi->encoder);
+       mipi_dsi_host_unregister(&dsi->dsi_host);
+}
+
+static const struct component_ops xlnx_dsi_component_ops = {
+       .bind   = xlnx_dsi_bind,
+       .unbind = xlnx_dsi_unbind,
+};
+
+static int xlnx_dsi_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct resource *res;
+       struct xlnx_dsi *dsi;
+       int ret;
+       unsigned long rate;
+
+       dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
+       if (!dsi)
+               return -ENOMEM;
+
+       dsi->dsi_host.ops = &xlnx_dsi_ops;
+       dsi->dsi_host.dev = dev;
+       dsi->dev = dev;
+
+       ret = xlnx_dsi_parse_dt(dsi);
+       if (ret)
+               return ret;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       dsi->iomem = devm_ioremap_resource(dev, res);
+       if (IS_ERR(dsi->iomem)) {
+               dev_err(dev, "failed to remap io region\n");
+               return PTR_ERR(dsi->iomem);
+       }
+       platform_set_drvdata(pdev, dsi);
+
+       ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ);
+       if (ret) {
+               dev_err(dev, "failed to set dphy clk rate %d\n", ret);
+               return ret;
+       }
+
+       ret = clk_prepare_enable(dsi->dphy_clk_200M);
+       if (ret) {
+               dev_err(dev, "failed to enable dphy clk %d\n", ret);
+               return ret;
+       }
+
+       rate = clk_get_rate(dsi->dphy_clk_200M);
+       if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) {
+               dev_err(dev, "Error DPHY clock = %lu\n", rate);
+               ret = -EINVAL;
+               goto err_disable_dphy_clk;
+       }
+
+       ret = clk_prepare_enable(dsi->video_aclk);
+       if (ret) {
+               dev_err(dev, "failed to enable video clk %d\n", ret);
+               goto err_disable_dphy_clk;
+       }
+
+       ret = component_add(dev, &xlnx_dsi_component_ops);
+       if (ret < 0)
+               goto err_disable_video_clk;
+
+       return ret;
+
+err_disable_video_clk:
+       clk_disable_unprepare(dsi->video_aclk);
+err_disable_dphy_clk:
+       clk_disable_unprepare(dsi->dphy_clk_200M);
+       return ret;
+}
+
+static int xlnx_dsi_remove(struct platform_device *pdev)
+{
+       struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
+
+       component_del(&pdev->dev, &xlnx_dsi_component_ops);
+       clk_disable_unprepare(dsi->video_aclk);
+       clk_disable_unprepare(dsi->dphy_clk_200M);
+
+       return 0;
+}
+
+static const struct of_device_id xlnx_dsi_of_match[] = {
+       { .compatible = "xlnx-dsi", },
+       { /* end of table */ },
+};
+MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match);
+
+static struct platform_driver dsi_driver = {
+       .probe = xlnx_dsi_probe,
+       .remove = xlnx_dsi_remove,
+       .driver = {
+               .name = "xlnx-dsi",
+               .of_match_table = xlnx_dsi_of_match,
+       },
+};
+
+module_platform_driver(dsi_driver);
+
+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Hyun Kwon]

Hi GVRao,

Thanks for the patch. Sorry for late reply.

On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> data from AXI-4 stream interface.
> 
> It supports upto 4 lanes, optional register interface for the DPHY,

I don't see the register interface for dphy support.

> multiple RGB color formats, command mode and video mode.
> This is a MIPI-DSI host driver and provides DSI bus for panels.
> This driver also helps to communicate with its panel using panel
> framework.
> 
> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> ---
>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
>  drivers/gpu/drm/xlnx/Makefile   |   2 +
>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 768 insertions(+)
>  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
> 
> diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> index aa6cd88..73873cf 100644
> --- a/drivers/gpu/drm/xlnx/Kconfig
> +++ b/drivers/gpu/drm/xlnx/Kconfig
> @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
>  	  This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
>  	  this option if you have a Xilinx ZynqMP SoC with DisplayPort
>  	  subsystem.
> +
> +config DRM_XLNX_DSI
> +        tristate "Xilinx DRM DSI Subsystem Driver"
> +        select DRM_MIPI_DSI
> +        select DRM_PANEL
> +        select DRM_PANEL_SIMPLE
> +        help
> +	  This enables support for Xilinx MIPI-DSI.

This sentence is not needed with below. Could you please rephrase the whole?

> +	  This is a DRM/KMS driver for Xilinx programmable DSI controller.
> +	  Choose this option if you have a Xilinx MIPI DSI-TX controller
> +	  subsytem.

These seem incorrectly indented.

> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> index 2b844c6..b7ee6ef 100644
> --- a/drivers/gpu/drm/xlnx/Makefile
> +++ b/drivers/gpu/drm/xlnx/Makefile
> @@ -1,2 +1,4 @@
>  zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
>  obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
> +
> +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
> diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> new file mode 100644
> index 0000000..b8cae59
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> @@ -0,0 +1,755 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx FPGA MIPI DSI Tx Controller driver
> + *
> + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> + *
> + * Authors:
> + * - Saurabh Sengar <saurabhs@xilinx.com>
> + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/device.h>
> +#include <linux/iopoll.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>
> +#include <linux/phy/phy.h>
> +
> +#include <video/mipi_display.h>
> +#include <video/videomode.h>
> +
> +/* DSI Tx IP registers */
> +#define XDSI_CCR			0x00
> +#define XDSI_CCR_COREENB		BIT(0)
> +#define XDSI_CCR_SOFTRST		BIT(1)
> +#define XDSI_CCR_CRREADY		BIT(2)
> +#define XDSI_CCR_CMDMODE		BIT(3)
> +#define XDSI_CCR_DFIFORST		BIT(4)
> +#define XDSI_CCR_CMDFIFORST		BIT(5)
> +#define XDSI_PCR			0x04
> +#define XDSI_PCR_VIDEOMODE(x)		(((x) & 0x3) << 3)
> +#define XDSI_PCR_VIDEOMODE_MASK		(0x3 << 3)
> +#define XDSI_PCR_VIDEOMODE_SHIFT	3
> +#define XDSI_PCR_BLLPTYPE(x)		((x) << 5)
> +#define XDSI_PCR_BLLPMODE(x)		((x) << 6)
> +#define XDSI_PCR_EOTPENABLE(x)		((x) << 13)
> +#define XDSI_GIER			0x20
> +#define XDSI_ISR			0x24
> +#define XDSI_IER			0x28
> +#define XDSI_STR			0x2C
> +#define XDSI_STR_RDY_SHPKT		BIT(6)
> +#define XDSI_STR_RDY_LNGPKT		BIT(7)
> +#define XDSI_STR_DFIFO_FULL		BIT(8)
> +#define XDSI_STR_DFIFO_EMPTY		BIT(9)
> +#define XDSI_STR_WAITFR_DATA		BIT(10)
> +#define XDSI_STR_CMD_EXE_PGS		BIT(11)
> +#define XDSI_STR_CCMD_PROC		BIT(12)
> +#define XDSI_STR_LPKT_MASK		(0x5 << 7)
> +#define XDSI_CMD			0x30
> +#define XDSI_CMD_QUEUE_PACKET(x)	((x) & GENMASK(23, 0))
> +#define XDSI_DFR			0x34
> +#define XDSI_TIME1			0x50
> +#define XDSI_TIME1_BLLP_BURST(x)	((x) & GENMASK(15, 0))
> +#define XDSI_TIME1_HSA(x)		(((x) & GENMASK(15, 0)) << 16)
> +#define XDSI_TIME2			0x54
> +#define XDSI_TIME2_VACT(x)		((x) & GENMASK(15, 0))
> +#define XDSI_TIME2_HACT(x)		(((x) & GENMASK(15, 0)) << 16)
> +#define XDSI_HACT_MULTIPLIER		GENMASK(1, 0)
> +#define XDSI_TIME3			0x58
> +#define XDSI_TIME3_HFP(x)		((x) & GENMASK(15, 0))
> +#define XDSI_TIME3_HBP(x)		(((x) & GENMASK(15, 0)) << 16)
> +#define XDSI_TIME4			0x5c
> +#define XDSI_TIME4_VFP(x)		((x) & GENMASK(7, 0))
> +#define XDSI_TIME4_VBP(x)		(((x) & GENMASK(7, 0)) << 8)
> +#define XDSI_TIME4_VSA(x)		(((x) & GENMASK(7, 0)) << 16)
> +#define XDSI_LTIME			0x60
> +#define XDSI_BLLP_TIME			0x64
> +/*
> + * XDSI_NUM_DATA_T represents number of data types in the
> + * enum mipi_dsi_pixel_format in the MIPI DSI part of DRM framework.
> + */
> +#define XDSI_NUM_DATA_T			4
> +#define XDSI_VIDEO_MODE_SYNC_PULSE	0x0
> +#define XDSI_VIDEO_MODE_SYNC_EVENT	0x1
> +#define XDSI_VIDEO_MODE_BURST		0x2

Please remove these unused 3 definitions.

> +
> +#define XDSI_DPHY_CLK_MIN	197000000000UL
> +#define XDSI_DPHY_CLK_MAX	203000000000UL
> +#define XDSI_DPHY_CLK_REQ	200000000000UL
> +
> +/* command timeout in usec */
> +#define XDSI_CMD_TIMEOUT_VAL	(3000)
> +
> +/**
> + * struct xlnx_dsi - Xilinx DSI-TX core
> + * @encoder: DRM encoder structure
> + * @dsi_host: DSI host device
> + * @connector: DRM connector structure
> + * @panel_node: MIPI DSI device panel node
> + * @panel:  DRM panel structure
> + * @dev: device structure
> + * @iomem: Base address of DSI subsystem
> + * @lanes: number of active data lanes supported by DSI controller
> + * @cmdmode: command mode support
> + * @mode_flags: DSI operation mode related flags
> + * @format: pixel format for video mode of DSI controller
> + * @vm: videomode data structure
> + * @mul_factor: multiplication factor for HACT timing parameter
> + * @video_aclk: Video clock
> + * @dphy_clk_200M: 200MHz DPHY clock and AXI Lite clock
> + */
> +struct xlnx_dsi {
> +	struct drm_encoder encoder;
> +	struct mipi_dsi_host dsi_host;
> +	struct drm_connector connector;
> +	struct device_node *panel_node;
> +	struct drm_panel *panel;
> +	struct device *dev;
> +	void __iomem *iomem;
> +	u32 lanes;
> +	bool cmdmode;
> +	u32 mode_flags;
> +	enum mipi_dsi_pixel_format format;
> +	struct videomode vm;
> +	u32 mul_factor;
> +	struct clk *video_aclk;
> +	struct clk *dphy_clk_200M;
> +};
> +
> +#define host_to_dsi(host) container_of(host, struct xlnx_dsi, dsi_host)
> +#define connector_to_dsi(c) container_of(c, struct xlnx_dsi, connector)
> +#define encoder_to_dsi(e) container_of(e, struct xlnx_dsi, encoder)
> +
> +static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val)
> +{
> +	writel(val, base + offset);
> +}
> +
> +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +/**
> + * xlnx_dsi_set_display_mode - Configure DSI timing registers
> + * @dsi: xilinx DSI structure
> + *
> + * This function writes the timing parameters of DSI IP which are
> + * retrieved from panel timing values.
> + */
> +static void xlnx_dsi_set_display_mode(struct xlnx_dsi *dsi)

This function better directly inline to xlnx_dsi_atomic_mode_set(). Then,
the videomode is not really needed. Please remove it and use drm_display_mode.

> +{
> +	struct videomode *vm = &dsi->vm;
> +	u32 reg, video_mode;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >>
> +		      XDSI_PCR_VIDEOMODE_SHIFT;
> +
> +	/* configure the HSA value only if non_burst_sync_pluse video mode */
> +	if (!video_mode &&
> +	    (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
> +		reg = XDSI_TIME1_HSA(vm->hsync_len);
> +		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
> +	}
> +
> +	reg = XDSI_TIME4_VFP(vm->vfront_porch) |
> +	      XDSI_TIME4_VBP(vm->vback_porch) |
> +	      XDSI_TIME4_VSA(vm->vsync_len);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
> +
> +	reg = XDSI_TIME3_HFP(vm->hfront_porch) |
> +	      XDSI_TIME3_HBP(vm->hback_porch);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
> +
> +	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
> +		(dsi->mul_factor) / 100);

Remove unneeded ().

> +	/*
> +	 * The HACT parameter received from panel timing values should be
> +	 * divisible by 4. The reason for this is, the word count given as
> +	 * input to DSI controller is HACT * mul_factor. The mul_factor is
> +	 * 3, 2.25, 2.25, 2 respectively for RGB888, RGB666_L, RGB666_P and
> +	 * RGB565.
> +	 * e.g. for RGB666_L color format and 1080p, the word count is
> +	 * 1920*2.25 = 4320 which is divisible by 4 and it is a valid input
> +	 * to DSI controller. Based on this 2.25 mul factor, we come up with
> +	 * the division factor of (XDSI_HACT_MULTIPLIER) as 4 for checking
> +	 */
> +	if ((vm->hactive & XDSI_HACT_MULTIPLIER) != 0)

if (!(vm->hactive & XDSI_HACT_MULTIPLIER))?

> +		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");

dev_warn() better come with more details. Probably it can mention that
hactive is not aligned along with its value.

> +
> +	reg = XDSI_TIME2_HACT((vm->hactive) * (dsi->mul_factor) / 100) |

Ditto. No ().

> +	      XDSI_TIME2_VACT(vm->vactive);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME2, reg);
> +
> +	dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive);
> +}
> +
> +/**
> + * xlnx_dsi_set_display_enable - Enables the DSI Tx IP core enable
> + * register bit
> + * @dsi: Xilinx DSI structure
> + *
> + * This function takes the DSI strucure and enables the core enable bit
> + * of core configuration register.
> + */
> +static void xlnx_dsi_set_display_enable(struct xlnx_dsi *dsi)
> +{
> +	u32 reg;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
> +	reg |= XDSI_CCR_COREENB;
> +

Nit. It can be less lines by removing this empty line and setting the reg
at declarartion an so on.

> +	xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
> +	dev_dbg(dsi->dev, "MIPI DSI Tx controller is enabled.\n");
> +}
> +
> +/**
> + * xlnx_dsi_set_display_disable - Disable the DSI Tx IP core enable
> + * register bit
> + * @dsi: Xilinx DSI structure
> + *
> + * This function takes the DSI strucure and disables the core enable bit
> + * of core configuration register.
> + */
> +static void xlnx_dsi_set_display_disable(struct xlnx_dsi *dsi)
> +{
> +	u32 reg;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
> +	reg &= ~XDSI_CCR_COREENB;
> +

Ditto.

> +	xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
> +	dev_dbg(dsi->dev, "DSI Tx is disabled. reset regs to default values\n");
> +}
> +
> +/**
> + * xlnx_dsi_host_transfer - transfer command to panel
> + * @host: mipi dsi host structure
> + * @msg: mipi dsi msg with type, length and data
> + *
> + * This function is valid only in command mode.
> + * It checks the command fifo empty status and writes into
> + * data or cmd register and waits for the completion status.
> + *
> + * Return:	number of bytes, on success and error number on failure

Nit. Extra spaces.

> + */
> +static ssize_t xlnx_dsi_host_transfer(struct mipi_dsi_host *host,
> +				      const struct mipi_dsi_msg *msg)
> +{
> +	struct xlnx_dsi *dsi = host_to_dsi(host);
> +	u32 data0, data1, cmd0, status, val;
> +	const char *tx_buf = msg->tx_buf;
> +
> +	if (!(xlnx_dsi_readl(dsi->iomem, XDSI_CCR) & (XDSI_CCR_COREENB |
> +						      XDSI_CCR_CMDMODE))) {

This passes when one of these 2 bits is set, and I don't think it's right.

> +		dev_err(dsi->dev, "dsi command mode not enabled\n");
> +		return -EINVAL;
> +	}
> +
> +	if (msg->type == MIPI_DSI_DCS_LONG_WRITE) {
> +		status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
> +					    ((val & XDSI_STR_LPKT_MASK) ==
> +					     XDSI_STR_LPKT_MASK), 1,

Please remove unneeded (). Same for rest below.

> +					    XDSI_CMD_TIMEOUT_VAL);

It'd be nice if these delay / timeout values are described..

> +		if (status) {
> +			dev_err(dsi->dev, "long cmd fifo not empty!\n");
> +			return -ETIMEDOUT;

Isn't this checking if the controller is ready for long packet transaction?
Then maybe -EBUSY is better. For timeout, the 'status' can propagate.
Same for below.

> +		}
> +		data0 = tx_buf[0] | (tx_buf[1] << 8) | (tx_buf[2] << 16) |
> +			(tx_buf[3] << 24);
> +		data1 = tx_buf[4] | (tx_buf[5] << 8);
> +		cmd0 = msg->type | (MIPI_DSI_DCS_READ << 8);
> +
> +		xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data0);
> +		xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data1);
> +		xlnx_dsi_writel(dsi->iomem, XDSI_CMD, cmd0);
> +	} else {

I'm not familiar with DSI transaction handling, but is it safe to assume
all other than long write is short packet write (param, without param)?

> +		data0 = tx_buf[0];

This line can be removed.

> +		if (msg->type == MIPI_DSI_DCS_SHORT_WRITE_PARAM)
> +			data0 = MIPI_DSI_DCS_SHORT_WRITE_PARAM |
> +				(tx_buf[0] << 8) | (tx_buf[1] << 16);
> +		else
> +			data0 = MIPI_DSI_DCS_SHORT_WRITE | (tx_buf[0] << 8);

I'd put () for this if - else statments, but it may be just me.


Then, this can mvoe to right before xlnx_dsi_writel() below.

> +
> +		status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
> +					    ((val & XDSI_STR_RDY_SHPKT) ==
> +					     XDSI_STR_RDY_SHPKT), 1,
> +					    XDSI_CMD_TIMEOUT_VAL);
> +		if (status) {
> +			dev_err(dsi->dev, "short cmd fifo not empty\n");
> +			return -ETIMEDOUT;
> +		}
> +		xlnx_dsi_writel(dsi->iomem, XDSI_CMD, data0);
> +	}
> +
> +	status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
> +				    (!(val & XDSI_STR_CMD_EXE_PGS)), 1,
> +				    XDSI_CMD_TIMEOUT_VAL);
> +	if (status) {
> +		dev_err(dsi->dev, "cmd time out\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return msg->tx_len;
> +}
> +
> +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
> +				struct mipi_dsi_device *device)
> +{
> +	u32 panel_lanes;
> +	struct xlnx_dsi *dsi = host_to_dsi(host);
> +
> +	panel_lanes = device->lanes;

I'd do this above,

	u32 panel_lanes = device->lanes

> +	dsi->mode_flags = device->mode_flags;
> +	dsi->panel_node = device->dev.of_node;
> +
> +	if (panel_lanes != dsi->lanes) {
> +		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
> +			panel_lanes, dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (device->format != dsi->format) {
> +		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
> +			device->format, dsi->format);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->connector.dev)
> +		drm_helper_hpd_irq_event(dsi->connector.dev);
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_host_detach(struct mipi_dsi_host *host,
> +				struct mipi_dsi_device *device)
> +{
> +	struct xlnx_dsi *dsi = host_to_dsi(host);
> +
> +	if (dsi->panel) {
> +		drm_panel_detach(dsi->panel);
> +		dsi->panel = NULL;
> +	}
> +
> +	if (dsi->connector.dev)
> +		drm_helper_hpd_irq_event(dsi->connector.dev);
> +
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
> +	.attach = xlnx_dsi_host_attach,
> +	.detach = xlnx_dsi_host_detach,
> +	.transfer = xlnx_dsi_host_transfer,
> +};
> +
> +static int xlnx_dsi_connector_dpms(struct drm_connector *connector,
> +				   int mode)
> +{
> +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
> +	int ret;
> +
> +	dev_dbg(dsi->dev, "connector dpms state: %d\n", mode);
> +
> +	switch (mode) {
> +	case DRM_MODE_DPMS_ON:
> +		ret = drm_panel_prepare(dsi->panel);
> +		if (ret < 0) {
> +			dev_err(dsi->dev, "DRM panel not found\n");
> +			return ret;
> +		}
> +
> +		ret = drm_panel_enable(dsi->panel);
> +		if (ret < 0) {
> +			drm_panel_unprepare(dsi->panel);
> +			dev_err(dsi->dev, "DRM panel not enabled\n");
> +			return ret;
> +		}
> +		break;
> +	default:
> +		drm_panel_disable(dsi->panel);
> +		drm_panel_unprepare(dsi->panel);
> +		break;
> +	}
> +
> +	return drm_helper_connector_dpms(connector, mode);
> +}
> +
> +static enum drm_connector_status
> +xlnx_dsi_detect(struct drm_connector *connector, bool force)
> +{
> +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
> +
> +	if (!dsi->panel) {
> +		dsi->panel = of_drm_find_panel(dsi->panel_node);
> +		if (dsi->panel) {
> +			drm_panel_attach(dsi->panel, &dsi->connector);

Above are quite static, so can't it be done in host attach? Or do you see more
dynamic use cases?

> +			if (dsi->cmdmode) {
> +				xlnx_dsi_writel(dsi->iomem, XDSI_CCR,
> +						XDSI_CCR_CMDMODE |
> +						XDSI_CCR_COREENB);
> +				drm_panel_prepare(dsi->panel);

Just to confirm, is this not needed for non-command mode? And how is this
aligned with same call in dpms(), meaning the panel will be prepared
twice in command mode.

> +				xlnx_dsi_writel(dsi->iomem, XDSI_CCR, 0);
> +			}
> +		}
> +	} else if (!dsi->panel_node) {
> +		xlnx_dsi_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> +		drm_panel_detach(dsi->panel);
> +		dsi->panel = NULL;
> +	}
> +
> +	if (dsi->panel)
> +		return connector_status_connected;
> +
> +	return connector_status_disconnected;
> +}
> +
> +static void xlnx_dsi_connector_destroy(struct drm_connector *connector)
> +{
> +	drm_connector_unregister(connector);
> +	drm_connector_cleanup(connector);
> +	connector->dev = NULL;
> +}
> +
> +static const struct drm_connector_funcs xlnx_dsi_connector_funcs = {
> +	.detect = xlnx_dsi_detect,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = xlnx_dsi_connector_destroy,
> +	.reset			= drm_atomic_helper_connector_reset,

This better indent more consistently. Please do it through the entire file.

> +	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
> +	.dpms = xlnx_dsi_connector_dpms,
> +};
> +
> +static int xlnx_dsi_get_modes(struct drm_connector *connector)
> +{
> +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
> +
> +	if (dsi->panel)
> +		return drm_panel_get_modes(dsi->panel, connector);
> +
> +	return 0;
> +}
> +
> +static struct drm_encoder *
> +xlnx_dsi_best_encoder(struct drm_connector *connector)
> +{
> +	return &(connector_to_dsi(connector)->encoder);
> +}
> +
> +static const struct drm_connector_helper_funcs
> +xlnx_dsi_connector_helper_funcs = {
> +	.get_modes = xlnx_dsi_get_modes,
> +	.best_encoder = xlnx_dsi_best_encoder,
> +};
> +
> +static int xlnx_dsi_create_connector(struct drm_encoder *encoder)
> +{
> +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> +	struct drm_connector *connector = &dsi->connector;
> +	struct drm_device *drm = encoder->dev;
> +	int ret;
> +
> +	connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> +	ret = drm_connector_init(encoder->dev, connector,
> +				 &xlnx_dsi_connector_funcs,
> +				 DRM_MODE_CONNECTOR_DSI);
> +	if (ret) {
> +		dev_err(dsi->dev, "Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +
> +	drm_connector_helper_add(connector, &xlnx_dsi_connector_helper_funcs);
> +	drm_connector_attach_encoder(connector, encoder);
> +	if (!drm->registered)
> +		return 0;
> +
> +	drm_connector_register(connector);
> +
> +	return 0;
> +}
> +
> +/**
> + * xlnx_dsi_atomic_mode_set -  derive the DSI timing parameters
> + *
> + * @encoder: pointer to Xilinx DRM encoder
> + * @crtc_state: Pointer to drm core crtc state
> + * @connector_state: DSI connector drm state
> + *
> + * This function derives the DSI IP timing parameters from the timing
> + * values given in the attached panel driver.
> + */
> +static void
> +xlnx_dsi_atomic_mode_set(struct drm_encoder *encoder,
> +			 struct drm_crtc_state *crtc_state,
> +				 struct drm_connector_state *connector_state)
> +{
> +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> +	struct videomode *vm = &dsi->vm;
> +	struct drm_display_mode *m = &crtc_state->adjusted_mode;
> +
> +	vm->hactive = m->hdisplay;
> +	vm->vactive = m->vdisplay;
> +	vm->vfront_porch = m->vsync_start - m->vdisplay;
> +	vm->vback_porch = m->vtotal - m->vsync_end;
> +	vm->vsync_len = m->vsync_end - m->vsync_start;
> +	vm->hfront_porch = m->hsync_start - m->hdisplay;
> +	vm->hback_porch = m->htotal - m->hsync_end;
> +	vm->hsync_len = m->hsync_end - m->hsync_start;
> +	xlnx_dsi_set_display_mode(dsi);

As commented above, this doesn't have to be re-directed through videomode.
Then the function can be inlined here.

> +}
> +
> +static void xlnx_dsi_disable(struct drm_encoder *encoder)
> +{
> +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> +
> +	xlnx_dsi_set_display_disable(dsi);

	xlnx_dsi_set_display_disable(encoder_to_dsi(encoder));

Maybe even this wrapper can be removed?

> +}
> +
> +static void xlnx_dsi_enable(struct drm_encoder *encoder)
> +{
> +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> +
> +	xlnx_dsi_set_display_enable(dsi);

Ditto.

> +}
> +
> +static const struct drm_encoder_helper_funcs
> +xlnx_dsi_encoder_helper_funcs = {
> +	.atomic_mode_set = xlnx_dsi_atomic_mode_set,
> +	.enable = xlnx_dsi_enable,
> +	.disable = xlnx_dsi_disable,
> +};
> +
> +static const struct drm_encoder_funcs xlnx_dsi_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi)
> +{
> +	struct device *dev = dsi->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret;
> +	u32 datatype;
> +	static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225, 200};
> +
> +	dsi->dphy_clk_200M = devm_clk_get(dev, "dphy_clk_200M");
> +	if (IS_ERR(dsi->dphy_clk_200M)) {
> +		ret = PTR_ERR(dsi->dphy_clk_200M);
> +		dev_err(dev, "failed to get dphy_clk_200M %d\n", ret);
> +		return ret;
> +	}
> +
> +	dsi->video_aclk = devm_clk_get(dev, "s_axis_aclk");
> +	if (IS_ERR(dsi->video_aclk)) {
> +		ret = PTR_ERR(dsi->video_aclk);
> +		dev_err(dev, "failed to get video_clk %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "xlnx,dsi-num-lanes", &dsi->lanes);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "missing xlnx,dsi-num-lanes property\n");
> +		return ret;
> +	}
> +	if (dsi->lanes > 4 || dsi->lanes < 1) {
> +		dev_err(dsi->dev, "%d lanes : invalid lanes\n", dsi->lanes);
> +		return -EINVAL;
> +	}

Nit. An empty line here would be helpful for readability.

> +	ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "missing xlnx,dsi-data-type property\n");
> +		return ret;
> +	}
> +	dsi->format = datatype;
> +	if (datatype > MIPI_DSI_FMT_RGB565) {
> +		dev_err(dsi->dev, "Invalid xlnx,dsi-data-type string\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * multiplication factor used for HACT, based on data type.
> +	 *
> +	 * e.g. for RGB666_L datatype and 1920x1080 resolution,
> +	 * the Hact (WC) would be as follows -
> +	 * 1920 pixels * 18 bits per pixel / 8 bits per byte
> +	 * = 1920 pixels * 2.25 bytes per pixel = 4320 bytes.
> +	 *
> +	 * Data Type - Multiplication factor
> +	 * RGB888    - 3
> +	 * RGB666_L  - 2.25
> +-	 * RGB666_P  - 2.25
> +	 * RGB565    - 2
> +	 *
> +	 * Since the multiplication factor maybe a floating number,
> +	 * a 100x multiplication factor is used.
> +	 */
> +	dsi->mul_factor = xdsi_mul_fact[datatype];
> +
> +	dsi->cmdmode = of_property_read_bool(node, "xlnx,dsi-cmd-mode");
> +
> +	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
> +	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
> +	dev_dbg(dsi->dev, "DSI controller cmd mode = %d\n", dsi->cmdmode);
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_bind(struct device *dev, struct device *master,
> +			 void *data)
> +{
> +	struct xlnx_dsi *dsi = dev_get_drvdata(dev);
> +	struct drm_encoder *encoder = &dsi->encoder;
> +	struct drm_device *drm_dev = data;
> +	int ret;
> +
> +	drm_encoder_init(drm_dev, encoder, &xlnx_dsi_encoder_funcs,
> +			 DRM_MODE_ENCODER_DSI, NULL);
> +	drm_encoder_helper_add(encoder, &xlnx_dsi_encoder_helper_funcs);
> +
> +	encoder->possible_crtcs = 1;
> +
> +	ret = xlnx_dsi_create_connector(encoder);
> +	if (ret) {
> +		dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret);
> +		drm_encoder_cleanup(encoder);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> +	if (ret < 0) {

Just nit. if (ret) would do.

> +		dev_err(dsi->dev, "failed to register DSI host: %d\n", ret);
> +		xlnx_dsi_connector_destroy(&dsi->connector);
> +		drm_encoder_cleanup(encoder);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void xlnx_dsi_unbind(struct device *dev, struct device *master,
> +			    void *data)
> +{
> +	struct xlnx_dsi *dsi = dev_get_drvdata(dev);
> +
> +	xlnx_dsi_disable(&dsi->encoder);
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +}
> +
> +static const struct component_ops xlnx_dsi_component_ops = {
> +	.bind	= xlnx_dsi_bind,
> +	.unbind	= xlnx_dsi_unbind,
> +};
> +
> +static int xlnx_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct xlnx_dsi *dsi;
> +	int ret;
> +	unsigned long rate;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dsi_host.ops = &xlnx_dsi_ops;
> +	dsi->dsi_host.dev = dev;
> +	dsi->dev = dev;
> +
> +	ret = xlnx_dsi_parse_dt(dsi);
> +	if (ret)
> +		return ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dsi->iomem = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dsi->iomem)) {
> +		dev_err(dev, "failed to remap io region\n");
> +		return PTR_ERR(dsi->iomem);
> +	}
> +	platform_set_drvdata(pdev, dsi);
> +
> +	ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ);
> +	if (ret) {
> +		dev_err(dev, "failed to set dphy clk rate %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(dsi->dphy_clk_200M);
> +	if (ret) {
> +		dev_err(dev, "failed to enable dphy clk %d\n", ret);
> +		return ret;
> +	}
> +
> +	rate = clk_get_rate(dsi->dphy_clk_200M);
> +	if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) {

A brief comment for this line wold be helpful.

> +		dev_err(dev, "Error DPHY clock = %lu\n", rate);
> +		ret = -EINVAL;
> +		goto err_disable_dphy_clk;
> +	}
> +
> +	ret = clk_prepare_enable(dsi->video_aclk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable video clk %d\n", ret);
> +		goto err_disable_dphy_clk;
> +	}
> +
> +	ret = component_add(dev, &xlnx_dsi_component_ops);
> +	if (ret < 0)

I prefer if (ret) where possible, but it may be just me. I let you decide.

> +		goto err_disable_video_clk;
> +
> +	return ret;

return 0;? but up to you.

> +
> +err_disable_video_clk:
> +	clk_disable_unprepare(dsi->video_aclk);
> +err_disable_dphy_clk:
> +	clk_disable_unprepare(dsi->dphy_clk_200M);
> +	return ret;
> +}

At least, this initial one better come with a complete display pipeline,
ex crtc / plane are missing. This won't be functional without those.
I believe Laurent was planning to make the xlnx drm layer as a library,
so hopefully this can be a good starting point and come with drivers aligned
with it.

> +
> +static int xlnx_dsi_remove(struct platform_device *pdev)
> +{
> +	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
> +
> +	component_del(&pdev->dev, &xlnx_dsi_component_ops);
> +	clk_disable_unprepare(dsi->video_aclk);
> +	clk_disable_unprepare(dsi->dphy_clk_200M);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xlnx_dsi_of_match[] = {
> +	{ .compatible = "xlnx-dsi", },

This should change. I believe it should be something like, xlnx,<dsi ip name>-<version>

Thanks,
-hyun

> +	{ /* end of table */ },
> +};
> +MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match);
> +
> +static struct platform_driver dsi_driver = {
> +	.probe = xlnx_dsi_probe,
> +	.remove = xlnx_dsi_remove,
> +	.driver = {
> +		.name = "xlnx-dsi",
> +		.of_match_table = xlnx_dsi_of_match,
> +	},
> +};
> +
> +module_platform_driver(dsi_driver);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
> 

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Laurent Pinchart]

Hi GVRao,

Thank you for the patch.

On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> > The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> > data from AXI-4 stream interface.
> > 
> > It supports upto 4 lanes, optional register interface for the DPHY,
> 
> I don't see the register interface for dphy support.

I think the D-PHY should be supported through a PHY driver, as it seems
to be shared between different subsystems.

> > multiple RGB color formats, command mode and video mode.
> > This is a MIPI-DSI host driver and provides DSI bus for panels.
> > This driver also helps to communicate with its panel using panel
> > framework.
> > 
> > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > ---
> >  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> >  drivers/gpu/drm/xlnx/Makefile   |   2 +
> >  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++

Daniel Vetter has recently expressed his opiion that bridge drivers
should go to drivers/gpu/drm/bridge/. It would then be
drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.

> >  3 files changed, 768 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
> > 
> > diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> > index aa6cd88..73873cf 100644
> > --- a/drivers/gpu/drm/xlnx/Kconfig
> > +++ b/drivers/gpu/drm/xlnx/Kconfig
> > @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
> >  	  This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
> >  	  this option if you have a Xilinx ZynqMP SoC with DisplayPort
> >  	  subsystem.
> > +
> > +config DRM_XLNX_DSI
> > +        tristate "Xilinx DRM DSI Subsystem Driver"
> > +        select DRM_MIPI_DSI
> > +        select DRM_PANEL
> > +        select DRM_PANEL_SIMPLE
> > +        help
> > +	  This enables support for Xilinx MIPI-DSI.
> 
> This sentence is not needed with below. Could you please rephrase the whole?
> 
> > +	  This is a DRM/KMS driver for Xilinx programmable DSI controller.
> > +	  Choose this option if you have a Xilinx MIPI DSI-TX controller
> > +	  subsytem.
> 
> These seem incorrectly indented.
> 
> > diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> > index 2b844c6..b7ee6ef 100644
> > --- a/drivers/gpu/drm/xlnx/Makefile
> > +++ b/drivers/gpu/drm/xlnx/Makefile
> > @@ -1,2 +1,4 @@
> >  zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
> >  obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
> > +
> > +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
> > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > new file mode 100644
> > index 0000000..b8cae59
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > @@ -0,0 +1,755 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Xilinx FPGA MIPI DSI Tx Controller driver
> > + *
> > + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> > + *
> > + * Authors:
> > + * - Saurabh Sengar <saurabhs@xilinx.com>
> > + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > + */
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_connector.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_encoder.h>
> > +#include <drm/drm_fourcc.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_probe_helper.h>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/component.h>
> > +#include <linux/device.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/phy/phy.h>
> > +
> > +#include <video/mipi_display.h>
> > +#include <video/videomode.h>
> > +
> > +/* DSI Tx IP registers */
> > +#define XDSI_CCR			0x00
> > +#define XDSI_CCR_COREENB		BIT(0)
> > +#define XDSI_CCR_SOFTRST		BIT(1)
> > +#define XDSI_CCR_CRREADY		BIT(2)
> > +#define XDSI_CCR_CMDMODE		BIT(3)
> > +#define XDSI_CCR_DFIFORST		BIT(4)
> > +#define XDSI_CCR_CMDFIFORST		BIT(5)
> > +#define XDSI_PCR			0x04
> > +#define XDSI_PCR_VIDEOMODE(x)		(((x) & 0x3) << 3)
> > +#define XDSI_PCR_VIDEOMODE_MASK		(0x3 << 3)
> > +#define XDSI_PCR_VIDEOMODE_SHIFT	3
> > +#define XDSI_PCR_BLLPTYPE(x)		((x) << 5)
> > +#define XDSI_PCR_BLLPMODE(x)		((x) << 6)
> > +#define XDSI_PCR_EOTPENABLE(x)		((x) << 13)
> > +#define XDSI_GIER			0x20
> > +#define XDSI_ISR			0x24
> > +#define XDSI_IER			0x28
> > +#define XDSI_STR			0x2C
> > +#define XDSI_STR_RDY_SHPKT		BIT(6)
> > +#define XDSI_STR_RDY_LNGPKT		BIT(7)
> > +#define XDSI_STR_DFIFO_FULL		BIT(8)
> > +#define XDSI_STR_DFIFO_EMPTY		BIT(9)
> > +#define XDSI_STR_WAITFR_DATA		BIT(10)
> > +#define XDSI_STR_CMD_EXE_PGS		BIT(11)
> > +#define XDSI_STR_CCMD_PROC		BIT(12)
> > +#define XDSI_STR_LPKT_MASK		(0x5 << 7)
> > +#define XDSI_CMD			0x30
> > +#define XDSI_CMD_QUEUE_PACKET(x)	((x) & GENMASK(23, 0))
> > +#define XDSI_DFR			0x34
> > +#define XDSI_TIME1			0x50
> > +#define XDSI_TIME1_BLLP_BURST(x)	((x) & GENMASK(15, 0))
> > +#define XDSI_TIME1_HSA(x)		(((x) & GENMASK(15, 0)) << 16)
> > +#define XDSI_TIME2			0x54
> > +#define XDSI_TIME2_VACT(x)		((x) & GENMASK(15, 0))
> > +#define XDSI_TIME2_HACT(x)		(((x) & GENMASK(15, 0)) << 16)
> > +#define XDSI_HACT_MULTIPLIER		GENMASK(1, 0)
> > +#define XDSI_TIME3			0x58
> > +#define XDSI_TIME3_HFP(x)		((x) & GENMASK(15, 0))
> > +#define XDSI_TIME3_HBP(x)		(((x) & GENMASK(15, 0)) << 16)
> > +#define XDSI_TIME4			0x5c
> > +#define XDSI_TIME4_VFP(x)		((x) & GENMASK(7, 0))
> > +#define XDSI_TIME4_VBP(x)		(((x) & GENMASK(7, 0)) << 8)
> > +#define XDSI_TIME4_VSA(x)		(((x) & GENMASK(7, 0)) << 16)
> > +#define XDSI_LTIME			0x60
> > +#define XDSI_BLLP_TIME			0x64
> > +/*
> > + * XDSI_NUM_DATA_T represents number of data types in the
> > + * enum mipi_dsi_pixel_format in the MIPI DSI part of DRM framework.
> > + */
> > +#define XDSI_NUM_DATA_T			4
> > +#define XDSI_VIDEO_MODE_SYNC_PULSE	0x0
> > +#define XDSI_VIDEO_MODE_SYNC_EVENT	0x1
> > +#define XDSI_VIDEO_MODE_BURST		0x2
> 
> Please remove these unused 3 definitions.
> 
> > +
> > +#define XDSI_DPHY_CLK_MIN	197000000000UL
> > +#define XDSI_DPHY_CLK_MAX	203000000000UL
> > +#define XDSI_DPHY_CLK_REQ	200000000000UL
> > +
> > +/* command timeout in usec */
> > +#define XDSI_CMD_TIMEOUT_VAL	(3000)
> > +
> > +/**
> > + * struct xlnx_dsi - Xilinx DSI-TX core
> > + * @encoder: DRM encoder structure
> > + * @dsi_host: DSI host device
> > + * @connector: DRM connector structure
> > + * @panel_node: MIPI DSI device panel node
> > + * @panel:  DRM panel structure
> > + * @dev: device structure
> > + * @iomem: Base address of DSI subsystem
> > + * @lanes: number of active data lanes supported by DSI controller
> > + * @cmdmode: command mode support
> > + * @mode_flags: DSI operation mode related flags
> > + * @format: pixel format for video mode of DSI controller
> > + * @vm: videomode data structure
> > + * @mul_factor: multiplication factor for HACT timing parameter
> > + * @video_aclk: Video clock
> > + * @dphy_clk_200M: 200MHz DPHY clock and AXI Lite clock
> > + */
> > +struct xlnx_dsi {
> > +	struct drm_encoder encoder;
> > +	struct mipi_dsi_host dsi_host;
> > +	struct drm_connector connector;
> > +	struct device_node *panel_node;
> > +	struct drm_panel *panel;
> > +	struct device *dev;
> > +	void __iomem *iomem;
> > +	u32 lanes;
> > +	bool cmdmode;
> > +	u32 mode_flags;
> > +	enum mipi_dsi_pixel_format format;
> > +	struct videomode vm;
> > +	u32 mul_factor;
> > +	struct clk *video_aclk;
> > +	struct clk *dphy_clk_200M;
> > +};
> > +
> > +#define host_to_dsi(host) container_of(host, struct xlnx_dsi, dsi_host)
> > +#define connector_to_dsi(c) container_of(c, struct xlnx_dsi, connector)
> > +#define encoder_to_dsi(e) container_of(e, struct xlnx_dsi, encoder)
> > +
> > +static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val)
> > +{
> > +	writel(val, base + offset);
> > +}
> > +
> > +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset)
> > +{
> > +	return readl(base + offset);
> > +}
> > +
> > +/**
> > + * xlnx_dsi_set_display_mode - Configure DSI timing registers
> > + * @dsi: xilinx DSI structure
> > + *
> > + * This function writes the timing parameters of DSI IP which are
> > + * retrieved from panel timing values.
> > + */
> > +static void xlnx_dsi_set_display_mode(struct xlnx_dsi *dsi)
> 
> This function better directly inline to xlnx_dsi_atomic_mode_set(). Then,
> the videomode is not really needed. Please remove it and use drm_display_mode.
> 
> > +{
> > +	struct videomode *vm = &dsi->vm;
> > +	u32 reg, video_mode;
> > +
> > +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> > +	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >>
> > +		      XDSI_PCR_VIDEOMODE_SHIFT;
> > +
> > +	/* configure the HSA value only if non_burst_sync_pluse video mode */
> > +	if (!video_mode &&
> > +	    (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
> > +		reg = XDSI_TIME1_HSA(vm->hsync_len);
> > +		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
> > +	}
> > +
> > +	reg = XDSI_TIME4_VFP(vm->vfront_porch) |
> > +	      XDSI_TIME4_VBP(vm->vback_porch) |
> > +	      XDSI_TIME4_VSA(vm->vsync_len);
> > +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
> > +
> > +	reg = XDSI_TIME3_HFP(vm->hfront_porch) |
> > +	      XDSI_TIME3_HBP(vm->hback_porch);
> > +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
> > +
> > +	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
> > +		(dsi->mul_factor) / 100);
> 
> Remove unneeded ().
> 
> > +	/*
> > +	 * The HACT parameter received from panel timing values should be
> > +	 * divisible by 4. The reason for this is, the word count given as
> > +	 * input to DSI controller is HACT * mul_factor. The mul_factor is
> > +	 * 3, 2.25, 2.25, 2 respectively for RGB888, RGB666_L, RGB666_P and
> > +	 * RGB565.
> > +	 * e.g. for RGB666_L color format and 1080p, the word count is
> > +	 * 1920*2.25 = 4320 which is divisible by 4 and it is a valid input
> > +	 * to DSI controller. Based on this 2.25 mul factor, we come up with
> > +	 * the division factor of (XDSI_HACT_MULTIPLIER) as 4 for checking
> > +	 */
> > +	if ((vm->hactive & XDSI_HACT_MULTIPLIER) != 0)
> 
> if (!(vm->hactive & XDSI_HACT_MULTIPLIER))?
> 
> > +		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
> 
> dev_warn() better come with more details. Probably it can mention that
> hactive is not aligned along with its value.
> 
> > +
> > +	reg = XDSI_TIME2_HACT((vm->hactive) * (dsi->mul_factor) / 100) |
> 
> Ditto. No ().
> 
> > +	      XDSI_TIME2_VACT(vm->vactive);
> > +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME2, reg);
> > +
> > +	dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive);
> > +}
> > +
> > +/**
> > + * xlnx_dsi_set_display_enable - Enables the DSI Tx IP core enable
> > + * register bit
> > + * @dsi: Xilinx DSI structure
> > + *
> > + * This function takes the DSI strucure and enables the core enable bit
> > + * of core configuration register.
> > + */
> > +static void xlnx_dsi_set_display_enable(struct xlnx_dsi *dsi)
> > +{
> > +	u32 reg;
> > +
> > +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
> > +	reg |= XDSI_CCR_COREENB;
> > +
> 
> Nit. It can be less lines by removing this empty line and setting the reg
> at declarartion an so on.
> 
> > +	xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
> > +	dev_dbg(dsi->dev, "MIPI DSI Tx controller is enabled.\n");
> > +}
> > +
> > +/**
> > + * xlnx_dsi_set_display_disable - Disable the DSI Tx IP core enable
> > + * register bit
> > + * @dsi: Xilinx DSI structure
> > + *
> > + * This function takes the DSI strucure and disables the core enable bit
> > + * of core configuration register.
> > + */
> > +static void xlnx_dsi_set_display_disable(struct xlnx_dsi *dsi)
> > +{
> > +	u32 reg;
> > +
> > +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
> > +	reg &= ~XDSI_CCR_COREENB;
> > +
> 
> Ditto.
> 
> > +	xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
> > +	dev_dbg(dsi->dev, "DSI Tx is disabled. reset regs to default values\n");
> > +}
> > +
> > +/**
> > + * xlnx_dsi_host_transfer - transfer command to panel
> > + * @host: mipi dsi host structure
> > + * @msg: mipi dsi msg with type, length and data
> > + *
> > + * This function is valid only in command mode.
> > + * It checks the command fifo empty status and writes into
> > + * data or cmd register and waits for the completion status.
> > + *
> > + * Return:	number of bytes, on success and error number on failure
> 
> Nit. Extra spaces.
> 
> > + */
> > +static ssize_t xlnx_dsi_host_transfer(struct mipi_dsi_host *host,
> > +				      const struct mipi_dsi_msg *msg)
> > +{
> > +	struct xlnx_dsi *dsi = host_to_dsi(host);
> > +	u32 data0, data1, cmd0, status, val;
> > +	const char *tx_buf = msg->tx_buf;
> > +
> > +	if (!(xlnx_dsi_readl(dsi->iomem, XDSI_CCR) & (XDSI_CCR_COREENB |
> > +						      XDSI_CCR_CMDMODE))) {
> 
> This passes when one of these 2 bits is set, and I don't think it's right.
> 
> > +		dev_err(dsi->dev, "dsi command mode not enabled\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (msg->type == MIPI_DSI_DCS_LONG_WRITE) {
> > +		status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
> > +					    ((val & XDSI_STR_LPKT_MASK) ==
> > +					     XDSI_STR_LPKT_MASK), 1,
> 
> Please remove unneeded (). Same for rest below.
> 
> > +					    XDSI_CMD_TIMEOUT_VAL);
> 
> It'd be nice if these delay / timeout values are described..
> 
> > +		if (status) {
> > +			dev_err(dsi->dev, "long cmd fifo not empty!\n");
> > +			return -ETIMEDOUT;
> 
> Isn't this checking if the controller is ready for long packet transaction?
> Then maybe -EBUSY is better. For timeout, the 'status' can propagate.
> Same for below.
> 
> > +		}
> > +		data0 = tx_buf[0] | (tx_buf[1] << 8) | (tx_buf[2] << 16) |
> > +			(tx_buf[3] << 24);
> > +		data1 = tx_buf[4] | (tx_buf[5] << 8);
> > +		cmd0 = msg->type | (MIPI_DSI_DCS_READ << 8);
> > +
> > +		xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data0);
> > +		xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data1);
> > +		xlnx_dsi_writel(dsi->iomem, XDSI_CMD, cmd0);
> > +	} else {
> 
> I'm not familiar with DSI transaction handling, but is it safe to assume
> all other than long write is short packet write (param, without param)?
> 
> > +		data0 = tx_buf[0];
> 
> This line can be removed.
> 
> > +		if (msg->type == MIPI_DSI_DCS_SHORT_WRITE_PARAM)
> > +			data0 = MIPI_DSI_DCS_SHORT_WRITE_PARAM |
> > +				(tx_buf[0] << 8) | (tx_buf[1] << 16);
> > +		else
> > +			data0 = MIPI_DSI_DCS_SHORT_WRITE | (tx_buf[0] << 8);
> 
> I'd put () for this if - else statments, but it may be just me.
> 
> 
> Then, this can mvoe to right before xlnx_dsi_writel() below.
> 
> > +
> > +		status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
> > +					    ((val & XDSI_STR_RDY_SHPKT) ==
> > +					     XDSI_STR_RDY_SHPKT), 1,
> > +					    XDSI_CMD_TIMEOUT_VAL);
> > +		if (status) {
> > +			dev_err(dsi->dev, "short cmd fifo not empty\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +		xlnx_dsi_writel(dsi->iomem, XDSI_CMD, data0);
> > +	}
> > +
> > +	status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
> > +				    (!(val & XDSI_STR_CMD_EXE_PGS)), 1,
> > +				    XDSI_CMD_TIMEOUT_VAL);
> > +	if (status) {
> > +		dev_err(dsi->dev, "cmd time out\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return msg->tx_len;
> > +}
> > +
> > +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
> > +				struct mipi_dsi_device *device)
> > +{
> > +	u32 panel_lanes;
> > +	struct xlnx_dsi *dsi = host_to_dsi(host);
> > +
> > +	panel_lanes = device->lanes;
> 
> I'd do this above,
> 
> 	u32 panel_lanes = device->lanes
> 
> > +	dsi->mode_flags = device->mode_flags;
> > +	dsi->panel_node = device->dev.of_node;
> > +
> > +	if (panel_lanes != dsi->lanes) {
> > +		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
> > +			panel_lanes, dsi->lanes);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (device->format != dsi->format) {
> > +		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
> > +			device->format, dsi->format);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (dsi->connector.dev)
> > +		drm_helper_hpd_irq_event(dsi->connector.dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int xlnx_dsi_host_detach(struct mipi_dsi_host *host,
> > +				struct mipi_dsi_device *device)
> > +{
> > +	struct xlnx_dsi *dsi = host_to_dsi(host);
> > +
> > +	if (dsi->panel) {
> > +		drm_panel_detach(dsi->panel);
> > +		dsi->panel = NULL;
> > +	}
> > +
> > +	if (dsi->connector.dev)
> > +		drm_helper_hpd_irq_event(dsi->connector.dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
> > +	.attach = xlnx_dsi_host_attach,
> > +	.detach = xlnx_dsi_host_detach,
> > +	.transfer = xlnx_dsi_host_transfer,
> > +};
> > +
> > +static int xlnx_dsi_connector_dpms(struct drm_connector *connector,
> > +				   int mode)
> > +{
> > +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
> > +	int ret;
> > +
> > +	dev_dbg(dsi->dev, "connector dpms state: %d\n", mode);
> > +
> > +	switch (mode) {
> > +	case DRM_MODE_DPMS_ON:
> > +		ret = drm_panel_prepare(dsi->panel);
> > +		if (ret < 0) {
> > +			dev_err(dsi->dev, "DRM panel not found\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = drm_panel_enable(dsi->panel);
> > +		if (ret < 0) {
> > +			drm_panel_unprepare(dsi->panel);
> > +			dev_err(dsi->dev, "DRM panel not enabled\n");
> > +			return ret;
> > +		}
> > +		break;
> > +	default:
> > +		drm_panel_disable(dsi->panel);
> > +		drm_panel_unprepare(dsi->panel);
> > +		break;
> > +	}
> > +
> > +	return drm_helper_connector_dpms(connector, mode);
> > +}
> > +
> > +static enum drm_connector_status
> > +xlnx_dsi_detect(struct drm_connector *connector, bool force)
> > +{
> > +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
> > +
> > +	if (!dsi->panel) {
> > +		dsi->panel = of_drm_find_panel(dsi->panel_node);
> > +		if (dsi->panel) {
> > +			drm_panel_attach(dsi->panel, &dsi->connector);
> 
> Above are quite static, so can't it be done in host attach? Or do you see more
> dynamic use cases?
> 
> > +			if (dsi->cmdmode) {
> > +				xlnx_dsi_writel(dsi->iomem, XDSI_CCR,
> > +						XDSI_CCR_CMDMODE |
> > +						XDSI_CCR_COREENB);
> > +				drm_panel_prepare(dsi->panel);
> 
> Just to confirm, is this not needed for non-command mode? And how is this
> aligned with same call in dpms(), meaning the panel will be prepared
> twice in command mode.
> 
> > +				xlnx_dsi_writel(dsi->iomem, XDSI_CCR, 0);
> > +			}
> > +		}
> > +	} else if (!dsi->panel_node) {
> > +		xlnx_dsi_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> > +		drm_panel_detach(dsi->panel);
> > +		dsi->panel = NULL;
> > +	}
> > +
> > +	if (dsi->panel)
> > +		return connector_status_connected;
> > +
> > +	return connector_status_disconnected;
> > +}
> > +
> > +static void xlnx_dsi_connector_destroy(struct drm_connector *connector)
> > +{
> > +	drm_connector_unregister(connector);
> > +	drm_connector_cleanup(connector);
> > +	connector->dev = NULL;
> > +}
> > +
> > +static const struct drm_connector_funcs xlnx_dsi_connector_funcs = {
> > +	.detect = xlnx_dsi_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = xlnx_dsi_connector_destroy,
> > +	.reset			= drm_atomic_helper_connector_reset,
> 
> This better indent more consistently. Please do it through the entire file.
> 
> > +	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
> > +	.dpms = xlnx_dsi_connector_dpms,
> > +};
> > +
> > +static int xlnx_dsi_get_modes(struct drm_connector *connector)
> > +{
> > +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
> > +
> > +	if (dsi->panel)
> > +		return drm_panel_get_modes(dsi->panel, connector);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct drm_encoder *
> > +xlnx_dsi_best_encoder(struct drm_connector *connector)
> > +{
> > +	return &(connector_to_dsi(connector)->encoder);
> > +}
> > +
> > +static const struct drm_connector_helper_funcs
> > +xlnx_dsi_connector_helper_funcs = {
> > +	.get_modes = xlnx_dsi_get_modes,
> > +	.best_encoder = xlnx_dsi_best_encoder,
> > +};
> > +
> > +static int xlnx_dsi_create_connector(struct drm_encoder *encoder)
> > +{
> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> > +	struct drm_connector *connector = &dsi->connector;
> > +	struct drm_device *drm = encoder->dev;
> > +	int ret;
> > +
> > +	connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +
> > +	ret = drm_connector_init(encoder->dev, connector,
> > +				 &xlnx_dsi_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_DSI);
> > +	if (ret) {
> > +		dev_err(dsi->dev, "Failed to initialize connector with drm\n");
> > +		return ret;
> > +	}
> > +
> > +	drm_connector_helper_add(connector, &xlnx_dsi_connector_helper_funcs);
> > +	drm_connector_attach_encoder(connector, encoder);
> > +	if (!drm->registered)
> > +		return 0;
> > +
> > +	drm_connector_register(connector);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xlnx_dsi_atomic_mode_set -  derive the DSI timing parameters
> > + *
> > + * @encoder: pointer to Xilinx DRM encoder
> > + * @crtc_state: Pointer to drm core crtc state
> > + * @connector_state: DSI connector drm state
> > + *
> > + * This function derives the DSI IP timing parameters from the timing
> > + * values given in the attached panel driver.
> > + */
> > +static void
> > +xlnx_dsi_atomic_mode_set(struct drm_encoder *encoder,
> > +			 struct drm_crtc_state *crtc_state,
> > +				 struct drm_connector_state *connector_state)
> > +{
> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> > +	struct videomode *vm = &dsi->vm;
> > +	struct drm_display_mode *m = &crtc_state->adjusted_mode;
> > +
> > +	vm->hactive = m->hdisplay;
> > +	vm->vactive = m->vdisplay;
> > +	vm->vfront_porch = m->vsync_start - m->vdisplay;
> > +	vm->vback_porch = m->vtotal - m->vsync_end;
> > +	vm->vsync_len = m->vsync_end - m->vsync_start;
> > +	vm->hfront_porch = m->hsync_start - m->hdisplay;
> > +	vm->hback_porch = m->htotal - m->hsync_end;
> > +	vm->hsync_len = m->hsync_end - m->hsync_start;
> > +	xlnx_dsi_set_display_mode(dsi);
> 
> As commented above, this doesn't have to be re-directed through videomode.
> Then the function can be inlined here.
> 
> > +}
> > +
> > +static void xlnx_dsi_disable(struct drm_encoder *encoder)
> > +{
> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> > +
> > +	xlnx_dsi_set_display_disable(dsi);
> 
> 	xlnx_dsi_set_display_disable(encoder_to_dsi(encoder));
> 
> Maybe even this wrapper can be removed?
> 
> > +}
> > +
> > +static void xlnx_dsi_enable(struct drm_encoder *encoder)
> > +{
> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
> > +
> > +	xlnx_dsi_set_display_enable(dsi);
> 
> Ditto.
> 
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs
> > +xlnx_dsi_encoder_helper_funcs = {
> > +	.atomic_mode_set = xlnx_dsi_atomic_mode_set,
> > +	.enable = xlnx_dsi_enable,
> > +	.disable = xlnx_dsi_disable,
> > +};
> > +
> > +static const struct drm_encoder_funcs xlnx_dsi_encoder_funcs = {
> > +	.destroy = drm_encoder_cleanup,
> > +};
> > +
> > +static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi)
> > +{
> > +	struct device *dev = dsi->dev;
> > +	struct device_node *node = dev->of_node;
> > +	int ret;
> > +	u32 datatype;
> > +	static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225, 200};
> > +
> > +	dsi->dphy_clk_200M = devm_clk_get(dev, "dphy_clk_200M");
> > +	if (IS_ERR(dsi->dphy_clk_200M)) {
> > +		ret = PTR_ERR(dsi->dphy_clk_200M);
> > +		dev_err(dev, "failed to get dphy_clk_200M %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dsi->video_aclk = devm_clk_get(dev, "s_axis_aclk");
> > +	if (IS_ERR(dsi->video_aclk)) {
> > +		ret = PTR_ERR(dsi->video_aclk);
> > +		dev_err(dev, "failed to get video_clk %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = of_property_read_u32(node, "xlnx,dsi-num-lanes", &dsi->lanes);
> > +	if (ret < 0) {
> > +		dev_err(dsi->dev, "missing xlnx,dsi-num-lanes property\n");
> > +		return ret;
> > +	}
> > +	if (dsi->lanes > 4 || dsi->lanes < 1) {
> > +		dev_err(dsi->dev, "%d lanes : invalid lanes\n", dsi->lanes);
> > +		return -EINVAL;
> > +	}
> 
> Nit. An empty line here would be helpful for readability.
> 
> > +	ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);
> > +	if (ret < 0) {
> > +		dev_err(dsi->dev, "missing xlnx,dsi-data-type property\n");
> > +		return ret;
> > +	}
> > +	dsi->format = datatype;
> > +	if (datatype > MIPI_DSI_FMT_RGB565) {
> > +		dev_err(dsi->dev, "Invalid xlnx,dsi-data-type string\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * multiplication factor used for HACT, based on data type.
> > +	 *
> > +	 * e.g. for RGB666_L datatype and 1920x1080 resolution,
> > +	 * the Hact (WC) would be as follows -
> > +	 * 1920 pixels * 18 bits per pixel / 8 bits per byte
> > +	 * = 1920 pixels * 2.25 bytes per pixel = 4320 bytes.
> > +	 *
> > +	 * Data Type - Multiplication factor
> > +	 * RGB888    - 3
> > +	 * RGB666_L  - 2.25
> > +-	 * RGB666_P  - 2.25
> > +	 * RGB565    - 2
> > +	 *
> > +	 * Since the multiplication factor maybe a floating number,
> > +	 * a 100x multiplication factor is used.
> > +	 */
> > +	dsi->mul_factor = xdsi_mul_fact[datatype];
> > +
> > +	dsi->cmdmode = of_property_read_bool(node, "xlnx,dsi-cmd-mode");
> > +
> > +	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
> > +	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
> > +	dev_dbg(dsi->dev, "DSI controller cmd mode = %d\n", dsi->cmdmode);
> > +
> > +	return 0;
> > +}
> > +
> > +static int xlnx_dsi_bind(struct device *dev, struct device *master,
> > +			 void *data)
> > +{
> > +	struct xlnx_dsi *dsi = dev_get_drvdata(dev);
> > +	struct drm_encoder *encoder = &dsi->encoder;
> > +	struct drm_device *drm_dev = data;
> > +	int ret;
> > +
> > +	drm_encoder_init(drm_dev, encoder, &xlnx_dsi_encoder_funcs,
> > +			 DRM_MODE_ENCODER_DSI, NULL);
> > +	drm_encoder_helper_add(encoder, &xlnx_dsi_encoder_helper_funcs);
> > +
> > +	encoder->possible_crtcs = 1;
> > +
> > +	ret = xlnx_dsi_create_connector(encoder);
> > +	if (ret) {
> > +		dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret);
> > +		drm_encoder_cleanup(encoder);
> > +		return ret;
> > +	}
> > +
> > +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> > +	if (ret < 0) {
> 
> Just nit. if (ret) would do.
> 
> > +		dev_err(dsi->dev, "failed to register DSI host: %d\n", ret);
> > +		xlnx_dsi_connector_destroy(&dsi->connector);
> > +		drm_encoder_cleanup(encoder);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void xlnx_dsi_unbind(struct device *dev, struct device *master,
> > +			    void *data)
> > +{
> > +	struct xlnx_dsi *dsi = dev_get_drvdata(dev);
> > +
> > +	xlnx_dsi_disable(&dsi->encoder);
> > +	mipi_dsi_host_unregister(&dsi->dsi_host);
> > +}
> > +
> > +static const struct component_ops xlnx_dsi_component_ops = {
> > +	.bind	= xlnx_dsi_bind,
> > +	.unbind	= xlnx_dsi_unbind,
> > +};
> > +
> > +static int xlnx_dsi_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *res;
> > +	struct xlnx_dsi *dsi;
> > +	int ret;
> > +	unsigned long rate;
> > +
> > +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> > +	if (!dsi)
> > +		return -ENOMEM;
> > +
> > +	dsi->dsi_host.ops = &xlnx_dsi_ops;
> > +	dsi->dsi_host.dev = dev;
> > +	dsi->dev = dev;
> > +
> > +	ret = xlnx_dsi_parse_dt(dsi);
> > +	if (ret)
> > +		return ret;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	dsi->iomem = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(dsi->iomem)) {
> > +		dev_err(dev, "failed to remap io region\n");
> > +		return PTR_ERR(dsi->iomem);
> > +	}
> > +	platform_set_drvdata(pdev, dsi);
> > +
> > +	ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ);
> > +	if (ret) {
> > +		dev_err(dev, "failed to set dphy clk rate %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(dsi->dphy_clk_200M);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable dphy clk %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	rate = clk_get_rate(dsi->dphy_clk_200M);
> > +	if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) {
> 
> A brief comment for this line wold be helpful.
> 
> > +		dev_err(dev, "Error DPHY clock = %lu\n", rate);
> > +		ret = -EINVAL;
> > +		goto err_disable_dphy_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(dsi->video_aclk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable video clk %d\n", ret);
> > +		goto err_disable_dphy_clk;
> > +	}
> > +
> > +	ret = component_add(dev, &xlnx_dsi_component_ops);

The driver should expose the DSI-TX as a drm_bridge instead of using the
component framework. You shouldn't register a drm_encoder, and I don't
think you should register a drm_connector either. Only bridge operations
should be exposed, and the drm_bridge .attach() operation should return
an error when DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. The top-level
driver using this bridge should create the drm_encoder and
drm_connector, most likely using drm_bridge_connector_init() to create
the connector.

> > +	if (ret < 0)
> 
> I prefer if (ret) where possible, but it may be just me. I let you decide.
> 
> > +		goto err_disable_video_clk;
> > +
> > +	return ret;
> 
> return 0;? but up to you.
> 
> > +
> > +err_disable_video_clk:
> > +	clk_disable_unprepare(dsi->video_aclk);
> > +err_disable_dphy_clk:
> > +	clk_disable_unprepare(dsi->dphy_clk_200M);
> > +	return ret;
> > +}
> 
> At least, this initial one better come with a complete display pipeline,
> ex crtc / plane are missing. This won't be functional without those.
> I believe Laurent was planning to make the xlnx drm layer as a library,
> so hopefully this can be a good starting point and come with drivers aligned
> with it.
> 
> > +
> > +static int xlnx_dsi_remove(struct platform_device *pdev)
> > +{
> > +	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
> > +
> > +	component_del(&pdev->dev, &xlnx_dsi_component_ops);
> > +	clk_disable_unprepare(dsi->video_aclk);
> > +	clk_disable_unprepare(dsi->dphy_clk_200M);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id xlnx_dsi_of_match[] = {
> > +	{ .compatible = "xlnx-dsi", },
> 
> This should change. I believe it should be something like, xlnx,<dsi ip name>-<version>
> 
> > +	{ /* end of table */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match);
> > +
> > +static struct platform_driver dsi_driver = {
> > +	.probe = xlnx_dsi_probe,
> > +	.remove = xlnx_dsi_remove,
> > +	.driver = {
> > +		.name = "xlnx-dsi",
> > +		.of_match_table = xlnx_dsi_of_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(dsi_driver);
> > +
> > +MODULE_AUTHOR("Xilinx, Inc.");
> > +MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver");
> > +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Hyun Kwon]

Hi Laurent,

On Sat, 2020-05-23 at 20:08:13 -0700, Laurent Pinchart wrote:
> Hi GVRao,
> 
> Thank you for the patch.
> 
> On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> > On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> > > The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> > > data from AXI-4 stream interface.
> > > 
> > > It supports upto 4 lanes, optional register interface for the DPHY,
> > 
> > I don't see the register interface for dphy support.
> 
> I think the D-PHY should be supported through a PHY driver, as it seems
> to be shared between different subsystems.
> 

Right, if the logic is shared across subsystems. I can't tell if that's
the case as the IP comes as a single block. Maybe GVRao can confirm.

> > > multiple RGB color formats, command mode and video mode.
> > > This is a MIPI-DSI host driver and provides DSI bus for panels.
> > > This driver also helps to communicate with its panel using panel
> > > framework.
> > > 
> > > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > > ---
> > >  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> > >  drivers/gpu/drm/xlnx/Makefile   |   2 +
> > >  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> 
> Daniel Vetter has recently expressed his opiion that bridge drivers
> should go to drivers/gpu/drm/bridge/. It would then be
> drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
> 
> > >  3 files changed, 768 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
> > > 
> > > diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> > > index aa6cd88..73873cf 100644
> > > --- a/drivers/gpu/drm/xlnx/Kconfig
> > > +++ b/drivers/gpu/drm/xlnx/Kconfig
> > > @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
> > >  	  This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
> > >  	  this option if you have a Xilinx ZynqMP SoC with DisplayPort
> > >  	  subsystem.
> > > +
> > > +config DRM_XLNX_DSI
> > > +        tristate "Xilinx DRM DSI Subsystem Driver"
> > > +        select DRM_MIPI_DSI
> > > +        select DRM_PANEL
> > > +        select DRM_PANEL_SIMPLE
> > > +        help
> > > +	  This enables support for Xilinx MIPI-DSI.
> > 
> > This sentence is not needed with below. Could you please rephrase the whole?
> > 
> > > +	  This is a DRM/KMS driver for Xilinx programmable DSI controller.
> > > +	  Choose this option if you have a Xilinx MIPI DSI-TX controller
> > > +	  subsytem.
> > 
> > These seem incorrectly indented.
> > 
> > > diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> > > index 2b844c6..b7ee6ef 100644
> > > --- a/drivers/gpu/drm/xlnx/Makefile
> > > +++ b/drivers/gpu/drm/xlnx/Makefile
> > > @@ -1,2 +1,4 @@
> > >  zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
> > >  obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
> > > +
> > > +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
> > > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > > new file mode 100644
> > > index 0000000..b8cae59
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > > @@ -0,0 +1,755 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Xilinx FPGA MIPI DSI Tx Controller driver
> > > + *
> > > + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> > > + *
> > > + * Authors:
> > > + * - Saurabh Sengar <saurabhs@xilinx.com>
> > > + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > > + */
> > > +
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_connector.h>
> > > +#include <drm/drm_crtc.h>
> > > +#include <drm/drm_crtc_helper.h>
> > > +#include <drm/drm_device.h>
> > > +#include <drm/drm_encoder.h>
> > > +#include <drm/drm_fourcc.h>
> > > +#include <drm/drm_gem_cma_helper.h>
> > > +#include <drm/drm_mipi_dsi.h>
> > > +#include <drm/drm_panel.h>
> > > +#include <drm/drm_probe_helper.h>
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/component.h>
> > > +#include <linux/device.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/phy/phy.h>
> > > +
> > > +#include <video/mipi_display.h>
> > > +#include <video/videomode.h>
> > > +
> > > +/* DSI Tx IP registers */
> > > +#define XDSI_CCR			0x00
> > > +#define XDSI_CCR_COREENB		BIT(0)
> > > +#define XDSI_CCR_SOFTRST		BIT(1)
> > > +#define XDSI_CCR_CRREADY		BIT(2)
> > > +#define XDSI_CCR_CMDMODE		BIT(3)
> > > +#define XDSI_CCR_DFIFORST		BIT(4)
> > > +#define XDSI_CCR_CMDFIFORST		BIT(5)
> > > +#define XDSI_PCR			0x04

[snip]

> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(dsi->video_aclk);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to enable video clk %d\n", ret);
> > > +		goto err_disable_dphy_clk;
> > > +	}
> > > +
> > > +	ret = component_add(dev, &xlnx_dsi_component_ops);
> 
> The driver should expose the DSI-TX as a drm_bridge instead of using the
> component framework. You shouldn't register a drm_encoder, and I don't
> think you should register a drm_connector either. Only bridge operations
> should be exposed, and the drm_bridge .attach() operation should return
> an error when DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. The top-level
> driver using this bridge should create the drm_encoder and
> drm_connector, most likely using drm_bridge_connector_init() to create
> the connector.
> 

Not clear to me if this has to be a bridge, and then what it will be attached
to. The IP block itself pretty much self-contains all functionalities already,
just like any other drm encoder / connector, so it doesn't have to be wrapped
around by any other layer. Please let me know your thought, so I can understand
better. :-)

Thanks,
-hyun

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Laurent Pinchart]

Hi Hyun,

On Wed, May 27, 2020 at 10:54:35AM -0700, Hyun Kwon wrote:
> On Sat, 2020-05-23 at 20:08:13 -0700, Laurent Pinchart wrote:
> > On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> >> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> >>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> >>> data from AXI-4 stream interface.
> >>> 
> >>> It supports upto 4 lanes, optional register interface for the DPHY,
> >> 
> >> I don't see the register interface for dphy support.
> > 
> > I think the D-PHY should be supported through a PHY driver, as it seems
> > to be shared between different subsystems.
> 
> Right, if the logic is shared across subsystems. I can't tell if that's
> the case as the IP comes as a single block. Maybe GVRao can confirm.

I believe the CSI2-RX subsystem uses the same D-PHY IP core, but a
confirmation would be nice.

> >>> multiple RGB color formats, command mode and video mode.
> >>> This is a MIPI-DSI host driver and provides DSI bus for panels.
> >>> This driver also helps to communicate with its panel using panel
> >>> framework.
> >>> 
> >>> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> >>> ---
> >>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> >>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
> >>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> > 
> > Daniel Vetter has recently expressed his opiion that bridge drivers
> > should go to drivers/gpu/drm/bridge/. It would then be
> > drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
> > 
> >>>  3 files changed, 768 insertions(+)
> >>>  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
> >>> 
> >>> diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> >>> index aa6cd88..73873cf 100644
> >>> --- a/drivers/gpu/drm/xlnx/Kconfig
> >>> +++ b/drivers/gpu/drm/xlnx/Kconfig
> >>> @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
> >>>  	  This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
> >>>  	  this option if you have a Xilinx ZynqMP SoC with DisplayPort
> >>>  	  subsystem.
> >>> +
> >>> +config DRM_XLNX_DSI
> >>> +        tristate "Xilinx DRM DSI Subsystem Driver"
> >>> +        select DRM_MIPI_DSI
> >>> +        select DRM_PANEL
> >>> +        select DRM_PANEL_SIMPLE
> >>> +        help
> >>> +	  This enables support for Xilinx MIPI-DSI.
> >> 
> >> This sentence is not needed with below. Could you please rephrase the whole?
> >> 
> >>> +	  This is a DRM/KMS driver for Xilinx programmable DSI controller.
> >>> +	  Choose this option if you have a Xilinx MIPI DSI-TX controller
> >>> +	  subsytem.
> >> 
> >> These seem incorrectly indented.
> >> 
> >>> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> >>> index 2b844c6..b7ee6ef 100644
> >>> --- a/drivers/gpu/drm/xlnx/Makefile
> >>> +++ b/drivers/gpu/drm/xlnx/Makefile
> >>> @@ -1,2 +1,4 @@
> >>>  zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
> >>>  obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
> >>> +
> >>> +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
> >>> diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> >>> new file mode 100644
> >>> index 0000000..b8cae59
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> >>> @@ -0,0 +1,755 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Xilinx FPGA MIPI DSI Tx Controller driver
> >>> + *
> >>> + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> >>> + *
> >>> + * Authors:
> >>> + * - Saurabh Sengar <saurabhs@xilinx.com>
> >>> + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> >>> + */
> >>> +
> >>> +#include <drm/drm_atomic_helper.h>
> >>> +#include <drm/drm_connector.h>
> >>> +#include <drm/drm_crtc.h>
> >>> +#include <drm/drm_crtc_helper.h>
> >>> +#include <drm/drm_device.h>
> >>> +#include <drm/drm_encoder.h>
> >>> +#include <drm/drm_fourcc.h>
> >>> +#include <drm/drm_gem_cma_helper.h>
> >>> +#include <drm/drm_mipi_dsi.h>
> >>> +#include <drm/drm_panel.h>
> >>> +#include <drm/drm_probe_helper.h>
> >>> +
> >>> +#include <linux/clk.h>
> >>> +#include <linux/component.h>
> >>> +#include <linux/device.h>
> >>> +#include <linux/iopoll.h>
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/of_graph.h>
> >>> +#include <linux/phy/phy.h>
> >>> +
> >>> +#include <video/mipi_display.h>
> >>> +#include <video/videomode.h>
> >>> +
> >>> +/* DSI Tx IP registers */
> >>> +#define XDSI_CCR			0x00
> >>> +#define XDSI_CCR_COREENB		BIT(0)
> >>> +#define XDSI_CCR_SOFTRST		BIT(1)
> >>> +#define XDSI_CCR_CRREADY		BIT(2)
> >>> +#define XDSI_CCR_CMDMODE		BIT(3)
> >>> +#define XDSI_CCR_DFIFORST		BIT(4)
> >>> +#define XDSI_CCR_CMDFIFORST		BIT(5)
> >>> +#define XDSI_PCR			0x04
> 
> [snip]
> 
> >>> +	}
> >>> +
> >>> +	ret = clk_prepare_enable(dsi->video_aclk);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "failed to enable video clk %d\n", ret);
> >>> +		goto err_disable_dphy_clk;
> >>> +	}
> >>> +
> >>> +	ret = component_add(dev, &xlnx_dsi_component_ops);
> > 
> > The driver should expose the DSI-TX as a drm_bridge instead of using the
> > component framework. You shouldn't register a drm_encoder, and I don't
> > think you should register a drm_connector either. Only bridge operations
> > should be exposed, and the drm_bridge .attach() operation should return
> > an error when DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. The top-level
> > driver using this bridge should create the drm_encoder and
> > drm_connector, most likely using drm_bridge_connector_init() to create
> > the connector.
> 
> Not clear to me if this has to be a bridge, and then what it will be attached
> to. The IP block itself pretty much self-contains all functionalities already,
> just like any other drm encoder / connector, so it doesn't have to be wrapped
> around by any other layer. Please let me know your thought, so I can understand
> better. :-)

The DSI output will likely often be connected to a DSI panel, but it
could also be connected to another bridge, for instance to an ADV7533
DSI-to-HDMI bridge. In that case an HDMI connector needs to be created,
not a DSI connector. This is why we are moving towards a model where
bridge drivers only handle the bridge device, and the drm_encoder and
drm_connector is created externally, but the display controller driver.
The drm_bridge_connector_init() helper can automate connector creation
for a chain of bridges.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Hyun Kwon]

Hi Laurent,

On Wed, 2020-05-27 at 15:45:24 -0700, Laurent Pinchart wrote:
> Hi Hyun,
> 
> On Wed, May 27, 2020 at 10:54:35AM -0700, Hyun Kwon wrote:
> > On Sat, 2020-05-23 at 20:08:13 -0700, Laurent Pinchart wrote:
> > > On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> > >> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> > >>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> > >>> data from AXI-4 stream interface.
> > >>> 
> > >>> It supports upto 4 lanes, optional register interface for the DPHY,
> > >> 
> > >> I don't see the register interface for dphy support.
> > > 
> > > I think the D-PHY should be supported through a PHY driver, as it seems
> > > to be shared between different subsystems.
> > 
> > Right, if the logic is shared across subsystems. I can't tell if that's
> > the case as the IP comes as a single block. Maybe GVRao can confirm.
> 
> I believe the CSI2-RX subsystem uses the same D-PHY IP core, but a
> confirmation would be nice.
> 
> > >>> multiple RGB color formats, command mode and video mode.
> > >>> This is a MIPI-DSI host driver and provides DSI bus for panels.
> > >>> This driver also helps to communicate with its panel using panel
> > >>> framework.
> > >>> 
> > >>> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > >>> ---
> > >>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> > >>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
> > >>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> > > 
> > > Daniel Vetter has recently expressed his opiion that bridge drivers
> > > should go to drivers/gpu/drm/bridge/. It would then be
> > > drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
> > > 
> > >>>  3 files changed, 768 insertions(+)
> > >>>  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
> > >>> 
> > >>> diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> > >>> index aa6cd88..73873cf 100644
> > >>> --- a/drivers/gpu/drm/xlnx/Kconfig
> > >>> +++ b/drivers/gpu/drm/xlnx/Kconfig
> > >>> @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
> > >>>  	  This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
> > >>>  	  this option if you have a Xilinx ZynqMP SoC with DisplayPort
> > >>>  	  subsystem.
> > >>> +
> > >>> +config DRM_XLNX_DSI
> > >>> +        tristate "Xilinx DRM DSI Subsystem Driver"
> > >>> +        select DRM_MIPI_DSI
> > >>> +        select DRM_PANEL
> > >>> +        select DRM_PANEL_SIMPLE
> > >>> +        help
> > >>> +	  This enables support for Xilinx MIPI-DSI.
> > >> 
> > >> This sentence is not needed with below. Could you please rephrase the whole?
> > >> 
> > >>> +	  This is a DRM/KMS driver for Xilinx programmable DSI controller.
> > >>> +	  Choose this option if you have a Xilinx MIPI DSI-TX controller
> > >>> +	  subsytem.
> > >> 
> > >> These seem incorrectly indented.
> > >> 
> > >>> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> > >>> index 2b844c6..b7ee6ef 100644
> > >>> --- a/drivers/gpu/drm/xlnx/Makefile
> > >>> +++ b/drivers/gpu/drm/xlnx/Makefile
> > >>> @@ -1,2 +1,4 @@
> > >>>  zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
> > >>>  obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
> > >>> +
> > >>> +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
> > >>> diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > >>> new file mode 100644
> > >>> index 0000000..b8cae59
> > >>> --- /dev/null
> > >>> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > >>> @@ -0,0 +1,755 @@
> > >>> +// SPDX-License-Identifier: GPL-2.0
> > >>> +/*
> > >>> + * Xilinx FPGA MIPI DSI Tx Controller driver
> > >>> + *
> > >>> + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> > >>> + *
> > >>> + * Authors:
> > >>> + * - Saurabh Sengar <saurabhs@xilinx.com>
> > >>> + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > >>> + */
> > >>> +
> > >>> +#include <drm/drm_atomic_helper.h>
> > >>> +#include <drm/drm_connector.h>
> > >>> +#include <drm/drm_crtc.h>
> > >>> +#include <drm/drm_crtc_helper.h>
> > >>> +#include <drm/drm_device.h>
> > >>> +#include <drm/drm_encoder.h>
> > >>> +#include <drm/drm_fourcc.h>
> > >>> +#include <drm/drm_gem_cma_helper.h>
> > >>> +#include <drm/drm_mipi_dsi.h>
> > >>> +#include <drm/drm_panel.h>
> > >>> +#include <drm/drm_probe_helper.h>
> > >>> +
> > >>> +#include <linux/clk.h>
> > >>> +#include <linux/component.h>
> > >>> +#include <linux/device.h>
> > >>> +#include <linux/iopoll.h>
> > >>> +#include <linux/of_device.h>
> > >>> +#include <linux/of_graph.h>
> > >>> +#include <linux/phy/phy.h>
> > >>> +
> > >>> +#include <video/mipi_display.h>
> > >>> +#include <video/videomode.h>
> > >>> +
> > >>> +/* DSI Tx IP registers */
> > >>> +#define XDSI_CCR			0x00
> > >>> +#define XDSI_CCR_COREENB		BIT(0)
> > >>> +#define XDSI_CCR_SOFTRST		BIT(1)
> > >>> +#define XDSI_CCR_CRREADY		BIT(2)
> > >>> +#define XDSI_CCR_CMDMODE		BIT(3)
> > >>> +#define XDSI_CCR_DFIFORST		BIT(4)
> > >>> +#define XDSI_CCR_CMDFIFORST		BIT(5)
> > >>> +#define XDSI_PCR			0x04
> > 
> > [snip]
> > 
> > >>> +	}
> > >>> +
> > >>> +	ret = clk_prepare_enable(dsi->video_aclk);
> > >>> +	if (ret) {
> > >>> +		dev_err(dev, "failed to enable video clk %d\n", ret);
> > >>> +		goto err_disable_dphy_clk;
> > >>> +	}
> > >>> +
> > >>> +	ret = component_add(dev, &xlnx_dsi_component_ops);
> > > 
> > > The driver should expose the DSI-TX as a drm_bridge instead of using the
> > > component framework. You shouldn't register a drm_encoder, and I don't
> > > think you should register a drm_connector either. Only bridge operations
> > > should be exposed, and the drm_bridge .attach() operation should return
> > > an error when DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. The top-level
> > > driver using this bridge should create the drm_encoder and
> > > drm_connector, most likely using drm_bridge_connector_init() to create
> > > the connector.
> > 
> > Not clear to me if this has to be a bridge, and then what it will be attached
> > to. The IP block itself pretty much self-contains all functionalities already,
> > just like any other drm encoder / connector, so it doesn't have to be wrapped
> > around by any other layer. Please let me know your thought, so I can understand
> > better. :-)
> 
> The DSI output will likely often be connected to a DSI panel, but it
> could also be connected to another bridge, for instance to an ADV7533
> DSI-to-HDMI bridge. In that case an HDMI connector needs to be created,
> not a DSI connector. This is why we are moving towards a model where
> bridge drivers only handle the bridge device, and the drm_encoder and
> drm_connector is created externally, but the display controller driver.
> The drm_bridge_connector_init() helper can automate connector creation
> for a chain of bridges.

Ah I see. Thanks for explanation. It seems relatively new changes and more
scalable. The above case is less likely with FPGA pipeline. But right, it can
still happen in certain cases, ex with 96 boards that have DSI interface only,
and that will help. So I believe encoder / connector drivers (there are more
in Xilinx tree) need to be converted to bridges, with a new top level driver
handling the bridges.

Thanks,
-hyun

RE: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Venkateshwar Rao Gannavarapu]

Hi Laurent & Hyun,

Thanks for the review.

>-----Original Message-----
>From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>Sent: Sunday, May 24, 2020 8:38 AM
>To: Venkateshwar Rao Gannavarapu <VGANNAVA@xilinx.com>
>Cc: Hyun Kwon <hyunk@xilinx.com>; Hyun Kwon <hyunk@xilinx.com>; dri-
>devel@lists.freedesktop.org; airlied@linux.ie; daniel@ffwll.ch; linux-
>kernel@vger.kernel.org; Sandip Kothari <sandipk@xilinx.com>; Venkateshwar
>Rao Gannavarapu <VGANNAVA@xilinx.com>
>Subject: Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem
>
>Hi GVRao,
>
>Thank you for the patch.
>
>On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
>> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu
>wrote:
>> > The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
>> > data from AXI-4 stream interface.
>> >
>> > It supports upto 4 lanes, optional register interface for the DPHY,
>>
>> I don't see the register interface for dphy support.
>
>I think the D-PHY should be supported through a PHY driver, as it seems to be
>shared between different subsystems.
>
IP has the provision to read DPHY register for debug purpose only.
No programming of DPHY is required in subsystem.

>> > multiple RGB color formats, command mode and video mode.
>> > This is a MIPI-DSI host driver and provides DSI bus for panels.
>> > This driver also helps to communicate with its panel using panel
>> > framework.
>> >
>> > Signed-off-by: Venkateshwar Rao Gannavarapu
>> > <venkateshwar.rao.gannavarapu@xilinx.com>
>> > ---
>> >  drivers/gpu/drm/xlnx/Kconfig    |  11 +
>> >  drivers/gpu/drm/xlnx/Makefile   |   2 +
>> >  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755
>> > ++++++++++++++++++++++++++++++++++++++++
>
>Daniel Vetter has recently expressed his opiion that bridge drivers should go to
>drivers/gpu/drm/bridge/. It would then be drivers/gpu/drm/bridge/xlnx/. I don't
>have a strong opinion myself.
>
>> >  3 files changed, 768 insertions(+)
>> >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
>> >
>> > diff --git a/drivers/gpu/drm/xlnx/Kconfig
>> > b/drivers/gpu/drm/xlnx/Kconfig index aa6cd88..73873cf 100644
>> > --- a/drivers/gpu/drm/xlnx/Kconfig
>> > +++ b/drivers/gpu/drm/xlnx/Kconfig
>> > @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
>> >  	  This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
>> >  	  this option if you have a Xilinx ZynqMP SoC with DisplayPort
>> >  	  subsystem.
>> > +
>> > +config DRM_XLNX_DSI
>> > +        tristate "Xilinx DRM DSI Subsystem Driver"
>> > +        select DRM_MIPI_DSI
>> > +        select DRM_PANEL
>> > +        select DRM_PANEL_SIMPLE
>> > +        help
>> > +	  This enables support for Xilinx MIPI-DSI.
>>
>> This sentence is not needed with below. Could you please rephrase the whole?

OK.
>>
>> > +	  This is a DRM/KMS driver for Xilinx programmable DSI controller.
>> > +	  Choose this option if you have a Xilinx MIPI DSI-TX controller
>> > +	  subsytem.
>>
>> These seem incorrectly indented.
Will check the indentation.
>>
>> > diff --git a/drivers/gpu/drm/xlnx/Makefile
>> > b/drivers/gpu/drm/xlnx/Makefile index 2b844c6..b7ee6ef 100644
>> > --- a/drivers/gpu/drm/xlnx/Makefile
>> > +++ b/drivers/gpu/drm/xlnx/Makefile
>> > @@ -1,2 +1,4 @@
>> >  zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
>> >  obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
>> > +
>> > +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
>> > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c
>> > b/drivers/gpu/drm/xlnx/xlnx_dsi.c new file mode 100644 index
>> > 0000000..b8cae59
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
>> > @@ -0,0 +1,755 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +/*
>> > + * Xilinx FPGA MIPI DSI Tx Controller driver
>> > + *
>> > + * Copyright (C) 2017 - 2019 Xilinx, Inc.
>> > + *
>> > + * Authors:
>> > + * - Saurabh Sengar <saurabhs@xilinx.com>
>> > + * - Venkateshwar Rao Gannavarapu
>> > +<venkateshwar.rao.gannavarapu@xilinx.com>
>> > + */
>> > +
>> > +#include <drm/drm_atomic_helper.h>
>> > +#include <drm/drm_connector.h>
>> > +#include <drm/drm_crtc.h>
>> > +#include <drm/drm_crtc_helper.h>
>> > +#include <drm/drm_device.h>
>> > +#include <drm/drm_encoder.h>
>> > +#include <drm/drm_fourcc.h>
>> > +#include <drm/drm_gem_cma_helper.h> #include <drm/drm_mipi_dsi.h>
>> > +#include <drm/drm_panel.h> #include <drm/drm_probe_helper.h>
>> > +
>> > +#include <linux/clk.h>
>> > +#include <linux/component.h>
>> > +#include <linux/device.h>
>> > +#include <linux/iopoll.h>
>> > +#include <linux/of_device.h>
>> > +#include <linux/of_graph.h>
>> > +#include <linux/phy/phy.h>
>> > +
>> > +#include <video/mipi_display.h>
>> > +#include <video/videomode.h>
>> > +
>> > +/* DSI Tx IP registers */
>> > +#define XDSI_CCR			0x00
>> > +#define XDSI_CCR_COREENB		BIT(0)
>> > +#define XDSI_CCR_SOFTRST		BIT(1)
>> > +#define XDSI_CCR_CRREADY		BIT(2)
>> > +#define XDSI_CCR_CMDMODE		BIT(3)
>> > +#define XDSI_CCR_DFIFORST		BIT(4)
>> > +#define XDSI_CCR_CMDFIFORST		BIT(5)
>> > +#define XDSI_PCR			0x04
>> > +#define XDSI_PCR_VIDEOMODE(x)		(((x) & 0x3) << 3)
>> > +#define XDSI_PCR_VIDEOMODE_MASK		(0x3 << 3)
>> > +#define XDSI_PCR_VIDEOMODE_SHIFT	3
>> > +#define XDSI_PCR_BLLPTYPE(x)		((x) << 5)
>> > +#define XDSI_PCR_BLLPMODE(x)		((x) << 6)
>> > +#define XDSI_PCR_EOTPENABLE(x)		((x) << 13)
>> > +#define XDSI_GIER			0x20
>> > +#define XDSI_ISR			0x24
>> > +#define XDSI_IER			0x28
>> > +#define XDSI_STR			0x2C
>> > +#define XDSI_STR_RDY_SHPKT		BIT(6)
>> > +#define XDSI_STR_RDY_LNGPKT		BIT(7)
>> > +#define XDSI_STR_DFIFO_FULL		BIT(8)
>> > +#define XDSI_STR_DFIFO_EMPTY		BIT(9)
>> > +#define XDSI_STR_WAITFR_DATA		BIT(10)
>> > +#define XDSI_STR_CMD_EXE_PGS		BIT(11)
>> > +#define XDSI_STR_CCMD_PROC		BIT(12)
>> > +#define XDSI_STR_LPKT_MASK		(0x5 << 7)
>> > +#define XDSI_CMD			0x30
>> > +#define XDSI_CMD_QUEUE_PACKET(x)	((x) & GENMASK(23, 0))
>> > +#define XDSI_DFR			0x34
>> > +#define XDSI_TIME1			0x50
>> > +#define XDSI_TIME1_BLLP_BURST(x)	((x) & GENMASK(15, 0))
>> > +#define XDSI_TIME1_HSA(x)		(((x) & GENMASK(15, 0)) << 16)
>> > +#define XDSI_TIME2			0x54
>> > +#define XDSI_TIME2_VACT(x)		((x) & GENMASK(15, 0))
>> > +#define XDSI_TIME2_HACT(x)		(((x) & GENMASK(15, 0)) << 16)
>> > +#define XDSI_HACT_MULTIPLIER		GENMASK(1, 0)
>> > +#define XDSI_TIME3			0x58
>> > +#define XDSI_TIME3_HFP(x)		((x) & GENMASK(15, 0))
>> > +#define XDSI_TIME3_HBP(x)		(((x) & GENMASK(15, 0)) << 16)
>> > +#define XDSI_TIME4			0x5c
>> > +#define XDSI_TIME4_VFP(x)		((x) & GENMASK(7, 0))
>> > +#define XDSI_TIME4_VBP(x)		(((x) & GENMASK(7, 0)) << 8)
>> > +#define XDSI_TIME4_VSA(x)		(((x) & GENMASK(7, 0)) << 16)
>> > +#define XDSI_LTIME			0x60
>> > +#define XDSI_BLLP_TIME			0x64
>> > +/*
>> > + * XDSI_NUM_DATA_T represents number of data types in the
>> > + * enum mipi_dsi_pixel_format in the MIPI DSI part of DRM framework.
>> > + */
>> > +#define XDSI_NUM_DATA_T			4
>> > +#define XDSI_VIDEO_MODE_SYNC_PULSE	0x0
>> > +#define XDSI_VIDEO_MODE_SYNC_EVENT	0x1
>> > +#define XDSI_VIDEO_MODE_BURST		0x2
>>
>> Please remove these unused 3 definitions.
OK.
>>
>> > +
>> > +#define XDSI_DPHY_CLK_MIN	197000000000UL
>> > +#define XDSI_DPHY_CLK_MAX	203000000000UL
>> > +#define XDSI_DPHY_CLK_REQ	200000000000UL
>> > +
>> > +/* command timeout in usec */
>> > +#define XDSI_CMD_TIMEOUT_VAL	(3000)
>> > +
>> > +/**
>> > + * struct xlnx_dsi - Xilinx DSI-TX core
>> > + * @encoder: DRM encoder structure
>> > + * @dsi_host: DSI host device
>> > + * @connector: DRM connector structure
>> > + * @panel_node: MIPI DSI device panel node
>> > + * @panel:  DRM panel structure
>> > + * @dev: device structure
>> > + * @iomem: Base address of DSI subsystem
>> > + * @lanes: number of active data lanes supported by DSI controller
>> > + * @cmdmode: command mode support
>> > + * @mode_flags: DSI operation mode related flags
>> > + * @format: pixel format for video mode of DSI controller
>> > + * @vm: videomode data structure
>> > + * @mul_factor: multiplication factor for HACT timing parameter
>> > + * @video_aclk: Video clock
>> > + * @dphy_clk_200M: 200MHz DPHY clock and AXI Lite clock  */ struct
>> > +xlnx_dsi {
>> > +	struct drm_encoder encoder;
>> > +	struct mipi_dsi_host dsi_host;
>> > +	struct drm_connector connector;
>> > +	struct device_node *panel_node;
>> > +	struct drm_panel *panel;
>> > +	struct device *dev;
>> > +	void __iomem *iomem;
>> > +	u32 lanes;
>> > +	bool cmdmode;
>> > +	u32 mode_flags;
>> > +	enum mipi_dsi_pixel_format format;
>> > +	struct videomode vm;
>> > +	u32 mul_factor;
>> > +	struct clk *video_aclk;
>> > +	struct clk *dphy_clk_200M;
>> > +};
>> > +
>> > +#define host_to_dsi(host) container_of(host, struct xlnx_dsi,
>> > +dsi_host) #define connector_to_dsi(c) container_of(c, struct
>> > +xlnx_dsi, connector) #define encoder_to_dsi(e) container_of(e,
>> > +struct xlnx_dsi, encoder)
>> > +
>> > +static inline void xlnx_dsi_writel(void __iomem *base, int offset,
>> > +u32 val) {
>> > +	writel(val, base + offset);
>> > +}
>> > +
>> > +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset) {
>> > +	return readl(base + offset);
>> > +}
>> > +
>> > +/**
>> > + * xlnx_dsi_set_display_mode - Configure DSI timing registers
>> > + * @dsi: xilinx DSI structure
>> > + *
>> > + * This function writes the timing parameters of DSI IP which are
>> > + * retrieved from panel timing values.
>> > + */
>> > +static void xlnx_dsi_set_display_mode(struct xlnx_dsi *dsi)
>>
>> This function better directly inline to xlnx_dsi_atomic_mode_set().
>> Then, the videomode is not really needed. Please remove it and use
>drm_display_mode.
OK.
>>
>> > +{
>> > +	struct videomode *vm = &dsi->vm;
>> > +	u32 reg, video_mode;
>> > +
>> > +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
>> > +	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >>
>> > +		      XDSI_PCR_VIDEOMODE_SHIFT;
>> > +
>> > +	/* configure the HSA value only if non_burst_sync_pluse video mode */
>> > +	if (!video_mode &&
>> > +	    (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
>> > +		reg = XDSI_TIME1_HSA(vm->hsync_len);
>> > +		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
>> > +	}
>> > +
>> > +	reg = XDSI_TIME4_VFP(vm->vfront_porch) |
>> > +	      XDSI_TIME4_VBP(vm->vback_porch) |
>> > +	      XDSI_TIME4_VSA(vm->vsync_len);
>> > +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
>> > +
>> > +	reg = XDSI_TIME3_HFP(vm->hfront_porch) |
>> > +	      XDSI_TIME3_HBP(vm->hback_porch);
>> > +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
>> > +
>> > +	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
>> > +		(dsi->mul_factor) / 100);
>>
>> Remove unneeded ().
OK.
>>
>> > +	/*
>> > +	 * The HACT parameter received from panel timing values should be
>> > +	 * divisible by 4. The reason for this is, the word count given as
>> > +	 * input to DSI controller is HACT * mul_factor. The mul_factor is
>> > +	 * 3, 2.25, 2.25, 2 respectively for RGB888, RGB666_L, RGB666_P and
>> > +	 * RGB565.
>> > +	 * e.g. for RGB666_L color format and 1080p, the word count is
>> > +	 * 1920*2.25 = 4320 which is divisible by 4 and it is a valid input
>> > +	 * to DSI controller. Based on this 2.25 mul factor, we come up with
>> > +	 * the division factor of (XDSI_HACT_MULTIPLIER) as 4 for checking
>> > +	 */
>> > +	if ((vm->hactive & XDSI_HACT_MULTIPLIER) != 0)
>>
>> if (!(vm->hactive & XDSI_HACT_MULTIPLIER))?
>>
>> > +		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
>>
>> dev_warn() better come with more details. Probably it can mention that
>> hactive is not aligned along with its value.
OK.
>>
>> > +
>> > +	reg = XDSI_TIME2_HACT((vm->hactive) * (dsi->mul_factor) / 100) |
>>
>> Ditto. No ().
>>
>> > +	      XDSI_TIME2_VACT(vm->vactive);
>> > +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME2, reg);
>> > +
>> > +	dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive);
>> > +}
>> > +
>> > +/**
>> > + * xlnx_dsi_set_display_enable - Enables the DSI Tx IP core enable
>> > + * register bit
>> > + * @dsi: Xilinx DSI structure
>> > + *
>> > + * This function takes the DSI strucure and enables the core enable
>> > +bit
>> > + * of core configuration register.
>> > + */
>> > +static void xlnx_dsi_set_display_enable(struct xlnx_dsi *dsi) {
>> > +	u32 reg;
>> > +
>> > +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
>> > +	reg |= XDSI_CCR_COREENB;
>> > +
>>
>> Nit. It can be less lines by removing this empty line and setting the
>> reg at declarartion an so on.
OK.
>>
>> > +	xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
>> > +	dev_dbg(dsi->dev, "MIPI DSI Tx controller is enabled.\n"); }
>> > +
>> > +/**
>> > + * xlnx_dsi_set_display_disable - Disable the DSI Tx IP core enable
>> > + * register bit
>> > + * @dsi: Xilinx DSI structure
>> > + *
>> > + * This function takes the DSI strucure and disables the core
>> > +enable bit
>> > + * of core configuration register.
>> > + */
>> > +static void xlnx_dsi_set_display_disable(struct xlnx_dsi *dsi) {
>> > +	u32 reg;
>> > +
>> > +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
>> > +	reg &= ~XDSI_CCR_COREENB;
>> > +
>>
>> Ditto.
>>
>> > +	xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
>> > +	dev_dbg(dsi->dev, "DSI Tx is disabled. reset regs to default
>> > +values\n"); }
>> > +
>> > +/**
>> > + * xlnx_dsi_host_transfer - transfer command to panel
>> > + * @host: mipi dsi host structure
>> > + * @msg: mipi dsi msg with type, length and data
>> > + *
>> > + * This function is valid only in command mode.
>> > + * It checks the command fifo empty status and writes into
>> > + * data or cmd register and waits for the completion status.
>> > + *
>> > + * Return:	number of bytes, on success and error number on failure
>>
>> Nit. Extra spaces.
>>
>> > + */
>> > +static ssize_t xlnx_dsi_host_transfer(struct mipi_dsi_host *host,
>> > +				      const struct mipi_dsi_msg *msg) {
>> > +	struct xlnx_dsi *dsi = host_to_dsi(host);
>> > +	u32 data0, data1, cmd0, status, val;
>> > +	const char *tx_buf = msg->tx_buf;
>> > +
>> > +	if (!(xlnx_dsi_readl(dsi->iomem, XDSI_CCR) & (XDSI_CCR_COREENB |
>> > +						      XDSI_CCR_CMDMODE))) {
>>
>> This passes when one of these 2 bits is set, and I don't think it's right.
OK.
>>
>> > +		dev_err(dsi->dev, "dsi command mode not enabled\n");
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	if (msg->type == MIPI_DSI_DCS_LONG_WRITE) {
>> > +		status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
>> > +					    ((val & XDSI_STR_LPKT_MASK) ==
>> > +					     XDSI_STR_LPKT_MASK), 1,
>>
>> Please remove unneeded (). Same for rest below.
OK.
>>
>> > +					    XDSI_CMD_TIMEOUT_VAL);
>>
>> It'd be nice if these delay / timeout values are described..
I will add the description or comment
>>
>> > +		if (status) {
>> > +			dev_err(dsi->dev, "long cmd fifo not empty!\n");
>> > +			return -ETIMEDOUT;
>>
>> Isn't this checking if the controller is ready for long packet transaction?
>> Then maybe -EBUSY is better. For timeout, the 'status' can propagate.
>> Same for below.
OK.
>>
>> > +		}
>> > +		data0 = tx_buf[0] | (tx_buf[1] << 8) | (tx_buf[2] << 16) |
>> > +			(tx_buf[3] << 24);
>> > +		data1 = tx_buf[4] | (tx_buf[5] << 8);
>> > +		cmd0 = msg->type | (MIPI_DSI_DCS_READ << 8);
>> > +
>> > +		xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data0);
>> > +		xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data1);
>> > +		xlnx_dsi_writel(dsi->iomem, XDSI_CMD, cmd0);
>> > +	} else {
>>
>> I'm not familiar with DSI transaction handling, but is it safe to
>> assume all other than long write is short packet write (param, without
>param)?
I can add check for short packet.
>>
>> > +		data0 = tx_buf[0];
>>
>> This line can be removed.
OK.
>>
>> > +		if (msg->type == MIPI_DSI_DCS_SHORT_WRITE_PARAM)
>> > +			data0 = MIPI_DSI_DCS_SHORT_WRITE_PARAM |
>> > +				(tx_buf[0] << 8) | (tx_buf[1] << 16);
>> > +		else
>> > +			data0 = MIPI_DSI_DCS_SHORT_WRITE | (tx_buf[0] <<
>8);
>>
>> I'd put () for this if - else statments, but it may be just me.
>>
>>
>> Then, this can mvoe to right before xlnx_dsi_writel() below.
>>
>> > +
>> > +		status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
>> > +					    ((val & XDSI_STR_RDY_SHPKT) ==
>> > +					     XDSI_STR_RDY_SHPKT), 1,
>> > +					    XDSI_CMD_TIMEOUT_VAL);
>> > +		if (status) {
>> > +			dev_err(dsi->dev, "short cmd fifo not empty\n");
>> > +			return -ETIMEDOUT;
>> > +		}
>> > +		xlnx_dsi_writel(dsi->iomem, XDSI_CMD, data0);
>> > +	}
>> > +
>> > +	status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
>> > +				    (!(val & XDSI_STR_CMD_EXE_PGS)), 1,
>> > +				    XDSI_CMD_TIMEOUT_VAL);
>> > +	if (status) {
>> > +		dev_err(dsi->dev, "cmd time out\n");
>> > +		return -ETIMEDOUT;
>> > +	}
>> > +
>> > +	return msg->tx_len;
>> > +}
>> > +
>> > +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
>> > +				struct mipi_dsi_device *device) {
>> > +	u32 panel_lanes;
>> > +	struct xlnx_dsi *dsi = host_to_dsi(host);
>> > +
>> > +	panel_lanes = device->lanes;
>>
>> I'd do this above,
>>
>> 	u32 panel_lanes = device->lanes
>>
>> > +	dsi->mode_flags = device->mode_flags;
>> > +	dsi->panel_node = device->dev.of_node;
>> > +
>> > +	if (panel_lanes != dsi->lanes) {
>> > +		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
>> > +			panel_lanes, dsi->lanes);
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	if (device->format != dsi->format) {
>> > +		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI =
>%d\n",
>> > +			device->format, dsi->format);
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	if (dsi->connector.dev)
>> > +		drm_helper_hpd_irq_event(dsi->connector.dev);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static int xlnx_dsi_host_detach(struct mipi_dsi_host *host,
>> > +				struct mipi_dsi_device *device) {
>> > +	struct xlnx_dsi *dsi = host_to_dsi(host);
>> > +
>> > +	if (dsi->panel) {
>> > +		drm_panel_detach(dsi->panel);
>> > +		dsi->panel = NULL;
>> > +	}
>> > +
>> > +	if (dsi->connector.dev)
>> > +		drm_helper_hpd_irq_event(dsi->connector.dev);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
>> > +	.attach = xlnx_dsi_host_attach,
>> > +	.detach = xlnx_dsi_host_detach,
>> > +	.transfer = xlnx_dsi_host_transfer, };
>> > +
>> > +static int xlnx_dsi_connector_dpms(struct drm_connector *connector,
>> > +				   int mode)
>> > +{
>> > +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
>> > +	int ret;
>> > +
>> > +	dev_dbg(dsi->dev, "connector dpms state: %d\n", mode);
>> > +
>> > +	switch (mode) {
>> > +	case DRM_MODE_DPMS_ON:
>> > +		ret = drm_panel_prepare(dsi->panel);
>> > +		if (ret < 0) {
>> > +			dev_err(dsi->dev, "DRM panel not found\n");
>> > +			return ret;
>> > +		}
>> > +
>> > +		ret = drm_panel_enable(dsi->panel);
>> > +		if (ret < 0) {
>> > +			drm_panel_unprepare(dsi->panel);
>> > +			dev_err(dsi->dev, "DRM panel not enabled\n");
>> > +			return ret;
>> > +		}
>> > +		break;
>> > +	default:
>> > +		drm_panel_disable(dsi->panel);
>> > +		drm_panel_unprepare(dsi->panel);
>> > +		break;
>> > +	}
>> > +
>> > +	return drm_helper_connector_dpms(connector, mode); }
>> > +
>> > +static enum drm_connector_status
>> > +xlnx_dsi_detect(struct drm_connector *connector, bool force) {
>> > +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
>> > +
>> > +	if (!dsi->panel) {
>> > +		dsi->panel = of_drm_find_panel(dsi->panel_node);
>> > +		if (dsi->panel) {
>> > +			drm_panel_attach(dsi->panel, &dsi->connector);
>>
>> Above are quite static, so can't it be done in host attach? Or do you
>> see more dynamic use cases?
I will check the possibility.
>>
>> > +			if (dsi->cmdmode) {
>> > +				xlnx_dsi_writel(dsi->iomem, XDSI_CCR,
>> > +						XDSI_CCR_CMDMODE |
>> > +						XDSI_CCR_COREENB);
>> > +				drm_panel_prepare(dsi->panel);
>>
>> Just to confirm, is this not needed for non-command mode? And how is
>> this aligned with same call in dpms(), meaning the panel will be
>> prepared twice in command mode.
There is a GUI option in IP design to select command mode support.
I will check and ensure panel prepare won't happen twice.
>>
>> > +				xlnx_dsi_writel(dsi->iomem, XDSI_CCR, 0);
>> > +			}
>> > +		}
>> > +	} else if (!dsi->panel_node) {
>> > +		xlnx_dsi_connector_dpms(connector, DRM_MODE_DPMS_OFF);
>> > +		drm_panel_detach(dsi->panel);
>> > +		dsi->panel = NULL;
>> > +	}
>> > +
>> > +	if (dsi->panel)
>> > +		return connector_status_connected;
>> > +
>> > +	return connector_status_disconnected; }
>> > +
>> > +static void xlnx_dsi_connector_destroy(struct drm_connector
>> > +*connector) {
>> > +	drm_connector_unregister(connector);
>> > +	drm_connector_cleanup(connector);
>> > +	connector->dev = NULL;
>> > +}
>> > +
>> > +static const struct drm_connector_funcs xlnx_dsi_connector_funcs = {
>> > +	.detect = xlnx_dsi_detect,
>> > +	.fill_modes = drm_helper_probe_single_connector_modes,
>> > +	.destroy = xlnx_dsi_connector_destroy,
>> > +	.reset			= drm_atomic_helper_connector_reset,
>>
>> This better indent more consistently. Please do it through the entire file.
>>
>> > +	.atomic_duplicate_state	=
>drm_atomic_helper_connector_duplicate_state,
>> > +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>> > +	.dpms = xlnx_dsi_connector_dpms,
>> > +};
>> > +
>> > +static int xlnx_dsi_get_modes(struct drm_connector *connector) {
>> > +	struct xlnx_dsi *dsi = connector_to_dsi(connector);
>> > +
>> > +	if (dsi->panel)
>> > +		return drm_panel_get_modes(dsi->panel, connector);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static struct drm_encoder *
>> > +xlnx_dsi_best_encoder(struct drm_connector *connector) {
>> > +	return &(connector_to_dsi(connector)->encoder);
>> > +}
>> > +
>> > +static const struct drm_connector_helper_funcs
>> > +xlnx_dsi_connector_helper_funcs = {
>> > +	.get_modes = xlnx_dsi_get_modes,
>> > +	.best_encoder = xlnx_dsi_best_encoder, };
>> > +
>> > +static int xlnx_dsi_create_connector(struct drm_encoder *encoder) {
>> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
>> > +	struct drm_connector *connector = &dsi->connector;
>> > +	struct drm_device *drm = encoder->dev;
>> > +	int ret;
>> > +
>> > +	connector->polled = DRM_CONNECTOR_POLL_HPD;
>> > +
>> > +	ret = drm_connector_init(encoder->dev, connector,
>> > +				 &xlnx_dsi_connector_funcs,
>> > +				 DRM_MODE_CONNECTOR_DSI);
>> > +	if (ret) {
>> > +		dev_err(dsi->dev, "Failed to initialize connector with drm\n");
>> > +		return ret;
>> > +	}
>> > +
>> > +	drm_connector_helper_add(connector,
>&xlnx_dsi_connector_helper_funcs);
>> > +	drm_connector_attach_encoder(connector, encoder);
>> > +	if (!drm->registered)
>> > +		return 0;
>> > +
>> > +	drm_connector_register(connector);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +/**
>> > + * xlnx_dsi_atomic_mode_set -  derive the DSI timing parameters
>> > + *
>> > + * @encoder: pointer to Xilinx DRM encoder
>> > + * @crtc_state: Pointer to drm core crtc state
>> > + * @connector_state: DSI connector drm state
>> > + *
>> > + * This function derives the DSI IP timing parameters from the
>> > +timing
>> > + * values given in the attached panel driver.
>> > + */
>> > +static void
>> > +xlnx_dsi_atomic_mode_set(struct drm_encoder *encoder,
>> > +			 struct drm_crtc_state *crtc_state,
>> > +				 struct drm_connector_state *connector_state)
>{
>> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
>> > +	struct videomode *vm = &dsi->vm;
>> > +	struct drm_display_mode *m = &crtc_state->adjusted_mode;
>> > +
>> > +	vm->hactive = m->hdisplay;
>> > +	vm->vactive = m->vdisplay;
>> > +	vm->vfront_porch = m->vsync_start - m->vdisplay;
>> > +	vm->vback_porch = m->vtotal - m->vsync_end;
>> > +	vm->vsync_len = m->vsync_end - m->vsync_start;
>> > +	vm->hfront_porch = m->hsync_start - m->hdisplay;
>> > +	vm->hback_porch = m->htotal - m->hsync_end;
>> > +	vm->hsync_len = m->hsync_end - m->hsync_start;
>> > +	xlnx_dsi_set_display_mode(dsi);
>>
>> As commented above, this doesn't have to be re-directed through videomode.
>> Then the function can be inlined here.
OK.
>>
>> > +}
>> > +
>> > +static void xlnx_dsi_disable(struct drm_encoder *encoder) {
>> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
>> > +
>> > +	xlnx_dsi_set_display_disable(dsi);
>>
>> 	xlnx_dsi_set_display_disable(encoder_to_dsi(encoder));
>>
>> Maybe even this wrapper can be removed?
OK.
>>
>> > +}
>> > +
>> > +static void xlnx_dsi_enable(struct drm_encoder *encoder) {
>> > +	struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
>> > +
>> > +	xlnx_dsi_set_display_enable(dsi);
>>
>> Ditto.
>>
>> > +}
>> > +
>> > +static const struct drm_encoder_helper_funcs
>> > +xlnx_dsi_encoder_helper_funcs = {
>> > +	.atomic_mode_set = xlnx_dsi_atomic_mode_set,
>> > +	.enable = xlnx_dsi_enable,
>> > +	.disable = xlnx_dsi_disable,
>> > +};
>> > +
>> > +static const struct drm_encoder_funcs xlnx_dsi_encoder_funcs = {
>> > +	.destroy = drm_encoder_cleanup,
>> > +};
>> > +
>> > +static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi) {
>> > +	struct device *dev = dsi->dev;
>> > +	struct device_node *node = dev->of_node;
>> > +	int ret;
>> > +	u32 datatype;
>> > +	static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225,
>> > +200};
>> > +
>> > +	dsi->dphy_clk_200M = devm_clk_get(dev, "dphy_clk_200M");
>> > +	if (IS_ERR(dsi->dphy_clk_200M)) {
>> > +		ret = PTR_ERR(dsi->dphy_clk_200M);
>> > +		dev_err(dev, "failed to get dphy_clk_200M %d\n", ret);
>> > +		return ret;
>> > +	}
>> > +
>> > +	dsi->video_aclk = devm_clk_get(dev, "s_axis_aclk");
>> > +	if (IS_ERR(dsi->video_aclk)) {
>> > +		ret = PTR_ERR(dsi->video_aclk);
>> > +		dev_err(dev, "failed to get video_clk %d\n", ret);
>> > +		return ret;
>> > +	}
>> > +
>> > +	ret = of_property_read_u32(node, "xlnx,dsi-num-lanes", &dsi->lanes);
>> > +	if (ret < 0) {
>> > +		dev_err(dsi->dev, "missing xlnx,dsi-num-lanes property\n");
>> > +		return ret;
>> > +	}
>> > +	if (dsi->lanes > 4 || dsi->lanes < 1) {
>> > +		dev_err(dsi->dev, "%d lanes : invalid lanes\n", dsi->lanes);
>> > +		return -EINVAL;
>> > +	}
>>
>> Nit. An empty line here would be helpful for readability.
OK.
>>
>> > +	ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);
>> > +	if (ret < 0) {
>> > +		dev_err(dsi->dev, "missing xlnx,dsi-data-type property\n");
>> > +		return ret;
>> > +	}
>> > +	dsi->format = datatype;
>> > +	if (datatype > MIPI_DSI_FMT_RGB565) {
>> > +		dev_err(dsi->dev, "Invalid xlnx,dsi-data-type string\n");
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	/*
>> > +	 * multiplication factor used for HACT, based on data type.
>> > +	 *
>> > +	 * e.g. for RGB666_L datatype and 1920x1080 resolution,
>> > +	 * the Hact (WC) would be as follows -
>> > +	 * 1920 pixels * 18 bits per pixel / 8 bits per byte
>> > +	 * = 1920 pixels * 2.25 bytes per pixel = 4320 bytes.
>> > +	 *
>> > +	 * Data Type - Multiplication factor
>> > +	 * RGB888    - 3
>> > +	 * RGB666_L  - 2.25
>> > +-	 * RGB666_P  - 2.25
>> > +	 * RGB565    - 2
>> > +	 *
>> > +	 * Since the multiplication factor maybe a floating number,
>> > +	 * a 100x multiplication factor is used.
>> > +	 */
>> > +	dsi->mul_factor = xdsi_mul_fact[datatype];
>> > +
>> > +	dsi->cmdmode = of_property_read_bool(node, "xlnx,dsi-cmd-mode");
>> > +
>> > +	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
>> > +	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
>> > +	dev_dbg(dsi->dev, "DSI controller cmd mode = %d\n", dsi->cmdmode);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static int xlnx_dsi_bind(struct device *dev, struct device *master,
>> > +			 void *data)
>> > +{
>> > +	struct xlnx_dsi *dsi = dev_get_drvdata(dev);
>> > +	struct drm_encoder *encoder = &dsi->encoder;
>> > +	struct drm_device *drm_dev = data;
>> > +	int ret;
>> > +
>> > +	drm_encoder_init(drm_dev, encoder, &xlnx_dsi_encoder_funcs,
>> > +			 DRM_MODE_ENCODER_DSI, NULL);
>> > +	drm_encoder_helper_add(encoder, &xlnx_dsi_encoder_helper_funcs);
>> > +
>> > +	encoder->possible_crtcs = 1;
>> > +
>> > +	ret = xlnx_dsi_create_connector(encoder);
>> > +	if (ret) {
>> > +		dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret);
>> > +		drm_encoder_cleanup(encoder);
>> > +		return ret;
>> > +	}
>> > +
>> > +	ret = mipi_dsi_host_register(&dsi->dsi_host);
>> > +	if (ret < 0) {
>>
>> Just nit. if (ret) would do.
>>
>> > +		dev_err(dsi->dev, "failed to register DSI host: %d\n", ret);
>> > +		xlnx_dsi_connector_destroy(&dsi->connector);
>> > +		drm_encoder_cleanup(encoder);
>> > +		return ret;
>> > +	}
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static void xlnx_dsi_unbind(struct device *dev, struct device *master,
>> > +			    void *data)
>> > +{
>> > +	struct xlnx_dsi *dsi = dev_get_drvdata(dev);
>> > +
>> > +	xlnx_dsi_disable(&dsi->encoder);
>> > +	mipi_dsi_host_unregister(&dsi->dsi_host);
>> > +}
>> > +
>> > +static const struct component_ops xlnx_dsi_component_ops = {
>> > +	.bind	= xlnx_dsi_bind,
>> > +	.unbind	= xlnx_dsi_unbind,
>> > +};
>> > +
>> > +static int xlnx_dsi_probe(struct platform_device *pdev) {
>> > +	struct device *dev = &pdev->dev;
>> > +	struct resource *res;
>> > +	struct xlnx_dsi *dsi;
>> > +	int ret;
>> > +	unsigned long rate;
>> > +
>> > +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
>> > +	if (!dsi)
>> > +		return -ENOMEM;
>> > +
>> > +	dsi->dsi_host.ops = &xlnx_dsi_ops;
>> > +	dsi->dsi_host.dev = dev;
>> > +	dsi->dev = dev;
>> > +
>> > +	ret = xlnx_dsi_parse_dt(dsi);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > +	dsi->iomem = devm_ioremap_resource(dev, res);
>> > +	if (IS_ERR(dsi->iomem)) {
>> > +		dev_err(dev, "failed to remap io region\n");
>> > +		return PTR_ERR(dsi->iomem);
>> > +	}
>> > +	platform_set_drvdata(pdev, dsi);
>> > +
>> > +	ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ);
>> > +	if (ret) {
>> > +		dev_err(dev, "failed to set dphy clk rate %d\n", ret);
>> > +		return ret;
>> > +	}
>> > +
>> > +	ret = clk_prepare_enable(dsi->dphy_clk_200M);
>> > +	if (ret) {
>> > +		dev_err(dev, "failed to enable dphy clk %d\n", ret);
>> > +		return ret;
>> > +	}
>> > +
>> > +	rate = clk_get_rate(dsi->dphy_clk_200M);
>> > +	if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) {
>>
>> A brief comment for this line wold be helpful.
OK. I will add comment.
>>
>> > +		dev_err(dev, "Error DPHY clock = %lu\n", rate);
>> > +		ret = -EINVAL;
>> > +		goto err_disable_dphy_clk;
>> > +	}
>> > +
>> > +	ret = clk_prepare_enable(dsi->video_aclk);
>> > +	if (ret) {
>> > +		dev_err(dev, "failed to enable video clk %d\n", ret);
>> > +		goto err_disable_dphy_clk;
>> > +	}
>> > +
>> > +	ret = component_add(dev, &xlnx_dsi_component_ops);
>
>The driver should expose the DSI-TX as a drm_bridge instead of using the
>component framework. You shouldn't register a drm_encoder, and I don't think
>you should register a drm_connector either. Only bridge operations should be
>exposed, and the drm_bridge .attach() operation should return an error when
>DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. The top-level driver using
>this bridge should create the drm_encoder and drm_connector, most likely using
>drm_bridge_connector_init() to create the connector.
>
>> > +	if (ret < 0)
>>
>> I prefer if (ret) where possible, but it may be just me. I let you decide.
>>
>> > +		goto err_disable_video_clk;
>> > +
>> > +	return ret;
>>
>> return 0;? but up to you.
>>
>> > +
>> > +err_disable_video_clk:
>> > +	clk_disable_unprepare(dsi->video_aclk);
>> > +err_disable_dphy_clk:
>> > +	clk_disable_unprepare(dsi->dphy_clk_200M);
>> > +	return ret;
>> > +}
>>
>> At least, this initial one better come with a complete display
>> pipeline, ex crtc / plane are missing. This won't be functional without those.
>> I believe Laurent was planning to make the xlnx drm layer as a
>> library, so hopefully this can be a good starting point and come with
>> drivers aligned with it.
>>
>> > +
>> > +static int xlnx_dsi_remove(struct platform_device *pdev) {
>> > +	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
>> > +
>> > +	component_del(&pdev->dev, &xlnx_dsi_component_ops);
>> > +	clk_disable_unprepare(dsi->video_aclk);
>> > +	clk_disable_unprepare(dsi->dphy_clk_200M);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static const struct of_device_id xlnx_dsi_of_match[] = {
>> > +	{ .compatible = "xlnx-dsi", },
>>
>> This should change. I believe it should be something like, xlnx,<dsi
>> ip name>-<version>
OK. I will update in v2 patch.
>>
>> > +	{ /* end of table */ },
>> > +};
>> > +MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match);
>> > +
>> > +static struct platform_driver dsi_driver = {
>> > +	.probe = xlnx_dsi_probe,
>> > +	.remove = xlnx_dsi_remove,
>> > +	.driver = {
>> > +		.name = "xlnx-dsi",
>> > +		.of_match_table = xlnx_dsi_of_match,
>> > +	},
>> > +};
>> > +
>> > +module_platform_driver(dsi_driver);
>> > +
>> > +MODULE_AUTHOR("Xilinx, Inc.");
>> > +MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver");
>> > +MODULE_LICENSE("GPL v2");
>
>--
>Regards,
>
>Laurent Pinchart

Thanks,
GVRao
 

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Laurent Pinchart]

Hi GVRao,

On Sun, May 31, 2020 at 05:41:50PM +0000, Venkateshwar Rao Gannavarapu wrote:
> On Sunday, May 24, 2020 8:38 AM, Laurent Pinchart wrote:
> > On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> >> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> >>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> >>> data from AXI-4 stream interface.
> >>>
> >>> It supports upto 4 lanes, optional register interface for the DPHY,
> >>
> >> I don't see the register interface for dphy support.
> >
> > I think the D-PHY should be supported through a PHY driver, as it seems to be
> > shared between different subsystems.
>
> IP has the provision to read DPHY register for debug purpose only.
> No programming of DPHY is required in subsystem.

Do you know if this is the same D-PHY as used in the CSI2-RX subsystem ?

> >>> multiple RGB color formats, command mode and video mode.
> >>> This is a MIPI-DSI host driver and provides DSI bus for panels.
> >>> This driver also helps to communicate with its panel using panel
> >>> framework.
> >>>
> >>> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> >>> ---
> >>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> >>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
> >>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> >
> > Daniel Vetter has recently expressed his opiion that bridge drivers should go to
> > drivers/gpu/drm/bridge/. It would then be drivers/gpu/drm/bridge/xlnx/. I don't
> > have a strong opinion myself.
> >
> >>>  3 files changed, 768 insertions(+)
> >>>  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

-- 
Regards,

Laurent Pinchart

RE: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Venkateshwar Rao Gannavarapu]

Hi Laurent,

Thanks for the review. 
Please see my comments about D-PHY and bridge driver implementation.

>-----Original Message-----
>From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>Sent: Sunday, June 7, 2020 7:55 AM
>To: Venkateshwar Rao Gannavarapu <VGANNAVA@xilinx.com>
>Cc: Hyun Kwon <hyunk@xilinx.com>; dri-devel@lists.freedesktop.org;
>airlied@linux.ie; daniel@ffwll.ch; linux-kernel@vger.kernel.org; Sandip Kothari
><sandipk@xilinx.com>
>Subject: Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem
>
>Hi GVRao,
>
>On Sun, May 31, 2020 at 05:41:50PM +0000, Venkateshwar Rao Gannavarapu
>wrote:
>> On Sunday, May 24, 2020 8:38 AM, Laurent Pinchart wrote:
>> > On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
>> >> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu
>wrote:
>> >>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
>> >>> data from AXI-4 stream interface.
>> >>>
>> >>> It supports upto 4 lanes, optional register interface for the
>> >>> DPHY,
>> >>
>> >> I don't see the register interface for dphy support.
>> >
>> > I think the D-PHY should be supported through a PHY driver, as it
>> > seems to be shared between different subsystems.
>>
>> IP has the provision to read DPHY register for debug purpose only.
>> No programming of DPHY is required in subsystem.
>
>Do you know if this is the same D-PHY as used in the CSI2-RX subsystem ?
 
Same D-PHY core has been used in MIPI CSI2 RXSS, but with different configuration.
>
>> >>> multiple RGB color formats, command mode and video mode.
>> >>> This is a MIPI-DSI host driver and provides DSI bus for panels.
>> >>> This driver also helps to communicate with its panel using panel
>> >>> framework.
>> >>>
>> >>> Signed-off-by: Venkateshwar Rao Gannavarapu
>> >>> <venkateshwar.rao.gannavarapu@xilinx.com>
>> >>> ---
>> >>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
>> >>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
>> >>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755
>> >>> ++++++++++++++++++++++++++++++++++++++++
>> >
>> > Daniel Vetter has recently expressed his opiion that bridge drivers
>> > should go to drivers/gpu/drm/bridge/. It would then be
>> > drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.

The DSI-TX subsystem IP block is not a bridge driver.
The DSI-TX subsystem IP block itself contains all the drm encoder/connector
functionality and it’s the last node in display pipe line. I didn't see any hard
requirement to implement it into bridge driver and I see many DSI drivers are
implemented as encoder drivers.
Xilinx PL DRM encoder drivers are implemented in modular approach so that
they can work with any CRTC driver which handles the DMA calls.
So, at this stage we want to upstream as encoder driver only.
>> >
>> >>>  3 files changed, 768 insertions(+)  create mode 100644
>> >>> drivers/gpu/drm/xlnx/xlnx_dsi.c
>
>[snip]
>
>--
>Regards,
>
>Laurent Pinchart

Regards,
GVRao
 

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Laurent Pinchart]

Hi GVRao,

Sorry for the delayed reply.

On Tue, Jun 09, 2020 at 02:48:25AM +0000, Venkateshwar Rao Gannavarapu wrote:
> Hi Laurent,
> 
> Thanks for the review. 
> Please see my comments about D-PHY and bridge driver implementation.
> 
> On Sunday, June 7, 2020 7:55 AM, Laurent Pinchart wrote:
> > On Sun, May 31, 2020 at 05:41:50PM +0000, Venkateshwar Rao Gannavarapu wrote:
> >> On Sunday, May 24, 2020 8:38 AM, Laurent Pinchart wrote:
> >>> On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> >>>> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> >>>>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> >>>>> data from AXI-4 stream interface.
> >>>>>
> >>>>> It supports upto 4 lanes, optional register interface for the
> >>>>> DPHY,
> >>>>
> >>>> I don't see the register interface for dphy support.
> >>>
> >>> I think the D-PHY should be supported through a PHY driver, as it
> >>> seems to be shared between different subsystems.
> >>
> >> IP has the provision to read DPHY register for debug purpose only.
> >> No programming of DPHY is required in subsystem.
> >
> > Do you know if this is the same D-PHY as used in the CSI2-RX subsystem ?
>  
> Same D-PHY core has been used in MIPI CSI2 RXSS, but with different configuration.
>
> >>>>> multiple RGB color formats, command mode and video mode.
> >>>>> This is a MIPI-DSI host driver and provides DSI bus for panels.
> >>>>> This driver also helps to communicate with its panel using panel
> >>>>> framework.
> >>>>>
> >>>>> Signed-off-by: Venkateshwar Rao Gannavarapu
> >>>>> <venkateshwar.rao.gannavarapu@xilinx.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> >>>>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
> >>>>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> >>>
> >>> Daniel Vetter has recently expressed his opiion that bridge drivers
> >>> should go to drivers/gpu/drm/bridge/. It would then be
> >>> drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
> 
> The DSI-TX subsystem IP block is not a bridge driver.
> The DSI-TX subsystem IP block itself contains all the drm encoder/connector
> functionality and it’s the last node in display pipe line.

The DSI-TX subsystem IP block is indeed an encoder from a hardware point
of view, but it's not necessarily the last block in the display
pipeline. While the output of the IP core goes of the the SoC, tt would
be entirely feasible to connect it to a DP to HDMI bridge on the board,
such as the ANX7737 ([1]) for instance. This is why we're pushing for
all encoder (from a hardware point of view) drivers to be implemented as
DRM bridge, in order to make them usable in different display pipelines,
without hardcoding the assumption they will be the last encoder in the
pipeline.

> I didn't see any hard
> requirement to implement it into bridge driver and I see many DSI drivers are
> implemented as encoder drivers.
> Xilinx PL DRM encoder drivers are implemented in modular approach so that
> they can work with any CRTC driver which handles the DMA calls.
> So, at this stage we want to upstream as encoder driver only.
>
> >>>>>  3 files changed, 768 insertions(+)  create mode 100644
> >>>>> drivers/gpu/drm/xlnx/xlnx_dsi.c

[1] https://www.analogix.com/en/products/convertersbridges/anx7737

-- 
Regards,

Laurent Pinchart

RE: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Venkateshwar Rao Gannavarapu]

Hi Laurent,

Thanks for the comment.

>-----Original Message-----
>From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>Sent: Wednesday, June 17, 2020 3:18 AM
>To: Venkateshwar Rao Gannavarapu <VGANNAVA@xilinx.com>
>Cc: Hyun Kwon <hyunk@xilinx.com>; dri-devel@lists.freedesktop.org;
>airlied@linux.ie; daniel@ffwll.ch; linux-kernel@vger.kernel.org; Sandip Kothari
><sandipk@xilinx.com>; Vishal Sagar <vsagar@xilinx.com>
>Subject: Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem
>
>Hi GVRao,
>
>Sorry for the delayed reply.
>
>On Tue, Jun 09, 2020 at 02:48:25AM +0000, Venkateshwar Rao Gannavarapu
>wrote:
>> Hi Laurent,
>>
>> Thanks for the review.
>> Please see my comments about D-PHY and bridge driver implementation.
>>
>> On Sunday, June 7, 2020 7:55 AM, Laurent Pinchart wrote:
>> > On Sun, May 31, 2020 at 05:41:50PM +0000, Venkateshwar Rao
>Gannavarapu wrote:
>> >> On Sunday, May 24, 2020 8:38 AM, Laurent Pinchart wrote:
>> >>> On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
>> >>>> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu
>wrote:
>> >>>>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display
>> >>>>> video data from AXI-4 stream interface.
>> >>>>>
>> >>>>> It supports upto 4 lanes, optional register interface for the
>> >>>>> DPHY,
>> >>>>
>> >>>> I don't see the register interface for dphy support.
>> >>>
>> >>> I think the D-PHY should be supported through a PHY driver, as it
>> >>> seems to be shared between different subsystems.
>> >>
>> >> IP has the provision to read DPHY register for debug purpose only.
>> >> No programming of DPHY is required in subsystem.
>> >
>> > Do you know if this is the same D-PHY as used in the CSI2-RX subsystem ?
>>
>> Same D-PHY core has been used in MIPI CSI2 RXSS, but with different
>configuration.
>>
>> >>>>> multiple RGB color formats, command mode and video mode.
>> >>>>> This is a MIPI-DSI host driver and provides DSI bus for panels.
>> >>>>> This driver also helps to communicate with its panel using panel
>> >>>>> framework.
>> >>>>>
>> >>>>> Signed-off-by: Venkateshwar Rao Gannavarapu
>> >>>>> <venkateshwar.rao.gannavarapu@xilinx.com>
>> >>>>> ---
>> >>>>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
>> >>>>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
>> >>>>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755
>> >>>>> ++++++++++++++++++++++++++++++++++++++++
>> >>>
>> >>> Daniel Vetter has recently expressed his opiion that bridge
>> >>> drivers should go to drivers/gpu/drm/bridge/. It would then be
>> >>> drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
>>
>> The DSI-TX subsystem IP block is not a bridge driver.
>> The DSI-TX subsystem IP block itself contains all the drm
>> encoder/connector functionality and it’s the last node in display pipe line.
>
>The DSI-TX subsystem IP block is indeed an encoder from a hardware point of
>view, but it's not necessarily the last block in the display pipeline. While the
>output of the IP core goes of the the SoC, tt would be entirely feasible to
>connect it to a DP to HDMI bridge on the board, such as the ANX7737 ([1]) for
>instance. This is why we're pushing for all encoder (from a hardware point of
>view) drivers to be implemented as DRM bridge, in order to make them usable
>in different display pipelines, without hardcoding the assumption they will be
>the last encoder in the pipeline.

Thanks for the details.
I can understand it as SoC requirement where crtc is fixed, but as a FPGA product
encoder driver should work with any of crtc driver.  And I still not see bridge implementation as hard requirement.
Could you please explain what problem we face, if implemented as encoder driver.
>
>> I didn't see any hard
>> requirement to implement it into bridge driver and I see many DSI
>> drivers are implemented as encoder drivers.
>> Xilinx PL DRM encoder drivers are implemented in modular approach so
>> that they can work with any CRTC driver which handles the DMA calls.
>> So, at this stage we want to upstream as encoder driver only.
>>
>> >>>>>  3 files changed, 768 insertions(+)  create mode 100644
>> >>>>> drivers/gpu/drm/xlnx/xlnx_dsi.c
>
>[1] https://www.analogix.com/en/products/convertersbridges/anx7737
>
>--
>Regards,
>
>Laurent Pinchart

Regards,
GVRao
 

Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Laurent Pinchart]

Hi GVRao,

Thank you for pinging me privately.

On Mon, Jun 22, 2020 at 02:19:22PM +0000, Venkateshwar Rao Gannavarapu wrote:
> On Wednesday, June 17, 2020 3:18 AM, Laurent Pinchart wrote:
> > On Tue, Jun 09, 2020 at 02:48:25AM +0000, Venkateshwar Rao Gannavarapu wrote:
> >> Hi Laurent,
> >>
> >> Thanks for the review.
> >> Please see my comments about D-PHY and bridge driver implementation.
> >>
> >> On Sunday, June 7, 2020 7:55 AM, Laurent Pinchart wrote:
> >>> On Sun, May 31, 2020 at 05:41:50PM +0000, Venkateshwar Rao Gannavarapu wrote:
> >>>> On Sunday, May 24, 2020 8:38 AM, Laurent Pinchart wrote:
> >>>>> On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> >>>>>> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> >>>>>>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display
> >>>>>>> video data from AXI-4 stream interface.
> >>>>>>>
> >>>>>>> It supports upto 4 lanes, optional register interface for the
> >>>>>>> DPHY,
> >>>>>>
> >>>>>> I don't see the register interface for dphy support.
> >>>>>
> >>>>> I think the D-PHY should be supported through a PHY driver, as it
> >>>>> seems to be shared between different subsystems.
> >>>>
> >>>> IP has the provision to read DPHY register for debug purpose only.
> >>>> No programming of DPHY is required in subsystem.
> >>>
> >>> Do you know if this is the same D-PHY as used in the CSI2-RX subsystem ?
> >>
> >> Same D-PHY core has been used in MIPI CSI2 RXSS, but with different configuration.
> >>
> >>>>>>> multiple RGB color formats, command mode and video mode.
> >>>>>>> This is a MIPI-DSI host driver and provides DSI bus for panels.
> >>>>>>> This driver also helps to communicate with its panel using panel
> >>>>>>> framework.
> >>>>>>>
> >>>>>>> Signed-off-by: Venkateshwar Rao Gannavarapu
> >>>>>>> <venkateshwar.rao.gannavarapu@xilinx.com>
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> >>>>>>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
> >>>>>>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> >>>>>
> >>>>> Daniel Vetter has recently expressed his opiion that bridge
> >>>>> drivers should go to drivers/gpu/drm/bridge/. It would then be
> >>>>> drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
> >>
> >> The DSI-TX subsystem IP block is not a bridge driver.
> >> The DSI-TX subsystem IP block itself contains all the drm
> >> encoder/connector functionality and it’s the last node in display pipe line.
> >
> > The DSI-TX subsystem IP block is indeed an encoder from a hardware point of
> > view, but it's not necessarily the last block in the display pipeline. While the
> > output of the IP core goes of the the SoC, tt would be entirely feasible to
> > connect it to a DP to HDMI bridge on the board, such as the ANX7737 ([1]) for
> > instance. This is why we're pushing for all encoder (from a hardware point of
> > view) drivers to be implemented as DRM bridge, in order to make them usable
> > in different display pipelines, without hardcoding the assumption they will be
> > the last encoder in the pipeline.
> 
> Thanks for the details.
> I can understand it as SoC requirement where crtc is fixed, but as a FPGA product
> encoder driver should work with any of crtc driver.  And I still not
> see bridge implementation as hard requirement.
> Could you please explain what problem we face, if implemented as encoder driver.

First of all, there's no "encoder driver" in DRM/KMS. We have a "DRM
encoder slave" infrastructure that has been deprecated for a long time
in favour of DRM bridge, but apart from that, there's no "encoder
driver".

When the DRM/KMS API was designed, it modelled display devices using
four main objects:

- Planes, that represented a memory input to the compositor
- CRTCs, that represented the scan out hardware
- Encoders, that represented a device encoding the output of the
  composer to the display bus format (LVDS, HDMI, DisplayPort, ...)
- Connectors, that represented the physical connector

Drivers initially implemented all these objects internally, without much
code sharing for encoders (for instance the same I2C encoder chip would
have support code inside the i915 driver and inside the nouveau driver).

Those four objects exist in the kernel and are exposed to userspace. As
we can't break the API, this is set in stone. The model was later
extended with additional objects that are not exposed to userspace:

- Panels allow sharing code related to display panels, and are
  implemented as panel drivers, registering a drm_panel structure with
  the DRM core.

- Slave encoders allow sharing code related to encoders, mostly for I2C
  chips. This has quickly been deprecated.

- Bridges also allow sharing code related to encoders, initially for
  I2C/SPI encoder chips outside of the SoC.

The bridge infrastructure was initially designed to be an add-on to the
encoder, allowing the framework to share code for external encoders. For
instance, and SoC that contains an parallel RGB (output of the CRTC) to
LVDS encoder, and an on-board I2C LVDS to HDMI encoder, would handle the
former in the display controller driver, and the latter as a DRM bridge.

We later figured out that some systems had more than one additional chip
attached to the output of the in-SoC encoder. The DRM bridge
infrastructure received support for bridge chaining, where multiple
bridges can be attached one after the other at the output of an encoder.
At that point it became possible to model the in-SoC encoder IP cores as
a DRM bridge, and that has been done mostly for IP cores that are used
in different SoCs (such as the Synopsys DWC-HDMI for instance).
Supporting internal encoders that are specific to a particular display
controller through the DRM bridge infrastructure was possible too, but
nobody bothered migrating existing code.

Much more recently, we realized there was one more missing features in
the DRM bridge infrastructure. Bridges always made assumptions as
whether they would be in the middle of the chain or at the end of it,
and created a DRM connector in the latter case. This became problematic
when the same bridge got used in the middle of the chain and at the end
of it in different systems. We have thus started to migrate the DRM
bridge framework to be more flexible, and allow delegating connector
operations to different bridges, based on the features they implement,
and not based on their position in the chain. In this model, which we
are actively transitioning to, the DRM connector is created by the
display controller drivers, not the bridge drivers, and the connector
operations are implemented in a way that calls the appropriate bridge
operations (see [2] for more information).

Implementing support for an in-SoC encoder that is part of a display
controller as a DRM bridge is not mandatory, even if I recommend doing
so. In the case at hand, the DSI TX is an IP core (or rather a set of IP
cores, but from the outside it's one coherent unit) that can be used in
different designs. It's not tied to a particular display controller, but
can be used with different IP cores that would all fulfil the role of a
CRTC. It thus needs to be supported by code that can be shared between
different DRM drivers, instantiating the DSI TX device from DT. This is
exactly what the DRM bridge framework provides, a way to abstract a
component in the display pipeline, register it with the DRM core, and
using it in different DRM drivers.

If I were to stop here, implementing support for the DSI TX using a
Xilinx-specific component registration mechanism would duplicate what
DRM bridge provides, but apart from code duplication, there wouldn't be
large drawbacks. However, the position of the DSI TX in the display
pipeline isn't fixed. It is a fact that it terminates the part of the
display pipeline that is internal to the SoC, as it interfaces directly
with the pins of the SoC through the D-PHY. That's the only guarantee on
the IP core position though: it can be followed by other encoders (for
instance an on-board DSI to HDMI or DSI to DP encoder) instead of being
connected directly to a DSI panel, but can also be preceded by other
IP cores (I'm in particular thinking about a scaler, other options are
possible as we're dealing with an FPGA here). For these reasons,
modelling the DSI TX as a bridge would not only avoid duplication of
code, but would also provide the ability to support a wider variety of
use cases and pipelines, if all the components are implemented as DRM
bridges.

There's one caveat though. DRM bridges may be chained, but only in a
linear fashion, starting at the CRTC output and going to the display.
More complex topologies are not possible today with higher fan-in or
fan-out. This is a feature that we theoretically need to support, and I
don't know when it will become an important enough issue that we'll have
to implement it. That day, there will be two options, extending the DRM
bridge infrastructure to support more complex topologies, or using a
Xilinx-specific framework. The latter would really be reimplementing a
more complex DRM bridge tailored to Xilinx-specific architectures, which
I'd like to avoid, but we may not be able to implement this in DRM
bridge if the Xilinx-specific constraints can't be abstracted in a
generic way. In that case, we wouldn't be able to use DRM bridge, but we
would need a much more complex framework than what this patch
implements. I don't think it makes sense to go there today already
without having an actual use case and a customer need.

I hope this explains more clearly why DRM bridge is the preferred option
today.

[2] https://lore.kernel.org/dri-devel/20200417180819.GE5861@pendragon.ideasonboard.com/

> >> I didn't see any hard
> >> requirement to implement it into bridge driver and I see many DSI
> >> drivers are implemented as encoder drivers.
> >> Xilinx PL DRM encoder drivers are implemented in modular approach so
> >> that they can work with any CRTC driver which handles the DMA calls.
> >> So, at this stage we want to upstream as encoder driver only.
> >>
> >>>>>>>  3 files changed, 768 insertions(+)  create mode 100644
> >>>>>>> drivers/gpu/drm/xlnx/xlnx_dsi.c
> >
> > [1] https://www.analogix.com/en/products/convertersbridges/anx7737

-- 
Regards,

Laurent Pinchart