[PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

Add a driver for Cadence DPI -> DSI bridge.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Changes in v2:
- rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
- return the correct error when devm_clk_get(sysclk) fails
- add missing depends on OF and select DRM_PANEL in the Kconfig entry
---
 drivers/gpu/drm/bridge/Kconfig    |    9 +
 drivers/gpu/drm/bridge/Makefile   |    1 +
 drivers/gpu/drm/bridge/cdns-dsi.c | 1077 +++++++++++++++++++++++++++++++++++++
 3 files changed, 1087 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index f6968d3b4b41..7d24b0e11634 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -17,6 +17,15 @@ config DRM_ANALOGIX_ANX78XX
 	  the HDMI output of an application processor to MyDP
 	  or DisplayPort.
 
+config DRM_CDNS_DSI
+	tristate "Cadence DPI/DSI bridge"
+	select DRM_KMS_HELPER
+	select DRM_MIPI_DSI
+	select DRM_PANEL
+	depends on OF
+	help
+	  Support Cadence DPI to DSI bridge.
+
 config DRM_DUMB_VGA_DAC
 	tristate "Dumb VGA DAC Bridge support"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 3fe2226ee2f2..73a3e32a8005 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,6 +1,7 @@
 ccflags-y := -Iinclude/drm
 
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
+obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
 obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
new file mode 100644
index 000000000000..70a4b91c80ee
--- /dev/null
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -0,0 +1,1077 @@
+/*
+ * Copyright: 2017 Cadence Design Systems, Inc.
+ *
+ * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+#include <video/mipi_display.h>
+
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define MCTL_MAIN_DATA_CTL		0x4
+#define DIS_DYN_CLK_CTL			BIT(30)
+#define IF_INTERLACED_EN(x)		BIT(26 + (x))
+#define TE_MIPI_POLLING_EN		BIT(25)
+#define TE_HW_POLLING_EN		BIT(24)
+#define DISP_EOT_GEN			BIT(18)
+#define HOST_EOT_GEN			BIT(17)
+#define DISP_GEN_CHECKSUM		BIT(16)
+#define DISP_GEN_ECC			BIT(15)
+#define BTA_EN				BIT(14)
+#define READ_EN				BIT(13)
+#define REG_TE_EN			BIT(12)
+#define IF_TE_EN(x)			BIT(8 + (x))
+#define TBG_SEL				BIT(7)
+#define TVG_SEL				BIT(6)
+#define VID_EN				BIT(5)
+#define IF_VID_SELECT(x)		((x) << 2)
+#define IF_VID_SELECT_MASK		GENMASK(4, 2)
+#define IF_VID_MODE			BIT(1)
+#define LINK_EN				BIT(0)
+
+#define MCTL_MAIN_PHY_CTL		0x8
+#define CLK_FORCE_STOP			BIT(14)
+#define WAIT_BURST_TIME(x)		((x) << 10)
+#define DATA_ULPM_EN(x)			BIT(6 + (x))
+#define CLK_ULPM_EN			BIT(5)
+#define CLK_CONTINUOUS			BIT(4)
+#define DATA_FORCE_STOP			BIT(3)
+#define DATA_LANE_EN(x)			BIT((x) - 1)
+
+#define MCTL_LANE_STS			0x10
+#define LANE_STATE_START		0
+#define LANE_STATE_IDLE			1
+#define LANE_STATE_WRITE		2
+#define LANE_STATE_ULPM			3
+#define LANE_STATE_READ			4
+#define DATA_LANE_STATE(l, val)		\
+	(((val) >> (2 + 2 * (l) + ((l) ? 1 : 0))) & GENMASK((l) ? 1 : 2, 0))
+#define CLK_LANE_STATE_HS		2
+#define CLK_LANE_STATE(val)		((val) & GENMASK(1, 0))
+
+#define MCTL_DPHY_TIMEOUT1		0x14
+#define CLK_DIV(x)			(x)
+#define HSTX_TIMEOUT(x)			((x) << 4)
+
+#define MCTL_DPHY_TIMEOUT2		0x18
+#define LPRX_TIMEOUT(x)			(x)
+
+#define MCTL_ULPOUT_TIME		0x1c
+#define DATA_LANE_ULPOUT_TIME(x)	((x) << 9)
+#define CLK_LANE_ULPOUT_TIME(x)		(x)
+
+#define MCTL_DPHY_STATIC		0x20
+#define CLK_UNIT_INTERVAL(x)		(((x) / 4) << 10)
+#define INVERT_DATA_HS(l)		BIT((l) * (2 + 1) + 1)
+#define SWAP_DATA_PINS(l)		BIT((l) * (2 + 1))
+#define INVERT_CLK_HS			BIT(1)
+#define SWAP_CLK_PINS			BIT(0)
+
+#define MCTL_MAIN_EN			0x24
+#define IF_EN(x)			BIT(13 + (x))
+#define DATA_LANE_ULPM_REQ(l)		BIT(9 + (l))
+#define CLK_LANE_ULPM_REQ		BIT(8)
+#define DATA_LANE_START(x)		BIT(4 + (x))
+#define CLK_LANE_EN			BIT(3)
+#define PLL_START			BIT(0)
+
+#define MCTL_MAIN_STS			0x28
+#define MCTL_MAIN_STS_CTL		0x130
+#define MCTL_MAIN_STS_CLR		0x150
+#define MCTL_MAIN_STS_FLAG		0x170
+#define DATA_LANE_MAP_ERR		BIT(12)
+#define IF_UNTERM_PKT_ERR(x)		BIT(8 + (x))
+#define LPRX_TIMEOUT_ERR		BIT(7)
+#define HSTX_TIMEOUT_ERR		BIT(6)
+#define DATA_LANE_RDY(l)		BIT(2 + (l))
+#define CLK_LANE_RDY			BIT(1)
+#define PLL_LOCKED			BIT(0)
+
+#define MCTL_DPHY_ERR			0x2c
+#define MCTL_DPHY_ERR_CTL1		0x148
+#define MCTL_DPHY_ERR_CLR		0x168
+#define MCTL_DPHY_ERR_FLAG		0x188
+#define ERR_CONT_LP(x, l)		BIT(18 + ((x) * 4) + (l))
+#define ERR_CONTROL(l)			BIT(14 + (l))
+#define ERR_SYNESC(l)			BIT(10 + (l))
+#define ERR_ESC(l)			BIT(6 + (l))
+
+#define MCTL_DPHY_ERR_CTL2		0x14c
+#define ERR_CONT_LP_EDGE(x, l)		BIT(12 + ((x) * 4) + (l))
+#define ERR_CONTROL_EDGE(l)		BIT(8 + (l))
+#define ERR_SYN_ESC_EDGE(l)		BIT(4 + (l))
+#define ERR_ESC_EDGE(l)			BIT(0 + (l))
+
+#define CMD_MODE_CTL			0x70
+#define IF_VCHAN_ID(x, c)		((c) << ((x) * 2))
+#define IF_LP_EN(x)			BIT(8 + (x))
+
+#define CMD_MODE_CTL2			0x74
+#define TE_TIMEOUT(x)			((x) << 11)
+#define FILL_VALUE(x)			((x) << 3)
+#define ARB_IF_WITH_HIGHEST_PRIORITY(x)	((x) << 1)
+#define ARB_ROUND_ROBIN_MODE		BIT(0)
+
+#define CMD_MODE_STS			0x78
+#define CMD_MODE_STS_CTL		0x134
+#define CMD_MODE_STS_CLR		0x154
+#define CMD_MODE_STS_FLAG		0x174
+#define ERR_IF_UNDERRUN(x)		BIT(4 + (x))
+#define ERR_UNWANTED_READ		BIT(3)
+#define ERR_TE_MISS			BIT(2)
+#define ERR_NO_TE			BIT(1)
+#define CSM_RUNNING			BIT(0)
+
+#define DIRECT_CMD_SEND			0x80
+
+#define DIRECT_CMD_MAIN_SETTINGS	0x84
+#define TRIGGER_VAL(x)			((x) << 25)
+#define CMD_LP_EN			BIT(24)
+#define CMD_SIZE(x)			((x) << 16)
+#define CMD_VCHAN_ID(x)			((x) << 14)
+#define CMD_DATATYPE(x)			((x) << 8)
+#define CMD_LONG			BIT(3)
+#define WRITE_CMD			0
+#define READ_CMD			1
+#define TE_REQ				4
+#define TRIGGER_REQ			5
+#define BTA_REQ				6
+
+#define DIRECT_CMD_STS			0x88
+#define DIRECT_CMD_STS_CTL		0x138
+#define DIRECT_CMD_STS_CLR		0x158
+#define DIRECT_CMD_STS_FLAG		0x178
+#define RCVD_ACK_VAL(val)		((val) >> 16)
+#define RCVD_TRIGGER_VAL(val)		(((val) & GENMASK(14, 11)) >> 11)
+#define READ_COMPLETED_WITH_ERR		BIT(10)
+#define BTA_FINISHED			BIT(9)
+#define BTA_COMPLETED			BIT(8)
+#define TE_RCVD				BIT(7)
+#define TRIGGER_RCVD			BIT(6)
+#define ACK_WITH_ERR_RCVD		BIT(5)
+#define ACK_RCVD			BIT(4)
+#define READ_COMPLETED			BIT(3)
+#define TRIGGER_COMPLETED		BIT(2)
+#define WRITE_COMPLETED			BIT(1)
+#define SENDING_CMD			BIT(0)
+
+#define DIRECT_CMD_STOP_READ		0x8c
+
+#define DIRECT_CMD_WRDATA		0x90
+#define WRDAT(x, val)			((val) << ((x) * 8))
+
+#define DIRECT_CMD_FIFO_RST		0x94
+
+#define DIRECT_CMD_RDDATA		0xa0
+#define RDDAT(x, val)			(((val) >> ((x) * 8)) & GENMASK(7, 0))
+
+#define DIRECT_CMD_RD_PROPS		0xa4
+#define RD_DCS				BIT(18)
+#define RD_VCHAN_ID(val)		(((val) >> 16) & GENMASK(1, 0))
+#define RD_SIZE(val)			((val) & GENMASK(15, 0))
+
+#define DIRECT_CMD_RD_STS		0xa8
+#define DIRECT_CMD_RD_STS_CTL		0x13c
+#define DIRECT_CMD_RD_STS_CLR		0x15c
+#define DIRECT_CMD_RD_STS_FLAG		0x17c
+#define ERR_EOT_WITH_ERR		BIT(8)
+#define ERR_MISSING_EOT			BIT(7)
+#define ERR_WRONG_LENGTH		BIT(6)
+#define ERR_OVERSIZE			BIT(5)
+#define ERR_RECEIVE			BIT(4)
+#define ERR_UNDECODABLE			BIT(3)
+#define ERR_CHECKSUM			BIT(2)
+#define ERR_UNCORRECTABLE		BIT(1)
+#define ERR_FIXED			BIT(0)
+
+#define VID_MAIN_CTL			0xb0
+#define VID_FIELD_SW			BIT(28)
+#define VID_INTERLACED_EN		BIT(27)
+#define RECOVERY_MODE(x)		((x) << 25)
+#define REG_BLKEOL_MODE(x)		((x) << 23)
+#define REG_BLKLINE_MODE(x)		((x) << 21)
+#define SYNC_PULSE_HORIZONTAL		BIT(20)
+#define SYNC_PULSE_ACTIVE		BIT(19)
+#define BURST_MODE			BIT(18)
+#define VID_PIXEL_MODE_MASK		GENMASK(17, 14)
+#define VID_PIXEL_MODE_RGB565		(0 << 14)
+#define VID_PIXEL_MODE_RGB666_PACKED	(1 << 14)
+#define VID_PIXEL_MODE_RGB666		(2 << 14)
+#define VID_PIXEL_MODE_RGB888		(3 << 14)
+#define VID_PIXEL_MODE_RGB101010	(4 << 14)
+#define VID_PIXEL_MODE_RGB121212	(5 << 14)
+#define VID_PIXEL_MODE_YUV420		(8 << 14)
+#define VID_PIXEL_MODE_YUV422_PACKED	(9 << 14)
+#define VID_PIXEL_MODE_YUV422		(10 << 14)
+#define VID_PIXEL_MODE_YUV422_24B	(11 << 14)
+#define VID_DATATYPE(x)			((x) << 8)
+#define VID_VIRTCHAN_ID(iface, x)	((x) << (4 + (iface) * 2))
+#define STOP_MODE(x)			((x) << 2)
+#define START_MODE(x)			(x)
+
+#define VID_VSIZE1			0xb4
+#define VFP_LEN(x)			((x) << 12)
+#define VBP_LEN(x)			((x) << 6)
+#define VSA_LEN(x)			(x)
+
+#define VID_VSIZE2			0xb8
+#define VACT_LEN(x)			(x)
+
+#define VID_HSIZE1			0xc0
+#define HFP_LEN(x)			((x) << 20)
+#define HBP_LEN(x)			((x) << 10)
+#define HSA_LEN(x)			(x)
+
+#define VID_HSIZE2			0xc4
+#define HACT_LEN(x)			(x)
+
+#define VID_BLKSIZE1			0xcc
+#define BLK_EOL_PKT_LEN(x)		((x) << 15)
+#define BLK_LINE_EVENT_PKT_LEN(x)	(x)
+
+#define VID_BLKSIZE2			0xd0
+#define BLK_LINE_PULSE_PKT_LEN(x)	(x)
+
+#define VID_PKT_TIME			0xd8
+#define BLK_EOL_DURATION(x)		(x)
+
+#define VID_DPHY_TIME			0xdc
+#define REG_WAKEUP_TIME(x)		((x) << 17)
+#define REG_LINE_DURATION(x)		(x)
+
+#define VID_ERR_COLOR1			0xe0
+#define COL_GREEN(x)			((x) << 12)
+#define COL_RED(x)			(x)
+
+#define VID_ERR_COLOR2			0xe4
+#define PAD_VAL(x)			((x) << 12)
+#define COL_BLUE(x)			(x)
+
+#define VID_VPOS			0xe8
+#define LINE_VAL(val)			(((val) & GENMASK(14, 2)) >> 2)
+#define LINE_POS(val)			((val) & GENMASK(1, 0))
+
+#define VID_HPOS			0xec
+#define HORIZ_VAL(val)			(((val) & GENMASK(17, 3)) >> 3)
+#define HORIZ_POS(val)			((val) & GENMASK(2, 0))
+
+#define VID_MODE_STS			0xf0
+#define VID_MODE_STS_CTL		0x140
+#define VID_MODE_STS_CLR		0x160
+#define VID_MODE_STS_FLAG		0x180
+#define VSG_RECOVERY			BIT(10)
+#define ERR_VRS_WRONG_LEN		BIT(9)
+#define ERR_LONG_READ			BIT(8)
+#define ERR_LINE_WRITE			BIT(7)
+#define ERR_BURST_WRITE			BIT(6)
+#define ERR_SMALL_HEIGHT		BIT(5)
+#define ERR_SMALL_LEN			BIT(4)
+#define ERR_MISSING_VSYNC		BIT(3)
+#define ERR_MISSING_HSYNC		BIT(2)
+#define ERR_MISSING_DATA		BIT(1)
+#define VSG_RUNNING			BIT(0)
+
+#define VID_VCA_SETTING1		0xf4
+#define BURST_LP			BIT(16)
+#define MAX_BURST_LIMIT(x)		(x)
+
+#define VID_VCA_SETTING2		0xf8
+#define MAX_LINE_LIMIT(x)		((x) << 16)
+#define EXACT_BURST_LIMIT(x)		(x)
+
+#define TVG_CTL				0xfc
+#define TVG_STRIPE_SIZE(x)		((x) << 5)
+#define TVG_MODE_MASK			GENMASK(4, 3)
+#define TVG_MODE_SINGLE_COLOR		(0 << 3)
+#define TVG_MODE_VSTRIPES		(2 << 3)
+#define TVG_MODE_HSTRIPES		(3 << 3)
+#define TVG_STOPMODE_MASK		GENMASK(2, 1)
+#define TVG_STOPMODE_EOF		(0 << 1)
+#define TVG_STOPMODE_EOL		(1 << 1)
+#define TVG_STOPMODE_NOW		(2 << 1)
+#define TVG_RUN				BIT(0)
+
+#define TVG_IMG_SIZE			0x100
+#define TVG_NBLINES(x)			((x) << 16)
+#define TVG_LINE_SIZE(x)		(x)
+
+#define TVG_COLOR1			0x104
+#define TVG_COL1_GREEN(x)		((x) << 12)
+#define TVG_COL1_RED(x)			(x)
+
+#define TVG_COLOR1_BIS			0x108
+#define TVG_COL1_BLUE(x)		(x)
+
+#define TVG_COLOR2			0x10c
+#define TVG_COL2_GREEN(x)		((x) << 12)
+#define TVG_COL2_RED(x)			(x)
+
+#define TVG_COLOR2_BIS			0x110
+#define TVG_COL2_BLUE(x)		(x)
+
+#define TVG_STS				0x114
+#define TVG_STS_RUNNING			BIT(0)
+
+#define TBG_CTL				0x118
+#define TBG_MODE_MASK			GENMASK(4, 3)
+#define TBG_MODE_START_1B_STOP		(0 << 3)
+#define TBG_MODE_START_2B_STOP		(1 << 3)
+#define TBG_MODE_START_BURST_CNT_STOP	(2 << 3)
+#define TBG_MODE_START_BURST_STOP	(3 << 3)
+#define TBG_DATA_SEL			BIT(2)
+#define TBG_HS_REQ			BIT(1)
+#define TBG_START			BIT(0)
+
+#define TBG_SETTING1			0x11c
+#define TBG_SETTING2			0x120
+
+#define TBG_STS				0x124
+#define TBG_STS_RUNNING			BIT(0)
+
+#define TVG_TBG_STS_CTL			0x144
+#define TVG_TBG_STS_CLR			0x164
+#define TVG_TBG_STS_FLAG		0x184
+#define TVG_TBG_STS_TBG_RUNNING		BIT(1)
+#define TVG_TBG_STS_TVG_RUNNING		BIT(0)
+
+#define STS_CTL_EDGE(e)			((e) << 16)
+
+#define DPHY_LANES_MAP			0x198
+#define DAT_REMAP_CFG(b, l)		((l) << ((b) * 8))
+
+#define DPI_IRQ_EN			0x1a0
+#define DPI_IRQ_CLR			0x1a4
+#define DPI_IRQ_STS			0x1a8
+#define PIXEL_BUF_OVERFLOW		BIT(0)
+
+#define DPI_CFG				0x1ac
+#define DPI_CFG_FIFO_LEVEL(x)		((x) & GENMASK(15, 0))
+#define DPI_CFG_FIFO_DEPTH(x)		((x) >> 16)
+
+#define DPHY_CFG0			0x1b0
+#define DPHY_C_RSTB			BIT(20)
+#define DPHY_D_RSTB(x)			((x) << 16)
+#define DPHY_TIF_FORCE_WRITE		BIT(12)
+#define DPHY_PLL_PDN			BIT(10)
+#define DPHY_CMN_PDN			BIT(9)
+#define DPHY_C_PDN			BIT(8)
+#define DPHY_D_PDN(x)			((x) << 4)
+#define DPHY_PLL_PSO			BIT(1)
+#define DPHY_CMN_PSO			BIT(0)
+
+#define DPHY_CFG1			0x1b4
+#define PDHY_PLL_OPDIV(x)		((x) << 20)
+#define PDHY_PLL_IPDIV(x)		((x) << 12)
+#define PDHY_PLL_FBDIV(x)		(x)
+
+#define DPHY_PLL_TM_LO			0x1b8
+#define DPHY_PLL_TM_MID			0x1bc
+#define DPHY_PLL_TM_HI			0x1c0
+
+#define DPHY_STATUS			0x1c4
+#define PPI_D_RX_ULPS_ESC(x)		((x) >> 12)
+#define PPI_C_TX_READY_HS		BIT(8)
+#define PPI_PLL_LOCK			BIT(7)
+#define PPI_PLL_COARSE			BIT(6)
+#define PPI_PLL_COARSE_CODE(x)		((x) & GENMASK(5, 0))
+
+#define DPHY_BIST			0x1c8
+#define PSO_BYPASS_CTX_EN		BIT(12)
+#define PSO_BYPASS_TX_EN(l)		BIT(8 + (l))
+#define BIST_CTX_EN			BIT(4)
+#define BIST_TX_EN(l)			BIT(l)
+
+#define TEST_GENERIC			0x1cc
+#define TEST_STATUS(x)			((x) >> 16)
+#define TEST_CTRL(x)			(x)
+
+#define ID_REG				0x1f0
+#define REV_VENDOR_ID(x)		(((x) & GENMASK(31, 20)) >> 20)
+#define REV_PRODUCT_ID(x)		(((x) & GENMASK(19, 12)) >> 12)
+#define REV_HW(x)			(((x) & GENMASK(11, 8)) >> 8)
+#define REV_MAJOR(x)			(((x) & GENMASK(7, 4)) >> 4)
+#define REV_MINOR(x)			((x) & GENMASK(3, 0))
+
+#define IP_CONF				0x1f4
+#define SP_HS_FIFO_DEPTH(x)		(((x) & GENMASK(30, 26)) >> 26)
+#define SP_LP_FIFO_DEPTH(x)		(((x) & GENMASK(25, 21)) >> 21)
+#define VRS_FIFO_DEPTH(x)			(((x) & GENMASK(20, 16)) >> 16)
+#define DIRCMD_FIFO_DEPTH(x)		(((x) & GENMASK(15, 13)) >> 13)
+#define SDI_IFACE_32			BIT(12)
+#define INTERNAL_DATAPATH_32		(0 << 10)
+#define INTERNAL_DATAPATH_16		(1 << 10)
+#define INTERNAL_DATAPATH_8		(3 << 10)
+#define INTERNAL_DATAPATH_SIZE		((x) & GENMASK(11, 10))
+#define INTERNAL_DATAPATH_32		(0 << 10)
+#define INTERNAL_DATAPATH_16		(1 << 10)
+#define INTERNAL_DATAPATH_8		(3 << 10)
+#define NUM_IFACE(x)			((((x) & GENMASK(9, 8)) >> 8) + 1)
+#define MAX_LANE_NB(x)			(((x) & GENMASK(7, 6)) >> 6)
+#define RX_FIFO_DEPTH(x)		((x) & GENMASK(5, 0))
+
+enum cdns_dsi_output_type {
+	CDNS_DSI_PANEL = 0,
+	CDNS_DSI_BRIDGE = 1,
+};
+
+struct cdns_dsi_output {
+	struct mipi_dsi_device *dev;
+	struct drm_connector connector;
+	enum cdns_dsi_output_type type;
+	union {
+		struct drm_panel *panel;
+		struct drm_bridge *bridge;
+	};
+};
+
+enum cdns_dsi_input_id {
+	CDNS_DPI_INPUT = 0,
+};
+
+struct cdns_dsi_input {
+	enum cdns_dsi_input_id id;
+	struct drm_bridge bridge;
+	struct cdns_dsi *dsi;
+};
+
+struct cdns_dsi {
+	struct mipi_dsi_host base;
+	void __iomem *regs;
+	struct cdns_dsi_input *input;
+	struct cdns_dsi_output *output;
+	unsigned int direct_cmd_fifo_depth;
+	unsigned int rx_fifo_depth;
+	struct completion direct_cmd_comp;
+	struct clk *pclk;
+	struct clk *sysclk;
+};
+
+static inline struct cdns_dsi *to_cdns_dsi(struct mipi_dsi_host *host)
+{
+	return container_of(host, struct cdns_dsi, base);
+}
+
+static inline struct cdns_dsi_input *
+bridge_to_cdns_dsi_input(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct cdns_dsi_input, bridge);
+}
+
+static inline struct cdns_dsi_output *
+connector_to_cdns_dsi_output(struct drm_connector *connector)
+{
+	return container_of(connector, struct cdns_dsi_output, connector);
+}
+
+static const struct drm_connector_funcs cdns_dsi_panel_conn_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int cdns_dsi_panel_connector_get_modes(struct drm_connector *connector)
+{
+	struct cdns_dsi_output *output;
+
+	output = connector_to_cdns_dsi_output(connector);
+
+	return drm_panel_get_modes(output->panel);
+}
+
+static const struct drm_connector_helper_funcs cdns_dsi_panel_conn_helper_funcs = {
+	.get_modes = cdns_dsi_panel_connector_get_modes,
+};
+
+static int cdns_dsi_output_attach_panel(struct cdns_dsi_output *output)
+{
+	struct cdns_dsi *dsi = to_cdns_dsi(output->dev->host);
+	struct drm_device *drm = dsi->input->bridge.dev;
+	int ret;
+
+	ret = drm_connector_init(drm, &output->connector,
+				 &cdns_dsi_panel_conn_funcs,
+				 DRM_MODE_CONNECTOR_DSI);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(&output->connector,
+				 &cdns_dsi_panel_conn_helper_funcs);
+
+	ret = drm_mode_connector_attach_encoder(&output->connector,
+						dsi->input->bridge.encoder);
+	if (ret)
+		return ret;
+
+	return drm_panel_attach(output->panel, &output->connector);
+}
+
+static int cdns_dsi_bridge_attach(struct drm_bridge *bridge)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	struct cdns_dsi_output *output = input->dsi->output;
+	int ret;
+
+	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
+		dev_err(input->dsi->base.dev,
+			"cdns-dsi driver is only compatible with DRM devices supporting atomic updates");
+		return -ENOTSUPP;
+	}
+
+	switch (output->type) {
+	case CDNS_DSI_PANEL:
+		ret = cdns_dsi_output_attach_panel(output);
+		break;
+
+	case CDNS_DSI_BRIDGE:
+		ret = drm_bridge_attach(bridge->encoder, output->bridge,
+					bridge);
+		break;
+
+	default:
+		ret = -ENOTSUPP;
+	}
+
+	return ret;
+}
+
+static bool cdns_dsi_bridge_mode_fixup(struct drm_bridge *bridge,
+				       const struct drm_display_mode *mode,
+				       struct drm_display_mode *adj)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	int bpp;
+
+	/*
+	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
+	 * least 1.
+	 */
+	if (adj->crtc_vtotal - adj->crtc_vsync_end < 2)
+		return false;
+
+	/* VSA_DSI = VSA_DPI and must be at least 2. */
+	if (adj->crtc_vsync_end - adj->crtc_vsync_start < 2)
+		return false;
+
+	/* HACT must be a 32-bits aligned. */
+	bpp = mipi_dsi_pixel_format_to_bpp(input->dsi->output->dev->format);
+	if ((adj->hdisplay * bpp) % 32)
+		return false;
+
+	return true;
+}
+
+static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	struct cdns_dsi *dsi = input->dsi;
+	u32 val;
+
+	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
+	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
+		 DISP_EOT_GEN);
+	writel(val, dsi->regs + MCTL_MAIN_DATA_CTL);
+
+	val = readl(dsi->regs + MCTL_MAIN_EN) & ~IF_EN(input->id);
+	writel(val, dsi->regs + MCTL_MAIN_EN);
+}
+
+#define DSI_HBP_FRAME_OVERHEAD		12
+#define DSI_HSA_FRAME_OVERHEAD		14
+#define DSI_HFP_FRAME_OVERHEAD		6
+#define DSI_HSS_VSS_VSEFRAME_OVERHEAD	6
+#define DSI_EOT_PKT_SIZE		4
+
+static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	struct cdns_dsi *dsi = input->dsi;
+	struct cdns_dsi_output *output = dsi->output;
+	struct drm_display_mode *mode;
+	u32 hsize0, hsa, hline, tmp;
+	int bpp, nlanes;
+
+	mode = &bridge->encoder->crtc->state->adjusted_mode;
+	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
+	nlanes = output->dev->lanes;
+
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+		tmp = mode->crtc_htotal - mode->crtc_hsync_end;
+	else
+		tmp = mode->crtc_htotal - mode->crtc_hsync_start;
+	tmp = (tmp * bpp) / 8;
+	tmp = tmp < DSI_HBP_FRAME_OVERHEAD ? 0 : tmp - DSI_HBP_FRAME_OVERHEAD;
+	hsize0 = HBP_LEN(tmp);
+
+	/* HFP can be disabled in burst mode only. */
+	if ((output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_HFP) &&
+	    (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST))
+		tmp = 0;
+	else
+		tmp = mode->crtc_hsync_start - mode->crtc_hdisplay;
+	tmp = (tmp * bpp) / 8;
+	tmp = tmp < DSI_HFP_FRAME_OVERHEAD ? 0 : tmp - DSI_HFP_FRAME_OVERHEAD;
+	hsize0 |= HFP_LEN(tmp);
+
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+		tmp = mode->crtc_hsync_end - mode->crtc_hsync_start;
+	else
+		tmp = 0;
+	tmp = (tmp * 8) / bpp;
+	tmp = tmp < DSI_HSA_FRAME_OVERHEAD ? 0 : tmp - DSI_HSA_FRAME_OVERHEAD;
+	hsa = tmp;
+	hsize0 |= HSA_LEN(tmp);
+
+	writel(hsize0, dsi->regs + VID_HSIZE1);
+	writel((mode->crtc_hdisplay * bpp) / 8, dsi->regs + VID_HSIZE2);
+
+	writel(VBP_LEN(mode->crtc_vtotal - mode->crtc_vsync_end - 1) |
+	       VFP_LEN(mode->crtc_vsync_start - mode->crtc_vdisplay) |
+	       VSA_LEN(mode->crtc_vsync_end - mode->crtc_vsync_start),
+	       dsi->regs + VID_VSIZE1);
+	writel(mode->crtc_vdisplay, dsi->regs + VID_VSIZE2);
+
+	hline = (mode->crtc_htotal * bpp) / 8;
+	tmp = hline - DSI_HSA_FRAME_OVERHEAD;
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+		tmp -= hsa + DSI_HSA_FRAME_OVERHEAD;
+
+	writel(0, dsi->regs + VID_BLKSIZE1);
+	writel(BLK_LINE_PULSE_PKT_LEN(tmp), dsi->regs + VID_BLKSIZE2);
+
+	tmp = DIV_ROUND_UP(hline, nlanes) - DIV_ROUND_UP(hsa, nlanes);
+
+	if (!(output->dev->mode_flags & MIPI_DSI_MODE_EOT_PACKET))
+		tmp -= DIV_ROUND_UP(DSI_EOT_PKT_SIZE, nlanes);
+
+	writel(REG_WAKEUP_TIME(0x1a8) | REG_LINE_DURATION(tmp),
+	       dsi->regs + VID_DPHY_TIME);
+
+	writel(0x0, dsi->regs + VID_PKT_TIME);
+
+	writel(MAX_BURST_LIMIT(0), dsi->regs + VID_VCA_SETTING1);
+	writel(MAX_LINE_LIMIT(0x6ec) | EXACT_BURST_LIMIT(0),
+	       dsi->regs + VID_VCA_SETTING2);
+
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO) {
+		switch (output->dev->format) {
+		case MIPI_DSI_FMT_RGB888:
+			tmp = VID_PIXEL_MODE_RGB888 |
+			      VID_DATATYPE(MIPI_DSI_PACKED_PIXEL_STREAM_24);
+			break;
+
+		case MIPI_DSI_FMT_RGB666:
+			tmp = VID_PIXEL_MODE_RGB666 |
+			      VID_DATATYPE(MIPI_DSI_PIXEL_STREAM_3BYTE_18);
+			break;
+
+		case MIPI_DSI_FMT_RGB666_PACKED:
+			tmp = VID_PIXEL_MODE_RGB666_PACKED |
+			      VID_DATATYPE(MIPI_DSI_PACKED_PIXEL_STREAM_18);
+			break;
+
+		case MIPI_DSI_FMT_RGB565:
+			tmp = VID_PIXEL_MODE_RGB666_PACKED |
+			      VID_DATATYPE(MIPI_DSI_PACKED_PIXEL_STREAM_16);
+			break;
+
+		default:
+			dev_err(dsi->base.dev, "Unsupported DSI format\n");
+			return;
+		}
+
+		if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+			tmp |= SYNC_PULSE_ACTIVE | SYNC_PULSE_HORIZONTAL;
+
+		tmp |= REG_BLKLINE_MODE(1) | REG_BLKEOL_MODE(1) |
+		       RECOVERY_MODE(1);
+
+		writel(tmp, dsi->regs + VID_MAIN_CTL);
+	}
+
+	tmp = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
+	tmp &= ~(IF_VID_SELECT_MASK | HOST_EOT_GEN | IF_VID_MODE);
+
+	if (!(output->dev->mode_flags & MIPI_DSI_MODE_EOT_PACKET))
+		tmp |= HOST_EOT_GEN;
+
+	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO)
+		tmp |= IF_VID_MODE | IF_VID_SELECT(input->id) | VID_EN;
+
+	writel(tmp, dsi->regs + MCTL_MAIN_DATA_CTL);
+
+	tmp = readl(dsi->regs + MCTL_MAIN_EN) | IF_EN(input->id);
+	writel(tmp, dsi->regs + MCTL_MAIN_EN);
+}
+
+static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
+	.attach = cdns_dsi_bridge_attach,
+	.mode_fixup = cdns_dsi_bridge_mode_fixup,
+	.disable = cdns_dsi_bridge_disable,
+	.enable = cdns_dsi_bridge_enable,
+};
+
+static int cdns_dsi_init_link(struct cdns_dsi *dsi)
+{
+	u32 val;
+	int i;
+
+	writel(CLK_UNIT_INTERVAL(16), dsi->regs + MCTL_DPHY_STATIC);
+	writel(CLK_DIV(0xb) | HSTX_TIMEOUT(0xed8afff),
+	       dsi->regs + MCTL_DPHY_TIMEOUT1);
+	writel(LPRX_TIMEOUT(0xf30fffff), dsi->regs + MCTL_DPHY_TIMEOUT2);
+
+	val = WAIT_BURST_TIME(0xf);
+	for (i = 1; i < dsi->output->dev->lanes; i++)
+		val |= DATA_LANE_EN(i);
+
+	if (!(dsi->output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
+		val |= CLK_CONTINUOUS;
+
+	writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
+
+	writel(CLK_LANE_ULPOUT_TIME(0x105) | DATA_LANE_ULPOUT_TIME(0x1d5),
+	       dsi->regs + MCTL_ULPOUT_TIME);
+
+	writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
+
+	val = CLK_LANE_EN | PLL_START;
+	for (i = 0; i < dsi->output->dev->lanes; i++)
+		val |= DATA_LANE_START(i);
+
+	writel(val, dsi->regs + MCTL_MAIN_EN);
+
+	ndelay(100);
+
+	return 0;
+}
+
+static int cdns_dsi_attach(struct mipi_dsi_host *host,
+			   struct mipi_dsi_device *dev)
+{
+	struct cdns_dsi *dsi = to_cdns_dsi(host);
+	struct cdns_dsi_output *output;
+	int ret;
+
+	/* TODO: support multi-devices setup? */
+	if (dsi->output)
+		return -EBUSY;
+
+	output = devm_kzalloc(host->dev, sizeof(*output), GFP_KERNEL);
+	if (!output)
+		return -ENOMEM;
+
+	output->dev = dev;
+
+	output->panel = of_drm_find_panel(dev->dev.of_node);
+	if (output->panel) {
+		output->type = CDNS_DSI_PANEL;
+	} else {
+		output->bridge = of_drm_find_bridge(dev->dev.of_node);
+		if (!output->bridge) {
+			dev_err(host->dev,
+				"%s is neither a panel nor a bridge",
+				dev->name);
+			return -ENOTSUPP;
+		}
+
+		output->type = CDNS_DSI_BRIDGE;
+		dsi->input->bridge.next = output->bridge;
+	}
+
+	dsi->output = output;
+
+	ret = cdns_dsi_init_link(dsi);
+	if (ret)
+		return ret;
+
+	/* FIXME: should be moved somewhere else. */
+	return drm_bridge_add(&dsi->input->bridge);
+}
+
+static int cdns_dsi_detach(struct mipi_dsi_host *host,
+			   struct mipi_dsi_device *dev)
+{
+	struct cdns_dsi *dsi = to_cdns_dsi(host);
+
+	writel(0, dsi->regs + MCTL_MAIN_EN);
+	writel(0, dsi->regs + MCTL_MAIN_DATA_CTL);
+	writel(0, dsi->regs + MCTL_MAIN_PHY_CTL);
+
+	return 0;
+}
+
+static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
+{
+	struct cdns_dsi *dsi = data;
+	irqreturn_t ret = IRQ_NONE;
+	u32 flag, ctl;
+
+	flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
+	if (flag) {
+		ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
+		ctl &= ~flag;
+		writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);
+		complete(&dsi->direct_cmd_comp);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
+				 const struct mipi_dsi_msg *msg)
+{
+	struct cdns_dsi *dsi = to_cdns_dsi(host);
+	u32 cmd, sts, val, wait = WRITE_COMPLETED, ctl = 0;
+	struct mipi_dsi_packet packet;
+	int ret, i, tx_len, rx_len;
+
+	ret = mipi_dsi_create_packet(&packet, msg);
+	if (ret)
+		return ret;
+
+	tx_len = msg->tx_buf ? msg->tx_len : 0;
+	rx_len = msg->rx_buf ? msg->rx_len : 0;
+
+	/* For read operations, the maximum TX len is 2. */
+	if (rx_len && tx_len > 2)
+		return -ENOTSUPP;
+
+	/* TX len is limited by the CMD FIFO depth. */
+	if (tx_len > dsi->direct_cmd_fifo_depth)
+		return -ENOTSUPP;
+
+	/* RX len is limited by the RX FIFO depth. */
+	if (rx_len > dsi->rx_fifo_depth)
+		return -ENOTSUPP;
+
+	cmd = CMD_SIZE(tx_len) | CMD_VCHAN_ID(msg->channel) |
+	      CMD_DATATYPE(msg->type);
+
+	if (msg->flags & MIPI_DSI_MSG_USE_LPM)
+		cmd |= CMD_LP_EN;
+
+	if (mipi_dsi_packet_format_is_long(msg->type))
+		cmd |= CMD_LONG;
+
+	if (rx_len) {
+		cmd |= READ_CMD;
+		wait = READ_COMPLETED_WITH_ERR | READ_COMPLETED;
+		ctl = READ_EN | BTA_EN;
+	} else if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
+		cmd |= BTA_REQ;
+		wait = ACK_WITH_ERR_RCVD | ACK_RCVD;
+		ctl = BTA_EN;
+	}
+
+	writel(readl(dsi->regs + MCTL_MAIN_DATA_CTL) | ctl,
+	       dsi->regs + MCTL_MAIN_DATA_CTL);
+
+	writel(cmd, dsi->regs + DIRECT_CMD_MAIN_SETTINGS);
+
+	for (i = 0; i < tx_len; i += 4) {
+		const u8 *buf = msg->tx_buf;
+		int j;
+
+		val = 0;
+		for (j = 0; j < 4 && j + i < tx_len; j++)
+			val |= (u32)buf[i + j] << (8 * j);
+
+		writel(val, dsi->regs + DIRECT_CMD_WRDATA);
+	}
+
+	/* Clear status flags before sending the command. */
+	writel(wait, dsi->regs + DIRECT_CMD_STS_CLR);
+	writel(wait, dsi->regs + DIRECT_CMD_STS_CTL);
+	reinit_completion(&dsi->direct_cmd_comp);
+	writel(0, dsi->regs + DIRECT_CMD_SEND);
+
+	wait_for_completion_timeout(&dsi->direct_cmd_comp,
+				    msecs_to_jiffies(1000));
+
+	sts = readl(dsi->regs + DIRECT_CMD_STS);
+	writel(wait, dsi->regs + DIRECT_CMD_STS_CLR);
+	writel(0, dsi->regs + DIRECT_CMD_STS_CTL);
+
+	writel(readl(dsi->regs + MCTL_MAIN_DATA_CTL) & ~ctl,
+	       dsi->regs + MCTL_MAIN_DATA_CTL);
+
+	/* We did not receive the events we were waiting for. */
+	if (!(sts & wait))
+		return -ETIMEDOUT;
+
+	/* READ of WRITE with ACK failed. */
+	if (sts & (READ_COMPLETED_WITH_ERR | ACK_WITH_ERR_RCVD))
+		return -EIO;
+
+	for (i = 0; i < rx_len; i += 4) {
+		u8 *buf = msg->rx_buf;
+		int j;
+
+		val = readl(dsi->regs + DIRECT_CMD_RDDATA);
+		for (j = 0; j < 4 && j + i < rx_len; j++)
+			buf[i + j] = val >> (8 * j);
+	}
+
+	return 0;
+}
+
+static const struct mipi_dsi_host_ops cdns_dsi_ops = {
+	.attach = cdns_dsi_attach,
+	.detach = cdns_dsi_detach,
+	.transfer = cdns_dsi_transfer,
+};
+
+static int cdns_dsi_drm_probe(struct platform_device *pdev)
+{
+	struct cdns_dsi *dsi;
+	struct cdns_dsi_input *input;
+	struct resource *res;
+	int ret, irq;
+	u32 val;
+
+	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
+	if (!dsi)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dsi->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dsi->regs))
+		return PTR_ERR(dsi->regs);
+
+	dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(dsi->pclk))
+		return PTR_ERR(dsi->pclk);
+
+	dsi->sysclk = devm_clk_get(&pdev->dev, "sysclk");
+	if (IS_ERR(dsi->sysclk))
+		return PTR_ERR(dsi->sysclk);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = clk_prepare_enable(dsi->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(dsi->sysclk);
+	if (ret)
+		goto err_dis_pclk;
+
+	val = readl(dsi->regs + ID_REG);
+	if (REV_VENDOR_ID(val) != 0xcad) {
+		dev_err(&pdev->dev, "invalid vendor id\n");
+		return -EINVAL;
+	}
+
+	val = readl(dsi->regs + IP_CONF);
+	dsi->direct_cmd_fifo_depth = 1 << (DIRCMD_FIFO_DEPTH(val) + 2);
+	dsi->rx_fifo_depth = RX_FIFO_DEPTH(val);
+	init_completion(&dsi->direct_cmd_comp);
+
+	writel(0, dsi->regs + MCTL_MAIN_DATA_CTL);
+	writel(0, dsi->regs + MCTL_MAIN_EN);
+	writel(0, dsi->regs + MCTL_MAIN_PHY_CTL);
+
+	input = devm_kzalloc(&pdev->dev, sizeof(*input), GFP_KERNEL);
+	if (!input) {
+		ret = -ENOMEM;
+		goto err_dis_sysclk;
+	}
+
+	input->dsi = dsi;
+	dsi->input = input;
+
+	/* Mask all interrupts before registering the IRQ handler. */
+	writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
+	writel(0, dsi->regs + MCTL_DPHY_ERR_CTL1);
+	writel(0, dsi->regs + CMD_MODE_STS_CTL);
+	writel(0, dsi->regs + DIRECT_CMD_STS_CTL);
+	writel(0, dsi->regs + DIRECT_CMD_RD_STS_CTL);
+	writel(0, dsi->regs + VID_MODE_STS_CTL);
+	writel(0, dsi->regs + TVG_TBG_STS_CTL);
+	writel(0, dsi->regs + DPI_IRQ_EN);
+	ret = devm_request_irq(&pdev->dev, irq, cdns_dsi_interrupt, 0,
+			       dev_name(&pdev->dev), dsi);
+	if (ret)
+		goto err_dis_sysclk;
+
+	dsi->base.dev = &pdev->dev;
+	dsi->base.ops = &cdns_dsi_ops;
+
+	ret = mipi_dsi_host_register(&dsi->base);
+	if (ret)
+		goto err_dis_sysclk;
+
+	input->bridge.funcs = &cdns_dsi_bridge_funcs;
+	input->bridge.of_node = pdev->dev.of_node;
+
+	platform_set_drvdata(pdev, dsi);
+
+	return 0;
+
+err_dis_sysclk:
+	clk_disable_unprepare(dsi->sysclk);
+
+err_dis_pclk:
+	clk_disable_unprepare(dsi->pclk);
+
+	return ret;
+}
+
+static int cdns_dsi_drm_remove(struct platform_device *pdev)
+{
+	struct cdns_dsi *dsi = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&dsi->input->bridge);
+	mipi_dsi_host_unregister(&dsi->base);
+	clk_disable_unprepare(dsi->sysclk);
+	clk_disable_unprepare(dsi->pclk);
+
+	return 0;
+}
+
+static const struct of_device_id cdns_dsi_of_match[] = {
+	{ .compatible = "cdns,dsi" },
+	{ },
+};
+
+static struct platform_driver cdns_dsi_platform_driver = {
+	.probe  = cdns_dsi_drm_probe,
+	.remove = cdns_dsi_drm_remove,
+	.driver = {
+		.name   = "cdns-dsi",
+		.of_match_table = cdns_dsi_of_match,
+	},
+};
+module_platform_driver(cdns_dsi_platform_driver);
+
+MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
+MODULE_DESCRIPTION("Cadence DSI driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:cdns-dsi");
+
-- 
2.7.4

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

[PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Boris Brezillon]

Document the bindings used for the Cadence DSI bridge.

Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
new file mode 100644
index 000000000000..770c5c5b1e93
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
@@ -0,0 +1,55 @@
+Cadence DSI bridge
+==================
+
+The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
+
+Required properties:
+- compatible: should be set to "cdns,dsi".
+- reg: physical base address and length of the controller's registers.
+- interrupts: interrupt line connected to the DSI bridge.
+- clocks: DSI bridge clocks.
+- clock-names: must contain "pclk" and "sysclk".
+- phys: phandle link to the MIPI D-PHY controller.
+- phy-names: must contain "dphy".
+- #address-cells: must be set to 1.
+- #size-cells: must be set to 0.
+
+Required subnodes:
+- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
+  Currently contains a single input port at address 0 representing the DPI
+  input. Other ports will be added later to support the SDI inputs.
+  Port 0 should be connected to a DPI encoder output.
+- one subnode per DSI device connected on the DSI bus. Each DSI device should
+  contain a reg property encoding its address on the bus.
+
+Example:
+
+	dsi0: dsi@fd0c0000 {
+		compatible = "cdns,dsi";
+		reg = <0x0 0xfd0c0000 0x0 0x1000>;
+		clocks = <&pclk>, <&sysclk>;
+		clock-names = "pclk", "hclk";
+		interrupts = <1>;
+		phys = <&dphy1>;
+		phy-names = "dphy";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				dsi0_dpi_input: endpoint {
+					remote-endpoint = <&xxx_dpi_output>;
+				};
+			};
+		};
+
+		panel: dsi-dev@0 {
+			compatible = "<vendor,panel>";
+			reg = <0>;
+		};
+	};
+
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Archit Taneja]

Hi,

On 06/02/2017 05:34 PM, Boris Brezillon wrote:
> Document the bindings used for the Cadence DSI bridge.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> new file mode 100644
> index 000000000000..770c5c5b1e93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> @@ -0,0 +1,55 @@
> +Cadence DSI bridge
> +==================
> +
> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.

Is this a separate chip, or an IP integrated into SoCs? If it's the 
latter, I don't think DPI on the its input side is the right term to 
use. Maybe RGB would be more appropriate here.

> +
> +Required properties:
> +- compatible: should be set to "cdns,dsi".

Would it be better to take a dw-hdmi like approach here? I.e, the
binding should be specific to the SoC that integrates this DSI
bridge?

> +- reg: physical base address and length of the controller's registers.
> +- interrupts: interrupt line connected to the DSI bridge.
> +- clocks: DSI bridge clocks.
> +- clock-names: must contain "pclk" and "sysclk".
> +- phys: phandle link to the MIPI D-PHY controller.
> +- phy-names: must contain "dphy".
> +- #address-cells: must be set to 1.
> +- #size-cells: must be set to 0.
> +
> +Required subnodes:
> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> +  Currently contains a single input port at address 0 representing the DPI
> +  input. Other ports will be added later to support the SDI inputs.
> +  Port 0 should be connected to a DPI encoder output.

The output of the DSI bridge may be another bridge, which could be i2c
controlled. In that case, it won't be a child of the DSI bridge. For
such scenarios, we might want to define an output port for the bridge.

Thanks,
Archit

> +- one subnode per DSI device connected on the DSI bus. Each DSI device should
> +  contain a reg property encoding its address on the bus.
> +
> +Example:
> +
> +	dsi0: dsi@fd0c0000 {
> +		compatible = "cdns,dsi";
> +		reg = <0x0 0xfd0c0000 0x0 0x1000>;
> +		clocks = <&pclk>, <&sysclk>;
> +		clock-names = "pclk", "hclk";
> +		interrupts = <1>;
> +		phys = <&dphy1>;
> +		phy-names = "dphy";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				dsi0_dpi_input: endpoint {
> +					remote-endpoint = <&xxx_dpi_output>;
> +				};
> +			};
> +		};
> +
> +		panel: dsi-dev@0 {
> +			compatible = "<vendor,panel>";
> +			reg = <0>;
> +		};
> +	};
> +
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Archit Taneja]

On 6/2/2017 5:34 PM, Boris Brezillon wrote:
> Add a driver for Cadence DPI -> DSI bridge.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> Changes in v2:
> - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> - return the correct error when devm_clk_get(sysclk) fails
> - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> ---
>   drivers/gpu/drm/bridge/Kconfig    |    9 +
>   drivers/gpu/drm/bridge/Makefile   |    1 +
>   drivers/gpu/drm/bridge/cdns-dsi.c | 1077 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 1087 insertions(+)
>   create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index f6968d3b4b41..7d24b0e11634 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -17,6 +17,15 @@ config DRM_ANALOGIX_ANX78XX
>   	  the HDMI output of an application processor to MyDP
>   	  or DisplayPort.
>   
> +config DRM_CDNS_DSI
> +	tristate "Cadence DPI/DSI bridge"
> +	select DRM_KMS_HELPER
> +	select DRM_MIPI_DSI
> +	select DRM_PANEL
> +	depends on OF
> +	help
> +	  Support Cadence DPI to DSI bridge.
> +
>   config DRM_DUMB_VGA_DAC
>   	tristate "Dumb VGA DAC Bridge support"
>   	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 3fe2226ee2f2..73a3e32a8005 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>   ccflags-y := -Iinclude/drm
>   
>   obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> +obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>   obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
>   obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>   obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> new file mode 100644
> index 000000000000..70a4b91c80ee
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> @@ -0,0 +1,1077 @@
> +/*
> + * Copyright: 2017 Cadence Design Systems, Inc.
> + *
> + * Author: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +#include <video/mipi_display.h>
> +
> +#include <linux/clk.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define MCTL_MAIN_DATA_CTL		0x4
> +#define DIS_DYN_CLK_CTL			BIT(30)
> +#define IF_INTERLACED_EN(x)		BIT(26 + (x))
> +#define TE_MIPI_POLLING_EN		BIT(25)
> +#define TE_HW_POLLING_EN		BIT(24)
> +#define DISP_EOT_GEN			BIT(18)
> +#define HOST_EOT_GEN			BIT(17)
> +#define DISP_GEN_CHECKSUM		BIT(16)
> +#define DISP_GEN_ECC			BIT(15)
> +#define BTA_EN				BIT(14)
> +#define READ_EN				BIT(13)
> +#define REG_TE_EN			BIT(12)
> +#define IF_TE_EN(x)			BIT(8 + (x))
> +#define TBG_SEL				BIT(7)
> +#define TVG_SEL				BIT(6)
> +#define VID_EN				BIT(5)
> +#define IF_VID_SELECT(x)		((x) << 2)
> +#define IF_VID_SELECT_MASK		GENMASK(4, 2)
> +#define IF_VID_MODE			BIT(1)
> +#define LINK_EN				BIT(0)
> +
> +#define MCTL_MAIN_PHY_CTL		0x8
> +#define CLK_FORCE_STOP			BIT(14)
> +#define WAIT_BURST_TIME(x)		((x) << 10)
> +#define DATA_ULPM_EN(x)			BIT(6 + (x))
> +#define CLK_ULPM_EN			BIT(5)
> +#define CLK_CONTINUOUS			BIT(4)
> +#define DATA_FORCE_STOP			BIT(3)
> +#define DATA_LANE_EN(x)			BIT((x) - 1)
> +
> +#define MCTL_LANE_STS			0x10
> +#define LANE_STATE_START		0
> +#define LANE_STATE_IDLE			1
> +#define LANE_STATE_WRITE		2
> +#define LANE_STATE_ULPM			3
> +#define LANE_STATE_READ			4
> +#define DATA_LANE_STATE(l, val)		\
> +	(((val) >> (2 + 2 * (l) + ((l) ? 1 : 0))) & GENMASK((l) ? 1 : 2, 0))
> +#define CLK_LANE_STATE_HS		2
> +#define CLK_LANE_STATE(val)		((val) & GENMASK(1, 0))
> +
> +#define MCTL_DPHY_TIMEOUT1		0x14
> +#define CLK_DIV(x)			(x)
> +#define HSTX_TIMEOUT(x)			((x) << 4)
> +
> +#define MCTL_DPHY_TIMEOUT2		0x18
> +#define LPRX_TIMEOUT(x)			(x)
> +
> +#define MCTL_ULPOUT_TIME		0x1c
> +#define DATA_LANE_ULPOUT_TIME(x)	((x) << 9)
> +#define CLK_LANE_ULPOUT_TIME(x)		(x)
> +
> +#define MCTL_DPHY_STATIC		0x20
> +#define CLK_UNIT_INTERVAL(x)		(((x) / 4) << 10)
> +#define INVERT_DATA_HS(l)		BIT((l) * (2 + 1) + 1)
> +#define SWAP_DATA_PINS(l)		BIT((l) * (2 + 1))
> +#define INVERT_CLK_HS			BIT(1)
> +#define SWAP_CLK_PINS			BIT(0)
> +
> +#define MCTL_MAIN_EN			0x24
> +#define IF_EN(x)			BIT(13 + (x))
> +#define DATA_LANE_ULPM_REQ(l)		BIT(9 + (l))
> +#define CLK_LANE_ULPM_REQ		BIT(8)
> +#define DATA_LANE_START(x)		BIT(4 + (x))
> +#define CLK_LANE_EN			BIT(3)
> +#define PLL_START			BIT(0)
> +
> +#define MCTL_MAIN_STS			0x28
> +#define MCTL_MAIN_STS_CTL		0x130
> +#define MCTL_MAIN_STS_CLR		0x150
> +#define MCTL_MAIN_STS_FLAG		0x170
> +#define DATA_LANE_MAP_ERR		BIT(12)
> +#define IF_UNTERM_PKT_ERR(x)		BIT(8 + (x))
> +#define LPRX_TIMEOUT_ERR		BIT(7)
> +#define HSTX_TIMEOUT_ERR		BIT(6)
> +#define DATA_LANE_RDY(l)		BIT(2 + (l))
> +#define CLK_LANE_RDY			BIT(1)
> +#define PLL_LOCKED			BIT(0)
> +
> +#define MCTL_DPHY_ERR			0x2c
> +#define MCTL_DPHY_ERR_CTL1		0x148
> +#define MCTL_DPHY_ERR_CLR		0x168
> +#define MCTL_DPHY_ERR_FLAG		0x188
> +#define ERR_CONT_LP(x, l)		BIT(18 + ((x) * 4) + (l))
> +#define ERR_CONTROL(l)			BIT(14 + (l))
> +#define ERR_SYNESC(l)			BIT(10 + (l))
> +#define ERR_ESC(l)			BIT(6 + (l))
> +
> +#define MCTL_DPHY_ERR_CTL2		0x14c
> +#define ERR_CONT_LP_EDGE(x, l)		BIT(12 + ((x) * 4) + (l))
> +#define ERR_CONTROL_EDGE(l)		BIT(8 + (l))
> +#define ERR_SYN_ESC_EDGE(l)		BIT(4 + (l))
> +#define ERR_ESC_EDGE(l)			BIT(0 + (l))
> +
> +#define CMD_MODE_CTL			0x70
> +#define IF_VCHAN_ID(x, c)		((c) << ((x) * 2))
> +#define IF_LP_EN(x)			BIT(8 + (x))
> +
> +#define CMD_MODE_CTL2			0x74
> +#define TE_TIMEOUT(x)			((x) << 11)
> +#define FILL_VALUE(x)			((x) << 3)
> +#define ARB_IF_WITH_HIGHEST_PRIORITY(x)	((x) << 1)
> +#define ARB_ROUND_ROBIN_MODE		BIT(0)
> +
> +#define CMD_MODE_STS			0x78
> +#define CMD_MODE_STS_CTL		0x134
> +#define CMD_MODE_STS_CLR		0x154
> +#define CMD_MODE_STS_FLAG		0x174
> +#define ERR_IF_UNDERRUN(x)		BIT(4 + (x))
> +#define ERR_UNWANTED_READ		BIT(3)
> +#define ERR_TE_MISS			BIT(2)
> +#define ERR_NO_TE			BIT(1)
> +#define CSM_RUNNING			BIT(0)
> +
> +#define DIRECT_CMD_SEND			0x80
> +
> +#define DIRECT_CMD_MAIN_SETTINGS	0x84
> +#define TRIGGER_VAL(x)			((x) << 25)
> +#define CMD_LP_EN			BIT(24)
> +#define CMD_SIZE(x)			((x) << 16)
> +#define CMD_VCHAN_ID(x)			((x) << 14)
> +#define CMD_DATATYPE(x)			((x) << 8)
> +#define CMD_LONG			BIT(3)
> +#define WRITE_CMD			0
> +#define READ_CMD			1
> +#define TE_REQ				4
> +#define TRIGGER_REQ			5
> +#define BTA_REQ				6
> +
> +#define DIRECT_CMD_STS			0x88
> +#define DIRECT_CMD_STS_CTL		0x138
> +#define DIRECT_CMD_STS_CLR		0x158
> +#define DIRECT_CMD_STS_FLAG		0x178
> +#define RCVD_ACK_VAL(val)		((val) >> 16)
> +#define RCVD_TRIGGER_VAL(val)		(((val) & GENMASK(14, 11)) >> 11)
> +#define READ_COMPLETED_WITH_ERR		BIT(10)
> +#define BTA_FINISHED			BIT(9)
> +#define BTA_COMPLETED			BIT(8)
> +#define TE_RCVD				BIT(7)
> +#define TRIGGER_RCVD			BIT(6)
> +#define ACK_WITH_ERR_RCVD		BIT(5)
> +#define ACK_RCVD			BIT(4)
> +#define READ_COMPLETED			BIT(3)
> +#define TRIGGER_COMPLETED		BIT(2)
> +#define WRITE_COMPLETED			BIT(1)
> +#define SENDING_CMD			BIT(0)
> +
> +#define DIRECT_CMD_STOP_READ		0x8c
> +
> +#define DIRECT_CMD_WRDATA		0x90
> +#define WRDAT(x, val)			((val) << ((x) * 8))
> +
> +#define DIRECT_CMD_FIFO_RST		0x94
> +
> +#define DIRECT_CMD_RDDATA		0xa0
> +#define RDDAT(x, val)			(((val) >> ((x) * 8)) & GENMASK(7, 0))
> +
> +#define DIRECT_CMD_RD_PROPS		0xa4
> +#define RD_DCS				BIT(18)
> +#define RD_VCHAN_ID(val)		(((val) >> 16) & GENMASK(1, 0))
> +#define RD_SIZE(val)			((val) & GENMASK(15, 0))
> +
> +#define DIRECT_CMD_RD_STS		0xa8
> +#define DIRECT_CMD_RD_STS_CTL		0x13c
> +#define DIRECT_CMD_RD_STS_CLR		0x15c
> +#define DIRECT_CMD_RD_STS_FLAG		0x17c
> +#define ERR_EOT_WITH_ERR		BIT(8)
> +#define ERR_MISSING_EOT			BIT(7)
> +#define ERR_WRONG_LENGTH		BIT(6)
> +#define ERR_OVERSIZE			BIT(5)
> +#define ERR_RECEIVE			BIT(4)
> +#define ERR_UNDECODABLE			BIT(3)
> +#define ERR_CHECKSUM			BIT(2)
> +#define ERR_UNCORRECTABLE		BIT(1)
> +#define ERR_FIXED			BIT(0)
> +
> +#define VID_MAIN_CTL			0xb0
> +#define VID_FIELD_SW			BIT(28)
> +#define VID_INTERLACED_EN		BIT(27)
> +#define RECOVERY_MODE(x)		((x) << 25)
> +#define REG_BLKEOL_MODE(x)		((x) << 23)
> +#define REG_BLKLINE_MODE(x)		((x) << 21)
> +#define SYNC_PULSE_HORIZONTAL		BIT(20)
> +#define SYNC_PULSE_ACTIVE		BIT(19)
> +#define BURST_MODE			BIT(18)
> +#define VID_PIXEL_MODE_MASK		GENMASK(17, 14)
> +#define VID_PIXEL_MODE_RGB565		(0 << 14)
> +#define VID_PIXEL_MODE_RGB666_PACKED	(1 << 14)
> +#define VID_PIXEL_MODE_RGB666		(2 << 14)
> +#define VID_PIXEL_MODE_RGB888		(3 << 14)
> +#define VID_PIXEL_MODE_RGB101010	(4 << 14)
> +#define VID_PIXEL_MODE_RGB121212	(5 << 14)
> +#define VID_PIXEL_MODE_YUV420		(8 << 14)
> +#define VID_PIXEL_MODE_YUV422_PACKED	(9 << 14)
> +#define VID_PIXEL_MODE_YUV422		(10 << 14)
> +#define VID_PIXEL_MODE_YUV422_24B	(11 << 14)
> +#define VID_DATATYPE(x)			((x) << 8)
> +#define VID_VIRTCHAN_ID(iface, x)	((x) << (4 + (iface) * 2))
> +#define STOP_MODE(x)			((x) << 2)
> +#define START_MODE(x)			(x)
> +
> +#define VID_VSIZE1			0xb4
> +#define VFP_LEN(x)			((x) << 12)
> +#define VBP_LEN(x)			((x) << 6)
> +#define VSA_LEN(x)			(x)
> +
> +#define VID_VSIZE2			0xb8
> +#define VACT_LEN(x)			(x)
> +
> +#define VID_HSIZE1			0xc0
> +#define HFP_LEN(x)			((x) << 20)
> +#define HBP_LEN(x)			((x) << 10)
> +#define HSA_LEN(x)			(x)
> +
> +#define VID_HSIZE2			0xc4
> +#define HACT_LEN(x)			(x)
> +
> +#define VID_BLKSIZE1			0xcc
> +#define BLK_EOL_PKT_LEN(x)		((x) << 15)
> +#define BLK_LINE_EVENT_PKT_LEN(x)	(x)
> +
> +#define VID_BLKSIZE2			0xd0
> +#define BLK_LINE_PULSE_PKT_LEN(x)	(x)
> +
> +#define VID_PKT_TIME			0xd8
> +#define BLK_EOL_DURATION(x)		(x)
> +
> +#define VID_DPHY_TIME			0xdc
> +#define REG_WAKEUP_TIME(x)		((x) << 17)
> +#define REG_LINE_DURATION(x)		(x)
> +
> +#define VID_ERR_COLOR1			0xe0
> +#define COL_GREEN(x)			((x) << 12)
> +#define COL_RED(x)			(x)
> +
> +#define VID_ERR_COLOR2			0xe4
> +#define PAD_VAL(x)			((x) << 12)
> +#define COL_BLUE(x)			(x)
> +
> +#define VID_VPOS			0xe8
> +#define LINE_VAL(val)			(((val) & GENMASK(14, 2)) >> 2)
> +#define LINE_POS(val)			((val) & GENMASK(1, 0))
> +
> +#define VID_HPOS			0xec
> +#define HORIZ_VAL(val)			(((val) & GENMASK(17, 3)) >> 3)
> +#define HORIZ_POS(val)			((val) & GENMASK(2, 0))
> +
> +#define VID_MODE_STS			0xf0
> +#define VID_MODE_STS_CTL		0x140
> +#define VID_MODE_STS_CLR		0x160
> +#define VID_MODE_STS_FLAG		0x180
> +#define VSG_RECOVERY			BIT(10)
> +#define ERR_VRS_WRONG_LEN		BIT(9)
> +#define ERR_LONG_READ			BIT(8)
> +#define ERR_LINE_WRITE			BIT(7)
> +#define ERR_BURST_WRITE			BIT(6)
> +#define ERR_SMALL_HEIGHT		BIT(5)
> +#define ERR_SMALL_LEN			BIT(4)
> +#define ERR_MISSING_VSYNC		BIT(3)
> +#define ERR_MISSING_HSYNC		BIT(2)
> +#define ERR_MISSING_DATA		BIT(1)
> +#define VSG_RUNNING			BIT(0)
> +
> +#define VID_VCA_SETTING1		0xf4
> +#define BURST_LP			BIT(16)
> +#define MAX_BURST_LIMIT(x)		(x)
> +
> +#define VID_VCA_SETTING2		0xf8
> +#define MAX_LINE_LIMIT(x)		((x) << 16)
> +#define EXACT_BURST_LIMIT(x)		(x)
> +
> +#define TVG_CTL				0xfc
> +#define TVG_STRIPE_SIZE(x)		((x) << 5)
> +#define TVG_MODE_MASK			GENMASK(4, 3)
> +#define TVG_MODE_SINGLE_COLOR		(0 << 3)
> +#define TVG_MODE_VSTRIPES		(2 << 3)
> +#define TVG_MODE_HSTRIPES		(3 << 3)
> +#define TVG_STOPMODE_MASK		GENMASK(2, 1)
> +#define TVG_STOPMODE_EOF		(0 << 1)
> +#define TVG_STOPMODE_EOL		(1 << 1)
> +#define TVG_STOPMODE_NOW		(2 << 1)
> +#define TVG_RUN				BIT(0)
> +
> +#define TVG_IMG_SIZE			0x100
> +#define TVG_NBLINES(x)			((x) << 16)
> +#define TVG_LINE_SIZE(x)		(x)
> +
> +#define TVG_COLOR1			0x104
> +#define TVG_COL1_GREEN(x)		((x) << 12)
> +#define TVG_COL1_RED(x)			(x)
> +
> +#define TVG_COLOR1_BIS			0x108
> +#define TVG_COL1_BLUE(x)		(x)
> +
> +#define TVG_COLOR2			0x10c
> +#define TVG_COL2_GREEN(x)		((x) << 12)
> +#define TVG_COL2_RED(x)			(x)
> +
> +#define TVG_COLOR2_BIS			0x110
> +#define TVG_COL2_BLUE(x)		(x)
> +
> +#define TVG_STS				0x114
> +#define TVG_STS_RUNNING			BIT(0)
> +
> +#define TBG_CTL				0x118
> +#define TBG_MODE_MASK			GENMASK(4, 3)
> +#define TBG_MODE_START_1B_STOP		(0 << 3)
> +#define TBG_MODE_START_2B_STOP		(1 << 3)
> +#define TBG_MODE_START_BURST_CNT_STOP	(2 << 3)
> +#define TBG_MODE_START_BURST_STOP	(3 << 3)
> +#define TBG_DATA_SEL			BIT(2)
> +#define TBG_HS_REQ			BIT(1)
> +#define TBG_START			BIT(0)
> +
> +#define TBG_SETTING1			0x11c
> +#define TBG_SETTING2			0x120
> +
> +#define TBG_STS				0x124
> +#define TBG_STS_RUNNING			BIT(0)
> +
> +#define TVG_TBG_STS_CTL			0x144
> +#define TVG_TBG_STS_CLR			0x164
> +#define TVG_TBG_STS_FLAG		0x184
> +#define TVG_TBG_STS_TBG_RUNNING		BIT(1)
> +#define TVG_TBG_STS_TVG_RUNNING		BIT(0)
> +
> +#define STS_CTL_EDGE(e)			((e) << 16)
> +
> +#define DPHY_LANES_MAP			0x198
> +#define DAT_REMAP_CFG(b, l)		((l) << ((b) * 8))
> +
> +#define DPI_IRQ_EN			0x1a0
> +#define DPI_IRQ_CLR			0x1a4
> +#define DPI_IRQ_STS			0x1a8
> +#define PIXEL_BUF_OVERFLOW		BIT(0)
> +
> +#define DPI_CFG				0x1ac
> +#define DPI_CFG_FIFO_LEVEL(x)		((x) & GENMASK(15, 0))
> +#define DPI_CFG_FIFO_DEPTH(x)		((x) >> 16)
> +
> +#define DPHY_CFG0			0x1b0
> +#define DPHY_C_RSTB			BIT(20)
> +#define DPHY_D_RSTB(x)			((x) << 16)
> +#define DPHY_TIF_FORCE_WRITE		BIT(12)
> +#define DPHY_PLL_PDN			BIT(10)
> +#define DPHY_CMN_PDN			BIT(9)
> +#define DPHY_C_PDN			BIT(8)
> +#define DPHY_D_PDN(x)			((x) << 4)
> +#define DPHY_PLL_PSO			BIT(1)
> +#define DPHY_CMN_PSO			BIT(0)
> +
> +#define DPHY_CFG1			0x1b4
> +#define PDHY_PLL_OPDIV(x)		((x) << 20)
> +#define PDHY_PLL_IPDIV(x)		((x) << 12)
> +#define PDHY_PLL_FBDIV(x)		(x)
> +
> +#define DPHY_PLL_TM_LO			0x1b8
> +#define DPHY_PLL_TM_MID			0x1bc
> +#define DPHY_PLL_TM_HI			0x1c0
> +
> +#define DPHY_STATUS			0x1c4
> +#define PPI_D_RX_ULPS_ESC(x)		((x) >> 12)
> +#define PPI_C_TX_READY_HS		BIT(8)
> +#define PPI_PLL_LOCK			BIT(7)
> +#define PPI_PLL_COARSE			BIT(6)
> +#define PPI_PLL_COARSE_CODE(x)		((x) & GENMASK(5, 0))
> +
> +#define DPHY_BIST			0x1c8
> +#define PSO_BYPASS_CTX_EN		BIT(12)
> +#define PSO_BYPASS_TX_EN(l)		BIT(8 + (l))
> +#define BIST_CTX_EN			BIT(4)
> +#define BIST_TX_EN(l)			BIT(l)
> +

Do the above DPHY registers actually configure the PHY used with the
controller, or do we need to configure any additional register outside
of this block to get things working?

I ask because they aren't used in the code below, and the DT binding
for this device specifies the PHY as a separate device. What's the
plan in the future for PHY?

> +#define TEST_GENERIC			0x1cc
> +#define TEST_STATUS(x)			((x) >> 16)
> +#define TEST_CTRL(x)			(x)
> +
> +#define ID_REG				0x1f0
> +#define REV_VENDOR_ID(x)		(((x) & GENMASK(31, 20)) >> 20)
> +#define REV_PRODUCT_ID(x)		(((x) & GENMASK(19, 12)) >> 12)
> +#define REV_HW(x)			(((x) & GENMASK(11, 8)) >> 8)
> +#define REV_MAJOR(x)			(((x) & GENMASK(7, 4)) >> 4)
> +#define REV_MINOR(x)			((x) & GENMASK(3, 0))
> +
> +#define IP_CONF				0x1f4
> +#define SP_HS_FIFO_DEPTH(x)		(((x) & GENMASK(30, 26)) >> 26)
> +#define SP_LP_FIFO_DEPTH(x)		(((x) & GENMASK(25, 21)) >> 21)
> +#define VRS_FIFO_DEPTH(x)			(((x) & GENMASK(20, 16)) >> 16)
> +#define DIRCMD_FIFO_DEPTH(x)		(((x) & GENMASK(15, 13)) >> 13)
> +#define SDI_IFACE_32			BIT(12)
> +#define INTERNAL_DATAPATH_32		(0 << 10)
> +#define INTERNAL_DATAPATH_16		(1 << 10)
> +#define INTERNAL_DATAPATH_8		(3 << 10)
> +#define INTERNAL_DATAPATH_SIZE		((x) & GENMASK(11, 10))
> +#define INTERNAL_DATAPATH_32		(0 << 10)
> +#define INTERNAL_DATAPATH_16		(1 << 10)
> +#define INTERNAL_DATAPATH_8		(3 << 10)
> +#define NUM_IFACE(x)			((((x) & GENMASK(9, 8)) >> 8) + 1)
> +#define MAX_LANE_NB(x)			(((x) & GENMASK(7, 6)) >> 6)
> +#define RX_FIFO_DEPTH(x)		((x) & GENMASK(5, 0))
> +
> +enum cdns_dsi_output_type {
> +	CDNS_DSI_PANEL = 0,
> +	CDNS_DSI_BRIDGE = 1,
> +};
> +
> +struct cdns_dsi_output {
> +	struct mipi_dsi_device *dev;
> +	struct drm_connector connector;
> +	enum cdns_dsi_output_type type;
> +	union {
> +		struct drm_panel *panel;
> +		struct drm_bridge *bridge;
> +	};
> +};
> +
> +enum cdns_dsi_input_id {
> +	CDNS_DPI_INPUT = 0,
> +};
> +
> +struct cdns_dsi_input {
> +	enum cdns_dsi_input_id id;
> +	struct drm_bridge bridge;
> +	struct cdns_dsi *dsi;
> +};
> +
> +struct cdns_dsi {
> +	struct mipi_dsi_host base;
> +	void __iomem *regs;
> +	struct cdns_dsi_input *input;
> +	struct cdns_dsi_output *output;
> +	unsigned int direct_cmd_fifo_depth;
> +	unsigned int rx_fifo_depth;
> +	struct completion direct_cmd_comp;
> +	struct clk *pclk;
> +	struct clk *sysclk;
> +};
> +
> +static inline struct cdns_dsi *to_cdns_dsi(struct mipi_dsi_host *host)
> +{
> +	return container_of(host, struct cdns_dsi, base);
> +}
> +
> +static inline struct cdns_dsi_input *
> +bridge_to_cdns_dsi_input(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct cdns_dsi_input, bridge);
> +}
> +
> +static inline struct cdns_dsi_output *
> +connector_to_cdns_dsi_output(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct cdns_dsi_output, connector);
> +}
> +
> +static const struct drm_connector_funcs cdns_dsi_panel_conn_funcs = {
> +	.dpms = drm_atomic_helper_connector_dpms,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int cdns_dsi_panel_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct cdns_dsi_output *output;
> +
> +	output = connector_to_cdns_dsi_output(connector);
> +
> +	return drm_panel_get_modes(output->panel);
> +}
> +
> +static const struct drm_connector_helper_funcs cdns_dsi_panel_conn_helper_funcs = {
> +	.get_modes = cdns_dsi_panel_connector_get_modes,
> +};
> +
> +static int cdns_dsi_output_attach_panel(struct cdns_dsi_output *output)
> +{
> +	struct cdns_dsi *dsi = to_cdns_dsi(output->dev->host);
> +	struct drm_device *drm = dsi->input->bridge.dev;
> +	int ret;
> +
> +	ret = drm_connector_init(drm, &output->connector,
> +				 &cdns_dsi_panel_conn_funcs,
> +				 DRM_MODE_CONNECTOR_DSI);
> +	if (ret)
> +		return ret;
> +
> +	drm_connector_helper_add(&output->connector,
> +				 &cdns_dsi_panel_conn_helper_funcs);
> +
> +	ret = drm_mode_connector_attach_encoder(&output->connector,
> +						dsi->input->bridge.encoder);
> +	if (ret)
> +		return ret;
> +
> +	return drm_panel_attach(output->panel, &output->connector);
> +}
> +
> +static int cdns_dsi_bridge_attach(struct drm_bridge *bridge)
> +{
> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> +	struct cdns_dsi_output *output = input->dsi->output;
> +	int ret;
> +
> +	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
> +		dev_err(input->dsi->base.dev,
> +			"cdns-dsi driver is only compatible with DRM devices supporting atomic updates");
> +		return -ENOTSUPP;
> +	}
> +
> +	switch (output->type) {
> +	case CDNS_DSI_PANEL:
> +		ret = cdns_dsi_output_attach_panel(output);
> +		break;
> +
> +	case CDNS_DSI_BRIDGE:
> +		ret = drm_bridge_attach(bridge->encoder, output->bridge,
> +					bridge);
> +		break;

Could you have a look at Eric's dsi-panel-bridge patch-set? I think it
should simplify things for this driver too.

> +
> +	default:
> +		ret = -ENOTSUPP;
> +	}
> +
> +	return ret;
> +}
> +
> +static bool cdns_dsi_bridge_mode_fixup(struct drm_bridge *bridge,
> +				       const struct drm_display_mode *mode,
> +				       struct drm_display_mode *adj)
> +{
> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> +	int bpp;
> +
> +	/*
> +	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
> +	 * least 1.
> +	 */
> +	if (adj->crtc_vtotal - adj->crtc_vsync_end < 2)
> +		return false;
> +
> +	/* VSA_DSI = VSA_DPI and must be at least 2. */
> +	if (adj->crtc_vsync_end - adj->crtc_vsync_start < 2)
> +		return false;
> +
> +	/* HACT must be a 32-bits aligned. */
> +	bpp = mipi_dsi_pixel_format_to_bpp(input->dsi->output->dev->format);
> +	if ((adj->hdisplay * bpp) % 32)
> +		return false;
> +
> +	return true;

We could consider using the new mode_valid helpers that Jose recently
added. This might be better as bridge mode_valid() op.


> +}
> +
> +static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> +	struct cdns_dsi *dsi = input->dsi;
> +	u32 val;
> +
> +	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
> +	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
> +		 DISP_EOT_GEN);
> +	writel(val, dsi->regs + MCTL_MAIN_DATA_CTL);
> +
> +	val = readl(dsi->regs + MCTL_MAIN_EN) & ~IF_EN(input->id);
> +	writel(val, dsi->regs + MCTL_MAIN_EN);
> +}
> +
> +#define DSI_HBP_FRAME_OVERHEAD		12
> +#define DSI_HSA_FRAME_OVERHEAD		14
> +#define DSI_HFP_FRAME_OVERHEAD		6
> +#define DSI_HSS_VSS_VSEFRAME_OVERHEAD	6
> +#define DSI_EOT_PKT_SIZE		4
> +
> +static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
> +{
> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> +	struct cdns_dsi *dsi = input->dsi;
> +	struct cdns_dsi_output *output = dsi->output;
> +	struct drm_display_mode *mode;
> +	u32 hsize0, hsa, hline, tmp;
> +	int bpp, nlanes;
> +
> +	mode = &bridge->encoder->crtc->state->adjusted_mode;
> +	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
> +	nlanes = output->dev->lanes;
> +
> +	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +		tmp = mode->crtc_htotal - mode->crtc_hsync_end;
> +	else
> +		tmp = mode->crtc_htotal - mode->crtc_hsync_start;
> +	tmp = (tmp * bpp) / 8;
> +	tmp = tmp < DSI_HBP_FRAME_OVERHEAD ? 0 : tmp - DSI_HBP_FRAME_OVERHEAD;
> +	hsize0 = HBP_LEN(tmp);
> +
> +	/* HFP can be disabled in burst mode only. */
> +	if ((output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_HFP) &&
> +	    (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST))
> +		tmp = 0;
> +	else
> +		tmp = mode->crtc_hsync_start - mode->crtc_hdisplay;
> +	tmp = (tmp * bpp) / 8;
> +	tmp = tmp < DSI_HFP_FRAME_OVERHEAD ? 0 : tmp - DSI_HFP_FRAME_OVERHEAD;
> +	hsize0 |= HFP_LEN(tmp);
> +
> +	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +		tmp = mode->crtc_hsync_end - mode->crtc_hsync_start;
> +	else
> +		tmp = 0;
> +	tmp = (tmp * 8) / bpp;
> +	tmp = tmp < DSI_HSA_FRAME_OVERHEAD ? 0 : tmp - DSI_HSA_FRAME_OVERHEAD;
> +	hsa = tmp;
> +	hsize0 |= HSA_LEN(tmp);
> +
> +	writel(hsize0, dsi->regs + VID_HSIZE1);
> +	writel((mode->crtc_hdisplay * bpp) / 8, dsi->regs + VID_HSIZE2);
> +
> +	writel(VBP_LEN(mode->crtc_vtotal - mode->crtc_vsync_end - 1) |
> +	       VFP_LEN(mode->crtc_vsync_start - mode->crtc_vdisplay) |
> +	       VSA_LEN(mode->crtc_vsync_end - mode->crtc_vsync_start),
> +	       dsi->regs + VID_VSIZE1);
> +	writel(mode->crtc_vdisplay, dsi->regs + VID_VSIZE2);
> +
> +	hline = (mode->crtc_htotal * bpp) / 8;
> +	tmp = hline - DSI_HSA_FRAME_OVERHEAD;
> +	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +		tmp -= hsa + DSI_HSA_FRAME_OVERHEAD;
> +
> +	writel(0, dsi->regs + VID_BLKSIZE1);
> +	writel(BLK_LINE_PULSE_PKT_LEN(tmp), dsi->regs + VID_BLKSIZE2);
> +
> +	tmp = DIV_ROUND_UP(hline, nlanes) - DIV_ROUND_UP(hsa, nlanes);
> +
> +	if (!(output->dev->mode_flags & MIPI_DSI_MODE_EOT_PACKET))
> +		tmp -= DIV_ROUND_UP(DSI_EOT_PKT_SIZE, nlanes);
> +
> +	writel(REG_WAKEUP_TIME(0x1a8) | REG_LINE_DURATION(tmp),
> +	       dsi->regs + VID_DPHY_TIME);
> +
> +	writel(0x0, dsi->regs + VID_PKT_TIME);
> +
> +	writel(MAX_BURST_LIMIT(0), dsi->regs + VID_VCA_SETTING1);
> +	writel(MAX_LINE_LIMIT(0x6ec) | EXACT_BURST_LIMIT(0),
> +	       dsi->regs + VID_VCA_SETTING2);
> +
> +	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO) {
> +		switch (output->dev->format) {
> +		case MIPI_DSI_FMT_RGB888:
> +			tmp = VID_PIXEL_MODE_RGB888 |
> +			      VID_DATATYPE(MIPI_DSI_PACKED_PIXEL_STREAM_24);
> +			break;
> +
> +		case MIPI_DSI_FMT_RGB666:
> +			tmp = VID_PIXEL_MODE_RGB666 |
> +			      VID_DATATYPE(MIPI_DSI_PIXEL_STREAM_3BYTE_18);
> +			break;
> +
> +		case MIPI_DSI_FMT_RGB666_PACKED:
> +			tmp = VID_PIXEL_MODE_RGB666_PACKED |
> +			      VID_DATATYPE(MIPI_DSI_PACKED_PIXEL_STREAM_18);
> +			break;
> +
> +		case MIPI_DSI_FMT_RGB565:
> +			tmp = VID_PIXEL_MODE_RGB666_PACKED |
> +			      VID_DATATYPE(MIPI_DSI_PACKED_PIXEL_STREAM_16);
> +			break;
> +
> +		default:
> +			dev_err(dsi->base.dev, "Unsupported DSI format\n");
> +			return;
> +		}
> +
> +		if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +			tmp |= SYNC_PULSE_ACTIVE | SYNC_PULSE_HORIZONTAL;
> +
> +		tmp |= REG_BLKLINE_MODE(1) | REG_BLKEOL_MODE(1) |
> +		       RECOVERY_MODE(1);
> +
> +		writel(tmp, dsi->regs + VID_MAIN_CTL);
> +	}
> +
> +	tmp = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
> +	tmp &= ~(IF_VID_SELECT_MASK | HOST_EOT_GEN | IF_VID_MODE);
> +
> +	if (!(output->dev->mode_flags & MIPI_DSI_MODE_EOT_PACKET))
> +		tmp |= HOST_EOT_GEN;
> +
> +	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO)
> +		tmp |= IF_VID_MODE | IF_VID_SELECT(input->id) | VID_EN;
> +
> +	writel(tmp, dsi->regs + MCTL_MAIN_DATA_CTL);
> +
> +	tmp = readl(dsi->regs + MCTL_MAIN_EN) | IF_EN(input->id);
> +	writel(tmp, dsi->regs + MCTL_MAIN_EN);
> +}
> +
> +static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
> +	.attach = cdns_dsi_bridge_attach,
> +	.mode_fixup = cdns_dsi_bridge_mode_fixup,
> +	.disable = cdns_dsi_bridge_disable,
> +	.enable = cdns_dsi_bridge_enable,
> +};
> +
> +static int cdns_dsi_init_link(struct cdns_dsi *dsi)
> +{
> +	u32 val;
> +	int i;
> +
> +	writel(CLK_UNIT_INTERVAL(16), dsi->regs + MCTL_DPHY_STATIC);
> +	writel(CLK_DIV(0xb) | HSTX_TIMEOUT(0xed8afff),
> +	       dsi->regs + MCTL_DPHY_TIMEOUT1);
> +	writel(LPRX_TIMEOUT(0xf30fffff), dsi->regs + MCTL_DPHY_TIMEOUT2);
> +
> +	val = WAIT_BURST_TIME(0xf);
> +	for (i = 1; i < dsi->output->dev->lanes; i++)
> +		val |= DATA_LANE_EN(i);
> +
> +	if (!(dsi->output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> +		val |= CLK_CONTINUOUS;
> +
> +	writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
> +
> +	writel(CLK_LANE_ULPOUT_TIME(0x105) | DATA_LANE_ULPOUT_TIME(0x1d5),
> +	       dsi->regs + MCTL_ULPOUT_TIME);
> +
> +	writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
> +
> +	val = CLK_LANE_EN | PLL_START;
> +	for (i = 0; i < dsi->output->dev->lanes; i++)
> +		val |= DATA_LANE_START(i);
> +
> +	writel(val, dsi->regs + MCTL_MAIN_EN);
> +
> +	ndelay(100);
> +
> +	return 0;
> +}
> +
> +static int cdns_dsi_attach(struct mipi_dsi_host *host,
> +			   struct mipi_dsi_device *dev)
> +{
> +	struct cdns_dsi *dsi = to_cdns_dsi(host);
> +	struct cdns_dsi_output *output;
> +	int ret;
> +
> +	/* TODO: support multi-devices setup? */
> +	if (dsi->output)
> +		return -EBUSY;
> +
> +	output = devm_kzalloc(host->dev, sizeof(*output), GFP_KERNEL);
> +	if (!output)
> +		return -ENOMEM;
> +
> +	output->dev = dev;
> +
> +	output->panel = of_drm_find_panel(dev->dev.of_node);
> +	if (output->panel) {
> +		output->type = CDNS_DSI_PANEL;
> +	} else {
> +		output->bridge = of_drm_find_bridge(dev->dev.of_node);
> +		if (!output->bridge) {
> +			dev_err(host->dev,
> +				"%s is neither a panel nor a bridge",
> +				dev->name);
> +			return -ENOTSUPP;
> +		}
> +
> +		output->type = CDNS_DSI_BRIDGE;
> +		dsi->input->bridge.next = output->bridge;
> +	}
> +
> +	dsi->output = output;
> +
> +	ret = cdns_dsi_init_link(dsi);
> +	if (ret)
> +		return ret;
> +
> +	/* FIXME: should be moved somewhere else. */
> +	return drm_bridge_add(&dsi->input->bridge);

In the driver probe?

> +}
> +
> +static int cdns_dsi_detach(struct mipi_dsi_host *host,
> +			   struct mipi_dsi_device *dev)
> +{
> +	struct cdns_dsi *dsi = to_cdns_dsi(host);
> +
> +	writel(0, dsi->regs + MCTL_MAIN_EN);
> +	writel(0, dsi->regs + MCTL_MAIN_DATA_CTL);
> +	writel(0, dsi->regs + MCTL_MAIN_PHY_CTL);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
> +{
> +	struct cdns_dsi *dsi = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 flag, ctl;
> +
> +	flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
> +	if (flag) {
> +		ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
> +		ctl &= ~flag;
> +		writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);
> +		complete(&dsi->direct_cmd_comp);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
> +				 const struct mipi_dsi_msg *msg)
> +{
> +	struct cdns_dsi *dsi = to_cdns_dsi(host);
> +	u32 cmd, sts, val, wait = WRITE_COMPLETED, ctl = 0;
> +	struct mipi_dsi_packet packet;
> +	int ret, i, tx_len, rx_len;
> +
> +	ret = mipi_dsi_create_packet(&packet, msg);
> +	if (ret)
> +		return ret;
> +
> +	tx_len = msg->tx_buf ? msg->tx_len : 0;
> +	rx_len = msg->rx_buf ? msg->rx_len : 0;
> +
> +	/* For read operations, the maximum TX len is 2. */
> +	if (rx_len && tx_len > 2)
> +		return -ENOTSUPP;
> +
> +	/* TX len is limited by the CMD FIFO depth. */
> +	if (tx_len > dsi->direct_cmd_fifo_depth)
> +		return -ENOTSUPP;
> +
> +	/* RX len is limited by the RX FIFO depth. */
> +	if (rx_len > dsi->rx_fifo_depth)
> +		return -ENOTSUPP;
> +
> +	cmd = CMD_SIZE(tx_len) | CMD_VCHAN_ID(msg->channel) |
> +	      CMD_DATATYPE(msg->type);
> +
> +	if (msg->flags & MIPI_DSI_MSG_USE_LPM)
> +		cmd |= CMD_LP_EN;
> +
> +	if (mipi_dsi_packet_format_is_long(msg->type))
> +		cmd |= CMD_LONG;
> +
> +	if (rx_len) {
> +		cmd |= READ_CMD;
> +		wait = READ_COMPLETED_WITH_ERR | READ_COMPLETED;
> +		ctl = READ_EN | BTA_EN;
> +	} else if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
> +		cmd |= BTA_REQ;
> +		wait = ACK_WITH_ERR_RCVD | ACK_RCVD;
> +		ctl = BTA_EN;
> +	}
> +
> +	writel(readl(dsi->regs + MCTL_MAIN_DATA_CTL) | ctl,
> +	       dsi->regs + MCTL_MAIN_DATA_CTL);
> +
> +	writel(cmd, dsi->regs + DIRECT_CMD_MAIN_SETTINGS);
> +
> +	for (i = 0; i < tx_len; i += 4) {
> +		const u8 *buf = msg->tx_buf;
> +		int j;
> +
> +		val = 0;
> +		for (j = 0; j < 4 && j + i < tx_len; j++)
> +			val |= (u32)buf[i + j] << (8 * j);
> +
> +		writel(val, dsi->regs + DIRECT_CMD_WRDATA);
> +	}
> +
> +	/* Clear status flags before sending the command. */
> +	writel(wait, dsi->regs + DIRECT_CMD_STS_CLR);
> +	writel(wait, dsi->regs + DIRECT_CMD_STS_CTL);
> +	reinit_completion(&dsi->direct_cmd_comp);
> +	writel(0, dsi->regs + DIRECT_CMD_SEND);
> +
> +	wait_for_completion_timeout(&dsi->direct_cmd_comp,
> +				    msecs_to_jiffies(1000));
> +
> +	sts = readl(dsi->regs + DIRECT_CMD_STS);
> +	writel(wait, dsi->regs + DIRECT_CMD_STS_CLR);
> +	writel(0, dsi->regs + DIRECT_CMD_STS_CTL);
> +
> +	writel(readl(dsi->regs + MCTL_MAIN_DATA_CTL) & ~ctl,
> +	       dsi->regs + MCTL_MAIN_DATA_CTL);
> +
> +	/* We did not receive the events we were waiting for. */
> +	if (!(sts & wait))
> +		return -ETIMEDOUT;
> +
> +	/* READ of WRITE with ACK failed. */
> +	if (sts & (READ_COMPLETED_WITH_ERR | ACK_WITH_ERR_RCVD))
> +		return -EIO;
> +
> +	for (i = 0; i < rx_len; i += 4) {
> +		u8 *buf = msg->rx_buf;
> +		int j;
> +
> +		val = readl(dsi->regs + DIRECT_CMD_RDDATA);
> +		for (j = 0; j < 4 && j + i < rx_len; j++)
> +			buf[i + j] = val >> (8 * j);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops cdns_dsi_ops = {
> +	.attach = cdns_dsi_attach,
> +	.detach = cdns_dsi_detach,
> +	.transfer = cdns_dsi_transfer,
> +};
> +
> +static int cdns_dsi_drm_probe(struct platform_device *pdev)

I wanted to bring up the point I made in the DT patch too: Is it
more suitable to implement this is a library with bind/unbind, and
probe/remove helpers like it's done for dw-hdmi. Could there be SoCs
that integrate this IP but require some additional wrapper
configurations to make things work?

Looks good otherwise.

Thanks,
Archit

> +{
> +	struct cdns_dsi *dsi;
> +	struct cdns_dsi_input *input;
> +	struct resource *res;
> +	int ret, irq;
> +	u32 val;
> +
> +	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dsi->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(dsi->regs))
> +		return PTR_ERR(dsi->regs);
> +
> +	dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(dsi->pclk))
> +		return PTR_ERR(dsi->pclk);
> +
> +	dsi->sysclk = devm_clk_get(&pdev->dev, "sysclk");
> +	if (IS_ERR(dsi->sysclk))
> +		return PTR_ERR(dsi->sysclk);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = clk_prepare_enable(dsi->pclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(dsi->sysclk);
> +	if (ret)
> +		goto err_dis_pclk;
> +
> +	val = readl(dsi->regs + ID_REG);
> +	if (REV_VENDOR_ID(val) != 0xcad) {
> +		dev_err(&pdev->dev, "invalid vendor id\n");
> +		return -EINVAL;
> +	}
> +
> +	val = readl(dsi->regs + IP_CONF);
> +	dsi->direct_cmd_fifo_depth = 1 << (DIRCMD_FIFO_DEPTH(val) + 2);
> +	dsi->rx_fifo_depth = RX_FIFO_DEPTH(val);
> +	init_completion(&dsi->direct_cmd_comp);
> +
> +	writel(0, dsi->regs + MCTL_MAIN_DATA_CTL);
> +	writel(0, dsi->regs + MCTL_MAIN_EN);
> +	writel(0, dsi->regs + MCTL_MAIN_PHY_CTL);
> +
> +	input = devm_kzalloc(&pdev->dev, sizeof(*input), GFP_KERNEL);
> +	if (!input) {
> +		ret = -ENOMEM;
> +		goto err_dis_sysclk;
> +	}
> +
> +	input->dsi = dsi;
> +	dsi->input = input;
> +
> +	/* Mask all interrupts before registering the IRQ handler. */
> +	writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
> +	writel(0, dsi->regs + MCTL_DPHY_ERR_CTL1);
> +	writel(0, dsi->regs + CMD_MODE_STS_CTL);
> +	writel(0, dsi->regs + DIRECT_CMD_STS_CTL);
> +	writel(0, dsi->regs + DIRECT_CMD_RD_STS_CTL);
> +	writel(0, dsi->regs + VID_MODE_STS_CTL);
> +	writel(0, dsi->regs + TVG_TBG_STS_CTL);
> +	writel(0, dsi->regs + DPI_IRQ_EN);
> +	ret = devm_request_irq(&pdev->dev, irq, cdns_dsi_interrupt, 0,
> +			       dev_name(&pdev->dev), dsi);
> +	if (ret)
> +		goto err_dis_sysclk;
> +
> +	dsi->base.dev = &pdev->dev;
> +	dsi->base.ops = &cdns_dsi_ops;
> +
> +	ret = mipi_dsi_host_register(&dsi->base);
> +	if (ret)
> +		goto err_dis_sysclk;
> +
> +	input->bridge.funcs = &cdns_dsi_bridge_funcs;
> +	input->bridge.of_node = pdev->dev.of_node;
> +
> +	platform_set_drvdata(pdev, dsi);
> +
> +	return 0;
> +
> +err_dis_sysclk:
> +	clk_disable_unprepare(dsi->sysclk);
> +
> +err_dis_pclk:
> +	clk_disable_unprepare(dsi->pclk);
> +
> +	return ret;
> +}
> +
> +static int cdns_dsi_drm_remove(struct platform_device *pdev)
> +{
> +	struct cdns_dsi *dsi = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&dsi->input->bridge);
> +	mipi_dsi_host_unregister(&dsi->base);
> +	clk_disable_unprepare(dsi->sysclk);
> +	clk_disable_unprepare(dsi->pclk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id cdns_dsi_of_match[] = {
> +	{ .compatible = "cdns,dsi" },
> +	{ },
> +};
> +
> +static struct platform_driver cdns_dsi_platform_driver = {
> +	.probe  = cdns_dsi_drm_probe,
> +	.remove = cdns_dsi_drm_remove,
> +	.driver = {
> +		.name   = "cdns-dsi",
> +		.of_match_table = cdns_dsi_of_match,
> +	},
> +};
> +module_platform_driver(cdns_dsi_platform_driver);
> +
> +MODULE_AUTHOR("Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
> +MODULE_DESCRIPTION("Cadence DSI driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:cdns-dsi");
> +
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Boris Brezillon]

On Sat, 3 Jun 2017 23:43:17 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> Hi,
> 
> On 06/02/2017 05:34 PM, Boris Brezillon wrote:
> > Document the bindings used for the Cadence DSI bridge.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > new file mode 100644
> > index 000000000000..770c5c5b1e93
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > @@ -0,0 +1,55 @@
> > +Cadence DSI bridge
> > +==================
> > +
> > +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.  
> 
> Is this a separate chip, or an IP integrated into SoCs?

It's supposed to be integrated into SoCs.

> If it's the 
> latter, I don't think DPI on the its input side is the right term to 
> use. Maybe RGB would be more appropriate here.

Well, the datasheet explicitly mentions DPI, and you can also send
pixels in YUV422 and YUV420 format on this bus, so I don't think RGB is
appropriate, but if you really want me to use RGB I can change that.

BTW, can you detail why DPI is not appropriate for internal parallel
busses. I don't understand why it makes a difference when the bus is exposed
through external pins.

> 
> > +
> > +Required properties:
> > +- compatible: should be set to "cdns,dsi".  
> 
> Would it be better to take a dw-hdmi like approach here? I.e, the
> binding should be specific to the SoC that integrates this DSI
> bridge?

Hm, I was considering something slightly different: adding new compat
strings to the DSI bridge driver and keep the interface to the display
controller drivers as simple as possible to avoid duplicating the glue
used to bind the component in all display controller drivers embedding
this bridge.

Note that right now we don't have any SoC embedding this IP. It has
been tested on an FPGA with a very basic display controller (designed
for testing purpose only). By exposing this IP as a simple bridge, we
can easily attach it to any kind of display controller, and if we ever
need to use the component framework, then it should be pretty easy to
add support for this feature as a follow-up patch.

> 
> > +- reg: physical base address and length of the controller's registers.
> > +- interrupts: interrupt line connected to the DSI bridge.
> > +- clocks: DSI bridge clocks.
> > +- clock-names: must contain "pclk" and "sysclk".
> > +- phys: phandle link to the MIPI D-PHY controller.
> > +- phy-names: must contain "dphy".
> > +- #address-cells: must be set to 1.
> > +- #size-cells: must be set to 0.
> > +
> > +Required subnodes:
> > +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> > +  Currently contains a single input port at address 0 representing the DPI
> > +  input. Other ports will be added later to support the SDI inputs.
> > +  Port 0 should be connected to a DPI encoder output.  
> 
> The output of the DSI bridge may be another bridge, which could be i2c
> controlled. In that case, it won't be a child of the DSI bridge. For
> such scenarios, we might want to define an output port for the bridge.

Hm, okay. IIRC, this is something you mentioned when I asked how to
describe input/output ports for a DSI bridge a while ago.

I'm still not sure how the links between input and output endpoint are
supposed to be described.

For example, if you take the case where you have the DSI device
directly described under the DSI host controller, should I create
another node for this output port? Something like the following?

	dsi@xxx {
		#address-cells = <1>;
		#size-cells = <0>;

		ports {
			#address-cells = <1>;
			#size-cells = <0>;
			dpi_in: port@0 {
				reg = <0>;
				#address-cells = <1>;
				#size-cells = <0>;

				endpoint@0 {
					remote-endpoint = <&dpi_out>;
				};
			};

			dsi_out0: port@1 {
				reg = <1>;
				#address-cells = <1>;
				#size-cells = <0>;

				dsi_out0: endpoint@0 {
					remote-endpoint = <&dsi_panel0_in>;
				};
			};

			dsi_out0: port@2 {
				reg = <2>;
				#address-cells = <1>;
				#size-cells = <0>;

				dsi_out1: endpoint@0 {
					remote-endpoint = <&dsi_panel1_in>;
				};
			};
		};

		panel@0 {
			compatible = "...";
			reg = <0>;
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				#address-cells = <1>;
		                #size-cells = <0>;
				reg = <0>;

				dsi_panel0_in: endpoint@0 {
					remote-endpoint = <&dsi_out0>;
				};
			};
		};

		panel@1 {
			compatible = "...";
			reg = <1>;
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				#address-cells = <1>;
		                #size-cells = <0>;
				reg = <0>;

				dsi_panel1_in: endpoint@0 {
					remote-endpoint = <&dsi_out1>;
				};
			};
		};
	};

Thanks for the review,

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

Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

Hi Archit,

On Sun, 4 Jun 2017 00:20:06 +0530
Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:


> > +
> > +#define DPHY_CFG0			0x1b0
> > +#define DPHY_C_RSTB			BIT(20)
> > +#define DPHY_D_RSTB(x)			((x) << 16)
> > +#define DPHY_TIF_FORCE_WRITE		BIT(12)
> > +#define DPHY_PLL_PDN			BIT(10)
> > +#define DPHY_CMN_PDN			BIT(9)
> > +#define DPHY_C_PDN			BIT(8)
> > +#define DPHY_D_PDN(x)			((x) << 4)
> > +#define DPHY_PLL_PSO			BIT(1)
> > +#define DPHY_CMN_PSO			BIT(0)
> > +
> > +#define DPHY_CFG1			0x1b4
> > +#define PDHY_PLL_OPDIV(x)		((x) << 20)
> > +#define PDHY_PLL_IPDIV(x)		((x) << 12)
> > +#define PDHY_PLL_FBDIV(x)		(x)
> > +
> > +#define DPHY_PLL_TM_LO			0x1b8
> > +#define DPHY_PLL_TM_MID			0x1bc
> > +#define DPHY_PLL_TM_HI			0x1c0
> > +
> > +#define DPHY_STATUS			0x1c4
> > +#define PPI_D_RX_ULPS_ESC(x)		((x) >> 12)
> > +#define PPI_C_TX_READY_HS		BIT(8)
> > +#define PPI_PLL_LOCK			BIT(7)
> > +#define PPI_PLL_COARSE			BIT(6)
> > +#define PPI_PLL_COARSE_CODE(x)		((x) & GENMASK(5, 0))
> > +
> > +#define DPHY_BIST			0x1c8
> > +#define PSO_BYPASS_CTX_EN		BIT(12)
> > +#define PSO_BYPASS_TX_EN(l)		BIT(8 + (l))
> > +#define BIST_CTX_EN			BIT(4)
> > +#define BIST_TX_EN(l)			BIT(l)
> > +  
> 
> Do the above DPHY registers actually configure the PHY used with the
> controller, or do we need to configure any additional register outside
> of this block to get things working?

The DPHY has a separate I/O mem range with its own interface. I didn't
provide support for this part yet because the interface is not stable
yet.

> 
> I ask because they aren't used in the code below, and the DT binding
> for this device specifies the PHY as a separate device. What's the
> plan in the future for PHY?

The short-term plan is to add support for this DPHY in the cdns-dsi
driver. The driver will just retrieve the I/O mem range and interrupt
attached to the cdns DPHY block by following the phandle and using
of helpers to do the conversion, and then use these resources
internally.

The mid/long-term plan is to add a dphy framework (on top of the PHY
framework) to let dphy providers expose their features in a generic
manner.
There are 2 pros to this generic DPHY framework/layer:
- you could possibly use DPHY and DSI blocks provided by 2 different
  vendors (not sure how feasible this is in practice)
- CSI can also use DPHY as its PHY layer, so DPHY drivers could be
  shared between V4L and DRM drivers

Note that I can't promise the mid/long-term goals are achievable,
because my knowledge on DPHY is quite limited, but designing the DT
bindings to handle this use case is really important, because of the DT
stable ABI thing. 


> > +
> > +static int cdns_dsi_bridge_attach(struct drm_bridge *bridge)
> > +{
> > +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> > +	struct cdns_dsi_output *output = input->dsi->output;
> > +	int ret;
> > +
> > +	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
> > +		dev_err(input->dsi->base.dev,
> > +			"cdns-dsi driver is only compatible with DRM devices supporting atomic updates");
> > +		return -ENOTSUPP;
> > +	}
> > +
> > +	switch (output->type) {
> > +	case CDNS_DSI_PANEL:
> > +		ret = cdns_dsi_output_attach_panel(output);
> > +		break;
> > +
> > +	case CDNS_DSI_BRIDGE:
> > +		ret = drm_bridge_attach(bridge->encoder, output->bridge,
> > +					bridge);
> > +		break;  
> 
> Could you have a look at Eric's dsi-panel-bridge patch-set? I think it
> should simplify things for this driver too.

Yep, that was planned. Was just waiting for this feature to reach
drm-misc-next (which seems to be the case ;-)).

> 
> > +
> > +	default:
> > +		ret = -ENOTSUPP;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static bool cdns_dsi_bridge_mode_fixup(struct drm_bridge *bridge,
> > +				       const struct drm_display_mode *mode,
> > +				       struct drm_display_mode *adj)
> > +{
> > +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> > +	int bpp;
> > +
> > +	/*
> > +	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
> > +	 * least 1.
> > +	 */
> > +	if (adj->crtc_vtotal - adj->crtc_vsync_end < 2)
> > +		return false;
> > +
> > +	/* VSA_DSI = VSA_DPI and must be at least 2. */
> > +	if (adj->crtc_vsync_end - adj->crtc_vsync_start < 2)
> > +		return false;
> > +
> > +	/* HACT must be a 32-bits aligned. */
> > +	bpp = mipi_dsi_pixel_format_to_bpp(input->dsi->output->dev->format);
> > +	if ((adj->hdisplay * bpp) % 32)
> > +		return false;
> > +
> > +	return true;  
> 
> We could consider using the new mode_valid helpers that Jose recently
> added. This might be better as bridge mode_valid() op.

Ditto. It was on my TODO list. I'll address that in my v3.

> 
> 
> > +}

[...]

> > +
> > +static int cdns_dsi_attach(struct mipi_dsi_host *host,
> > +			   struct mipi_dsi_device *dev)
> > +{
> > +	struct cdns_dsi *dsi = to_cdns_dsi(host);
> > +	struct cdns_dsi_output *output;
> > +	int ret;
> > +
> > +	/* TODO: support multi-devices setup? */
> > +	if (dsi->output)
> > +		return -EBUSY;
> > +
> > +	output = devm_kzalloc(host->dev, sizeof(*output), GFP_KERNEL);
> > +	if (!output)
> > +		return -ENOMEM;
> > +
> > +	output->dev = dev;
> > +
> > +	output->panel = of_drm_find_panel(dev->dev.of_node);
> > +	if (output->panel) {
> > +		output->type = CDNS_DSI_PANEL;
> > +	} else {
> > +		output->bridge = of_drm_find_bridge(dev->dev.of_node);
> > +		if (!output->bridge) {
> > +			dev_err(host->dev,
> > +				"%s is neither a panel nor a bridge",
> > +				dev->name);
> > +			return -ENOTSUPP;
> > +		}
> > +
> > +		output->type = CDNS_DSI_BRIDGE;
> > +		dsi->input->bridge.next = output->bridge;
> > +	}
> > +
> > +	dsi->output = output;
> > +
> > +	ret = cdns_dsi_init_link(dsi);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* FIXME: should be moved somewhere else. */
> > +	return drm_bridge_add(&dsi->input->bridge);  
> 
> In the driver probe?

I assumed that not everything was in place at probe time, but maybe I
was wrong. This should be good, as long as all DSI devices have been
instantiated and attached after calling mipi_dsi_host_register(). I
guess this is something we can ensure by using the proposed bindings
(with all DSI output ports described in the DT) and returning
EPROBE_DEFER if at least one of the DSI device is missing at probe time.

> 
> > +}
> > +

[...]

> > +
> > +static int cdns_dsi_drm_probe(struct platform_device *pdev)  
> 
> I wanted to bring up the point I made in the DT patch too: Is it
> more suitable to implement this is a library with bind/unbind, and
> probe/remove helpers like it's done for dw-hdmi. Could there be SoCs
> that integrate this IP but require some additional wrapper
> configurations to make things work?

As answered in my previous reply, yes it can be done this way, but is it
worth introducing this infrastructure right now? Note that we can still
overload the compatible to support SoC specific integration of this IP
directly in this driver.
For the rest, it should be pretty transparent to connect a DPI encoder
to this DSI bridge.

Right now, we have an easy way to connect a DPI encoder to this DSI
bridge. The only thing that could be missing is the pixel format
negotiation (the format transiting on the DPI bus), and this is a
runtime thing, so maybe we can extend the drm_bridge interface to allow
such negotiation.

Note that this is a need I had on atmel platforms as well, because the
DPI bus can be connected to several devices (panels or external
bridges), and, depending on the device we are enabling in the pipeline,
we have to reconfigure the DPI encoder to send the pixel in the
appropriate format.

> 
> Looks good otherwise.

Thanks for your review.

Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1674 bytes --]

On 02/06/17 15:04, Boris Brezillon wrote:
> Document the bindings used for the Cadence DSI bridge.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> new file mode 100644
> index 000000000000..770c5c5b1e93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> @@ -0,0 +1,55 @@
> +Cadence DSI bridge
> +==================
> +
> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
> +
> +Required properties:
> +- compatible: should be set to "cdns,dsi".
> +- reg: physical base address and length of the controller's registers.
> +- interrupts: interrupt line connected to the DSI bridge.
> +- clocks: DSI bridge clocks.
> +- clock-names: must contain "pclk" and "sysclk".

clock-names doesn't match the example below.

> +- phys: phandle link to the MIPI D-PHY controller.
> +- phy-names: must contain "dphy".
> +- #address-cells: must be set to 1.
> +- #size-cells: must be set to 0.
> +
> +Required subnodes:
> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> +  Currently contains a single input port at address 0 representing the DPI
> +  input. Other ports will be added later to support the SDI inputs.

Typo with "SDI".

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Boris Brezillon]

Hi Tomi,

On Tue, 6 Jun 2017 15:30:26 +0300
Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:

> On 02/06/17 15:04, Boris Brezillon wrote:
> > Document the bindings used for the Cadence DSI bridge.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > new file mode 100644
> > index 000000000000..770c5c5b1e93
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > @@ -0,0 +1,55 @@
> > +Cadence DSI bridge
> > +==================
> > +
> > +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
> > +
> > +Required properties:
> > +- compatible: should be set to "cdns,dsi".
> > +- reg: physical base address and length of the controller's registers.
> > +- interrupts: interrupt line connected to the DSI bridge.
> > +- clocks: DSI bridge clocks.
> > +- clock-names: must contain "pclk" and "sysclk".  
> 
> clock-names doesn't match the example below.

Indeed. I'll fix the example.

> 
> > +- phys: phandle link to the MIPI D-PHY controller.
> > +- phy-names: must contain "dphy".
> > +- #address-cells: must be set to 1.
> > +- #size-cells: must be set to 0.
> > +
> > +Required subnodes:
> > +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> > +  Currently contains a single input port at address 0 representing the DPI
> > +  input. Other ports will be added later to support the SDI inputs.  
> 
> Typo with "SDI".

No, the 2nd and 3rd input ports are really called SDI. Here
is the datasheet description:

"
SDI: 
Serial Display Interface - this is the name of the block that is built
to interface the Display application processor to the DSI link. This is
a proprietary interface.
"

Thanks,

Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 5013 bytes --]

On 06/06/17 12:35, Boris Brezillon wrote:
> On Sat, 3 Jun 2017 23:43:17 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
> 
>> Hi,
>>
>> On 06/02/2017 05:34 PM, Boris Brezillon wrote:
>>> Document the bindings used for the Cadence DSI bridge.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>>> ---
>>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>>>  1 file changed, 55 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> new file mode 100644
>>> index 000000000000..770c5c5b1e93
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> @@ -0,0 +1,55 @@
>>> +Cadence DSI bridge
>>> +==================
>>> +
>>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.  
>>
>> Is this a separate chip, or an IP integrated into SoCs?
> 
> It's supposed to be integrated into SoCs.
> 
>> If it's the 
>> latter, I don't think DPI on the its input side is the right term to 
>> use. Maybe RGB would be more appropriate here.
> 
> Well, the datasheet explicitly mentions DPI, and you can also send
> pixels in YUV422 and YUV420 format on this bus, so I don't think RGB is
> appropriate, but if you really want me to use RGB I can change that.
> 
> BTW, can you detail why DPI is not appropriate for internal parallel
> busses. I don't understand why it makes a difference when the bus is exposed
> through external pins.

I think MIPI DPI is fine, if it is indeed MIPI DPI. But mot all parallel
video busses are MIPI DPI.

>>> +Required subnodes:
>>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
>>> +  Currently contains a single input port at address 0 representing the DPI
>>> +  input. Other ports will be added later to support the SDI inputs.
>>> +  Port 0 should be connected to a DPI encoder output.  
>>
>> The output of the DSI bridge may be another bridge, which could be i2c
>> controlled. In that case, it won't be a child of the DSI bridge. For
>> such scenarios, we might want to define an output port for the bridge.
> 
> Hm, okay. IIRC, this is something you mentioned when I asked how to
> describe input/output ports for a DSI bridge a while ago.
> 
> I'm still not sure how the links between input and output endpoint are
> supposed to be described.
> 
> For example, if you take the case where you have the DSI device
> directly described under the DSI host controller, should I create
> another node for this output port? Something like the following?
> 
> 	dsi@xxx {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			dpi_in: port@0 {
> 				reg = <0>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				endpoint@0 {
> 					remote-endpoint = <&dpi_out>;
> 				};
> 			};
> 
> 			dsi_out0: port@1 {
> 				reg = <1>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				dsi_out0: endpoint@0 {
> 					remote-endpoint = <&dsi_panel0_in>;
> 				};
> 			};
> 
> 			dsi_out0: port@2 {
> 				reg = <2>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				dsi_out1: endpoint@0 {
> 					remote-endpoint = <&dsi_panel1_in>;
> 				};
> 			};
> 		};
> 
> 		panel@0 {
> 			compatible = "...";
> 			reg = <0>;
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				#address-cells = <1>;
> 		                #size-cells = <0>;
> 				reg = <0>;
> 
> 				dsi_panel0_in: endpoint@0 {
> 					remote-endpoint = <&dsi_out0>;
> 				};
> 			};
> 		};
> 
> 		panel@1 {
> 			compatible = "...";
> 			reg = <1>;
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				#address-cells = <1>;
> 		                #size-cells = <0>;
> 				reg = <0>;
> 
> 				dsi_panel1_in: endpoint@0 {
> 					remote-endpoint = <&dsi_out1>;
> 				};
> 			};
> 		};
> 	};
> 

The ports & endpoints describe the video path, and the node child-parent
relationship describe the control path. And "port" is a physical
connector of some sort, and endpoint is a virtual channel or such.

So, you can have DSI peripherals which are either children of the DSI
bridge, and can be controlled with DSI commands. Or, you can have, say,
i2c peripherals, defined under an i2c node, which just take the video
stream from the DSI bridge. Both would have similar ports & endpoints,
but the DT nodes would be located under different parents.

Also, you can't have two output ports unless the DSI bridge has actually
multiple output pins. If the two panels are connected to the same DSI
pins, and the DSI virtual channel is used to direct the output to the
correct panel, then these should be endpoints.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Boris Brezillon]

On Tue, 6 Jun 2017 15:40:25 +0300
Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:

> On 06/06/17 12:35, Boris Brezillon wrote:
> > On Sat, 3 Jun 2017 23:43:17 +0530
> > Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> >   
> >> Hi,
> >>
> >> On 06/02/2017 05:34 PM, Boris Brezillon wrote:  
> >>> Document the bindings used for the Cadence DSI bridge.
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >>> ---
> >>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >>>  1 file changed, 55 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> new file mode 100644
> >>> index 000000000000..770c5c5b1e93
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> @@ -0,0 +1,55 @@
> >>> +Cadence DSI bridge
> >>> +==================
> >>> +
> >>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.    
> >>
> >> Is this a separate chip, or an IP integrated into SoCs?  
> > 
> > It's supposed to be integrated into SoCs.
> >   
> >> If it's the 
> >> latter, I don't think DPI on the its input side is the right term to 
> >> use. Maybe RGB would be more appropriate here.  
> > 
> > Well, the datasheet explicitly mentions DPI, and you can also send
> > pixels in YUV422 and YUV420 format on this bus, so I don't think RGB is
> > appropriate, but if you really want me to use RGB I can change that.
> > 
> > BTW, can you detail why DPI is not appropriate for internal parallel
> > busses. I don't understand why it makes a difference when the bus is exposed
> > through external pins.  
> 
> I think MIPI DPI is fine, if it is indeed MIPI DPI. But mot all parallel
> video busses are MIPI DPI.
> 
> >>> +Required subnodes:
> >>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> >>> +  Currently contains a single input port at address 0 representing the DPI
> >>> +  input. Other ports will be added later to support the SDI inputs.
> >>> +  Port 0 should be connected to a DPI encoder output.    
> >>
> >> The output of the DSI bridge may be another bridge, which could be i2c
> >> controlled. In that case, it won't be a child of the DSI bridge. For
> >> such scenarios, we might want to define an output port for the bridge.  
> > 
> > Hm, okay. IIRC, this is something you mentioned when I asked how to
> > describe input/output ports for a DSI bridge a while ago.
> > 
> > I'm still not sure how the links between input and output endpoint are
> > supposed to be described.
> > 
> > For example, if you take the case where you have the DSI device
> > directly described under the DSI host controller, should I create
> > another node for this output port? Something like the following?
> > 
> > 	dsi@xxx {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		ports {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			dpi_in: port@0 {
> > 				reg = <0>;
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				endpoint@0 {
> > 					remote-endpoint = <&dpi_out>;
> > 				};
> > 			};
> > 
> > 			dsi_out0: port@1 {
> > 				reg = <1>;
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				dsi_out0: endpoint@0 {
> > 					remote-endpoint = <&dsi_panel0_in>;
> > 				};
> > 			};
> > 
> > 			dsi_out0: port@2 {
> > 				reg = <2>;
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				dsi_out1: endpoint@0 {
> > 					remote-endpoint = <&dsi_panel1_in>;
> > 				};
> > 			};
> > 		};
> > 
> > 		panel@0 {
> > 			compatible = "...";
> > 			reg = <0>;
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			port@0 {
> > 				#address-cells = <1>;
> > 		                #size-cells = <0>;
> > 				reg = <0>;
> > 
> > 				dsi_panel0_in: endpoint@0 {
> > 					remote-endpoint = <&dsi_out0>;
> > 				};
> > 			};
> > 		};
> > 
> > 		panel@1 {
> > 			compatible = "...";
> > 			reg = <1>;
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			port@0 {
> > 				#address-cells = <1>;
> > 		                #size-cells = <0>;
> > 				reg = <0>;
> > 
> > 				dsi_panel1_in: endpoint@0 {
> > 					remote-endpoint = <&dsi_out1>;
> > 				};
> > 			};
> > 		};
> > 	};
> >   
> 
> The ports & endpoints describe the video path, and the node child-parent
> relationship describe the control path. And "port" is a physical
> connector of some sort, and endpoint is a virtual channel or such.
> 
> So, you can have DSI peripherals which are either children of the DSI
> bridge, and can be controlled with DSI commands. Or, you can have, say,
> i2c peripherals, defined under an i2c node, which just take the video
> stream from the DSI bridge. Both would have similar ports & endpoints,
> but the DT nodes would be located under different parents.
> 
> Also, you can't have two output ports unless the DSI bridge has actually
> multiple output pins. If the two panels are connected to the same DSI
> pins, and the DSI virtual channel is used to direct the output to the
> correct panel, then these should be endpoints.

Okay. Thanks for the clarification. Can you confirm that this version
is correct?

 	dsi@xxx {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
 		ports {
 			#address-cells = <1>;
 			#size-cells = <0>;
			dpi_in: port@0 {
 				reg = <0>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
 				endpoint@0 {
 					remote-endpoint = <&dpi_out>;
 				};
 			};
 
 			dsi_out: port@1 {
 				reg = <1>;
				#address-cells = <1>;
 				#size-cells = <0>;
 
 				dsi_out_vc0: endpoint@0 {
					reg = <0>;
 					remote-endpoint = <&dsi_panel0_in>;
				};

 				dsi_out_vc1: endpoint@1 {
					reg = <1>;
 					remote-endpoint = <&dsi_panel1_in>;
 				};
 			};
 		};
 
 		panel@0 {
 			compatible = "...";
 			reg = <0>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
 			port@0 {
 				#address-cells = <1>;
 		                #size-cells = <0>;
 				reg = <0>;
 
 				dsi_panel0_in: endpoint@0 {
					reg = <0>;
 					remote-endpoint = <&dsi_out_vc0>;
 				};
 			};
 		};
 
 		panel@1 {
 			compatible = "...";
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
 			port@0 {
 				#address-cells = <1>;
 		                #size-cells = <0>;
 				reg = <0>;
 
 				dsi_panel1_in: endpoint@0 {
					reg = <0>;
 					remote-endpoint = <&dsi_out_vc1>;
 				};
 			};
 		};
 	};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1967 bytes --]

On 06/06/17 15:48, Boris Brezillon wrote:

> Okay. Thanks for the clarification. Can you confirm that this version
> is correct?
> 
>  	dsi@xxx {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
>  		ports {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> 			dpi_in: port@0 {
>  				reg = <0>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
>  				endpoint@0 {
>  					remote-endpoint = <&dpi_out>;
>  				};
>  			};
>  
>  			dsi_out: port@1 {
>  				reg = <1>;
> 				#address-cells = <1>;
>  				#size-cells = <0>;
>  
>  				dsi_out_vc0: endpoint@0 {
> 					reg = <0>;
>  					remote-endpoint = <&dsi_panel0_in>;
> 				};
> 
>  				dsi_out_vc1: endpoint@1 {
> 					reg = <1>;
>  					remote-endpoint = <&dsi_panel1_in>;
>  				};
>  			};
>  		};
>  
>  		panel@0 {
>  			compatible = "...";
>  			reg = <0>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
>  			port@0 {
>  				#address-cells = <1>;
>  		                #size-cells = <0>;
>  				reg = <0>;
>  
>  				dsi_panel0_in: endpoint@0 {
> 					reg = <0>;
>  					remote-endpoint = <&dsi_out_vc0>;
>  				};
>  			};
>  		};
>  
>  		panel@1 {
>  			compatible = "...";
>  			reg = <1>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
>  			port@0 {
>  				#address-cells = <1>;
>  		                #size-cells = <0>;
>  				reg = <0>;
>  
>  				dsi_panel1_in: endpoint@0 {
> 					reg = <0>;
>  					remote-endpoint = <&dsi_out_vc1>;
>  				};
>  			};
>  		};
>  	};
> 

Looks correct to me. I think it can be a bit shorter though:

- You don't need #address-cells and #size-cells for all. I think those
are inherited from the parent.
- If there's just one port and one endpoint, you can leave the 'reg'
out, as it's considered to be 0 by default.

So for the panel, you can have just:

port {
	dsi_panel1_in: endpoint {
		remote-endpoint = <&dsi_out_vc1>;
	};
};


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2497 bytes --]

On 06/06/17 15:37, Boris Brezillon wrote:
> Hi Tomi,
> 
> On Tue, 6 Jun 2017 15:30:26 +0300
> Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> 
>> On 02/06/17 15:04, Boris Brezillon wrote:
>>> Document the bindings used for the Cadence DSI bridge.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> ---
>>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>>>  1 file changed, 55 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> new file mode 100644
>>> index 000000000000..770c5c5b1e93
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> @@ -0,0 +1,55 @@
>>> +Cadence DSI bridge
>>> +==================
>>> +
>>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
>>> +
>>> +Required properties:
>>> +- compatible: should be set to "cdns,dsi".
>>> +- reg: physical base address and length of the controller's registers.
>>> +- interrupts: interrupt line connected to the DSI bridge.
>>> +- clocks: DSI bridge clocks.
>>> +- clock-names: must contain "pclk" and "sysclk".  
>>
>> clock-names doesn't match the example below.
> 
> Indeed. I'll fix the example.
> 
>>
>>> +- phys: phandle link to the MIPI D-PHY controller.
>>> +- phy-names: must contain "dphy".
>>> +- #address-cells: must be set to 1.
>>> +- #size-cells: must be set to 0.
>>> +
>>> +Required subnodes:
>>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
>>> +  Currently contains a single input port at address 0 representing the DPI
>>> +  input. Other ports will be added later to support the SDI inputs.  
>>
>> Typo with "SDI".
> 
> No, the 2nd and 3rd input ports are really called SDI. Here
> is the datasheet description:
> 
> "
> SDI: 
> Serial Display Interface - this is the name of the block that is built
> to interface the Display application processor to the DSI link. This is
> a proprietary interface.
> "

Ok. Well, I think that's a bit pointless comment in the binding doc,
it'll only confuse =). Describe what the current binding is, not what
might be added later (but that can be mentioned in the commit desc if
you want).

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Boris Brezillon]

On Tue, 6 Jun 2017 16:01:45 +0300
Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:

> On 06/06/17 15:37, Boris Brezillon wrote:
> > Hi Tomi,
> > 
> > On Tue, 6 Jun 2017 15:30:26 +0300
> > Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> >   
> >> On 02/06/17 15:04, Boris Brezillon wrote:  
> >>> Document the bindings used for the Cadence DSI bridge.
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >>> ---
> >>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >>>  1 file changed, 55 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> new file mode 100644
> >>> index 000000000000..770c5c5b1e93
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> @@ -0,0 +1,55 @@
> >>> +Cadence DSI bridge
> >>> +==================
> >>> +
> >>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
> >>> +
> >>> +Required properties:
> >>> +- compatible: should be set to "cdns,dsi".
> >>> +- reg: physical base address and length of the controller's registers.
> >>> +- interrupts: interrupt line connected to the DSI bridge.
> >>> +- clocks: DSI bridge clocks.
> >>> +- clock-names: must contain "pclk" and "sysclk".    
> >>
> >> clock-names doesn't match the example below.  
> > 
> > Indeed. I'll fix the example.
> >   
> >>  
> >>> +- phys: phandle link to the MIPI D-PHY controller.
> >>> +- phy-names: must contain "dphy".
> >>> +- #address-cells: must be set to 1.
> >>> +- #size-cells: must be set to 0.
> >>> +
> >>> +Required subnodes:
> >>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> >>> +  Currently contains a single input port at address 0 representing the DPI
> >>> +  input. Other ports will be added later to support the SDI inputs.    
> >>
> >> Typo with "SDI".  
> > 
> > No, the 2nd and 3rd input ports are really called SDI. Here
> > is the datasheet description:
> > 
> > "
> > SDI: 
> > Serial Display Interface - this is the name of the block that is built
> > to interface the Display application processor to the DSI link. This is
> > a proprietary interface.
> > "  
> 
> Ok. Well, I think that's a bit pointless comment in the binding doc,
> it'll only confuse =). Describe what the current binding is, not what
> might be added later (but that can be mentioned in the commit desc if
> you want).

OK. Will do that.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1747 bytes --]

On 02/06/17 15:04, Boris Brezillon wrote:

> +enum cdns_dsi_output_type {
> +	CDNS_DSI_PANEL = 0,
> +	CDNS_DSI_BRIDGE = 1,
> +};

Just my opinion, but I think you should only define values for enums
when those values actually mean something and are needed. In this case,
it doesn't matter which values those enums have.

> +static int cdns_dsi_init_link(struct cdns_dsi *dsi)
> +{
> +	u32 val;
> +	int i;
> +
> +	writel(CLK_UNIT_INTERVAL(16), dsi->regs + MCTL_DPHY_STATIC);
> +	writel(CLK_DIV(0xb) | HSTX_TIMEOUT(0xed8afff),
> +	       dsi->regs + MCTL_DPHY_TIMEOUT1);
> +	writel(LPRX_TIMEOUT(0xf30fffff), dsi->regs + MCTL_DPHY_TIMEOUT2);
> +
> +	val = WAIT_BURST_TIME(0xf);
> +	for (i = 1; i < dsi->output->dev->lanes; i++)
> +		val |= DATA_LANE_EN(i);
> +
> +	if (!(dsi->output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> +		val |= CLK_CONTINUOUS;
> +
> +	writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
> +
> +	writel(CLK_LANE_ULPOUT_TIME(0x105) | DATA_LANE_ULPOUT_TIME(0x1d5),
> +	       dsi->regs + MCTL_ULPOUT_TIME);
> +
> +	writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
> +
> +	val = CLK_LANE_EN | PLL_START;
> +	for (i = 0; i < dsi->output->dev->lanes; i++)
> +		val |= DATA_LANE_START(i);
> +
> +	writel(val, dsi->regs + MCTL_MAIN_EN);
> +
> +	ndelay(100);
> +
> +	return 0;
> +}

There are quite a bit of magic values here (elsewhere too). Looking at
the names of the macros, many of them probably represent spans of time,
in clock ticks? Would it make more sense to have the times defined in,
say, nanoseconds, and a function to convert the ns to clock ticks?

And a hardcoded CLK_DIV, does that work? Is the clock rate fixed?

(I don't have the datasheet yet =)

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon]

On Tue, 6 Jun 2017 16:30:15 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 02/06/17 15:04, Boris Brezillon wrote:
> 
> > +enum cdns_dsi_output_type {
> > +	CDNS_DSI_PANEL = 0,
> > +	CDNS_DSI_BRIDGE = 1,
> > +};  
> 
> Just my opinion, but I think you should only define values for enums
> when those values actually mean something and are needed. In this case,
> it doesn't matter which values those enums have.

Actually, this will go away in the next version (see Archit's comment).

> 
> > +static int cdns_dsi_init_link(struct cdns_dsi *dsi)
> > +{
> > +	u32 val;
> > +	int i;
> > +
> > +	writel(CLK_UNIT_INTERVAL(16), dsi->regs + MCTL_DPHY_STATIC);
> > +	writel(CLK_DIV(0xb) | HSTX_TIMEOUT(0xed8afff),
> > +	       dsi->regs + MCTL_DPHY_TIMEOUT1);
> > +	writel(LPRX_TIMEOUT(0xf30fffff), dsi->regs + MCTL_DPHY_TIMEOUT2);
> > +
> > +	val = WAIT_BURST_TIME(0xf);
> > +	for (i = 1; i < dsi->output->dev->lanes; i++)
> > +		val |= DATA_LANE_EN(i);
> > +
> > +	if (!(dsi->output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> > +		val |= CLK_CONTINUOUS;
> > +
> > +	writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
> > +
> > +	writel(CLK_LANE_ULPOUT_TIME(0x105) | DATA_LANE_ULPOUT_TIME(0x1d5),
> > +	       dsi->regs + MCTL_ULPOUT_TIME);
> > +
> > +	writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
> > +
> > +	val = CLK_LANE_EN | PLL_START;
> > +	for (i = 0; i < dsi->output->dev->lanes; i++)
> > +		val |= DATA_LANE_START(i);
> > +
> > +	writel(val, dsi->regs + MCTL_MAIN_EN);
> > +
> > +	ndelay(100);
> > +
> > +	return 0;
> > +}  
> 
> There are quite a bit of magic values here (elsewhere too). Looking at
> the names of the macros, many of them probably represent spans of time,
> in clock ticks? Would it make more sense to have the times defined in,
> say, nanoseconds, and a function to convert the ns to clock ticks?
> 
> And a hardcoded CLK_DIV, does that work? Is the clock rate fixed?

Okay, it seems I have all the necessary information to dynamically
calculate ULPOUT_TIME values (these values are based on sysclk and
ULPOUT_TIME should be 1ms).

It's a bit more complicated for MCTL_DPHY_TIMEOUT1 and
MCTL_DPHY_TIMEOUT2, because counters are based on the dsi_tx_byte clock
which is derived from the DPHY PLL, and I don't have the final
spec of the DPHY block yet, which means I can't extract the dsi_tx_byte
clock rate.
A temporary solution would be to hardcore the tx_byte_clk in the driver
and do all the calculation based on this hardcoded value. Or maybe we
should expose the DPHY PLL in the DT.

I also don't know what CLK_UNIT_INTERVAL() is encoding. I'll check with
Cadence engineers.

Thanks,

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

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Andrzej Hajda]

Hi,

Just spotted this thread.

On 06.06.2017 14:58, Tomi Valkeinen wrote:
> On 06/06/17 15:48, Boris Brezillon wrote:
>
>> Okay. Thanks for the clarification. Can you confirm that this version
>> is correct?
>>
>>  	dsi@xxx {
>>  		#address-cells = <1>;
>>  		#size-cells = <0>;
>>  
>>  		ports {
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>> 			dpi_in: port@0 {
>>  				reg = <0>;
>>  				#address-cells = <1>;
>>  				#size-cells = <0>;
>>  
>>  				endpoint@0 {
>>  					remote-endpoint = <&dpi_out>;
>>  				};
>>  			};
>>  
>>  			dsi_out: port@1 {
>>  				reg = <1>;
>> 				#address-cells = <1>;
>>  				#size-cells = <0>;
>>  
>>  				dsi_out_vc0: endpoint@0 {
>> 					reg = <0>;
>>  					remote-endpoint = <&dsi_panel0_in>;
>> 				};
>>
>>  				dsi_out_vc1: endpoint@1 {
>> 					reg = <1>;
>>  					remote-endpoint = <&dsi_panel1_in>;
>>  				};
>>  			};
>>  		};
>>  
>>  		panel@0 {
>>  			compatible = "...";
>>  			reg = <0>;
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  
>>  			port@0 {
>>  				#address-cells = <1>;
>>  		                #size-cells = <0>;
>>  				reg = <0>;
>>  
>>  				dsi_panel0_in: endpoint@0 {
>> 					reg = <0>;
>>  					remote-endpoint = <&dsi_out_vc0>;
>>  				};
>>  			};
>>  		};
>>  
>>  		panel@1 {
>>  			compatible = "...";
>>  			reg = <1>;
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  
>>  			port@0 {
>>  				#address-cells = <1>;
>>  		                #size-cells = <0>;
>>  				reg = <0>;
>>  
>>  				dsi_panel1_in: endpoint@0 {
>> 					reg = <0>;
>>  					remote-endpoint = <&dsi_out_vc1>;
>>  				};
>>  			};
>>  		};
>>  	};
>>
> Looks correct to me. I think it can be a bit shorter though:
>
> - You don't need #address-cells and #size-cells for all. I think those
> are inherited from the parent.
> - If there's just one port and one endpoint, you can leave the 'reg'
> out, as it's considered to be 0 by default.
>
> So for the panel, you can have just:
>
> port {
> 	dsi_panel1_in: endpoint {
> 		remote-endpoint = <&dsi_out_vc1>;
> 	};
> };

In case DSI bus is used to both control and sending video signal you can
skip video links from dsi-host to dsi-child, so nodes can look like:


 	dsi@xxx {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
 		ports {
 			#address-cells = <1>;
 			#size-cells = <0>;
			dpi_in: port@0 {
 				reg = <0>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
 				endpoint@0 {
 					remote-endpoint = <&dpi_out>;
 				};
 			};
 
 		};
 
 		panel@0 {
 			compatible = "...";
 			reg = <0>;
 		};
 
 		panel@1 {
 			compatible = "...";
 			reg = <1>;
 		};
 	};



Regards
Andrzej

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Archit Taneja]

On 06/06/2017 06:28 PM, Tomi Valkeinen wrote:
> On 06/06/17 15:48, Boris Brezillon wrote:
> 
>> Okay. Thanks for the clarification. Can you confirm that this version
>> is correct?
>>
>>   	dsi@xxx {
>>   		#address-cells = <1>;
>>   		#size-cells = <0>;
>>   
>>   		ports {
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>> 			dpi_in: port@0 {
>>   				reg = <0>;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>>   				endpoint@0 {
>>   					remote-endpoint = <&dpi_out>;
>>   				};
>>   			};
>>   
>>   			dsi_out: port@1 {
>>   				reg = <1>;
>> 				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>>   				dsi_out_vc0: endpoint@0 {
>> 					reg = <0>;
>>   					remote-endpoint = <&dsi_panel0_in>;
>> 				};
>>
>>   				dsi_out_vc1: endpoint@1 {
>> 					reg = <1>;
>>   					remote-endpoint = <&dsi_panel1_in>;
>>   				};
>>   			};
>>   		};
>>   
>>   		panel@0 {
>>   			compatible = "...";
>>   			reg = <0>;
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>>   
>>   			port@0 {
>>   				#address-cells = <1>;
>>   		                #size-cells = <0>;
>>   				reg = <0>;
>>   
>>   				dsi_panel0_in: endpoint@0 {
>> 					reg = <0>;
>>   					remote-endpoint = <&dsi_out_vc0>;
>>   				};
>>   			};
>>   		};
>>   
>>   		panel@1 {
>>   			compatible = "...";
>>   			reg = <1>;
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>>   
>>   			port@0 {
>>   				#address-cells = <1>;
>>   		                #size-cells = <0>;
>>   				reg = <0>;
>>   
>>   				dsi_panel1_in: endpoint@0 {
>> 					reg = <0>;
>>   					remote-endpoint = <&dsi_out_vc1>;
>>   				};
>>   			};
>>   		};
>>   	};
>>
> 
> Looks correct to me. I think it can be a bit shorter though:
> 
> - You don't need #address-cells and #size-cells for all. I think those
> are inherited from the parent.
> - If there's just one port and one endpoint, you can leave the 'reg'
> out, as it's considered to be 0 by default.
> 
> So for the panel, you can have just:
> 
> port {
> 	dsi_panel1_in: endpoint {
> 		remote-endpoint = <&dsi_out_vc1>;
> 	};
> };

Looks good to me too.

Thanks,
Archit


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Archit Taneja]

On 06/06/2017 03:50 PM, Boris Brezillon wrote:
> Hi Archit,
> 
> On Sun, 4 Jun 2017 00:20:06 +0530
> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> 
> 
>>> +
>>> +#define DPHY_CFG0			0x1b0
>>> +#define DPHY_C_RSTB			BIT(20)
>>> +#define DPHY_D_RSTB(x)			((x) << 16)
>>> +#define DPHY_TIF_FORCE_WRITE		BIT(12)
>>> +#define DPHY_PLL_PDN			BIT(10)
>>> +#define DPHY_CMN_PDN			BIT(9)
>>> +#define DPHY_C_PDN			BIT(8)
>>> +#define DPHY_D_PDN(x)			((x) << 4)
>>> +#define DPHY_PLL_PSO			BIT(1)
>>> +#define DPHY_CMN_PSO			BIT(0)
>>> +
>>> +#define DPHY_CFG1			0x1b4
>>> +#define PDHY_PLL_OPDIV(x)		((x) << 20)
>>> +#define PDHY_PLL_IPDIV(x)		((x) << 12)
>>> +#define PDHY_PLL_FBDIV(x)		(x)
>>> +
>>> +#define DPHY_PLL_TM_LO			0x1b8
>>> +#define DPHY_PLL_TM_MID			0x1bc
>>> +#define DPHY_PLL_TM_HI			0x1c0
>>> +
>>> +#define DPHY_STATUS			0x1c4
>>> +#define PPI_D_RX_ULPS_ESC(x)		((x) >> 12)
>>> +#define PPI_C_TX_READY_HS		BIT(8)
>>> +#define PPI_PLL_LOCK			BIT(7)
>>> +#define PPI_PLL_COARSE			BIT(6)
>>> +#define PPI_PLL_COARSE_CODE(x)		((x) & GENMASK(5, 0))
>>> +
>>> +#define DPHY_BIST			0x1c8
>>> +#define PSO_BYPASS_CTX_EN		BIT(12)
>>> +#define PSO_BYPASS_TX_EN(l)		BIT(8 + (l))
>>> +#define BIST_CTX_EN			BIT(4)
>>> +#define BIST_TX_EN(l)			BIT(l)
>>> +
>>
>> Do the above DPHY registers actually configure the PHY used with the
>> controller, or do we need to configure any additional register outside
>> of this block to get things working?
> 
> The DPHY has a separate I/O mem range with its own interface. I didn't
> provide support for this part yet because the interface is not stable
> yet.
> 
>>
>> I ask because they aren't used in the code below, and the DT binding
>> for this device specifies the PHY as a separate device. What's the
>> plan in the future for PHY?
> 
> The short-term plan is to add support for this DPHY in the cdns-dsi
> driver. The driver will just retrieve the I/O mem range and interrupt
> attached to the cdns DPHY block by following the phandle and using
> of helpers to do the conversion, and then use these resources
> internally.
> 
> The mid/long-term plan is to add a dphy framework (on top of the PHY
> framework) to let dphy providers expose their features in a generic
> manner.
> There are 2 pros to this generic DPHY framework/layer:
> - you could possibly use DPHY and DSI blocks provided by 2 different
>    vendors (not sure how feasible this is in practice)
> - CSI can also use DPHY as its PHY layer, so DPHY drivers could be
>    shared between V4L and DRM drivers
> 
> Note that I can't promise the mid/long-term goals are achievable,
> because my knowledge on DPHY is quite limited, but designing the DT
> bindings to handle this use case is really important, because of the DT
> stable ABI thing.

Okay. There are patches to support the synopsis DW DSI bridge driver
in the list, and the consensus there was that it isn't necessary to
have a separate DPHY node for blocks which are tightly integrated
with a SoC:

https://patchwork.kernel.org/patch/9764395/

I was wondering if we should adopt to a standard way of dealing with
the phys, i.e, let the SoC driver provide phy ops as platform data
versus having a separate PHY driver altogether.

In any case, I don't think there isn't harm having a PHY node in
the bindings even if we go the other way.

> 
> 
>>> +
>>> +static int cdns_dsi_bridge_attach(struct drm_bridge *bridge)
>>> +{
>>> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>> +	struct cdns_dsi_output *output = input->dsi->output;
>>> +	int ret;
>>> +
>>> +	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
>>> +		dev_err(input->dsi->base.dev,
>>> +			"cdns-dsi driver is only compatible with DRM devices supporting atomic updates");
>>> +		return -ENOTSUPP;
>>> +	}
>>> +
>>> +	switch (output->type) {
>>> +	case CDNS_DSI_PANEL:
>>> +		ret = cdns_dsi_output_attach_panel(output);
>>> +		break;
>>> +
>>> +	case CDNS_DSI_BRIDGE:
>>> +		ret = drm_bridge_attach(bridge->encoder, output->bridge,
>>> +					bridge);
>>> +		break;
>>
>> Could you have a look at Eric's dsi-panel-bridge patch-set? I think it
>> should simplify things for this driver too.
> 
> Yep, that was planned. Was just waiting for this feature to reach
> drm-misc-next (which seems to be the case ;-)).
> 
>>
>>> +
>>> +	default:
>>> +		ret = -ENOTSUPP;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static bool cdns_dsi_bridge_mode_fixup(struct drm_bridge *bridge,
>>> +				       const struct drm_display_mode *mode,
>>> +				       struct drm_display_mode *adj)
>>> +{
>>> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>> +	int bpp;
>>> +
>>> +	/*
>>> +	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>>> +	 * least 1.
>>> +	 */
>>> +	if (adj->crtc_vtotal - adj->crtc_vsync_end < 2)
>>> +		return false;
>>> +
>>> +	/* VSA_DSI = VSA_DPI and must be at least 2. */
>>> +	if (adj->crtc_vsync_end - adj->crtc_vsync_start < 2)
>>> +		return false;
>>> +
>>> +	/* HACT must be a 32-bits aligned. */
>>> +	bpp = mipi_dsi_pixel_format_to_bpp(input->dsi->output->dev->format);
>>> +	if ((adj->hdisplay * bpp) % 32)
>>> +		return false;
>>> +
>>> +	return true;
>>
>> We could consider using the new mode_valid helpers that Jose recently
>> added. This might be better as bridge mode_valid() op.
> 
> Ditto. It was on my TODO list. I'll address that in my v3.
> 
>>
>>
>>> +}
> 
> [...]
> 
>>> +
>>> +static int cdns_dsi_attach(struct mipi_dsi_host *host,
>>> +			   struct mipi_dsi_device *dev)
>>> +{
>>> +	struct cdns_dsi *dsi = to_cdns_dsi(host);
>>> +	struct cdns_dsi_output *output;
>>> +	int ret;
>>> +
>>> +	/* TODO: support multi-devices setup? */
>>> +	if (dsi->output)
>>> +		return -EBUSY;
>>> +
>>> +	output = devm_kzalloc(host->dev, sizeof(*output), GFP_KERNEL);
>>> +	if (!output)
>>> +		return -ENOMEM;
>>> +
>>> +	output->dev = dev;
>>> +
>>> +	output->panel = of_drm_find_panel(dev->dev.of_node);
>>> +	if (output->panel) {
>>> +		output->type = CDNS_DSI_PANEL;
>>> +	} else {
>>> +		output->bridge = of_drm_find_bridge(dev->dev.of_node);
>>> +		if (!output->bridge) {
>>> +			dev_err(host->dev,
>>> +				"%s is neither a panel nor a bridge",
>>> +				dev->name);
>>> +			return -ENOTSUPP;
>>> +		}
>>> +
>>> +		output->type = CDNS_DSI_BRIDGE;
>>> +		dsi->input->bridge.next = output->bridge;
>>> +	}
>>> +
>>> +	dsi->output = output;
>>> +
>>> +	ret = cdns_dsi_init_link(dsi);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* FIXME: should be moved somewhere else. */
>>> +	return drm_bridge_add(&dsi->input->bridge);
>>
>> In the driver probe?
> 
> I assumed that not everything was in place at probe time, but maybe I
> was wrong. This should be good, as long as all DSI devices have been
> instantiated and attached after calling mipi_dsi_host_register(). I
> guess this is something we can ensure by using the proposed bindings
> (with all DSI output ports described in the DT) and returning
> EPROBE_DEFER if at least one of the DSI device is missing at probe time.
> 
>>
>>> +}
>>> +
> 
> [...]
> 
>>> +
>>> +static int cdns_dsi_drm_probe(struct platform_device *pdev)
>>
>> I wanted to bring up the point I made in the DT patch too: Is it
>> more suitable to implement this is a library with bind/unbind, and
>> probe/remove helpers like it's done for dw-hdmi. Could there be SoCs
>> that integrate this IP but require some additional wrapper
>> configurations to make things work?
> 
> As answered in my previous reply, yes it can be done this way, but is it
> worth introducing this infrastructure right now? Note that we can still
> overload the compatible to support SoC specific integration of this IP
> directly in this driver.
> For the rest, it should be pretty transparent to connect a DPI encoder
> to this DSI bridge.

Okay. Sounds good to me.

Thanks,
Archit

> 
> Right now, we have an easy way to connect a DPI encoder to this DSI
> bridge. The only thing that could be missing is the pixel format
> negotiation (the format transiting on the DPI bus), and this is a
> runtime thing, so maybe we can extend the drm_bridge interface to allow
> such negotiation.
> 
> Note that this is a need I had on atmel platforms as well, because the
> DPI bus can be connected to several devices (panels or external
> bridges), and, depending on the device we are enabling in the pipeline,
> we have to reconfigure the DPI encoder to send the pixel in the
> appropriate format.
> 
>>
>> Looks good otherwise.
> 
> Thanks for your review.
> 
> Boris
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Boris Brezillon]

On Tue, 13 Jun 2017 11:02:47 +0200
Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:

> Hi,
> 
> Just spotted this thread.
> 
> On 06.06.2017 14:58, Tomi Valkeinen wrote:
> > On 06/06/17 15:48, Boris Brezillon wrote:
> >  
> >> Okay. Thanks for the clarification. Can you confirm that this version
> >> is correct?
> >>
> >>  	dsi@xxx {
> >>  		#address-cells = <1>;
> >>  		#size-cells = <0>;
> >>  
> >>  		ports {
> >>  			#address-cells = <1>;
> >>  			#size-cells = <0>;
> >> 			dpi_in: port@0 {
> >>  				reg = <0>;
> >>  				#address-cells = <1>;
> >>  				#size-cells = <0>;
> >>  
> >>  				endpoint@0 {
> >>  					remote-endpoint = <&dpi_out>;
> >>  				};
> >>  			};
> >>  
> >>  			dsi_out: port@1 {
> >>  				reg = <1>;
> >> 				#address-cells = <1>;
> >>  				#size-cells = <0>;
> >>  
> >>  				dsi_out_vc0: endpoint@0 {
> >> 					reg = <0>;
> >>  					remote-endpoint = <&dsi_panel0_in>;
> >> 				};
> >>
> >>  				dsi_out_vc1: endpoint@1 {
> >> 					reg = <1>;
> >>  					remote-endpoint = <&dsi_panel1_in>;
> >>  				};
> >>  			};
> >>  		};
> >>  
> >>  		panel@0 {
> >>  			compatible = "...";
> >>  			reg = <0>;
> >>  			#address-cells = <1>;
> >>  			#size-cells = <0>;
> >>  
> >>  			port@0 {
> >>  				#address-cells = <1>;
> >>  		                #size-cells = <0>;
> >>  				reg = <0>;
> >>  
> >>  				dsi_panel0_in: endpoint@0 {
> >> 					reg = <0>;
> >>  					remote-endpoint = <&dsi_out_vc0>;
> >>  				};
> >>  			};
> >>  		};
> >>  
> >>  		panel@1 {
> >>  			compatible = "...";
> >>  			reg = <1>;
> >>  			#address-cells = <1>;
> >>  			#size-cells = <0>;
> >>  
> >>  			port@0 {
> >>  				#address-cells = <1>;
> >>  		                #size-cells = <0>;
> >>  				reg = <0>;
> >>  
> >>  				dsi_panel1_in: endpoint@0 {
> >> 					reg = <0>;
> >>  					remote-endpoint = <&dsi_out_vc1>;
> >>  				};
> >>  			};
> >>  		};
> >>  	};
> >>  
> > Looks correct to me. I think it can be a bit shorter though:
> >
> > - You don't need #address-cells and #size-cells for all. I think those
> > are inherited from the parent.
> > - If there's just one port and one endpoint, you can leave the 'reg'
> > out, as it's considered to be 0 by default.
> >
> > So for the panel, you can have just:
> >
> > port {
> > 	dsi_panel1_in: endpoint {
> > 		remote-endpoint = <&dsi_out_vc1>;
> > 	};
> > };  
> 
> In case DSI bus is used to both control and sending video signal you can
> skip video links from dsi-host to dsi-child, so nodes can look like:
> 
> 
>  	dsi@xxx {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
>  		ports {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> 			dpi_in: port@0 {
>  				reg = <0>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
>  				endpoint@0 {
>  					remote-endpoint = <&dpi_out>;
>  				};
>  			};
>  
>  		};
>  
>  		panel@0 {
>  			compatible = "...";
>  			reg = <0>;
>  		};
>  
>  		panel@1 {
>  			compatible = "...";
>  			reg = <1>;
>  		};
>  	};
> 

Does that mean I should make port1 (AKA DSI ouput port) optional?
IMHO, it's clearer when these links are explicitly described in the DT,
but maybe there are good reasons to keep it implicit for the "control
through DSI" case.

Tomi, Archit, any opinion on this?

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Archit Taneja]

On 06/19/2017 03:42 PM, Boris Brezillon wrote:
> On Tue, 13 Jun 2017 11:02:47 +0200
> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> 
>> Hi,
>>
>> Just spotted this thread.
>>
>> On 06.06.2017 14:58, Tomi Valkeinen wrote:
>>> On 06/06/17 15:48, Boris Brezillon wrote:
>>>   
>>>> Okay. Thanks for the clarification. Can you confirm that this version
>>>> is correct?
>>>>
>>>>   	dsi@xxx {
>>>>   		#address-cells = <1>;
>>>>   		#size-cells = <0>;
>>>>   
>>>>   		ports {
>>>>   			#address-cells = <1>;
>>>>   			#size-cells = <0>;
>>>> 			dpi_in: port@0 {
>>>>   				reg = <0>;
>>>>   				#address-cells = <1>;
>>>>   				#size-cells = <0>;
>>>>   
>>>>   				endpoint@0 {
>>>>   					remote-endpoint = <&dpi_out>;
>>>>   				};
>>>>   			};
>>>>   
>>>>   			dsi_out: port@1 {
>>>>   				reg = <1>;
>>>> 				#address-cells = <1>;
>>>>   				#size-cells = <0>;
>>>>   
>>>>   				dsi_out_vc0: endpoint@0 {
>>>> 					reg = <0>;
>>>>   					remote-endpoint = <&dsi_panel0_in>;
>>>> 				};
>>>>
>>>>   				dsi_out_vc1: endpoint@1 {
>>>> 					reg = <1>;
>>>>   					remote-endpoint = <&dsi_panel1_in>;
>>>>   				};
>>>>   			};
>>>>   		};
>>>>   
>>>>   		panel@0 {
>>>>   			compatible = "...";
>>>>   			reg = <0>;
>>>>   			#address-cells = <1>;
>>>>   			#size-cells = <0>;
>>>>   
>>>>   			port@0 {
>>>>   				#address-cells = <1>;
>>>>   		                #size-cells = <0>;
>>>>   				reg = <0>;
>>>>   
>>>>   				dsi_panel0_in: endpoint@0 {
>>>> 					reg = <0>;
>>>>   					remote-endpoint = <&dsi_out_vc0>;
>>>>   				};
>>>>   			};
>>>>   		};
>>>>   
>>>>   		panel@1 {
>>>>   			compatible = "...";
>>>>   			reg = <1>;
>>>>   			#address-cells = <1>;
>>>>   			#size-cells = <0>;
>>>>   
>>>>   			port@0 {
>>>>   				#address-cells = <1>;
>>>>   		                #size-cells = <0>;
>>>>   				reg = <0>;
>>>>   
>>>>   				dsi_panel1_in: endpoint@0 {
>>>> 					reg = <0>;
>>>>   					remote-endpoint = <&dsi_out_vc1>;
>>>>   				};
>>>>   			};
>>>>   		};
>>>>   	};
>>>>   
>>> Looks correct to me. I think it can be a bit shorter though:
>>>
>>> - You don't need #address-cells and #size-cells for all. I think those
>>> are inherited from the parent.
>>> - If there's just one port and one endpoint, you can leave the 'reg'
>>> out, as it's considered to be 0 by default.
>>>
>>> So for the panel, you can have just:
>>>
>>> port {
>>> 	dsi_panel1_in: endpoint {
>>> 		remote-endpoint = <&dsi_out_vc1>;
>>> 	};
>>> };
>>
>> In case DSI bus is used to both control and sending video signal you can
>> skip video links from dsi-host to dsi-child, so nodes can look like:
>>
>>
>>   	dsi@xxx {
>>   		#address-cells = <1>;
>>   		#size-cells = <0>;
>>   
>>   		ports {
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>> 			dpi_in: port@0 {
>>   				reg = <0>;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>>   				endpoint@0 {
>>   					remote-endpoint = <&dpi_out>;
>>   				};
>>   			};
>>   
>>   		};
>>   
>>   		panel@0 {
>>   			compatible = "...";
>>   			reg = <0>;
>>   		};
>>   
>>   		panel@1 {
>>   			compatible = "...";
>>   			reg = <1>;
>>   		};
>>   	};
>>
> 
> Does that mean I should make port1 (AKA DSI ouput port) optional?
> IMHO, it's clearer when these links are explicitly described in the DT,
> but maybe there are good reasons to keep it implicit for the "control
> through DSI" case.
> 
> Tomi, Archit, any opinion on this?

I guess there isn't any harm in having the links explicitly described. It's
just that those ports won't be used by the driver in the "control through DSI"
case.

For the MSM DSI host bindings, we actually keep the DSI 'data-lanes' param in the
DSI output port, so it's mandatory even if the panel/bridge is controlled via
the host DSI bus.

Andrzej,

Are there any reasons why keeping the host-to-child links in the "control through
DSI" case could be harmful?

Archit

> 
> Regards,
> 
> Boris
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

[Andrzej Hajda]

On 20.06.2017 08:56, Archit Taneja wrote:
>
> On 06/19/2017 03:42 PM, Boris Brezillon wrote:
>> On Tue, 13 Jun 2017 11:02:47 +0200
>> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>
>>> Hi,
>>>
>>> Just spotted this thread.
>>>
>>> On 06.06.2017 14:58, Tomi Valkeinen wrote:
>>>> On 06/06/17 15:48, Boris Brezillon wrote:
>>>>   
>>>>> Okay. Thanks for the clarification. Can you confirm that this version
>>>>> is correct?
>>>>>
>>>>>   	dsi@xxx {
>>>>>   		#address-cells = <1>;
>>>>>   		#size-cells = <0>;
>>>>>   
>>>>>   		ports {
>>>>>   			#address-cells = <1>;
>>>>>   			#size-cells = <0>;
>>>>> 			dpi_in: port@0 {
>>>>>   				reg = <0>;
>>>>>   				#address-cells = <1>;
>>>>>   				#size-cells = <0>;
>>>>>   
>>>>>   				endpoint@0 {
>>>>>   					remote-endpoint = <&dpi_out>;
>>>>>   				};
>>>>>   			};
>>>>>   
>>>>>   			dsi_out: port@1 {
>>>>>   				reg = <1>;
>>>>> 				#address-cells = <1>;
>>>>>   				#size-cells = <0>;
>>>>>   
>>>>>   				dsi_out_vc0: endpoint@0 {
>>>>> 					reg = <0>;
>>>>>   					remote-endpoint = <&dsi_panel0_in>;
>>>>> 				};
>>>>>
>>>>>   				dsi_out_vc1: endpoint@1 {
>>>>> 					reg = <1>;
>>>>>   					remote-endpoint = <&dsi_panel1_in>;
>>>>>   				};
>>>>>   			};
>>>>>   		};
>>>>>   
>>>>>   		panel@0 {
>>>>>   			compatible = "...";
>>>>>   			reg = <0>;
>>>>>   			#address-cells = <1>;
>>>>>   			#size-cells = <0>;
>>>>>   
>>>>>   			port@0 {
>>>>>   				#address-cells = <1>;
>>>>>   		                #size-cells = <0>;
>>>>>   				reg = <0>;
>>>>>   
>>>>>   				dsi_panel0_in: endpoint@0 {
>>>>> 					reg = <0>;
>>>>>   					remote-endpoint = <&dsi_out_vc0>;
>>>>>   				};
>>>>>   			};
>>>>>   		};
>>>>>   
>>>>>   		panel@1 {
>>>>>   			compatible = "...";
>>>>>   			reg = <1>;
>>>>>   			#address-cells = <1>;
>>>>>   			#size-cells = <0>;
>>>>>   
>>>>>   			port@0 {
>>>>>   				#address-cells = <1>;
>>>>>   		                #size-cells = <0>;
>>>>>   				reg = <0>;
>>>>>   
>>>>>   				dsi_panel1_in: endpoint@0 {
>>>>> 					reg = <0>;
>>>>>   					remote-endpoint = <&dsi_out_vc1>;
>>>>>   				};
>>>>>   			};
>>>>>   		};
>>>>>   	};
>>>>>   
>>>> Looks correct to me. I think it can be a bit shorter though:
>>>>
>>>> - You don't need #address-cells and #size-cells for all. I think those
>>>> are inherited from the parent.
>>>> - If there's just one port and one endpoint, you can leave the 'reg'
>>>> out, as it's considered to be 0 by default.
>>>>
>>>> So for the panel, you can have just:
>>>>
>>>> port {
>>>> 	dsi_panel1_in: endpoint {
>>>> 		remote-endpoint = <&dsi_out_vc1>;
>>>> 	};
>>>> };
>>> In case DSI bus is used to both control and sending video signal you can
>>> skip video links from dsi-host to dsi-child, so nodes can look like:
>>>
>>>
>>>   	dsi@xxx {
>>>   		#address-cells = <1>;
>>>   		#size-cells = <0>;
>>>   
>>>   		ports {
>>>   			#address-cells = <1>;
>>>   			#size-cells = <0>;
>>> 			dpi_in: port@0 {
>>>   				reg = <0>;
>>>   				#address-cells = <1>;
>>>   				#size-cells = <0>;
>>>   
>>>   				endpoint@0 {
>>>   					remote-endpoint = <&dpi_out>;
>>>   				};
>>>   			};
>>>   
>>>   		};
>>>   
>>>   		panel@0 {
>>>   			compatible = "...";
>>>   			reg = <0>;
>>>   		};
>>>   
>>>   		panel@1 {
>>>   			compatible = "...";
>>>   			reg = <1>;
>>>   		};
>>>   	};
>>>
>> Does that mean I should make port1 (AKA DSI ouput port) optional?
>> IMHO, it's clearer when these links are explicitly described in the DT,
>> but maybe there are good reasons to keep it implicit for the "control
>> through DSI" case.
>>
>> Tomi, Archit, any opinion on this?
> I guess there isn't any harm in having the links explicitly described. It's
> just that those ports won't be used by the driver in the "control through DSI"
> case.
>
> For the MSM DSI host bindings, we actually keep the DSI 'data-lanes' param in the
> DSI output port, so it's mandatory even if the panel/bridge is controlled via
> the host DSI bus.
>
> Andrzej,
>
> Are there any reasons why keeping the host-to-child links in the "control through
> DSI" case could be harmful?

They are redundant - DSI bus already describes the 'link', so there are
classical potential issues connected with redundancy:
- which info should be parsed by the driver,
- what to do if links provide different information than DSI bus.
And as I understand current device-tree policy is to avoid them in such
case, see [1].

[1]: http://marc.info/?l=dri-devel&m=148354108702517&w=2

Regards
Andrzej


>
> Archit
>
>> Regards,
>>
>> Boris
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html