dm: video: bridge: add operation to read EDID

This header will be used in anx6345 driver

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/anx9804.c | 54 +--------------------------
 include/anx98xx-edp.h   | 98 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 53 deletions(-)
 create mode 100644 include/anx98xx-edp.h

drivers/video/anx9804.c +1 -53

index 37ad69a039..67f7da7d18 100755
@@ -12,61 +12,9 @@
 
 #include <common.h>
 #include <i2c.h>
+#include <anx98xx-edp.h>
 #include "anx9804.h"
 
-/* Registers at i2c address 0x38 */
-
-#define ANX9804_HDCP_CONTROL_0_REG				0x01
-
-#define ANX9804_SYS_CTRL2_REG					0x81
-#define ANX9804_SYS_CTRL2_CHA_STA				0x04
-
-#define ANX9804_SYS_CTRL3_REG					0x82
-#define ANX9804_SYS_CTRL3_VALID_CTRL				BIT(0)
-#define ANX9804_SYS_CTRL3_F_VALID				BIT(1)
-#define ANX9804_SYS_CTRL3_HPD_CTRL				BIT(4)
-#define ANX9804_SYS_CTRL3_F_HPD					BIT(5)
-
-#define ANX9804_LINK_BW_SET_REG					0xa0
-#define ANX9804_LANE_COUNT_SET_REG				0xa1
-#define ANX9804_TRAINING_PTN_SET_REG				0xa2
-#define ANX9804_TRAINING_LANE0_SET_REG				0xa3
-#define ANX9804_TRAINING_LANE1_SET_REG				0xa4
-#define ANX9804_TRAINING_LANE2_SET_REG				0xa5
-#define ANX9804_TRAINING_LANE3_SET_REG				0xa6
-
-#define ANX9804_LINK_TRAINING_CTRL_REG				0xa8
-#define ANX9804_LINK_TRAINING_CTRL_EN				BIT(0)
-
-#define ANX9804_LINK_DEBUG_REG					0xb8
-#define ANX9804_PLL_CTRL_REG					0xc7	
-#define ANX9804_ANALOG_POWER_DOWN_REG				0xc8
-
-/* Registers at i2c address 0x39 */
-
-#define ANX9804_DEV_IDH_REG					0x03
-
-#define ANX9804_POWERD_CTRL_REG					0x05
-#define ANX9804_POWERD_AUDIO					BIT(4)
-
-#define ANX9804_RST_CTRL_REG					0x06
-
-#define ANX9804_RST_CTRL2_REG					0x07
-#define ANX9804_RST_CTRL2_AUX					BIT(2)
-#define ANX9804_RST_CTRL2_AC_MODE				BIT(6)
-
-#define ANX9804_VID_CTRL1_REG					0x08
-#define ANX9804_VID_CTRL1_VID_EN				BIT(7)
-#define ANX9804_VID_CTRL1_EDGE					BIT(0)
-
-#define ANX9804_VID_CTRL2_REG					0x09
-#define ANX9804_ANALOG_DEBUG_REG1				0xdc
-#define ANX9804_ANALOG_DEBUG_REG3				0xde
-#define ANX9804_PLL_FILTER_CTRL1				0xdf
-#define ANX9804_PLL_FILTER_CTRL3				0xe1
-#define ANX9804_PLL_FILTER_CTRL					0xe2
-#define ANX9804_PLL_CTRL3					0xe6
-
 /**
  * anx9804_init() - Init anx9804 parallel lcd to edp bridge chip
  *

include/anx98xx-edp.h new file +98 -0

new file mode 100644
index 0000000000..f7e8baa167
@@ -0,0 +1,98 @@
+/*
+ * Copyright (C) 2015 Hans de Goede <[email protected]>
+ * Copyright (C) 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/* Registers at i2c address 0x38 */
+
+#define ANX9804_HDCP_CONTROL_0_REG				0x01
+
+#define ANX9804_SYS_CTRL1_REG					0x80
+#define ANX9804_SYS_CTRL1_PD_IO					0x80
+#define ANX9804_SYS_CTRL1_PD_VID				0x40
+#define ANX9804_SYS_CTRL1_PD_LINK				0x20
+#define ANX9804_SYS_CTRL1_PD_TOTAL				0x10
+#define ANX9804_SYS_CTRL1_MODE_SEL				0x08
+#define ANX9804_SYS_CTRL1_DET_STA				0x04
+#define ANX9804_SYS_CTRL1_FORCE_DET				0x02
+#define ANX9804_SYS_CTRL1_DET_CTRL				0x01
+
+#define ANX9804_SYS_CTRL2_REG					0x81
+#define ANX9804_SYS_CTRL2_CHA_STA				0x04
+
+#define ANX9804_SYS_CTRL3_REG					0x82
+#define ANX9804_SYS_CTRL3_VALID_CTRL				BIT(0)
+#define ANX9804_SYS_CTRL3_F_VALID				BIT(1)
+#define ANX9804_SYS_CTRL3_HPD_CTRL				BIT(4)
+#define ANX9804_SYS_CTRL3_F_HPD					BIT(5)
+
+#define ANX9804_LINK_BW_SET_REG					0xa0
+#define ANX9804_LANE_COUNT_SET_REG				0xa1
+#define ANX9804_TRAINING_PTN_SET_REG				0xa2
+#define ANX9804_TRAINING_LANE0_SET_REG				0xa3
+#define ANX9804_TRAINING_LANE1_SET_REG				0xa4
+#define ANX9804_TRAINING_LANE2_SET_REG				0xa5
+#define ANX9804_TRAINING_LANE3_SET_REG				0xa6
+
+#define ANX9804_LINK_TRAINING_CTRL_REG				0xa8
+#define ANX9804_LINK_TRAINING_CTRL_EN				BIT(0)
+
+#define ANX9804_LINK_DEBUG_REG					0xb8
+#define ANX9804_PLL_CTRL_REG					0xc7
+#define ANX9804_ANALOG_POWER_DOWN_REG				0xc8
+
+#define ANX9804_AUX_CH_STA					0xe0
+#define ANX9804_AUX_BUSY					BIT(4)
+#define ANX9804_AUX_STATUS_MASK					0x0f
+
+#define ANX9804_DP_AUX_RX_COMM					0xe3
+#define ANX9804_AUX_RX_COMM_I2C_DEFER				BIT(3)
+#define ANX9804_AUX_RX_COMM_AUX_DEFER				BIT(1)
+
+#define ANX9804_DP_AUX_CH_CTL_1					0xe5
+#define ANX9804_AUX_LENGTH(x)					(((x - 1) & 0x0f) << 4)
+#define ANX9804_AUX_TX_COMM_MASK				0x0f
+#define ANX9804_AUX_TX_COMM_DP_TRANSACTION			BIT(3)
+#define ANX9804_AUX_TX_COMM_MOT					BIT(2)
+#define ANX9804_AUX_TX_COMM_READ				BIT(0)
+
+#define ANX9804_DP_AUX_ADDR_7_0					0xe6
+#define ANX9804_DP_AUX_ADDR_15_8				0xe7
+#define ANX9804_DP_AUX_ADDR_19_16				0xe8
+
+#define ANX9804_DP_AUX_CH_CTL_2					0xe9
+#define ANX9804_ADDR_ONLY					BIT(1)
+#define ANX9804_AUX_EN						BIT(0)
+
+#define ANX9804_BUF_DATA_0					0xf0
+
+/* Registers at i2c address 0x39 */
+
+#define ANX9804_DEV_IDH_REG					0x03
+
+#define ANX9804_POWERD_CTRL_REG					0x05
+#define ANX9804_POWERD_AUDIO					BIT(4)
+
+#define ANX9804_RST_CTRL_REG					0x06
+
+#define ANX9804_RST_CTRL2_REG					0x07
+#define ANX9804_RST_CTRL2_AUX					BIT(2)
+#define ANX9804_RST_CTRL2_AC_MODE				BIT(6)
+
+#define ANX9804_VID_CTRL1_REG					0x08
+#define ANX9804_VID_CTRL1_VID_EN				BIT(7)
+#define ANX9804_VID_CTRL1_EDGE					BIT(0)
+
+#define ANX9804_VID_CTRL2_REG					0x09
+#define ANX9804_ANALOG_DEBUG_REG1				0xdc
+#define ANX9804_ANALOG_DEBUG_REG3				0xde
+#define ANX9804_PLL_FILTER_CTRL1				0xdf
+#define ANX9804_PLL_FILTER_CTRL3				0xe1
+#define ANX9804_PLL_FILTER_CTRL					0xe2
+#define ANX9804_PLL_CTRL3					0xe6
+
+#define ANX9804_DP_INT_STA					0xf7
+#define ANX9804_RPLY_RECEIV					BIT(1)
+#define ANX9804_AUX_ERR						BIT(0)

This is a eDP bridge similar to ANX9804, it allows to connect eDP panels to the chips that can output only parallel signal

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/bridge/Kconfig   |   8 +
 drivers/video/bridge/Makefile  |   1 +
 drivers/video/bridge/anx6345.c | 419 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 428 insertions(+)
 create mode 100644 drivers/video/bridge/anx6345.c

drivers/video/bridge/Kconfig +8 -0

index 2a3b6c4bee..765f7380b8 100644
@@ -25,3 +25,11 @@ config VIDEO_BRIDGE_NXP_PTN3460
 	  signalling) converter. It enables an LVDS LCD panel to be connected
 	  to an eDP output device such as an SoC that lacks LVDS capability,
 	  or where LVDS requires too many signals to route on the PCB.
+
+config VIDEO_BRIDGE_ANALOGIX_ANX6345
+	bool "Support Analogix ANX6345 RGB->DP bridge"
+	depends on VIDEO_BRIDGE
+	select DM_I2C
+	help
+	 The Analogix ANX6345 is RGB-to-DP converter. It enables an eDP LCD
+	 panel to be connected to an parallel LCD interface.

drivers/video/bridge/Makefile +1 -0

index ce731fa4ca..2a746c6f8b 100644
@@ -7,3 +7,4 @@
 obj-$(CONFIG_VIDEO_BRIDGE) += video-bridge-uclass.o
 obj-$(CONFIG_VIDEO_BRIDGE_PARADE_PS862X) += ps862x.o
 obj-$(CONFIG_VIDEO_BRIDGE_NXP_PTN3460) += ptn3460.o
+obj-$(CONFIG_VIDEO_BRIDGE_ANALOGIX_ANX6345) += anx6345.o

drivers/video/bridge/anx6345.c new file +419 -0

new file mode 100644
index 0000000000..6bac9a51a9
@@ -0,0 +1,419 @@
+/*
+ * Copyright (C) 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <i2c.h>
+#include <edid.h>
+#include <video_bridge.h>
+#include <anx98xx-edp.h>
+
+#define DP_MAX_LINK_RATE		0x001
+#define DP_MAX_LANE_COUNT		0x002
+#define DP_MAX_LANE_COUNT_MASK		0x1f
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct anx6345_priv {
+	u8 edid[EDID_SIZE];
+};
+
+static int anx6345_write(struct udevice *dev, unsigned addr_off,
+			 unsigned char reg_addr, unsigned char value)
+{
+	uint8_t buf[2];
+	struct i2c_msg msg;
+	int ret;
+
+	msg.addr = addr_off;
+	msg.flags = 0;
+	buf[0] = reg_addr;
+	buf[1] = value;
+	msg.buf = buf;
+	msg.len = 2;
+	ret = dm_i2c_xfer(dev, &msg, 1);
+	if (ret) {
+		debug("%s: write failed, reg=%#x, value=%#x, ret=%d\n",
+		      __func__, reg_addr, value, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int anx6345_read(struct udevice *dev, unsigned addr_off,
+			unsigned char reg_addr, unsigned char *value)
+{
+	uint8_t addr, val;
+	struct i2c_msg msg[2];
+	int ret;
+
+	msg[0].addr = addr_off;
+	msg[0].flags = 0;
+	addr = reg_addr;
+	msg[0].buf = &addr;
+	msg[0].len = 1;
+	msg[1].addr = addr_off;
+	msg[1].flags = I2C_M_RD;
+	msg[1].buf = &val;
+	msg[1].len = 1;
+	ret = dm_i2c_xfer(dev, msg, 2);
+	if (ret) {
+		debug("%s: read failed, reg=%.2x, value=%p, ret=%d\n",
+		      __func__, (int)reg_addr, value, ret);
+		return ret;
+	}
+	*value = val;
+
+	return 0;
+}
+
+static int anx6345_write_r0(struct udevice *dev, unsigned char reg_addr,
+			    unsigned char value)
+{
+	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+	return anx6345_write(dev, chip->chip_addr, reg_addr, value);
+}
+
+static int anx6345_read_r0(struct udevice *dev, unsigned char reg_addr,
+			   unsigned char *value)
+{
+	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+	return anx6345_read(dev, chip->chip_addr, reg_addr, value);
+}
+
+static int anx6345_write_r1(struct udevice *dev, unsigned char reg_addr,
+			    unsigned char value)
+{
+	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+	return anx6345_write(dev, chip->chip_addr + 1, reg_addr, value);
+}
+
+static int anx6345_read_r1(struct udevice *dev, unsigned char reg_addr,
+			   unsigned char *value)
+{
+	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+	return anx6345_read(dev, chip->chip_addr + 1, reg_addr, value);
+}
+
+static int anx6345_set_backlight(struct udevice *dev, int percent)
+{
+	return -ENOSYS;
+}
+
+static int anx6345_aux_wait(struct udevice *dev)
+{
+	int ret = -ETIMEDOUT;
+	u8 v;
+	int retries = 1000;
+	do {
+		anx6345_read_r0(dev, ANX9804_DP_AUX_CH_CTL_2, &v);
+		if (!(v & ANX9804_AUX_EN)) {
+			ret = 0;
+			break;
+		}
+		udelay(100);
+	} while (retries--);
+
+	if (ret) {
+		debug("%s: timed out waiting for AUX_EN to clear\n", __func__);
+		return ret;
+	}
+
+	ret = -ETIMEDOUT;
+	retries = 1000;
+	do {
+		anx6345_read_r1(dev, ANX9804_DP_INT_STA, &v);
+		if (v & ANX9804_RPLY_RECEIV) {
+			ret = 0;
+			break;
+		}
+		udelay(100);
+	} while (retries--);
+
+	if (ret) {
+		debug("%s: timed out waiting to receive reply\n", __func__);
+		return ret;
+	}
+
+	/* Clear RPLY_RECEIV bit */
+	anx6345_write_r1(dev, ANX9804_DP_INT_STA, v);
+
+	anx6345_read_r0(dev, ANX9804_AUX_CH_STA, &v);
+	if ((v & ANX9804_AUX_STATUS_MASK) != 0) {
+		debug("AUX status: %d\n", v & ANX9804_AUX_STATUS_MASK);
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+static void anx6345_aux_addr(struct udevice *dev, u32 addr)
+{
+	u8 val;
+
+	val = addr & 0xff;
+	anx6345_write_r0(dev, ANX9804_DP_AUX_ADDR_7_0, val);
+	val = (addr >> 8) & 0xff;
+	anx6345_write_r0(dev, ANX9804_DP_AUX_ADDR_15_8, val);
+	val = (addr >> 16) & 0x0f;
+	anx6345_write_r0(dev, ANX9804_DP_AUX_ADDR_19_16, val);
+}
+
+static int anx6345_aux_transfer(struct udevice *dev, u8 req, u32 addr, u8 *buf, size_t len)
+{
+	int i, ret;
+	u8 ctrl1 = req;
+	u8 ctrl2 = ANX9804_AUX_EN;
+
+	if (len > 16)
+		return -E2BIG;
+
+	if (len)
+		ctrl1 |= ANX9804_AUX_LENGTH(len);
+	else
+		ctrl2 |= ANX9804_ADDR_ONLY;
+
+	if (len && !(req & ANX9804_AUX_TX_COMM_READ)) {
+		for (i = 0; i < len; i++)
+			anx6345_write_r0(dev, ANX9804_BUF_DATA_0 + i, buf[i]);
+	}
+
+	anx6345_aux_addr(dev, addr);
+	anx6345_write_r0(dev, ANX9804_DP_AUX_CH_CTL_1, ctrl1);
+	anx6345_write_r0(dev, ANX9804_DP_AUX_CH_CTL_2, ctrl2);
+	ret = anx6345_aux_wait(dev);
+	if (ret) {
+		debug("AUX transaction timed out\n");
+		return ret;
+	}
+
+	if (len && (req & ANX9804_AUX_TX_COMM_READ)) {
+		for (i = 0; i < len; i++)
+			anx6345_read_r0(dev, ANX9804_BUF_DATA_0 + i, &buf[i]);
+	}
+
+	return 0;
+
+}
+
+static int anx6345_read_aux_i2c(struct udevice *dev, u8 chip_addr,
+			        u8 offset,
+				size_t count,
+				u8 *buf)
+{
+	int i, ret;
+	size_t cur_cnt;
+	u8 cur_offset;
+	for (i = 0; i < count; i += 16) {
+		cur_cnt = (count - i) > 16 ? 16 : count - i;
+		cur_offset = offset + i;
+		ret = anx6345_aux_transfer(dev, ANX9804_AUX_TX_COMM_MOT,
+					   chip_addr, &cur_offset, 1);
+		if (ret) {
+			debug("%s: failed to set i2c offset: %d\n", __func__, ret);
+			return ret;
+		}
+		ret = anx6345_aux_transfer(dev, ANX9804_AUX_TX_COMM_READ,
+					   chip_addr, buf + i, cur_cnt);
+		if (ret) {
+			debug("%s: failed to read from i2c device: %d\n", __func__, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int anx6345_read_dpcd(struct udevice *dev, u32 reg, u8 *val)
+{
+	int ret;
+
+	ret = anx6345_aux_transfer(dev,
+				   ANX9804_AUX_TX_COMM_READ |
+				   ANX9804_AUX_TX_COMM_DP_TRANSACTION,
+				   reg, val, 1);
+	if (ret) {
+		debug ("Failed to read DPCD\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int anx6345_read_edid(struct udevice *dev, u8 *buf, int size)
+{
+	struct anx6345_priv *priv = dev_get_priv(dev);
+
+	if (size > EDID_SIZE)
+		size = EDID_SIZE;
+	memcpy(buf, priv->edid, size);
+
+	return size;
+}
+
+static int anx6345_attach(struct udevice *dev)
+{
+	/* No-op */
+	return 0;
+}
+
+static int anx6345_enable(struct udevice *dev)
+{
+	u8 chipid, colordepth, lanes, data_rate, c;
+	int ret, i, bpp;
+	struct display_timing timing;
+	struct anx6345_priv *priv = dev_get_priv(dev);
+
+	/* Deassert reset and enable power */
+	ret = video_bridge_set_active(dev, true);
+	if (ret)
+		return ret;
+
+	/* Reset */
+	anx6345_write_r1(dev, ANX9804_RST_CTRL_REG, 1);
+	mdelay(100);
+	anx6345_write_r1(dev, ANX9804_RST_CTRL_REG, 0);
+
+	/* Write 0 to the powerdown reg (powerup everything) */
+	anx6345_write_r1(dev, ANX9804_POWERD_CTRL_REG, 0);
+
+	ret = anx6345_read_r1(dev, ANX9804_DEV_IDH_REG, &chipid);
+	if (ret)
+		debug("%s: read id failed: %d\n", __func__, ret);
+
+	switch (chipid) {
+	case 0x63:
+		debug("ANX63xx detected.\n");
+		break;
+	default:
+		debug("Error anx6345 chipid mismatch: %.2x\n", (int)chipid);
+		return -ENODEV;
+	}
+
+	for (i = 0; i < 100; i++) {
+		anx6345_read_r0(dev, ANX9804_SYS_CTRL2_REG, &c);
+		anx6345_write_r0(dev, ANX9804_SYS_CTRL2_REG, c);
+		anx6345_read_r0(dev, ANX9804_SYS_CTRL2_REG, &c);
+		if ((c & ANX9804_SYS_CTRL2_CHA_STA) == 0)
+			break;
+
+		mdelay(5);
+	}
+	if (i == 100)
+		debug("Error anx6345 clock is not stable\n");
+
+	/* Set a bunch of analog related register values */
+	anx6345_write_r0(dev, ANX9804_PLL_CTRL_REG, 0x00);
+	anx6345_write_r1(dev, ANX9804_ANALOG_DEBUG_REG1, 0x70);
+	anx6345_write_r0(dev, ANX9804_LINK_DEBUG_REG, 0x30);
+
+	/* Force HPD */
+	anx6345_write_r0(dev, ANX9804_SYS_CTRL3_REG,
+		      ANX9804_SYS_CTRL3_F_HPD | ANX9804_SYS_CTRL3_HPD_CTRL);
+
+	/* Power up and configure lanes */
+	anx6345_write_r0(dev, ANX9804_ANALOG_POWER_DOWN_REG, 0x00);
+	anx6345_write_r0(dev, ANX9804_TRAINING_LANE0_SET_REG, 0x00);
+	anx6345_write_r0(dev, ANX9804_TRAINING_LANE1_SET_REG, 0x00);
+	anx6345_write_r0(dev, ANX9804_TRAINING_LANE2_SET_REG, 0x00);
+	anx6345_write_r0(dev, ANX9804_TRAINING_LANE3_SET_REG, 0x00);
+
+	/* Reset AUX CH */
+	anx6345_write_r1(dev, ANX9804_RST_CTRL2_REG,
+		      ANX9804_RST_CTRL2_AUX);
+	anx6345_write_r1(dev, ANX9804_RST_CTRL2_REG, 0);
+
+	/* Powerdown audio and some other unused bits */
+	anx6345_write_r1(dev, ANX9804_POWERD_CTRL_REG, ANX9804_POWERD_AUDIO);
+	anx6345_write_r0(dev, ANX9804_HDCP_CONTROL_0_REG, 0x00);
+	anx6345_write_r0(dev, 0xa7, 0x00);
+
+	anx6345_read_aux_i2c(dev, 0x50, 0x0, EDID_SIZE, priv->edid);
+	if (edid_get_timing(priv->edid, EDID_SIZE, &timing, &bpp) != 0) {
+		debug("Failed to parse EDID\n");
+		return -EIO;
+	}
+	debug("%s: panel found: %dx%d, bpp %d\n", __func__,
+		timing.hactive.typ, timing.vactive.typ,
+		bpp);
+	if (bpp == 6)
+		colordepth = 0x00; /* 6 bit */
+	else
+		colordepth = 0x10; /* 8 bit */
+	anx6345_write_r1(dev, ANX9804_VID_CTRL2_REG, colordepth);
+
+	if (anx6345_read_dpcd(dev, DP_MAX_LINK_RATE, &data_rate)) {
+		debug("%s: Failed to DP_MAX_LINK_RATE\n", __func__);
+		return -EIO;
+	}
+	debug("%s: data_rate: %d\n", __func__, (int)data_rate);
+	if (anx6345_read_dpcd(dev, DP_MAX_LANE_COUNT, &lanes)) {
+		debug("%s: Failed to read DP_MAX_LANE_COUNT\n", __func__);
+		return -EIO;
+	}
+	lanes &= DP_MAX_LANE_COUNT_MASK;
+	debug("%s: lanes: %d\n", __func__, (int)lanes);
+
+	/* Set data-rate / lanes */
+	anx6345_write_r0(dev, ANX9804_LINK_BW_SET_REG, data_rate);
+	anx6345_write_r0(dev, ANX9804_LANE_COUNT_SET_REG, lanes);
+
+	/* Link training */
+	anx6345_write_r0(dev, ANX9804_LINK_TRAINING_CTRL_REG,
+		      ANX9804_LINK_TRAINING_CTRL_EN);
+	mdelay(5);
+	for (i = 0; i < 100; i++) {
+		anx6345_read_r0(dev, ANX9804_LINK_TRAINING_CTRL_REG, &c);
+		if ((chipid == 0x63) && (c & 0x80) == 0)
+			break;
+
+		mdelay(5);
+	}
+	if(i == 100) {
+		debug("Error anx6345 link training timeout\n");
+		return -ENODEV;
+	}
+
+	/* Enable */
+	anx6345_write_r1(dev, ANX9804_VID_CTRL1_REG,
+		      ANX9804_VID_CTRL1_VID_EN | ANX9804_VID_CTRL1_EDGE);
+	/* Force stream valid */
+	anx6345_write_r0(dev, ANX9804_SYS_CTRL3_REG,
+		      ANX9804_SYS_CTRL3_F_HPD | ANX9804_SYS_CTRL3_HPD_CTRL |
+		      ANX9804_SYS_CTRL3_F_VALID | ANX9804_SYS_CTRL3_VALID_CTRL);
+
+	return 0;
+}
+
+static int anx6345_probe(struct udevice *dev)
+{
+	if (device_get_uclass_id(dev->parent) != UCLASS_I2C)
+		return -EPROTONOSUPPORT;
+
+	return anx6345_enable(dev);
+}
+
+struct video_bridge_ops anx6345_ops = {
+	.attach = anx6345_attach,
+	.set_backlight = anx6345_set_backlight,
+	.read_edid = anx6345_read_edid,
+};
+
+static const struct udevice_id anx6345_ids[] = {
+	{ .compatible = "analogix,anx6345", },
+	{ }
+};
+
+U_BOOT_DRIVER(analogix_anx6345) = {
+	.name	= "analogix_anx6345",
+	.id	= UCLASS_VIDEO_BRIDGE,
+	.of_match = anx6345_ids,
+	.probe	= anx6345_probe,
+	.ops	= &anx6345_ops,
+	.priv_auto_alloc_size = sizeof(struct anx6345_priv),
+};

Quoted 2 lines · Vasily Khoruzhick

This is a eDP bridge similar to ANX9804, it allows to connect eDP panels to the chips that can output only parallel signal

Have you tried using the existing driver?
Icenowy did this some months ago[1], and she got away with quite a small patch to support the ANX6345 as well:

https://github.com/Icenowy/u-boot/commit/354b24c2064ab4f1b13568f61ab24ea97294a16d

Cheers,
Andre.

[1] https://github.com/Icenowy/u-boot/commits/a64-pb-lcd

Quoted 459 lines

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 drivers/video/bridge/Kconfig   |   8 +
 drivers/video/bridge/Makefile  |   1 +
 drivers/video/bridge/anx6345.c | 419 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 428 insertions(+)
 create mode 100644 drivers/video/bridge/anx6345.c

drivers/video/bridge/Kconfig +8 -0

index 2a3b6c4bee..765f7380b8 100644
@@ -25,3 +25,11 @@ config VIDEO_BRIDGE_NXP_PTN3460
        signalling) converter. It enables an LVDS LCD panel to be connected
        to an eDP output device such as an SoC that lacks LVDS capability,
        or where LVDS requires too many signals to route on the PCB.
+
+config VIDEO_BRIDGE_ANALOGIX_ANX6345
+     bool "Support Analogix ANX6345 RGB->DP bridge"
+     depends on VIDEO_BRIDGE
+     select DM_I2C
+     help
+      The Analogix ANX6345 is RGB-to-DP converter. It enables an eDP LCD
+      panel to be connected to an parallel LCD interface.

drivers/video/bridge/Makefile +1 -0

index ce731fa4ca..2a746c6f8b 100644
@@ -7,3 +7,4 @@
 obj-$(CONFIG_VIDEO_BRIDGE) += video-bridge-uclass.o
 obj-$(CONFIG_VIDEO_BRIDGE_PARADE_PS862X) += ps862x.o
 obj-$(CONFIG_VIDEO_BRIDGE_NXP_PTN3460) += ptn3460.o
+obj-$(CONFIG_VIDEO_BRIDGE_ANALOGIX_ANX6345) += anx6345.o

drivers/video/bridge/anx6345.c new file +419 -0

new file mode 100644
index 0000000000..6bac9a51a9
@@ -0,0 +1,419 @@
+/*
+ * Copyright (C) 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:  GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <i2c.h>
+#include <edid.h>
+#include <video_bridge.h>
+#include <anx98xx-edp.h>
+
+#define DP_MAX_LINK_RATE             0x001
+#define DP_MAX_LANE_COUNT            0x002
+#define DP_MAX_LANE_COUNT_MASK               0x1f
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct anx6345_priv {
+     u8 edid[EDID_SIZE];
+};
+
+static int anx6345_write(struct udevice *dev, unsigned addr_off,
+                      unsigned char reg_addr, unsigned char value)
+{
+     uint8_t buf[2];
+     struct i2c_msg msg;
+     int ret;
+
+     msg.addr = addr_off;
+     msg.flags = 0;
+     buf[0] = reg_addr;
+     buf[1] = value;
+     msg.buf = buf;
+     msg.len = 2;
+     ret = dm_i2c_xfer(dev, &msg, 1);
+     if (ret) {
+             debug("%s: write failed, reg=%#x, value=%#x, ret=%d\n",
+                   __func__, reg_addr, value, ret);
+             return ret;
+     }
+
+     return 0;
+}
+
+static int anx6345_read(struct udevice *dev, unsigned addr_off,
+                     unsigned char reg_addr, unsigned char *value)
+{
+     uint8_t addr, val;
+     struct i2c_msg msg[2];
+     int ret;
+
+     msg[0].addr = addr_off;
+     msg[0].flags = 0;
+     addr = reg_addr;
+     msg[0].buf = &addr;
+     msg[0].len = 1;
+     msg[1].addr = addr_off;
+     msg[1].flags = I2C_M_RD;
+     msg[1].buf = &val;
+     msg[1].len = 1;
+     ret = dm_i2c_xfer(dev, msg, 2);
+     if (ret) {
+             debug("%s: read failed, reg=%.2x, value=%p, ret=%d\n",
+                   __func__, (int)reg_addr, value, ret);
+             return ret;
+     }
+     *value = val;
+
+     return 0;
+}
+
+static int anx6345_write_r0(struct udevice *dev, unsigned char reg_addr,
+                         unsigned char value)
+{
+     struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+     return anx6345_write(dev, chip->chip_addr, reg_addr, value);
+}
+
+static int anx6345_read_r0(struct udevice *dev, unsigned char reg_addr,
+                        unsigned char *value)
+{
+     struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+     return anx6345_read(dev, chip->chip_addr, reg_addr, value);
+}
+
+static int anx6345_write_r1(struct udevice *dev, unsigned char reg_addr,
+                         unsigned char value)
+{
+     struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+     return anx6345_write(dev, chip->chip_addr + 1, reg_addr, value);
+}
+
+static int anx6345_read_r1(struct udevice *dev, unsigned char reg_addr,
+                        unsigned char *value)
+{
+     struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+     return anx6345_read(dev, chip->chip_addr + 1, reg_addr, value);
+}
+
+static int anx6345_set_backlight(struct udevice *dev, int percent)
+{
+     return -ENOSYS;
+}
+
+static int anx6345_aux_wait(struct udevice *dev)
+{
+     int ret = -ETIMEDOUT;
+     u8 v;
+     int retries = 1000;
+     do {
+             anx6345_read_r0(dev, ANX9804_DP_AUX_CH_CTL_2, &v);
+             if (!(v & ANX9804_AUX_EN)) {
+                     ret = 0;
+                     break;
+             }
+             udelay(100);
+     } while (retries--);
+
+     if (ret) {
+             debug("%s: timed out waiting for AUX_EN to clear\n", __func__);
+             return ret;
+     }
+
+     ret = -ETIMEDOUT;
+     retries = 1000;
+     do {
+             anx6345_read_r1(dev, ANX9804_DP_INT_STA, &v);
+             if (v & ANX9804_RPLY_RECEIV) {
+                     ret = 0;
+                     break;
+             }
+             udelay(100);
+     } while (retries--);
+
+     if (ret) {
+             debug("%s: timed out waiting to receive reply\n", __func__);
+             return ret;
+     }
+
+     /* Clear RPLY_RECEIV bit */
+     anx6345_write_r1(dev, ANX9804_DP_INT_STA, v);
+
+     anx6345_read_r0(dev, ANX9804_AUX_CH_STA, &v);
+     if ((v & ANX9804_AUX_STATUS_MASK) != 0) {
+             debug("AUX status: %d\n", v & ANX9804_AUX_STATUS_MASK);
+             ret = -EIO;
+     }
+
+     return ret;
+}
+
+static void anx6345_aux_addr(struct udevice *dev, u32 addr)
+{
+     u8 val;
+
+     val = addr & 0xff;
+     anx6345_write_r0(dev, ANX9804_DP_AUX_ADDR_7_0, val);
+     val = (addr >> 8) & 0xff;
+     anx6345_write_r0(dev, ANX9804_DP_AUX_ADDR_15_8, val);
+     val = (addr >> 16) & 0x0f;
+     anx6345_write_r0(dev, ANX9804_DP_AUX_ADDR_19_16, val);
+}
+
+static int anx6345_aux_transfer(struct udevice *dev, u8 req, u32 addr, u8 *buf, size_t len)
+{
+     int i, ret;
+     u8 ctrl1 = req;
+     u8 ctrl2 = ANX9804_AUX_EN;
+
+     if (len > 16)
+             return -E2BIG;
+
+     if (len)
+             ctrl1 |= ANX9804_AUX_LENGTH(len);
+     else
+             ctrl2 |= ANX9804_ADDR_ONLY;
+
+     if (len && !(req & ANX9804_AUX_TX_COMM_READ)) {
+             for (i = 0; i < len; i++)
+                     anx6345_write_r0(dev, ANX9804_BUF_DATA_0 + i, buf[i]);
+     }
+
+     anx6345_aux_addr(dev, addr);
+     anx6345_write_r0(dev, ANX9804_DP_AUX_CH_CTL_1, ctrl1);
+     anx6345_write_r0(dev, ANX9804_DP_AUX_CH_CTL_2, ctrl2);
+     ret = anx6345_aux_wait(dev);
+     if (ret) {
+             debug("AUX transaction timed out\n");
+             return ret;
+     }
+
+     if (len && (req & ANX9804_AUX_TX_COMM_READ)) {
+             for (i = 0; i < len; i++)
+                     anx6345_read_r0(dev, ANX9804_BUF_DATA_0 + i, &buf[i]);
+     }
+
+     return 0;
+
+}
+
+static int anx6345_read_aux_i2c(struct udevice *dev, u8 chip_addr,
+                             u8 offset,
+                             size_t count,
+                             u8 *buf)
+{
+     int i, ret;
+     size_t cur_cnt;
+     u8 cur_offset;
+     for (i = 0; i < count; i += 16) {
+             cur_cnt = (count - i) > 16 ? 16 : count - i;
+             cur_offset = offset + i;
+             ret = anx6345_aux_transfer(dev, ANX9804_AUX_TX_COMM_MOT,
+                                        chip_addr, &cur_offset, 1);
+             if (ret) {
+                     debug("%s: failed to set i2c offset: %d\n", __func__, ret);
+                     return ret;
+             }
+             ret = anx6345_aux_transfer(dev, ANX9804_AUX_TX_COMM_READ,
+                                        chip_addr, buf + i, cur_cnt);
+             if (ret) {
+                     debug("%s: failed to read from i2c device: %d\n", __func__, ret);
+                     return ret;
+             }
+     }
+
+     return 0;
+}
+
+static int anx6345_read_dpcd(struct udevice *dev, u32 reg, u8 *val)
+{
+     int ret;
+
+     ret = anx6345_aux_transfer(dev,
+                                ANX9804_AUX_TX_COMM_READ |
+                                ANX9804_AUX_TX_COMM_DP_TRANSACTION,
+                                reg, val, 1);
+     if (ret) {
+             debug ("Failed to read DPCD\n");
+             return ret;
+     }
+
+     return 0;
+}
+
+static int anx6345_read_edid(struct udevice *dev, u8 *buf, int size)
+{
+     struct anx6345_priv *priv = dev_get_priv(dev);
+
+     if (size > EDID_SIZE)
+             size = EDID_SIZE;
+     memcpy(buf, priv->edid, size);
+
+     return size;
+}
+
+static int anx6345_attach(struct udevice *dev)
+{
+     /* No-op */
+     return 0;
+}
+
+static int anx6345_enable(struct udevice *dev)
+{
+     u8 chipid, colordepth, lanes, data_rate, c;
+     int ret, i, bpp;
+     struct display_timing timing;
+     struct anx6345_priv *priv = dev_get_priv(dev);
+
+     /* Deassert reset and enable power */
+     ret = video_bridge_set_active(dev, true);
+     if (ret)
+             return ret;
+
+     /* Reset */
+     anx6345_write_r1(dev, ANX9804_RST_CTRL_REG, 1);
+     mdelay(100);
+     anx6345_write_r1(dev, ANX9804_RST_CTRL_REG, 0);
+
+     /* Write 0 to the powerdown reg (powerup everything) */
+     anx6345_write_r1(dev, ANX9804_POWERD_CTRL_REG, 0);
+
+     ret = anx6345_read_r1(dev, ANX9804_DEV_IDH_REG, &chipid);
+     if (ret)
+             debug("%s: read id failed: %d\n", __func__, ret);
+
+     switch (chipid) {
+     case 0x63:
+             debug("ANX63xx detected.\n");
+             break;
+     default:
+             debug("Error anx6345 chipid mismatch: %.2x\n", (int)chipid);
+             return -ENODEV;
+     }
+
+     for (i = 0; i < 100; i++) {
+             anx6345_read_r0(dev, ANX9804_SYS_CTRL2_REG, &c);
+             anx6345_write_r0(dev, ANX9804_SYS_CTRL2_REG, c);
+             anx6345_read_r0(dev, ANX9804_SYS_CTRL2_REG, &c);
+             if ((c & ANX9804_SYS_CTRL2_CHA_STA) == 0)
+                     break;
+
+             mdelay(5);
+     }
+     if (i == 100)
+             debug("Error anx6345 clock is not stable\n");
+
+     /* Set a bunch of analog related register values */
+     anx6345_write_r0(dev, ANX9804_PLL_CTRL_REG, 0x00);
+     anx6345_write_r1(dev, ANX9804_ANALOG_DEBUG_REG1, 0x70);
+     anx6345_write_r0(dev, ANX9804_LINK_DEBUG_REG, 0x30);
+
+     /* Force HPD */
+     anx6345_write_r0(dev, ANX9804_SYS_CTRL3_REG,
+                   ANX9804_SYS_CTRL3_F_HPD | ANX9804_SYS_CTRL3_HPD_CTRL);
+
+     /* Power up and configure lanes */
+     anx6345_write_r0(dev, ANX9804_ANALOG_POWER_DOWN_REG, 0x00);
+     anx6345_write_r0(dev, ANX9804_TRAINING_LANE0_SET_REG, 0x00);
+     anx6345_write_r0(dev, ANX9804_TRAINING_LANE1_SET_REG, 0x00);
+     anx6345_write_r0(dev, ANX9804_TRAINING_LANE2_SET_REG, 0x00);
+     anx6345_write_r0(dev, ANX9804_TRAINING_LANE3_SET_REG, 0x00);
+
+     /* Reset AUX CH */
+     anx6345_write_r1(dev, ANX9804_RST_CTRL2_REG,
+                   ANX9804_RST_CTRL2_AUX);
+     anx6345_write_r1(dev, ANX9804_RST_CTRL2_REG, 0);
+
+     /* Powerdown audio and some other unused bits */
+     anx6345_write_r1(dev, ANX9804_POWERD_CTRL_REG, ANX9804_POWERD_AUDIO);
+     anx6345_write_r0(dev, ANX9804_HDCP_CONTROL_0_REG, 0x00);
+     anx6345_write_r0(dev, 0xa7, 0x00);
+
+     anx6345_read_aux_i2c(dev, 0x50, 0x0, EDID_SIZE, priv->edid);
+     if (edid_get_timing(priv->edid, EDID_SIZE, &timing, &bpp) != 0) {
+             debug("Failed to parse EDID\n");
+             return -EIO;
+     }
+     debug("%s: panel found: %dx%d, bpp %d\n", __func__,
+             timing.hactive.typ, timing.vactive.typ,
+             bpp);
+     if (bpp == 6)
+             colordepth = 0x00; /* 6 bit */
+     else
+             colordepth = 0x10; /* 8 bit */
+     anx6345_write_r1(dev, ANX9804_VID_CTRL2_REG, colordepth);
+
+     if (anx6345_read_dpcd(dev, DP_MAX_LINK_RATE, &data_rate)) {
+             debug("%s: Failed to DP_MAX_LINK_RATE\n", __func__);
+             return -EIO;
+     }
+     debug("%s: data_rate: %d\n", __func__, (int)data_rate);
+     if (anx6345_read_dpcd(dev, DP_MAX_LANE_COUNT, &lanes)) {
+             debug("%s: Failed to read DP_MAX_LANE_COUNT\n", __func__);
+             return -EIO;
+     }
+     lanes &= DP_MAX_LANE_COUNT_MASK;
+     debug("%s: lanes: %d\n", __func__, (int)lanes);
+
+     /* Set data-rate / lanes */
+     anx6345_write_r0(dev, ANX9804_LINK_BW_SET_REG, data_rate);
+     anx6345_write_r0(dev, ANX9804_LANE_COUNT_SET_REG, lanes);
+
+     /* Link training */
+     anx6345_write_r0(dev, ANX9804_LINK_TRAINING_CTRL_REG,
+                   ANX9804_LINK_TRAINING_CTRL_EN);
+     mdelay(5);
+     for (i = 0; i < 100; i++) {
+             anx6345_read_r0(dev, ANX9804_LINK_TRAINING_CTRL_REG, &c);
+             if ((chipid == 0x63) && (c & 0x80) == 0)
+                     break;
+
+             mdelay(5);
+     }
+     if(i == 100) {
+             debug("Error anx6345 link training timeout\n");
+             return -ENODEV;
+     }
+
+     /* Enable */
+     anx6345_write_r1(dev, ANX9804_VID_CTRL1_REG,
+                   ANX9804_VID_CTRL1_VID_EN | ANX9804_VID_CTRL1_EDGE);
+     /* Force stream valid */
+     anx6345_write_r0(dev, ANX9804_SYS_CTRL3_REG,
+                   ANX9804_SYS_CTRL3_F_HPD | ANX9804_SYS_CTRL3_HPD_CTRL |
+                   ANX9804_SYS_CTRL3_F_VALID | ANX9804_SYS_CTRL3_VALID_CTRL);
+
+     return 0;
+}
+
+static int anx6345_probe(struct udevice *dev)
+{
+     if (device_get_uclass_id(dev->parent) != UCLASS_I2C)
+             return -EPROTONOSUPPORT;
+
+     return anx6345_enable(dev);
+}
+
+struct video_bridge_ops anx6345_ops = {
+     .attach = anx6345_attach,
+     .set_backlight = anx6345_set_backlight,
+     .read_edid = anx6345_read_edid,
+};
+
+static const struct udevice_id anx6345_ids[] = {
+     { .compatible = "analogix,anx6345", },
+     { }
+};
+
+U_BOOT_DRIVER(analogix_anx6345) = {
+     .name   = "analogix_anx6345",
+     .id     = UCLASS_VIDEO_BRIDGE,
+     .of_match = anx6345_ids,
+     .probe  = anx6345_probe,
+     .ops    = &anx6345_ops,
+     .priv_auto_alloc_size = sizeof(struct anx6345_priv),
+};

Extend DE2 driver with LCD support

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 arch/arm/mach-sunxi/Kconfig     |   2 +-
 drivers/video/sunxi/Makefile    |   2 +-
 drivers/video/sunxi/sunxi_de2.c |  17 +++++
 drivers/video/sunxi/sunxi_lcd.c | 142 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/sunxi/sunxi_lcd.c

arch/arm/mach-sunxi/Kconfig +1 -1

index 2309f59999..06d697e3a7 100644
@@ -680,7 +680,7 @@ config VIDEO_LCD_MODE
 
 config VIDEO_LCD_DCLK_PHASE
 	int "LCD panel display clock phase"
-	depends on VIDEO
+	depends on VIDEO || DM_VIDEO
 	default 1
 	---help---
 	Select LCD panel display clock phase shift, range 0-3.

drivers/video/sunxi/Makefile +1 -1

index 0d64c2021f..8c91766c24 100644
@@ -6,4 +6,4 @@
 #
 
 obj-$(CONFIG_VIDEO_SUNXI) += sunxi_display.o lcdc.o tve_common.o ../videomodes.o
-obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o
+obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o sunxi_lcd.o

drivers/video/sunxi/sunxi_de2.c +1 -0

index ee67764ac5..a838bbacd1 100644
@@ -232,6 +232,23 @@ static int sunxi_de2_probe(struct udevice *dev)
 	if (!(gd->flags & GD_FLG_RELOC))
 		return 0;
 
+	ret = uclass_find_device_by_name(UCLASS_DISPLAY,

drivers/video/sunxi/sunxi_de2.c +6 -0

+			return 0;
+		}
+	}
+
+	debug("%s: lcd display not found (ret=%d)\n", __func__, ret);
+
 	ret = uclass_find_device_by_name(UCLASS_DISPLAY,
 					 "sunxi_dw_hdmi", &disp);
 	if (!ret) {

drivers/video/sunxi/sunxi_lcd.c new file +40 -0

new file mode 100644
index 0000000000..154eb5835e
@@ -0,0 +1,142 @@
+/*
+ * Allwinner LCD driver
+ *
+ * (C) Copyright 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <display.h>
+#include <video_bridge.h>
+#include <backlight.h>
+#include <dm.h>
+#include <edid.h>
+#include <asm/io.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/lcdc.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+
+struct sunxi_lcd_priv {
+	struct display_timing timing;
+	int panel_bpp;
+};
+
+static void sunxi_lcdc_config_pinmux(void)
+{
+	int pin;
+	for (pin = SUNXI_GPD(0); pin <= SUNXI_GPD(21); pin++) {
+		sunxi_gpio_set_cfgpin(pin, SUNXI_GPD_LCD0);
+		sunxi_gpio_set_drv(pin, 3);
+	}
+}
+
+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+	struct sunxi_ccm_reg * const ccm =
+	       (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+	struct sunxi_lcdc_reg * const lcdc =

drivers/video/sunxi/sunxi_lcd.c new file +20 -0

+	lcdc_pll_set(ccm, 0, edid->pixelclock.typ / 1000,
+	             &clk_div, &clk_double);
+	lcdc_tcon0_mode_set(lcdc, edid, clk_div, false,
+	                    priv->panel_bpp, CONFIG_VIDEO_LCD_DCLK_PHASE);
+	lcdc_enable(lcdc, priv->panel_bpp);
+
+	ret = uclass_get_device(UCLASS_PANEL_BACKLIGHT, 0, &backlight);
+	if (!ret)
+		backlight_enable(backlight);
+
+	return 0;
+}
+
+static int sunxi_lcd_read_timing(struct udevice *dev,
+                                struct display_timing *timing)
+{
+	struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+	memcpy(timing, &priv->timing, sizeof(struct display_timing));
+
+	return 0;

drivers/video/sunxi/sunxi_lcd.c new file +26 -0

+
+#ifdef CONFIG_VIDEO_BRIDGE
+	/* Try to get timings from bridge first */
+	ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &cdev);
+	if (!ret) {
+		u8 edid[EDID_SIZE];
+		int channel_bpp;
+
+		ret = video_bridge_attach(cdev);
+		if (ret) {
+			debug("video bridge attach failed: %d\n", ret);
+			return ret;
+		}
+		ret = video_bridge_read_edid(cdev, edid, EDID_SIZE);
+		if (ret <= 0) {
+			debug("video bridge failed to read edid: %d\n", ret);
+			return ret ? ret : -EIO;
+		}
+		ret = edid_get_timing(edid, ret, &priv->timing, &channel_bpp);
+		priv->panel_bpp = channel_bpp * 3;
+		return ret;
+	}
+	debug("video bridge not found: %d\n", ret);
+#endif
+	/* Fallback to timings from DT if there's no bridge */
+	ret = uclass_get_device(UCLASS_PANEL, 0, &cdev);

drivers/video/sunxi/sunxi_lcd.c new file +9 -0

+	priv->panel_bpp = 16;
+
+	return 0;
+}
+
+static const struct dm_display_ops sunxi_lcd_ops = {
+       .read_timing = sunxi_lcd_read_timing,
+       .enable = sunxi_lcd_enable,
+};

Quoted 1 line · Vasily Khoruzhick

Extend DE2 driver with LCD support

(All) your commit messages could use a bit more details.

Here, for example, explaining the following things would help:
- Why are you creating yet another file

- What is the situation with old Allwinner SoCs that should share the same code

- What are the expected users

- Which SoC / board have you tested it on

etc...

Quoted 40 lines

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 arch/arm/mach-sunxi/Kconfig     |   2 +-
 drivers/video/sunxi/Makefile    |   2 +-
 drivers/video/sunxi/sunxi_de2.c |  17 +++++
 drivers/video/sunxi/sunxi_lcd.c | 142 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/sunxi/sunxi_lcd.c

arch/arm/mach-sunxi/Kconfig +1 -1

index 2309f59999..06d697e3a7 100644
@@ -680,7 +680,7 @@ config VIDEO_LCD_MODE
 
 config VIDEO_LCD_DCLK_PHASE
      int "LCD panel display clock phase"
-     depends on VIDEO
+     depends on VIDEO || DM_VIDEO
      default 1
      ---help---
      Select LCD panel display clock phase shift, range 0-3.

drivers/video/sunxi/Makefile +1 -1

index 0d64c2021f..8c91766c24 100644
@@ -6,4 +6,4 @@
 #
 
 obj-$(CONFIG_VIDEO_SUNXI) += sunxi_display.o lcdc.o tve_common.o ../videomodes.o
-obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o
+obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o sunxi_lcd.o

drivers/video/sunxi/sunxi_de2.c +0 -0

index ee67764ac5..a838bbacd1 100644
@@ -232,6 +232,23 @@ static int sunxi_de2_probe(struct udevice *dev)
      if (!(gd->flags & GD_FLG_RELOC))
              return 0;
 

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev) · line 245

Quoted 11 lines

drivers/video/sunxi/sunxi_de2.c +11 -0

+     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
+                                      "sunxi_lcd", &disp);
+     if (!ret) {
+             int mux;
+
+             mux = 0;
+
+             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
+                                  false);
+             if (!ret) {
+                     video_set_flush_dcache(dev, 1);

Why do you need to flush the dcache here?

Quoted 61 lines

drivers/video/sunxi/sunxi_de2.c +6 -0

+                     return 0;
+             }
+     }
+
+     debug("%s: lcd display not found (ret=%d)\n", __func__, ret);
+
      ret = uclass_find_device_by_name(UCLASS_DISPLAY,
                                       "sunxi_dw_hdmi", &disp);
      if (!ret) {

drivers/video/sunxi/sunxi_lcd.c new file +45 -0

new file mode 100644
index 0000000000..154eb5835e
@@ -0,0 +1,142 @@
+/*
+ * Allwinner LCD driver
+ *
+ * (C) Copyright 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:  GPL-2.0+
+ */
+
+#include <common.h>
+#include <display.h>
+#include <video_bridge.h>
+#include <backlight.h>
+#include <dm.h>
+#include <edid.h>
+#include <asm/io.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/lcdc.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+
+struct sunxi_lcd_priv {
+     struct display_timing timing;
+     int panel_bpp;
+};
+
+static void sunxi_lcdc_config_pinmux(void)
+{
+     int pin;
+     for (pin = SUNXI_GPD(0); pin <= SUNXI_GPD(21); pin++) {
+             sunxi_gpio_set_cfgpin(pin, SUNXI_GPD_LCD0);
+             sunxi_gpio_set_drv(pin, 3);
+     }
+}
+
+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+     struct sunxi_ccm_reg * const ccm =
+            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+     struct sunxi_lcdc_reg * const lcdc =
+            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     struct udevice *backlight;
+     int clk_div, clk_double, ret;
+

drivers/video/sunxi/sunxi_lcd.c line 50

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+     /* Reset off */
+     setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
+
+     /* Clock on */
+     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);

This has nothing to do with using a panel or not, it should be in lcdc_init().

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Quoted 24 lines

drivers/video/sunxi/sunxi_lcd.c new file +24 -0

+     lcdc_pll_set(ccm, 0, edid->pixelclock.typ / 1000,
+                  &clk_div, &clk_double);
+     lcdc_tcon0_mode_set(lcdc, edid, clk_div, false,
+                         priv->panel_bpp, CONFIG_VIDEO_LCD_DCLK_PHASE);
+     lcdc_enable(lcdc, priv->panel_bpp);
+
+     ret = uclass_get_device(UCLASS_PANEL_BACKLIGHT, 0, &backlight);
+     if (!ret)
+             backlight_enable(backlight);
+
+     return 0;
+}
+
+static int sunxi_lcd_read_timing(struct udevice *dev,
+                                struct display_timing *timing)
+{
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     memcpy(timing, &priv->timing, sizeof(struct display_timing));
+
+     return 0;
+}
+
+static int sunxi_lcd_probe(struct udevice *dev)
+{

drivers/video/sunxi/sunxi_lcd.c line 83

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     struct udevice *cdev;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     int ret;
+
+     /* make sure that clock is active */
+     clock_set_pll10(432000000);

Why do you need it active, and why at that rate?

Quoted 31 lines

drivers/video/sunxi/sunxi_lcd.c new file +31 -0

+
+#ifdef CONFIG_VIDEO_BRIDGE
+     /* Try to get timings from bridge first */
+     ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &cdev);
+     if (!ret) {
+             u8 edid[EDID_SIZE];
+             int channel_bpp;
+
+             ret = video_bridge_attach(cdev);
+             if (ret) {
+                     debug("video bridge attach failed: %d\n", ret);
+                     return ret;
+             }
+             ret = video_bridge_read_edid(cdev, edid, EDID_SIZE);
+             if (ret <= 0) {
+                     debug("video bridge failed to read edid: %d\n", ret);
+                     return ret ? ret : -EIO;
+             }
+             ret = edid_get_timing(edid, ret, &priv->timing, &channel_bpp);
+             priv->panel_bpp = channel_bpp * 3;
+             return ret;
+     }
+     debug("video bridge not found: %d\n", ret);
+#endif
+     /* Fallback to timings from DT if there's no bridge */
+     ret = uclass_get_device(UCLASS_PANEL, 0, &cdev);
+     if (ret) {
+             debug("video panel not found: %d\n", ret);
+             return ret;
+     }
+

drivers/video/sunxi/sunxi_lcd.c line 119

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+     if (fdtdec_decode_display_timing(gd->fdt_blob, dev_of_offset(cdev),
+                                      0, &priv->timing)) {
+             debug("%s: Failed to decode display timing\n", __func__);
+             return -EINVAL;
+     }

How does that work with simple-panel style bindings that are most of the panels merged these days?

Quoted 14 lines

drivers/video/sunxi/sunxi_lcd.c new file +13 -0

+     priv->panel_bpp = 16;
+
+     return 0;
+}
+
+static const struct dm_display_ops sunxi_lcd_ops = {
+       .read_timing = sunxi_lcd_read_timing,
+       .enable = sunxi_lcd_enable,
+};
+
+U_BOOT_DRIVER(sunxi_lcd) = {
+     .name   = "sunxi_lcd",
+     .id     = UCLASS_DISPLAY,

drivers/video/sunxi/sunxi_lcd.c line 138

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     .ops    = &sunxi_lcd_ops,
+     .probe  = sunxi_lcd_probe,
+     .priv_auto_alloc_size = sizeof(struct sunxi_lcd_priv),
+};
+
+#ifdef CONFIG_MACH_SUN50I

Why do you restrict it to the A64 here?

Thanks,
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

On Tue, Sep 19, 2017 at 1:33 AM, Maxime Ripard

Quoted 4 lines · <[email protected]> wrote:

Quoted 1 line · Vasily Khoruzhick

Extend DE2 driver with LCD support

(All) your commit messages could use a bit more details.

OK, will add in v2.

Quoted 2 lines

Here, for example, explaining the following things would help:
- Why are you creating yet another file

Are you talking about any specific file? I guess adding another driver justifies creation of another file.

Quoted 3 lines

- What is the situation with old Allwinner SoCs that should share

the same code

As far as I can tell, DE2 is present in H3, V3s, A64 and newer. LCD is supported in A64 only. I.e. hardware is not present
in H3 or V3s

Quoted 1 line

- What are the expected users

Pinebook

Quoted 1 line

- Which SoC / board have you tested it on

A64 / Pinebook

Quoted 53 lines

etc...

Quoted 51 lines

Signed-off-by Vasily Khoruzhick <[email protected]>

---

 arch/arm/mach-sunxi/Kconfig     |   2 +-
 drivers/video/sunxi/Makefile    |   2 +-
 drivers/video/sunxi/sunxi_de2.c |  17 +++++
 drivers/video/sunxi/sunxi_lcd.c | 142
 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 161
 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/sunxi/sunxi_lcd.c

arch/arm/mach-sunxi/Kconfig +1 -1

index 2309f59999..06d697e3a7 100644
@@ -680,7 +680,7 @@ config VIDEO_LCD_MODE
 
 config VIDEO_LCD_DCLK_PHASE
 
      int "LCD panel display clock phase"
 
-     depends on VIDEO
+     depends on VIDEO || DM_VIDEO
 

default 1
---help---
Select LCD panel display clock phase shift, range 0-3.

drivers/video/sunxi/Makefile +0 -0

index 0d64c2021f..8c91766c24 100644
@@ -6,4 +6,4 @@
 
 #
 
 obj-$(CONFIG_VIDEO_SUNXI) += sunxi_display.o lcdc.o tve_common.o

../videomodes.o>>
-obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o
+obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o sunxi_lcd.o diff --git a/drivers/video/sunxi/sunxi_de2.c b/drivers/video/sunxi/sunxi_de2.c index ee67764ac5..a838bbacd1 100644

drivers/video/sunxi/sunxi_de2.c +0 -0

@@ -232,6 +232,23 @@ static int sunxi_de2_probe(struct udevice *dev)
 
      if (!(gd->flags & GD_FLG_RELOC))
 
              return 0;
 

Quoted 14 lines

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev) · line 245

Quoted 12 lines

drivers/video/sunxi/sunxi_de2.c +11 -0

+     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
+                                      "sunxi_lcd", &disp);
+     if (!ret) {
+             int mux;
+
+             mux = 0;
+
+             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp,
 mux,
+                                  false);
+             if (!ret) {
+                     video_set_flush_dcache(dev, 1);

Why do you need to flush the dcache here?

Copied from HDMI driver init. If it's not necessary why it's here for HDMI?

Quoted 64 lines

Quoted 64 lines

drivers/video/sunxi/sunxi_de2.c +6 -0

+                     return 0;
+             }
+     }
+
+     debug("%s: lcd display not found (ret=%d)\n", __func__, ret);
+

      ret = uclass_find_device_by_name(UCLASS_DISPLAY,
      
                                       "sunxi_dw_hdmi", &disp);
      
      if (!ret) {

(quoted diff) +0 -0

b/drivers/video/sunxi/sunxi_lcd.c new file mode 100644 index 0000000000..154eb5835e

drivers/video/sunxi/sunxi_lcd.c new file +45 -0

@@ -0,0 +1,142 @@
+/*
+ * Allwinner LCD driver
+ *
+ * (C) Copyright 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:  GPL-2.0+
+ */
+
+#include <common.h>
+#include <display.h>
+#include <video_bridge.h>
+#include <backlight.h>
+#include <dm.h>
+#include <edid.h>
+#include <asm/io.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/lcdc.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+
+struct sunxi_lcd_priv {
+     struct display_timing timing;
+     int panel_bpp;
+};
+
+static void sunxi_lcdc_config_pinmux(void)
+{
+     int pin;
+     for (pin = SUNXI_GPD(0); pin <= SUNXI_GPD(21); pin++) {
+             sunxi_gpio_set_cfgpin(pin, SUNXI_GPD_LCD0);
+             sunxi_gpio_set_drv(pin, 3);
+     }
+}
+
+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+     struct sunxi_ccm_reg * const ccm =
+            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+     struct sunxi_lcdc_reg * const lcdc =
+            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     struct udevice *backlight;
+     int clk_div, clk_double, ret;
+

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 50

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+     /* Reset off */
+     setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
+
+     /* Clock on */
+     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);

This has nothing to do with using a panel or not, it should be in lcdc_init().

Why? We don't need neither take it out of reset nor turn the clock on it if LCD is not used (e.g. HDMI-only case).

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Quoted 24 lines

Quoted 24 lines

drivers/video/sunxi/sunxi_lcd.c new file +24 -0

+     lcdc_pll_set(ccm, 0, edid->pixelclock.typ / 1000,
+                  &clk_div, &clk_double);
+     lcdc_tcon0_mode_set(lcdc, edid, clk_div, false,
+                         priv->panel_bpp, CONFIG_VIDEO_LCD_DCLK_PHASE);
+     lcdc_enable(lcdc, priv->panel_bpp);
+
+     ret = uclass_get_device(UCLASS_PANEL_BACKLIGHT, 0, &backlight);
+     if (!ret)
+             backlight_enable(backlight);
+
+     return 0;
+}
+
+static int sunxi_lcd_read_timing(struct udevice *dev,
+                                struct display_timing *timing)
+{
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     memcpy(timing, &priv->timing, sizeof(struct display_timing));
+
+     return 0;
+}
+
+static int sunxi_lcd_probe(struct udevice *dev)
+{

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 83

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     struct udevice *cdev;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     int ret;
+
+     /* make sure that clock is active */
+     clock_set_pll10(432000000);

Why do you need it active, and why at that rate?

Will check.

Quoted 32 lines

Quoted 32 lines

drivers/video/sunxi/sunxi_lcd.c new file +16 -0

+
+#ifdef CONFIG_VIDEO_BRIDGE
+     /* Try to get timings from bridge first */
+     ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &cdev);
+     if (!ret) {
+             u8 edid[EDID_SIZE];
+             int channel_bpp;
+
+             ret = video_bridge_attach(cdev);
+             if (ret) {
+                     debug("video bridge attach failed: %d\n", ret);
+                     return ret;
+             }
+             ret = video_bridge_read_edid(cdev, edid, EDID_SIZE);
+             if (ret <= 0) {
+                     debug("video bridge failed to read edid: %d\n",

ret);

drivers/video/sunxi/sunxi_lcd.c new file +3 -0

+                     return ret ? ret : -EIO;
+             }
+             ret = edid_get_timing(edid, ret, &priv->timing,

&channel_bpp); + priv->panel_bpp = channel_bpp * 3;

drivers/video/sunxi/sunxi_lcd.c new file +11 -0

+             return ret;
+     }
+     debug("video bridge not found: %d\n", ret);
+#endif
+     /* Fallback to timings from DT if there's no bridge */
+     ret = uclass_get_device(UCLASS_PANEL, 0, &cdev);
+     if (ret) {
+             debug("video panel not found: %d\n", ret);
+             return ret;
+     }
+

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 119

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+     if (fdtdec_decode_display_timing(gd->fdt_blob, dev_of_offset(cdev),
+                                      0, &priv->timing)) {
+             debug("%s: Failed to decode display timing\n", __func__);
+             return -EINVAL;
+     }

How does that work with simple-panel style bindings that are most of the panels merged these days?

It should just work, but I haven't even tested this part of the code - I don't have a panel with parallel interface nor A64 board to connect it to.
Do you want me to drop it and make this driver dependent on CONFIG_VIDEO_BRIDGE

Quoted 13 lines

Quoted 13 lines

drivers/video/sunxi/sunxi_lcd.c new file +13 -0

+     priv->panel_bpp = 16;
+
+     return 0;
+}
+
+static const struct dm_display_ops sunxi_lcd_ops = {
+       .read_timing = sunxi_lcd_read_timing,
+       .enable = sunxi_lcd_enable,
+};
+
+U_BOOT_DRIVER(sunxi_lcd) = {
+     .name   = "sunxi_lcd",
+     .id     = UCLASS_DISPLAY,

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 138

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     .ops    = &sunxi_lcd_ops,
+     .probe  = sunxi_lcd_probe,
+     .priv_auto_alloc_size = sizeof(struct sunxi_lcd_priv),
+};
+
+#ifdef CONFIG_MACH_SUN50I

Why do you restrict it to the A64 here?

Because the hardware is present in A64 only.

Quoted 4 lines · Maxime Ripard

Quoted 1 line · Vasily Khoruzhick

Extend DE2 driver with LCD support

(All) your commit messages could use a bit more details.

OK, will add in v2.

Quoted 2 lines

Here, for example, explaining the following things would help:
- Why are you creating yet another file

Are you talking about any specific file? I guess adding another driver justifies creation of another file.

Quoted 2 lines

- What is the situation with old Allwinner SoCs that should share the same code

As far as I can tell, DE2 is present in H3, V3s, A64 and newer. LCD is supported in A64 only. I.e. hardware is not present
in H3 or V3s

Quoted 2 lines

- What are the expected users

Pinebook

Quoted 1 line

- Which SoC / board have you tested it on

A64 / Pinebook

Quoted 9 lines

etc...

Quoted 6 lines

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 arch/arm/mach-sunxi/Kconfig     |   2 +-
 drivers/video/sunxi/Makefile    |   2 +-
 drivers/video/sunxi/sunxi_de2.c |  17 +++++
 drivers/video/sunxi/sunxi_lcd.c | 142

++++++++++++++++++++++++++++++++++++++++

Quoted 4 lines

Review

Quoted 4 lines

 4 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/sunxi/sunxi_lcd.c

(quoted diff) +0 -0

b/arch/arm/mach-sunxi/Kconfig

Quoted 5 lines

Quoted 5 lines

index 2309f59999..06d697e3a7 100644

arch/arm/mach-sunxi/Kconfig +0 -0

@@ -680,7 +680,7 @@ config VIDEO_LCD_MODE
 

Quoted 8 lines

Review

Quoted 8 lines

arch/arm/mach-sunxi/Kconfig +1 -1

 config VIDEO_LCD_DCLK_PHASE
      int "LCD panel display clock phase"
-     depends on VIDEO
+     depends on VIDEO || DM_VIDEO
      default 1
      ---help---
      Select LCD panel display clock phase shift, range 0-3.

(quoted diff) +0 -0

b/drivers/video/sunxi/Makefile

Quoted 7 lines

drivers/video/sunxi/Makefile line 8

Quoted 7 lines

index 0d64c2021f..8c91766c24 100644

drivers/video/sunxi/Makefile +0 -0

@@ -6,4 +6,4 @@
 #
 
 obj-$(CONFIG_VIDEO_SUNXI) += sunxi_display.o lcdc.o tve_common.o

../videomodes.o

Quoted 1 line

drivers/video/sunxi/Makefile line 9

Quoted 1 line

drivers/video/sunxi/Makefile +0 -1

-obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o

../dw_hdmi.o

Quoted 1 line

drivers/video/sunxi/Makefile line 9

Quoted 1 line

drivers/video/sunxi/Makefile +1 -0

+obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o

../dw_hdmi.o sunxi_lcd.o

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/video/sunxi/sunxi_de2.c

Quoted 7 lines

Quoted 7 lines

index ee67764ac5..a838bbacd1 100644

drivers/video/sunxi/sunxi_de2.c +0 -0

@@ -232,6 +232,23 @@ static int sunxi_de2_probe(struct udevice *dev)
      if (!(gd->flags & GD_FLG_RELOC))
              return 0;
 

Quoted 8 lines

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev)

Quoted 8 lines

drivers/video/sunxi/sunxi_de2.c +8 -0

+     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
+                                      "sunxi_lcd", &disp);
+     if (!ret) {
+             int mux;
+
+             mux = 0;
+
+             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32,

disp, mux,

Quoted 5 lines

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev) · line 245

Quoted 3 lines

drivers/video/sunxi/sunxi_de2.c +3 -0

+                                  false);
+             if (!ret) {
+                     video_set_flush_dcache(dev, 1);

Why do you need to flush the dcache here?

Copied from HDMI driver init. If it's not necessary why it's here for HDMI?

Quoted 11 lines

Review

Quoted 11 lines

drivers/video/sunxi/sunxi_de2.c +6 -0

+                     return 0;
+             }
+     }
+
+     debug("%s: lcd display not found (ret=%d)\n", __func__, ret);
+
      ret = uclass_find_device_by_name(UCLASS_DISPLAY,
                                       "sunxi_dw_hdmi", &disp);
      if (!ret) {

(quoted diff) +0 -0

b/drivers/video/sunxi/sunxi_lcd.c

Quoted 44 lines

Quoted 44 lines

new file mode 100644
index 0000000000..154eb5835e

drivers/video/sunxi/sunxi_lcd.c new file +39 -0

@@ -0,0 +1,142 @@
+/*
+ * Allwinner LCD driver
+ *
+ * (C) Copyright 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:  GPL-2.0+
+ */
+
+#include <common.h>
+#include <display.h>
+#include <video_bridge.h>
+#include <backlight.h>
+#include <dm.h>
+#include <edid.h>
+#include <asm/io.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/lcdc.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+
+struct sunxi_lcd_priv {
+     struct display_timing timing;
+     int panel_bpp;
+};
+
+static void sunxi_lcdc_config_pinmux(void)
+{
+     int pin;
+     for (pin = SUNXI_GPD(0); pin <= SUNXI_GPD(21); pin++) {
+             sunxi_gpio_set_cfgpin(pin, SUNXI_GPD_LCD0);
+             sunxi_gpio_set_drv(pin, 3);
+     }
+}
+
+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+     struct sunxi_ccm_reg * const ccm =
+            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c new file +8 -0

+     struct sunxi_lcdc_reg * const lcdc =
+            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     struct udevice *backlight;
+     int clk_div, clk_double, ret;
+
+     /* Reset off */
+     setbits_le32(&ccm->ahb_reset1_cfg, 1 <<

AHB_RESET_OFFSET_LCD0);

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c line 50

Quoted 3 lines

drivers/video/sunxi/sunxi_lcd.c new file +3 -0

+
+     /* Clock on */
+     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);

This has nothing to do with using a panel or not, it should be in lcdc_init().

Why? We don't need neither take it out of reset nor turn the clock on it if LCD is not used (e.g. HDMI-only case).

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Quoted 4 lines

drivers/video/sunxi/sunxi_lcd.c

Quoted 4 lines

drivers/video/sunxi/sunxi_lcd.c new file +4 -0

+     lcdc_pll_set(ccm, 0, edid->pixelclock.typ / 1000,
+                  &clk_div, &clk_double);
+     lcdc_tcon0_mode_set(lcdc, edid, clk_div, false,
+                         priv->panel_bpp,

CONFIG_VIDEO_LCD_DCLK_PHASE);

Quoted 3 lines

drivers/video/sunxi/sunxi_lcd.c

Quoted 3 lines

drivers/video/sunxi/sunxi_lcd.c new file +3 -0

+     lcdc_enable(lcdc, priv->panel_bpp);
+
+     ret = uclass_get_device(UCLASS_PANEL_BACKLIGHT, 0,

&backlight);

Quoted 17 lines

Quoted 17 lines

drivers/video/sunxi/sunxi_lcd.c new file +17 -0

+     if (!ret)
+             backlight_enable(backlight);
+
+     return 0;
+}
+
+static int sunxi_lcd_read_timing(struct udevice *dev,
+                                struct display_timing *timing)
+{
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     memcpy(timing, &priv->timing, sizeof(struct display_timing));
+
+     return 0;
+}
+
+static int sunxi_lcd_probe(struct udevice *dev)
+{

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 83

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     struct udevice *cdev;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     int ret;
+
+     /* make sure that clock is active */
+     clock_set_pll10(432000000);

Why do you need it active, and why at that rate?

Will check.

Quoted 11 lines

drivers/video/sunxi/sunxi_lcd.c

Quoted 11 lines

drivers/video/sunxi/sunxi_lcd.c new file +11 -0

+
+#ifdef CONFIG_VIDEO_BRIDGE
+     /* Try to get timings from bridge first */
+     ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &cdev);
+     if (!ret) {
+             u8 edid[EDID_SIZE];
+             int channel_bpp;
+
+             ret = video_bridge_attach(cdev);
+             if (ret) {
+                     debug("video bridge attach failed: %d\n",

ret);

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+                     return ret;
+             }
+             ret = video_bridge_read_edid(cdev, edid, EDID_SIZE);
+             if (ret <= 0) {
+                     debug("video bridge failed to read edid:

%d\n", ret);

Quoted 3 lines

drivers/video/sunxi/sunxi_lcd.c

Quoted 3 lines

drivers/video/sunxi/sunxi_lcd.c new file +3 -0

+                     return ret ? ret : -EIO;
+             }
+             ret = edid_get_timing(edid, ret, &priv->timing,

&channel_bpp);

Quoted 5 lines

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+             priv->panel_bpp = channel_bpp * 3;
+             return ret;
+     }
+     debug("video bridge not found: %d\n", ret);
+#endif

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c new file +8 -0

+     /* Fallback to timings from DT if there's no bridge */
+     ret = uclass_get_device(UCLASS_PANEL, 0, &cdev);
+     if (ret) {
+             debug("video panel not found: %d\n", ret);
+             return ret;
+     }
+
+     if (fdtdec_decode_display_timing(gd->fdt_blob,

dev_of_offset(cdev),

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c line 116

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+                                      0, &priv->timing)) {
+             debug("%s: Failed to decode display timing\n",

__func__);

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c line 118

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+             return -EINVAL;
+     }

How does that work with simple-panel style bindings that are most of the panels merged these days?

It should just work, but I haven't even tested this part of the code - I don't have a panel with parallel interface nor A64 board to connect it to.
Do you want me to drop it and make this driver dependent on CONFIG_VIDEO_BRIDGE

Quoted 14 lines

Quoted 14 lines

drivers/video/sunxi/sunxi_lcd.c new file +13 -0

+     priv->panel_bpp = 16;
+
+     return 0;
+}
+
+static const struct dm_display_ops sunxi_lcd_ops = {
+       .read_timing = sunxi_lcd_read_timing,
+       .enable = sunxi_lcd_enable,
+};
+
+U_BOOT_DRIVER(sunxi_lcd) = {
+     .name   = "sunxi_lcd",
+     .id     = UCLASS_DISPLAY,

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 138

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     .ops    = &sunxi_lcd_ops,
+     .probe  = sunxi_lcd_probe,
+     .priv_auto_alloc_size = sizeof(struct sunxi_lcd_priv),
+};
+
+#ifdef CONFIG_MACH_SUN50I

Why do you restrict it to the A64 here?

Because the hardware is present in A64 only.

Quoted 7 lines

Thanks,
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Quoted 4 lines · Maxime Ripard

Quoted 1 line · Vasily Khoruzhick

Extend DE2 driver with LCD support

(All) your commit messages could use a bit more details.

OK, will add in v2.

Quoted 2 lines

Here, for example, explaining the following things would help:
- Why are you creating yet another file

Are you talking about any specific file? I guess adding another driver justifies creation of another file.

Quoted 2 lines

- What is the situation with old Allwinner SoCs that should share the same code

As far as I can tell, DE2 is present in H3, V3s, A64 and newer. LCD is supported in A64 only. I.e. hardware is not present
in H3 or V3s

Quoted 1 line

- What are the expected users

Pinebook

Quoted 1 line

- Which SoC / board have you tested it on

A64 / Pinebook

Quoted 43 lines

etc...

Quoted 40 lines

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 arch/arm/mach-sunxi/Kconfig     |   2 +-
 drivers/video/sunxi/Makefile    |   2 +-
 drivers/video/sunxi/sunxi_de2.c |  17 +++++
 drivers/video/sunxi/sunxi_lcd.c | 142 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/sunxi/sunxi_lcd.c

arch/arm/mach-sunxi/Kconfig +1 -1

index 2309f59999..06d697e3a7 100644
@@ -680,7 +680,7 @@ config VIDEO_LCD_MODE
 
 config VIDEO_LCD_DCLK_PHASE
      int "LCD panel display clock phase"
-     depends on VIDEO
+     depends on VIDEO || DM_VIDEO
      default 1
      ---help---
      Select LCD panel display clock phase shift, range 0-3.

drivers/video/sunxi/Makefile +1 -1

index 0d64c2021f..8c91766c24 100644
@@ -6,4 +6,4 @@
 #
 
 obj-$(CONFIG_VIDEO_SUNXI) += sunxi_display.o lcdc.o tve_common.o ../videomodes.o
-obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o
+obj-$(CONFIG_VIDEO_DE2) += sunxi_de2.o sunxi_dw_hdmi.o lcdc.o ../dw_hdmi.o sunxi_lcd.o

drivers/video/sunxi/sunxi_de2.c +0 -0

index ee67764ac5..a838bbacd1 100644
@@ -232,6 +232,23 @@ static int sunxi_de2_probe(struct udevice *dev)
      if (!(gd->flags & GD_FLG_RELOC))
              return 0;
 

Quoted 13 lines

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev) · line 245

Quoted 11 lines

drivers/video/sunxi/sunxi_de2.c +11 -0

+     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
+                                      "sunxi_lcd", &disp);
+     if (!ret) {
+             int mux;
+
+             mux = 0;
+
+             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
+                                  false);
+             if (!ret) {
+                     video_set_flush_dcache(dev, 1);

Why do you need to flush the dcache here?

Copied from HDMI driver init. If it's not necessary why it's here for HDMI?

Quoted 60 lines

Quoted 60 lines

drivers/video/sunxi/sunxi_de2.c +6 -0

+                     return 0;
+             }
+     }
+
+     debug("%s: lcd display not found (ret=%d)\n", __func__, ret);
+
      ret = uclass_find_device_by_name(UCLASS_DISPLAY,
                                       "sunxi_dw_hdmi", &disp);
      if (!ret) {

drivers/video/sunxi/sunxi_lcd.c new file +45 -0

new file mode 100644
index 0000000000..154eb5835e
@@ -0,0 +1,142 @@
+/*
+ * Allwinner LCD driver
+ *
+ * (C) Copyright 2017 Vasily Khoruzhick <[email protected]>
+ *
+ * SPDX-License-Identifier:  GPL-2.0+
+ */
+
+#include <common.h>
+#include <display.h>
+#include <video_bridge.h>
+#include <backlight.h>
+#include <dm.h>
+#include <edid.h>
+#include <asm/io.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/lcdc.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+
+struct sunxi_lcd_priv {
+     struct display_timing timing;
+     int panel_bpp;
+};
+
+static void sunxi_lcdc_config_pinmux(void)
+{
+     int pin;
+     for (pin = SUNXI_GPD(0); pin <= SUNXI_GPD(21); pin++) {
+             sunxi_gpio_set_cfgpin(pin, SUNXI_GPD_LCD0);
+             sunxi_gpio_set_drv(pin, 3);
+     }
+}
+
+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+     struct sunxi_ccm_reg * const ccm =
+            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+     struct sunxi_lcdc_reg * const lcdc =
+            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     struct udevice *backlight;
+     int clk_div, clk_double, ret;
+

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 50

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+     /* Reset off */
+     setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
+
+     /* Clock on */
+     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);

This has nothing to do with using a panel or not, it should be in lcdc_init().

Why? We don't need neither take it out of reset nor turn the clock on it if LCD is not used (e.g. HDMI-only case).

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 3 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Quoted 24 lines

Quoted 24 lines

drivers/video/sunxi/sunxi_lcd.c new file +24 -0

+     lcdc_pll_set(ccm, 0, edid->pixelclock.typ / 1000,
+                  &clk_div, &clk_double);
+     lcdc_tcon0_mode_set(lcdc, edid, clk_div, false,
+                         priv->panel_bpp, CONFIG_VIDEO_LCD_DCLK_PHASE);
+     lcdc_enable(lcdc, priv->panel_bpp);
+
+     ret = uclass_get_device(UCLASS_PANEL_BACKLIGHT, 0, &backlight);
+     if (!ret)
+             backlight_enable(backlight);
+
+     return 0;
+}
+
+static int sunxi_lcd_read_timing(struct udevice *dev,
+                                struct display_timing *timing)
+{
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     memcpy(timing, &priv->timing, sizeof(struct display_timing));
+
+     return 0;
+}
+
+static int sunxi_lcd_probe(struct udevice *dev)
+{

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 83

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     struct udevice *cdev;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     int ret;
+
+     /* make sure that clock is active */
+     clock_set_pll10(432000000);

Why do you need it active, and why at that rate?

Will check.

Quoted 31 lines

Quoted 31 lines

drivers/video/sunxi/sunxi_lcd.c new file +31 -0

+
+#ifdef CONFIG_VIDEO_BRIDGE
+     /* Try to get timings from bridge first */
+     ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &cdev);
+     if (!ret) {
+             u8 edid[EDID_SIZE];
+             int channel_bpp;
+
+             ret = video_bridge_attach(cdev);
+             if (ret) {
+                     debug("video bridge attach failed: %d\n", ret);
+                     return ret;
+             }
+             ret = video_bridge_read_edid(cdev, edid, EDID_SIZE);
+             if (ret <= 0) {
+                     debug("video bridge failed to read edid: %d\n", ret);
+                     return ret ? ret : -EIO;
+             }
+             ret = edid_get_timing(edid, ret, &priv->timing, &channel_bpp);
+             priv->panel_bpp = channel_bpp * 3;
+             return ret;
+     }
+     debug("video bridge not found: %d\n", ret);
+#endif
+     /* Fallback to timings from DT if there's no bridge */
+     ret = uclass_get_device(UCLASS_PANEL, 0, &cdev);
+     if (ret) {
+             debug("video panel not found: %d\n", ret);
+             return ret;
+     }
+

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 119

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c new file +5 -0

+     if (fdtdec_decode_display_timing(gd->fdt_blob, dev_of_offset(cdev),
+                                      0, &priv->timing)) {
+             debug("%s: Failed to decode display timing\n", __func__);
+             return -EINVAL;
+     }

How does that work with simple-panel style bindings that are most of the panels merged these days?

It should just work, but I haven't even tested this part of the code - I don't have a panel with parallel interface nor A64 board to connect it to.
Do you want me to drop it and make this driver dependent on CONFIG_VIDEO_BRIDGE

Quoted 13 lines

Quoted 13 lines

drivers/video/sunxi/sunxi_lcd.c new file +13 -0

+     priv->panel_bpp = 16;
+
+     return 0;
+}
+
+static const struct dm_display_ops sunxi_lcd_ops = {
+       .read_timing = sunxi_lcd_read_timing,
+       .enable = sunxi_lcd_enable,
+};
+
+U_BOOT_DRIVER(sunxi_lcd) = {
+     .name   = "sunxi_lcd",
+     .id     = UCLASS_DISPLAY,

Quoted 8 lines

drivers/video/sunxi/sunxi_lcd.c line 138

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+     .ops    = &sunxi_lcd_ops,
+     .probe  = sunxi_lcd_probe,
+     .priv_auto_alloc_size = sizeof(struct sunxi_lcd_priv),
+};
+
+#ifdef CONFIG_MACH_SUN50I

Why do you restrict it to the A64 here?

Because the hardware is present in A64 only.

Quoted 7 lines

Thanks,
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Quoted 14 lines

Quoted 13 lines

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev) · line 245

Quoted 11 lines

drivers/video/sunxi/sunxi_de2.c +11 -0

+     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
+                                      "sunxi_lcd", &disp);
+     if (!ret) {
+             int mux;
+
+             mux = 0;
+
+             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
+                                  false);
+             if (!ret) {
+                     video_set_flush_dcache(dev, 1);

Why do you need to flush the dcache here?

Quoted 1 line

Copied from HDMI driver init. If it's not necessary why it's here for HDMI?

DE2 is not cache aware, so CPU should flush dcache after updating framebuffer. If I remove this line, dcache isn't flushed when framebuffer is updated, and thus picture is a total mess (black background with some white stripes).

Quoted 20 lines

Quoted 19 lines

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+     struct sunxi_ccm_reg * const ccm =
+            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+     struct sunxi_lcdc_reg * const lcdc =

drivers/video/sunxi/sunxi_lcd.c line 50

Quoted 10 lines

drivers/video/sunxi/sunxi_lcd.c new file +10 -0

+            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     struct udevice *backlight;
+     int clk_div, clk_double, ret;
+
+     /* Reset off */
+     setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
+
+     /* Clock on */
+     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);

This has nothing to do with using a panel or not, it should be in lcdc_init().

Quoted 2 lines

Why? We don't need neither take it out of reset nor turn the clock on it if LCD is not used (e.g. HDMI-only case).

I'm leaving it here. It's not necessary for HDMI, and it doesn't work without it for LCD.

Quoted 9 lines

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Quoted 16 lines · Vasily Khoruzhick

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

drivers/video/sunxi/sunxi_de2.c +5 -0

+     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
+                                      "sunxi_lcd", &disp);
+     if (!ret) {
+             int mux;
+

Quoted 8 lines

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev) · line 245

Quoted 6 lines

drivers/video/sunxi/sunxi_de2.c +6 -0

+             mux = 0;
+
+             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
+                                  false);
+             if (!ret) {
+                     video_set_flush_dcache(dev, 1);

Why do you need to flush the dcache here?

Copied from HDMI driver init. If it's not necessary why it's here for HDMI?

Quoted 4 lines

DE2 is not cache aware, so CPU should flush dcache after updating framebuffer. If I remove this line, dcache isn't flushed when framebuffer is updated, and thus picture is a total mess (black background with some white stripes).

Ah, so the framebuffer is mapped cacheable. Ok, that's unusual, but that works. Can you put that in a comment?

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 7 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+     struct sunxi_ccm_reg * const ccm =
+            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+     struct sunxi_lcdc_reg * const lcdc =

drivers/video/sunxi/sunxi_lcd.c line 50

Quoted 10 lines

drivers/video/sunxi/sunxi_lcd.c new file +10 -0

+            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     struct udevice *backlight;
+     int clk_div, clk_double, ret;
+
+     /* Reset off */
+     setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
+
+     /* Clock on */
+     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);

This has nothing to do with using a panel or not, it should be in lcdc_init().

Quoted 5 lines

Quoted 2 lines

Why? We don't need neither take it out of reset nor turn the clock on it if LCD is not used (e.g. HDMI-only case).

I'm leaving it here. It's not necessary for HDMI, and it doesn't work without it for LCD.

I'm pretty sure it still needs to be done for HDMI. LCD0 is the TCON, and the TCON is used for HDMI too.

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 3 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Then move out the common code. It's kind of weird though, since the DE1 vs DE2 stuff is basically only for the layers part. The TCON is always there, and is mostly the same. So you should be able to re-use that with minor modifications.

Maxime

Quoted 16 lines · Vasily Khoruzhick

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

drivers/video/sunxi/sunxi_de2.c +5 -0

+     ret = uclass_find_device_by_name(UCLASS_DISPLAY,
+                                      "sunxi_lcd", &disp);
+     if (!ret) {
+             int mux;
+

Quoted 8 lines

drivers/video/sunxi/sunxi_de2.c static int sunxi_de2_probe(struct udevice *dev) · line 245

Quoted 6 lines

drivers/video/sunxi/sunxi_de2.c +6 -0

+             mux = 0;
+
+             ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
+                                  false);
+             if (!ret) {
+                     video_set_flush_dcache(dev, 1);

Why do you need to flush the dcache here?

Copied from HDMI driver init. If it's not necessary why it's here for HDMI?

Quoted 4 lines

DE2 is not cache aware, so CPU should flush dcache after updating framebuffer. If I remove this line, dcache isn't flushed when framebuffer is updated, and thus picture is a total mess (black background with some white stripes).

Ah, so the framebuffer is mapped cacheable. Ok, that's unusual, but that works. Can you put that in a comment?

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

Quoted 6 lines

drivers/video/sunxi/sunxi_lcd.c new file +6 -0

+static int sunxi_lcd_enable(struct udevice *dev, int bpp,
+                           const struct display_timing *edid)
+{
+     struct sunxi_ccm_reg * const ccm =
+            (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+     struct sunxi_lcdc_reg * const lcdc =

drivers/video/sunxi/sunxi_lcd.c line 50

Quoted 10 lines

drivers/video/sunxi/sunxi_lcd.c new file +10 -0

+            (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
+     struct sunxi_lcd_priv *priv = dev_get_priv(dev);
+     struct udevice *backlight;
+     int clk_div, clk_double, ret;
+
+     /* Reset off */
+     setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
+
+     /* Clock on */
+     setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_LCD0);

This has nothing to do with using a panel or not, it should be in lcdc_init().

Quoted 5 lines

Quoted 2 lines

Why? We don't need neither take it out of reset nor turn the clock on it if LCD is not used (e.g. HDMI-only case).

I'm leaving it here. It's not necessary for HDMI, and it doesn't work without it for LCD.

I'm pretty sure it still needs to be done for HDMI. LCD0 is the TCON, and the TCON is used for HDMI too.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Quoted 3 lines

Quoted 3 lines

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Then move out the common code. It's kind of weird though, since the DE1 vs DE2 stuff is basically only for the layers part. The TCON is always there, and is mostly the same. So you should be able to re-use that with minor modifications.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

drivers/video/sunxi/sunxi_lcd.c line 53

Quoted 2 lines

drivers/video/sunxi/sunxi_lcd.c new file +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Then move out the common code. It's kind of weird though, since the DE1 vs DE2 stuff is basically only for the layers part. The TCON is always there, and is mostly the same. So you should be able to re-use that with minor modifications.

I'm not sure what common code you're talking about. I've already moved out lcdc_pll_set(). Moving pinmux
configuration out into common code doesn't look reasonable. It's different for A64 -- for A64 it configures
GPD(0)-GPD(21) as function, while for other SoCs it's GPD(18)-GPD(27) or GPD(0)-GPD(27) depending
on SoC model. Anyway, pinmux configuration code for DE1 contains a number of ifdef-s that are not necessary
for DE2 -- these SoCs don't have DE2 and thus won't be supported.

Quoted 15 lines · Vasily Khoruzhick

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     lcdc_init(lcdc);
+     sunxi_lcdc_config_pinmux();

This is already handled in sunxi_lcdc_tcon0_mode_set, why duplicate it?

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Because the one that sunxi_lcdc_tcon0_mode_set() calls is DE1-specific. I don't want to split out that code that won't be used by DE2 driver.

Then move out the common code. It's kind of weird though, since the DE1 vs DE2 stuff is basically only for the layers part. The TCON is always there, and is mostly the same. So you should be able to re-use that with minor modifications.

Quoted 9 lines

I'm not sure what common code you're talking about. I've already moved out lcdc_pll_set(). Moving pinmux
configuration out into common code doesn't look reasonable. It's different for A64 -- for A64 it configures
GPD(0)-GPD(21) as function, while for other SoCs it's GPD(18)-GPD(27) or GPD(0)-GPD(27) depending
on SoC model. Anyway, pinmux configuration code for DE1 contains a number of ifdef-s that are not necessary
for DE2 -- these SoCs don't have DE2 and thus won't be supported.

Again, DE1 vs DE2 has nothing to do in the discussion

DE1 devices will look like this:
DE1 -> TCON -> HDMI / LCD / whatever -> pins

DE2 will be:
DE2 -> TCON -> HDMI / LCD / whatever -> pins

The only thing not in common between these two cases is the display engine used. Everything else should be common (except for special cases, like the HDMI controller itself that changed as well).

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com