Hi Sam, thanks for your comments.

-----Original Message----- From: Sam Ravnborg sam@ravnborg.org Sent: Sunday, April 26, 2020 1:59 AM To: Venkateshwar Rao Gannavarapu VGANNAVA@xilinx.com Cc: Hyun Kwon hyunk@xilinx.com; laurent.pinchart@ideasonboard.com; dri- devel@lists.freedesktop.org; Sandip Kothari sandipk@xilinx.com; airlied@linux.ie; linux-kernel@vger.kernel.org; Venkateshwar Rao Gannavarapu VGANNAVA@xilinx.com Subject: Re: [RFC PATCH 1/2] dt-bindings: display: xlnx: Add Xilinx DSI TX subsystem bindings

Hi Venkateshwar

On Tue, Apr 21, 2020 at 02:50:55AM +0530, Venkateshwar Rao Gannavarapu wrote:

...

This add a dt binding for Xilinx DSI TX subsystem.

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

(FPGA).

...

Signed-off-by: Venkateshwar Rao Gannavarapu

venkateshwar.rao.gannavarapu@xilinx.com

.../devicetree/bindings/display/xlnx/xlnx,dsi.txt | 68 ++++++++++++++++++++++

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

I will address bindings in YAML format in V2 patch.

...

1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt

diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt new file mode 100644 index 0000000..ef69729 --- /dev/null +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,dsi.txt @@ -0,0 +1,68 @@ +Device-Tree bindings for Xilinx MIPI DSI Tx IP core

+The IP core supports transmission of video data in MIPI DSI protocol.

+Required properties:

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

+"MIPI_DSI_FMT_RGB888",

• "MIPI_DSI_FMT_RGB666", "MIPI_DSI_FMT_RGB666_PACKED" or

"MIPI_DSI_FMT_RGB565".

...

• • #address-cells, #size-cells: should be set respectively to <1> and <0>
• according to DSI host bindings (see MIPI DSI bindings [1])
• • clock-names: Must contain "s_axis_aclk" and "dphy_clk_200M" in same

order as

...

• clocks listed in clocks property.
• • clocks: List of phandles to Video and 200Mhz DPHY clocks.
• • port: Logical block can be used / connected independently with
• external device. In the display controller port nodes, topology
• for entire pipeline should be described using the DT bindings defined in
• Documentation/devicetree/bindings/graph.txt.

...

• • simple_panel: The subnode for connected panel. This represents the
• DSI peripheral connected to the DSI host node. Please refer to
• Documentation/devicetree/bindings/display/mipi-dsi-bus.txt. The
• simple-panel driver has auo,b101uan01 panel timing parameters added

along

...

• with other existing panels. DSI driver derive the required Tx IP controller
• timing values from the panel timing parameters.A

Please always use either a port or a ports node.

OK.

Sam

...

+Required simple_panel properties:

• • compatible: Value should be one of the panel names in
• Documentation/devicetree/bindings/display/panel/. e.g. "auo,b101uan01".
• For available panel compatible strings, please refer to bindings in
• Documentation/devicetree/bindings/display/panel/

+Optional properties:

• • xlnx,dsi-cmd-mode: denotes command mode enable.

+[1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt

+Example:

•   mipi_dsi_tx_subsystem@80000000 {
  

•           compatible = "xlnx,dsi";
  

•           reg = <0x0 0x80000000 0x0 0x10000>;
  

•           xlnx,dsi-num-lanes = <4>;
  

•           xlnx,dsi-data-type = <MIPI_DSI_FMT_RGB888>;
  

•           #address-cells = <1>;
  

•           #size-cells = <0>;
  

•           clock-names = "dphy_clk_200M", "s_axis_aclk";
  

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

•           encoder_dsi_port: port@0 {
  

•                   reg = <0>;
  

•                   dsi_encoder: endpoint {
  

•                           remote-endpoint = <&xdsi_ep>;
  

•                   };
  

•           };
  

•           simple_panel: simple-panel@0 {
  

•                   compatible = "auo,b101uan01";
  

•                   reg = <0>;
  

•                   };
  

•           };
  

-- 2.7.4

This email and any attachments are intended for the sole use of the named

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

...

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi GVRao,

Thank you for the patch.

On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:

On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:

...

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

It supports upto 4 lanes, optional register interface for the DPHY,

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

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

...

multiple RGB color formats, command mode and video mode. This is a MIPI-DSI host driver and provides DSI bus for panels. This driver also helps to communicate with its panel using panel framework.

Signed-off-by: Venkateshwar Rao Gannavarapu venkateshwar.rao.gannavarapu@xilinx.com

drivers/gpu/drm/xlnx/Kconfig | 11 + drivers/gpu/drm/xlnx/Makefile | 2 + drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++

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

...

3 files changed, 768 insertions(+) create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig index aa6cd88..73873cf 100644 --- a/drivers/gpu/drm/xlnx/Kconfig +++ b/drivers/gpu/drm/xlnx/Kconfig @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose this option if you have a Xilinx ZynqMP SoC with DisplayPort subsystem.

+config DRM_XLNX_DSI

•    tristate "Xilinx DRM DSI Subsystem Driver"
  

•    select DRM_MIPI_DSI
  

•    select DRM_PANEL
  

•    select DRM_PANEL_SIMPLE
  

•    help
  

•  This enables support for Xilinx MIPI-DSI.
  

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

...

•  This is a DRM/KMS driver for Xilinx programmable DSI controller.
  

•  Choose this option if you have a Xilinx MIPI DSI-TX controller
  

•  subsytem.
  

These seem incorrectly indented.

...

diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile index 2b844c6..b7ee6ef 100644 --- a/drivers/gpu/drm/xlnx/Makefile +++ b/drivers/gpu/drm/xlnx/Makefile @@ -1,2 +1,4 @@ zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o

+obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c new file mode 100644 index 0000000..b8cae59 --- /dev/null +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c @@ -0,0 +1,755 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Xilinx FPGA MIPI DSI Tx Controller driver
• • Copyright (C) 2017 - 2019 Xilinx, Inc.
• • Authors:
• • • Saurabh Sengar saurabhs@xilinx.com
• • • Venkateshwar Rao Gannavarapu venkateshwar.rao.gannavarapu@xilinx.com
• */

+#include <drm/drm_atomic_helper.h> +#include <drm/drm_connector.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_device.h> +#include <drm/drm_encoder.h> +#include <drm/drm_fourcc.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h> +#include <drm/drm_probe_helper.h>

+#include <linux/clk.h> +#include <linux/component.h> +#include <linux/device.h> +#include <linux/iopoll.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/phy/phy.h>

+#include <video/mipi_display.h> +#include <video/videomode.h>

+/* DSI Tx IP registers */ +#define XDSI_CCR 0x00 +#define XDSI_CCR_COREENB BIT(0) +#define XDSI_CCR_SOFTRST BIT(1) +#define XDSI_CCR_CRREADY BIT(2) +#define XDSI_CCR_CMDMODE BIT(3) +#define XDSI_CCR_DFIFORST BIT(4) +#define XDSI_CCR_CMDFIFORST BIT(5) +#define XDSI_PCR 0x04 +#define XDSI_PCR_VIDEOMODE(x) (((x) & 0x3) << 3) +#define XDSI_PCR_VIDEOMODE_MASK (0x3 << 3) +#define XDSI_PCR_VIDEOMODE_SHIFT 3 +#define XDSI_PCR_BLLPTYPE(x) ((x) << 5) +#define XDSI_PCR_BLLPMODE(x) ((x) << 6) +#define XDSI_PCR_EOTPENABLE(x) ((x) << 13) +#define XDSI_GIER 0x20 +#define XDSI_ISR 0x24 +#define XDSI_IER 0x28 +#define XDSI_STR 0x2C +#define XDSI_STR_RDY_SHPKT BIT(6) +#define XDSI_STR_RDY_LNGPKT BIT(7) +#define XDSI_STR_DFIFO_FULL BIT(8) +#define XDSI_STR_DFIFO_EMPTY BIT(9) +#define XDSI_STR_WAITFR_DATA BIT(10) +#define XDSI_STR_CMD_EXE_PGS BIT(11) +#define XDSI_STR_CCMD_PROC BIT(12) +#define XDSI_STR_LPKT_MASK (0x5 << 7) +#define XDSI_CMD 0x30 +#define XDSI_CMD_QUEUE_PACKET(x) ((x) & GENMASK(23, 0)) +#define XDSI_DFR 0x34 +#define XDSI_TIME1 0x50 +#define XDSI_TIME1_BLLP_BURST(x) ((x) & GENMASK(15, 0)) +#define XDSI_TIME1_HSA(x) (((x) & GENMASK(15, 0)) << 16) +#define XDSI_TIME2 0x54 +#define XDSI_TIME2_VACT(x) ((x) & GENMASK(15, 0)) +#define XDSI_TIME2_HACT(x) (((x) & GENMASK(15, 0)) << 16) +#define XDSI_HACT_MULTIPLIER GENMASK(1, 0) +#define XDSI_TIME3 0x58 +#define XDSI_TIME3_HFP(x) ((x) & GENMASK(15, 0)) +#define XDSI_TIME3_HBP(x) (((x) & GENMASK(15, 0)) << 16) +#define XDSI_TIME4 0x5c +#define XDSI_TIME4_VFP(x) ((x) & GENMASK(7, 0)) +#define XDSI_TIME4_VBP(x) (((x) & GENMASK(7, 0)) << 8) +#define XDSI_TIME4_VSA(x) (((x) & GENMASK(7, 0)) << 16) +#define XDSI_LTIME 0x60 +#define XDSI_BLLP_TIME 0x64 +/*

• • XDSI_NUM_DATA_T represents number of data types in the
• • enum mipi_dsi_pixel_format in the MIPI DSI part of DRM framework.
• */

+#define XDSI_NUM_DATA_T 4 +#define XDSI_VIDEO_MODE_SYNC_PULSE 0x0 +#define XDSI_VIDEO_MODE_SYNC_EVENT 0x1 +#define XDSI_VIDEO_MODE_BURST 0x2

Please remove these unused 3 definitions.

...

+#define XDSI_DPHY_CLK_MIN 197000000000UL +#define XDSI_DPHY_CLK_MAX 203000000000UL +#define XDSI_DPHY_CLK_REQ 200000000000UL

+/* command timeout in usec */ +#define XDSI_CMD_TIMEOUT_VAL (3000)

+/**

• • struct xlnx_dsi - Xilinx DSI-TX core
• • @encoder: DRM encoder structure
• • @dsi_host: DSI host device
• • @connector: DRM connector structure
• • @panel_node: MIPI DSI device panel node
• • @panel: DRM panel structure
• • @dev: device structure
• • @iomem: Base address of DSI subsystem
• • @lanes: number of active data lanes supported by DSI controller
• • @cmdmode: command mode support
• • @mode_flags: DSI operation mode related flags
• • @format: pixel format for video mode of DSI controller
• • @vm: videomode data structure
• • @mul_factor: multiplication factor for HACT timing parameter
• • @video_aclk: Video clock
• • @dphy_clk_200M: 200MHz DPHY clock and AXI Lite clock
• */

+struct xlnx_dsi {

• struct drm_encoder encoder;
• struct mipi_dsi_host dsi_host;
• struct drm_connector connector;
• struct device_node *panel_node;
• struct drm_panel *panel;
• struct device *dev;
• void __iomem *iomem;
• u32 lanes;
• bool cmdmode;
• u32 mode_flags;
• enum mipi_dsi_pixel_format format;
• struct videomode vm;
• u32 mul_factor;
• struct clk *video_aclk;
• struct clk *dphy_clk_200M;

+};

+#define host_to_dsi(host) container_of(host, struct xlnx_dsi, dsi_host) +#define connector_to_dsi(c) container_of(c, struct xlnx_dsi, connector) +#define encoder_to_dsi(e) container_of(e, struct xlnx_dsi, encoder)

+static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val) +{

• writel(val, base + offset);

+}

+static inline u32 xlnx_dsi_readl(void __iomem *base, int offset) +{

• return readl(base + offset);

+}

+/**

• • xlnx_dsi_set_display_mode - Configure DSI timing registers
• • @dsi: xilinx DSI structure
• • This function writes the timing parameters of DSI IP which are
• • retrieved from panel timing values.
• */

+static void xlnx_dsi_set_display_mode(struct xlnx_dsi *dsi)

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

...

+{

• struct videomode *vm = &dsi->vm;
• u32 reg, video_mode;
• reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
• video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >>

•       XDSI_PCR_VIDEOMODE_SHIFT;
  

• /* configure the HSA value only if non_burst_sync_pluse video mode */
• if (!video_mode &&

•    (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
  

• reg = XDSI_TIME1_HSA(vm->hsync_len);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
  

• }
• reg = XDSI_TIME4_VFP(vm->vfront_porch) |

•      XDSI_TIME4_VBP(vm->vback_porch) |
  

•      XDSI_TIME4_VSA(vm->vsync_len);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
• reg = XDSI_TIME3_HFP(vm->hfront_porch) |

•      XDSI_TIME3_HBP(vm->hback_porch);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
• dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",

• (dsi->mul_factor) / 100);
  

Remove unneeded ().

...

• /*

• * The HACT parameter received from panel timing values should be
  

• * divisible by 4. The reason for this is, the word count given as
  

• * input to DSI controller is HACT * mul_factor. The mul_factor is
  

• * 3, 2.25, 2.25, 2 respectively for RGB888, RGB666_L, RGB666_P and
  

• * RGB565.
  

• * e.g. for RGB666_L color format and 1080p, the word count is
  

• * 1920*2.25 = 4320 which is divisible by 4 and it is a valid input
  

• * to DSI controller. Based on this 2.25 mul factor, we come up with
  

• * the division factor of (XDSI_HACT_MULTIPLIER) as 4 for checking
  

• */
  

• if ((vm->hactive & XDSI_HACT_MULTIPLIER) != 0)

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

...

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

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

...

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

Ditto. No ().

...

•      XDSI_TIME2_VACT(vm->vactive);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME2, reg);
• dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive);

+}

+/**

• • xlnx_dsi_set_display_enable - Enables the DSI Tx IP core enable
• • register bit
• • @dsi: Xilinx DSI structure
• • This function takes the DSI strucure and enables the core enable bit
• • of core configuration register.
• */

+static void xlnx_dsi_set_display_enable(struct xlnx_dsi *dsi) +{

• u32 reg;
• reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
• reg |= XDSI_CCR_COREENB;

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

...

• xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
• dev_dbg(dsi->dev, "MIPI DSI Tx controller is enabled.\n");

+}

+/**

• • xlnx_dsi_set_display_disable - Disable the DSI Tx IP core enable
• • register bit
• • @dsi: Xilinx DSI structure
• • This function takes the DSI strucure and disables the core enable bit
• • of core configuration register.
• */

+static void xlnx_dsi_set_display_disable(struct xlnx_dsi *dsi) +{

• u32 reg;
• reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
• reg &= ~XDSI_CCR_COREENB;

Ditto.

...

• xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
• dev_dbg(dsi->dev, "DSI Tx is disabled. reset regs to default values\n");

+}

+/**

• • xlnx_dsi_host_transfer - transfer command to panel
• • @host: mipi dsi host structure
• • @msg: mipi dsi msg with type, length and data
• • This function is valid only in command mode.
• • It checks the command fifo empty status and writes into
• • data or cmd register and waits for the completion status.
• • Return: number of bytes, on success and error number on failure

Nit. Extra spaces.

...

• */

+static ssize_t xlnx_dsi_host_transfer(struct mipi_dsi_host *host,

• 		      const struct mipi_dsi_msg *msg)
  

+{

• struct xlnx_dsi *dsi = host_to_dsi(host);
• u32 data0, data1, cmd0, status, val;
• const char *tx_buf = msg->tx_buf;
• if (!(xlnx_dsi_readl(dsi->iomem, XDSI_CCR) & (XDSI_CCR_COREENB |

• 				      XDSI_CCR_CMDMODE))) {
  

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

...

• dev_err(dsi->dev, "dsi command mode not enabled\n");
  

• return -EINVAL;
  

• }
• if (msg->type == MIPI_DSI_DCS_LONG_WRITE) {

• status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
  

• 			    ((val & XDSI_STR_LPKT_MASK) ==
  

• 			     XDSI_STR_LPKT_MASK), 1,
  

Please remove unneeded (). Same for rest below.

...

• 			    XDSI_CMD_TIMEOUT_VAL);
  

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

...

• if (status) {
  

• 	dev_err(dsi->dev, "long cmd fifo not empty!\n");
  

• 	return -ETIMEDOUT;
  

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

...

• }
  

• data0 = tx_buf[0] | (tx_buf[1] << 8) | (tx_buf[2] << 16) |
  

• 	(tx_buf[3] << 24);
  

• data1 = tx_buf[4] | (tx_buf[5] << 8);
  

• cmd0 = msg->type | (MIPI_DSI_DCS_READ << 8);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data0);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data1);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_CMD, cmd0);
  

• } else {

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

...

• data0 = tx_buf[0];
  

This line can be removed.

...

• if (msg->type == MIPI_DSI_DCS_SHORT_WRITE_PARAM)
  

• 	data0 = MIPI_DSI_DCS_SHORT_WRITE_PARAM |
  

• 		(tx_buf[0] << 8) | (tx_buf[1] << 16);
  

• else
  

• 	data0 = MIPI_DSI_DCS_SHORT_WRITE | (tx_buf[0] << 8);
  

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

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

...

• status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
  

• 			    ((val & XDSI_STR_RDY_SHPKT) ==
  

• 			     XDSI_STR_RDY_SHPKT), 1,
  

• 			    XDSI_CMD_TIMEOUT_VAL);
  

• if (status) {
  

• 	dev_err(dsi->dev, "short cmd fifo not empty\n");
  

• 	return -ETIMEDOUT;
  

• }
  

• xlnx_dsi_writel(dsi->iomem, XDSI_CMD, data0);
  

• }
• status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,

• 		    (!(val & XDSI_STR_CMD_EXE_PGS)), 1,
  

• 		    XDSI_CMD_TIMEOUT_VAL);
  

• if (status) {

• dev_err(dsi->dev, "cmd time out\n");
  

• return -ETIMEDOUT;
  

• }
• return msg->tx_len;

+}

+static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,

• 		struct mipi_dsi_device *device)
  

+{

• u32 panel_lanes;
• struct xlnx_dsi *dsi = host_to_dsi(host);
• panel_lanes = device->lanes;

I'd do this above,

u32 panel_lanes = device->lanes

...

• dsi->mode_flags = device->mode_flags;
• dsi->panel_node = device->dev.of_node;
• if (panel_lanes != dsi->lanes) {

• dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
  

• 	panel_lanes, dsi->lanes);
  

• return -EINVAL;
  

• }
• if (device->format != dsi->format) {

• dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n",
  

• 	device->format, dsi->format);
  

• return -EINVAL;
  

• }
• if (dsi->connector.dev)

• drm_helper_hpd_irq_event(dsi->connector.dev);
  

• return 0;

+}

+static int xlnx_dsi_host_detach(struct mipi_dsi_host *host,

• 		struct mipi_dsi_device *device)
  

+{

• struct xlnx_dsi *dsi = host_to_dsi(host);
• if (dsi->panel) {

• drm_panel_detach(dsi->panel);
  

• dsi->panel = NULL;
  

• }
• if (dsi->connector.dev)

• drm_helper_hpd_irq_event(dsi->connector.dev);
  

• return 0;

+}

+static const struct mipi_dsi_host_ops xlnx_dsi_ops = {

• .attach = xlnx_dsi_host_attach,
• .detach = xlnx_dsi_host_detach,
• .transfer = xlnx_dsi_host_transfer,

+};

+static int xlnx_dsi_connector_dpms(struct drm_connector *connector,

• 		   int mode)
  

+{

• struct xlnx_dsi *dsi = connector_to_dsi(connector);
• int ret;
• dev_dbg(dsi->dev, "connector dpms state: %d\n", mode);
• switch (mode) {
• case DRM_MODE_DPMS_ON:

• ret = drm_panel_prepare(dsi->panel);
  

• if (ret < 0) {
  

• 	dev_err(dsi->dev, "DRM panel not found\n");
  

• 	return ret;
  

• }
  

• ret = drm_panel_enable(dsi->panel);
  

• if (ret < 0) {
  

• 	drm_panel_unprepare(dsi->panel);
  

• 	dev_err(dsi->dev, "DRM panel not enabled\n");
  

• 	return ret;
  

• }
  

• break;
  

• default:

• drm_panel_disable(dsi->panel);
  

• drm_panel_unprepare(dsi->panel);
  

• break;
  

• }
• return drm_helper_connector_dpms(connector, mode);

+}

+static enum drm_connector_status +xlnx_dsi_detect(struct drm_connector *connector, bool force) +{

• struct xlnx_dsi *dsi = connector_to_dsi(connector);
• if (!dsi->panel) {

• dsi->panel = of_drm_find_panel(dsi->panel_node);
  

• if (dsi->panel) {
  

• 	drm_panel_attach(dsi->panel, &dsi->connector);
  

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

...

• 	if (dsi->cmdmode) {
  

• 		xlnx_dsi_writel(dsi->iomem, XDSI_CCR,
  

• 				XDSI_CCR_CMDMODE |
  

• 				XDSI_CCR_COREENB);
  

• 		drm_panel_prepare(dsi->panel);
  

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

...

• 		xlnx_dsi_writel(dsi->iomem, XDSI_CCR, 0);
  

• 	}
  

• }
  

• } else if (!dsi->panel_node) {

• xlnx_dsi_connector_dpms(connector, DRM_MODE_DPMS_OFF);
  

• drm_panel_detach(dsi->panel);
  

• dsi->panel = NULL;
  

• }
• if (dsi->panel)

• return connector_status_connected;
  

• return connector_status_disconnected;

+}

+static void xlnx_dsi_connector_destroy(struct drm_connector *connector) +{

• drm_connector_unregister(connector);
• drm_connector_cleanup(connector);
• connector->dev = NULL;

+}

+static const struct drm_connector_funcs xlnx_dsi_connector_funcs = {

• .detect = xlnx_dsi_detect,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = xlnx_dsi_connector_destroy,
• .reset = drm_atomic_helper_connector_reset,

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

...

• .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
• .dpms = xlnx_dsi_connector_dpms,

+};

+static int xlnx_dsi_get_modes(struct drm_connector *connector) +{

• struct xlnx_dsi *dsi = connector_to_dsi(connector);
• if (dsi->panel)

• return drm_panel_get_modes(dsi->panel, connector);
  

• return 0;

+}

+static struct drm_encoder * +xlnx_dsi_best_encoder(struct drm_connector *connector) +{

• return &(connector_to_dsi(connector)->encoder);

+}

+static const struct drm_connector_helper_funcs +xlnx_dsi_connector_helper_funcs = {

• .get_modes = xlnx_dsi_get_modes,
• .best_encoder = xlnx_dsi_best_encoder,

+};

+static int xlnx_dsi_create_connector(struct drm_encoder *encoder) +{

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• struct drm_connector *connector = &dsi->connector;
• struct drm_device *drm = encoder->dev;
• int ret;
• connector->polled = DRM_CONNECTOR_POLL_HPD;
• ret = drm_connector_init(encoder->dev, connector,

• 		 &xlnx_dsi_connector_funcs,
  

• 		 DRM_MODE_CONNECTOR_DSI);
  

• if (ret) {

• dev_err(dsi->dev, "Failed to initialize connector with drm\n");
  

• return ret;
  

• }
• drm_connector_helper_add(connector, &xlnx_dsi_connector_helper_funcs);
• drm_connector_attach_encoder(connector, encoder);
• if (!drm->registered)

• return 0;
  

• drm_connector_register(connector);
• return 0;

+}

+/**

• • xlnx_dsi_atomic_mode_set - derive the DSI timing parameters
• • @encoder: pointer to Xilinx DRM encoder
• • @crtc_state: Pointer to drm core crtc state
• • @connector_state: DSI connector drm state
• • This function derives the DSI IP timing parameters from the timing
• • values given in the attached panel driver.
• */

+static void +xlnx_dsi_atomic_mode_set(struct drm_encoder *encoder,

• 	 struct drm_crtc_state *crtc_state,
  

• 		 struct drm_connector_state *connector_state)
  

+{

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• struct videomode *vm = &dsi->vm;
• struct drm_display_mode *m = &crtc_state->adjusted_mode;
• vm->hactive = m->hdisplay;
• vm->vactive = m->vdisplay;
• vm->vfront_porch = m->vsync_start - m->vdisplay;
• vm->vback_porch = m->vtotal - m->vsync_end;
• vm->vsync_len = m->vsync_end - m->vsync_start;
• vm->hfront_porch = m->hsync_start - m->hdisplay;
• vm->hback_porch = m->htotal - m->hsync_end;
• vm->hsync_len = m->hsync_end - m->hsync_start;
• xlnx_dsi_set_display_mode(dsi);

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

...

+}

+static void xlnx_dsi_disable(struct drm_encoder *encoder) +{

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• xlnx_dsi_set_display_disable(dsi);

xlnx_dsi_set_display_disable(encoder_to_dsi(encoder));

Maybe even this wrapper can be removed?

...

+}

+static void xlnx_dsi_enable(struct drm_encoder *encoder) +{

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• xlnx_dsi_set_display_enable(dsi);

Ditto.

...

+}

+static const struct drm_encoder_helper_funcs +xlnx_dsi_encoder_helper_funcs = {

• .atomic_mode_set = xlnx_dsi_atomic_mode_set,
• .enable = xlnx_dsi_enable,
• .disable = xlnx_dsi_disable,

+};

+static const struct drm_encoder_funcs xlnx_dsi_encoder_funcs = {

• .destroy = drm_encoder_cleanup,

+};

+static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi) +{

• struct device *dev = dsi->dev;
• struct device_node *node = dev->of_node;
• int ret;
• u32 datatype;
• static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225, 200};
• dsi->dphy_clk_200M = devm_clk_get(dev, "dphy_clk_200M");
• if (IS_ERR(dsi->dphy_clk_200M)) {

• ret = PTR_ERR(dsi->dphy_clk_200M);
  

• dev_err(dev, "failed to get dphy_clk_200M %d\n", ret);
  

• return ret;
  

• }
• dsi->video_aclk = devm_clk_get(dev, "s_axis_aclk");
• if (IS_ERR(dsi->video_aclk)) {

• ret = PTR_ERR(dsi->video_aclk);
  

• dev_err(dev, "failed to get video_clk %d\n", ret);
  

• return ret;
  

• }
• ret = of_property_read_u32(node, "xlnx,dsi-num-lanes", &dsi->lanes);
• if (ret < 0) {

• dev_err(dsi->dev, "missing xlnx,dsi-num-lanes property\n");
  

• return ret;
  

• }
• if (dsi->lanes > 4 || dsi->lanes < 1) {

• dev_err(dsi->dev, "%d lanes : invalid lanes\n", dsi->lanes);
  

• return -EINVAL;
  

• }

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

...

• ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);
• if (ret < 0) {

• dev_err(dsi->dev, "missing xlnx,dsi-data-type property\n");
  

• return ret;
  

• }
• dsi->format = datatype;
• if (datatype > MIPI_DSI_FMT_RGB565) {

• dev_err(dsi->dev, "Invalid xlnx,dsi-data-type string\n");
  

• return -EINVAL;
  

• }
• /*

• * multiplication factor used for HACT, based on data type.
  

• *
  

• * e.g. for RGB666_L datatype and 1920x1080 resolution,
  

• * the Hact (WC) would be as follows -
  

• * 1920 pixels * 18 bits per pixel / 8 bits per byte
  

• * = 1920 pixels * 2.25 bytes per pixel = 4320 bytes.
  

• *
  

• * Data Type - Multiplication factor
  

• * RGB888    - 3
  

• * RGB666_L  - 2.25
  

+- * RGB666_P - 2.25

• * RGB565    - 2
  

• *
  

• * Since the multiplication factor maybe a floating number,
  

• * a 100x multiplication factor is used.
  

• */
  

• dsi->mul_factor = xdsi_mul_fact[datatype];
• dsi->cmdmode = of_property_read_bool(node, "xlnx,dsi-cmd-mode");
• dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
• dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
• dev_dbg(dsi->dev, "DSI controller cmd mode = %d\n", dsi->cmdmode);
• return 0;

+}

+static int xlnx_dsi_bind(struct device *dev, struct device *master,

• 	 void *data)
  

+{

• struct xlnx_dsi *dsi = dev_get_drvdata(dev);
• struct drm_encoder *encoder = &dsi->encoder;
• struct drm_device *drm_dev = data;
• int ret;
• drm_encoder_init(drm_dev, encoder, &xlnx_dsi_encoder_funcs,

• 	 DRM_MODE_ENCODER_DSI, NULL);
  

• drm_encoder_helper_add(encoder, &xlnx_dsi_encoder_helper_funcs);
• encoder->possible_crtcs = 1;
• ret = xlnx_dsi_create_connector(encoder);
• if (ret) {

• dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret);
  

• drm_encoder_cleanup(encoder);
  

• return ret;
  

• }
• ret = mipi_dsi_host_register(&dsi->dsi_host);
• if (ret < 0) {

Just nit. if (ret) would do.

...

• dev_err(dsi->dev, "failed to register DSI host: %d\n", ret);
  

• xlnx_dsi_connector_destroy(&dsi->connector);
  

• drm_encoder_cleanup(encoder);
  

• return ret;
  

• }
• return 0;

+}

+static void xlnx_dsi_unbind(struct device *dev, struct device *master,

• 	    void *data)
  

+{

• struct xlnx_dsi *dsi = dev_get_drvdata(dev);
• xlnx_dsi_disable(&dsi->encoder);
• mipi_dsi_host_unregister(&dsi->dsi_host);

+}

+static const struct component_ops xlnx_dsi_component_ops = {

• .bind = xlnx_dsi_bind,
• .unbind = xlnx_dsi_unbind,

+};

+static int xlnx_dsi_probe(struct platform_device *pdev) +{

• struct device *dev = &pdev->dev;
• struct resource *res;
• struct xlnx_dsi *dsi;
• int ret;
• unsigned long rate;
• dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
• if (!dsi)

• return -ENOMEM;
  

• dsi->dsi_host.ops = &xlnx_dsi_ops;
• dsi->dsi_host.dev = dev;
• dsi->dev = dev;
• ret = xlnx_dsi_parse_dt(dsi);
• if (ret)

• return ret;
  

• res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
• dsi->iomem = devm_ioremap_resource(dev, res);
• if (IS_ERR(dsi->iomem)) {

• dev_err(dev, "failed to remap io region\n");
  

• return PTR_ERR(dsi->iomem);
  

• }
• platform_set_drvdata(pdev, dsi);
• ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ);
• if (ret) {

• dev_err(dev, "failed to set dphy clk rate %d\n", ret);
  

• return ret;
  

• }
• ret = clk_prepare_enable(dsi->dphy_clk_200M);
• if (ret) {

• dev_err(dev, "failed to enable dphy clk %d\n", ret);
  

• return ret;
  

• }
• rate = clk_get_rate(dsi->dphy_clk_200M);
• if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) {

A brief comment for this line wold be helpful.

...

• dev_err(dev, "Error DPHY clock = %lu\n", rate);
  

• ret = -EINVAL;
  

• goto err_disable_dphy_clk;
  

• }
• ret = clk_prepare_enable(dsi->video_aclk);
• if (ret) {

• dev_err(dev, "failed to enable video clk %d\n", ret);
  

• goto err_disable_dphy_clk;
  

• }
• ret = component_add(dev, &xlnx_dsi_component_ops);

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

...

• if (ret < 0)

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

...

• goto err_disable_video_clk;
  

• return ret;

return 0;? but up to you.

...

+err_disable_video_clk:

• clk_disable_unprepare(dsi->video_aclk);

+err_disable_dphy_clk:

• clk_disable_unprepare(dsi->dphy_clk_200M);
• return ret;

+}

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

...

+static int xlnx_dsi_remove(struct platform_device *pdev) +{

• struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
• component_del(&pdev->dev, &xlnx_dsi_component_ops);
• clk_disable_unprepare(dsi->video_aclk);
• clk_disable_unprepare(dsi->dphy_clk_200M);
• return 0;

+}

+static const struct of_device_id xlnx_dsi_of_match[] = {

• { .compatible = "xlnx-dsi", },

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

...

• { /* end of table */ },

+}; +MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match);

+static struct platform_driver dsi_driver = {

• .probe = xlnx_dsi_probe,
• .remove = xlnx_dsi_remove,
• .driver = {

• .name = "xlnx-dsi",
  

• .of_match_table = xlnx_dsi_of_match,
  

• },

+};

+module_platform_driver(dsi_driver);

+MODULE_AUTHOR("Xilinx, Inc."); +MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver"); +MODULE_LICENSE("GPL v2");

Hi Hyun,

On Wed, May 27, 2020 at 10:54:35AM -0700, Hyun Kwon wrote:

On Sat, 2020-05-23 at 20:08:13 -0700, Laurent Pinchart wrote:

...

On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:

...

On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:

...

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

It supports upto 4 lanes, optional register interface for the DPHY,

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

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

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

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

...

...

...

multiple RGB color formats, command mode and video mode. This is a MIPI-DSI host driver and provides DSI bus for panels. This driver also helps to communicate with its panel using panel framework.

Signed-off-by: Venkateshwar Rao Gannavarapu venkateshwar.rao.gannavarapu@xilinx.com

drivers/gpu/drm/xlnx/Kconfig | 11 + drivers/gpu/drm/xlnx/Makefile | 2 + drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++

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

...

...

3 files changed, 768 insertions(+) create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig index aa6cd88..73873cf 100644 --- a/drivers/gpu/drm/xlnx/Kconfig +++ b/drivers/gpu/drm/xlnx/Kconfig @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose this option if you have a Xilinx ZynqMP SoC with DisplayPort subsystem.

+config DRM_XLNX_DSI

•    tristate "Xilinx DRM DSI Subsystem Driver"
  

•    select DRM_MIPI_DSI
  

•    select DRM_PANEL
  

•    select DRM_PANEL_SIMPLE
  

•    help
  

•  This enables support for Xilinx MIPI-DSI.
  

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

...

•  This is a DRM/KMS driver for Xilinx programmable DSI controller.
  

•  Choose this option if you have a Xilinx MIPI DSI-TX controller
  

•  subsytem.
  

These seem incorrectly indented.

...

diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile index 2b844c6..b7ee6ef 100644 --- a/drivers/gpu/drm/xlnx/Makefile +++ b/drivers/gpu/drm/xlnx/Makefile @@ -1,2 +1,4 @@ zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o

+obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c new file mode 100644 index 0000000..b8cae59 --- /dev/null +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c @@ -0,0 +1,755 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Xilinx FPGA MIPI DSI Tx Controller driver
• • Copyright (C) 2017 - 2019 Xilinx, Inc.
• • Authors:
• • • Saurabh Sengar saurabhs@xilinx.com
• • • Venkateshwar Rao Gannavarapu venkateshwar.rao.gannavarapu@xilinx.com
• */

+#include <drm/drm_atomic_helper.h> +#include <drm/drm_connector.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_device.h> +#include <drm/drm_encoder.h> +#include <drm/drm_fourcc.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h> +#include <drm/drm_probe_helper.h>

+#include <linux/clk.h> +#include <linux/component.h> +#include <linux/device.h> +#include <linux/iopoll.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/phy/phy.h>

+#include <video/mipi_display.h> +#include <video/videomode.h>

+/* DSI Tx IP registers */ +#define XDSI_CCR 0x00 +#define XDSI_CCR_COREENB BIT(0) +#define XDSI_CCR_SOFTRST BIT(1) +#define XDSI_CCR_CRREADY BIT(2) +#define XDSI_CCR_CMDMODE BIT(3) +#define XDSI_CCR_DFIFORST BIT(4) +#define XDSI_CCR_CMDFIFORST BIT(5) +#define XDSI_PCR 0x04

[snip]

...

...

...

• }
• ret = clk_prepare_enable(dsi->video_aclk);
• if (ret) {

• dev_err(dev, "failed to enable video clk %d\n", ret);
  

• goto err_disable_dphy_clk;
  

• }
• ret = component_add(dev, &xlnx_dsi_component_ops);

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

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

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

Hi Laurent & Hyun,

Thanks for the review.

-----Original Message----- From: Laurent Pinchart laurent.pinchart@ideasonboard.com Sent: Sunday, May 24, 2020 8:38 AM To: Venkateshwar Rao Gannavarapu VGANNAVA@xilinx.com Cc: Hyun Kwon hyunk@xilinx.com; Hyun Kwon hyunk@xilinx.com; dri- devel@lists.freedesktop.org; airlied@linux.ie; daniel@ffwll.ch; linux- kernel@vger.kernel.org; Sandip Kothari sandipk@xilinx.com; Venkateshwar Rao Gannavarapu VGANNAVA@xilinx.com Subject: Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

Hi GVRao,

Thank you for the patch.

On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:

...

On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu

wrote:

...

...

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

It supports upto 4 lanes, optional register interface for the DPHY,

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

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

IP has the provision to read DPHY register for debug purpose only. No programming of DPHY is required in subsystem.

...

...

multiple RGB color formats, command mode and video mode. This is a MIPI-DSI host driver and provides DSI bus for panels. This driver also helps to communicate with its panel using panel framework.

Signed-off-by: Venkateshwar Rao Gannavarapu

venkateshwar.rao.gannavarapu@xilinx.com

drivers/gpu/drm/xlnx/Kconfig | 11 + drivers/gpu/drm/xlnx/Makefile | 2 + drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++

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

...

...

3 files changed, 768 insertions(+) create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c

diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig index aa6cd88..73873cf 100644 --- a/drivers/gpu/drm/xlnx/Kconfig +++ b/drivers/gpu/drm/xlnx/Kconfig @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose this option if you have a Xilinx ZynqMP SoC with DisplayPort subsystem.

+config DRM_XLNX_DSI

•    tristate "Xilinx DRM DSI Subsystem Driver"
  

•    select DRM_MIPI_DSI
  

•    select DRM_PANEL
  

•    select DRM_PANEL_SIMPLE
  

•    help
  

•  This enables support for Xilinx MIPI-DSI.
  

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

OK.

...

...

•  This is a DRM/KMS driver for Xilinx programmable DSI controller.
  

•  Choose this option if you have a Xilinx MIPI DSI-TX controller
  

•  subsytem.
  

These seem incorrectly indented.

Will check the indentation.

...

...

diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile index 2b844c6..b7ee6ef 100644 --- a/drivers/gpu/drm/xlnx/Makefile +++ b/drivers/gpu/drm/xlnx/Makefile @@ -1,2 +1,4 @@ zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o

+obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c new file mode 100644 index 0000000..b8cae59 --- /dev/null +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c @@ -0,0 +1,755 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Xilinx FPGA MIPI DSI Tx Controller driver
• • Copyright (C) 2017 - 2019 Xilinx, Inc.
• • Authors:
• • • Saurabh Sengar saurabhs@xilinx.com
• • • Venkateshwar Rao Gannavarapu

+venkateshwar.rao.gannavarapu@xilinx.com

• */

+#include <drm/drm_atomic_helper.h> +#include <drm/drm_connector.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_device.h> +#include <drm/drm_encoder.h> +#include <drm/drm_fourcc.h> +#include <drm/drm_gem_cma_helper.h> #include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h> #include <drm/drm_probe_helper.h>

+#include <linux/clk.h> +#include <linux/component.h> +#include <linux/device.h> +#include <linux/iopoll.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/phy/phy.h>

+#include <video/mipi_display.h> +#include <video/videomode.h>

+/* DSI Tx IP registers */ +#define XDSI_CCR 0x00 +#define XDSI_CCR_COREENB BIT(0) +#define XDSI_CCR_SOFTRST BIT(1) +#define XDSI_CCR_CRREADY BIT(2) +#define XDSI_CCR_CMDMODE BIT(3) +#define XDSI_CCR_DFIFORST BIT(4) +#define XDSI_CCR_CMDFIFORST BIT(5) +#define XDSI_PCR 0x04 +#define XDSI_PCR_VIDEOMODE(x) (((x) & 0x3) << 3) +#define XDSI_PCR_VIDEOMODE_MASK (0x3 << 3) +#define XDSI_PCR_VIDEOMODE_SHIFT 3 +#define XDSI_PCR_BLLPTYPE(x) ((x) << 5) +#define XDSI_PCR_BLLPMODE(x) ((x) << 6) +#define XDSI_PCR_EOTPENABLE(x) ((x) << 13) +#define XDSI_GIER 0x20 +#define XDSI_ISR 0x24 +#define XDSI_IER 0x28 +#define XDSI_STR 0x2C +#define XDSI_STR_RDY_SHPKT BIT(6) +#define XDSI_STR_RDY_LNGPKT BIT(7) +#define XDSI_STR_DFIFO_FULL BIT(8) +#define XDSI_STR_DFIFO_EMPTY BIT(9) +#define XDSI_STR_WAITFR_DATA BIT(10) +#define XDSI_STR_CMD_EXE_PGS BIT(11) +#define XDSI_STR_CCMD_PROC BIT(12) +#define XDSI_STR_LPKT_MASK (0x5 << 7) +#define XDSI_CMD 0x30 +#define XDSI_CMD_QUEUE_PACKET(x) ((x) & GENMASK(23, 0)) +#define XDSI_DFR 0x34 +#define XDSI_TIME1 0x50 +#define XDSI_TIME1_BLLP_BURST(x) ((x) & GENMASK(15, 0)) +#define XDSI_TIME1_HSA(x) (((x) & GENMASK(15, 0)) << 16) +#define XDSI_TIME2 0x54 +#define XDSI_TIME2_VACT(x) ((x) & GENMASK(15, 0)) +#define XDSI_TIME2_HACT(x) (((x) & GENMASK(15, 0)) << 16) +#define XDSI_HACT_MULTIPLIER GENMASK(1, 0) +#define XDSI_TIME3 0x58 +#define XDSI_TIME3_HFP(x) ((x) & GENMASK(15, 0)) +#define XDSI_TIME3_HBP(x) (((x) & GENMASK(15, 0)) << 16) +#define XDSI_TIME4 0x5c +#define XDSI_TIME4_VFP(x) ((x) & GENMASK(7, 0)) +#define XDSI_TIME4_VBP(x) (((x) & GENMASK(7, 0)) << 8) +#define XDSI_TIME4_VSA(x) (((x) & GENMASK(7, 0)) << 16) +#define XDSI_LTIME 0x60 +#define XDSI_BLLP_TIME 0x64 +/*

• • XDSI_NUM_DATA_T represents number of data types in the
• • enum mipi_dsi_pixel_format in the MIPI DSI part of DRM framework.
• */

+#define XDSI_NUM_DATA_T 4 +#define XDSI_VIDEO_MODE_SYNC_PULSE 0x0 +#define XDSI_VIDEO_MODE_SYNC_EVENT 0x1 +#define XDSI_VIDEO_MODE_BURST 0x2

Please remove these unused 3 definitions.

OK.

...

...

+#define XDSI_DPHY_CLK_MIN 197000000000UL +#define XDSI_DPHY_CLK_MAX 203000000000UL +#define XDSI_DPHY_CLK_REQ 200000000000UL

+/* command timeout in usec */ +#define XDSI_CMD_TIMEOUT_VAL (3000)

+/**

• • struct xlnx_dsi - Xilinx DSI-TX core
• • @encoder: DRM encoder structure
• • @dsi_host: DSI host device
• • @connector: DRM connector structure
• • @panel_node: MIPI DSI device panel node
• • @panel: DRM panel structure
• • @dev: device structure
• • @iomem: Base address of DSI subsystem
• • @lanes: number of active data lanes supported by DSI controller
• • @cmdmode: command mode support
• • @mode_flags: DSI operation mode related flags
• • @format: pixel format for video mode of DSI controller
• • @vm: videomode data structure
• • @mul_factor: multiplication factor for HACT timing parameter
• • @video_aclk: Video clock
• • @dphy_clk_200M: 200MHz DPHY clock and AXI Lite clock */ struct

+xlnx_dsi {

• struct drm_encoder encoder;
• struct mipi_dsi_host dsi_host;
• struct drm_connector connector;
• struct device_node *panel_node;
• struct drm_panel *panel;
• struct device *dev;
• void __iomem *iomem;
• u32 lanes;
• bool cmdmode;
• u32 mode_flags;
• enum mipi_dsi_pixel_format format;
• struct videomode vm;
• u32 mul_factor;
• struct clk *video_aclk;
• struct clk *dphy_clk_200M;

+};

+#define host_to_dsi(host) container_of(host, struct xlnx_dsi, +dsi_host) #define connector_to_dsi(c) container_of(c, struct +xlnx_dsi, connector) #define encoder_to_dsi(e) container_of(e, +struct xlnx_dsi, encoder)

+static inline void xlnx_dsi_writel(void __iomem *base, int offset, +u32 val) {

• writel(val, base + offset);

+}

+static inline u32 xlnx_dsi_readl(void __iomem *base, int offset) {

• return readl(base + offset);

+}

+/**

• • xlnx_dsi_set_display_mode - Configure DSI timing registers
• • @dsi: xilinx DSI structure
• • This function writes the timing parameters of DSI IP which are
• • retrieved from panel timing values.
• */

+static void xlnx_dsi_set_display_mode(struct xlnx_dsi *dsi)

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

drm_display_mode.

OK.

...

...

+{

• struct videomode *vm = &dsi->vm;
• u32 reg, video_mode;
• reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR);
• video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >>

•       XDSI_PCR_VIDEOMODE_SHIFT;
  

• /* configure the HSA value only if non_burst_sync_pluse video mode */
• if (!video_mode &&

•    (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
  

• reg = XDSI_TIME1_HSA(vm->hsync_len);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg);
  

• }
• reg = XDSI_TIME4_VFP(vm->vfront_porch) |

•      XDSI_TIME4_VBP(vm->vback_porch) |
  

•      XDSI_TIME4_VSA(vm->vsync_len);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg);
• reg = XDSI_TIME3_HFP(vm->hfront_porch) |

•      XDSI_TIME3_HBP(vm->hback_porch);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg);
• dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n",

• (dsi->mul_factor) / 100);
  

Remove unneeded ().

OK.

...

...

• /*

• * The HACT parameter received from panel timing values should be
  

• * divisible by 4. The reason for this is, the word count given as
  

• * input to DSI controller is HACT * mul_factor. The mul_factor is
  

• * 3, 2.25, 2.25, 2 respectively for RGB888, RGB666_L, RGB666_P and
  

• * RGB565.
  

• * e.g. for RGB666_L color format and 1080p, the word count is
  

• * 1920*2.25 = 4320 which is divisible by 4 and it is a valid input
  

• * to DSI controller. Based on this 2.25 mul factor, we come up with
  

• * the division factor of (XDSI_HACT_MULTIPLIER) as 4 for checking
  

• */
  

• if ((vm->hactive & XDSI_HACT_MULTIPLIER) != 0)

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

...

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

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

OK.

...

...

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

Ditto. No ().

...

•      XDSI_TIME2_VACT(vm->vactive);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_TIME2, reg);
• dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive);

+}

+/**

• • xlnx_dsi_set_display_enable - Enables the DSI Tx IP core enable
• • register bit
• • @dsi: Xilinx DSI structure
• • This function takes the DSI strucure and enables the core enable

+bit

• • of core configuration register.
• */

+static void xlnx_dsi_set_display_enable(struct xlnx_dsi *dsi) {

• u32 reg;
• reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
• reg |= XDSI_CCR_COREENB;

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

OK.

...

...

• xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
• dev_dbg(dsi->dev, "MIPI DSI Tx controller is enabled.\n"); }

+/**

• • xlnx_dsi_set_display_disable - Disable the DSI Tx IP core enable
• • register bit
• • @dsi: Xilinx DSI structure
• • This function takes the DSI strucure and disables the core

+enable bit

• • of core configuration register.
• */

+static void xlnx_dsi_set_display_disable(struct xlnx_dsi *dsi) {

• u32 reg;
• reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR);
• reg &= ~XDSI_CCR_COREENB;

Ditto.

...

• xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg);
• dev_dbg(dsi->dev, "DSI Tx is disabled. reset regs to default

+values\n"); }

+/**

• • xlnx_dsi_host_transfer - transfer command to panel
• • @host: mipi dsi host structure
• • @msg: mipi dsi msg with type, length and data
• • This function is valid only in command mode.
• • It checks the command fifo empty status and writes into
• • data or cmd register and waits for the completion status.
• • Return: number of bytes, on success and error number on failure

Nit. Extra spaces.

...

• */

+static ssize_t xlnx_dsi_host_transfer(struct mipi_dsi_host *host,

• 		      const struct mipi_dsi_msg *msg) {
  

• struct xlnx_dsi *dsi = host_to_dsi(host);
• u32 data0, data1, cmd0, status, val;
• const char *tx_buf = msg->tx_buf;
• if (!(xlnx_dsi_readl(dsi->iomem, XDSI_CCR) & (XDSI_CCR_COREENB |

• 				      XDSI_CCR_CMDMODE))) {
  

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

OK.

...

...

• dev_err(dsi->dev, "dsi command mode not enabled\n");
  

• return -EINVAL;
  

• }
• if (msg->type == MIPI_DSI_DCS_LONG_WRITE) {

• status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
  

• 			    ((val & XDSI_STR_LPKT_MASK) ==
  

• 			     XDSI_STR_LPKT_MASK), 1,
  

Please remove unneeded (). Same for rest below.

OK.

...

...

• 			    XDSI_CMD_TIMEOUT_VAL);
  

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

I will add the description or comment

...

...

• if (status) {
  

• 	dev_err(dsi->dev, "long cmd fifo not empty!\n");
  

• 	return -ETIMEDOUT;
  

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

OK.

...

...

• }
  

• data0 = tx_buf[0] | (tx_buf[1] << 8) | (tx_buf[2] << 16) |
  

• 	(tx_buf[3] << 24);
  

• data1 = tx_buf[4] | (tx_buf[5] << 8);
  

• cmd0 = msg->type | (MIPI_DSI_DCS_READ << 8);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data0);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data1);
  

• xlnx_dsi_writel(dsi->iomem, XDSI_CMD, cmd0);
  

• } else {

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

param)?

I can add check for short packet.

...

...

• data0 = tx_buf[0];
  

This line can be removed.

OK.

...

...

• if (msg->type == MIPI_DSI_DCS_SHORT_WRITE_PARAM)
  

• 	data0 = MIPI_DSI_DCS_SHORT_WRITE_PARAM |
  

• 		(tx_buf[0] << 8) | (tx_buf[1] << 16);
  

• else
  

• 	data0 = MIPI_DSI_DCS_SHORT_WRITE | (tx_buf[0] <<
  

8);

...

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

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

...

• status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,
  

• 			    ((val & XDSI_STR_RDY_SHPKT) ==
  

• 			     XDSI_STR_RDY_SHPKT), 1,
  

• 			    XDSI_CMD_TIMEOUT_VAL);
  

• if (status) {
  

• 	dev_err(dsi->dev, "short cmd fifo not empty\n");
  

• 	return -ETIMEDOUT;
  

• }
  

• xlnx_dsi_writel(dsi->iomem, XDSI_CMD, data0);
  

• }
• status = readl_poll_timeout(dsi->iomem + XDSI_STR, val,

• 		    (!(val & XDSI_STR_CMD_EXE_PGS)), 1,
  

• 		    XDSI_CMD_TIMEOUT_VAL);
  

• if (status) {

• dev_err(dsi->dev, "cmd time out\n");
  

• return -ETIMEDOUT;
  

• }
• return msg->tx_len;

+}

+static int xlnx_dsi_host_attach(struct mipi_dsi_host *host,

• 		struct mipi_dsi_device *device) {
  

• u32 panel_lanes;
• struct xlnx_dsi *dsi = host_to_dsi(host);
• panel_lanes = device->lanes;

I'd do this above,

u32 panel_lanes = device->lanes

...

• dsi->mode_flags = device->mode_flags;
• dsi->panel_node = device->dev.of_node;
• if (panel_lanes != dsi->lanes) {

• dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n",
  

• 	panel_lanes, dsi->lanes);
  

• return -EINVAL;
  

• }
• if (device->format != dsi->format) {

• dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI =
  

%d\n",

...

...

• 	device->format, dsi->format);
  

• return -EINVAL;
  

• }
• if (dsi->connector.dev)

• drm_helper_hpd_irq_event(dsi->connector.dev);
  

• return 0;

+}

+static int xlnx_dsi_host_detach(struct mipi_dsi_host *host,

• 		struct mipi_dsi_device *device) {
  

• struct xlnx_dsi *dsi = host_to_dsi(host);
• if (dsi->panel) {

• drm_panel_detach(dsi->panel);
  

• dsi->panel = NULL;
  

• }
• if (dsi->connector.dev)

• drm_helper_hpd_irq_event(dsi->connector.dev);
  

• return 0;

+}

+static const struct mipi_dsi_host_ops xlnx_dsi_ops = {

• .attach = xlnx_dsi_host_attach,
• .detach = xlnx_dsi_host_detach,
• .transfer = xlnx_dsi_host_transfer, };

+static int xlnx_dsi_connector_dpms(struct drm_connector *connector,

• 		   int mode)
  

+{

• struct xlnx_dsi *dsi = connector_to_dsi(connector);
• int ret;
• dev_dbg(dsi->dev, "connector dpms state: %d\n", mode);
• switch (mode) {
• case DRM_MODE_DPMS_ON:

• ret = drm_panel_prepare(dsi->panel);
  

• if (ret < 0) {
  

• 	dev_err(dsi->dev, "DRM panel not found\n");
  

• 	return ret;
  

• }
  

• ret = drm_panel_enable(dsi->panel);
  

• if (ret < 0) {
  

• 	drm_panel_unprepare(dsi->panel);
  

• 	dev_err(dsi->dev, "DRM panel not enabled\n");
  

• 	return ret;
  

• }
  

• break;
  

• default:

• drm_panel_disable(dsi->panel);
  

• drm_panel_unprepare(dsi->panel);
  

• break;
  

• }
• return drm_helper_connector_dpms(connector, mode); }

+static enum drm_connector_status +xlnx_dsi_detect(struct drm_connector *connector, bool force) {

• struct xlnx_dsi *dsi = connector_to_dsi(connector);
• if (!dsi->panel) {

• dsi->panel = of_drm_find_panel(dsi->panel_node);
  

• if (dsi->panel) {
  

• 	drm_panel_attach(dsi->panel, &dsi->connector);
  

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

I will check the possibility.

...

...

• 	if (dsi->cmdmode) {
  

• 		xlnx_dsi_writel(dsi->iomem, XDSI_CCR,
  

• 				XDSI_CCR_CMDMODE |
  

• 				XDSI_CCR_COREENB);
  

• 		drm_panel_prepare(dsi->panel);
  

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

There is a GUI option in IP design to select command mode support. I will check and ensure panel prepare won't happen twice.

...

...

• 		xlnx_dsi_writel(dsi->iomem, XDSI_CCR, 0);
  

• 	}
  

• }
  

• } else if (!dsi->panel_node) {

• xlnx_dsi_connector_dpms(connector, DRM_MODE_DPMS_OFF);
  

• drm_panel_detach(dsi->panel);
  

• dsi->panel = NULL;
  

• }
• if (dsi->panel)

• return connector_status_connected;
  

• return connector_status_disconnected; }

+static void xlnx_dsi_connector_destroy(struct drm_connector +*connector) {

• drm_connector_unregister(connector);
• drm_connector_cleanup(connector);
• connector->dev = NULL;

+}

+static const struct drm_connector_funcs xlnx_dsi_connector_funcs = {

• .detect = xlnx_dsi_detect,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = xlnx_dsi_connector_destroy,
• .reset = drm_atomic_helper_connector_reset,

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

...

• .atomic_duplicate_state =

drm_atomic_helper_connector_duplicate_state,

...

...

• .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
• .dpms = xlnx_dsi_connector_dpms,

+};

+static int xlnx_dsi_get_modes(struct drm_connector *connector) {

• struct xlnx_dsi *dsi = connector_to_dsi(connector);
• if (dsi->panel)

• return drm_panel_get_modes(dsi->panel, connector);
  

• return 0;

+}

+static struct drm_encoder * +xlnx_dsi_best_encoder(struct drm_connector *connector) {

• return &(connector_to_dsi(connector)->encoder);

+}

+static const struct drm_connector_helper_funcs +xlnx_dsi_connector_helper_funcs = {

• .get_modes = xlnx_dsi_get_modes,
• .best_encoder = xlnx_dsi_best_encoder, };

+static int xlnx_dsi_create_connector(struct drm_encoder *encoder) {

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• struct drm_connector *connector = &dsi->connector;
• struct drm_device *drm = encoder->dev;
• int ret;
• connector->polled = DRM_CONNECTOR_POLL_HPD;
• ret = drm_connector_init(encoder->dev, connector,

• 		 &xlnx_dsi_connector_funcs,
  

• 		 DRM_MODE_CONNECTOR_DSI);
  

• if (ret) {

• dev_err(dsi->dev, "Failed to initialize connector with drm\n");
  

• return ret;
  

• }
• drm_connector_helper_add(connector,

&xlnx_dsi_connector_helper_funcs);

...

...

• drm_connector_attach_encoder(connector, encoder);
• if (!drm->registered)

• return 0;
  

• drm_connector_register(connector);
• return 0;

+}

+/**

• • xlnx_dsi_atomic_mode_set - derive the DSI timing parameters
• • @encoder: pointer to Xilinx DRM encoder
• • @crtc_state: Pointer to drm core crtc state
• • @connector_state: DSI connector drm state
• • This function derives the DSI IP timing parameters from the

+timing

• • values given in the attached panel driver.
• */

+static void +xlnx_dsi_atomic_mode_set(struct drm_encoder *encoder,

• 	 struct drm_crtc_state *crtc_state,
  

• 		 struct drm_connector_state *connector_state)
  

{

...

...

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• struct videomode *vm = &dsi->vm;
• struct drm_display_mode *m = &crtc_state->adjusted_mode;
• vm->hactive = m->hdisplay;
• vm->vactive = m->vdisplay;
• vm->vfront_porch = m->vsync_start - m->vdisplay;
• vm->vback_porch = m->vtotal - m->vsync_end;
• vm->vsync_len = m->vsync_end - m->vsync_start;
• vm->hfront_porch = m->hsync_start - m->hdisplay;
• vm->hback_porch = m->htotal - m->hsync_end;
• vm->hsync_len = m->hsync_end - m->hsync_start;
• xlnx_dsi_set_display_mode(dsi);

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

OK.

...

...

+}

+static void xlnx_dsi_disable(struct drm_encoder *encoder) {

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• xlnx_dsi_set_display_disable(dsi);

xlnx_dsi_set_display_disable(encoder_to_dsi(encoder));

Maybe even this wrapper can be removed?

OK.

...

...

+}

+static void xlnx_dsi_enable(struct drm_encoder *encoder) {

• struct xlnx_dsi *dsi = encoder_to_dsi(encoder);
• xlnx_dsi_set_display_enable(dsi);

Ditto.

...

+}

+static const struct drm_encoder_helper_funcs +xlnx_dsi_encoder_helper_funcs = {

• .atomic_mode_set = xlnx_dsi_atomic_mode_set,
• .enable = xlnx_dsi_enable,
• .disable = xlnx_dsi_disable,

+};

+static const struct drm_encoder_funcs xlnx_dsi_encoder_funcs = {

• .destroy = drm_encoder_cleanup,

+};

+static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi) {

• struct device *dev = dsi->dev;
• struct device_node *node = dev->of_node;
• int ret;
• u32 datatype;
• static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225,

+200};

• dsi->dphy_clk_200M = devm_clk_get(dev, "dphy_clk_200M");
• if (IS_ERR(dsi->dphy_clk_200M)) {

• ret = PTR_ERR(dsi->dphy_clk_200M);
  

• dev_err(dev, "failed to get dphy_clk_200M %d\n", ret);
  

• return ret;
  

• }
• dsi->video_aclk = devm_clk_get(dev, "s_axis_aclk");
• if (IS_ERR(dsi->video_aclk)) {

• ret = PTR_ERR(dsi->video_aclk);
  

• dev_err(dev, "failed to get video_clk %d\n", ret);
  

• return ret;
  

• }
• ret = of_property_read_u32(node, "xlnx,dsi-num-lanes", &dsi->lanes);
• if (ret < 0) {

• dev_err(dsi->dev, "missing xlnx,dsi-num-lanes property\n");
  

• return ret;
  

• }
• if (dsi->lanes > 4 || dsi->lanes < 1) {

• dev_err(dsi->dev, "%d lanes : invalid lanes\n", dsi->lanes);
  

• return -EINVAL;
  

• }

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

OK.

...

...

• ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype);
• if (ret < 0) {

• dev_err(dsi->dev, "missing xlnx,dsi-data-type property\n");
  

• return ret;
  

• }
• dsi->format = datatype;
• if (datatype > MIPI_DSI_FMT_RGB565) {

• dev_err(dsi->dev, "Invalid xlnx,dsi-data-type string\n");
  

• return -EINVAL;
  

• }
• /*

• * multiplication factor used for HACT, based on data type.
  

• *
  

• * e.g. for RGB666_L datatype and 1920x1080 resolution,
  

• * the Hact (WC) would be as follows -
  

• * 1920 pixels * 18 bits per pixel / 8 bits per byte
  

• * = 1920 pixels * 2.25 bytes per pixel = 4320 bytes.
  

• *
  

• * Data Type - Multiplication factor
  

• * RGB888    - 3
  

• * RGB666_L  - 2.25
  

+- * RGB666_P - 2.25

• * RGB565    - 2
  

• *
  

• * Since the multiplication factor maybe a floating number,
  

• * a 100x multiplication factor is used.
  

• */
  

• dsi->mul_factor = xdsi_mul_fact[datatype];
• dsi->cmdmode = of_property_read_bool(node, "xlnx,dsi-cmd-mode");
• dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes);
• dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype);
• dev_dbg(dsi->dev, "DSI controller cmd mode = %d\n", dsi->cmdmode);
• return 0;

+}

+static int xlnx_dsi_bind(struct device *dev, struct device *master,

• 	 void *data)
  

+{

• struct xlnx_dsi *dsi = dev_get_drvdata(dev);
• struct drm_encoder *encoder = &dsi->encoder;
• struct drm_device *drm_dev = data;
• int ret;
• drm_encoder_init(drm_dev, encoder, &xlnx_dsi_encoder_funcs,

• 	 DRM_MODE_ENCODER_DSI, NULL);
  

• drm_encoder_helper_add(encoder, &xlnx_dsi_encoder_helper_funcs);
• encoder->possible_crtcs = 1;
• ret = xlnx_dsi_create_connector(encoder);
• if (ret) {

• dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret);
  

• drm_encoder_cleanup(encoder);
  

• return ret;
  

• }
• ret = mipi_dsi_host_register(&dsi->dsi_host);
• if (ret < 0) {

Just nit. if (ret) would do.

...

• dev_err(dsi->dev, "failed to register DSI host: %d\n", ret);
  

• xlnx_dsi_connector_destroy(&dsi->connector);
  

• drm_encoder_cleanup(encoder);
  

• return ret;
  

• }
• return 0;

+}

+static void xlnx_dsi_unbind(struct device *dev, struct device *master,

• 	    void *data)
  

+{

• struct xlnx_dsi *dsi = dev_get_drvdata(dev);
• xlnx_dsi_disable(&dsi->encoder);
• mipi_dsi_host_unregister(&dsi->dsi_host);

+}

+static const struct component_ops xlnx_dsi_component_ops = {

• .bind = xlnx_dsi_bind,
• .unbind = xlnx_dsi_unbind,

+};

+static int xlnx_dsi_probe(struct platform_device *pdev) {

• struct device *dev = &pdev->dev;
• struct resource *res;
• struct xlnx_dsi *dsi;
• int ret;
• unsigned long rate;
• dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
• if (!dsi)

• return -ENOMEM;
  

• dsi->dsi_host.ops = &xlnx_dsi_ops;
• dsi->dsi_host.dev = dev;
• dsi->dev = dev;
• ret = xlnx_dsi_parse_dt(dsi);
• if (ret)

• return ret;
  

• res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
• dsi->iomem = devm_ioremap_resource(dev, res);
• if (IS_ERR(dsi->iomem)) {

• dev_err(dev, "failed to remap io region\n");
  

• return PTR_ERR(dsi->iomem);
  

• }
• platform_set_drvdata(pdev, dsi);
• ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ);
• if (ret) {

• dev_err(dev, "failed to set dphy clk rate %d\n", ret);
  

• return ret;
  

• }
• ret = clk_prepare_enable(dsi->dphy_clk_200M);
• if (ret) {

• dev_err(dev, "failed to enable dphy clk %d\n", ret);
  

• return ret;
  

• }
• rate = clk_get_rate(dsi->dphy_clk_200M);
• if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) {

A brief comment for this line wold be helpful.

OK. I will add comment.

...

...

• dev_err(dev, "Error DPHY clock = %lu\n", rate);
  

• ret = -EINVAL;
  

• goto err_disable_dphy_clk;
  

• }
• ret = clk_prepare_enable(dsi->video_aclk);
• if (ret) {

• dev_err(dev, "failed to enable video clk %d\n", ret);
  

• goto err_disable_dphy_clk;
  

• }
• ret = component_add(dev, &xlnx_dsi_component_ops);

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

...

...

• if (ret < 0)

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

...

• goto err_disable_video_clk;
  

• return ret;

return 0;? but up to you.

...

+err_disable_video_clk:

• clk_disable_unprepare(dsi->video_aclk);

+err_disable_dphy_clk:

• clk_disable_unprepare(dsi->dphy_clk_200M);
• return ret;

+}

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

...

+static int xlnx_dsi_remove(struct platform_device *pdev) {

• struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
• component_del(&pdev->dev, &xlnx_dsi_component_ops);
• clk_disable_unprepare(dsi->video_aclk);
• clk_disable_unprepare(dsi->dphy_clk_200M);
• return 0;

+}

+static const struct of_device_id xlnx_dsi_of_match[] = {

• { .compatible = "xlnx-dsi", },

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

OK. I will update in v2 patch.

...

...

• { /* end of table */ },

+}; +MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match);

+static struct platform_driver dsi_driver = {

• .probe = xlnx_dsi_probe,
• .remove = xlnx_dsi_remove,
• .driver = {

• .name = "xlnx-dsi",
  

• .of_match_table = xlnx_dsi_of_match,
  

• },

+};

+module_platform_driver(dsi_driver);

+MODULE_AUTHOR("Xilinx, Inc."); +MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver"); +MODULE_LICENSE("GPL v2");

-- Regards,

Laurent Pinchart

Thanks, GVRao