[PATCH V3 drm-dp 0/4] Add dp module in hibmc driver

[PATCH V3 drm-dp 0/4] Add dp module in hibmc driver

[Yongbang Shi]

From: baihan li <libaihan@huawei.com>

Realizing the basic display function of DP cable for DP connector
displaying. Add DP module in hibmc drm driver, which is for Hisilicon
Hibmc SoC which used for Out-of-band management. Blow is the general
hardware connection, both the Hibmc and the host CPU are on the same
mother board.

+----------+       +----------+      +----- ----+      +----------------+
|          | PCIe  |  Hibmc   |      |          |      |                |
|host CPU( |<----->| display  |<---->| dp kapi  |<---->| dp aux moduel  |
|arm64,x86)|       |subsystem |      |  moduel  |<---->| dp link moduel |
+----------+       +----------+      +----------+      +----------------+

---
ChangeLog:
v2 -> v3:
  - put the macro definations in latter patch where they are actually used, suggested by Dmitry Baryshkov.
  - rename some macro definations to make them sensible, suggested by Dmitry Baryshkov.
  - using FIELD_PREP and FIELD_GET, suggested by Dmitry Baryshkov.
  - using DP_DPCD_REV_foo, suggested by Dmitry Baryshkov.
  - using switchcase in dp_link_reduce_lane, suggested by Dmitry Baryshkov.
  - deleting dp_link_pattern2dpcd function and using macros directly, suggested by Dmitry Baryshkov.
  - deleting EFAULT error codes, suggested by Dmitry Baryshkov.
  - fix build errors reported by kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202410250305.UHKDhtxy-lkp@intel.com/
    Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@intel.com/
    Closes: https://lore.kernel.org/oe-kbuild-all/202410251136.1m7BlR68-lkp@intel.com/
  v2:https://lore.kernel.org/all/20241022124148.1952761-1-shiyongbang@huawei.com/
v1 -> v2:
  - using drm_dp_aux frame implement dp aux read and write functions, suggested by Jani Nikula.
  - using drm dp header files' dp macros instead, suggested by Andy Yan.
  - using drm_dp_* functions implement dp link training process, suggested by Jani Nikula.
  - changed some defines and functions to former patch, suggested by Dmitry Baryshkov.
  - sorting the headers including in dp_hw.h and hibmc_drm_drv.c files, suggested by Dmitry Baryshkov.
  - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
  - modifying drm_simple_encoder_init function, suggested by Dmitry Baryshkov.
  - refactoring struct hibmc_connector, suggested by Dmitry Baryshkov.
  - withdrawing the modification in hibmc_kms_init, suggested by Dmitry Baryshkov.
  - fix build errors reported by kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202410031735.8iRZZR6T-lkp@intel.com/
    Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@intel.com/
  v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
---

baihan li (4):
  drm/hisilicon/hibmc: add dp aux in hibmc drivers
  drm/hisilicon/hibmc: add dp link moduel in hibmc drivers
  drm/hisilicon/hibmc: add dp hw moduel in hibmc driver
  drm/hisilicon/hibmc: add dp module in hibmc

 drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c   | 162 ++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h   |  23 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  58 +++
 .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  19 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 237 ++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  31 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 346 ++++++++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h  |  25 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  76 ++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 128 +++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  16 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  21 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c   |  41 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  |  20 +-
 15 files changed, 1166 insertions(+), 40 deletions(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c

-- 
2.33.0

[PATCH V3 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc

[Yongbang Shi]

From: baihan li <libaihan@huawei.com>

Add dp aux read/write functions. They are basic functions
and will be used later.

Signed-off-by: baihan li <libaihan@huawei.com>
Signed-off-by: yongbang shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - put the macro definations in latter patch where they are actually used, suggested by Dmitry Baryshkov.
  - rename some macro definations to make them sensible, suggested by Dmitry Baryshkov.
  - using FIELD_PREP and FIELD_GET, suggested by Dmitry Baryshkov.
  - using DP_DPCD_REV_foo, suggested by Dmitry Baryshkov.
  - fix build errors reported by kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202410250305.UHKDhtxy-lkp@intel.com/
v1 -> v2:
  - using drm_dp_aux frame implement dp aux read and write functions, suggested by Jani Nikula.
  - using drm dp header files' dp macros instead, suggested by Andy Yan.
  v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c   | 162 ++++++++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h   |  23 +++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  58 +++++++
 .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  19 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  27 +++
 6 files changed, 291 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index d25c75e60d3d..8770ec6dfffd 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
-hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o
+hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
+	       dp/dp_aux.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
new file mode 100644
index 000000000000..49ecda672109
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2024 Hisilicon Limited.
+
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/minmax.h>
+#include <drm/drm_device.h>
+#include <drm/drm_print.h>
+#include "dp_comm.h"
+#include "dp_reg.h"
+#include "dp_aux.h"
+
+#define DP_MIN_PULSE_NUM 0x9
+
+static void hibmc_dp_aux_reset(const struct dp_dev *dp)
+{
+	dp_reg_write_field(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x0);
+	usleep_range(10, 15);
+	dp_reg_write_field(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x1);
+}
+
+static void hibmc_dp_aux_read_data(struct dp_dev *dp, u8 *buf, u8 size)
+{
+	u32 reg_num;
+	u32 value;
+	u32 num;
+	u8 i, j;
+
+	reg_num = DIV_ROUND_UP(size, BYTES_IN_U32);
+	for (i = 0; i < reg_num; i++) {
+		/* number of bytes read from a single register */
+		num = min(size - i * BYTES_IN_U32, BYTES_IN_U32);
+		value = readl(dp->base + DP_AUX_RD_DATA0 + i * BYTES_IN_U32);
+		/* convert the 32-bit value of the register to the buffer. */
+		for (j = 0; j < num; j++)
+			buf[i * BYTES_IN_U32 + j] = value >> (j * BITS_IN_U8);
+	}
+}
+
+static void hibmc_dp_aux_write_data(struct dp_dev *dp, u8 *buf, u8 size)
+{
+	u32 reg_num;
+	u32 value;
+	u8 i, j;
+	u32 num;
+
+	reg_num = DIV_ROUND_UP(size, BYTES_IN_U32);
+	for (i = 0; i < reg_num; i++) {
+		/* number of bytes written to a single register */
+		num = min_t(u8, size - i * BYTES_IN_U32, BYTES_IN_U32);
+		value = 0;
+		/* obtain the 32-bit value written to a single register. */
+		for (j = 0; j < num; j++)
+			value |= buf[i * BYTES_IN_U32 + j] << (j * BITS_IN_U8);
+		/* writing data to a single register */
+		writel(value, dp->base + DP_AUX_WR_DATA0 + i * BYTES_IN_U32);
+	}
+}
+
+static u32 hibmc_dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
+{
+	u32 aux_cmd = msg->request;
+
+	if (msg->size)
+		aux_cmd |= FIELD_PREP(AUX_CMD_REQ_LEN, (msg->size - 1));
+	else
+		aux_cmd |= FIELD_PREP(AUX_CMD_I2C_ADDR_ONLY, 1);
+
+	aux_cmd |= FIELD_PREP(AUX_CMD_ADDR, msg->address);
+
+	return aux_cmd;
+}
+
+/* ret >= 0 ,ret is size; ret < 0, ret is err code */
+static int hibmc_dp_aux_parse_xfer(struct dp_dev *dp, struct drm_dp_aux_msg *msg)
+{
+	u32 buf_data_cnt;
+	u32 aux_status;
+	int ret = 0;
+
+	aux_status = readl(dp->base + DP_AUX_STATUS);
+	msg->reply = FIELD_GET(DP_CFG_AUX_STATUS, aux_status);
+
+	if (aux_status & DP_CFG_AUX_TIMEOUT)
+		return -ETIMEDOUT;
+
+	/* only address */
+	if (!msg->size)
+		return 0;
+
+	if (msg->reply != DP_AUX_NATIVE_REPLY_ACK)
+		return 0;
+
+	buf_data_cnt = FIELD_GET(DP_CFG_AUX_READY_DATA_BYTE, aux_status);
+
+	switch (msg->request) {
+	case DP_AUX_NATIVE_WRITE:
+		ret = msg->size;
+		break;
+	case DP_AUX_I2C_WRITE | DP_AUX_I2C_MOT:
+		if (buf_data_cnt == AUX_I2C_WRITE_SUCCESS)
+			ret = msg->size;
+		else if (buf_data_cnt == AUX_I2C_WRITE_PARTIAL_SUCCESS)
+			ret = FIELD_GET(DP_CFG_AUX, aux_status);
+		break;
+	case DP_AUX_NATIVE_READ:
+	case DP_AUX_I2C_READ | DP_AUX_I2C_MOT:
+		buf_data_cnt--;
+		/* only the successful part of data is read */
+		if (buf_data_cnt != msg->size) {
+			ret = -EBUSY;
+		} else { /* all data is successfully read */
+			hibmc_dp_aux_read_data(dp, msg->buffer, msg->size);
+			ret = msg->size;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+/* ret >= 0 ,ret is size; ret < 0, ret is err code */
+static ssize_t hibmc_dp_aux_xfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
+{
+	struct dp_dev *dp = container_of(aux, struct dp_dev, aux);
+	u32 aux_cmd;
+	int ret;
+	u32 val; /* val will be assigned at the beginning of readl_poll_timeout function */
+
+	writel(0, dp->base + DP_AUX_WR_DATA0);
+	writel(0, dp->base + DP_AUX_WR_DATA1);
+	writel(0, dp->base + DP_AUX_WR_DATA2);
+	writel(0, dp->base + DP_AUX_WR_DATA3);
+
+	hibmc_dp_aux_write_data(dp, msg->buffer, msg->size);
+
+	aux_cmd = hibmc_dp_aux_build_cmd(msg);
+	writel(aux_cmd, dp->base + DP_AUX_CMD_ADDR);
+
+	/* enable aux transfer */
+	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_REQ, 0x1);
+	ret = readl_poll_timeout(dp->base + DP_AUX_REQ, val, !(val & DP_CFG_AUX_REQ), 50, 5000);
+	if (ret) {
+		hibmc_dp_aux_reset(dp);
+		return ret;
+	}
+
+	return hibmc_dp_aux_parse_xfer(dp, msg);
+}
+
+void hibmc_dp_aux_init(struct dp_dev *dp)
+{
+	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
+	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
+	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_MIN_PULSE_NUM, DP_MIN_PULSE_NUM);
+
+	dp->aux.transfer = hibmc_dp_aux_xfer;
+	dp->aux.is_remote = 0;
+	drm_dp_aux_init(&dp->aux);
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
new file mode 100644
index 000000000000..794bb0482ea7
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (c) 2024 Hisilicon Limited. */
+
+#ifndef DP_AUX_H
+#define DP_AUX_H
+
+#include <linux/bitops.h>
+#include "dp_comm.h"
+
+#define AUX_I2C_WRITE_SUCCESS		0x1
+#define AUX_I2C_WRITE_PARTIAL_SUCCESS	0x2
+#define EQ_MAX_RETRY			5
+#define BYTES_IN_U32			4
+#define BITS_IN_U8			8
+
+/* aux_cmd_addr register shift */
+#define AUX_CMD_REQ_LEN			GENMASK(7, 4)
+#define AUX_CMD_ADDR			GENMASK(27, 8)
+#define AUX_CMD_I2C_ADDR_ONLY		BIT(28)
+
+void hibmc_dp_aux_init(struct dp_dev *dp);
+
+#endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
new file mode 100644
index 000000000000..61a59fd59962
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (c) 2024 Hisilicon Limited. */
+
+#ifndef DP_COMM_H
+#define DP_COMM_H
+
+#include <linux/types.h>
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/kernel.h>
+#include <linux/bitfield.h>
+#include <linux/io.h>
+
+#include <drm/display/drm_dp_helper.h>
+
+#define dp_reg_read_field(addr, mask)				\
+		FIELD_GET(mask, readl(addr))
+
+#define dp_field_modify(reg_value, mask, value) ({		\
+		(reg_value) &= ~(mask);				\
+		(reg_value) |= FIELD_PREP(mask, value); })
+
+#define dp_reg_write_field(addr, mask, val) ({			\
+		typeof(addr) _addr = (addr);			\
+		u32 _value = readl(_addr);			\
+		dp_field_modify(_value, mask, val);		\
+		writel(_value, _addr); })
+
+struct link_status {
+	bool clock_recovered;
+	bool channel_equalized;
+	u8 cr_done_lanes;
+};
+
+struct link_cap {
+	int rx_dpcd_revision;
+	u8 link_rate;
+	u8 lanes;
+	bool is_tps3;
+	bool is_tps4;
+};
+
+struct hibmc_dp_link {
+	struct link_status status;
+	u8 *train_set;
+	struct link_cap cap;
+};
+
+struct dp_dev {
+	struct hibmc_dp_link link;
+	struct drm_dp_aux aux;
+	struct drm_device *dev;
+	void __iomem *base;
+	u8 dpcd[DP_RECEIVER_CAP_SIZE];
+};
+
+#endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
new file mode 100644
index 000000000000..0b965e6ba7b3
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (c) 2024 Hisilicon Limited. */
+
+#ifndef DP_CONFIG_H
+#define DP_CONFIG_H
+
+#define DP_BPP			24
+#define DP_SYMBOL_PER_FCLK	4
+#define DP_MSA1			0x20
+#define DP_MSA2			0x845c00
+#define DP_OFFSET		0x1e0000
+#define DP_HDCP			0x2
+#define DP_INT_RST		0xffff
+#define DP_DPTX_RST		0x3ff
+#define DP_CLK_EN		0x7
+#define DP_SYNC_EN_MASK		0x3
+#define DP_LINK_RATE_CAL	27
+
+#endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
new file mode 100644
index 000000000000..83cf0cc06ae2
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (c) 2024 Hisilicon Limited. */
+
+#ifndef DP_REG_H
+#define DP_REG_H
+
+#define DP_AUX_CMD_ADDR			0x50
+#define DP_AUX_WR_DATA0			0x54
+#define DP_AUX_WR_DATA1			0x58
+#define DP_AUX_WR_DATA2			0x5c
+#define DP_AUX_WR_DATA3			0x60
+#define DP_AUX_RD_DATA0			0x64
+#define DP_AUX_REQ			0x74
+#define DP_AUX_STATUS			0x78
+#define DP_DPTX_RST_CTRL		0x700
+
+#define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
+#define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
+#define DP_CFG_AUX_MIN_PULSE_NUM		GENMASK(13, 9)
+#define DP_CFG_AUX_REQ				BIT(0)
+#define DP_CFG_AUX_RST_N			BIT(4)
+#define DP_CFG_AUX_TIMEOUT			BIT(0)
+#define DP_CFG_AUX_READY_DATA_BYTE		GENMASK(16, 12)
+#define DP_CFG_AUX				GENMASK(24, 17)
+#define DP_CFG_AUX_STATUS			GENMASK(11, 4)
+
+#endif
-- 
2.33.0

[PATCH V3 drm-dp 2/4] drm/hisilicon/hibmc: add dp link moduel in hibmc

[Yongbang Shi]

From: baihan li <libaihan@huawei.com>

Add link training process functions in this moduel.

Signed-off-by: baihan li <libaihan@huawei.com>
Signed-off-by: yongbang shi <shiyongbang@huawei.com>
---
Changelog:
v2 -> v3:
  - using switchcase in dp_link_reduce_lane, suggested by Dmitry Baryshkov.
  - deleting dp_link_pattern2dpcd function and using macros directly, suggested by Dmitry Baryshkov.
  - deleting EFAULT error codes, suggested by Dmitry Baryshkov.
v1 -> v2:
  - using drm_dp_* functions implement dp link training process, suggested by Jani Nikula.
  - fix build errors reported by kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202410031735.8iRZZR6T-lkp@intel.com/
  v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile     |   2 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c | 346 +++++++++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h |  25 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h  |   8 +
 4 files changed, 380 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index 8770ec6dfffd..94d77da88bbf 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
-	       dp/dp_aux.o
+	       dp/dp_aux.o dp/dp_link.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
new file mode 100644
index 000000000000..7146de020c93
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
@@ -0,0 +1,346 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2024 Hisilicon Limited.
+
+#include <linux/delay.h>
+#include <drm/drm_device.h>
+#include <drm/drm_print.h>
+#include "dp_comm.h"
+#include "dp_reg.h"
+#include "dp_link.h"
+#include "dp_aux.h"
+
+const u8 link_rate_map[] = {DP_LINK_BW_1_62, DP_LINK_BW_2_7,
+			    DP_LINK_BW_5_4, DP_LINK_BW_8_1};
+
+static int hibmc_dp_link_training_configure(struct dp_dev *dp)
+{
+	u8 buf[2];
+	int ret;
+
+	/* DP 2 lane */
+	dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_LANE_DATA_EN,
+			   dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x3 : 0x1);
+	dp_reg_write_field(dp->base + DP_DPTX_GCTL0, DP_CFG_PHY_LANE_NUM,
+			   dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x1 : 0);
+
+	/* enhanced frame */
+	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_FRAME_MODE, 0x1);
+
+	/* set rate and lane count */
+	buf[0] = hibmc_dp_get_link_rate(dp->link.cap.link_rate);
+	buf[1] = DP_LANE_COUNT_ENHANCED_FRAME_EN | dp->link.cap.lanes;
+	ret = drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		drm_dbg_dp(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
+		return ret >= 0 ? -EIO : ret;
+	}
+
+	/* set 8b/10b and downspread */
+	buf[0] = 0x10;
+	buf[1] = 0x1;
+	ret = drm_dp_dpcd_write(&dp->aux, DP_DOWNSPREAD_CTRL, buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		drm_dbg_dp(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", ret);
+		return ret >= 0 ? -EIO : ret;
+	}
+
+	ret = drm_dp_read_dpcd_caps(&dp->aux, dp->dpcd);
+	if (ret)
+		drm_err(dp->dev, "dp aux read dpcd failed, ret: %d\n", ret);
+
+	return ret;
+}
+
+static int hibmc_dp_link_set_pattern(struct dp_dev *dp, int pattern)
+{
+	int ret;
+	u8 val;
+	u8 buf;
+
+	buf = (u8)pattern;
+	if (pattern != DP_TRAINING_PATTERN_DISABLE && pattern != DP_TRAINING_PATTERN_4) {
+		buf |= DP_LINK_SCRAMBLING_DISABLE;
+		dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0x1);
+	} else {
+		dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0);
+	}
+
+	switch (pattern) {
+	case DP_TRAINING_PATTERN_1:
+		val = 1;
+		break;
+	case DP_TRAINING_PATTERN_2:
+		val = 2;
+		break;
+	case DP_TRAINING_PATTERN_3:
+		val = 3;
+		break;
+	case DP_TRAINING_PATTERN_4:
+		val = 4;
+		break;
+	default:
+		val = 0;
+	}
+
+	dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_PAT_SEL, val);
+
+	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		drm_dbg_dp(dp->dev, "dp aux write training pattern set failed\n");
+		return ret >= 0 ? -EIO : ret;
+	}
+
+	return 0;
+}
+
+static int hibmc_dp_link_training_cr_pre(struct dp_dev *dp)
+{
+	u8 *train_set = dp->link.train_set;
+	int ret;
+	u8 i;
+
+	ret = hibmc_dp_link_training_configure(dp);
+	if (ret)
+		return ret;
+
+	ret = hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_1);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < dp->link.cap.lanes; i++)
+		train_set[i] = DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
+
+	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set, dp->link.cap.lanes);
+	if (ret != dp->link.cap.lanes) {
+		drm_dbg_dp(dp->dev, "dp aux write training lane set failed\n");
+		return ret >= 0 ? -EIO : ret;
+	}
+
+	return 0;
+}
+
+static bool hibmc_dp_link_get_adjust_train(struct dp_dev *dp, u8 lane_status[DP_LINK_STATUS_SIZE])
+{
+	u8 pre_emph[DP_LANE_NUM_MAX] = {0};
+	u8 voltage[DP_LANE_NUM_MAX] = {0};
+	bool changed = false;
+	u8 train_set;
+	u8 lane;
+
+	for (lane = 0; lane < dp->link.cap.lanes; lane++) {
+		voltage[lane] = drm_dp_get_adjust_request_voltage(lane_status, lane);
+		pre_emph[lane] = drm_dp_get_adjust_request_pre_emphasis(lane_status, lane);
+	}
+
+	for (lane = 0; lane < dp->link.cap.lanes; lane++) {
+		train_set = voltage[lane] | pre_emph[lane];
+		if (dp->link.train_set[lane] != train_set) {
+			changed = true;
+			dp->link.train_set[lane] = train_set;
+		}
+	}
+
+	return changed;
+}
+
+u8 hibmc_dp_get_link_rate(u8 index)
+{
+	return link_rate_map[index];
+}
+
+static int hibmc_dp_link_reduce_rate(struct dp_dev *dp)
+{
+	if (dp->link.cap.link_rate > 0) {
+		dp->link.cap.link_rate--;
+		return 0;
+	}
+
+	drm_err(dp->dev, "dp link training reduce rate failed, already lowest rate\n");
+
+	return -ELNRNG;
+}
+
+static int hibmc_dp_link_reduce_lane(struct dp_dev *dp)
+{
+	switch (dp->link.cap.lanes) {
+	case DP_LANE_NUM_2:
+		dp->link.cap.lanes--;
+		break;
+	case DP_LANE_NUM_1:
+		drm_err(dp->dev, "dp link training reduce lane failed, already reach minimum\n");
+		return -ELNRNG;
+	}
+
+	return 0;
+}
+
+static int hibmc_dp_link_training_cr(struct dp_dev *dp)
+{
+	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
+	bool level_changed;
+	u32 voltage_tries;
+	u32 cr_tries;
+	u32 max_cr;
+	int ret;
+
+	/*
+	 * DP 1.4 spec define 10 for maxtries value, for pre DP 1.4 version set a limit of 80
+	 * (4 voltage levels x 4 preemphasis levels x 5 identical voltage retries)
+	 */
+	max_cr = dp->link.cap.rx_dpcd_revision >= DP_DPCD_REV_14 ? 10 : 80;
+
+	voltage_tries = 1;
+	for (cr_tries = 0; cr_tries < max_cr; cr_tries++) {
+		drm_dp_link_train_clock_recovery_delay(&dp->aux, dp->dpcd);
+
+		ret = drm_dp_dpcd_read_link_status(&dp->aux, lane_status);
+		if (ret != DP_LINK_STATUS_SIZE) {
+			drm_err(dp->dev, "Get lane status failed\n");
+			return ret;
+		}
+
+		if (drm_dp_clock_recovery_ok(lane_status, dp->link.cap.lanes)) {
+			drm_info(dp->dev, "dp link training cr done\n");
+			dp->link.status.clock_recovered = true;
+			return 0;
+		}
+
+		if (voltage_tries == 5) {
+			drm_info(dp->dev, "same voltage tries 5 times\n");
+			dp->link.status.clock_recovered = false;
+			return 0;
+		}
+
+		level_changed = hibmc_dp_link_get_adjust_train(dp, lane_status);
+		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, dp->link.train_set,
+					dp->link.cap.lanes);
+		if (ret != dp->link.cap.lanes) {
+			drm_dbg_dp(dp->dev, "Update link training failed\n");
+			return ret >= 0 ? -EIO : ret;
+		}
+
+		voltage_tries = level_changed ? 1 : voltage_tries + 1;
+	}
+
+	drm_err(dp->dev, "dp link training clock recovery %u timers failed\n", max_cr);
+	dp->link.status.clock_recovered = false;
+
+	return 0;
+}
+
+static int hibmc_dp_link_training_channel_eq(struct dp_dev *dp)
+{
+	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
+	u8 eq_tries;
+	int tps;
+	int ret;
+
+	if (dp->link.cap.is_tps4)
+		tps = DP_TRAINING_PATTERN_4;
+	else if (dp->link.cap.is_tps3)
+		tps = DP_TRAINING_PATTERN_3;
+	else
+		tps = DP_TRAINING_PATTERN_2;
+
+	ret = hibmc_dp_link_set_pattern(dp, tps);
+	if (ret)
+		return ret;
+
+	for (eq_tries = 0; eq_tries < EQ_MAX_RETRY; eq_tries++) {
+		drm_dp_link_train_channel_eq_delay(&dp->aux, dp->dpcd);
+
+		ret = drm_dp_dpcd_read_link_status(&dp->aux, lane_status);
+		if (ret != DP_LINK_STATUS_SIZE) {
+			drm_err(dp->dev, "get lane status failed\n");
+			break;
+		}
+
+		if (!drm_dp_clock_recovery_ok(lane_status, dp->link.cap.lanes)) {
+			drm_info(dp->dev, "clock recovery check failed\n");
+			drm_info(dp->dev, "cannot continue channel equalization\n");
+			dp->link.status.clock_recovered = false;
+			break;
+		}
+
+		if (drm_dp_channel_eq_ok(lane_status, dp->link.cap.lanes)) {
+			dp->link.status.channel_equalized = true;
+			drm_info(dp->dev, "dp link training eq done\n");
+			break;
+		}
+
+		hibmc_dp_link_get_adjust_train(dp, lane_status);
+		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
+					dp->link.train_set, dp->link.cap.lanes);
+		if (ret != dp->link.cap.lanes) {
+			drm_dbg_dp(dp->dev, "Update link training failed\n");
+			ret = (ret >= 0) ? -EIO : ret;
+			break;
+		}
+	}
+
+	if (eq_tries == EQ_MAX_RETRY)
+		drm_err(dp->dev, "channel equalization failed %u times\n", eq_tries);
+
+	hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int hibmc_dp_link_downgrade_training_cr(struct dp_dev *dp)
+{
+	if (hibmc_dp_link_reduce_rate(dp))
+		return hibmc_dp_link_reduce_lane(dp);
+
+	return 0;
+}
+
+static int hibmc_dp_link_downgrade_training_eq(struct dp_dev *dp)
+{
+	if ((dp->link.status.clock_recovered && !dp->link.status.channel_equalized)) {
+		if (!hibmc_dp_link_reduce_lane(dp))
+			return 0;
+	}
+
+	return hibmc_dp_link_reduce_rate(dp);
+}
+
+int hibmc_dp_link_training(struct dp_dev *dp)
+{
+	struct hibmc_dp_link *link = &dp->link;
+	int ret;
+
+	while (true) {
+		ret = hibmc_dp_link_training_cr_pre(dp);
+		if (ret)
+			goto err;
+
+		ret = hibmc_dp_link_training_cr(dp);
+		if (ret)
+			goto err;
+
+		if (!link->status.clock_recovered) {
+			ret = hibmc_dp_link_downgrade_training_cr(dp);
+			if (ret)
+				goto err;
+			continue;
+		}
+
+		ret = hibmc_dp_link_training_channel_eq(dp);
+		if (ret)
+			goto err;
+
+		if (!link->status.channel_equalized) {
+			ret = hibmc_dp_link_downgrade_training_eq(dp);
+			if (ret)
+				goto err;
+			continue;
+		}
+
+		return 0;
+	}
+
+err:
+	hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h
new file mode 100644
index 000000000000..b4958d122083
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (c) 2024 Hisilicon Limited. */
+
+#ifndef DP_LINK_H
+#define DP_LINK_H
+
+#include "dp_comm.h"
+
+#define DP_LANE_NUM_MAX		2
+#define DP_LANE_STATUS_SIZE	1
+#define DP_LANE_NUM_1		0x1
+#define DP_LANE_NUM_2		0x2
+
+enum dp_pattern_e {
+	DP_PATTERN_NO = 0,
+	DP_PATTERN_TPS1,
+	DP_PATTERN_TPS2,
+	DP_PATTERN_TPS3,
+	DP_PATTERN_TPS4,
+};
+
+int hibmc_dp_link_training(struct dp_dev *dp);
+u8 hibmc_dp_get_link_rate(u8 index);
+
+#endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
index 83cf0cc06ae2..1032f6cde761 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -12,16 +12,24 @@
 #define DP_AUX_RD_DATA0			0x64
 #define DP_AUX_REQ			0x74
 #define DP_AUX_STATUS			0x78
+#define DP_PHYIF_CTRL0			0xa0
+#define DP_VIDEO_CTRL			0x100
 #define DP_DPTX_RST_CTRL		0x700
+#define DP_DPTX_GCTL0			0x708
 
 #define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
 #define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
+#define DP_CFG_STREAM_FRAME_MODE		BIT(6)
 #define DP_CFG_AUX_MIN_PULSE_NUM		GENMASK(13, 9)
+#define DP_CFG_LANE_DATA_EN			GENMASK(11, 8)
+#define DP_CFG_PHY_LANE_NUM			GENMASK(2, 1)
 #define DP_CFG_AUX_REQ				BIT(0)
 #define DP_CFG_AUX_RST_N			BIT(4)
 #define DP_CFG_AUX_TIMEOUT			BIT(0)
 #define DP_CFG_AUX_READY_DATA_BYTE		GENMASK(16, 12)
 #define DP_CFG_AUX				GENMASK(24, 17)
 #define DP_CFG_AUX_STATUS			GENMASK(11, 4)
+#define DP_CFG_SCRAMBLE_EN			BIT(0)
+#define DP_CFG_PAT_SEL				GENMASK(7, 4)
 
 #endif
-- 
2.33.0

[PATCH V3 drm-dp 3/4] drm/hisilicon/hibmc: add dp hw moduel in hibmc

[Yongbang Shi]

From: baihan li <libaihan@huawei.com>

Build a dp level that hibmc driver can enable dp by
calling their functions.

Signed-off-by: baihan li <libaihan@huawei.com>
Signed-off-by: yongbang shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - fix build errors reported by kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@intel.com/
v1 -> v2:
  - changed some defines and functions to former patch, suggested by Dmitry Baryshkov.
  - sorting the headers including in dp_hw.h and hibmc_drm_drv.c files, suggested by Dmitry Baryshkov.
  - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
  - fix build errors reported by kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@intel.com/
  v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile    |   2 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c  | 237 ++++++++++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h  |  31 +++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h |  41 ++++
 4 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index 94d77da88bbf..214228052ccf 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
-	       dp/dp_aux.o dp/dp_link.o
+	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
new file mode 100644
index 000000000000..214897798bdb
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -0,0 +1,237 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2024 Hisilicon Limited.
+
+#include <linux/io.h>
+#include <linux/delay.h>
+#include "dp_config.h"
+#include "dp_comm.h"
+#include "dp_reg.h"
+#include "dp_hw.h"
+#include "dp_link.h"
+#include "dp_aux.h"
+
+static int hibmc_dp_link_init(struct dp_dev *dp)
+{
+	dp->link.cap.lanes = 2;
+	dp->link.train_set = devm_kzalloc(dp->dev->dev,
+					  dp->link.cap.lanes * sizeof(u8), GFP_KERNEL);
+	if (!dp->link.train_set)
+		return -ENOMEM;
+
+	dp->link.cap.link_rate = 1;
+
+	return 0;
+}
+
+static void hibmc_dp_set_tu(struct dp_dev *dp, struct drm_display_mode *mode)
+{
+	u32 tu_symbol_frac_size;
+	u32 tu_symbol_size;
+	u32 rate_ks;
+	u8 lane_num;
+	u32 value;
+	u32 bpp;
+
+	lane_num = dp->link.cap.lanes;
+	if (lane_num == 0) {
+		drm_err(dp->dev, "set tu failed, lane num cannot be 0!\n");
+		return;
+	}
+
+	bpp = DP_BPP;
+	rate_ks = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
+	value = (mode->clock * bpp * 5) / (61 * lane_num * rate_ks);
+
+	if (value % 10 == 9) { /* 9 carry */
+		tu_symbol_size = value / 10 + 1;
+		tu_symbol_frac_size = 0;
+	} else {
+		tu_symbol_size = value / 10;
+		tu_symbol_frac_size = value % 10 + 1;
+	}
+
+	drm_info(dp->dev, "tu value: %u.%u value: %u\n",
+		 tu_symbol_size, tu_symbol_frac_size, value);
+
+	dp_reg_write_field(dp->base + DP_VIDEO_PACKET,
+			   DP_CFG_STREAM_TU_SYMBOL_SIZE, tu_symbol_size);
+	dp_reg_write_field(dp->base + DP_VIDEO_PACKET,
+			   DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE, tu_symbol_frac_size);
+}
+
+static void hibmc_dp_set_sst(struct dp_dev *dp, struct drm_display_mode *mode)
+{
+	u32 hblank_size;
+	u32 htotal_size;
+	u32 htotal_int;
+	u32 hblank_int;
+	u32 fclk; /* flink_clock */
+
+	fclk = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
+
+	/* ssc: 9947 / 10000 = 0.9947 */
+	htotal_int = mode->htotal * 9947 / 10000;
+	htotal_size = (u32)(htotal_int * fclk / (DP_SYMBOL_PER_FCLK * (mode->clock / 1000)));
+
+	/* ssc: max effect bandwidth 53 / 10000 = 0.53% */
+	hblank_int = (mode->htotal - mode->hdisplay) - mode->hdisplay * 53 / 10000;
+	hblank_size = hblank_int * fclk * 9947 /
+		      (mode->clock * 10 * DP_SYMBOL_PER_FCLK);
+
+	drm_info(dp->dev, "h_active %u v_active %u htotal_size %u hblank_size %u",
+		 mode->hdisplay, mode->vdisplay, htotal_size, hblank_size);
+	drm_info(dp->dev, "flink_clock %u pixel_clock %d", fclk, (mode->clock / 1000));
+
+	dp_reg_write_field(dp->base + DP_VIDEO_HORIZONTAL_SIZE,
+			   DP_CFG_STREAM_HTOTAL_SIZE, htotal_size);
+	dp_reg_write_field(dp->base + DP_VIDEO_HORIZONTAL_SIZE,
+			   DP_CFG_STREAM_HBLANK_SIZE, hblank_size);
+}
+
+static void hibmc_dp_link_cfg(struct dp_dev *dp, struct drm_display_mode *mode)
+{
+	u32 timing_delay;
+	u32 vblank;
+	u32 hstart;
+	u32 vstart;
+
+	vblank = mode->vtotal - mode->vdisplay;
+	timing_delay = mode->htotal - mode->hsync_start;
+	hstart = mode->htotal - mode->hsync_start;
+	vstart = mode->vtotal - mode->vsync_start;
+
+	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG0,
+			   DP_CFG_TIMING_GEN0_HBLANK, (mode->htotal - mode->hdisplay));
+	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG0,
+			   DP_CFG_TIMING_GEN0_HACTIVE, mode->hdisplay);
+
+	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG2,
+			   DP_CFG_TIMING_GEN0_VBLANK, vblank);
+	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG2,
+			   DP_CFG_TIMING_GEN0_VACTIVE, mode->vdisplay);
+	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG3,
+			   DP_CFG_TIMING_GEN0_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
+
+	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG0,
+			   DP_CFG_STREAM_HACTIVE, mode->hdisplay);
+	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG0,
+			   DP_CFG_STREAM_HBLANK, (mode->htotal - mode->hdisplay));
+	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG2,
+			   DP_CFG_STREAM_HSYNC_WIDTH, (mode->hsync_end - mode->hsync_start));
+
+	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG1,
+			   DP_CFG_STREAM_VACTIVE, mode->vdisplay);
+	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG1,
+			   DP_CFG_STREAM_VBLANK, vblank);
+	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG3,
+			   DP_CFG_STREAM_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
+	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG3,
+			   DP_CFG_STREAM_VSYNC_WIDTH, (mode->vsync_end - mode->vsync_start));
+
+	dp_reg_write_field(dp->base + DP_VIDEO_MSA0,
+			   DP_CFG_STREAM_VSTART, vstart);
+	dp_reg_write_field(dp->base + DP_VIDEO_MSA0,
+			   DP_CFG_STREAM_HSTART, hstart);
+
+	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_VSYNC_POLARITY,
+			   mode->flags & DRM_MODE_FLAG_PVSYNC ? 1 : 0);
+	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_HSYNC_POLARITY,
+			   mode->flags & DRM_MODE_FLAG_PHSYNC ? 1 : 0);
+
+	/* MSA mic 0 and 1 */
+	writel(DP_MSA1, dp->base + DP_VIDEO_MSA1);
+	writel(DP_MSA2, dp->base + DP_VIDEO_MSA2);
+
+	hibmc_dp_set_tu(dp, mode);
+
+	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_RGB_ENABLE, 0x1);
+	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_VIDEO_MAPPING, 0);
+
+	/* divide 2: up even */
+	if (timing_delay % 2)
+		timing_delay++;
+
+	dp_reg_write_field(dp->base + DP_TIMING_MODEL_CTRL,
+			   DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1, timing_delay);
+
+	hibmc_dp_set_sst(dp, mode);
+}
+
+int hibmc_dp_hw_init(struct hibmc_dp *dp)
+{
+	struct drm_device *drm_dev = dp->drm_dev;
+	struct dp_dev *dp_dev;
+	int ret;
+
+	dp_dev = devm_kzalloc(drm_dev->dev, sizeof(struct dp_dev), GFP_KERNEL);
+	if (!dp_dev)
+		return -ENOMEM;
+
+	dp->dp_dev = dp_dev;
+
+	dp_dev->dev = drm_dev;
+	dp_dev->base = dp->mmio + DP_OFFSET;
+
+	hibmc_dp_aux_init(dp_dev);
+
+	ret = hibmc_dp_link_init(dp_dev);
+	if (ret) {
+		drm_err(drm_dev, "dp link init failed\n");
+		return ret;
+	}
+
+	/* hdcp data */
+	writel(DP_HDCP, dp_dev->base + DP_HDCP_CFG);
+	/* int init */
+	writel(0, dp_dev->base + DP_INTR_ENABLE);
+	writel(DP_INT_RST, dp_dev->base + DP_INTR_ORIGINAL_STATUS);
+	/* rst */
+	writel(DP_DPTX_RST, dp_dev->base + DP_DPTX_RST_CTRL);
+	/* clock enable */
+	writel(DP_CLK_EN, dp_dev->base + DP_DPTX_CLK_CTRL);
+
+	return 0;
+}
+
+void hibmc_dp_hw_uninit(struct hibmc_dp *dp)
+{
+	// keep this uninit interface in the future use
+}
+
+void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
+{
+	struct dp_dev *dp_dev = dp->dp_dev;
+
+	if (enable) {
+		dp_reg_write_field(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0x1);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+		dp_reg_write_field(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0x1);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+	} else {
+		dp_reg_write_field(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+		dp_reg_write_field(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+	}
+
+	msleep(50);
+}
+
+int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
+{
+	struct dp_dev *dp_dev = dp->dp_dev;
+	int ret;
+
+	if (!dp_dev->link.status.channel_equalized) {
+		ret = hibmc_dp_link_training(dp_dev);
+		if (ret) {
+			drm_err(dp->drm_dev, "dp link training failed, ret: %d\n", ret);
+			return ret;
+		}
+	}
+
+	hibmc_dp_display_en(dp, false);
+	hibmc_dp_link_cfg(dp_dev, mode);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
new file mode 100644
index 000000000000..de802aaa8b4a
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (c) 2024 Hisilicon Limited. */
+
+#ifndef DP_KAPI_H
+#define DP_KAPI_H
+
+#include <linux/types.h>
+#include <linux/delay.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_print.h>
+#include <video/videomode.h>
+
+struct dp_dev;
+
+struct hibmc_dp {
+	struct dp_dev *dp_dev;
+	struct drm_device *drm_dev;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+	void __iomem *mmio;
+};
+
+int hibmc_dp_hw_init(struct hibmc_dp *dp);
+void hibmc_dp_hw_uninit(struct hibmc_dp *dp);
+int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
+void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
+
+#endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
index 1032f6cde761..3dcb847057a4 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -14,8 +14,26 @@
 #define DP_AUX_STATUS			0x78
 #define DP_PHYIF_CTRL0			0xa0
 #define DP_VIDEO_CTRL			0x100
+#define DP_VIDEO_CONFIG0		0x104
+#define DP_VIDEO_CONFIG1		0x108
+#define DP_VIDEO_CONFIG2		0x10c
+#define DP_VIDEO_CONFIG3		0x110
+#define DP_VIDEO_PACKET			0x114
+#define DP_VIDEO_MSA0			0x118
+#define DP_VIDEO_MSA1			0x11c
+#define DP_VIDEO_MSA2			0x120
+#define DP_VIDEO_HORIZONTAL_SIZE	0X124
+#define DP_TIMING_GEN_CONFIG0		0x26c
+#define DP_TIMING_GEN_CONFIG2		0x274
+#define DP_TIMING_GEN_CONFIG3		0x278
+#define DP_HDCP_CFG			0x600
+#define DP_INTR_ENABLE			0x720
+#define DP_INTR_ORIGINAL_STATUS		0x728
 #define DP_DPTX_RST_CTRL		0x700
+#define DP_DPTX_CLK_CTRL		0x704
 #define DP_DPTX_GCTL0			0x708
+#define DP_TIMING_MODEL_CTRL		0x884
+#define DP_TIMING_SYNC_CTRL		0xFF0
 
 #define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
 #define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
@@ -31,5 +49,28 @@
 #define DP_CFG_AUX_STATUS			GENMASK(11, 4)
 #define DP_CFG_SCRAMBLE_EN			BIT(0)
 #define DP_CFG_PAT_SEL				GENMASK(7, 4)
+#define DP_CFG_TIMING_GEN0_HACTIVE		GENMASK(31, 16)
+#define DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
+#define DP_CFG_TIMING_GEN0_VACTIVE		GENMASK(31, 16)
+#define DP_CFG_TIMING_GEN0_VBLANK		GENMASK(15, 0)
+#define DP_CFG_TIMING_GEN0_VFRONT_PORCH		GENMASK(31, 16)
+#define DP_CFG_STREAM_HACTIVE			GENMASK(31, 16)
+#define DP_CFG_STREAM_HBLANK			GENMASK(15, 0)
+#define DP_CFG_STREAM_HSYNC_WIDTH		GENMASK(15, 0)
+#define DP_CFG_STREAM_VACTIVE			GENMASK(31, 16)
+#define DP_CFG_STREAM_VBLANK			GENMASK(15, 0)
+#define DP_CFG_STREAM_VFRONT_PORCH		GENMASK(31, 16)
+#define DP_CFG_STREAM_VSYNC_WIDTH		GENMASK(15, 0)
+#define DP_CFG_STREAM_VSTART			GENMASK(31, 16)
+#define DP_CFG_STREAM_HSTART			GENMASK(15, 0)
+#define DP_CFG_STREAM_VSYNC_POLARITY		BIT(8)
+#define DP_CFG_STREAM_HSYNC_POLARITY		BIT(7)
+#define DP_CFG_STREAM_RGB_ENABLE		BIT(1)
+#define DP_CFG_STREAM_VIDEO_MAPPING		GENMASK(5, 2)
+#define DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1	GENMASK(31, 16)
+#define DP_CFG_STREAM_TU_SYMBOL_SIZE		GENMASK(5, 0)
+#define DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE	GENMASK(9, 6)
+#define DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
+#define DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
 
 #endif
-- 
2.33.0

[PATCH V3 drm-dp 4/4] drm/hisilicon/hibmc: add dp module in hibmc

[Yongbang Shi]

From: baihan li <libaihan@huawei.com>

To support DP interface displaying in hibmc driver. Add
a encoder and connector for DP modual.

Signed-off-by: baihan li <libaihan@huawei.com>
Signed-off-by: yongbang shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - fix build errors reported by kernel test robot <lkp@intel.com>
    Closes: https://lore.kernel.org/oe-kbuild-all/202410251136.1m7BlR68-lkp@intel.com/
v1 -> v2:
  - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
  - modifying drm_simple_encoder_init function, suggested by Dmitry Baryshkov.
  - refactoring struct hibmc_connector, suggested by Dmitry Baryshkov.
  - withdrawing the modification in hibmc_kms_init, suggested by Dmitry Baryshkov.
  v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile      |   2 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 128 ++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  16 +++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  21 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c   |  41 +++---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  |  20 +--
 6 files changed, 188 insertions(+), 40 deletions(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index 214228052ccf..95a4ed599d98 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
-	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
+	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o hibmc_drm_dp.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
new file mode 100644
index 000000000000..1e0f2ef39ba6
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2024 Hisilicon Limited.
+
+#include <linux/io.h>
+
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_edid.h>
+
+#include "hibmc_drm_drv.h"
+#include "dp/dp_hw.h"
+
+static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
+{
+	int count;
+
+	count = drm_add_modes_noedid(connector, connector->dev->mode_config.max_width,
+				     connector->dev->mode_config.max_height);
+	drm_set_preferred_mode(connector, 1024, 768); // temporary implementation
+
+	return count;
+}
+
+static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
+	.get_modes = hibmc_dp_connector_get_modes,
+};
+
+static const struct drm_connector_funcs hibmc_dp_conn_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int hibmc_dp_prepare(struct hibmc_dp *dp, struct drm_display_mode *mode)
+{
+	int ret;
+
+	hibmc_dp_display_en(dp, false);
+
+	ret = hibmc_dp_mode_set(dp, mode);
+	if (ret)
+		drm_err(dp->drm_dev, "hibmc dp mode set failed: %d\n", ret);
+
+	return ret;
+}
+
+static void hibmc_dp_encoder_enable(struct drm_encoder *drm_encoder,
+				    struct drm_atomic_state *state)
+{
+	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
+	struct drm_display_mode *mode = &drm_encoder->crtc->state->mode;
+
+	if (hibmc_dp_prepare(dp, mode))
+		return;
+
+	hibmc_dp_display_en(dp, true);
+}
+
+static void hibmc_dp_encoder_disable(struct drm_encoder *drm_encoder,
+				     struct drm_atomic_state *state)
+{
+	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
+
+	hibmc_dp_display_en(dp, false);
+}
+
+static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
+	.atomic_enable = hibmc_dp_encoder_enable,
+	.atomic_disable = hibmc_dp_encoder_disable,
+};
+
+void hibmc_dp_uninit(struct hibmc_drm_private *priv)
+{
+	hibmc_dp_hw_uninit(&priv->dp);
+}
+
+int hibmc_dp_init(struct hibmc_drm_private *priv)
+{
+	struct drm_device *dev = &priv->dev;
+	struct drm_crtc *crtc = &priv->crtc;
+	struct hibmc_dp *dp = &priv->dp;
+	struct drm_connector *connector = &dp->connector;
+	struct drm_encoder *encoder = &dp->encoder;
+	int ret;
+
+	dp->mmio = priv->mmio;
+	dp->drm_dev = dev;
+
+	ret = hibmc_dp_hw_init(&priv->dp);
+	if (ret) {
+		drm_err(dev, "hibmc dp hw init failed: %d\n", ret);
+		return ret;
+	}
+
+	hibmc_dp_display_en(&priv->dp, false);
+
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
+	if (ret) {
+		drm_err(dev, "init dp encoder failed: %d\n", ret);
+		goto err_init;
+	}
+
+	drm_encoder_helper_add(encoder, &hibmc_dp_encoder_helper_funcs);
+
+	ret = drm_connector_init(dev, connector, &hibmc_dp_conn_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret) {
+		drm_err(dev, "init dp connector failed: %d\n", ret);
+		goto err_init;
+	}
+
+	drm_connector_helper_add(connector, &hibmc_dp_conn_helper_funcs);
+
+	drm_connector_attach_encoder(connector, encoder);
+
+	return 0;
+
+err_init:
+	hibmc_dp_hw_uninit(&priv->dp);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 9f9b19ea0587..f98ac94a18b9 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -27,6 +27,10 @@
 #include "hibmc_drm_drv.h"
 #include "hibmc_drm_regs.h"
 
+#define DP_HOST_SERDES_CTRL		0x1f001c
+#define DP_HOST_SERDES_CTRL_VAL		0x8A00
+#define DP_HOST_SERDES_CTRL_MASK	0x7FFFE
+
 DEFINE_DRM_GEM_FOPS(hibmc_fops);
 
 static irqreturn_t hibmc_interrupt(int irq, void *arg)
@@ -116,6 +120,14 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
 		return ret;
 	}
 
+	/* if DP existed, init DP */
+	if ((readl(priv->mmio + DP_HOST_SERDES_CTRL) &
+	     DP_HOST_SERDES_CTRL_MASK) == DP_HOST_SERDES_CTRL_VAL) {
+		ret = hibmc_dp_init(priv);
+		if (ret)
+			drm_err(dev, "failed to init dp: %d\n", ret);
+	}
+
 	ret = hibmc_vdac_init(priv);
 	if (ret) {
 		drm_err(dev, "failed to init vdac: %d\n", ret);
@@ -239,6 +251,7 @@ static int hibmc_hw_init(struct hibmc_drm_private *priv)
 
 static int hibmc_unload(struct drm_device *dev)
 {
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
 	drm_atomic_helper_shutdown(dev);
@@ -247,6 +260,9 @@ static int hibmc_unload(struct drm_device *dev)
 
 	pci_disable_msi(to_pci_dev(dev->dev));
 
+	if (priv->dp.encoder.possible_crtcs)
+		hibmc_dp_uninit(priv);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index 6b566f3aeecb..1b78d313a6c2 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -19,10 +19,12 @@
 #include <linux/i2c.h>
 
 #include <drm/drm_framebuffer.h>
+#include "dp/dp_hw.h"
 
-struct hibmc_connector {
-	struct drm_connector base;
-
+struct hibmc_vdac {
+	struct drm_device *dev;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
 	struct i2c_adapter adapter;
 	struct i2c_algo_bit_data bit_data;
 };
@@ -35,13 +37,13 @@ struct hibmc_drm_private {
 	struct drm_device dev;
 	struct drm_plane primary_plane;
 	struct drm_crtc crtc;
-	struct drm_encoder encoder;
-	struct hibmc_connector connector;
+	struct hibmc_dp dp;
+	struct hibmc_vdac vdac;
 };
 
-static inline struct hibmc_connector *to_hibmc_connector(struct drm_connector *connector)
+static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
 {
-	return container_of(connector, struct hibmc_connector, base);
+	return container_of(connector, struct hibmc_vdac, connector);
 }
 
 static inline struct hibmc_drm_private *to_hibmc_drm_private(struct drm_device *dev)
@@ -57,6 +59,9 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
 int hibmc_de_init(struct hibmc_drm_private *priv);
 int hibmc_vdac_init(struct hibmc_drm_private *priv);
 
-int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_connector *connector);
+int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *connector);
+
+int hibmc_dp_init(struct hibmc_drm_private *priv);
+void hibmc_dp_uninit(struct hibmc_drm_private *priv);
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
index e6e48651c15c..99b3b77b5445 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
@@ -25,8 +25,8 @@
 
 static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
 {
-	struct hibmc_connector *hibmc_connector = data;
-	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector->base.dev);
+	struct hibmc_vdac *vdac = data;
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
 	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
 
 	if (value) {
@@ -45,8 +45,8 @@ static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
 
 static int hibmc_get_i2c_signal(void *data, u32 mask)
 {
-	struct hibmc_connector *hibmc_connector = data;
-	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector->base.dev);
+	struct hibmc_vdac *vdac = data;
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
 	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
 
 	if ((tmp_dir & mask) != mask) {
@@ -77,22 +77,21 @@ static int hibmc_ddc_getscl(void *data)
 	return hibmc_get_i2c_signal(data, I2C_SCL_MASK);
 }
 
-int hibmc_ddc_create(struct drm_device *drm_dev,
-		     struct hibmc_connector *connector)
+int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *vdac)
 {
-	connector->adapter.owner = THIS_MODULE;
-	snprintf(connector->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
-	connector->adapter.dev.parent = drm_dev->dev;
-	i2c_set_adapdata(&connector->adapter, connector);
-	connector->adapter.algo_data = &connector->bit_data;
-
-	connector->bit_data.udelay = 20;
-	connector->bit_data.timeout = usecs_to_jiffies(2000);
-	connector->bit_data.data = connector;
-	connector->bit_data.setsda = hibmc_ddc_setsda;
-	connector->bit_data.setscl = hibmc_ddc_setscl;
-	connector->bit_data.getsda = hibmc_ddc_getsda;
-	connector->bit_data.getscl = hibmc_ddc_getscl;
-
-	return i2c_bit_add_bus(&connector->adapter);
+	vdac->adapter.owner = THIS_MODULE;
+	snprintf(vdac->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
+	vdac->adapter.dev.parent = drm_dev->dev;
+	i2c_set_adapdata(&vdac->adapter, vdac);
+	vdac->adapter.algo_data = &vdac->bit_data;
+
+	vdac->bit_data.udelay = 20;
+	vdac->bit_data.timeout = usecs_to_jiffies(2000);
+	vdac->bit_data.data = vdac;
+	vdac->bit_data.setsda = hibmc_ddc_setsda;
+	vdac->bit_data.setscl = hibmc_ddc_setscl;
+	vdac->bit_data.getsda = hibmc_ddc_getsda;
+	vdac->bit_data.getscl = hibmc_ddc_getscl;
+
+	return i2c_bit_add_bus(&vdac->adapter);
 }
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 409c551c92af..05e19ea4c9f9 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -24,11 +24,11 @@
 
 static int hibmc_connector_get_modes(struct drm_connector *connector)
 {
-	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
+	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
 	const struct drm_edid *drm_edid;
 	int count;
 
-	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
+	drm_edid = drm_edid_read_ddc(connector, &vdac->adapter);
 
 	drm_edid_connector_update(connector, drm_edid);
 
@@ -51,9 +51,9 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
 
 static void hibmc_connector_destroy(struct drm_connector *connector)
 {
-	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
+	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
 
-	i2c_del_adapter(&hibmc_connector->adapter);
+	i2c_del_adapter(&vdac->adapter);
 	drm_connector_cleanup(connector);
 }
 
@@ -93,20 +93,20 @@ static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
 int hibmc_vdac_init(struct hibmc_drm_private *priv)
 {
 	struct drm_device *dev = &priv->dev;
-	struct hibmc_connector *hibmc_connector = &priv->connector;
-	struct drm_encoder *encoder = &priv->encoder;
+	struct hibmc_vdac *vdac = &priv->vdac;
+	struct drm_encoder *encoder = &vdac->encoder;
 	struct drm_crtc *crtc = &priv->crtc;
-	struct drm_connector *connector = &hibmc_connector->base;
+	struct drm_connector *connector = &vdac->connector;
 	int ret;
 
-	ret = hibmc_ddc_create(dev, hibmc_connector);
+	ret = hibmc_ddc_create(dev, vdac);
 	if (ret) {
 		drm_err(dev, "failed to create ddc: %d\n", ret);
 		return ret;
 	}
 
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
-	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
 	if (ret) {
 		drm_err(dev, "failed to init encoder: %d\n", ret);
 		return ret;
@@ -117,7 +117,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &hibmc_connector_funcs,
 					  DRM_MODE_CONNECTOR_VGA,
-					  &hibmc_connector->adapter);
+					  &vdac->adapter);
 	if (ret) {
 		drm_err(dev, "failed to init connector: %d\n", ret);
 		return ret;
-- 
2.33.0

Re: [PATCH V3 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc

[Dmitry Baryshkov]

On Fri, Nov 01, 2024 at 06:50:25PM +0800, Yongbang Shi wrote:
> From: baihan li <libaihan@huawei.com>
> 
> Add dp aux read/write functions. They are basic functions
> and will be used later.
> 
> Signed-off-by: baihan li <libaihan@huawei.com>
> Signed-off-by: yongbang shi <shiyongbang@huawei.com>

Is this a proper capitalization of your names? Usually the first letter
of each of them is capital.

> ---
> ChangeLog:
> v2 -> v3:
>   - put the macro definations in latter patch where they are actually used, suggested by Dmitry Baryshkov.
>   - rename some macro definations to make them sensible, suggested by Dmitry Baryshkov.
>   - using FIELD_PREP and FIELD_GET, suggested by Dmitry Baryshkov.
>   - using DP_DPCD_REV_foo, suggested by Dmitry Baryshkov.
>   - fix build errors reported by kernel test robot <lkp@intel.com>
>     Closes: https://lore.kernel.org/oe-kbuild-all/202410250305.UHKDhtxy-lkp@intel.com/
> v1 -> v2:
>   - using drm_dp_aux frame implement dp aux read and write functions, suggested by Jani Nikula.
>   - using drm dp header files' dp macros instead, suggested by Andy Yan.
>   v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c   | 162 ++++++++++++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h   |  23 +++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  58 +++++++
>  .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  19 ++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  27 +++
>  6 files changed, 291 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index d25c75e60d3d..8770ec6dfffd 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o
> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> +	       dp/dp_aux.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
> new file mode 100644
> index 000000000000..49ecda672109
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2024 Hisilicon Limited.
> +
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/minmax.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_print.h>
> +#include "dp_comm.h"
> +#include "dp_reg.h"
> +#include "dp_aux.h"
> +
> +#define DP_MIN_PULSE_NUM 0x9
> +
> +static void hibmc_dp_aux_reset(const struct dp_dev *dp)
> +{
> +	dp_reg_write_field(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x0);
> +	usleep_range(10, 15);
> +	dp_reg_write_field(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x1);
> +}
> +
> +static void hibmc_dp_aux_read_data(struct dp_dev *dp, u8 *buf, u8 size)
> +{
> +	u32 reg_num;
> +	u32 value;
> +	u32 num;
> +	u8 i, j;
> +
> +	reg_num = DIV_ROUND_UP(size, BYTES_IN_U32);
> +	for (i = 0; i < reg_num; i++) {
> +		/* number of bytes read from a single register */
> +		num = min(size - i * BYTES_IN_U32, BYTES_IN_U32);
> +		value = readl(dp->base + DP_AUX_RD_DATA0 + i * BYTES_IN_U32);
> +		/* convert the 32-bit value of the register to the buffer. */
> +		for (j = 0; j < num; j++)
> +			buf[i * BYTES_IN_U32 + j] = value >> (j * BITS_IN_U8);

put_unaligned_le32()

> +	}
> +}
> +
> +static void hibmc_dp_aux_write_data(struct dp_dev *dp, u8 *buf, u8 size)
> +{
> +	u32 reg_num;
> +	u32 value;
> +	u8 i, j;
> +	u32 num;
> +
> +	reg_num = DIV_ROUND_UP(size, BYTES_IN_U32);
> +	for (i = 0; i < reg_num; i++) {
> +		/* number of bytes written to a single register */
> +		num = min_t(u8, size - i * BYTES_IN_U32, BYTES_IN_U32);
> +		value = 0;
> +		/* obtain the 32-bit value written to a single register. */
> +		for (j = 0; j < num; j++)
> +			value |= buf[i * BYTES_IN_U32 + j] << (j * BITS_IN_U8);

get_unaligned_le32()

> +		/* writing data to a single register */
> +		writel(value, dp->base + DP_AUX_WR_DATA0 + i * BYTES_IN_U32);
> +	}
> +}
> +
> +static u32 hibmc_dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
> +{
> +	u32 aux_cmd = msg->request;
> +
> +	if (msg->size)
> +		aux_cmd |= FIELD_PREP(AUX_CMD_REQ_LEN, (msg->size - 1));
> +	else
> +		aux_cmd |= FIELD_PREP(AUX_CMD_I2C_ADDR_ONLY, 1);
> +
> +	aux_cmd |= FIELD_PREP(AUX_CMD_ADDR, msg->address);
> +
> +	return aux_cmd;
> +}
> +
> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
> +static int hibmc_dp_aux_parse_xfer(struct dp_dev *dp, struct drm_dp_aux_msg *msg)
> +{
> +	u32 buf_data_cnt;
> +	u32 aux_status;
> +	int ret = 0;
> +
> +	aux_status = readl(dp->base + DP_AUX_STATUS);
> +	msg->reply = FIELD_GET(DP_CFG_AUX_STATUS, aux_status);
> +
> +	if (aux_status & DP_CFG_AUX_TIMEOUT)
> +		return -ETIMEDOUT;
> +
> +	/* only address */
> +	if (!msg->size)
> +		return 0;
> +
> +	if (msg->reply != DP_AUX_NATIVE_REPLY_ACK)
> +		return 0;

Should this be an error instead?

> +
> +	buf_data_cnt = FIELD_GET(DP_CFG_AUX_READY_DATA_BYTE, aux_status);
> +
> +	switch (msg->request) {
> +	case DP_AUX_NATIVE_WRITE:
> +		ret = msg->size;
> +		break;
> +	case DP_AUX_I2C_WRITE | DP_AUX_I2C_MOT:
> +		if (buf_data_cnt == AUX_I2C_WRITE_SUCCESS)
> +			ret = msg->size;
> +		else if (buf_data_cnt == AUX_I2C_WRITE_PARTIAL_SUCCESS)
> +			ret = FIELD_GET(DP_CFG_AUX, aux_status);

Replace all ret= with returns. Nothing happens after the switch-case, so
return right away.

> +		break;
> +	case DP_AUX_NATIVE_READ:
> +	case DP_AUX_I2C_READ | DP_AUX_I2C_MOT:
> +		buf_data_cnt--;
> +		/* only the successful part of data is read */

Nit: only a part of data wass read successfully.
move the comment into the underlying `

> +		if (buf_data_cnt != msg->size) {
> +			ret = -EBUSY;
> +		} else { /* all data is successfully read */
> +			hibmc_dp_aux_read_data(dp, msg->buffer, msg->size);
> +			ret = msg->size;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
> +static ssize_t hibmc_dp_aux_xfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> +{
> +	struct dp_dev *dp = container_of(aux, struct dp_dev, aux);
> +	u32 aux_cmd;
> +	int ret;
> +	u32 val; /* val will be assigned at the beginning of readl_poll_timeout function */
> +
> +	writel(0, dp->base + DP_AUX_WR_DATA0);
> +	writel(0, dp->base + DP_AUX_WR_DATA1);
> +	writel(0, dp->base + DP_AUX_WR_DATA2);
> +	writel(0, dp->base + DP_AUX_WR_DATA3);
> +
> +	hibmc_dp_aux_write_data(dp, msg->buffer, msg->size);
> +
> +	aux_cmd = hibmc_dp_aux_build_cmd(msg);
> +	writel(aux_cmd, dp->base + DP_AUX_CMD_ADDR);
> +
> +	/* enable aux transfer */
> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_REQ, 0x1);
> +	ret = readl_poll_timeout(dp->base + DP_AUX_REQ, val, !(val & DP_CFG_AUX_REQ), 50, 5000);
> +	if (ret) {
> +		hibmc_dp_aux_reset(dp);
> +		return ret;
> +	}
> +
> +	return hibmc_dp_aux_parse_xfer(dp, msg);
> +}
> +
> +void hibmc_dp_aux_init(struct dp_dev *dp)
> +{
> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_MIN_PULSE_NUM, DP_MIN_PULSE_NUM);
> +
> +	dp->aux.transfer = hibmc_dp_aux_xfer;
> +	dp->aux.is_remote = 0;
> +	drm_dp_aux_init(&dp->aux);
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
> new file mode 100644
> index 000000000000..794bb0482ea7
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2024 Hisilicon Limited. */
> +
> +#ifndef DP_AUX_H
> +#define DP_AUX_H
> +
> +#include <linux/bitops.h>
> +#include "dp_comm.h"
> +
> +#define AUX_I2C_WRITE_SUCCESS		0x1

Move to the source file

> +#define AUX_I2C_WRITE_PARTIAL_SUCCESS	0x2

Ditto

> +#define EQ_MAX_RETRY			5

Unused

> +#define BYTES_IN_U32			4


> +#define BITS_IN_U8			8

This one will go away after using foo_unaligned_le32

> +
> +/* aux_cmd_addr register shift */
> +#define AUX_CMD_REQ_LEN			GENMASK(7, 4)
> +#define AUX_CMD_ADDR			GENMASK(27, 8)
> +#define AUX_CMD_I2C_ADDR_ONLY		BIT(28)

These can go to the source file too. Or to dp_reg.h. Don't spawn that
all over the place.

> +
> +void hibmc_dp_aux_init(struct dp_dev *dp);

As the header becomes nearly empty, merge the remnants to a generic
header.

> +
> +#endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> new file mode 100644
> index 000000000000..61a59fd59962
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2024 Hisilicon Limited. */
> +
> +#ifndef DP_COMM_H
> +#define DP_COMM_H
> +
> +#include <linux/types.h>
> +#include <linux/bitops.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/io.h>
> +
> +#include <drm/display/drm_dp_helper.h>
> +
> +#define dp_reg_read_field(addr, mask)				\

These are still using dp_ prefix. Also using static inline functions
might be easier to handle.

> +		FIELD_GET(mask, readl(addr))
> +
> +#define dp_field_modify(reg_value, mask, value) ({		\
> +		(reg_value) &= ~(mask);				\
> +		(reg_value) |= FIELD_PREP(mask, value); })
> +
> +#define dp_reg_write_field(addr, mask, val) ({			\
> +		typeof(addr) _addr = (addr);			\
> +		u32 _value = readl(_addr);			\
> +		dp_field_modify(_value, mask, val);		\
> +		writel(_value, _addr); })

I'm still very unsatisfied here. You have an RMW code with no locking.
If anything writes to the same location at the same time, the driver is
busted.

> +
> +struct link_status {

It is hibmc-specific, so please prefix accordingly.

> +	bool clock_recovered;
> +	bool channel_equalized;
> +	u8 cr_done_lanes;
> +};
> +
> +struct link_cap {
> +	int rx_dpcd_revision;
> +	u8 link_rate;
> +	u8 lanes;
> +	bool is_tps3;
> +	bool is_tps4;
> +};
> +
> +struct hibmc_dp_link {
> +	struct link_status status;
> +	u8 *train_set;
> +	struct link_cap cap;
> +};

Please don't define the structures/fields that are not used by the
patch. Introduce them when required.

> +
> +struct dp_dev {
> +	struct hibmc_dp_link link;
> +	struct drm_dp_aux aux;
> +	struct drm_device *dev;
> +	void __iomem *base;
> +	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> +};
> +
> +#endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> new file mode 100644
> index 000000000000..0b965e6ba7b3
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2024 Hisilicon Limited. */
> +
> +#ifndef DP_CONFIG_H
> +#define DP_CONFIG_H
> +
> +#define DP_BPP			24
> +#define DP_SYMBOL_PER_FCLK	4
> +#define DP_MSA1			0x20
> +#define DP_MSA2			0x845c00
> +#define DP_OFFSET		0x1e0000
> +#define DP_HDCP			0x2
> +#define DP_INT_RST		0xffff
> +#define DP_DPTX_RST		0x3ff
> +#define DP_CLK_EN		0x7
> +#define DP_SYNC_EN_MASK		0x3
> +#define DP_LINK_RATE_CAL	27
> +
> +#endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> new file mode 100644
> index 000000000000..83cf0cc06ae2
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2024 Hisilicon Limited. */
> +
> +#ifndef DP_REG_H
> +#define DP_REG_H
> +
> +#define DP_AUX_CMD_ADDR			0x50
> +#define DP_AUX_WR_DATA0			0x54
> +#define DP_AUX_WR_DATA1			0x58
> +#define DP_AUX_WR_DATA2			0x5c
> +#define DP_AUX_WR_DATA3			0x60
> +#define DP_AUX_RD_DATA0			0x64
> +#define DP_AUX_REQ			0x74
> +#define DP_AUX_STATUS			0x78
> +#define DP_DPTX_RST_CTRL		0x700
> +
> +#define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
> +#define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
> +#define DP_CFG_AUX_MIN_PULSE_NUM		GENMASK(13, 9)
> +#define DP_CFG_AUX_REQ				BIT(0)
> +#define DP_CFG_AUX_RST_N			BIT(4)
> +#define DP_CFG_AUX_TIMEOUT			BIT(0)
> +#define DP_CFG_AUX_READY_DATA_BYTE		GENMASK(16, 12)
> +#define DP_CFG_AUX				GENMASK(24, 17)
> +#define DP_CFG_AUX_STATUS			GENMASK(11, 4)
> +
> +#endif
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 drm-dp 2/4] drm/hisilicon/hibmc: add dp link moduel in hibmc

[Dmitry Baryshkov]

On Fri, Nov 01, 2024 at 06:50:26PM +0800, Yongbang Shi wrote:
> From: baihan li <libaihan@huawei.com>
> 
> Add link training process functions in this moduel.
> 
> Signed-off-by: baihan li <libaihan@huawei.com>
> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
> ---
> Changelog:
> v2 -> v3:
>   - using switchcase in dp_link_reduce_lane, suggested by Dmitry Baryshkov.
>   - deleting dp_link_pattern2dpcd function and using macros directly, suggested by Dmitry Baryshkov.
>   - deleting EFAULT error codes, suggested by Dmitry Baryshkov.
> v1 -> v2:
>   - using drm_dp_* functions implement dp link training process, suggested by Jani Nikula.
>   - fix build errors reported by kernel test robot <lkp@intel.com>
>     Closes: https://lore.kernel.org/oe-kbuild-all/202410031735.8iRZZR6T-lkp@intel.com/
>   v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile     |   2 +-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c | 346 +++++++++++++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h |  25 ++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h  |   8 +
>  4 files changed, 380 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index 8770ec6dfffd..94d77da88bbf 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> -	       dp/dp_aux.o
> +	       dp/dp_aux.o dp/dp_link.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
> new file mode 100644
> index 000000000000..7146de020c93
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2024 Hisilicon Limited.
> +
> +#include <linux/delay.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_print.h>
> +#include "dp_comm.h"
> +#include "dp_reg.h"
> +#include "dp_link.h"
> +#include "dp_aux.h"
> +
> +const u8 link_rate_map[] = {DP_LINK_BW_1_62, DP_LINK_BW_2_7,
> +			    DP_LINK_BW_5_4, DP_LINK_BW_8_1};

hibmc_dp_link_rate_map[]

> +
> +static int hibmc_dp_link_training_configure(struct dp_dev *dp)
> +{
> +	u8 buf[2];
> +	int ret;
> +
> +	/* DP 2 lane */
> +	dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_LANE_DATA_EN,
> +			   dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x3 : 0x1);
> +	dp_reg_write_field(dp->base + DP_DPTX_GCTL0, DP_CFG_PHY_LANE_NUM,
> +			   dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x1 : 0);
> +
> +	/* enhanced frame */
> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_FRAME_MODE, 0x1);
> +
> +	/* set rate and lane count */
> +	buf[0] = hibmc_dp_get_link_rate(dp->link.cap.link_rate);
> +	buf[1] = DP_LANE_COUNT_ENHANCED_FRAME_EN | dp->link.cap.lanes;
> +	ret = drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, buf, sizeof(buf));
> +	if (ret != sizeof(buf)) {
> +		drm_dbg_dp(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +
> +	/* set 8b/10b and downspread */
> +	buf[0] = 0x10;
> +	buf[1] = 0x1;
> +	ret = drm_dp_dpcd_write(&dp->aux, DP_DOWNSPREAD_CTRL, buf, sizeof(buf));
> +	if (ret != sizeof(buf)) {
> +		drm_dbg_dp(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", ret);
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +
> +	ret = drm_dp_read_dpcd_caps(&dp->aux, dp->dpcd);
> +	if (ret)
> +		drm_err(dp->dev, "dp aux read dpcd failed, ret: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int hibmc_dp_link_set_pattern(struct dp_dev *dp, int pattern)
> +{
> +	int ret;
> +	u8 val;
> +	u8 buf;
> +
> +	buf = (u8)pattern;
> +	if (pattern != DP_TRAINING_PATTERN_DISABLE && pattern != DP_TRAINING_PATTERN_4) {
> +		buf |= DP_LINK_SCRAMBLING_DISABLE;
> +		dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0x1);
> +	} else {
> +		dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0);
> +	}
> +
> +	switch (pattern) {
> +	case DP_TRAINING_PATTERN_1:
> +		val = 1;
> +		break;
> +	case DP_TRAINING_PATTERN_2:
> +		val = 2;
> +		break;
> +	case DP_TRAINING_PATTERN_3:
> +		val = 3;
> +		break;
> +	case DP_TRAINING_PATTERN_4:
> +		val = 4;
> +		break;
> +	default:
> +		val = 0;

return -EINVAL?

> +	}
> +
> +	dp_reg_write_field(dp->base + DP_PHYIF_CTRL0, DP_CFG_PAT_SEL, val);
> +
> +	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
> +	if (ret != sizeof(buf)) {
> +		drm_dbg_dp(dp->dev, "dp aux write training pattern set failed\n");
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hibmc_dp_link_training_cr_pre(struct dp_dev *dp)
> +{
> +	u8 *train_set = dp->link.train_set;
> +	int ret;
> +	u8 i;
> +
> +	ret = hibmc_dp_link_training_configure(dp);
> +	if (ret)
> +		return ret;
> +
> +	ret = hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_1);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < dp->link.cap.lanes; i++)
> +		train_set[i] = DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
> +
> +	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set, dp->link.cap.lanes);
> +	if (ret != dp->link.cap.lanes) {
> +		drm_dbg_dp(dp->dev, "dp aux write training lane set failed\n");
> +		return ret >= 0 ? -EIO : ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool hibmc_dp_link_get_adjust_train(struct dp_dev *dp, u8 lane_status[DP_LINK_STATUS_SIZE])
> +{
> +	u8 pre_emph[DP_LANE_NUM_MAX] = {0};
> +	u8 voltage[DP_LANE_NUM_MAX] = {0};
> +	bool changed = false;
> +	u8 train_set;
> +	u8 lane;
> +
> +	for (lane = 0; lane < dp->link.cap.lanes; lane++) {
> +		voltage[lane] = drm_dp_get_adjust_request_voltage(lane_status, lane);
> +		pre_emph[lane] = drm_dp_get_adjust_request_pre_emphasis(lane_status, lane);

train_set[lane] = drm_dp_get_adjust_request_voltage() |
                  drm_dp_get_adjust_request_pre_emphasis();

> +	}
> +
> +	for (lane = 0; lane < dp->link.cap.lanes; lane++) {
> +		train_set = voltage[lane] | pre_emph[lane];
> +		if (dp->link.train_set[lane] != train_set) {
> +			changed = true;
> +			dp->link.train_set[lane] = train_set;
> +		}
> +	}

	if (memcmp(dp->link.train_set, train_set)) {
		memcpy(dp->link.train_set, train_set);
		return true;
	}

	return false;

> +
> +	return changed;
> +}
> +
> +u8 hibmc_dp_get_link_rate(u8 index)
> +{
> +	return link_rate_map[index];
> +}
> +
> +static int hibmc_dp_link_reduce_rate(struct dp_dev *dp)
> +{
> +	if (dp->link.cap.link_rate > 0) {
> +		dp->link.cap.link_rate--;
> +		return 0;
> +	}
> +
> +	drm_err(dp->dev, "dp link training reduce rate failed, already lowest rate\n");
> +
> +	return -ELNRNG;

Link number out of range? Just -EIO or -EINVAL

> +}
> +
> +static int hibmc_dp_link_reduce_lane(struct dp_dev *dp)
> +{
> +	switch (dp->link.cap.lanes) {
> +	case DP_LANE_NUM_2:
> +		dp->link.cap.lanes--;
> +		break;
> +	case DP_LANE_NUM_1:
> +		drm_err(dp->dev, "dp link training reduce lane failed, already reach minimum\n");
> +		return -ELNRNG;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hibmc_dp_link_training_cr(struct dp_dev *dp)
> +{
> +	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
> +	bool level_changed;
> +	u32 voltage_tries;
> +	u32 cr_tries;
> +	u32 max_cr;
> +	int ret;
> +
> +	/*
> +	 * DP 1.4 spec define 10 for maxtries value, for pre DP 1.4 version set a limit of 80
> +	 * (4 voltage levels x 4 preemphasis levels x 5 identical voltage retries)
> +	 */
> +	max_cr = dp->link.cap.rx_dpcd_revision >= DP_DPCD_REV_14 ? 10 : 80;
> +
> +	voltage_tries = 1;
> +	for (cr_tries = 0; cr_tries < max_cr; cr_tries++) {
> +		drm_dp_link_train_clock_recovery_delay(&dp->aux, dp->dpcd);
> +
> +		ret = drm_dp_dpcd_read_link_status(&dp->aux, lane_status);
> +		if (ret != DP_LINK_STATUS_SIZE) {
> +			drm_err(dp->dev, "Get lane status failed\n");
> +			return ret;
> +		}
> +
> +		if (drm_dp_clock_recovery_ok(lane_status, dp->link.cap.lanes)) {
> +			drm_info(dp->dev, "dp link training cr done\n");
> +			dp->link.status.clock_recovered = true;
> +			return 0;
> +		}
> +
> +		if (voltage_tries == 5) {
> +			drm_info(dp->dev, "same voltage tries 5 times\n");
> +			dp->link.status.clock_recovered = false;
> +			return 0;
> +		}
> +
> +		level_changed = hibmc_dp_link_get_adjust_train(dp, lane_status);
> +		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, dp->link.train_set,
> +					dp->link.cap.lanes);
> +		if (ret != dp->link.cap.lanes) {
> +			drm_dbg_dp(dp->dev, "Update link training failed\n");
> +			return ret >= 0 ? -EIO : ret;
> +		}
> +
> +		voltage_tries = level_changed ? 1 : voltage_tries + 1;
> +	}
> +
> +	drm_err(dp->dev, "dp link training clock recovery %u timers failed\n", max_cr);
> +	dp->link.status.clock_recovered = false;
> +
> +	return 0;
> +}
> +
> +static int hibmc_dp_link_training_channel_eq(struct dp_dev *dp)
> +{
> +	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
> +	u8 eq_tries;
> +	int tps;
> +	int ret;
> +
> +	if (dp->link.cap.is_tps4)
> +		tps = DP_TRAINING_PATTERN_4;
> +	else if (dp->link.cap.is_tps3)
> +		tps = DP_TRAINING_PATTERN_3;
> +	else
> +		tps = DP_TRAINING_PATTERN_2;
> +
> +	ret = hibmc_dp_link_set_pattern(dp, tps);
> +	if (ret)
> +		return ret;
> +
> +	for (eq_tries = 0; eq_tries < EQ_MAX_RETRY; eq_tries++) {
> +		drm_dp_link_train_channel_eq_delay(&dp->aux, dp->dpcd);
> +
> +		ret = drm_dp_dpcd_read_link_status(&dp->aux, lane_status);
> +		if (ret != DP_LINK_STATUS_SIZE) {
> +			drm_err(dp->dev, "get lane status failed\n");
> +			break;
> +		}
> +
> +		if (!drm_dp_clock_recovery_ok(lane_status, dp->link.cap.lanes)) {
> +			drm_info(dp->dev, "clock recovery check failed\n");
> +			drm_info(dp->dev, "cannot continue channel equalization\n");
> +			dp->link.status.clock_recovered = false;
> +			break;
> +		}
> +
> +		if (drm_dp_channel_eq_ok(lane_status, dp->link.cap.lanes)) {
> +			dp->link.status.channel_equalized = true;
> +			drm_info(dp->dev, "dp link training eq done\n");
> +			break;
> +		}
> +
> +		hibmc_dp_link_get_adjust_train(dp, lane_status);
> +		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
> +					dp->link.train_set, dp->link.cap.lanes);
> +		if (ret != dp->link.cap.lanes) {
> +			drm_dbg_dp(dp->dev, "Update link training failed\n");
> +			ret = (ret >= 0) ? -EIO : ret;
> +			break;
> +		}
> +	}
> +
> +	if (eq_tries == EQ_MAX_RETRY)
> +		drm_err(dp->dev, "channel equalization failed %u times\n", eq_tries);
> +
> +	hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int hibmc_dp_link_downgrade_training_cr(struct dp_dev *dp)
> +{
> +	if (hibmc_dp_link_reduce_rate(dp))
> +		return hibmc_dp_link_reduce_lane(dp);
> +
> +	return 0;
> +}
> +
> +static int hibmc_dp_link_downgrade_training_eq(struct dp_dev *dp)
> +{
> +	if ((dp->link.status.clock_recovered && !dp->link.status.channel_equalized)) {
> +		if (!hibmc_dp_link_reduce_lane(dp))
> +			return 0;
> +	}
> +
> +	return hibmc_dp_link_reduce_rate(dp);
> +}
> +
> +int hibmc_dp_link_training(struct dp_dev *dp)
> +{
> +	struct hibmc_dp_link *link = &dp->link;
> +	int ret;
> +
> +	while (true) {
> +		ret = hibmc_dp_link_training_cr_pre(dp);
> +		if (ret)
> +			goto err;
> +
> +		ret = hibmc_dp_link_training_cr(dp);
> +		if (ret)
> +			goto err;
> +
> +		if (!link->status.clock_recovered) {
> +			ret = hibmc_dp_link_downgrade_training_cr(dp);
> +			if (ret)
> +				goto err;
> +			continue;
> +		}
> +
> +		ret = hibmc_dp_link_training_channel_eq(dp);
> +		if (ret)
> +			goto err;
> +
> +		if (!link->status.channel_equalized) {
> +			ret = hibmc_dp_link_downgrade_training_eq(dp);
> +			if (ret)
> +				goto err;
> +			continue;
> +		}
> +
> +		return 0;
> +	}
> +
> +err:
> +	hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h
> new file mode 100644
> index 000000000000..b4958d122083
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2024 Hisilicon Limited. */
> +
> +#ifndef DP_LINK_H
> +#define DP_LINK_H
> +
> +#include "dp_comm.h"
> +
> +#define DP_LANE_NUM_MAX		2
> +#define DP_LANE_STATUS_SIZE	1

unused

> +#define DP_LANE_NUM_1		0x1
> +#define DP_LANE_NUM_2		0x2

Are you sure that you need to define 1 and 2? It's not like cap.lanes
requires a define to be understood.

> +
> +enum dp_pattern_e {
> +	DP_PATTERN_NO = 0,
> +	DP_PATTERN_TPS1,
> +	DP_PATTERN_TPS2,
> +	DP_PATTERN_TPS3,
> +	DP_PATTERN_TPS4,
> +};

Unused

> +
> +int hibmc_dp_link_training(struct dp_dev *dp);
> +u8 hibmc_dp_get_link_rate(u8 index);
> +
> +#endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> index 83cf0cc06ae2..1032f6cde761 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -12,16 +12,24 @@
>  #define DP_AUX_RD_DATA0			0x64
>  #define DP_AUX_REQ			0x74
>  #define DP_AUX_STATUS			0x78
> +#define DP_PHYIF_CTRL0			0xa0
> +#define DP_VIDEO_CTRL			0x100
>  #define DP_DPTX_RST_CTRL		0x700
> +#define DP_DPTX_GCTL0			0x708
>  
>  #define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
>  #define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
> +#define DP_CFG_STREAM_FRAME_MODE		BIT(6)
>  #define DP_CFG_AUX_MIN_PULSE_NUM		GENMASK(13, 9)
> +#define DP_CFG_LANE_DATA_EN			GENMASK(11, 8)
> +#define DP_CFG_PHY_LANE_NUM			GENMASK(2, 1)
>  #define DP_CFG_AUX_REQ				BIT(0)
>  #define DP_CFG_AUX_RST_N			BIT(4)
>  #define DP_CFG_AUX_TIMEOUT			BIT(0)
>  #define DP_CFG_AUX_READY_DATA_BYTE		GENMASK(16, 12)
>  #define DP_CFG_AUX				GENMASK(24, 17)
>  #define DP_CFG_AUX_STATUS			GENMASK(11, 4)
> +#define DP_CFG_SCRAMBLE_EN			BIT(0)
> +#define DP_CFG_PAT_SEL				GENMASK(7, 4)
>  
>  #endif
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 drm-dp 3/4] drm/hisilicon/hibmc: add dp hw moduel in hibmc

[Dmitry Baryshkov]

On Fri, Nov 01, 2024 at 06:50:27PM +0800, Yongbang Shi wrote:
> From: baihan li <libaihan@huawei.com>
> 
> Build a dp level that hibmc driver can enable dp by
> calling their functions.
> 
> Signed-off-by: baihan li <libaihan@huawei.com>
> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - fix build errors reported by kernel test robot <lkp@intel.com>
>     Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@intel.com/
> v1 -> v2:
>   - changed some defines and functions to former patch, suggested by Dmitry Baryshkov.
>   - sorting the headers including in dp_hw.h and hibmc_drm_drv.c files, suggested by Dmitry Baryshkov.
>   - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
>   - fix build errors reported by kernel test robot <lkp@intel.com>
>     Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@intel.com/
>   v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile    |   2 +-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c  | 237 ++++++++++++++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h  |  31 +++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h |  41 ++++
>  4 files changed, 310 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index 94d77da88bbf..214228052ccf 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> -	       dp/dp_aux.o dp/dp_link.o
> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> new file mode 100644
> index 000000000000..214897798bdb
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2024 Hisilicon Limited.
> +
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include "dp_config.h"
> +#include "dp_comm.h"
> +#include "dp_reg.h"
> +#include "dp_hw.h"
> +#include "dp_link.h"
> +#include "dp_aux.h"
> +
> +static int hibmc_dp_link_init(struct dp_dev *dp)
> +{
> +	dp->link.cap.lanes = 2;
> +	dp->link.train_set = devm_kzalloc(dp->dev->dev,
> +					  dp->link.cap.lanes * sizeof(u8), GFP_KERNEL);

Can you replace it just with an array, removing a need for an additional
allocation?

> +	if (!dp->link.train_set)
> +		return -ENOMEM;
> +
> +	dp->link.cap.link_rate = 1;

Ok, this is why I don't link using indices for link rates. Which rate is
this? Unlike cap.lanes this is pure magic number. I think it should be
handled other way around: store actual link rate and convert to the
register value when required.

> +
> +	return 0;
> +}
> +
> +static void hibmc_dp_set_tu(struct dp_dev *dp, struct drm_display_mode *mode)
> +{
> +	u32 tu_symbol_frac_size;
> +	u32 tu_symbol_size;
> +	u32 rate_ks;
> +	u8 lane_num;
> +	u32 value;
> +	u32 bpp;
> +
> +	lane_num = dp->link.cap.lanes;
> +	if (lane_num == 0) {
> +		drm_err(dp->dev, "set tu failed, lane num cannot be 0!\n");
> +		return;
> +	}
> +
> +	bpp = DP_BPP;

Where is this defined? Is it hibmc-specific or a generic value?

> +	rate_ks = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;

same question

> +	value = (mode->clock * bpp * 5) / (61 * lane_num * rate_ks);
> +
> +	if (value % 10 == 9) { /* 9 carry */
> +		tu_symbol_size = value / 10 + 1;
> +		tu_symbol_frac_size = 0;
> +	} else {
> +		tu_symbol_size = value / 10;
> +		tu_symbol_frac_size = value % 10 + 1;
> +	}
> +
> +	drm_info(dp->dev, "tu value: %u.%u value: %u\n",
> +		 tu_symbol_size, tu_symbol_frac_size, value);
> +
> +	dp_reg_write_field(dp->base + DP_VIDEO_PACKET,
> +			   DP_CFG_STREAM_TU_SYMBOL_SIZE, tu_symbol_size);
> +	dp_reg_write_field(dp->base + DP_VIDEO_PACKET,
> +			   DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE, tu_symbol_frac_size);
> +}
> +
> +static void hibmc_dp_set_sst(struct dp_dev *dp, struct drm_display_mode *mode)
> +{
> +	u32 hblank_size;
> +	u32 htotal_size;
> +	u32 htotal_int;
> +	u32 hblank_int;
> +	u32 fclk; /* flink_clock */
> +
> +	fclk = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
> +
> +	/* ssc: 9947 / 10000 = 0.9947 */

This is obvious. More interesting question might be what exactly is
0.9947 (or 0.53 %).

> +	htotal_int = mode->htotal * 9947 / 10000;
> +	htotal_size = (u32)(htotal_int * fclk / (DP_SYMBOL_PER_FCLK * (mode->clock / 1000)));

Drop u32 ?

> +
> +	/* ssc: max effect bandwidth 53 / 10000 = 0.53% */
> +	hblank_int = (mode->htotal - mode->hdisplay) - mode->hdisplay * 53 / 10000;
> +	hblank_size = hblank_int * fclk * 9947 /
> +		      (mode->clock * 10 * DP_SYMBOL_PER_FCLK);
> +
> +	drm_info(dp->dev, "h_active %u v_active %u htotal_size %u hblank_size %u",
> +		 mode->hdisplay, mode->vdisplay, htotal_size, hblank_size);
> +	drm_info(dp->dev, "flink_clock %u pixel_clock %d", fclk, (mode->clock / 1000));
> +
> +	dp_reg_write_field(dp->base + DP_VIDEO_HORIZONTAL_SIZE,
> +			   DP_CFG_STREAM_HTOTAL_SIZE, htotal_size);
> +	dp_reg_write_field(dp->base + DP_VIDEO_HORIZONTAL_SIZE,
> +			   DP_CFG_STREAM_HBLANK_SIZE, hblank_size);
> +}
> +
> +static void hibmc_dp_link_cfg(struct dp_dev *dp, struct drm_display_mode *mode)
> +{
> +	u32 timing_delay;
> +	u32 vblank;
> +	u32 hstart;
> +	u32 vstart;
> +
> +	vblank = mode->vtotal - mode->vdisplay;
> +	timing_delay = mode->htotal - mode->hsync_start;
> +	hstart = mode->htotal - mode->hsync_start;
> +	vstart = mode->vtotal - mode->vsync_start;
> +
> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG0,
> +			   DP_CFG_TIMING_GEN0_HBLANK, (mode->htotal - mode->hdisplay));
> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG0,
> +			   DP_CFG_TIMING_GEN0_HACTIVE, mode->hdisplay);
> +
> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG2,
> +			   DP_CFG_TIMING_GEN0_VBLANK, vblank);
> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG2,
> +			   DP_CFG_TIMING_GEN0_VACTIVE, mode->vdisplay);
> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG3,
> +			   DP_CFG_TIMING_GEN0_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
> +
> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG0,
> +			   DP_CFG_STREAM_HACTIVE, mode->hdisplay);
> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG0,
> +			   DP_CFG_STREAM_HBLANK, (mode->htotal - mode->hdisplay));
> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG2,
> +			   DP_CFG_STREAM_HSYNC_WIDTH, (mode->hsync_end - mode->hsync_start));
> +
> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG1,
> +			   DP_CFG_STREAM_VACTIVE, mode->vdisplay);
> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG1,
> +			   DP_CFG_STREAM_VBLANK, vblank);
> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG3,
> +			   DP_CFG_STREAM_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG3,
> +			   DP_CFG_STREAM_VSYNC_WIDTH, (mode->vsync_end - mode->vsync_start));
> +
> +	dp_reg_write_field(dp->base + DP_VIDEO_MSA0,
> +			   DP_CFG_STREAM_VSTART, vstart);
> +	dp_reg_write_field(dp->base + DP_VIDEO_MSA0,
> +			   DP_CFG_STREAM_HSTART, hstart);
> +
> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_VSYNC_POLARITY,
> +			   mode->flags & DRM_MODE_FLAG_PVSYNC ? 1 : 0);
> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_HSYNC_POLARITY,
> +			   mode->flags & DRM_MODE_FLAG_PHSYNC ? 1 : 0);
> +
> +	/* MSA mic 0 and 1 */
> +	writel(DP_MSA1, dp->base + DP_VIDEO_MSA1);
> +	writel(DP_MSA2, dp->base + DP_VIDEO_MSA2);
> +
> +	hibmc_dp_set_tu(dp, mode);
> +
> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_RGB_ENABLE, 0x1);
> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_VIDEO_MAPPING, 0);
> +
> +	/* divide 2: up even */
> +	if (timing_delay % 2)
> +		timing_delay++;
> +
> +	dp_reg_write_field(dp->base + DP_TIMING_MODEL_CTRL,
> +			   DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1, timing_delay);
> +
> +	hibmc_dp_set_sst(dp, mode);
> +}
> +
> +int hibmc_dp_hw_init(struct hibmc_dp *dp)
> +{
> +	struct drm_device *drm_dev = dp->drm_dev;
> +	struct dp_dev *dp_dev;
> +	int ret;
> +
> +	dp_dev = devm_kzalloc(drm_dev->dev, sizeof(struct dp_dev), GFP_KERNEL);
> +	if (!dp_dev)
> +		return -ENOMEM;
> +
> +	dp->dp_dev = dp_dev;
> +
> +	dp_dev->dev = drm_dev;
> +	dp_dev->base = dp->mmio + DP_OFFSET;
> +
> +	hibmc_dp_aux_init(dp_dev);
> +
> +	ret = hibmc_dp_link_init(dp_dev);
> +	if (ret) {
> +		drm_err(drm_dev, "dp link init failed\n");
> +		return ret;
> +	}
> +
> +	/* hdcp data */
> +	writel(DP_HDCP, dp_dev->base + DP_HDCP_CFG);
> +	/* int init */
> +	writel(0, dp_dev->base + DP_INTR_ENABLE);
> +	writel(DP_INT_RST, dp_dev->base + DP_INTR_ORIGINAL_STATUS);
> +	/* rst */
> +	writel(DP_DPTX_RST, dp_dev->base + DP_DPTX_RST_CTRL);
> +	/* clock enable */
> +	writel(DP_CLK_EN, dp_dev->base + DP_DPTX_CLK_CTRL);
> +
> +	return 0;
> +}
> +
> +void hibmc_dp_hw_uninit(struct hibmc_dp *dp)
> +{
> +	// keep this uninit interface in the future use

no reason to, introduce it when required, not the other way around

> +}
> +
> +void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
> +{
> +	struct dp_dev *dp_dev = dp->dp_dev;
> +
> +	if (enable) {
> +		dp_reg_write_field(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0x1);
> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
> +		dp_reg_write_field(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0x1);
> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
> +	} else {
> +		dp_reg_write_field(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0);
> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
> +		dp_reg_write_field(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0);
> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
> +	}
> +
> +	msleep(50);
> +}
> +
> +int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
> +{
> +	struct dp_dev *dp_dev = dp->dp_dev;
> +	int ret;
> +
> +	if (!dp_dev->link.status.channel_equalized) {
> +		ret = hibmc_dp_link_training(dp_dev);
> +		if (ret) {
> +			drm_err(dp->drm_dev, "dp link training failed, ret: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	hibmc_dp_display_en(dp, false);
> +	hibmc_dp_link_cfg(dp_dev, mode);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> new file mode 100644
> index 000000000000..de802aaa8b4a
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2024 Hisilicon Limited. */
> +
> +#ifndef DP_KAPI_H
> +#define DP_KAPI_H
> +
> +#include <linux/types.h>
> +#include <linux/delay.h>
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_print.h>
> +#include <video/videomode.h>
> +
> +struct dp_dev;

hibmc_dp_dev

> +
> +struct hibmc_dp {
> +	struct dp_dev *dp_dev;
> +	struct drm_device *drm_dev;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
> +	void __iomem *mmio;
> +};
> +
> +int hibmc_dp_hw_init(struct hibmc_dp *dp);
> +void hibmc_dp_hw_uninit(struct hibmc_dp *dp);
> +int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
> +void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
> +
> +#endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> index 1032f6cde761..3dcb847057a4 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -14,8 +14,26 @@
>  #define DP_AUX_STATUS			0x78
>  #define DP_PHYIF_CTRL0			0xa0
>  #define DP_VIDEO_CTRL			0x100
> +#define DP_VIDEO_CONFIG0		0x104
> +#define DP_VIDEO_CONFIG1		0x108
> +#define DP_VIDEO_CONFIG2		0x10c
> +#define DP_VIDEO_CONFIG3		0x110
> +#define DP_VIDEO_PACKET			0x114
> +#define DP_VIDEO_MSA0			0x118
> +#define DP_VIDEO_MSA1			0x11c
> +#define DP_VIDEO_MSA2			0x120
> +#define DP_VIDEO_HORIZONTAL_SIZE	0X124
> +#define DP_TIMING_GEN_CONFIG0		0x26c
> +#define DP_TIMING_GEN_CONFIG2		0x274
> +#define DP_TIMING_GEN_CONFIG3		0x278
> +#define DP_HDCP_CFG			0x600
> +#define DP_INTR_ENABLE			0x720
> +#define DP_INTR_ORIGINAL_STATUS		0x728
>  #define DP_DPTX_RST_CTRL		0x700
> +#define DP_DPTX_CLK_CTRL		0x704
>  #define DP_DPTX_GCTL0			0x708
> +#define DP_TIMING_MODEL_CTRL		0x884
> +#define DP_TIMING_SYNC_CTRL		0xFF0
>  
>  #define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
>  #define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
> @@ -31,5 +49,28 @@
>  #define DP_CFG_AUX_STATUS			GENMASK(11, 4)
>  #define DP_CFG_SCRAMBLE_EN			BIT(0)
>  #define DP_CFG_PAT_SEL				GENMASK(7, 4)
> +#define DP_CFG_TIMING_GEN0_HACTIVE		GENMASK(31, 16)
> +#define DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
> +#define DP_CFG_TIMING_GEN0_VACTIVE		GENMASK(31, 16)
> +#define DP_CFG_TIMING_GEN0_VBLANK		GENMASK(15, 0)
> +#define DP_CFG_TIMING_GEN0_VFRONT_PORCH		GENMASK(31, 16)
> +#define DP_CFG_STREAM_HACTIVE			GENMASK(31, 16)
> +#define DP_CFG_STREAM_HBLANK			GENMASK(15, 0)
> +#define DP_CFG_STREAM_HSYNC_WIDTH		GENMASK(15, 0)
> +#define DP_CFG_STREAM_VACTIVE			GENMASK(31, 16)
> +#define DP_CFG_STREAM_VBLANK			GENMASK(15, 0)
> +#define DP_CFG_STREAM_VFRONT_PORCH		GENMASK(31, 16)
> +#define DP_CFG_STREAM_VSYNC_WIDTH		GENMASK(15, 0)
> +#define DP_CFG_STREAM_VSTART			GENMASK(31, 16)
> +#define DP_CFG_STREAM_HSTART			GENMASK(15, 0)
> +#define DP_CFG_STREAM_VSYNC_POLARITY		BIT(8)
> +#define DP_CFG_STREAM_HSYNC_POLARITY		BIT(7)
> +#define DP_CFG_STREAM_RGB_ENABLE		BIT(1)
> +#define DP_CFG_STREAM_VIDEO_MAPPING		GENMASK(5, 2)
> +#define DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1	GENMASK(31, 16)
> +#define DP_CFG_STREAM_TU_SYMBOL_SIZE		GENMASK(5, 0)
> +#define DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE	GENMASK(9, 6)
> +#define DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
> +#define DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
>  
>  #endif
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 drm-dp 4/4] drm/hisilicon/hibmc: add dp module in hibmc

[Dmitry Baryshkov]

On Fri, Nov 01, 2024 at 06:50:28PM +0800, Yongbang Shi wrote:
> From: baihan li <libaihan@huawei.com>
> 
> To support DP interface displaying in hibmc driver. Add
> a encoder and connector for DP modual.
> 
> Signed-off-by: baihan li <libaihan@huawei.com>
> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - fix build errors reported by kernel test robot <lkp@intel.com>
>     Closes: https://lore.kernel.org/oe-kbuild-all/202410251136.1m7BlR68-lkp@intel.com/
> v1 -> v2:
>   - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
>   - modifying drm_simple_encoder_init function, suggested by Dmitry Baryshkov.
>   - refactoring struct hibmc_connector, suggested by Dmitry Baryshkov.
>   - withdrawing the modification in hibmc_kms_init, suggested by Dmitry Baryshkov.
>   v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile      |   2 +-
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 128 ++++++++++++++++++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  16 +++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  21 +--
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c   |  41 +++---
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  |  20 +--
>  6 files changed, 188 insertions(+), 40 deletions(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index 214228052ccf..95a4ed599d98 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> -	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o hibmc_drm_dp.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> new file mode 100644
> index 000000000000..1e0f2ef39ba6
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2024 Hisilicon Limited.
> +
> +#include <linux/io.h>
> +
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_edid.h>
> +
> +#include "hibmc_drm_drv.h"
> +#include "dp/dp_hw.h"
> +
> +static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
> +{
> +	int count;
> +
> +	count = drm_add_modes_noedid(connector, connector->dev->mode_config.max_width,
> +				     connector->dev->mode_config.max_height);
> +	drm_set_preferred_mode(connector, 1024, 768); // temporary implementation
> +
> +	return count;
> +}
> +
> +static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
> +	.get_modes = hibmc_dp_connector_get_modes,
> +};
> +
> +static const struct drm_connector_funcs hibmc_dp_conn_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int hibmc_dp_prepare(struct hibmc_dp *dp, struct drm_display_mode *mode)

inline at the calling site

> +{
> +	int ret;
> +
> +	hibmc_dp_display_en(dp, false);
> +
> +	ret = hibmc_dp_mode_set(dp, mode);
> +	if (ret)
> +		drm_err(dp->drm_dev, "hibmc dp mode set failed: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void hibmc_dp_encoder_enable(struct drm_encoder *drm_encoder,
> +				    struct drm_atomic_state *state)
> +{
> +	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
> +	struct drm_display_mode *mode = &drm_encoder->crtc->state->mode;
> +
> +	if (hibmc_dp_prepare(dp, mode))
> +		return;
> +
> +	hibmc_dp_display_en(dp, true);
> +}
> +
> +static void hibmc_dp_encoder_disable(struct drm_encoder *drm_encoder,
> +				     struct drm_atomic_state *state)
> +{
> +	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
> +
> +	hibmc_dp_display_en(dp, false);
> +}
> +
> +static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
> +	.atomic_enable = hibmc_dp_encoder_enable,
> +	.atomic_disable = hibmc_dp_encoder_disable,
> +};
> +
> +void hibmc_dp_uninit(struct hibmc_drm_private *priv)
> +{
> +	hibmc_dp_hw_uninit(&priv->dp);
> +}
> +
> +int hibmc_dp_init(struct hibmc_drm_private *priv)
> +{
> +	struct drm_device *dev = &priv->dev;
> +	struct drm_crtc *crtc = &priv->crtc;
> +	struct hibmc_dp *dp = &priv->dp;
> +	struct drm_connector *connector = &dp->connector;
> +	struct drm_encoder *encoder = &dp->encoder;
> +	int ret;
> +
> +	dp->mmio = priv->mmio;
> +	dp->drm_dev = dev;
> +
> +	ret = hibmc_dp_hw_init(&priv->dp);
> +	if (ret) {
> +		drm_err(dev, "hibmc dp hw init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	hibmc_dp_display_en(&priv->dp, false);
> +
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
> +	if (ret) {
> +		drm_err(dev, "init dp encoder failed: %d\n", ret);
> +		goto err_init;
> +	}
> +
> +	drm_encoder_helper_add(encoder, &hibmc_dp_encoder_helper_funcs);
> +
> +	ret = drm_connector_init(dev, connector, &hibmc_dp_conn_funcs,
> +				 DRM_MODE_CONNECTOR_DisplayPort);
> +	if (ret) {
> +		drm_err(dev, "init dp connector failed: %d\n", ret);
> +		goto err_init;
> +	}
> +
> +	drm_connector_helper_add(connector, &hibmc_dp_conn_helper_funcs);
> +
> +	drm_connector_attach_encoder(connector, encoder);
> +
> +	return 0;
> +
> +err_init:
> +	hibmc_dp_hw_uninit(&priv->dp);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 9f9b19ea0587..f98ac94a18b9 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -27,6 +27,10 @@
>  #include "hibmc_drm_drv.h"
>  #include "hibmc_drm_regs.h"
>  
> +#define DP_HOST_SERDES_CTRL		0x1f001c
> +#define DP_HOST_SERDES_CTRL_VAL		0x8A00
> +#define DP_HOST_SERDES_CTRL_MASK	0x7FFFE
> +
>  DEFINE_DRM_GEM_FOPS(hibmc_fops);
>  
>  static irqreturn_t hibmc_interrupt(int irq, void *arg)
> @@ -116,6 +120,14 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>  		return ret;
>  	}
>  
> +	/* if DP existed, init DP */
> +	if ((readl(priv->mmio + DP_HOST_SERDES_CTRL) &
> +	     DP_HOST_SERDES_CTRL_MASK) == DP_HOST_SERDES_CTRL_VAL) {
> +		ret = hibmc_dp_init(priv);
> +		if (ret)
> +			drm_err(dev, "failed to init dp: %d\n", ret);
> +	}
> +
>  	ret = hibmc_vdac_init(priv);
>  	if (ret) {
>  		drm_err(dev, "failed to init vdac: %d\n", ret);
> @@ -239,6 +251,7 @@ static int hibmc_hw_init(struct hibmc_drm_private *priv)
>  
>  static int hibmc_unload(struct drm_device *dev)
>  {
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  
>  	drm_atomic_helper_shutdown(dev);
> @@ -247,6 +260,9 @@ static int hibmc_unload(struct drm_device *dev)
>  
>  	pci_disable_msi(to_pci_dev(dev->dev));
>  
> +	if (priv->dp.encoder.possible_crtcs)
> +		hibmc_dp_uninit(priv);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index 6b566f3aeecb..1b78d313a6c2 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -19,10 +19,12 @@
>  #include <linux/i2c.h>
>  
>  #include <drm/drm_framebuffer.h>
> +#include "dp/dp_hw.h"
>  
> -struct hibmc_connector {
> -	struct drm_connector base;
> -
> +struct hibmc_vdac {
> +	struct drm_device *dev;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
>  	struct i2c_adapter adapter;
>  	struct i2c_algo_bit_data bit_data;
>  };
> @@ -35,13 +37,13 @@ struct hibmc_drm_private {
>  	struct drm_device dev;
>  	struct drm_plane primary_plane;
>  	struct drm_crtc crtc;
> -	struct drm_encoder encoder;
> -	struct hibmc_connector connector;
> +	struct hibmc_dp dp;
> +	struct hibmc_vdac vdac;

Please separate hibmc_vdac and hibmc_dp-related changes into separate patches.

>  };
>  
> -static inline struct hibmc_connector *to_hibmc_connector(struct drm_connector *connector)
> +static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
>  {
> -	return container_of(connector, struct hibmc_connector, base);
> +	return container_of(connector, struct hibmc_vdac, connector);
>  }
>  
>  static inline struct hibmc_drm_private *to_hibmc_drm_private(struct drm_device *dev)
> @@ -57,6 +59,9 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
>  int hibmc_de_init(struct hibmc_drm_private *priv);
>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
>  
> -int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_connector *connector);
> +int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *connector);
> +
> +int hibmc_dp_init(struct hibmc_drm_private *priv);
> +void hibmc_dp_uninit(struct hibmc_drm_private *priv);
>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> index e6e48651c15c..99b3b77b5445 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> @@ -25,8 +25,8 @@
>  
>  static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
>  {
> -	struct hibmc_connector *hibmc_connector = data;
> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector->base.dev);
> +	struct hibmc_vdac *vdac = data;
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
>  	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
>  
>  	if (value) {
> @@ -45,8 +45,8 @@ static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
>  
>  static int hibmc_get_i2c_signal(void *data, u32 mask)
>  {
> -	struct hibmc_connector *hibmc_connector = data;
> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector->base.dev);
> +	struct hibmc_vdac *vdac = data;
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
>  	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
>  
>  	if ((tmp_dir & mask) != mask) {
> @@ -77,22 +77,21 @@ static int hibmc_ddc_getscl(void *data)
>  	return hibmc_get_i2c_signal(data, I2C_SCL_MASK);
>  }
>  
> -int hibmc_ddc_create(struct drm_device *drm_dev,
> -		     struct hibmc_connector *connector)
> +int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *vdac)
>  {
> -	connector->adapter.owner = THIS_MODULE;
> -	snprintf(connector->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
> -	connector->adapter.dev.parent = drm_dev->dev;
> -	i2c_set_adapdata(&connector->adapter, connector);
> -	connector->adapter.algo_data = &connector->bit_data;
> -
> -	connector->bit_data.udelay = 20;
> -	connector->bit_data.timeout = usecs_to_jiffies(2000);
> -	connector->bit_data.data = connector;
> -	connector->bit_data.setsda = hibmc_ddc_setsda;
> -	connector->bit_data.setscl = hibmc_ddc_setscl;
> -	connector->bit_data.getsda = hibmc_ddc_getsda;
> -	connector->bit_data.getscl = hibmc_ddc_getscl;
> -
> -	return i2c_bit_add_bus(&connector->adapter);
> +	vdac->adapter.owner = THIS_MODULE;
> +	snprintf(vdac->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
> +	vdac->adapter.dev.parent = drm_dev->dev;
> +	i2c_set_adapdata(&vdac->adapter, vdac);
> +	vdac->adapter.algo_data = &vdac->bit_data;
> +
> +	vdac->bit_data.udelay = 20;
> +	vdac->bit_data.timeout = usecs_to_jiffies(2000);
> +	vdac->bit_data.data = vdac;
> +	vdac->bit_data.setsda = hibmc_ddc_setsda;
> +	vdac->bit_data.setscl = hibmc_ddc_setscl;
> +	vdac->bit_data.getsda = hibmc_ddc_getsda;
> +	vdac->bit_data.getscl = hibmc_ddc_getscl;
> +
> +	return i2c_bit_add_bus(&vdac->adapter);
>  }
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index 409c551c92af..05e19ea4c9f9 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -24,11 +24,11 @@
>  
>  static int hibmc_connector_get_modes(struct drm_connector *connector)
>  {
> -	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
> +	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
>  	const struct drm_edid *drm_edid;
>  	int count;
>  
> -	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
> +	drm_edid = drm_edid_read_ddc(connector, &vdac->adapter);
>  
>  	drm_edid_connector_update(connector, drm_edid);
>  
> @@ -51,9 +51,9 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
>  
>  static void hibmc_connector_destroy(struct drm_connector *connector)
>  {
> -	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
> +	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
>  
> -	i2c_del_adapter(&hibmc_connector->adapter);
> +	i2c_del_adapter(&vdac->adapter);
>  	drm_connector_cleanup(connector);
>  }
>  
> @@ -93,20 +93,20 @@ static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
>  int hibmc_vdac_init(struct hibmc_drm_private *priv)
>  {
>  	struct drm_device *dev = &priv->dev;
> -	struct hibmc_connector *hibmc_connector = &priv->connector;
> -	struct drm_encoder *encoder = &priv->encoder;
> +	struct hibmc_vdac *vdac = &priv->vdac;
> +	struct drm_encoder *encoder = &vdac->encoder;
>  	struct drm_crtc *crtc = &priv->crtc;
> -	struct drm_connector *connector = &hibmc_connector->base;
> +	struct drm_connector *connector = &vdac->connector;
>  	int ret;
>  
> -	ret = hibmc_ddc_create(dev, hibmc_connector);
> +	ret = hibmc_ddc_create(dev, vdac);
>  	if (ret) {
>  		drm_err(dev, "failed to create ddc: %d\n", ret);
>  		return ret;
>  	}
>  
>  	encoder->possible_crtcs = drm_crtc_mask(crtc);
> -	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> +	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
>  	if (ret) {
>  		drm_err(dev, "failed to init encoder: %d\n", ret);
>  		return ret;
> @@ -117,7 +117,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>  	ret = drm_connector_init_with_ddc(dev, connector,
>  					  &hibmc_connector_funcs,
>  					  DRM_MODE_CONNECTOR_VGA,
> -					  &hibmc_connector->adapter);
> +					  &vdac->adapter);
>  	if (ret) {
>  		drm_err(dev, "failed to init connector: %d\n", ret);
>  		return ret;
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 drm-dp 2/4] drm/hisilicon/hibmc: add dp link moduel in hibmc

[kernel test robot]

Hi Yongbang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.12-rc6 next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yongbang-Shi/drm-hisilicon-hibmc-add-dp-aux-in-hibmc/20241101-185648
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20241101105028.2177274-3-shiyongbang%40huawei.com
patch subject: [PATCH V3 drm-dp 2/4] drm/hisilicon/hibmc: add dp link moduel in hibmc
config: csky-randconfig-r121-20241103 (https://download.01.org/0day-ci/archive/20241104/202411041559.WIfxRN6n-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20241104/202411041559.WIfxRN6n-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411041559.WIfxRN6n-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c:12:10: sparse: sparse: symbol 'link_rate_map' was not declared. Should it be static?

vim +/link_rate_map +12 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c

    11	
  > 12	const u8 link_rate_map[] = {DP_LINK_BW_1_62, DP_LINK_BW_2_7,
    13				    DP_LINK_BW_5_4, DP_LINK_BW_8_1};
    14	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V3 drm-dp 3/4] drm/hisilicon/hibmc: add dp hw moduel in hibmc

[Yongbang Shi]

> On Fri, Nov 01, 2024 at 06:50:27PM +0800, Yongbang Shi wrote:
>> From: baihan li <libaihan@huawei.com>
>>
>> Build a dp level that hibmc driver can enable dp by
>> calling their functions.
>>
>> Signed-off-by: baihan li <libaihan@huawei.com>
>> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - fix build errors reported by kernel test robot <lkp@intel.com>
>>      Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@intel.com/
>> v1 -> v2:
>>    - changed some defines and functions to former patch, suggested by Dmitry Baryshkov.
>>    - sorting the headers including in dp_hw.h and hibmc_drm_drv.c files, suggested by Dmitry Baryshkov.
>>    - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
>>    - fix build errors reported by kernel test robot <lkp@intel.com>
>>      Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@intel.com/
>>    v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile    |   2 +-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c  | 237 ++++++++++++++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h  |  31 +++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h |  41 ++++
>>   4 files changed, 310 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 94d77da88bbf..214228052ccf 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,5 +1,5 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>> -	       dp/dp_aux.o dp/dp_link.o
>> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
>>   
>>   obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> new file mode 100644
>> index 000000000000..214897798bdb
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -0,0 +1,237 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright (c) 2024 Hisilicon Limited.
>> +
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include "dp_config.h"
>> +#include "dp_comm.h"
>> +#include "dp_reg.h"
>> +#include "dp_hw.h"
>> +#include "dp_link.h"
>> +#include "dp_aux.h"
>> +
>> +static int hibmc_dp_link_init(struct dp_dev *dp)
>> +{
>> +	dp->link.cap.lanes = 2;
>> +	dp->link.train_set = devm_kzalloc(dp->dev->dev,
>> +					  dp->link.cap.lanes * sizeof(u8), GFP_KERNEL);
> Can you replace it just with an array, removing a need for an additional
> allocation?
>
>> +	if (!dp->link.train_set)
>> +		return -ENOMEM;
>> +
>> +	dp->link.cap.link_rate = 1;
> Ok, this is why I don't link using indices for link rates. Which rate is
> this? Unlike cap.lanes this is pure magic number. I think it should be
> handled other way around: store actual link rate and convert to the
> register value when required.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static void hibmc_dp_set_tu(struct dp_dev *dp, struct drm_display_mode *mode)
>> +{
>> +	u32 tu_symbol_frac_size;
>> +	u32 tu_symbol_size;
>> +	u32 rate_ks;
>> +	u8 lane_num;
>> +	u32 value;
>> +	u32 bpp;
>> +
>> +	lane_num = dp->link.cap.lanes;
>> +	if (lane_num == 0) {
>> +		drm_err(dp->dev, "set tu failed, lane num cannot be 0!\n");
>> +		return;
>> +	}
>> +
>> +	bpp = DP_BPP;
> Where is this defined? Is it hibmc-specific or a generic value?
>
>> +	rate_ks = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
> same question

Hi Dmitry,
Thanks for your detailed suggestions and questions. These two are defined in dp_config.h.
Thanks,
Baihan.


>> +	value = (mode->clock * bpp * 5) / (61 * lane_num * rate_ks);
>> +
>> +	if (value % 10 == 9) { /* 9 carry */
>> +		tu_symbol_size = value / 10 + 1;
>> +		tu_symbol_frac_size = 0;
>> +	} else {
>> +		tu_symbol_size = value / 10;
>> +		tu_symbol_frac_size = value % 10 + 1;
>> +	}
>> +
>> +	drm_info(dp->dev, "tu value: %u.%u value: %u\n",
>> +		 tu_symbol_size, tu_symbol_frac_size, value);
>> +
>> +	dp_reg_write_field(dp->base + DP_VIDEO_PACKET,
>> +			   DP_CFG_STREAM_TU_SYMBOL_SIZE, tu_symbol_size);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_PACKET,
>> +			   DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE, tu_symbol_frac_size);
>> +}
>> +
>> +static void hibmc_dp_set_sst(struct dp_dev *dp, struct drm_display_mode *mode)
>> +{
>> +	u32 hblank_size;
>> +	u32 htotal_size;
>> +	u32 htotal_int;
>> +	u32 hblank_int;
>> +	u32 fclk; /* flink_clock */
>> +
>> +	fclk = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
>> +
>> +	/* ssc: 9947 / 10000 = 0.9947 */
> This is obvious. More interesting question might be what exactly is
> 0.9947 (or 0.53 %).
>
>> +	htotal_int = mode->htotal * 9947 / 10000;
>> +	htotal_size = (u32)(htotal_int * fclk / (DP_SYMBOL_PER_FCLK * (mode->clock / 1000)));
> Drop u32 ?
>
>> +
>> +	/* ssc: max effect bandwidth 53 / 10000 = 0.53% */
>> +	hblank_int = (mode->htotal - mode->hdisplay) - mode->hdisplay * 53 / 10000;
>> +	hblank_size = hblank_int * fclk * 9947 /
>> +		      (mode->clock * 10 * DP_SYMBOL_PER_FCLK);
>> +
>> +	drm_info(dp->dev, "h_active %u v_active %u htotal_size %u hblank_size %u",
>> +		 mode->hdisplay, mode->vdisplay, htotal_size, hblank_size);
>> +	drm_info(dp->dev, "flink_clock %u pixel_clock %d", fclk, (mode->clock / 1000));
>> +
>> +	dp_reg_write_field(dp->base + DP_VIDEO_HORIZONTAL_SIZE,
>> +			   DP_CFG_STREAM_HTOTAL_SIZE, htotal_size);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_HORIZONTAL_SIZE,
>> +			   DP_CFG_STREAM_HBLANK_SIZE, hblank_size);
>> +}
>> +
>> +static void hibmc_dp_link_cfg(struct dp_dev *dp, struct drm_display_mode *mode)
>> +{
>> +	u32 timing_delay;
>> +	u32 vblank;
>> +	u32 hstart;
>> +	u32 vstart;
>> +
>> +	vblank = mode->vtotal - mode->vdisplay;
>> +	timing_delay = mode->htotal - mode->hsync_start;
>> +	hstart = mode->htotal - mode->hsync_start;
>> +	vstart = mode->vtotal - mode->vsync_start;
>> +
>> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG0,
>> +			   DP_CFG_TIMING_GEN0_HBLANK, (mode->htotal - mode->hdisplay));
>> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG0,
>> +			   DP_CFG_TIMING_GEN0_HACTIVE, mode->hdisplay);
>> +
>> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG2,
>> +			   DP_CFG_TIMING_GEN0_VBLANK, vblank);
>> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG2,
>> +			   DP_CFG_TIMING_GEN0_VACTIVE, mode->vdisplay);
>> +	dp_reg_write_field(dp->base + DP_TIMING_GEN_CONFIG3,
>> +			   DP_CFG_TIMING_GEN0_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
>> +
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG0,
>> +			   DP_CFG_STREAM_HACTIVE, mode->hdisplay);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG0,
>> +			   DP_CFG_STREAM_HBLANK, (mode->htotal - mode->hdisplay));
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG2,
>> +			   DP_CFG_STREAM_HSYNC_WIDTH, (mode->hsync_end - mode->hsync_start));
>> +
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG1,
>> +			   DP_CFG_STREAM_VACTIVE, mode->vdisplay);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG1,
>> +			   DP_CFG_STREAM_VBLANK, vblank);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG3,
>> +			   DP_CFG_STREAM_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CONFIG3,
>> +			   DP_CFG_STREAM_VSYNC_WIDTH, (mode->vsync_end - mode->vsync_start));
>> +
>> +	dp_reg_write_field(dp->base + DP_VIDEO_MSA0,
>> +			   DP_CFG_STREAM_VSTART, vstart);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_MSA0,
>> +			   DP_CFG_STREAM_HSTART, hstart);
>> +
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_VSYNC_POLARITY,
>> +			   mode->flags & DRM_MODE_FLAG_PVSYNC ? 1 : 0);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_HSYNC_POLARITY,
>> +			   mode->flags & DRM_MODE_FLAG_PHSYNC ? 1 : 0);
>> +
>> +	/* MSA mic 0 and 1 */
>> +	writel(DP_MSA1, dp->base + DP_VIDEO_MSA1);
>> +	writel(DP_MSA2, dp->base + DP_VIDEO_MSA2);
>> +
>> +	hibmc_dp_set_tu(dp, mode);
>> +
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_RGB_ENABLE, 0x1);
>> +	dp_reg_write_field(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_VIDEO_MAPPING, 0);
>> +
>> +	/* divide 2: up even */
>> +	if (timing_delay % 2)
>> +		timing_delay++;
>> +
>> +	dp_reg_write_field(dp->base + DP_TIMING_MODEL_CTRL,
>> +			   DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1, timing_delay);
>> +
>> +	hibmc_dp_set_sst(dp, mode);
>> +}
>> +
>> +int hibmc_dp_hw_init(struct hibmc_dp *dp)
>> +{
>> +	struct drm_device *drm_dev = dp->drm_dev;
>> +	struct dp_dev *dp_dev;
>> +	int ret;
>> +
>> +	dp_dev = devm_kzalloc(drm_dev->dev, sizeof(struct dp_dev), GFP_KERNEL);
>> +	if (!dp_dev)
>> +		return -ENOMEM;
>> +
>> +	dp->dp_dev = dp_dev;
>> +
>> +	dp_dev->dev = drm_dev;
>> +	dp_dev->base = dp->mmio + DP_OFFSET;
>> +
>> +	hibmc_dp_aux_init(dp_dev);
>> +
>> +	ret = hibmc_dp_link_init(dp_dev);
>> +	if (ret) {
>> +		drm_err(drm_dev, "dp link init failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/* hdcp data */
>> +	writel(DP_HDCP, dp_dev->base + DP_HDCP_CFG);
>> +	/* int init */
>> +	writel(0, dp_dev->base + DP_INTR_ENABLE);
>> +	writel(DP_INT_RST, dp_dev->base + DP_INTR_ORIGINAL_STATUS);
>> +	/* rst */
>> +	writel(DP_DPTX_RST, dp_dev->base + DP_DPTX_RST_CTRL);
>> +	/* clock enable */
>> +	writel(DP_CLK_EN, dp_dev->base + DP_DPTX_CLK_CTRL);
>> +
>> +	return 0;
>> +}
>> +
>> +void hibmc_dp_hw_uninit(struct hibmc_dp *dp)
>> +{
>> +	// keep this uninit interface in the future use
> no reason to, introduce it when required, not the other way around
>
>> +}
>> +
>> +void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
>> +{
>> +	struct dp_dev *dp_dev = dp->dp_dev;
>> +
>> +	if (enable) {
>> +		dp_reg_write_field(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0x1);
>> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
>> +		dp_reg_write_field(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0x1);
>> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
>> +	} else {
>> +		dp_reg_write_field(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0);
>> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
>> +		dp_reg_write_field(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0);
>> +		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
>> +	}
>> +
>> +	msleep(50);
>> +}
>> +
>> +int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>> +{
>> +	struct dp_dev *dp_dev = dp->dp_dev;
>> +	int ret;
>> +
>> +	if (!dp_dev->link.status.channel_equalized) {
>> +		ret = hibmc_dp_link_training(dp_dev);
>> +		if (ret) {
>> +			drm_err(dp->drm_dev, "dp link training failed, ret: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	hibmc_dp_display_en(dp, false);
>> +	hibmc_dp_link_cfg(dp_dev, mode);
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> new file mode 100644
>> index 000000000000..de802aaa8b4a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2024 Hisilicon Limited. */
>> +
>> +#ifndef DP_KAPI_H
>> +#define DP_KAPI_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/delay.h>
>> +
>> +#include <drm/drm_device.h>
>> +#include <drm/drm_encoder.h>
>> +#include <drm/drm_connector.h>
>> +#include <drm/drm_print.h>
>> +#include <video/videomode.h>
>> +
>> +struct dp_dev;
> hibmc_dp_dev
>
>> +
>> +struct hibmc_dp {
>> +	struct dp_dev *dp_dev;
>> +	struct drm_device *drm_dev;
>> +	struct drm_encoder encoder;
>> +	struct drm_connector connector;
>> +	void __iomem *mmio;
>> +};
>> +
>> +int hibmc_dp_hw_init(struct hibmc_dp *dp);
>> +void hibmc_dp_hw_uninit(struct hibmc_dp *dp);
>> +int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>> +void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> index 1032f6cde761..3dcb847057a4 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -14,8 +14,26 @@
>>   #define DP_AUX_STATUS			0x78
>>   #define DP_PHYIF_CTRL0			0xa0
>>   #define DP_VIDEO_CTRL			0x100
>> +#define DP_VIDEO_CONFIG0		0x104
>> +#define DP_VIDEO_CONFIG1		0x108
>> +#define DP_VIDEO_CONFIG2		0x10c
>> +#define DP_VIDEO_CONFIG3		0x110
>> +#define DP_VIDEO_PACKET			0x114
>> +#define DP_VIDEO_MSA0			0x118
>> +#define DP_VIDEO_MSA1			0x11c
>> +#define DP_VIDEO_MSA2			0x120
>> +#define DP_VIDEO_HORIZONTAL_SIZE	0X124
>> +#define DP_TIMING_GEN_CONFIG0		0x26c
>> +#define DP_TIMING_GEN_CONFIG2		0x274
>> +#define DP_TIMING_GEN_CONFIG3		0x278
>> +#define DP_HDCP_CFG			0x600
>> +#define DP_INTR_ENABLE			0x720
>> +#define DP_INTR_ORIGINAL_STATUS		0x728
>>   #define DP_DPTX_RST_CTRL		0x700
>> +#define DP_DPTX_CLK_CTRL		0x704
>>   #define DP_DPTX_GCTL0			0x708
>> +#define DP_TIMING_MODEL_CTRL		0x884
>> +#define DP_TIMING_SYNC_CTRL		0xFF0
>>   
>>   #define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
>>   #define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
>> @@ -31,5 +49,28 @@
>>   #define DP_CFG_AUX_STATUS			GENMASK(11, 4)
>>   #define DP_CFG_SCRAMBLE_EN			BIT(0)
>>   #define DP_CFG_PAT_SEL				GENMASK(7, 4)
>> +#define DP_CFG_TIMING_GEN0_HACTIVE		GENMASK(31, 16)
>> +#define DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
>> +#define DP_CFG_TIMING_GEN0_VACTIVE		GENMASK(31, 16)
>> +#define DP_CFG_TIMING_GEN0_VBLANK		GENMASK(15, 0)
>> +#define DP_CFG_TIMING_GEN0_VFRONT_PORCH		GENMASK(31, 16)
>> +#define DP_CFG_STREAM_HACTIVE			GENMASK(31, 16)
>> +#define DP_CFG_STREAM_HBLANK			GENMASK(15, 0)
>> +#define DP_CFG_STREAM_HSYNC_WIDTH		GENMASK(15, 0)
>> +#define DP_CFG_STREAM_VACTIVE			GENMASK(31, 16)
>> +#define DP_CFG_STREAM_VBLANK			GENMASK(15, 0)
>> +#define DP_CFG_STREAM_VFRONT_PORCH		GENMASK(31, 16)
>> +#define DP_CFG_STREAM_VSYNC_WIDTH		GENMASK(15, 0)
>> +#define DP_CFG_STREAM_VSTART			GENMASK(31, 16)
>> +#define DP_CFG_STREAM_HSTART			GENMASK(15, 0)
>> +#define DP_CFG_STREAM_VSYNC_POLARITY		BIT(8)
>> +#define DP_CFG_STREAM_HSYNC_POLARITY		BIT(7)
>> +#define DP_CFG_STREAM_RGB_ENABLE		BIT(1)
>> +#define DP_CFG_STREAM_VIDEO_MAPPING		GENMASK(5, 2)
>> +#define DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1	GENMASK(31, 16)
>> +#define DP_CFG_STREAM_TU_SYMBOL_SIZE		GENMASK(5, 0)
>> +#define DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE	GENMASK(9, 6)
>> +#define DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
>> +#define DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
>>   
>>   #endif
>> -- 
>> 2.33.0
>>

Re: [PATCH V3 drm-dp 4/4] drm/hisilicon/hibmc: add dp module in hibmc

[Yongbang Shi]

I will accept your advices in all the patchs.
Thanks,
Baihan.

> On Fri, Nov 01, 2024 at 06:50:28PM +0800, Yongbang Shi wrote:
>> From: baihan li <libaihan@huawei.com>
>>
>> To support DP interface displaying in hibmc driver. Add
>> a encoder and connector for DP modual.
>>
>> Signed-off-by: baihan li <libaihan@huawei.com>
>> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - fix build errors reported by kernel test robot <lkp@intel.com>
>>      Closes: https://lore.kernel.org/oe-kbuild-all/202410251136.1m7BlR68-lkp@intel.com/
>> v1 -> v2:
>>    - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
>>    - modifying drm_simple_encoder_init function, suggested by Dmitry Baryshkov.
>>    - refactoring struct hibmc_connector, suggested by Dmitry Baryshkov.
>>    - withdrawing the modification in hibmc_kms_init, suggested by Dmitry Baryshkov.
>>    v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile      |   2 +-
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 128 ++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  16 +++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  21 +--
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c   |  41 +++---
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  |  20 +--
>>   6 files changed, 188 insertions(+), 40 deletions(-)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 214228052ccf..95a4ed599d98 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,5 +1,5 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>> -	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
>> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o hibmc_drm_dp.o
>>   
>>   obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> new file mode 100644
>> index 000000000000..1e0f2ef39ba6
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -0,0 +1,128 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright (c) 2024 Hisilicon Limited.
>> +
>> +#include <linux/io.h>
>> +
>> +#include <drm/drm_probe_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_modes.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_edid.h>
>> +
>> +#include "hibmc_drm_drv.h"
>> +#include "dp/dp_hw.h"
>> +
>> +static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>> +{
>> +	int count;
>> +
>> +	count = drm_add_modes_noedid(connector, connector->dev->mode_config.max_width,
>> +				     connector->dev->mode_config.max_height);
>> +	drm_set_preferred_mode(connector, 1024, 768); // temporary implementation
>> +
>> +	return count;
>> +}
>> +
>> +static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>> +	.get_modes = hibmc_dp_connector_get_modes,
>> +};
>> +
>> +static const struct drm_connector_funcs hibmc_dp_conn_funcs = {
>> +	.reset = drm_atomic_helper_connector_reset,
>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.destroy = drm_connector_cleanup,
>> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +static int hibmc_dp_prepare(struct hibmc_dp *dp, struct drm_display_mode *mode)
> inline at the calling site
>
>> +{
>> +	int ret;
>> +
>> +	hibmc_dp_display_en(dp, false);
>> +
>> +	ret = hibmc_dp_mode_set(dp, mode);
>> +	if (ret)
>> +		drm_err(dp->drm_dev, "hibmc dp mode set failed: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static void hibmc_dp_encoder_enable(struct drm_encoder *drm_encoder,
>> +				    struct drm_atomic_state *state)
>> +{
>> +	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
>> +	struct drm_display_mode *mode = &drm_encoder->crtc->state->mode;
>> +
>> +	if (hibmc_dp_prepare(dp, mode))
>> +		return;
>> +
>> +	hibmc_dp_display_en(dp, true);
>> +}
>> +
>> +static void hibmc_dp_encoder_disable(struct drm_encoder *drm_encoder,
>> +				     struct drm_atomic_state *state)
>> +{
>> +	struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp, encoder);
>> +
>> +	hibmc_dp_display_en(dp, false);
>> +}
>> +
>> +static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>> +	.atomic_enable = hibmc_dp_encoder_enable,
>> +	.atomic_disable = hibmc_dp_encoder_disable,
>> +};
>> +
>> +void hibmc_dp_uninit(struct hibmc_drm_private *priv)
>> +{
>> +	hibmc_dp_hw_uninit(&priv->dp);
>> +}
>> +
>> +int hibmc_dp_init(struct hibmc_drm_private *priv)
>> +{
>> +	struct drm_device *dev = &priv->dev;
>> +	struct drm_crtc *crtc = &priv->crtc;
>> +	struct hibmc_dp *dp = &priv->dp;
>> +	struct drm_connector *connector = &dp->connector;
>> +	struct drm_encoder *encoder = &dp->encoder;
>> +	int ret;
>> +
>> +	dp->mmio = priv->mmio;
>> +	dp->drm_dev = dev;
>> +
>> +	ret = hibmc_dp_hw_init(&priv->dp);
>> +	if (ret) {
>> +		drm_err(dev, "hibmc dp hw init failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	hibmc_dp_display_en(&priv->dp, false);
>> +
>> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
>> +	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
>> +	if (ret) {
>> +		drm_err(dev, "init dp encoder failed: %d\n", ret);
>> +		goto err_init;
>> +	}
>> +
>> +	drm_encoder_helper_add(encoder, &hibmc_dp_encoder_helper_funcs);
>> +
>> +	ret = drm_connector_init(dev, connector, &hibmc_dp_conn_funcs,
>> +				 DRM_MODE_CONNECTOR_DisplayPort);
>> +	if (ret) {
>> +		drm_err(dev, "init dp connector failed: %d\n", ret);
>> +		goto err_init;
>> +	}
>> +
>> +	drm_connector_helper_add(connector, &hibmc_dp_conn_helper_funcs);
>> +
>> +	drm_connector_attach_encoder(connector, encoder);
>> +
>> +	return 0;
>> +
>> +err_init:
>> +	hibmc_dp_hw_uninit(&priv->dp);
>> +
>> +	return ret;
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 9f9b19ea0587..f98ac94a18b9 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -27,6 +27,10 @@
>>   #include "hibmc_drm_drv.h"
>>   #include "hibmc_drm_regs.h"
>>   
>> +#define DP_HOST_SERDES_CTRL		0x1f001c
>> +#define DP_HOST_SERDES_CTRL_VAL		0x8A00
>> +#define DP_HOST_SERDES_CTRL_MASK	0x7FFFE
>> +
>>   DEFINE_DRM_GEM_FOPS(hibmc_fops);
>>   
>>   static irqreturn_t hibmc_interrupt(int irq, void *arg)
>> @@ -116,6 +120,14 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>>   		return ret;
>>   	}
>>   
>> +	/* if DP existed, init DP */
>> +	if ((readl(priv->mmio + DP_HOST_SERDES_CTRL) &
>> +	     DP_HOST_SERDES_CTRL_MASK) == DP_HOST_SERDES_CTRL_VAL) {
>> +		ret = hibmc_dp_init(priv);
>> +		if (ret)
>> +			drm_err(dev, "failed to init dp: %d\n", ret);
>> +	}
>> +
>>   	ret = hibmc_vdac_init(priv);
>>   	if (ret) {
>>   		drm_err(dev, "failed to init vdac: %d\n", ret);
>> @@ -239,6 +251,7 @@ static int hibmc_hw_init(struct hibmc_drm_private *priv)
>>   
>>   static int hibmc_unload(struct drm_device *dev)
>>   {
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>>   
>>   	drm_atomic_helper_shutdown(dev);
>> @@ -247,6 +260,9 @@ static int hibmc_unload(struct drm_device *dev)
>>   
>>   	pci_disable_msi(to_pci_dev(dev->dev));
>>   
>> +	if (priv->dp.encoder.possible_crtcs)
>> +		hibmc_dp_uninit(priv);
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index 6b566f3aeecb..1b78d313a6c2 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -19,10 +19,12 @@
>>   #include <linux/i2c.h>
>>   
>>   #include <drm/drm_framebuffer.h>
>> +#include "dp/dp_hw.h"
>>   
>> -struct hibmc_connector {
>> -	struct drm_connector base;
>> -
>> +struct hibmc_vdac {
>> +	struct drm_device *dev;
>> +	struct drm_encoder encoder;
>> +	struct drm_connector connector;
>>   	struct i2c_adapter adapter;
>>   	struct i2c_algo_bit_data bit_data;
>>   };
>> @@ -35,13 +37,13 @@ struct hibmc_drm_private {
>>   	struct drm_device dev;
>>   	struct drm_plane primary_plane;
>>   	struct drm_crtc crtc;
>> -	struct drm_encoder encoder;
>> -	struct hibmc_connector connector;
>> +	struct hibmc_dp dp;
>> +	struct hibmc_vdac vdac;
> Please separate hibmc_vdac and hibmc_dp-related changes into separate patches.
>
>>   };
>>   
>> -static inline struct hibmc_connector *to_hibmc_connector(struct drm_connector *connector)
>> +static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
>>   {
>> -	return container_of(connector, struct hibmc_connector, base);
>> +	return container_of(connector, struct hibmc_vdac, connector);
>>   }
>>   
>>   static inline struct hibmc_drm_private *to_hibmc_drm_private(struct drm_device *dev)
>> @@ -57,6 +59,9 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv,
>>   int hibmc_de_init(struct hibmc_drm_private *priv);
>>   int hibmc_vdac_init(struct hibmc_drm_private *priv);
>>   
>> -int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_connector *connector);
>> +int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *connector);
>> +
>> +int hibmc_dp_init(struct hibmc_drm_private *priv);
>> +void hibmc_dp_uninit(struct hibmc_drm_private *priv);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
>> index e6e48651c15c..99b3b77b5445 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
>> @@ -25,8 +25,8 @@
>>   
>>   static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
>>   {
>> -	struct hibmc_connector *hibmc_connector = data;
>> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector->base.dev);
>> +	struct hibmc_vdac *vdac = data;
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
>>   	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
>>   
>>   	if (value) {
>> @@ -45,8 +45,8 @@ static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
>>   
>>   static int hibmc_get_i2c_signal(void *data, u32 mask)
>>   {
>> -	struct hibmc_connector *hibmc_connector = data;
>> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector->base.dev);
>> +	struct hibmc_vdac *vdac = data;
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(vdac->connector.dev);
>>   	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
>>   
>>   	if ((tmp_dir & mask) != mask) {
>> @@ -77,22 +77,21 @@ static int hibmc_ddc_getscl(void *data)
>>   	return hibmc_get_i2c_signal(data, I2C_SCL_MASK);
>>   }
>>   
>> -int hibmc_ddc_create(struct drm_device *drm_dev,
>> -		     struct hibmc_connector *connector)
>> +int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *vdac)
>>   {
>> -	connector->adapter.owner = THIS_MODULE;
>> -	snprintf(connector->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
>> -	connector->adapter.dev.parent = drm_dev->dev;
>> -	i2c_set_adapdata(&connector->adapter, connector);
>> -	connector->adapter.algo_data = &connector->bit_data;
>> -
>> -	connector->bit_data.udelay = 20;
>> -	connector->bit_data.timeout = usecs_to_jiffies(2000);
>> -	connector->bit_data.data = connector;
>> -	connector->bit_data.setsda = hibmc_ddc_setsda;
>> -	connector->bit_data.setscl = hibmc_ddc_setscl;
>> -	connector->bit_data.getsda = hibmc_ddc_getsda;
>> -	connector->bit_data.getscl = hibmc_ddc_getscl;
>> -
>> -	return i2c_bit_add_bus(&connector->adapter);
>> +	vdac->adapter.owner = THIS_MODULE;
>> +	snprintf(vdac->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
>> +	vdac->adapter.dev.parent = drm_dev->dev;
>> +	i2c_set_adapdata(&vdac->adapter, vdac);
>> +	vdac->adapter.algo_data = &vdac->bit_data;
>> +
>> +	vdac->bit_data.udelay = 20;
>> +	vdac->bit_data.timeout = usecs_to_jiffies(2000);
>> +	vdac->bit_data.data = vdac;
>> +	vdac->bit_data.setsda = hibmc_ddc_setsda;
>> +	vdac->bit_data.setscl = hibmc_ddc_setscl;
>> +	vdac->bit_data.getsda = hibmc_ddc_getsda;
>> +	vdac->bit_data.getscl = hibmc_ddc_getscl;
>> +
>> +	return i2c_bit_add_bus(&vdac->adapter);
>>   }
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index 409c551c92af..05e19ea4c9f9 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -24,11 +24,11 @@
>>   
>>   static int hibmc_connector_get_modes(struct drm_connector *connector)
>>   {
>> -	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
>> +	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
>>   	const struct drm_edid *drm_edid;
>>   	int count;
>>   
>> -	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
>> +	drm_edid = drm_edid_read_ddc(connector, &vdac->adapter);
>>   
>>   	drm_edid_connector_update(connector, drm_edid);
>>   
>> @@ -51,9 +51,9 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
>>   
>>   static void hibmc_connector_destroy(struct drm_connector *connector)
>>   {
>> -	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
>> +	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
>>   
>> -	i2c_del_adapter(&hibmc_connector->adapter);
>> +	i2c_del_adapter(&vdac->adapter);
>>   	drm_connector_cleanup(connector);
>>   }
>>   
>> @@ -93,20 +93,20 @@ static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
>>   int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>   {
>>   	struct drm_device *dev = &priv->dev;
>> -	struct hibmc_connector *hibmc_connector = &priv->connector;
>> -	struct drm_encoder *encoder = &priv->encoder;
>> +	struct hibmc_vdac *vdac = &priv->vdac;
>> +	struct drm_encoder *encoder = &vdac->encoder;
>>   	struct drm_crtc *crtc = &priv->crtc;
>> -	struct drm_connector *connector = &hibmc_connector->base;
>> +	struct drm_connector *connector = &vdac->connector;
>>   	int ret;
>>   
>> -	ret = hibmc_ddc_create(dev, hibmc_connector);
>> +	ret = hibmc_ddc_create(dev, vdac);
>>   	if (ret) {
>>   		drm_err(dev, "failed to create ddc: %d\n", ret);
>>   		return ret;
>>   	}
>>   
>>   	encoder->possible_crtcs = drm_crtc_mask(crtc);
>> -	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
>> +	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
>>   	if (ret) {
>>   		drm_err(dev, "failed to init encoder: %d\n", ret);
>>   		return ret;
>> @@ -117,7 +117,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>   	ret = drm_connector_init_with_ddc(dev, connector,
>>   					  &hibmc_connector_funcs,
>>   					  DRM_MODE_CONNECTOR_VGA,
>> -					  &hibmc_connector->adapter);
>> +					  &vdac->adapter);
>>   	if (ret) {
>>   		drm_err(dev, "failed to init connector: %d\n", ret);
>>   		return ret;
>> -- 
>> 2.33.0
>>

Re: [PATCH V3 drm-dp 3/4] drm/hisilicon/hibmc: add dp hw moduel in hibmc

[Dmitry Baryshkov]

On Tue, 5 Nov 2024 at 06:06, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
> > On Fri, Nov 01, 2024 at 06:50:27PM +0800, Yongbang Shi wrote:
> >> From: baihan li <libaihan@huawei.com>
> >>
> >> Build a dp level that hibmc driver can enable dp by
> >> calling their functions.
> >>
> >> Signed-off-by: baihan li <libaihan@huawei.com>
> >> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
> >> ---
> >> ChangeLog:
> >> v2 -> v3:
> >>    - fix build errors reported by kernel test robot <lkp@intel.com>
> >>      Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@intel.com/
> >> v1 -> v2:
> >>    - changed some defines and functions to former patch, suggested by Dmitry Baryshkov.
> >>    - sorting the headers including in dp_hw.h and hibmc_drm_drv.c files, suggested by Dmitry Baryshkov.
> >>    - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
> >>    - fix build errors reported by kernel test robot <lkp@intel.com>
> >>      Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@intel.com/
> >>    v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
> >> ---
> >>   drivers/gpu/drm/hisilicon/hibmc/Makefile    |   2 +-
> >>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c  | 237 ++++++++++++++++++++
> >>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h  |  31 +++
> >>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h |  41 ++++
> >>   4 files changed, 310 insertions(+), 1 deletion(-)
> >>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> >>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> >>
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> >> index 94d77da88bbf..214228052ccf 100644
> >> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> >> @@ -1,5 +1,5 @@
> >>   # SPDX-License-Identifier: GPL-2.0-only
> >>   hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> >> -           dp/dp_aux.o dp/dp_link.o
> >> +           dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
> >>
> >>   obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> >> new file mode 100644
> >> index 000000000000..214897798bdb
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> >> @@ -0,0 +1,237 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +// Copyright (c) 2024 Hisilicon Limited.
> >> +
> >> +#include <linux/io.h>
> >> +#include <linux/delay.h>
> >> +#include "dp_config.h"
> >> +#include "dp_comm.h"
> >> +#include "dp_reg.h"
> >> +#include "dp_hw.h"
> >> +#include "dp_link.h"
> >> +#include "dp_aux.h"
> >> +
> >> +static int hibmc_dp_link_init(struct dp_dev *dp)
> >> +{
> >> +    dp->link.cap.lanes = 2;
> >> +    dp->link.train_set = devm_kzalloc(dp->dev->dev,
> >> +                                      dp->link.cap.lanes * sizeof(u8), GFP_KERNEL);
> > Can you replace it just with an array, removing a need for an additional
> > allocation?
> >
> >> +    if (!dp->link.train_set)
> >> +            return -ENOMEM;
> >> +
> >> +    dp->link.cap.link_rate = 1;
> > Ok, this is why I don't link using indices for link rates. Which rate is
> > this? Unlike cap.lanes this is pure magic number. I think it should be
> > handled other way around: store actual link rate and convert to the
> > register value when required.
> >
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static void hibmc_dp_set_tu(struct dp_dev *dp, struct drm_display_mode *mode)
> >> +{
> >> +    u32 tu_symbol_frac_size;
> >> +    u32 tu_symbol_size;
> >> +    u32 rate_ks;
> >> +    u8 lane_num;
> >> +    u32 value;
> >> +    u32 bpp;
> >> +
> >> +    lane_num = dp->link.cap.lanes;
> >> +    if (lane_num == 0) {
> >> +            drm_err(dp->dev, "set tu failed, lane num cannot be 0!\n");
> >> +            return;
> >> +    }
> >> +
> >> +    bpp = DP_BPP;
> > Where is this defined? Is it hibmc-specific or a generic value?
> >
> >> +    rate_ks = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
> > same question
>
> Hi Dmitry,
> Thanks for your detailed suggestions and questions. These two are defined in dp_config.h.

Please move defines to the corresponding patch, when the values are
being used. Also if these defines are HIBMC-specific, please use the
corresponding prefix (when one sees DP_foo they expect a constant
defined in the standard, not a driver-specific value).



-- 
With best wishes
Dmitry

Re: [PATCH V3 drm-dp 3/4] drm/hisilicon/hibmc: add dp hw moduel in hibmc

[Yongbang Shi]

> On Tue, 5 Nov 2024 at 06:06, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>> On Fri, Nov 01, 2024 at 06:50:27PM +0800, Yongbang Shi wrote:
>>>> From: baihan li <libaihan@huawei.com>
>>>>
>>>> Build a dp level that hibmc driver can enable dp by
>>>> calling their functions.
>>>>
>>>> Signed-off-by: baihan li <libaihan@huawei.com>
>>>> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
>>>> ---
>>>> ChangeLog:
>>>> v2 -> v3:
>>>>     - fix build errors reported by kernel test robot <lkp@intel.com>
>>>>       Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@intel.com/
>>>> v1 -> v2:
>>>>     - changed some defines and functions to former patch, suggested by Dmitry Baryshkov.
>>>>     - sorting the headers including in dp_hw.h and hibmc_drm_drv.c files, suggested by Dmitry Baryshkov.
>>>>     - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
>>>>     - fix build errors reported by kernel test robot <lkp@intel.com>
>>>>       Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@intel.com/
>>>>     v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
>>>> ---
>>>>    drivers/gpu/drm/hisilicon/hibmc/Makefile    |   2 +-
>>>>    drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c  | 237 ++++++++++++++++++++
>>>>    drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h  |  31 +++
>>>>    drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h |  41 ++++
>>>>    4 files changed, 310 insertions(+), 1 deletion(-)
>>>>    create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>>>>    create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>>>>
>>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>>>> index 94d77da88bbf..214228052ccf 100644
>>>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>>>> @@ -1,5 +1,5 @@
>>>>    # SPDX-License-Identifier: GPL-2.0-only
>>>>    hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>>>> -           dp/dp_aux.o dp/dp_link.o
>>>> +           dp/dp_aux.o dp/dp_link.o dp/dp_hw.o
>>>>
>>>>    obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>>>> new file mode 100644
>>>> index 000000000000..214897798bdb
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>>>> @@ -0,0 +1,237 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +// Copyright (c) 2024 Hisilicon Limited.
>>>> +
>>>> +#include <linux/io.h>
>>>> +#include <linux/delay.h>
>>>> +#include "dp_config.h"
>>>> +#include "dp_comm.h"
>>>> +#include "dp_reg.h"
>>>> +#include "dp_hw.h"
>>>> +#include "dp_link.h"
>>>> +#include "dp_aux.h"
>>>> +
>>>> +static int hibmc_dp_link_init(struct dp_dev *dp)
>>>> +{
>>>> +    dp->link.cap.lanes = 2;
>>>> +    dp->link.train_set = devm_kzalloc(dp->dev->dev,
>>>> +                                      dp->link.cap.lanes * sizeof(u8), GFP_KERNEL);
>>> Can you replace it just with an array, removing a need for an additional
>>> allocation?
>>>
>>>> +    if (!dp->link.train_set)
>>>> +            return -ENOMEM;
>>>> +
>>>> +    dp->link.cap.link_rate = 1;
>>> Ok, this is why I don't link using indices for link rates. Which rate is
>>> this? Unlike cap.lanes this is pure magic number. I think it should be
>>> handled other way around: store actual link rate and convert to the
>>> register value when required.
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void hibmc_dp_set_tu(struct dp_dev *dp, struct drm_display_mode *mode)
>>>> +{
>>>> +    u32 tu_symbol_frac_size;
>>>> +    u32 tu_symbol_size;
>>>> +    u32 rate_ks;
>>>> +    u8 lane_num;
>>>> +    u32 value;
>>>> +    u32 bpp;
>>>> +
>>>> +    lane_num = dp->link.cap.lanes;
>>>> +    if (lane_num == 0) {
>>>> +            drm_err(dp->dev, "set tu failed, lane num cannot be 0!\n");
>>>> +            return;
>>>> +    }
>>>> +
>>>> +    bpp = DP_BPP;
>>> Where is this defined? Is it hibmc-specific or a generic value?
>>>
>>>> +    rate_ks = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
>>> same question
>> Hi Dmitry,
>> Thanks for your detailed suggestions and questions. These two are defined in dp_config.h.
> Please move defines to the corresponding patch, when the values are
> being used. Also if these defines are HIBMC-specific, please use the
> corresponding prefix (when one sees DP_foo they expect a constant
> defined in the standard, not a driver-specific value).

Hi Dmitry,
I got it, I will fix it in next version.
Thanks,
Baihan.

Re: [PATCH V3 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc

[Yongbang Shi]

> On Fri, Nov 01, 2024 at 06:50:25PM +0800, Yongbang Shi wrote:
>> From: baihan li <libaihan@huawei.com>
>>
>> Add dp aux read/write functions. They are basic functions
>> and will be used later.
>>
>> Signed-off-by: baihan li <libaihan@huawei.com>
>> Signed-off-by: yongbang shi <shiyongbang@huawei.com>
> Is this a proper capitalization of your names? Usually the first letter
> of each of them is capital.
>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - put the macro definations in latter patch where they are actually used, suggested by Dmitry Baryshkov.
>>    - rename some macro definations to make them sensible, suggested by Dmitry Baryshkov.
>>    - using FIELD_PREP and FIELD_GET, suggested by Dmitry Baryshkov.
>>    - using DP_DPCD_REV_foo, suggested by Dmitry Baryshkov.
>>    - fix build errors reported by kernel test robot <lkp@intel.com>
>>      Closes: https://lore.kernel.org/oe-kbuild-all/202410250305.UHKDhtxy-lkp@intel.com/
>> v1 -> v2:
>>    - using drm_dp_aux frame implement dp aux read and write functions, suggested by Jani Nikula.
>>    - using drm dp header files' dp macros instead, suggested by Andy Yan.
>>    v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c   | 162 ++++++++++++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h   |  23 +++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  58 +++++++
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  19 ++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  27 +++
>>   6 files changed, 291 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index d25c75e60d3d..8770ec6dfffd 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,4 +1,5 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o
>> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>> +	       dp/dp_aux.o
>>   
>>   obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
>> new file mode 100644
>> index 000000000000..49ecda672109
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
>> @@ -0,0 +1,162 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright (c) 2024 Hisilicon Limited.
>> +
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/minmax.h>
>> +#include <drm/drm_device.h>
>> +#include <drm/drm_print.h>
>> +#include "dp_comm.h"
>> +#include "dp_reg.h"
>> +#include "dp_aux.h"
>> +
>> +#define DP_MIN_PULSE_NUM 0x9
>> +
>> +static void hibmc_dp_aux_reset(const struct dp_dev *dp)
>> +{
>> +	dp_reg_write_field(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x0);
>> +	usleep_range(10, 15);
>> +	dp_reg_write_field(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x1);
>> +}
>> +
>> +static void hibmc_dp_aux_read_data(struct dp_dev *dp, u8 *buf, u8 size)
>> +{
>> +	u32 reg_num;
>> +	u32 value;
>> +	u32 num;
>> +	u8 i, j;
>> +
>> +	reg_num = DIV_ROUND_UP(size, BYTES_IN_U32);
>> +	for (i = 0; i < reg_num; i++) {
>> +		/* number of bytes read from a single register */
>> +		num = min(size - i * BYTES_IN_U32, BYTES_IN_U32);
>> +		value = readl(dp->base + DP_AUX_RD_DATA0 + i * BYTES_IN_U32);
>> +		/* convert the 32-bit value of the register to the buffer. */
>> +		for (j = 0; j < num; j++)
>> +			buf[i * BYTES_IN_U32 + j] = value >> (j * BITS_IN_U8);
> put_unaligned_le32()
>
>> +	}
>> +}
>> +
>> +static void hibmc_dp_aux_write_data(struct dp_dev *dp, u8 *buf, u8 size)
>> +{
>> +	u32 reg_num;
>> +	u32 value;
>> +	u8 i, j;
>> +	u32 num;
>> +
>> +	reg_num = DIV_ROUND_UP(size, BYTES_IN_U32);
>> +	for (i = 0; i < reg_num; i++) {
>> +		/* number of bytes written to a single register */
>> +		num = min_t(u8, size - i * BYTES_IN_U32, BYTES_IN_U32);
>> +		value = 0;
>> +		/* obtain the 32-bit value written to a single register. */
>> +		for (j = 0; j < num; j++)
>> +			value |= buf[i * BYTES_IN_U32 + j] << (j * BITS_IN_U8);
> get_unaligned_le32()

Hi Dimitry,
Thanks for your advice. I try to use these foo_unaligned_le32(), however, here's the situation.
The buffer I put in aux write or read functions are not always 4bytes or 4-byte alignment,
so when I need to process 1 byte or 2 bytes data, I need to use put_unaligned_le16 or
put_unaligned_le8  with if-statement, it's kind of complicated. So I think it's fine
to keep these implementation here.
Thanks,
Baihan


>> +		/* writing data to a single register */
>> +		writel(value, dp->base + DP_AUX_WR_DATA0 + i * BYTES_IN_U32);
>> +	}
>> +}
>> +
>> +static u32 hibmc_dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
>> +{
>> +	u32 aux_cmd = msg->request;
>> +
>> +	if (msg->size)
>> +		aux_cmd |= FIELD_PREP(AUX_CMD_REQ_LEN, (msg->size - 1));
>> +	else
>> +		aux_cmd |= FIELD_PREP(AUX_CMD_I2C_ADDR_ONLY, 1);
>> +
>> +	aux_cmd |= FIELD_PREP(AUX_CMD_ADDR, msg->address);
>> +
>> +	return aux_cmd;
>> +}
>> +
>> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
>> +static int hibmc_dp_aux_parse_xfer(struct dp_dev *dp, struct drm_dp_aux_msg *msg)
>> +{
>> +	u32 buf_data_cnt;
>> +	u32 aux_status;
>> +	int ret = 0;
>> +
>> +	aux_status = readl(dp->base + DP_AUX_STATUS);
>> +	msg->reply = FIELD_GET(DP_CFG_AUX_STATUS, aux_status);
>> +
>> +	if (aux_status & DP_CFG_AUX_TIMEOUT)
>> +		return -ETIMEDOUT;
>> +
>> +	/* only address */
>> +	if (!msg->size)
>> +		return 0;
>> +
>> +	if (msg->reply != DP_AUX_NATIVE_REPLY_ACK)
>> +		return 0;
> Should this be an error instead?
>
>> +
>> +	buf_data_cnt = FIELD_GET(DP_CFG_AUX_READY_DATA_BYTE, aux_status);
>> +
>> +	switch (msg->request) {
>> +	case DP_AUX_NATIVE_WRITE:
>> +		ret = msg->size;
>> +		break;
>> +	case DP_AUX_I2C_WRITE | DP_AUX_I2C_MOT:
>> +		if (buf_data_cnt == AUX_I2C_WRITE_SUCCESS)
>> +			ret = msg->size;
>> +		else if (buf_data_cnt == AUX_I2C_WRITE_PARTIAL_SUCCESS)
>> +			ret = FIELD_GET(DP_CFG_AUX, aux_status);
> Replace all ret= with returns. Nothing happens after the switch-case, so
> return right away.
>
>> +		break;
>> +	case DP_AUX_NATIVE_READ:
>> +	case DP_AUX_I2C_READ | DP_AUX_I2C_MOT:
>> +		buf_data_cnt--;
>> +		/* only the successful part of data is read */
> Nit: only a part of data wass read successfully.
> move the comment into the underlying `
>
>> +		if (buf_data_cnt != msg->size) {
>> +			ret = -EBUSY;
>> +		} else { /* all data is successfully read */
>> +			hibmc_dp_aux_read_data(dp, msg->buffer, msg->size);
>> +			ret = msg->size;
>> +		}
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
>> +static ssize_t hibmc_dp_aux_xfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> +{
>> +	struct dp_dev *dp = container_of(aux, struct dp_dev, aux);
>> +	u32 aux_cmd;
>> +	int ret;
>> +	u32 val; /* val will be assigned at the beginning of readl_poll_timeout function */
>> +
>> +	writel(0, dp->base + DP_AUX_WR_DATA0);
>> +	writel(0, dp->base + DP_AUX_WR_DATA1);
>> +	writel(0, dp->base + DP_AUX_WR_DATA2);
>> +	writel(0, dp->base + DP_AUX_WR_DATA3);
>> +
>> +	hibmc_dp_aux_write_data(dp, msg->buffer, msg->size);
>> +
>> +	aux_cmd = hibmc_dp_aux_build_cmd(msg);
>> +	writel(aux_cmd, dp->base + DP_AUX_CMD_ADDR);
>> +
>> +	/* enable aux transfer */
>> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_REQ, 0x1);
>> +	ret = readl_poll_timeout(dp->base + DP_AUX_REQ, val, !(val & DP_CFG_AUX_REQ), 50, 5000);
>> +	if (ret) {
>> +		hibmc_dp_aux_reset(dp);
>> +		return ret;
>> +	}
>> +
>> +	return hibmc_dp_aux_parse_xfer(dp, msg);
>> +}
>> +
>> +void hibmc_dp_aux_init(struct dp_dev *dp)
>> +{
>> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> +	dp_reg_write_field(dp->base + DP_AUX_REQ, DP_CFG_AUX_MIN_PULSE_NUM, DP_MIN_PULSE_NUM);
>> +
>> +	dp->aux.transfer = hibmc_dp_aux_xfer;
>> +	dp->aux.is_remote = 0;
>> +	drm_dp_aux_init(&dp->aux);
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
>> new file mode 100644
>> index 000000000000..794bb0482ea7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2024 Hisilicon Limited. */
>> +
>> +#ifndef DP_AUX_H
>> +#define DP_AUX_H
>> +
>> +#include <linux/bitops.h>
>> +#include "dp_comm.h"
>> +
>> +#define AUX_I2C_WRITE_SUCCESS		0x1
> Move to the source file
>
>> +#define AUX_I2C_WRITE_PARTIAL_SUCCESS	0x2
> Ditto
>
>> +#define EQ_MAX_RETRY			5
> Unused
>
>> +#define BYTES_IN_U32			4
>
>> +#define BITS_IN_U8			8
> This one will go away after using foo_unaligned_le32
>
>> +
>> +/* aux_cmd_addr register shift */
>> +#define AUX_CMD_REQ_LEN			GENMASK(7, 4)
>> +#define AUX_CMD_ADDR			GENMASK(27, 8)
>> +#define AUX_CMD_I2C_ADDR_ONLY		BIT(28)
> These can go to the source file too. Or to dp_reg.h. Don't spawn that
> all over the place.
>
>> +
>> +void hibmc_dp_aux_init(struct dp_dev *dp);
> As the header becomes nearly empty, merge the remnants to a generic
> header.
>
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> new file mode 100644
>> index 000000000000..61a59fd59962
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> @@ -0,0 +1,58 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2024 Hisilicon Limited. */
>> +
>> +#ifndef DP_COMM_H
>> +#define DP_COMM_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/bitops.h>
>> +#include <linux/errno.h>
>> +#include <linux/mutex.h>
>> +#include <linux/kernel.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/io.h>
>> +
>> +#include <drm/display/drm_dp_helper.h>
>> +
>> +#define dp_reg_read_field(addr, mask)				\
> These are still using dp_ prefix. Also using static inline functions
> might be easier to handle.
>
>> +		FIELD_GET(mask, readl(addr))
>> +
>> +#define dp_field_modify(reg_value, mask, value) ({		\
>> +		(reg_value) &= ~(mask);				\
>> +		(reg_value) |= FIELD_PREP(mask, value); })
>> +
>> +#define dp_reg_write_field(addr, mask, val) ({			\
>> +		typeof(addr) _addr = (addr);			\
>> +		u32 _value = readl(_addr);			\
>> +		dp_field_modify(_value, mask, val);		\
>> +		writel(_value, _addr); })
> I'm still very unsatisfied here. You have an RMW code with no locking.
> If anything writes to the same location at the same time, the driver is
> busted.
>
>> +
>> +struct link_status {
> It is hibmc-specific, so please prefix accordingly.
>
>> +	bool clock_recovered;
>> +	bool channel_equalized;
>> +	u8 cr_done_lanes;
>> +};
>> +
>> +struct link_cap {
>> +	int rx_dpcd_revision;
>> +	u8 link_rate;
>> +	u8 lanes;
>> +	bool is_tps3;
>> +	bool is_tps4;
>> +};
>> +
>> +struct hibmc_dp_link {
>> +	struct link_status status;
>> +	u8 *train_set;
>> +	struct link_cap cap;
>> +};
> Please don't define the structures/fields that are not used by the
> patch. Introduce them when required.
>
>> +
>> +struct dp_dev {
>> +	struct hibmc_dp_link link;
>> +	struct drm_dp_aux aux;
>> +	struct drm_device *dev;
>> +	void __iomem *base;
>> +	u8 dpcd[DP_RECEIVER_CAP_SIZE];
>> +};
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> new file mode 100644
>> index 000000000000..0b965e6ba7b3
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2024 Hisilicon Limited. */
>> +
>> +#ifndef DP_CONFIG_H
>> +#define DP_CONFIG_H
>> +
>> +#define DP_BPP			24
>> +#define DP_SYMBOL_PER_FCLK	4
>> +#define DP_MSA1			0x20
>> +#define DP_MSA2			0x845c00
>> +#define DP_OFFSET		0x1e0000
>> +#define DP_HDCP			0x2
>> +#define DP_INT_RST		0xffff
>> +#define DP_DPTX_RST		0x3ff
>> +#define DP_CLK_EN		0x7
>> +#define DP_SYNC_EN_MASK		0x3
>> +#define DP_LINK_RATE_CAL	27
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> new file mode 100644
>> index 000000000000..83cf0cc06ae2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2024 Hisilicon Limited. */
>> +
>> +#ifndef DP_REG_H
>> +#define DP_REG_H
>> +
>> +#define DP_AUX_CMD_ADDR			0x50
>> +#define DP_AUX_WR_DATA0			0x54
>> +#define DP_AUX_WR_DATA1			0x58
>> +#define DP_AUX_WR_DATA2			0x5c
>> +#define DP_AUX_WR_DATA3			0x60
>> +#define DP_AUX_RD_DATA0			0x64
>> +#define DP_AUX_REQ			0x74
>> +#define DP_AUX_STATUS			0x78
>> +#define DP_DPTX_RST_CTRL		0x700
>> +
>> +#define DP_CFG_AUX_SYNC_LEN_SEL			BIT(1)
>> +#define DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
>> +#define DP_CFG_AUX_MIN_PULSE_NUM		GENMASK(13, 9)
>> +#define DP_CFG_AUX_REQ				BIT(0)
>> +#define DP_CFG_AUX_RST_N			BIT(4)
>> +#define DP_CFG_AUX_TIMEOUT			BIT(0)
>> +#define DP_CFG_AUX_READY_DATA_BYTE		GENMASK(16, 12)
>> +#define DP_CFG_AUX				GENMASK(24, 17)
>> +#define DP_CFG_AUX_STATUS			GENMASK(11, 4)
>> +
>> +#endif
>> -- 
>> 2.33.0
>>