[LINUX PATCH 0/2] Add Xilinx DSI-Tx DRM driver

[LINUX 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 pixel format.

This patch series adds the dt-binding and DRM driver for Xilinx DSI-Tx soft IP.

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 DSI 2.0 Tx subsystem documentation
  drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

 .../bindings/display/xlnx/xlnx,dsi-tx.yaml         | 105 +++++
 drivers/gpu/drm/xlnx/Kconfig                       |  14 +
 drivers/gpu/drm/xlnx/Makefile                      |   1 +
 drivers/gpu/drm/xlnx/xlnx_dsi.c                    | 456 +++++++++++++++++++++
 4 files changed, 576 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
 create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

--
1.8.3.1

[LINUX 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 pixel format.

This patch series adds the dt-binding and DRM driver for Xilinx DSI-Tx soft IP.

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 DSI 2.0 Tx subsystem documentation
  drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

 .../bindings/display/xlnx/xlnx,dsi-tx.yaml         | 105 +++++
 drivers/gpu/drm/xlnx/Kconfig                       |  14 +
 drivers/gpu/drm/xlnx/Makefile                      |   1 +
 drivers/gpu/drm/xlnx/xlnx_dsi.c                    | 456 +++++++++++++++++++++
 4 files changed, 576 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
 create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

--
1.8.3.1

[LINUX PATCH 1/2] dt-bindings: display: xlnx: Add DSI 2.0 Tx subsystem documentation

[Venkateshwar Rao Gannavarapu]

This patch adds 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>
---
 .../bindings/display/xlnx/xlnx,dsi-tx.yaml         | 105 +++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml

diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
new file mode 100644
index 0000000..8e23cf5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/xlnx/xlnx,dsi-tx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx DSI Transmitter subsystem
+
+maintainers:
+  - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
+
+description: |
+  The Xilinx DSI Transmitter Subsystem implements the Mobile Industry
+  Processor Interface based display interface. It supports the interface
+  with the programmable logic (FPGA).
+
+  For more details refer to PG238 Xilinx MIPI DSI-V2.0 Tx Subsystem.
+
+properties:
+  compatible:
+    const: xlnx,dsi-tx-v2.0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: List of clock specifiers
+    items:
+      - description: AXI Lite CPU clock
+      - description: D-phy clock
+
+  clock-names:
+    items:
+      - const: s_axis_aclk
+      - const: dphy_clk_200M
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        description:
+          Input port node to receive pixel data from the
+          display controller. Exactly one endpoint must be
+          specified.
+        properties:
+          endpoint:
+            $ref: /schemas/graph.yaml#/properties/endpoint
+            description: sub-node describing the input from CRTC
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          DSI output port node to the panel or the next bridge
+          in the chain
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    dsi_tx@80020000 {
+        compatible = "xlnx,dsi-tx-v2.0";
+        reg = <0x80020000 0x20000>;
+        clock-names = "s_axi_aclk", "dphy_clk_200M";
+        clocks = <&misc_clk_0>, <&misc_clk_1>;
+
+        panel@0 {
+                compatible = "auo,b101uan01";
+                reg = <0>;
+                port {
+                        panel_in: endpoint {
+                                remote-endpoint = <&mipi_dsi_out>;
+                        };
+                };
+        };
+
+        ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                        #size-cells = <0>;
+                        #address-cells = <1>;
+                        reg = <0>;
+                        mipi_dsi_in: endpoint@0 {
+                                reg = <0>;
+                                remote-endpoint = <&pl_disp_crtc>;
+                        };
+                };
+                port@1 {
+                        reg = <1>;
+                        mipi_dsi_out: endpoint {
+                                remote-endpoint = <&panel_in>;
+                        };
+                };
+        };
+    };
--
1.8.3.1

[LINUX PATCH 1/2] dt-bindings: display: xlnx: Add DSI 2.0 Tx subsystem documentation

[Venkateshwar Rao Gannavarapu]

This patch adds 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>
---
 .../bindings/display/xlnx/xlnx,dsi-tx.yaml         | 105 +++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml

diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
new file mode 100644
index 0000000..8e23cf5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/xlnx/xlnx,dsi-tx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx DSI Transmitter subsystem
+
+maintainers:
+  - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
+
+description: |
+  The Xilinx DSI Transmitter Subsystem implements the Mobile Industry
+  Processor Interface based display interface. It supports the interface
+  with the programmable logic (FPGA).
+
+  For more details refer to PG238 Xilinx MIPI DSI-V2.0 Tx Subsystem.
+
+properties:
+  compatible:
+    const: xlnx,dsi-tx-v2.0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: List of clock specifiers
+    items:
+      - description: AXI Lite CPU clock
+      - description: D-phy clock
+
+  clock-names:
+    items:
+      - const: s_axis_aclk
+      - const: dphy_clk_200M
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        description:
+          Input port node to receive pixel data from the
+          display controller. Exactly one endpoint must be
+          specified.
+        properties:
+          endpoint:
+            $ref: /schemas/graph.yaml#/properties/endpoint
+            description: sub-node describing the input from CRTC
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          DSI output port node to the panel or the next bridge
+          in the chain
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    dsi_tx@80020000 {
+        compatible = "xlnx,dsi-tx-v2.0";
+        reg = <0x80020000 0x20000>;
+        clock-names = "s_axi_aclk", "dphy_clk_200M";
+        clocks = <&misc_clk_0>, <&misc_clk_1>;
+
+        panel@0 {
+                compatible = "auo,b101uan01";
+                reg = <0>;
+                port {
+                        panel_in: endpoint {
+                                remote-endpoint = <&mipi_dsi_out>;
+                        };
+                };
+        };
+
+        ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                        #size-cells = <0>;
+                        #address-cells = <1>;
+                        reg = <0>;
+                        mipi_dsi_in: endpoint@0 {
+                                reg = <0>;
+                                remote-endpoint = <&pl_disp_crtc>;
+                        };
+                };
+                port@1 {
+                        reg = <1>;
+                        mipi_dsi_out: endpoint {
+                                remote-endpoint = <&panel_in>;
+                        };
+                };
+        };
+    };
--
1.8.3.1

Re: [LINUX PATCH 1/2] dt-bindings: display: xlnx: Add DSI 2.0 Tx subsystem documentation

[Laurent Pinchart]

Hi GVRao,

Thank you for the patch.

On Thu, May 12, 2022 at 07:23:12PM +0530, Venkateshwar Rao Gannavarapu wrote:
> This patch adds 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>
> ---
>  .../bindings/display/xlnx/xlnx,dsi-tx.yaml         | 105 +++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
> new file mode 100644
> index 0000000..8e23cf5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/xlnx/xlnx,dsi-tx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx DSI Transmitter subsystem
> +
> +maintainers:
> +  - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> +
> +description: |
> +  The Xilinx DSI Transmitter Subsystem implements the Mobile Industry
> +  Processor Interface based display interface. It supports the interface
> +  with the programmable logic (FPGA).
> +
> +  For more details refer to PG238 Xilinx MIPI DSI-V2.0 Tx Subsystem.
> +
> +properties:
> +  compatible:
> +    const: xlnx,dsi-tx-v2.0
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: List of clock specifiers

You can drop the description, clocks is always a list of clock
specifiers.

> +    items:
> +      - description: AXI Lite CPU clock
> +      - description: D-phy clock

s/D-phy/D-PHY/

> +
> +  clock-names:
> +    items:
> +      - const: s_axis_aclk
> +      - const: dphy_clk_200M
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        description:
> +          Input port node to receive pixel data from the
> +          display controller. Exactly one endpoint must be
> +          specified.
> +        properties:
> +          endpoint:
> +            $ref: /schemas/graph.yaml#/properties/endpoint
> +            description: sub-node describing the input from CRTC

"CRTC" is a DRM term, and DT bindings should document the hardware, not
the driver. I'd drop the endpoint description as I don't think it brings
much, and use /schemas/graph.yaml#/properties/port instead of port-base.

> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          DSI output port node to the panel or the next bridge
> +          in the chain

Same ere about "bridge". Maybe just

        description:
          Output port node to DSI device.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    dsi_tx@80020000 {
> +        compatible = "xlnx,dsi-tx-v2.0";
> +        reg = <0x80020000 0x20000>;
> +        clock-names = "s_axi_aclk", "dphy_clk_200M";

Wrong clock name.

> +        clocks = <&misc_clk_0>, <&misc_clk_1>;

You need #address-cells = <1> and #size-cells = <0> here to specify an
address for the panel.

This should have been caught by the schema validation. Please see
Documentation/devicetree/bindings/writing-schema.rst for instructions on
how to validate bindings.

> +
> +        panel@0 {

This will also fail to validate. You need to reference
dsi-controller.yaml. You can check the other bindings for DSI controller
for examples.

> +                compatible = "auo,b101uan01";
> +                reg = <0>;
> +                port {
> +                        panel_in: endpoint {
> +                                remote-endpoint = <&mipi_dsi_out>;
> +                        };
> +                };
> +        };
> +
> +        ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                        #size-cells = <0>;
> +                        #address-cells = <1>;
> +                        reg = <0>;
> +                        mipi_dsi_in: endpoint@0 {
> +                                reg = <0>;

With a single endpoint you can drop the reg as well as the @0, and the
size and address cells in the parent.

> +                                remote-endpoint = <&pl_disp_crtc>;
> +                        };
> +                };
> +                port@1 {
> +                        reg = <1>;
> +                        mipi_dsi_out: endpoint {
> +                                remote-endpoint = <&panel_in>;
> +                        };
> +                };
> +        };
> +    };

-- 
Regards,

Laurent Pinchart

Re: [LINUX PATCH 1/2] dt-bindings: display: xlnx: Add DSI 2.0 Tx subsystem documentation

[Laurent Pinchart]

Hi GVRao,

Thank you for the patch.

On Thu, May 12, 2022 at 07:23:12PM +0530, Venkateshwar Rao Gannavarapu wrote:
> This patch adds 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>
> ---
>  .../bindings/display/xlnx/xlnx,dsi-tx.yaml         | 105 +++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
> new file mode 100644
> index 0000000..8e23cf5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi-tx.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/xlnx/xlnx,dsi-tx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx DSI Transmitter subsystem
> +
> +maintainers:
> +  - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> +
> +description: |
> +  The Xilinx DSI Transmitter Subsystem implements the Mobile Industry
> +  Processor Interface based display interface. It supports the interface
> +  with the programmable logic (FPGA).
> +
> +  For more details refer to PG238 Xilinx MIPI DSI-V2.0 Tx Subsystem.
> +
> +properties:
> +  compatible:
> +    const: xlnx,dsi-tx-v2.0
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: List of clock specifiers

You can drop the description, clocks is always a list of clock
specifiers.

> +    items:
> +      - description: AXI Lite CPU clock
> +      - description: D-phy clock

s/D-phy/D-PHY/

> +
> +  clock-names:
> +    items:
> +      - const: s_axis_aclk
> +      - const: dphy_clk_200M
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        description:
> +          Input port node to receive pixel data from the
> +          display controller. Exactly one endpoint must be
> +          specified.
> +        properties:
> +          endpoint:
> +            $ref: /schemas/graph.yaml#/properties/endpoint
> +            description: sub-node describing the input from CRTC

"CRTC" is a DRM term, and DT bindings should document the hardware, not
the driver. I'd drop the endpoint description as I don't think it brings
much, and use /schemas/graph.yaml#/properties/port instead of port-base.

> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          DSI output port node to the panel or the next bridge
> +          in the chain

Same ere about "bridge". Maybe just

        description:
          Output port node to DSI device.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    dsi_tx@80020000 {
> +        compatible = "xlnx,dsi-tx-v2.0";
> +        reg = <0x80020000 0x20000>;
> +        clock-names = "s_axi_aclk", "dphy_clk_200M";

Wrong clock name.

> +        clocks = <&misc_clk_0>, <&misc_clk_1>;

You need #address-cells = <1> and #size-cells = <0> here to specify an
address for the panel.

This should have been caught by the schema validation. Please see
Documentation/devicetree/bindings/writing-schema.rst for instructions on
how to validate bindings.

> +
> +        panel@0 {

This will also fail to validate. You need to reference
dsi-controller.yaml. You can check the other bindings for DSI controller
for examples.

> +                compatible = "auo,b101uan01";
> +                reg = <0>;
> +                port {
> +                        panel_in: endpoint {
> +                                remote-endpoint = <&mipi_dsi_out>;
> +                        };
> +                };
> +        };
> +
> +        ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                        #size-cells = <0>;
> +                        #address-cells = <1>;
> +                        reg = <0>;
> +                        mipi_dsi_in: endpoint@0 {
> +                                reg = <0>;

With a single endpoint you can drop the reg as well as the @0, and the
size and address cells in the parent.

> +                                remote-endpoint = <&pl_disp_crtc>;
> +                        };
> +                };
> +                port@1 {
> +                        reg = <1>;
> +                        mipi_dsi_out: endpoint {
> +                                remote-endpoint = <&panel_in>;
> +                        };
> +                };
> +        };
> +    };

-- 
Regards,

Laurent Pinchart

[LINUX PATCH 2/2] drm: xlnx: dsi: 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
and multiple RGB color formats.
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    |  14 ++
 drivers/gpu/drm/xlnx/Makefile   |   1 +
 drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 471 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 c3d0826..caa632b 100644
--- a/drivers/gpu/drm/xlnx/Kconfig
+++ b/drivers/gpu/drm/xlnx/Kconfig
@@ -14,3 +14,17 @@ 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"
+	depends on DRM && OF
+	select DRM_KMS_HELPER
+	select DRM_MIPI_DSI
+	select DRM_PANEL
+	select BACKLIGHT_LCD_SUPPORT
+	select BACKLIGHT_CLASS_DEVICE
+	select DRM_PANEL_SIMPLE
+	help
+	  DRM bridge driver for Xilinx programmable DSI subsystem controller.
+	  choose this option if you hava a Xilinx MIPI-DSI Tx subsytem in
+	  video pipeline.
diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
index 51c24b7..1e97fbe 100644
--- a/drivers/gpu/drm/xlnx/Makefile
+++ b/drivers/gpu/drm/xlnx/Makefile
@@ -1,2 +1,3 @@
 zynqmp-dpsub-y := 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..a5291f3
--- /dev/null
+++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
@@ -0,0 +1,456 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *
+ * Xilinx FPGA MIPI DSI Tx Controller driver.
+ *
+ * Copyright (C) 2022 Xilinx, Inc.
+ *
+ * Author: Venkateshwar Rao G <vgannava@xilinx.com>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.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_LANES_MASK		3
+#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_PIXELFORMAT_MASK	(0x3 << 11)
+#define XDSI_PCR_PIXELFORMAT_SHIFT	11
+#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_NUM_DATA_T			4
+
+/**
+ * struct xlnx_dsi - Xilinx DSI-TX core
+ * @bridge: DRM bridge structure
+ * @dsi_host: DSI host device
+ * @panel_bridge: Panel bridge structure
+ * @panel:  DRM panel structure
+ * @dev: device structure
+ * @clks: clock source structure
+ * @iomem: Base address of DSI subsystem
+ * @mode_flags: DSI operation mode related flags
+ * @lanes: number of active data lanes supported by DSI controller
+ * @mul_factor: multiplication factor for HACT timing
+ * @format: pixel format for video mode of DSI controller
+ * @device_found: Flag to indicate device presence
+ */
+struct xlnx_dsi {
+	struct drm_bridge bridge;
+	struct mipi_dsi_host dsi_host;
+	struct drm_bridge *panel_bridge;
+	struct drm_panel *panel;
+	struct device *dev;
+	struct clk_bulk_data *clks;
+	void __iomem *iomem;
+	unsigned long mode_flags;
+	u32 lanes;
+	u32 mul_factor;
+	enum mipi_dsi_pixel_format format;
+	bool device_found;
+};
+
+static const struct clk_bulk_data xdsi_clks[] = {
+	{ .id = "s_axis_aclk" },
+	{ .id = "dphy_clk_200M" },
+};
+
+static inline struct xlnx_dsi *host_to_dsi(struct mipi_dsi_host *host)
+{
+	return container_of(host, struct xlnx_dsi, dsi_host);
+}
+
+static inline struct xlnx_dsi *bridge_to_dsi(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct xlnx_dsi, bridge);
+}
+
+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);
+}
+
+static int xlnx_dsi_panel_or_bridge(struct xlnx_dsi *dsi,
+				    struct device_node *node)
+{
+	struct drm_bridge *panel_bridge;
+	struct drm_panel *panel;
+	struct device *dev = dsi->dev;
+	struct device_node *endpoint = dev->of_node;
+	int ret;
+
+	ret = drm_of_find_panel_or_bridge(endpoint, 1, 0, &panel, &panel_bridge);
+	if (ret < 0) {
+		dev_err(dsi->dev, "failed to find panel / bridge\n");
+		return ret;
+	}
+
+	if (panel) {
+		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+		if (IS_ERR(panel_bridge))
+			return PTR_ERR(panel_bridge);
+		dsi->panel = panel;
+	}
+
+	dsi->panel_bridge = panel_bridge;
+
+	if (!dsi->panel_bridge) {
+		dev_err(dsi->dev, "panel not found\n");
+		return -EPROBE_DEFER;
+	}
+
+	return 0;
+}
+
+static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
+				struct mipi_dsi_device *device)
+{
+	struct xlnx_dsi *dsi = host_to_dsi(host);
+	u32 reg;
+
+	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
+	dsi->lanes = reg & XDSI_PCR_LANES_MASK;
+	dsi->format = (reg & XDSI_PCR_PIXELFORMAT_MASK) >>
+		XDSI_PCR_PIXELFORMAT_SHIFT;
+	dsi->mode_flags = device->mode_flags;
+
+	if (dsi->lanes != device->lanes) {
+		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
+			device->lanes, dsi->lanes);
+		return -EINVAL;
+	}
+
+	if (dsi->lanes > 4 || dsi->lanes < 1) {
+		dev_err(dsi->dev, "%d lanes : invalid xlnx,dsi-num-lanes\n",
+			dsi->lanes);
+		return -EINVAL;
+	}
+
+	if (dsi->format != device->format) {
+		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
+			device->format, dsi->format);
+		return -EINVAL;
+	}
+
+	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_disable(dsi->panel);
+		dsi->panel = NULL;
+	}
+
+	return 0;
+}
+
+static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
+	.attach = xlnx_dsi_host_attach,
+	.detach	= xlnx_dsi_host_detach,
+};
+
+static void
+xlnx_dsi_bridge_disable(struct drm_bridge *bridge,
+			struct drm_bridge_state *old_bridge_state)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+	u32 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\n");
+}
+
+static void
+xlnx_dsi_bridge_mode_set(struct drm_bridge *bridge,
+			 const struct drm_display_mode *mode,
+			 const struct drm_display_mode *adjusted_mode)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+	u32 reg, video_mode;
+
+	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
+	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >> XDSI_PCR_VIDEOMODE_SHIFT;
+
+	if (!video_mode && (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
+		reg = XDSI_TIME1_HSA(adjusted_mode->hsync_end -
+				     adjusted_mode->hsync_start);
+		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
+	}
+
+	reg = XDSI_TIME4_VFP(adjusted_mode->vsync_start -
+			     adjusted_mode->vdisplay) |
+		XDSI_TIME4_VBP(adjusted_mode->vtotal -
+			       adjusted_mode->vsync_end) |
+		XDSI_TIME4_VSA(adjusted_mode->vsync_end -
+			       adjusted_mode->vsync_start);
+	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
+
+	reg = XDSI_TIME3_HFP(adjusted_mode->hsync_start -
+			     adjusted_mode->hdisplay) |
+		XDSI_TIME3_HBP(adjusted_mode->htotal -
+			       adjusted_mode->hsync_end);
+	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
+	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
+		(dsi->mul_factor) / 100);
+
+	if ((adjusted_mode->hdisplay & XDSI_HACT_MULTIPLIER) != 0)
+		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
+
+	reg = XDSI_TIME2_HACT((adjusted_mode->hdisplay) * (dsi->mul_factor) / 100) |
+		XDSI_TIME2_VACT(adjusted_mode->vdisplay);
+
+	xlnx_dsi_writel(dsi->iomem, XDSI_PCR, XDSI_PCR_VIDEOMODE(BIT(0)));
+}
+
+static void xlnx_dsi_bridge_enable(struct drm_bridge *bridge,
+				   struct drm_bridge_state *old_bridge_state)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+	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");
+}
+
+static int xlnx_dsi_bridge_attach(struct drm_bridge *bridge,
+				  enum drm_bridge_attach_flags flags)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found\n");
+		return -ENODEV;
+	}
+
+	/* Set the encoder type as caller does not know it */
+	bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
+
+	if (!dsi->device_found) {
+		int ret;
+
+		ret = xlnx_dsi_panel_or_bridge(dsi, dsi->dev->of_node);
+		if (ret) {
+			dev_err(dsi->dev, "dsi_panel_or_bridge failed\n");
+			return ret;
+		}
+
+		dsi->device_found = true;
+	}
+
+	/* Attach the panel-bridge to the dsi bridge */
+	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge,
+				 flags);
+}
+
+static void xlnx_dsi_bridge_detach(struct drm_bridge *bridge)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+
+	drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
+}
+
+static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
+	.mode_set	= xlnx_dsi_bridge_mode_set,
+	.atomic_enable	= xlnx_dsi_bridge_enable,
+	.atomic_disable	= xlnx_dsi_bridge_disable,
+	.attach		= xlnx_dsi_bridge_attach,
+	.detach		= xlnx_dsi_bridge_detach,
+};
+
+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};
+
+	/*
+	 * Used as a multiplication factor for HACT based on used
+	 * DSI 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 is a floating number,
+	 * a 100x multiplication factor is used.
+	 */
+	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;
+	}
+	dsi->mul_factor = xdsi_mul_fact[datatype];
+
+	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
+	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
+
+	return 0;
+}
+
+static int xlnx_dsi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct xlnx_dsi *dsi;
+	int num_clks = ARRAY_SIZE(xdsi_clks);
+	int ret;
+
+	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
+	if (!dsi)
+		return -ENOMEM;
+
+	dsi->dev = dev;
+	dsi->clks = devm_kmemdup(dev, xdsi_clks, sizeof(xdsi_clks),
+				 GFP_KERNEL);
+	if (!dsi->clks)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dsi->iomem = devm_ioremap_resource(dev, res);
+	if (IS_ERR(dsi->iomem))
+		return PTR_ERR(dsi->iomem);
+
+	ret = clk_bulk_get(dev, num_clks, dsi->clks);
+	if (ret)
+		return ret;
+
+	ret = xlnx_dsi_parse_dt(dsi);
+	if (ret)
+		return ret;
+
+	ret = clk_bulk_prepare_enable(num_clks, dsi->clks);
+	if (ret)
+		goto err_clk_put;
+
+	platform_set_drvdata(pdev, dsi);
+	dsi->dsi_host.ops = &xlnx_dsi_ops;
+	dsi->dsi_host.dev = dev;
+
+	ret = mipi_dsi_host_register(&dsi->dsi_host);
+	if (ret) {
+		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
+		goto err_clk_put;
+	}
+
+	dsi->bridge.driver_private = dsi;
+	dsi->bridge.funcs = &xlnx_dsi_bridge_funcs;
+#ifdef CONFIG_OF
+	dsi->bridge.of_node = pdev->dev.of_node;
+#endif
+
+	drm_bridge_add(&dsi->bridge);
+
+err_clk_put:
+	clk_bulk_put(num_clks, dsi->clks);
+
+	return ret;
+}
+
+static int xlnx_dsi_remove(struct platform_device *pdev)
+{
+	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
+	int num_clks = ARRAY_SIZE(xdsi_clks);
+
+	mipi_dsi_host_unregister(&dsi->dsi_host);
+	clk_bulk_disable_unprepare(num_clks, dsi->clks);
+	clk_bulk_put(num_clks, dsi->clks);
+
+	return 0;
+}
+
+static const struct of_device_id xlnx_dsi_of_match[] = {
+	{ .compatible = "xlnx,dsi-tx-v2.0"},
+	{ }
+};
+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("Venkateshwar Rao G <vgannava@xilinx.com>");
+MODULE_DESCRIPTION("Xilinx MIPI DSI host controller driver");
+MODULE_LICENSE("GPL");
--
1.8.3.1

[LINUX PATCH 2/2] drm: xlnx: dsi: 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
and multiple RGB color formats.
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    |  14 ++
 drivers/gpu/drm/xlnx/Makefile   |   1 +
 drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 471 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 c3d0826..caa632b 100644
--- a/drivers/gpu/drm/xlnx/Kconfig
+++ b/drivers/gpu/drm/xlnx/Kconfig
@@ -14,3 +14,17 @@ 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"
+	depends on DRM && OF
+	select DRM_KMS_HELPER
+	select DRM_MIPI_DSI
+	select DRM_PANEL
+	select BACKLIGHT_LCD_SUPPORT
+	select BACKLIGHT_CLASS_DEVICE
+	select DRM_PANEL_SIMPLE
+	help
+	  DRM bridge driver for Xilinx programmable DSI subsystem controller.
+	  choose this option if you hava a Xilinx MIPI-DSI Tx subsytem in
+	  video pipeline.
diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
index 51c24b7..1e97fbe 100644
--- a/drivers/gpu/drm/xlnx/Makefile
+++ b/drivers/gpu/drm/xlnx/Makefile
@@ -1,2 +1,3 @@
 zynqmp-dpsub-y := 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..a5291f3
--- /dev/null
+++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
@@ -0,0 +1,456 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *
+ * Xilinx FPGA MIPI DSI Tx Controller driver.
+ *
+ * Copyright (C) 2022 Xilinx, Inc.
+ *
+ * Author: Venkateshwar Rao G <vgannava@xilinx.com>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.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_LANES_MASK		3
+#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_PIXELFORMAT_MASK	(0x3 << 11)
+#define XDSI_PCR_PIXELFORMAT_SHIFT	11
+#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_NUM_DATA_T			4
+
+/**
+ * struct xlnx_dsi - Xilinx DSI-TX core
+ * @bridge: DRM bridge structure
+ * @dsi_host: DSI host device
+ * @panel_bridge: Panel bridge structure
+ * @panel:  DRM panel structure
+ * @dev: device structure
+ * @clks: clock source structure
+ * @iomem: Base address of DSI subsystem
+ * @mode_flags: DSI operation mode related flags
+ * @lanes: number of active data lanes supported by DSI controller
+ * @mul_factor: multiplication factor for HACT timing
+ * @format: pixel format for video mode of DSI controller
+ * @device_found: Flag to indicate device presence
+ */
+struct xlnx_dsi {
+	struct drm_bridge bridge;
+	struct mipi_dsi_host dsi_host;
+	struct drm_bridge *panel_bridge;
+	struct drm_panel *panel;
+	struct device *dev;
+	struct clk_bulk_data *clks;
+	void __iomem *iomem;
+	unsigned long mode_flags;
+	u32 lanes;
+	u32 mul_factor;
+	enum mipi_dsi_pixel_format format;
+	bool device_found;
+};
+
+static const struct clk_bulk_data xdsi_clks[] = {
+	{ .id = "s_axis_aclk" },
+	{ .id = "dphy_clk_200M" },
+};
+
+static inline struct xlnx_dsi *host_to_dsi(struct mipi_dsi_host *host)
+{
+	return container_of(host, struct xlnx_dsi, dsi_host);
+}
+
+static inline struct xlnx_dsi *bridge_to_dsi(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct xlnx_dsi, bridge);
+}
+
+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);
+}
+
+static int xlnx_dsi_panel_or_bridge(struct xlnx_dsi *dsi,
+				    struct device_node *node)
+{
+	struct drm_bridge *panel_bridge;
+	struct drm_panel *panel;
+	struct device *dev = dsi->dev;
+	struct device_node *endpoint = dev->of_node;
+	int ret;
+
+	ret = drm_of_find_panel_or_bridge(endpoint, 1, 0, &panel, &panel_bridge);
+	if (ret < 0) {
+		dev_err(dsi->dev, "failed to find panel / bridge\n");
+		return ret;
+	}
+
+	if (panel) {
+		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+		if (IS_ERR(panel_bridge))
+			return PTR_ERR(panel_bridge);
+		dsi->panel = panel;
+	}
+
+	dsi->panel_bridge = panel_bridge;
+
+	if (!dsi->panel_bridge) {
+		dev_err(dsi->dev, "panel not found\n");
+		return -EPROBE_DEFER;
+	}
+
+	return 0;
+}
+
+static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
+				struct mipi_dsi_device *device)
+{
+	struct xlnx_dsi *dsi = host_to_dsi(host);
+	u32 reg;
+
+	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
+	dsi->lanes = reg & XDSI_PCR_LANES_MASK;
+	dsi->format = (reg & XDSI_PCR_PIXELFORMAT_MASK) >>
+		XDSI_PCR_PIXELFORMAT_SHIFT;
+	dsi->mode_flags = device->mode_flags;
+
+	if (dsi->lanes != device->lanes) {
+		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
+			device->lanes, dsi->lanes);
+		return -EINVAL;
+	}
+
+	if (dsi->lanes > 4 || dsi->lanes < 1) {
+		dev_err(dsi->dev, "%d lanes : invalid xlnx,dsi-num-lanes\n",
+			dsi->lanes);
+		return -EINVAL;
+	}
+
+	if (dsi->format != device->format) {
+		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
+			device->format, dsi->format);
+		return -EINVAL;
+	}
+
+	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_disable(dsi->panel);
+		dsi->panel = NULL;
+	}
+
+	return 0;
+}
+
+static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
+	.attach = xlnx_dsi_host_attach,
+	.detach	= xlnx_dsi_host_detach,
+};
+
+static void
+xlnx_dsi_bridge_disable(struct drm_bridge *bridge,
+			struct drm_bridge_state *old_bridge_state)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+	u32 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\n");
+}
+
+static void
+xlnx_dsi_bridge_mode_set(struct drm_bridge *bridge,
+			 const struct drm_display_mode *mode,
+			 const struct drm_display_mode *adjusted_mode)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+	u32 reg, video_mode;
+
+	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
+	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >> XDSI_PCR_VIDEOMODE_SHIFT;
+
+	if (!video_mode && (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
+		reg = XDSI_TIME1_HSA(adjusted_mode->hsync_end -
+				     adjusted_mode->hsync_start);
+		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
+	}
+
+	reg = XDSI_TIME4_VFP(adjusted_mode->vsync_start -
+			     adjusted_mode->vdisplay) |
+		XDSI_TIME4_VBP(adjusted_mode->vtotal -
+			       adjusted_mode->vsync_end) |
+		XDSI_TIME4_VSA(adjusted_mode->vsync_end -
+			       adjusted_mode->vsync_start);
+	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
+
+	reg = XDSI_TIME3_HFP(adjusted_mode->hsync_start -
+			     adjusted_mode->hdisplay) |
+		XDSI_TIME3_HBP(adjusted_mode->htotal -
+			       adjusted_mode->hsync_end);
+	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
+	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
+		(dsi->mul_factor) / 100);
+
+	if ((adjusted_mode->hdisplay & XDSI_HACT_MULTIPLIER) != 0)
+		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
+
+	reg = XDSI_TIME2_HACT((adjusted_mode->hdisplay) * (dsi->mul_factor) / 100) |
+		XDSI_TIME2_VACT(adjusted_mode->vdisplay);
+
+	xlnx_dsi_writel(dsi->iomem, XDSI_PCR, XDSI_PCR_VIDEOMODE(BIT(0)));
+}
+
+static void xlnx_dsi_bridge_enable(struct drm_bridge *bridge,
+				   struct drm_bridge_state *old_bridge_state)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+	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");
+}
+
+static int xlnx_dsi_bridge_attach(struct drm_bridge *bridge,
+				  enum drm_bridge_attach_flags flags)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found\n");
+		return -ENODEV;
+	}
+
+	/* Set the encoder type as caller does not know it */
+	bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
+
+	if (!dsi->device_found) {
+		int ret;
+
+		ret = xlnx_dsi_panel_or_bridge(dsi, dsi->dev->of_node);
+		if (ret) {
+			dev_err(dsi->dev, "dsi_panel_or_bridge failed\n");
+			return ret;
+		}
+
+		dsi->device_found = true;
+	}
+
+	/* Attach the panel-bridge to the dsi bridge */
+	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge,
+				 flags);
+}
+
+static void xlnx_dsi_bridge_detach(struct drm_bridge *bridge)
+{
+	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
+
+	drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
+}
+
+static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
+	.mode_set	= xlnx_dsi_bridge_mode_set,
+	.atomic_enable	= xlnx_dsi_bridge_enable,
+	.atomic_disable	= xlnx_dsi_bridge_disable,
+	.attach		= xlnx_dsi_bridge_attach,
+	.detach		= xlnx_dsi_bridge_detach,
+};
+
+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};
+
+	/*
+	 * Used as a multiplication factor for HACT based on used
+	 * DSI 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 is a floating number,
+	 * a 100x multiplication factor is used.
+	 */
+	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;
+	}
+	dsi->mul_factor = xdsi_mul_fact[datatype];
+
+	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
+	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
+
+	return 0;
+}
+
+static int xlnx_dsi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct xlnx_dsi *dsi;
+	int num_clks = ARRAY_SIZE(xdsi_clks);
+	int ret;
+
+	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
+	if (!dsi)
+		return -ENOMEM;
+
+	dsi->dev = dev;
+	dsi->clks = devm_kmemdup(dev, xdsi_clks, sizeof(xdsi_clks),
+				 GFP_KERNEL);
+	if (!dsi->clks)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dsi->iomem = devm_ioremap_resource(dev, res);
+	if (IS_ERR(dsi->iomem))
+		return PTR_ERR(dsi->iomem);
+
+	ret = clk_bulk_get(dev, num_clks, dsi->clks);
+	if (ret)
+		return ret;
+
+	ret = xlnx_dsi_parse_dt(dsi);
+	if (ret)
+		return ret;
+
+	ret = clk_bulk_prepare_enable(num_clks, dsi->clks);
+	if (ret)
+		goto err_clk_put;
+
+	platform_set_drvdata(pdev, dsi);
+	dsi->dsi_host.ops = &xlnx_dsi_ops;
+	dsi->dsi_host.dev = dev;
+
+	ret = mipi_dsi_host_register(&dsi->dsi_host);
+	if (ret) {
+		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
+		goto err_clk_put;
+	}
+
+	dsi->bridge.driver_private = dsi;
+	dsi->bridge.funcs = &xlnx_dsi_bridge_funcs;
+#ifdef CONFIG_OF
+	dsi->bridge.of_node = pdev->dev.of_node;
+#endif
+
+	drm_bridge_add(&dsi->bridge);
+
+err_clk_put:
+	clk_bulk_put(num_clks, dsi->clks);
+
+	return ret;
+}
+
+static int xlnx_dsi_remove(struct platform_device *pdev)
+{
+	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
+	int num_clks = ARRAY_SIZE(xdsi_clks);
+
+	mipi_dsi_host_unregister(&dsi->dsi_host);
+	clk_bulk_disable_unprepare(num_clks, dsi->clks);
+	clk_bulk_put(num_clks, dsi->clks);
+
+	return 0;
+}
+
+static const struct of_device_id xlnx_dsi_of_match[] = {
+	{ .compatible = "xlnx,dsi-tx-v2.0"},
+	{ }
+};
+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("Venkateshwar Rao G <vgannava@xilinx.com>");
+MODULE_DESCRIPTION("Xilinx MIPI DSI host controller driver");
+MODULE_LICENSE("GPL");
--
1.8.3.1

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Sam Ravnborg]

Hi Venkateshwar,

On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> and multiple RGB color formats.
> 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.

Thanks for submitting this driver. I have added a few comments in the
following that I hope you will find useful to improve the driver.

	Sam

> 
> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> ---
>  drivers/gpu/drm/xlnx/Kconfig    |  14 ++
>  drivers/gpu/drm/xlnx/Makefile   |   1 +
>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 471 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 c3d0826..caa632b 100644
> --- a/drivers/gpu/drm/xlnx/Kconfig
> +++ b/drivers/gpu/drm/xlnx/Kconfig
> @@ -14,3 +14,17 @@ 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.
It would be nice to have it in some sort of alphabetic order, both in
the Kconfig file and also the Makefile.

> +
> +config DRM_XLNX_DSI
> +	tristate "Xilinx DRM DSI Subsystem Driver"
> +	depends on DRM && OF
> +	select DRM_KMS_HELPER
> +	select DRM_MIPI_DSI
> +	select DRM_PANEL
> +	select BACKLIGHT_LCD_SUPPORT
The BACKLIGHT_LCD_SUPPORT symbol is not relevant and can be dropped.

> +	select BACKLIGHT_CLASS_DEVICE
> +	select DRM_PANEL_SIMPLE
The symbol DRM_PANEL_SIMPLE is used to enable the panel_simple driver
and should not be selected here.

> +	help
> +	  DRM bridge driver for Xilinx programmable DSI subsystem controller.
> +	  choose this option if you hava a Xilinx MIPI-DSI Tx subsytem in
> +	  video pipeline.
> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> index 51c24b7..1e97fbe 100644
> --- a/drivers/gpu/drm/xlnx/Makefile
> +++ b/drivers/gpu/drm/xlnx/Makefile
> @@ -1,2 +1,3 @@
>  zynqmp-dpsub-y := 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..a5291f3
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> @@ -0,0 +1,456 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *
> + * Xilinx FPGA MIPI DSI Tx Controller driver.
> + *
> + * Copyright (C) 2022 Xilinx, Inc.
> + *
> + * Author: Venkateshwar Rao G <vgannava@xilinx.com>
I noticed this is not the mail used in the s-o-b line.

> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.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_LANES_MASK		3
> +#define XDSI_PCR_VIDEOMODE(x)		(((x) & 0x3) << 3)
> +#define XDSI_PCR_VIDEOMODE_MASK		(0x3 << 3)
GENMASK()?
Same for other XXX_MASK definitions below

> +#define XDSI_PCR_VIDEOMODE_SHIFT	3
> +#define XDSI_PCR_BLLPTYPE(x)		((x) << 5)
> +#define XDSI_PCR_BLLPMODE(x)		((x) << 6)
> +#define XDSI_PCR_PIXELFORMAT_MASK	(0x3 << 11)
> +#define XDSI_PCR_PIXELFORMAT_SHIFT	11
> +#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_NUM_DATA_T			4
> +
> +/**
> + * struct xlnx_dsi - Xilinx DSI-TX core
> + * @bridge: DRM bridge structure
> + * @dsi_host: DSI host device
> + * @panel_bridge: Panel bridge structure
> + * @panel:  DRM panel structure
> + * @dev: device structure
> + * @clks: clock source structure
> + * @iomem: Base address of DSI subsystem
> + * @mode_flags: DSI operation mode related flags
> + * @lanes: number of active data lanes supported by DSI controller
> + * @mul_factor: multiplication factor for HACT timing
> + * @format: pixel format for video mode of DSI controller
> + * @device_found: Flag to indicate device presence
> + */
> +struct xlnx_dsi {
> +	struct drm_bridge bridge;
> +	struct mipi_dsi_host dsi_host;
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
It looks wrong that both a panel and a panel_bridge is required.
We should today only use the panel_bridge.

> +	struct device *dev;
> +	struct clk_bulk_data *clks;
> +	void __iomem *iomem;
> +	unsigned long mode_flags;
> +	u32 lanes;
> +	u32 mul_factor;
> +	enum mipi_dsi_pixel_format format;
> +	bool device_found;
> +};
> +
> +static const struct clk_bulk_data xdsi_clks[] = {
> +	{ .id = "s_axis_aclk" },
> +	{ .id = "dphy_clk_200M" },
> +};
> +
> +static inline struct xlnx_dsi *host_to_dsi(struct mipi_dsi_host *host)
> +{
> +	return container_of(host, struct xlnx_dsi, dsi_host);
> +}
> +
> +static inline struct xlnx_dsi *bridge_to_dsi(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct xlnx_dsi, bridge);
> +}
> +
> +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);
> +}
When I see implementations like this I wonder if a regmap would be
beneficial?

> +
> +static int xlnx_dsi_panel_or_bridge(struct xlnx_dsi *dsi,
> +				    struct device_node *node)
> +{
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
> +	struct device *dev = dsi->dev;
> +	struct device_node *endpoint = dev->of_node;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(endpoint, 1, 0, &panel, &panel_bridge);

From the documentation of drm_of_find_panel_or_bridge():

* This function is deprecated and should not be used in new drivers. Use
* devm_drm_of_get_bridge() instead.

Please update accordingly.
This will also avoid the panel/panel_bridge confusion.

> +	if (ret < 0) {
> +		dev_err(dsi->dev, "failed to find panel / bridge\n");
> +		return ret;
> +	}
> +
> +	if (panel) {
> +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +		if (IS_ERR(panel_bridge))
> +			return PTR_ERR(panel_bridge);
> +		dsi->panel = panel;
> +	}
> +
> +	dsi->panel_bridge = panel_bridge;
> +
> +	if (!dsi->panel_bridge) {
> +		dev_err(dsi->dev, "panel not found\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
> +				struct mipi_dsi_device *device)
> +{
> +	struct xlnx_dsi *dsi = host_to_dsi(host);
> +	u32 reg;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	dsi->lanes = reg & XDSI_PCR_LANES_MASK;
> +	dsi->format = (reg & XDSI_PCR_PIXELFORMAT_MASK) >>
> +		XDSI_PCR_PIXELFORMAT_SHIFT;
> +	dsi->mode_flags = device->mode_flags;
> +
> +	if (dsi->lanes != device->lanes) {
> +		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
> +			device->lanes, dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->lanes > 4 || dsi->lanes < 1) {
> +		dev_err(dsi->dev, "%d lanes : invalid xlnx,dsi-num-lanes\n",
> +			dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->format != device->format) {
> +		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
> +			device->format, dsi->format);
> +		return -EINVAL;
> +	}
> +
> +	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_disable(dsi->panel);
> +		dsi->panel = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
> +	.attach = xlnx_dsi_host_attach,
> +	.detach	= xlnx_dsi_host_detach,
> +};
> +
> +static void
> +xlnx_dsi_bridge_disable(struct drm_bridge *bridge,
> +			struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 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\n");
> +}
> +
> +static void
> +xlnx_dsi_bridge_mode_set(struct drm_bridge *bridge,
> +			 const struct drm_display_mode *mode,
> +			 const struct drm_display_mode *adjusted_mode)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 reg, video_mode;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >> XDSI_PCR_VIDEOMODE_SHIFT;
> +
> +	if (!video_mode && (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
> +		reg = XDSI_TIME1_HSA(adjusted_mode->hsync_end -
> +				     adjusted_mode->hsync_start);
> +		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
> +	}
> +
> +	reg = XDSI_TIME4_VFP(adjusted_mode->vsync_start -
> +			     adjusted_mode->vdisplay) |
> +		XDSI_TIME4_VBP(adjusted_mode->vtotal -
> +			       adjusted_mode->vsync_end) |
> +		XDSI_TIME4_VSA(adjusted_mode->vsync_end -
> +			       adjusted_mode->vsync_start);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
> +
> +	reg = XDSI_TIME3_HFP(adjusted_mode->hsync_start -
> +			     adjusted_mode->hdisplay) |
> +		XDSI_TIME3_HBP(adjusted_mode->htotal -
> +			       adjusted_mode->hsync_end);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
> +	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
> +		(dsi->mul_factor) / 100);
> +
> +	if ((adjusted_mode->hdisplay & XDSI_HACT_MULTIPLIER) != 0)
> +		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
Maybe catch this in a mode_valid() operation?

> +
> +	reg = XDSI_TIME2_HACT((adjusted_mode->hdisplay) * (dsi->mul_factor) / 100) |
> +		XDSI_TIME2_VACT(adjusted_mode->vdisplay);
> +
> +	xlnx_dsi_writel(dsi->iomem, XDSI_PCR, XDSI_PCR_VIDEOMODE(BIT(0)));
> +}
> +
> +static void xlnx_dsi_bridge_enable(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	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");
> +}
> +
> +static int xlnx_dsi_bridge_attach(struct drm_bridge *bridge,
> +				  enum drm_bridge_attach_flags flags)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found\n");
DRM_ERROR and friends are deprecated. Use drm_xxx or dev_xxx if possible, otherwise
fallback to pr_err.
> +		return -ENODEV;
> +	}
> +
> +	/* Set the encoder type as caller does not know it */
> +	bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
> +
> +	if (!dsi->device_found) {
> +		int ret;
> +
> +		ret = xlnx_dsi_panel_or_bridge(dsi, dsi->dev->of_node);
> +		if (ret) {
> +			dev_err(dsi->dev, "dsi_panel_or_bridge failed\n");
> +			return ret;
> +		}
> +
> +		dsi->device_found = true;
> +	}
> +
> +	/* Attach the panel-bridge to the dsi bridge */
> +	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge,
> +				 flags);
> +}
> +
> +static void xlnx_dsi_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
> +}
> +
> +static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
> +	.mode_set	= xlnx_dsi_bridge_mode_set,
From the documentation of the mode_set operation:
 * This is deprecated, do not use!
 * New drivers shall set their mode in the
 * &drm_bridge_funcs.atomic_enable operation.

Please adjust accordingly.

> +	.atomic_enable	= xlnx_dsi_bridge_enable,
> +	.atomic_disable	= xlnx_dsi_bridge_disable,
> +	.attach		= xlnx_dsi_bridge_attach,
> +	.detach		= xlnx_dsi_bridge_detach,
> +};
For a new bridge please implement all the mandatory atomic operations.

You will need at least:
	.atomic_get_output_bus_fmts = xlnx_dsi_bridge_get_output_bus_fmts,
	.atomic_get_input_bus_fmts = xlnx_dsi_bridge_get_input_bus_fmts,
	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
	.atomic_reset = drm_atomic_helper_bridge_reset,
};

> +
> +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};
> +
> +	/*
> +	 * Used as a multiplication factor for HACT based on used
> +	 * DSI 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 is a floating number,
> +	 * a 100x multiplication factor is used.
> +	 */
> +	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;
> +	}
> +	dsi->mul_factor = xdsi_mul_fact[datatype];
> +
> +	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
> +	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct xlnx_dsi *dsi;
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dev = dev;
> +	dsi->clks = devm_kmemdup(dev, xdsi_clks, sizeof(xdsi_clks),
> +				 GFP_KERNEL);
> +	if (!dsi->clks)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dsi->iomem = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dsi->iomem))
> +		return PTR_ERR(dsi->iomem);
> +
> +	ret = clk_bulk_get(dev, num_clks, dsi->clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = xlnx_dsi_parse_dt(dsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_bulk_prepare_enable(num_clks, dsi->clks);
> +	if (ret)
> +		goto err_clk_put;
> +
> +	platform_set_drvdata(pdev, dsi);
> +	dsi->dsi_host.ops = &xlnx_dsi_ops;
> +	dsi->dsi_host.dev = dev;
> +
> +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> +	if (ret) {
> +		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
> +		goto err_clk_put;
> +	}
> +
> +	dsi->bridge.driver_private = dsi;
> +	dsi->bridge.funcs = &xlnx_dsi_bridge_funcs;
> +#ifdef CONFIG_OF
The driver depends on CONFIG_OF - so no need to check it here.

> +	dsi->bridge.of_node = pdev->dev.of_node;
> +#endif
> +
> +	drm_bridge_add(&dsi->bridge);
> +
> +err_clk_put:
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return ret;
> +}
> +
> +static int xlnx_dsi_remove(struct platform_device *pdev)
> +{
> +	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +	clk_bulk_disable_unprepare(num_clks, dsi->clks);
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xlnx_dsi_of_match[] = {
> +	{ .compatible = "xlnx,dsi-tx-v2.0"},
> +	{ }
> +};
> +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("Venkateshwar Rao G <vgannava@xilinx.com>");
> +MODULE_DESCRIPTION("Xilinx MIPI DSI host controller driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.1

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Sam Ravnborg]

Hi Venkateshwar,

On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> and multiple RGB color formats.
> 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.

Thanks for submitting this driver. I have added a few comments in the
following that I hope you will find useful to improve the driver.

	Sam

> 
> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> ---
>  drivers/gpu/drm/xlnx/Kconfig    |  14 ++
>  drivers/gpu/drm/xlnx/Makefile   |   1 +
>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 471 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 c3d0826..caa632b 100644
> --- a/drivers/gpu/drm/xlnx/Kconfig
> +++ b/drivers/gpu/drm/xlnx/Kconfig
> @@ -14,3 +14,17 @@ 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.
It would be nice to have it in some sort of alphabetic order, both in
the Kconfig file and also the Makefile.

> +
> +config DRM_XLNX_DSI
> +	tristate "Xilinx DRM DSI Subsystem Driver"
> +	depends on DRM && OF
> +	select DRM_KMS_HELPER
> +	select DRM_MIPI_DSI
> +	select DRM_PANEL
> +	select BACKLIGHT_LCD_SUPPORT
The BACKLIGHT_LCD_SUPPORT symbol is not relevant and can be dropped.

> +	select BACKLIGHT_CLASS_DEVICE
> +	select DRM_PANEL_SIMPLE
The symbol DRM_PANEL_SIMPLE is used to enable the panel_simple driver
and should not be selected here.

> +	help
> +	  DRM bridge driver for Xilinx programmable DSI subsystem controller.
> +	  choose this option if you hava a Xilinx MIPI-DSI Tx subsytem in
> +	  video pipeline.
> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> index 51c24b7..1e97fbe 100644
> --- a/drivers/gpu/drm/xlnx/Makefile
> +++ b/drivers/gpu/drm/xlnx/Makefile
> @@ -1,2 +1,3 @@
>  zynqmp-dpsub-y := 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..a5291f3
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> @@ -0,0 +1,456 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *
> + * Xilinx FPGA MIPI DSI Tx Controller driver.
> + *
> + * Copyright (C) 2022 Xilinx, Inc.
> + *
> + * Author: Venkateshwar Rao G <vgannava@xilinx.com>
I noticed this is not the mail used in the s-o-b line.

> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.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_LANES_MASK		3
> +#define XDSI_PCR_VIDEOMODE(x)		(((x) & 0x3) << 3)
> +#define XDSI_PCR_VIDEOMODE_MASK		(0x3 << 3)
GENMASK()?
Same for other XXX_MASK definitions below

> +#define XDSI_PCR_VIDEOMODE_SHIFT	3
> +#define XDSI_PCR_BLLPTYPE(x)		((x) << 5)
> +#define XDSI_PCR_BLLPMODE(x)		((x) << 6)
> +#define XDSI_PCR_PIXELFORMAT_MASK	(0x3 << 11)
> +#define XDSI_PCR_PIXELFORMAT_SHIFT	11
> +#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_NUM_DATA_T			4
> +
> +/**
> + * struct xlnx_dsi - Xilinx DSI-TX core
> + * @bridge: DRM bridge structure
> + * @dsi_host: DSI host device
> + * @panel_bridge: Panel bridge structure
> + * @panel:  DRM panel structure
> + * @dev: device structure
> + * @clks: clock source structure
> + * @iomem: Base address of DSI subsystem
> + * @mode_flags: DSI operation mode related flags
> + * @lanes: number of active data lanes supported by DSI controller
> + * @mul_factor: multiplication factor for HACT timing
> + * @format: pixel format for video mode of DSI controller
> + * @device_found: Flag to indicate device presence
> + */
> +struct xlnx_dsi {
> +	struct drm_bridge bridge;
> +	struct mipi_dsi_host dsi_host;
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
It looks wrong that both a panel and a panel_bridge is required.
We should today only use the panel_bridge.

> +	struct device *dev;
> +	struct clk_bulk_data *clks;
> +	void __iomem *iomem;
> +	unsigned long mode_flags;
> +	u32 lanes;
> +	u32 mul_factor;
> +	enum mipi_dsi_pixel_format format;
> +	bool device_found;
> +};
> +
> +static const struct clk_bulk_data xdsi_clks[] = {
> +	{ .id = "s_axis_aclk" },
> +	{ .id = "dphy_clk_200M" },
> +};
> +
> +static inline struct xlnx_dsi *host_to_dsi(struct mipi_dsi_host *host)
> +{
> +	return container_of(host, struct xlnx_dsi, dsi_host);
> +}
> +
> +static inline struct xlnx_dsi *bridge_to_dsi(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct xlnx_dsi, bridge);
> +}
> +
> +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);
> +}
When I see implementations like this I wonder if a regmap would be
beneficial?

> +
> +static int xlnx_dsi_panel_or_bridge(struct xlnx_dsi *dsi,
> +				    struct device_node *node)
> +{
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
> +	struct device *dev = dsi->dev;
> +	struct device_node *endpoint = dev->of_node;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(endpoint, 1, 0, &panel, &panel_bridge);

From the documentation of drm_of_find_panel_or_bridge():

* This function is deprecated and should not be used in new drivers. Use
* devm_drm_of_get_bridge() instead.

Please update accordingly.
This will also avoid the panel/panel_bridge confusion.

> +	if (ret < 0) {
> +		dev_err(dsi->dev, "failed to find panel / bridge\n");
> +		return ret;
> +	}
> +
> +	if (panel) {
> +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +		if (IS_ERR(panel_bridge))
> +			return PTR_ERR(panel_bridge);
> +		dsi->panel = panel;
> +	}
> +
> +	dsi->panel_bridge = panel_bridge;
> +
> +	if (!dsi->panel_bridge) {
> +		dev_err(dsi->dev, "panel not found\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
> +				struct mipi_dsi_device *device)
> +{
> +	struct xlnx_dsi *dsi = host_to_dsi(host);
> +	u32 reg;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	dsi->lanes = reg & XDSI_PCR_LANES_MASK;
> +	dsi->format = (reg & XDSI_PCR_PIXELFORMAT_MASK) >>
> +		XDSI_PCR_PIXELFORMAT_SHIFT;
> +	dsi->mode_flags = device->mode_flags;
> +
> +	if (dsi->lanes != device->lanes) {
> +		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
> +			device->lanes, dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->lanes > 4 || dsi->lanes < 1) {
> +		dev_err(dsi->dev, "%d lanes : invalid xlnx,dsi-num-lanes\n",
> +			dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->format != device->format) {
> +		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
> +			device->format, dsi->format);
> +		return -EINVAL;
> +	}
> +
> +	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_disable(dsi->panel);
> +		dsi->panel = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
> +	.attach = xlnx_dsi_host_attach,
> +	.detach	= xlnx_dsi_host_detach,
> +};
> +
> +static void
> +xlnx_dsi_bridge_disable(struct drm_bridge *bridge,
> +			struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 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\n");
> +}
> +
> +static void
> +xlnx_dsi_bridge_mode_set(struct drm_bridge *bridge,
> +			 const struct drm_display_mode *mode,
> +			 const struct drm_display_mode *adjusted_mode)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 reg, video_mode;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >> XDSI_PCR_VIDEOMODE_SHIFT;
> +
> +	if (!video_mode && (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
> +		reg = XDSI_TIME1_HSA(adjusted_mode->hsync_end -
> +				     adjusted_mode->hsync_start);
> +		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
> +	}
> +
> +	reg = XDSI_TIME4_VFP(adjusted_mode->vsync_start -
> +			     adjusted_mode->vdisplay) |
> +		XDSI_TIME4_VBP(adjusted_mode->vtotal -
> +			       adjusted_mode->vsync_end) |
> +		XDSI_TIME4_VSA(adjusted_mode->vsync_end -
> +			       adjusted_mode->vsync_start);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
> +
> +	reg = XDSI_TIME3_HFP(adjusted_mode->hsync_start -
> +			     adjusted_mode->hdisplay) |
> +		XDSI_TIME3_HBP(adjusted_mode->htotal -
> +			       adjusted_mode->hsync_end);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
> +	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
> +		(dsi->mul_factor) / 100);
> +
> +	if ((adjusted_mode->hdisplay & XDSI_HACT_MULTIPLIER) != 0)
> +		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
Maybe catch this in a mode_valid() operation?

> +
> +	reg = XDSI_TIME2_HACT((adjusted_mode->hdisplay) * (dsi->mul_factor) / 100) |
> +		XDSI_TIME2_VACT(adjusted_mode->vdisplay);
> +
> +	xlnx_dsi_writel(dsi->iomem, XDSI_PCR, XDSI_PCR_VIDEOMODE(BIT(0)));
> +}
> +
> +static void xlnx_dsi_bridge_enable(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	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");
> +}
> +
> +static int xlnx_dsi_bridge_attach(struct drm_bridge *bridge,
> +				  enum drm_bridge_attach_flags flags)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found\n");
DRM_ERROR and friends are deprecated. Use drm_xxx or dev_xxx if possible, otherwise
fallback to pr_err.
> +		return -ENODEV;
> +	}
> +
> +	/* Set the encoder type as caller does not know it */
> +	bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
> +
> +	if (!dsi->device_found) {
> +		int ret;
> +
> +		ret = xlnx_dsi_panel_or_bridge(dsi, dsi->dev->of_node);
> +		if (ret) {
> +			dev_err(dsi->dev, "dsi_panel_or_bridge failed\n");
> +			return ret;
> +		}
> +
> +		dsi->device_found = true;
> +	}
> +
> +	/* Attach the panel-bridge to the dsi bridge */
> +	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge,
> +				 flags);
> +}
> +
> +static void xlnx_dsi_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
> +}
> +
> +static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
> +	.mode_set	= xlnx_dsi_bridge_mode_set,
From the documentation of the mode_set operation:
 * This is deprecated, do not use!
 * New drivers shall set their mode in the
 * &drm_bridge_funcs.atomic_enable operation.

Please adjust accordingly.

> +	.atomic_enable	= xlnx_dsi_bridge_enable,
> +	.atomic_disable	= xlnx_dsi_bridge_disable,
> +	.attach		= xlnx_dsi_bridge_attach,
> +	.detach		= xlnx_dsi_bridge_detach,
> +};
For a new bridge please implement all the mandatory atomic operations.

You will need at least:
	.atomic_get_output_bus_fmts = xlnx_dsi_bridge_get_output_bus_fmts,
	.atomic_get_input_bus_fmts = xlnx_dsi_bridge_get_input_bus_fmts,
	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
	.atomic_reset = drm_atomic_helper_bridge_reset,
};

> +
> +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};
> +
> +	/*
> +	 * Used as a multiplication factor for HACT based on used
> +	 * DSI 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 is a floating number,
> +	 * a 100x multiplication factor is used.
> +	 */
> +	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;
> +	}
> +	dsi->mul_factor = xdsi_mul_fact[datatype];
> +
> +	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
> +	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct xlnx_dsi *dsi;
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dev = dev;
> +	dsi->clks = devm_kmemdup(dev, xdsi_clks, sizeof(xdsi_clks),
> +				 GFP_KERNEL);
> +	if (!dsi->clks)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dsi->iomem = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dsi->iomem))
> +		return PTR_ERR(dsi->iomem);
> +
> +	ret = clk_bulk_get(dev, num_clks, dsi->clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = xlnx_dsi_parse_dt(dsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_bulk_prepare_enable(num_clks, dsi->clks);
> +	if (ret)
> +		goto err_clk_put;
> +
> +	platform_set_drvdata(pdev, dsi);
> +	dsi->dsi_host.ops = &xlnx_dsi_ops;
> +	dsi->dsi_host.dev = dev;
> +
> +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> +	if (ret) {
> +		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
> +		goto err_clk_put;
> +	}
> +
> +	dsi->bridge.driver_private = dsi;
> +	dsi->bridge.funcs = &xlnx_dsi_bridge_funcs;
> +#ifdef CONFIG_OF
The driver depends on CONFIG_OF - so no need to check it here.

> +	dsi->bridge.of_node = pdev->dev.of_node;
> +#endif
> +
> +	drm_bridge_add(&dsi->bridge);
> +
> +err_clk_put:
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return ret;
> +}
> +
> +static int xlnx_dsi_remove(struct platform_device *pdev)
> +{
> +	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +	clk_bulk_disable_unprepare(num_clks, dsi->clks);
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xlnx_dsi_of_match[] = {
> +	{ .compatible = "xlnx,dsi-tx-v2.0"},
> +	{ }
> +};
> +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("Venkateshwar Rao G <vgannava@xilinx.com>");
> +MODULE_DESCRIPTION("Xilinx MIPI DSI host controller driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.1

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Laurent Pinchart]

Hi Sam,

On Fri, May 13, 2022 at 01:05:06PM +0200, Sam Ravnborg wrote:
> On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> > and multiple RGB color formats.
> > 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.
> 
> Thanks for submitting this driver. I have added a few comments in the
> following that I hope you will find useful to improve the driver.
> 
> > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > ---
> >  drivers/gpu/drm/xlnx/Kconfig    |  14 ++
> >  drivers/gpu/drm/xlnx/Makefile   |   1 +
> >  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 471 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > new file mode 100644
> > index 0000000..a5291f3
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > +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);
> > +}
> 
> When I see implementations like this I wonder if a regmap would be
> beneficial?

regmap often seems overkill to me when the driver only needs plain
32-bit mmio read/write, given the overhead it adds at runtime. Is it
just me ?

[snip]

-- 
Regards,

Laurent Pinchart

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Laurent Pinchart]

Hi Sam,

On Fri, May 13, 2022 at 01:05:06PM +0200, Sam Ravnborg wrote:
> On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> > and multiple RGB color formats.
> > 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.
> 
> Thanks for submitting this driver. I have added a few comments in the
> following that I hope you will find useful to improve the driver.
> 
> > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > ---
> >  drivers/gpu/drm/xlnx/Kconfig    |  14 ++
> >  drivers/gpu/drm/xlnx/Makefile   |   1 +
> >  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 471 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > new file mode 100644
> > index 0000000..a5291f3
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > +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);
> > +}
> 
> When I see implementations like this I wonder if a regmap would be
> beneficial?

regmap often seems overkill to me when the driver only needs plain
32-bit mmio read/write, given the overhead it adds at runtime. Is it
just me ?

[snip]

-- 
Regards,

Laurent Pinchart

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Sam Ravnborg]

Hi Laurent,

> [snip]
> 
> > > +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);
> > > +}
> > 
> > When I see implementations like this I wonder if a regmap would be
> > beneficial?
> 
> regmap often seems overkill to me when the driver only needs plain
> 32-bit mmio read/write, given the overhead it adds at runtime. Is it
> just me ?

There are several points that speaks for using regmap:
- The interface is well known
- It has nice helpers - like update_bits
- No need for own wrappers, that sometimes are made in creative ways
  (not the case here)
- There is a possibility to add some run-time checks so one can catch
  attempt to write outside the register window, write to read-only
  registers etc.


On top of this - it is simple to configure:

static const struct regmap_config regmap_config = {
        .reg_bits = 32,
        .val_bits = 32,
        .reg_stride = 4,
};


From the probe function:

	priv->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
	if (IS_ERR(priv->regs))
		return dev_err_probe(dev, PTR_ERR(priv->regs), "Failed to get memory resource\n");

	regmap_cfg = regmap_config;
	regmap_cfg.max_register = res->end - res->start;
	priv->regmap = devm_regmap_init_mmio(dev, priv->regs, &regmap_cfg);
	if (IS_ERR(priv->regmap))
		return dev_err_probe(dev, PTR_ERR(priv->regmap), "Failed to init regmap\n");


The one point that brought me over was the well known interface.
But using wrappers works too.

	Sam

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Sam Ravnborg]

Hi Laurent,

> [snip]
> 
> > > +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);
> > > +}
> > 
> > When I see implementations like this I wonder if a regmap would be
> > beneficial?
> 
> regmap often seems overkill to me when the driver only needs plain
> 32-bit mmio read/write, given the overhead it adds at runtime. Is it
> just me ?

There are several points that speaks for using regmap:
- The interface is well known
- It has nice helpers - like update_bits
- No need for own wrappers, that sometimes are made in creative ways
  (not the case here)
- There is a possibility to add some run-time checks so one can catch
  attempt to write outside the register window, write to read-only
  registers etc.


On top of this - it is simple to configure:

static const struct regmap_config regmap_config = {
        .reg_bits = 32,
        .val_bits = 32,
        .reg_stride = 4,
};


From the probe function:

	priv->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
	if (IS_ERR(priv->regs))
		return dev_err_probe(dev, PTR_ERR(priv->regs), "Failed to get memory resource\n");

	regmap_cfg = regmap_config;
	regmap_cfg.max_register = res->end - res->start;
	priv->regmap = devm_regmap_init_mmio(dev, priv->regs, &regmap_cfg);
	if (IS_ERR(priv->regmap))
		return dev_err_probe(dev, PTR_ERR(priv->regmap), "Failed to init regmap\n");


The one point that brought me over was the well known interface.
But using wrappers works too.

	Sam

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Laurent Pinchart]

Hi Sam,

One more comment.

On Fri, May 13, 2022 at 01:05:06PM +0200, Sam Ravnborg wrote:
> On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> > and multiple RGB color formats.
> > 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.
> 
> Thanks for submitting this driver. I have added a few comments in the
> following that I hope you will find useful to improve the driver.
> 
> > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > ---
> >  drivers/gpu/drm/xlnx/Kconfig    |  14 ++
> >  drivers/gpu/drm/xlnx/Makefile   |   1 +
> >  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 471 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > new file mode 100644
> > index 0000000..a5291f3
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > +static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
> > +	.mode_set	= xlnx_dsi_bridge_mode_set,
> 
> From the documentation of the mode_set operation:
>  * This is deprecated, do not use!
>  * New drivers shall set their mode in the
>  * &drm_bridge_funcs.atomic_enable operation.
> 
> Please adjust accordingly.
> 
> > +	.atomic_enable	= xlnx_dsi_bridge_enable,
> > +	.atomic_disable	= xlnx_dsi_bridge_disable,
> > +	.attach		= xlnx_dsi_bridge_attach,
> > +	.detach		= xlnx_dsi_bridge_detach,
> > +};
> 
> For a new bridge please implement all the mandatory atomic operations.
> 
> You will need at least:
> 	.atomic_get_output_bus_fmts = xlnx_dsi_bridge_get_output_bus_fmts,

As this DSI encoder will never be the last bridge in the chain (there
should always be a panel or another bridge afterwards), I think this
function can be skipped.

> 	.atomic_get_input_bus_fmts = xlnx_dsi_bridge_get_input_bus_fmts,
> 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> 	.atomic_reset = drm_atomic_helper_bridge_reset,
> };

[snip]

-- 
Regards,

Laurent Pinchart

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Laurent Pinchart]

Hi Sam,

One more comment.

On Fri, May 13, 2022 at 01:05:06PM +0200, Sam Ravnborg wrote:
> On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> > and multiple RGB color formats.
> > 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.
> 
> Thanks for submitting this driver. I have added a few comments in the
> following that I hope you will find useful to improve the driver.
> 
> > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
> > ---
> >  drivers/gpu/drm/xlnx/Kconfig    |  14 ++
> >  drivers/gpu/drm/xlnx/Makefile   |   1 +
> >  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 471 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > new file mode 100644
> > index 0000000..a5291f3
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c

[snip]

> > +static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
> > +	.mode_set	= xlnx_dsi_bridge_mode_set,
> 
> From the documentation of the mode_set operation:
>  * This is deprecated, do not use!
>  * New drivers shall set their mode in the
>  * &drm_bridge_funcs.atomic_enable operation.
> 
> Please adjust accordingly.
> 
> > +	.atomic_enable	= xlnx_dsi_bridge_enable,
> > +	.atomic_disable	= xlnx_dsi_bridge_disable,
> > +	.attach		= xlnx_dsi_bridge_attach,
> > +	.detach		= xlnx_dsi_bridge_detach,
> > +};
> 
> For a new bridge please implement all the mandatory atomic operations.
> 
> You will need at least:
> 	.atomic_get_output_bus_fmts = xlnx_dsi_bridge_get_output_bus_fmts,

As this DSI encoder will never be the last bridge in the chain (there
should always be a panel or another bridge afterwards), I think this
function can be skipped.

> 	.atomic_get_input_bus_fmts = xlnx_dsi_bridge_get_input_bus_fmts,
> 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> 	.atomic_reset = drm_atomic_helper_bridge_reset,
> };

[snip]

-- 
Regards,

Laurent Pinchart

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Laurent Pinchart]

Hi GVRao,

Thank you for the patch.

On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> and multiple RGB color formats.
> 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    |  14 ++
>  drivers/gpu/drm/xlnx/Makefile   |   1 +
>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 471 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 c3d0826..caa632b 100644
> --- a/drivers/gpu/drm/xlnx/Kconfig
> +++ b/drivers/gpu/drm/xlnx/Kconfig
> @@ -14,3 +14,17 @@ 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"

I'd add

	depends on ARCH_ZYNQMP || COMPILE_TEST

to avoid adding an unnecessary option on unrelated platforms.

> +	depends on DRM && OF
> +	select DRM_KMS_HELPER
> +	select DRM_MIPI_DSI
> +	select DRM_PANEL
> +	select BACKLIGHT_LCD_SUPPORT
> +	select BACKLIGHT_CLASS_DEVICE
> +	select DRM_PANEL_SIMPLE

Could you please sort these alphabetically ?

> +	help
> +	  DRM bridge driver for Xilinx programmable DSI subsystem controller.
> +	  choose this option if you hava a Xilinx MIPI-DSI Tx subsytem in
> +	  video pipeline.
> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> index 51c24b7..1e97fbe 100644
> --- a/drivers/gpu/drm/xlnx/Makefile
> +++ b/drivers/gpu/drm/xlnx/Makefile
> @@ -1,2 +1,3 @@
>  zynqmp-dpsub-y := 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..a5291f3
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> @@ -0,0 +1,456 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *

You can drop this blank line.

> + * Xilinx FPGA MIPI DSI Tx Controller driver.
> + *
> + * Copyright (C) 2022 Xilinx, Inc.
> + *
> + * Author: Venkateshwar Rao G <vgannava@xilinx.com>
> + *

And this one too.

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.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_LANES_MASK		3
> +#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_PIXELFORMAT_MASK	(0x3 << 11)
> +#define XDSI_PCR_PIXELFORMAT_SHIFT	11
> +#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_NUM_DATA_T			4
> +
> +/**
> + * struct xlnx_dsi - Xilinx DSI-TX core
> + * @bridge: DRM bridge structure
> + * @dsi_host: DSI host device
> + * @panel_bridge: Panel bridge structure
> + * @panel:  DRM panel structure
> + * @dev: device structure
> + * @clks: clock source structure
> + * @iomem: Base address of DSI subsystem
> + * @mode_flags: DSI operation mode related flags
> + * @lanes: number of active data lanes supported by DSI controller
> + * @mul_factor: multiplication factor for HACT timing
> + * @format: pixel format for video mode of DSI controller
> + * @device_found: Flag to indicate device presence
> + */
> +struct xlnx_dsi {
> +	struct drm_bridge bridge;
> +	struct mipi_dsi_host dsi_host;
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
> +	struct device *dev;
> +	struct clk_bulk_data *clks;
> +	void __iomem *iomem;
> +	unsigned long mode_flags;
> +	u32 lanes;
> +	u32 mul_factor;
> +	enum mipi_dsi_pixel_format format;
> +	bool device_found;
> +};
> +
> +static const struct clk_bulk_data xdsi_clks[] = {
> +	{ .id = "s_axis_aclk" },
> +	{ .id = "dphy_clk_200M" },
> +};
> +
> +static inline struct xlnx_dsi *host_to_dsi(struct mipi_dsi_host *host)
> +{
> +	return container_of(host, struct xlnx_dsi, dsi_host);
> +}
> +
> +static inline struct xlnx_dsi *bridge_to_dsi(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct xlnx_dsi, bridge);
> +}
> +
> +static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val)

I'd padd the xlnx_dsi pointer instead of the base pointer to the read
and write functions. I would also rename this to xlnx_dsi_write() (and
same for read).

> +{
> +	writel(val, base + offset);
> +}
> +
> +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static int xlnx_dsi_panel_or_bridge(struct xlnx_dsi *dsi,
> +				    struct device_node *node)
> +{
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
> +	struct device *dev = dsi->dev;
> +	struct device_node *endpoint = dev->of_node;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(endpoint, 1, 0, &panel, &panel_bridge);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "failed to find panel / bridge\n");
> +		return ret;
> +	}
> +
> +	if (panel) {
> +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +		if (IS_ERR(panel_bridge))
> +			return PTR_ERR(panel_bridge);
> +		dsi->panel = panel;
> +	}
> +
> +	dsi->panel_bridge = panel_bridge;
> +
> +	if (!dsi->panel_bridge) {
> +		dev_err(dsi->dev, "panel not found\n");
> +		return -EPROBE_DEFER;
> +	}

Can this happen ?

> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
> +				struct mipi_dsi_device *device)
> +{
> +	struct xlnx_dsi *dsi = host_to_dsi(host);
> +	u32 reg;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	dsi->lanes = reg & XDSI_PCR_LANES_MASK;
> +	dsi->format = (reg & XDSI_PCR_PIXELFORMAT_MASK) >>
> +		XDSI_PCR_PIXELFORMAT_SHIFT;
> +	dsi->mode_flags = device->mode_flags;
> +
> +	if (dsi->lanes != device->lanes) {
> +		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
> +			device->lanes, dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->lanes > 4 || dsi->lanes < 1) {
> +		dev_err(dsi->dev, "%d lanes : invalid xlnx,dsi-num-lanes\n",

The driver doesn't use this DT property.

Could the number of lanes be read and validated at probe time ? The
format could also be read at probe time.

> +			dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->format != device->format) {
> +		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
> +			device->format, dsi->format);
> +		return -EINVAL;
> +	}

The usual pattern is to call drm_bridge_add() here (and
drm_bridge_remove() in the DSI detach function).

> +
> +	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_disable(dsi->panel);
> +		dsi->panel = NULL;
> +	}

This isn't needed, the panel is handled through a panel bridge which
will take care of disabling the panel.

> +
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
> +	.attach = xlnx_dsi_host_attach,
> +	.detach	= xlnx_dsi_host_detach,
> +};
> +
> +static void
> +xlnx_dsi_bridge_disable(struct drm_bridge *bridge,
> +			struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 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\n");

I'd drop the message, I don't think it's very useful. Same in
xlnx_dsi_bridge_enable().

> +}

Let's move this function after xlnx_dsi_bridge_enable() to match the
order in drm_bridge_funcs.

> +
> +static void
> +xlnx_dsi_bridge_mode_set(struct drm_bridge *bridge,
> +			 const struct drm_display_mode *mode,
> +			 const struct drm_display_mode *adjusted_mode)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 reg, video_mode;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >> XDSI_PCR_VIDEOMODE_SHIFT;
> +
> +	if (!video_mode && (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
> +		reg = XDSI_TIME1_HSA(adjusted_mode->hsync_end -
> +				     adjusted_mode->hsync_start);
> +		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
> +	}
> +
> +	reg = XDSI_TIME4_VFP(adjusted_mode->vsync_start -
> +			     adjusted_mode->vdisplay) |
> +		XDSI_TIME4_VBP(adjusted_mode->vtotal -
> +			       adjusted_mode->vsync_end) |
> +		XDSI_TIME4_VSA(adjusted_mode->vsync_end -
> +			       adjusted_mode->vsync_start);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
> +
> +	reg = XDSI_TIME3_HFP(adjusted_mode->hsync_start -
> +			     adjusted_mode->hdisplay) |
> +		XDSI_TIME3_HBP(adjusted_mode->htotal -
> +			       adjusted_mode->hsync_end);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
> +	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
> +		(dsi->mul_factor) / 100);

This is already printed in xlnx_dsi_parse_dt(), no need to repeat it
here.

> +
> +	if ((adjusted_mode->hdisplay & XDSI_HACT_MULTIPLIER) != 0)
> +		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
> +
> +	reg = XDSI_TIME2_HACT((adjusted_mode->hdisplay) * (dsi->mul_factor) / 100) |

No need for the inner parentheses.

> +		XDSI_TIME2_VACT(adjusted_mode->vdisplay);
> +
> +	xlnx_dsi_writel(dsi->iomem, XDSI_PCR, XDSI_PCR_VIDEOMODE(BIT(0)));
> +}
> +
> +static void xlnx_dsi_bridge_enable(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	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");
> +}
> +
> +static int xlnx_dsi_bridge_attach(struct drm_bridge *bridge,
> +				  enum drm_bridge_attach_flags flags)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	if (!bridge->encoder) {

Can this ever happen ?

> +		DRM_ERROR("Parent encoder object not found\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Set the encoder type as caller does not know it */
> +	bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;

I think you can skip this. The encoder type shouldn't be used by
userspace, it should be fine to leave it set to DRM_MODE_ENCODER_NONE.
If setting the type was required, it should be done by the
drm_bridge_connector helper, not by this driver.

> +
> +	if (!dsi->device_found) {
> +		int ret;
> +
> +		ret = xlnx_dsi_panel_or_bridge(dsi, dsi->dev->of_node);
> +		if (ret) {
> +			dev_err(dsi->dev, "dsi_panel_or_bridge failed\n");
> +			return ret;
> +		}
> +
> +		dsi->device_found = true;
> +	}

I'd move this to the DSI attach function. See cdns-dsi.c for an example.

> +
> +	/* Attach the panel-bridge to the dsi bridge */
> +	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge,
> +				 flags);
> +}
> +
> +static void xlnx_dsi_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
> +}
> +
> +static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
> +	.mode_set	= xlnx_dsi_bridge_mode_set,
> +	.atomic_enable	= xlnx_dsi_bridge_enable,
> +	.atomic_disable	= xlnx_dsi_bridge_disable,
> +	.attach		= xlnx_dsi_bridge_attach,
> +	.detach		= xlnx_dsi_bridge_detach,
> +};
> +
> +static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi)
> +{
> +	struct device *dev = dsi->dev;
> +	struct device_node *node = dev->of_node;

	struct device_node *node = dsi->dev->of_node;

> +	int ret;
> +	u32 datatype;
> +	static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225, 200};

Please move the static variables first.

> +
> +	/*
> +	 * Used as a multiplication factor for HACT based on used
> +	 * DSI 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 is a floating number,
> +	 * a 100x multiplication factor is used.
> +	 */
> +	ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);

This property needs to be documented in the bindings.

> +	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");

It's not a string.

> +		return -EINVAL;
> +	}
> +	dsi->mul_factor = xdsi_mul_fact[datatype];
> +
> +	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);

dsi->lanes isn't set yet here, you can drop this message.

> +	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct xlnx_dsi *dsi;
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dev = dev;
> +	dsi->clks = devm_kmemdup(dev, xdsi_clks, sizeof(xdsi_clks),
> +				 GFP_KERNEL);
> +	if (!dsi->clks)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dsi->iomem = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dsi->iomem))
> +		return PTR_ERR(dsi->iomem);
> +
> +	ret = clk_bulk_get(dev, num_clks, dsi->clks);

You can use devm_clk_bulk_get() and drop the clk_bulk_put() calls.

> +	if (ret)
> +		return ret;
> +
> +	ret = xlnx_dsi_parse_dt(dsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_bulk_prepare_enable(num_clks, dsi->clks);

As this will become the only use of num_clks, I'd write

	ret = clk_bulk_prepare_enable(ARRAY_SIZE(xdsi_clks), dsi->clks);

and drop the local variable. Same in remove().

Actually, shouldn't the driver implement runtime PM suspand and resume
handlers, and enable/disable clocks there ?

> +	if (ret)
> +		goto err_clk_put;
> +
> +	platform_set_drvdata(pdev, dsi);
> +	dsi->dsi_host.ops = &xlnx_dsi_ops;
> +	dsi->dsi_host.dev = dev;
> +
> +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> +	if (ret) {
> +		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
> +		goto err_clk_put;
> +	}
> +
> +	dsi->bridge.driver_private = dsi;
> +	dsi->bridge.funcs = &xlnx_dsi_bridge_funcs;
> +#ifdef CONFIG_OF

You depend on OF, so this isn't needed.

> +	dsi->bridge.of_node = pdev->dev.of_node;
> +#endif
> +
> +	drm_bridge_add(&dsi->bridge);
> +
> +err_clk_put:
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return ret;
> +}
> +
> +static int xlnx_dsi_remove(struct platform_device *pdev)
> +{
> +	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +	clk_bulk_disable_unprepare(num_clks, dsi->clks);
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xlnx_dsi_of_match[] = {
> +	{ .compatible = "xlnx,dsi-tx-v2.0"},
> +	{ }
> +};
> +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("Venkateshwar Rao G <vgannava@xilinx.com>");
> +MODULE_DESCRIPTION("Xilinx MIPI DSI host controller driver");
> +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart

Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem

[Laurent Pinchart]

Hi GVRao,

Thank you for the patch.

On Thu, May 12, 2022 at 07:23:13PM +0530, 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
> and multiple RGB color formats.
> 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    |  14 ++
>  drivers/gpu/drm/xlnx/Makefile   |   1 +
>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 471 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 c3d0826..caa632b 100644
> --- a/drivers/gpu/drm/xlnx/Kconfig
> +++ b/drivers/gpu/drm/xlnx/Kconfig
> @@ -14,3 +14,17 @@ 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"

I'd add

	depends on ARCH_ZYNQMP || COMPILE_TEST

to avoid adding an unnecessary option on unrelated platforms.

> +	depends on DRM && OF
> +	select DRM_KMS_HELPER
> +	select DRM_MIPI_DSI
> +	select DRM_PANEL
> +	select BACKLIGHT_LCD_SUPPORT
> +	select BACKLIGHT_CLASS_DEVICE
> +	select DRM_PANEL_SIMPLE

Could you please sort these alphabetically ?

> +	help
> +	  DRM bridge driver for Xilinx programmable DSI subsystem controller.
> +	  choose this option if you hava a Xilinx MIPI-DSI Tx subsytem in
> +	  video pipeline.
> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> index 51c24b7..1e97fbe 100644
> --- a/drivers/gpu/drm/xlnx/Makefile
> +++ b/drivers/gpu/drm/xlnx/Makefile
> @@ -1,2 +1,3 @@
>  zynqmp-dpsub-y := 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..a5291f3
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> @@ -0,0 +1,456 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *

You can drop this blank line.

> + * Xilinx FPGA MIPI DSI Tx Controller driver.
> + *
> + * Copyright (C) 2022 Xilinx, Inc.
> + *
> + * Author: Venkateshwar Rao G <vgannava@xilinx.com>
> + *

And this one too.

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.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_LANES_MASK		3
> +#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_PIXELFORMAT_MASK	(0x3 << 11)
> +#define XDSI_PCR_PIXELFORMAT_SHIFT	11
> +#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_NUM_DATA_T			4
> +
> +/**
> + * struct xlnx_dsi - Xilinx DSI-TX core
> + * @bridge: DRM bridge structure
> + * @dsi_host: DSI host device
> + * @panel_bridge: Panel bridge structure
> + * @panel:  DRM panel structure
> + * @dev: device structure
> + * @clks: clock source structure
> + * @iomem: Base address of DSI subsystem
> + * @mode_flags: DSI operation mode related flags
> + * @lanes: number of active data lanes supported by DSI controller
> + * @mul_factor: multiplication factor for HACT timing
> + * @format: pixel format for video mode of DSI controller
> + * @device_found: Flag to indicate device presence
> + */
> +struct xlnx_dsi {
> +	struct drm_bridge bridge;
> +	struct mipi_dsi_host dsi_host;
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
> +	struct device *dev;
> +	struct clk_bulk_data *clks;
> +	void __iomem *iomem;
> +	unsigned long mode_flags;
> +	u32 lanes;
> +	u32 mul_factor;
> +	enum mipi_dsi_pixel_format format;
> +	bool device_found;
> +};
> +
> +static const struct clk_bulk_data xdsi_clks[] = {
> +	{ .id = "s_axis_aclk" },
> +	{ .id = "dphy_clk_200M" },
> +};
> +
> +static inline struct xlnx_dsi *host_to_dsi(struct mipi_dsi_host *host)
> +{
> +	return container_of(host, struct xlnx_dsi, dsi_host);
> +}
> +
> +static inline struct xlnx_dsi *bridge_to_dsi(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct xlnx_dsi, bridge);
> +}
> +
> +static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val)

I'd padd the xlnx_dsi pointer instead of the base pointer to the read
and write functions. I would also rename this to xlnx_dsi_write() (and
same for read).

> +{
> +	writel(val, base + offset);
> +}
> +
> +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static int xlnx_dsi_panel_or_bridge(struct xlnx_dsi *dsi,
> +				    struct device_node *node)
> +{
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
> +	struct device *dev = dsi->dev;
> +	struct device_node *endpoint = dev->of_node;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(endpoint, 1, 0, &panel, &panel_bridge);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "failed to find panel / bridge\n");
> +		return ret;
> +	}
> +
> +	if (panel) {
> +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +		if (IS_ERR(panel_bridge))
> +			return PTR_ERR(panel_bridge);
> +		dsi->panel = panel;
> +	}
> +
> +	dsi->panel_bridge = panel_bridge;
> +
> +	if (!dsi->panel_bridge) {
> +		dev_err(dsi->dev, "panel not found\n");
> +		return -EPROBE_DEFER;
> +	}

Can this happen ?

> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,
> +				struct mipi_dsi_device *device)
> +{
> +	struct xlnx_dsi *dsi = host_to_dsi(host);
> +	u32 reg;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	dsi->lanes = reg & XDSI_PCR_LANES_MASK;
> +	dsi->format = (reg & XDSI_PCR_PIXELFORMAT_MASK) >>
> +		XDSI_PCR_PIXELFORMAT_SHIFT;
> +	dsi->mode_flags = device->mode_flags;
> +
> +	if (dsi->lanes != device->lanes) {
> +		dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
> +			device->lanes, dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->lanes > 4 || dsi->lanes < 1) {
> +		dev_err(dsi->dev, "%d lanes : invalid xlnx,dsi-num-lanes\n",

The driver doesn't use this DT property.

Could the number of lanes be read and validated at probe time ? The
format could also be read at probe time.

> +			dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	if (dsi->format != device->format) {
> +		dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
> +			device->format, dsi->format);
> +		return -EINVAL;
> +	}

The usual pattern is to call drm_bridge_add() here (and
drm_bridge_remove() in the DSI detach function).

> +
> +	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_disable(dsi->panel);
> +		dsi->panel = NULL;
> +	}

This isn't needed, the panel is handled through a panel bridge which
will take care of disabling the panel.

> +
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops xlnx_dsi_ops = {
> +	.attach = xlnx_dsi_host_attach,
> +	.detach	= xlnx_dsi_host_detach,
> +};
> +
> +static void
> +xlnx_dsi_bridge_disable(struct drm_bridge *bridge,
> +			struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 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\n");

I'd drop the message, I don't think it's very useful. Same in
xlnx_dsi_bridge_enable().

> +}

Let's move this function after xlnx_dsi_bridge_enable() to match the
order in drm_bridge_funcs.

> +
> +static void
> +xlnx_dsi_bridge_mode_set(struct drm_bridge *bridge,
> +			 const struct drm_display_mode *mode,
> +			 const struct drm_display_mode *adjusted_mode)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	u32 reg, video_mode;
> +
> +	reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
> +	video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >> XDSI_PCR_VIDEOMODE_SHIFT;
> +
> +	if (!video_mode && (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
> +		reg = XDSI_TIME1_HSA(adjusted_mode->hsync_end -
> +				     adjusted_mode->hsync_start);
> +		xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
> +	}
> +
> +	reg = XDSI_TIME4_VFP(adjusted_mode->vsync_start -
> +			     adjusted_mode->vdisplay) |
> +		XDSI_TIME4_VBP(adjusted_mode->vtotal -
> +			       adjusted_mode->vsync_end) |
> +		XDSI_TIME4_VSA(adjusted_mode->vsync_end -
> +			       adjusted_mode->vsync_start);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
> +
> +	reg = XDSI_TIME3_HFP(adjusted_mode->hsync_start -
> +			     adjusted_mode->hdisplay) |
> +		XDSI_TIME3_HBP(adjusted_mode->htotal -
> +			       adjusted_mode->hsync_end);
> +	xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
> +	dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",
> +		(dsi->mul_factor) / 100);

This is already printed in xlnx_dsi_parse_dt(), no need to repeat it
here.

> +
> +	if ((adjusted_mode->hdisplay & XDSI_HACT_MULTIPLIER) != 0)
> +		dev_warn(dsi->dev, "Incorrect HACT will be programmed\n");
> +
> +	reg = XDSI_TIME2_HACT((adjusted_mode->hdisplay) * (dsi->mul_factor) / 100) |

No need for the inner parentheses.

> +		XDSI_TIME2_VACT(adjusted_mode->vdisplay);
> +
> +	xlnx_dsi_writel(dsi->iomem, XDSI_PCR, XDSI_PCR_VIDEOMODE(BIT(0)));
> +}
> +
> +static void xlnx_dsi_bridge_enable(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *old_bridge_state)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +	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");
> +}
> +
> +static int xlnx_dsi_bridge_attach(struct drm_bridge *bridge,
> +				  enum drm_bridge_attach_flags flags)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	if (!bridge->encoder) {

Can this ever happen ?

> +		DRM_ERROR("Parent encoder object not found\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Set the encoder type as caller does not know it */
> +	bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;

I think you can skip this. The encoder type shouldn't be used by
userspace, it should be fine to leave it set to DRM_MODE_ENCODER_NONE.
If setting the type was required, it should be done by the
drm_bridge_connector helper, not by this driver.

> +
> +	if (!dsi->device_found) {
> +		int ret;
> +
> +		ret = xlnx_dsi_panel_or_bridge(dsi, dsi->dev->of_node);
> +		if (ret) {
> +			dev_err(dsi->dev, "dsi_panel_or_bridge failed\n");
> +			return ret;
> +		}
> +
> +		dsi->device_found = true;
> +	}

I'd move this to the DSI attach function. See cdns-dsi.c for an example.

> +
> +	/* Attach the panel-bridge to the dsi bridge */
> +	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge,
> +				 flags);
> +}
> +
> +static void xlnx_dsi_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct xlnx_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
> +}
> +
> +static const struct drm_bridge_funcs xlnx_dsi_bridge_funcs = {
> +	.mode_set	= xlnx_dsi_bridge_mode_set,
> +	.atomic_enable	= xlnx_dsi_bridge_enable,
> +	.atomic_disable	= xlnx_dsi_bridge_disable,
> +	.attach		= xlnx_dsi_bridge_attach,
> +	.detach		= xlnx_dsi_bridge_detach,
> +};
> +
> +static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi)
> +{
> +	struct device *dev = dsi->dev;
> +	struct device_node *node = dev->of_node;

	struct device_node *node = dsi->dev->of_node;

> +	int ret;
> +	u32 datatype;
> +	static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225, 200};

Please move the static variables first.

> +
> +	/*
> +	 * Used as a multiplication factor for HACT based on used
> +	 * DSI 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 is a floating number,
> +	 * a 100x multiplication factor is used.
> +	 */
> +	ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);

This property needs to be documented in the bindings.

> +	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");

It's not a string.

> +		return -EINVAL;
> +	}
> +	dsi->mul_factor = xdsi_mul_fact[datatype];
> +
> +	dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);

dsi->lanes isn't set yet here, you can drop this message.

> +	dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
> +
> +	return 0;
> +}
> +
> +static int xlnx_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct xlnx_dsi *dsi;
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dev = dev;
> +	dsi->clks = devm_kmemdup(dev, xdsi_clks, sizeof(xdsi_clks),
> +				 GFP_KERNEL);
> +	if (!dsi->clks)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dsi->iomem = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dsi->iomem))
> +		return PTR_ERR(dsi->iomem);
> +
> +	ret = clk_bulk_get(dev, num_clks, dsi->clks);

You can use devm_clk_bulk_get() and drop the clk_bulk_put() calls.

> +	if (ret)
> +		return ret;
> +
> +	ret = xlnx_dsi_parse_dt(dsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_bulk_prepare_enable(num_clks, dsi->clks);

As this will become the only use of num_clks, I'd write

	ret = clk_bulk_prepare_enable(ARRAY_SIZE(xdsi_clks), dsi->clks);

and drop the local variable. Same in remove().

Actually, shouldn't the driver implement runtime PM suspand and resume
handlers, and enable/disable clocks there ?

> +	if (ret)
> +		goto err_clk_put;
> +
> +	platform_set_drvdata(pdev, dsi);
> +	dsi->dsi_host.ops = &xlnx_dsi_ops;
> +	dsi->dsi_host.dev = dev;
> +
> +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> +	if (ret) {
> +		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
> +		goto err_clk_put;
> +	}
> +
> +	dsi->bridge.driver_private = dsi;
> +	dsi->bridge.funcs = &xlnx_dsi_bridge_funcs;
> +#ifdef CONFIG_OF

You depend on OF, so this isn't needed.

> +	dsi->bridge.of_node = pdev->dev.of_node;
> +#endif
> +
> +	drm_bridge_add(&dsi->bridge);
> +
> +err_clk_put:
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return ret;
> +}
> +
> +static int xlnx_dsi_remove(struct platform_device *pdev)
> +{
> +	struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
> +	int num_clks = ARRAY_SIZE(xdsi_clks);
> +
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +	clk_bulk_disable_unprepare(num_clks, dsi->clks);
> +	clk_bulk_put(num_clks, dsi->clks);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xlnx_dsi_of_match[] = {
> +	{ .compatible = "xlnx,dsi-tx-v2.0"},
> +	{ }
> +};
> +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("Venkateshwar Rao G <vgannava@xilinx.com>");
> +MODULE_DESCRIPTION("Xilinx MIPI DSI host controller driver");
> +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart