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

[PATCH V2 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:
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   |  31 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  74 ++++
 .../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  | 344 ++++++++++++++++++
 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    | 148 ++++++++
 .../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, 1208 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 V2 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>
---
ChangeLog:
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  |  31 ++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  74 +++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h  |  76 +++++++++
 5 files changed, 345 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_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..0078cafdf86d
--- /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 dp_aux_reset(const struct dp_dev *dp)
+{
+	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x0);
+	usleep_range(10, 15);
+	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x1);
+}
+
+static void dp_aux_read_data(struct dp_dev *dp, u8 *buf, u8 size)
+{
+	u32 reg_num;
+	u32 value;
+	u32 num;
+	u8 i, j;
+
+	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
+	for (i = 0; i < reg_num; i++) {
+		/* number of bytes read from a single register */
+		num = min(size - i * AUX_4_BYTE, AUX_4_BYTE);
+		value = readl(dp->base + DP_AUX_RD_DATA0 + i * AUX_4_BYTE);
+		/* convert the 32-bit value of the register to the buffer. */
+		for (j = 0; j < num; j++)
+			buf[i * AUX_4_BYTE + j] = value >> (j * AUX_8_BIT);
+	}
+}
+
+static void dp_aux_write_data(struct dp_dev *dp, u8 *buf, u8 size)
+{
+	u32 reg_num;
+	u32 value;
+	u8 i, j;
+	u32 num;
+
+	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
+	for (i = 0; i < reg_num; i++) {
+		/* number of bytes written to a single register */
+		num = min_t(u8, size - i * AUX_4_BYTE, AUX_4_BYTE);
+		value = 0;
+		/* obtain the 32-bit value written to a single register. */
+		for (j = 0; j < num; j++)
+			value |= buf[i * AUX_4_BYTE + j] << (j * AUX_8_BIT);
+		/* writing data to a single register */
+		writel(value, dp->base + DP_AUX_WR_DATA0 + i * AUX_4_BYTE);
+	}
+}
+
+static u32 dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
+{
+	u32 aux_cmd = msg->request;
+
+	if (msg->size)
+		aux_cmd |= (msg->size - 1) << AUX_CMD_REQ_LEN_S;
+	else
+		aux_cmd |= 1 << AUX_CMD_I2C_ADDR_ONLY_S;
+
+	aux_cmd |= msg->address << AUX_CMD_ADDR_S;
+
+	return aux_cmd;
+}
+
+/* ret >= 0 ,ret is size; ret < 0, ret is err code */
+static int 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 */
+			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 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);
+
+	dp_aux_write_data(dp, msg->buffer, msg->size);
+
+	aux_cmd = dp_aux_build_cmd(msg);
+	writel(aux_cmd, dp->base + DP_AUX_CMD_ADDR);
+
+	/* enable aux transfer */
+	dp_write_bits(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) {
+		dp_aux_reset(dp);
+		return ret;
+	}
+
+	return dp_aux_parse_xfer(dp, msg);
+}
+
+void dp_aux_init(struct dp_dev *dp)
+{
+	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
+	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
+	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_MIN_PULSE_NUM, DP_MIN_PULSE_NUM);
+
+	dp->aux.transfer = 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..6f95a3750d60
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
@@ -0,0 +1,31 @@
+/* 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 DP_CFG_AUX_S			17
+#define DP_CFG_AUX_STATUS_S		4
+
+#define AUX_4_BYTE			4
+#define AUX_4_BIT			4
+#define AUX_8_BIT			8
+
+#define AUX_READY_DATA_BYTE_S		12
+
+/* aux_cmd_addr register shift */
+#define AUX_CMD_REQ_LEN_S		4
+#define AUX_CMD_ADDR_S			8
+#define AUX_CMD_I2C_ADDR_ONLY_S		28
+
+void 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..26d97929dc06
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -0,0 +1,74 @@
+/* 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 REG_LENGTH 32
+
+static inline u32 dp_read_bits(void __iomem *addr, u32 bit_mask)
+{
+	u32 reg_val;
+
+	reg_val = readl(addr);
+
+	return (reg_val & bit_mask) >> __ffs(bit_mask);
+}
+
+static inline void dp_write_bits(void __iomem *addr, u32 bit_mask, u32 val)
+{
+	u32 reg_val;
+
+	reg_val = readl(addr);
+	reg_val &= ~bit_mask;
+	reg_val |= (val << __ffs(bit_mask)) & bit_mask;
+	writel(reg_val, addr);
+}
+
+enum dpcd_revision {
+	DPCD_REVISION_10 = 0x10,
+	DPCD_REVISION_11,
+	DPCD_REVISION_12,
+	DPCD_REVISION_13,
+	DPCD_REVISION_14,
+};
+
+struct link_status {
+	bool clock_recovered;
+	bool channel_equalized;
+	u8 cr_done_lanes;
+};
+
+struct link_cap {
+	enum dpcd_revision 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_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
new file mode 100644
index 000000000000..3dcb847057a4
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -0,0 +1,76 @@
+/* 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_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)
+#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)
+#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 V2 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc

[kernel test robot]

Hi Yongbang,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on linus/master v6.12-rc4 next-20241024]
[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/20241022-204925
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20241022124148.1952761-2-shiyongbang%40huawei.com
patch subject: [PATCH V2 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc
config: arm64-randconfig-004-20241024 (https://download.01.org/0day-ci/archive/20241025/202410250305.UHKDhtxy-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410250305.UHKDhtxy-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/202410250305.UHKDhtxy-lkp@intel.com/

All errors (new ones prefixed by >>):

   aarch64-linux-ld: drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.o: in function `dp_aux_init':
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c:154: multiple definition of `dp_aux_init'; drivers/gpu/drm/msm/dp/dp_aux.o:dp_aux.c:(.text+0x8a0): first defined here


vim +154 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c

   152	
   153	void dp_aux_init(struct dp_dev *dp)
 > 154	{

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

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

[Dmitry Baryshkov]

On Fri, Oct 25, 2024 at 03:52:49AM +0800, kernel test robot wrote:
> Hi Yongbang,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on drm-misc/drm-misc-next]
> [also build test ERROR on linus/master v6.12-rc4 next-20241024]
> [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/20241022-204925
> base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> patch link:    https://lore.kernel.org/r/20241022124148.1952761-2-shiyongbang%40huawei.com
> patch subject: [PATCH V2 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc
> config: arm64-randconfig-004-20241024 (https://download.01.org/0day-ci/archive/20241025/202410250305.UHKDhtxy-lkp@intel.com/config)
> compiler: aarch64-linux-gcc (GCC) 14.1.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410250305.UHKDhtxy-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/202410250305.UHKDhtxy-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    aarch64-linux-ld: drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.o: in function `dp_aux_init':
> >> drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c:154: multiple definition of `dp_aux_init'; drivers/gpu/drm/msm/dp/dp_aux.o:dp_aux.c:(.text+0x8a0): first defined here

I think both of us should switch to a more generic names. I'll send a
patchset for msm/dp, please use some less generic prefix for hibmc
driver.

> 
> 
> vim +154 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
> 
>    152	
>    153	void dp_aux_init(struct dp_dev *dp)
>  > 154	{
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Tue, Oct 22, 2024 at 08:41:45PM +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>
> ---
> ChangeLog:
> 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  |  31 ++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  74 +++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h  |  76 +++++++++
>  5 files changed, 345 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_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..0078cafdf86d
> --- /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 dp_aux_reset(const struct dp_dev *dp)
> +{
> +	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x0);
> +	usleep_range(10, 15);
> +	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x1);
> +}
> +
> +static void dp_aux_read_data(struct dp_dev *dp, u8 *buf, u8 size)
> +{
> +	u32 reg_num;
> +	u32 value;
> +	u32 num;
> +	u8 i, j;
> +
> +	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
> +	for (i = 0; i < reg_num; i++) {
> +		/* number of bytes read from a single register */
> +		num = min(size - i * AUX_4_BYTE, AUX_4_BYTE);
> +		value = readl(dp->base + DP_AUX_RD_DATA0 + i * AUX_4_BYTE);
> +		/* convert the 32-bit value of the register to the buffer. */
> +		for (j = 0; j < num; j++)
> +			buf[i * AUX_4_BYTE + j] = value >> (j * AUX_8_BIT);
> +	}
> +}
> +
> +static void dp_aux_write_data(struct dp_dev *dp, u8 *buf, u8 size)
> +{
> +	u32 reg_num;
> +	u32 value;
> +	u8 i, j;
> +	u32 num;
> +
> +	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
> +	for (i = 0; i < reg_num; i++) {
> +		/* number of bytes written to a single register */
> +		num = min_t(u8, size - i * AUX_4_BYTE, AUX_4_BYTE);
> +		value = 0;
> +		/* obtain the 32-bit value written to a single register. */
> +		for (j = 0; j < num; j++)
> +			value |= buf[i * AUX_4_BYTE + j] << (j * AUX_8_BIT);
> +		/* writing data to a single register */
> +		writel(value, dp->base + DP_AUX_WR_DATA0 + i * AUX_4_BYTE);
> +	}
> +}
> +
> +static u32 dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
> +{
> +	u32 aux_cmd = msg->request;
> +
> +	if (msg->size)
> +		aux_cmd |= (msg->size - 1) << AUX_CMD_REQ_LEN_S;
> +	else
> +		aux_cmd |= 1 << AUX_CMD_I2C_ADDR_ONLY_S;
> +
> +	aux_cmd |= msg->address << AUX_CMD_ADDR_S;
> +
> +	return aux_cmd;
> +}
> +
> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
> +static int 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 */
> +			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 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);
> +
> +	dp_aux_write_data(dp, msg->buffer, msg->size);
> +
> +	aux_cmd = dp_aux_build_cmd(msg);
> +	writel(aux_cmd, dp->base + DP_AUX_CMD_ADDR);
> +
> +	/* enable aux transfer */
> +	dp_write_bits(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) {
> +		dp_aux_reset(dp);
> +		return ret;
> +	}
> +
> +	return dp_aux_parse_xfer(dp, msg);
> +}
> +
> +void dp_aux_init(struct dp_dev *dp)
> +{
> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_MIN_PULSE_NUM, DP_MIN_PULSE_NUM);
> +
> +	dp->aux.transfer = 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..6f95a3750d60
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
> @@ -0,0 +1,31 @@
> +/* 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

unused

> +
> +#define DP_CFG_AUX_S			17
> +#define DP_CFG_AUX_STATUS_S		4

Both are unused

> +
> +#define AUX_4_BYTE			4
> +#define AUX_4_BIT			4
> +#define AUX_8_BIT			8

AUX_4_BIT is unused. For other defines I think it's better to provide
sensible names. BITS_IN_U32, BYTES_IN_U32

> +
> +#define AUX_READY_DATA_BYTE_S		12

unused

> +
> +/* aux_cmd_addr register shift */
> +#define AUX_CMD_REQ_LEN_S		4
> +#define AUX_CMD_ADDR_S			8
> +#define AUX_CMD_I2C_ADDR_ONLY_S		28

Please use FIELD_PREP instead of definig shifts (and shift is usally
_SHIFT, not _S)

> +
> +void 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..26d97929dc06
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> @@ -0,0 +1,74 @@
> +/* 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 REG_LENGTH 32
> +
> +static inline u32 dp_read_bits(void __iomem *addr, u32 bit_mask)
> +{
> +	u32 reg_val;
> +
> +	reg_val = readl(addr);
> +
> +	return (reg_val & bit_mask) >> __ffs(bit_mask);

FIELD_GET

> +}
> +
> +static inline void dp_write_bits(void __iomem *addr, u32 bit_mask, u32 val)
> +{
> +	u32 reg_val;
> +
> +	reg_val = readl(addr);
> +	reg_val &= ~bit_mask;
> +	reg_val |= (val << __ffs(bit_mask)) & bit_mask;

FIELD_PREP

> +	writel(reg_val, addr);

How is this protected from concurrent RMW cycles?

> +}
> +
> +enum dpcd_revision {
> +	DPCD_REVISION_10 = 0x10,
> +	DPCD_REVISION_11,
> +	DPCD_REVISION_12,
> +	DPCD_REVISION_13,
> +	DPCD_REVISION_14,

Any reason for ignoring defines in drm_dp.h?

> +};
> +
> +struct link_status {
> +	bool clock_recovered;
> +	bool channel_equalized;
> +	u8 cr_done_lanes;
> +};
> +
> +struct link_cap {
> +	enum dpcd_revision 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_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> new file mode 100644
> index 000000000000..3dcb847057a4
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -0,0 +1,76 @@
> +/* 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_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)
> +#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)
> +#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 V2 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc

[Yongbang Shi]

> On Tue, Oct 22, 2024 at 08:41:45PM +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>
>> ---
>> ChangeLog:
>> 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  |  31 ++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  74 +++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h  |  76 +++++++++
>>   5 files changed, 345 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_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..0078cafdf86d
>> --- /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 dp_aux_reset(const struct dp_dev *dp)
>> +{
>> +	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x0);
>> +	usleep_range(10, 15);
>> +	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x1);
>> +}
>> +
>> +static void dp_aux_read_data(struct dp_dev *dp, u8 *buf, u8 size)
>> +{
>> +	u32 reg_num;
>> +	u32 value;
>> +	u32 num;
>> +	u8 i, j;
>> +
>> +	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
>> +	for (i = 0; i < reg_num; i++) {
>> +		/* number of bytes read from a single register */
>> +		num = min(size - i * AUX_4_BYTE, AUX_4_BYTE);
>> +		value = readl(dp->base + DP_AUX_RD_DATA0 + i * AUX_4_BYTE);
>> +		/* convert the 32-bit value of the register to the buffer. */
>> +		for (j = 0; j < num; j++)
>> +			buf[i * AUX_4_BYTE + j] = value >> (j * AUX_8_BIT);
>> +	}
>> +}
>> +
>> +static void dp_aux_write_data(struct dp_dev *dp, u8 *buf, u8 size)
>> +{
>> +	u32 reg_num;
>> +	u32 value;
>> +	u8 i, j;
>> +	u32 num;
>> +
>> +	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
>> +	for (i = 0; i < reg_num; i++) {
>> +		/* number of bytes written to a single register */
>> +		num = min_t(u8, size - i * AUX_4_BYTE, AUX_4_BYTE);
>> +		value = 0;
>> +		/* obtain the 32-bit value written to a single register. */
>> +		for (j = 0; j < num; j++)
>> +			value |= buf[i * AUX_4_BYTE + j] << (j * AUX_8_BIT);
>> +		/* writing data to a single register */
>> +		writel(value, dp->base + DP_AUX_WR_DATA0 + i * AUX_4_BYTE);
>> +	}
>> +}
>> +
>> +static u32 dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
>> +{
>> +	u32 aux_cmd = msg->request;
>> +
>> +	if (msg->size)
>> +		aux_cmd |= (msg->size - 1) << AUX_CMD_REQ_LEN_S;
>> +	else
>> +		aux_cmd |= 1 << AUX_CMD_I2C_ADDR_ONLY_S;
>> +
>> +	aux_cmd |= msg->address << AUX_CMD_ADDR_S;
>> +
>> +	return aux_cmd;
>> +}
>> +
>> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
>> +static int 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 */
>> +			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 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);
>> +
>> +	dp_aux_write_data(dp, msg->buffer, msg->size);
>> +
>> +	aux_cmd = dp_aux_build_cmd(msg);
>> +	writel(aux_cmd, dp->base + DP_AUX_CMD_ADDR);
>> +
>> +	/* enable aux transfer */
>> +	dp_write_bits(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) {
>> +		dp_aux_reset(dp);
>> +		return ret;
>> +	}
>> +
>> +	return dp_aux_parse_xfer(dp, msg);
>> +}
>> +
>> +void dp_aux_init(struct dp_dev *dp)
>> +{
>> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_MIN_PULSE_NUM, DP_MIN_PULSE_NUM);
>> +
>> +	dp->aux.transfer = 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..6f95a3750d60
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
>> @@ -0,0 +1,31 @@
>> +/* 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
> unused
>
>> +
>> +#define DP_CFG_AUX_S			17
>> +#define DP_CFG_AUX_STATUS_S		4
> Both are unused
>
>> +
>> +#define AUX_4_BYTE			4
>> +#define AUX_4_BIT			4
>> +#define AUX_8_BIT			8
> AUX_4_BIT is unused. For other defines I think it's better to provide
> sensible names. BITS_IN_U32, BYTES_IN_U32
>
>> +
>> +#define AUX_READY_DATA_BYTE_S		12
> unused
>
>> +
>> +/* aux_cmd_addr register shift */
>> +#define AUX_CMD_REQ_LEN_S		4
>> +#define AUX_CMD_ADDR_S			8
>> +#define AUX_CMD_I2C_ADDR_ONLY_S		28
> Please use FIELD_PREP instead of definig shifts (and shift is usally
> _SHIFT, not _S)
>
>> +
>> +void 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..26d97929dc06
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> @@ -0,0 +1,74 @@
>> +/* 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 REG_LENGTH 32
>> +
>> +static inline u32 dp_read_bits(void __iomem *addr, u32 bit_mask)
>> +{
>> +	u32 reg_val;
>> +
>> +	reg_val = readl(addr);
>> +
>> +	return (reg_val & bit_mask) >> __ffs(bit_mask);
> FIELD_GET
>
>> +}
>> +
>> +static inline void dp_write_bits(void __iomem *addr, u32 bit_mask, u32 val)
>> +{
>> +	u32 reg_val;
>> +
>> +	reg_val = readl(addr);
>> +	reg_val &= ~bit_mask;
>> +	reg_val |= (val << __ffs(bit_mask)) & bit_mask;
> FIELD_PREP
>
>> +	writel(reg_val, addr);
> How is this protected from concurrent RMW cycles?
>
>> +}
>> +
>> +enum dpcd_revision {
>> +	DPCD_REVISION_10 = 0x10,
>> +	DPCD_REVISION_11,
>> +	DPCD_REVISION_12,
>> +	DPCD_REVISION_13,
>> +	DPCD_REVISION_14,
> Any reason for ignoring defines in drm_dp.h?

Hi Dmitry,
I tried it but still can't find it, if you know, can you tell me which macro I can use?
Thanks,
Baihan


>
>> +};
>> +
>> +struct link_status {
>> +	bool clock_recovered;
>> +	bool channel_equalized;
>> +	u8 cr_done_lanes;
>> +};
>> +
>> +struct link_cap {
>> +	enum dpcd_revision 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_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> new file mode 100644
>> index 000000000000..3dcb847057a4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -0,0 +1,76 @@
>> +/* 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_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)
>> +#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)
>> +#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 V2 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc

[Dmitry Baryshkov]

On Tue, 29 Oct 2024 at 16:15, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
> > On Tue, Oct 22, 2024 at 08:41:45PM +0800, Yongbang Shi wrote:
> >> From: baihan li <libaihan@huawei.com>

> >> +}
> >> +
> >> +enum dpcd_revision {
> >> +    DPCD_REVISION_10 = 0x10,
> >> +    DPCD_REVISION_11,
> >> +    DPCD_REVISION_12,
> >> +    DPCD_REVISION_13,
> >> +    DPCD_REVISION_14,
> > Any reason for ignoring defines in drm_dp.h?
>
> Hi Dmitry,
> I tried it but still can't find it, if you know, can you tell me which macro I can use?

# define DP_DPCD_REV_10                     0x10
# define DP_DPCD_REV_11                     0x11
etc

> Thanks,
> Baihan


-- 
With best wishes
Dmitry

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

[Yongbang Shi]

> On Tue, 29 Oct 2024 at 16:15, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>> On Tue, Oct 22, 2024 at 08:41:45PM +0800, Yongbang Shi wrote:
>>>> From: baihan li <libaihan@huawei.com>
>>>> +}
>>>> +
>>>> +enum dpcd_revision {
>>>> +    DPCD_REVISION_10 = 0x10,
>>>> +    DPCD_REVISION_11,
>>>> +    DPCD_REVISION_12,
>>>> +    DPCD_REVISION_13,
>>>> +    DPCD_REVISION_14,
>>> Any reason for ignoring defines in drm_dp.h?
>> Hi Dmitry,
>> I tried it but still can't find it, if you know, can you tell me which macro I can use?
> # define DP_DPCD_REV_10                     0x10
> # define DP_DPCD_REV_11                     0x11
> etc

Thanks, I got it.

Baihan.


>> Thanks,
>> Baihan
>

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

[Yongbang Shi]

> On Tue, Oct 22, 2024 at 08:41:45PM +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>
>> ---
>> ChangeLog:
>> 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  |  31 ++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  74 +++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h  |  76 +++++++++
>>   5 files changed, 345 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_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..0078cafdf86d
>> --- /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 dp_aux_reset(const struct dp_dev *dp)
>> +{
>> +	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x0);
>> +	usleep_range(10, 15);
>> +	dp_write_bits(dp->base + DP_DPTX_RST_CTRL, DP_CFG_AUX_RST_N, 0x1);
>> +}
>> +
>> +static void dp_aux_read_data(struct dp_dev *dp, u8 *buf, u8 size)
>> +{
>> +	u32 reg_num;
>> +	u32 value;
>> +	u32 num;
>> +	u8 i, j;
>> +
>> +	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
>> +	for (i = 0; i < reg_num; i++) {
>> +		/* number of bytes read from a single register */
>> +		num = min(size - i * AUX_4_BYTE, AUX_4_BYTE);
>> +		value = readl(dp->base + DP_AUX_RD_DATA0 + i * AUX_4_BYTE);
>> +		/* convert the 32-bit value of the register to the buffer. */
>> +		for (j = 0; j < num; j++)
>> +			buf[i * AUX_4_BYTE + j] = value >> (j * AUX_8_BIT);
>> +	}
>> +}
>> +
>> +static void dp_aux_write_data(struct dp_dev *dp, u8 *buf, u8 size)
>> +{
>> +	u32 reg_num;
>> +	u32 value;
>> +	u8 i, j;
>> +	u32 num;
>> +
>> +	reg_num = round_up(size, AUX_4_BYTE) / AUX_4_BYTE;
>> +	for (i = 0; i < reg_num; i++) {
>> +		/* number of bytes written to a single register */
>> +		num = min_t(u8, size - i * AUX_4_BYTE, AUX_4_BYTE);
>> +		value = 0;
>> +		/* obtain the 32-bit value written to a single register. */
>> +		for (j = 0; j < num; j++)
>> +			value |= buf[i * AUX_4_BYTE + j] << (j * AUX_8_BIT);
>> +		/* writing data to a single register */
>> +		writel(value, dp->base + DP_AUX_WR_DATA0 + i * AUX_4_BYTE);
>> +	}
>> +}
>> +
>> +static u32 dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
>> +{
>> +	u32 aux_cmd = msg->request;
>> +
>> +	if (msg->size)
>> +		aux_cmd |= (msg->size - 1) << AUX_CMD_REQ_LEN_S;
>> +	else
>> +		aux_cmd |= 1 << AUX_CMD_I2C_ADDR_ONLY_S;
>> +
>> +	aux_cmd |= msg->address << AUX_CMD_ADDR_S;
>> +
>> +	return aux_cmd;
>> +}
>> +
>> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
>> +static int 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 */
>> +			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 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);
>> +
>> +	dp_aux_write_data(dp, msg->buffer, msg->size);
>> +
>> +	aux_cmd = dp_aux_build_cmd(msg);
>> +	writel(aux_cmd, dp->base + DP_AUX_CMD_ADDR);
>> +
>> +	/* enable aux transfer */
>> +	dp_write_bits(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) {
>> +		dp_aux_reset(dp);
>> +		return ret;
>> +	}
>> +
>> +	return dp_aux_parse_xfer(dp, msg);
>> +}
>> +
>> +void dp_aux_init(struct dp_dev *dp)
>> +{
>> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> +	dp_write_bits(dp->base + DP_AUX_REQ, DP_CFG_AUX_MIN_PULSE_NUM, DP_MIN_PULSE_NUM);
>> +
>> +	dp->aux.transfer = 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..6f95a3750d60
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.h
>> @@ -0,0 +1,31 @@
>> +/* 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
> unused
>
>> +
>> +#define DP_CFG_AUX_S			17
>> +#define DP_CFG_AUX_STATUS_S		4
> Both are unused
>
>> +
>> +#define AUX_4_BYTE			4
>> +#define AUX_4_BIT			4
>> +#define AUX_8_BIT			8
> AUX_4_BIT is unused. For other defines I think it's better to provide
> sensible names. BITS_IN_U32, BYTES_IN_U32
>
>> +
>> +#define AUX_READY_DATA_BYTE_S		12
> unused
>
>> +
>> +/* aux_cmd_addr register shift */
>> +#define AUX_CMD_REQ_LEN_S		4
>> +#define AUX_CMD_ADDR_S			8
>> +#define AUX_CMD_I2C_ADDR_ONLY_S		28
> Please use FIELD_PREP instead of definig shifts (and shift is usally
> _SHIFT, not _S)
>
>> +
>> +void 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..26d97929dc06
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> @@ -0,0 +1,74 @@
>> +/* 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 REG_LENGTH 32
>> +
>> +static inline u32 dp_read_bits(void __iomem *addr, u32 bit_mask)
>> +{
>> +	u32 reg_val;
>> +
>> +	reg_val = readl(addr);
>> +
>> +	return (reg_val & bit_mask) >> __ffs(bit_mask);
> FIELD_GET
>
>> +}
>> +
>> +static inline void dp_write_bits(void __iomem *addr, u32 bit_mask, u32 val)
>> +{
>> +	u32 reg_val;
>> +
>> +	reg_val = readl(addr);
>> +	reg_val &= ~bit_mask;
>> +	reg_val |= (val << __ffs(bit_mask)) & bit_mask;
> FIELD_PREP
>
>> +	writel(reg_val, addr);
> How is this protected from concurrent RMW cycles?

Hi Dmitry,
This is just an encapsulation for readl or writel functions to simplify every shift operation when calling them.
I'll consider concurrency and have locks every time where I call these read and write functions and their contexts.
Thanks,
Baihan


>> +}
>> +
>> +enum dpcd_revision {
>> +	DPCD_REVISION_10 = 0x10,
>> +	DPCD_REVISION_11,
>> +	DPCD_REVISION_12,
>> +	DPCD_REVISION_13,
>> +	DPCD_REVISION_14,
> Any reason for ignoring defines in drm_dp.h?
>
>> +};
>> +
>> +struct link_status {
>> +	bool clock_recovered;
>> +	bool channel_equalized;
>> +	u8 cr_done_lanes;
>> +};
>> +
>> +struct link_cap {
>> +	enum dpcd_revision 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_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> new file mode 100644
>> index 000000000000..3dcb847057a4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -0,0 +1,76 @@
>> +/* 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_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)
>> +#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)
>> +#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 V2 drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc

[Dmitry Baryshkov]

On Tue, Oct 22, 2024 at 08:41:45PM +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>

Generic comment: all patches miss your SoB. This is SoB of the author,
so you as a submitted should add yours.


-- 
With best wishes
Dmitry

[PATCH V2 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>
---
ChangeLog:
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 | 344 +++++++++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h |  25 ++
 3 files changed, 370 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..b02a536e0689
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
@@ -0,0 +1,344 @@
+// 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 dp_link_training_configure(struct dp_dev *dp)
+{
+	u8 buf[2];
+	int ret;
+
+	/* DP 2 lane */
+	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_LANE_DATA_EN,
+		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x3 : 0x1);
+	dp_write_bits(dp->base + DP_DPTX_GCTL0, DP_CFG_PHY_LANE_NUM,
+		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x1 : 0);
+
+	/* enhanced frame */
+	dp_write_bits(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_FRAME_MODE, 0x1);
+
+	/* set rate and lane count */
+	buf[0] = 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_err(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
+		return 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_err(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", 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 dp_link_pattern2dpcd(struct dp_dev *dp, enum dp_pattern_e pattern)
+{
+	switch (pattern) {
+	case DP_PATTERN_NO:
+		return DP_TRAINING_PATTERN_DISABLE;
+	case DP_PATTERN_TPS1:
+		return DP_TRAINING_PATTERN_1;
+	case DP_PATTERN_TPS2:
+		return DP_TRAINING_PATTERN_2;
+	case DP_PATTERN_TPS3:
+		return DP_TRAINING_PATTERN_3;
+	case DP_PATTERN_TPS4:
+		return DP_TRAINING_PATTERN_4;
+	default:
+		drm_err(dp->dev, "dp link unknown pattern %d\n", pattern);
+		return -EINVAL;
+	}
+}
+
+static int dp_link_set_pattern(struct dp_dev *dp, enum dp_pattern_e pattern)
+{
+	int ret;
+	u8 buf;
+
+	ret = dp_link_pattern2dpcd(dp, pattern);
+	if (ret < 0)
+		return ret;
+
+	buf = (u8)ret;
+	if (pattern != DP_TRAINING_PATTERN_DISABLE && pattern != DP_TRAINING_PATTERN_4) {
+		buf |= DP_LINK_SCRAMBLING_DISABLE;
+		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0x1);
+	} else {
+		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0);
+	}
+
+	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_PAT_SEL, pattern);
+
+	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
+	if (ret != sizeof(buf))
+		drm_err(dp->dev, "dp aux write training pattern set failed\n");
+
+	return 0;
+}
+
+static int dp_link_training_cr_pre(struct dp_dev *dp)
+{
+	u8 *train_set = dp->link.train_set;
+	int ret;
+	u8 i;
+
+	ret = dp_link_training_configure(dp);
+	if (ret)
+		return ret;
+
+	ret = dp_link_set_pattern(dp, DP_PATTERN_TPS1);
+	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_err(dp->dev, "dp aux write training lane set failed\n");
+
+	return 0;
+}
+
+static bool 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;
+
+	/* not support level 3 */
+	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 dp_get_link_rate(u8 index)
+{
+	return link_rate_map[index];
+}
+
+static int 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 -EFAULT;
+}
+
+static int dp_link_reduce_lane(struct dp_dev *dp)
+{
+	if (dp->link.cap.lanes == DP_LANE_NUM_1) {
+		drm_err(dp->dev, "dp link training reduce lane failed, already reach minimum\n");
+		return -EFAULT;
+	}
+
+	/* currently only 1 lane */
+	dp->link.cap.lanes = DP_LANE_NUM_1;
+
+	return 0;
+}
+
+static int 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 >= DPCD_REVISION_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 = 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_err(dp->dev, "Update link training failed\n");
+			return 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 dp_link_training_channel_eq(struct dp_dev *dp)
+{
+	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
+	enum dp_pattern_e tps;
+	u8 eq_tries;
+	int ret;
+
+	if (dp->link.cap.is_tps4)
+		tps = DP_PATTERN_TPS4;
+	else if (dp->link.cap.is_tps3)
+		tps = DP_PATTERN_TPS3;
+	else
+		tps = DP_PATTERN_TPS2;
+
+	ret = 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;
+		}
+
+		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_err(dp->dev, "Update link training failed\n");
+			break;
+		}
+	}
+
+	if (eq_tries == EQ_MAX_RETRY)
+		drm_err(dp->dev, "channel equalization failed %u times\n", eq_tries);
+
+	dp_link_set_pattern(dp, DP_PATTERN_NO);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int dp_link_downgrade_training_cr(struct dp_dev *dp)
+{
+	if (dp_link_reduce_rate(dp))
+		return dp_link_reduce_lane(dp);
+
+	return 0;
+}
+
+static int dp_link_downgrade_training_eq(struct dp_dev *dp)
+{
+	if ((dp->link.status.clock_recovered && !dp->link.status.channel_equalized)) {
+		if (!dp_link_reduce_lane(dp))
+			return 0;
+	}
+
+	return dp_link_reduce_rate(dp);
+}
+
+int dp_link_training(struct dp_dev *dp)
+{
+	struct hibmc_dp_link *link = &dp->link;
+	int ret;
+
+	while (true) {
+		ret = dp_link_training_cr_pre(dp);
+		if (ret)
+			goto err;
+
+		ret = dp_link_training_cr(dp);
+		if (ret)
+			goto err;
+
+		if (!link->status.clock_recovered) {
+			ret = dp_link_downgrade_training_cr(dp);
+			if (ret)
+				goto err;
+			continue;
+		}
+
+		ret = dp_link_training_channel_eq(dp);
+		if (ret)
+			goto err;
+
+		if (!link->status.channel_equalized) {
+			ret = dp_link_downgrade_training_eq(dp);
+			if (ret)
+				goto err;
+			continue;
+		}
+
+		return 0;
+	}
+
+err:
+	dp_link_set_pattern(dp, DP_PATTERN_NO);
+
+	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..38877d8f473b
--- /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 dp_link_training(struct dp_dev *dp);
+u8 dp_get_link_rate(u8 index);
+
+#endif
-- 
2.33.0

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

[Dmitry Baryshkov]

On Tue, Oct 22, 2024 at 08:41:46PM +0800, Yongbang Shi wrote:
> From: baihan li <libaihan@huawei.com>
> 
> Add link training process functions in this moduel.

We should probably have a bounty for a developer who finally writes a
generic DP link training helpers.

> Signed-off-by: baihan li <libaihan@huawei.com>

Missing SoB

> ---
> ChangeLog:
> 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 | 344 +++++++++++++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h |  25 ++
>  3 files changed, 370 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..b02a536e0689
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
> @@ -0,0 +1,344 @@
> +// 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 dp_link_training_configure(struct dp_dev *dp)
> +{
> +	u8 buf[2];
> +	int ret;
> +
> +	/* DP 2 lane */
> +	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_LANE_DATA_EN,
> +		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x3 : 0x1);
> +	dp_write_bits(dp->base + DP_DPTX_GCTL0, DP_CFG_PHY_LANE_NUM,
> +		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x1 : 0);
> +
> +	/* enhanced frame */
> +	dp_write_bits(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_FRAME_MODE, 0x1);
> +
> +	/* set rate and lane count */
> +	buf[0] = 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_err(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
> +		return 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_err(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", 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 dp_link_pattern2dpcd(struct dp_dev *dp, enum dp_pattern_e pattern)
> +{
> +	switch (pattern) {
> +	case DP_PATTERN_NO:
> +		return DP_TRAINING_PATTERN_DISABLE;
> +	case DP_PATTERN_TPS1:
> +		return DP_TRAINING_PATTERN_1;
> +	case DP_PATTERN_TPS2:
> +		return DP_TRAINING_PATTERN_2;
> +	case DP_PATTERN_TPS3:
> +		return DP_TRAINING_PATTERN_3;
> +	case DP_PATTERN_TPS4:
> +		return DP_TRAINING_PATTERN_4;
> +	default:
> +		drm_err(dp->dev, "dp link unknown pattern %d\n", pattern);
> +		return -EINVAL;

Why do you need the extra defines / wrappers? Can you use
DP_TRAINING_PATTERN_foo directly?

> +	}
> +}
> +
> +static int dp_link_set_pattern(struct dp_dev *dp, enum dp_pattern_e pattern)
> +{
> +	int ret;
> +	u8 buf;
> +
> +	ret = dp_link_pattern2dpcd(dp, pattern);
> +	if (ret < 0)
> +		return ret;
> +
> +	buf = (u8)ret;
> +	if (pattern != DP_TRAINING_PATTERN_DISABLE && pattern != DP_TRAINING_PATTERN_4) {
> +		buf |= DP_LINK_SCRAMBLING_DISABLE;
> +		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0x1);
> +	} else {
> +		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0);
> +	}
> +
> +	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_PAT_SEL, pattern);
> +
> +	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
> +	if (ret != sizeof(buf))
> +		drm_err(dp->dev, "dp aux write training pattern set failed\n");
> +
> +	return 0;
> +}
> +
> +static int dp_link_training_cr_pre(struct dp_dev *dp)
> +{
> +	u8 *train_set = dp->link.train_set;
> +	int ret;
> +	u8 i;
> +
> +	ret = dp_link_training_configure(dp);
> +	if (ret)
> +		return ret;
> +
> +	ret = dp_link_set_pattern(dp, DP_PATTERN_TPS1);
> +	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_err(dp->dev, "dp aux write training lane set failed\n");
> +
> +	return 0;
> +}
> +
> +static bool 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;
> +
> +	/* not support level 3 */

??

> +	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 dp_get_link_rate(u8 index)
> +{
> +	return link_rate_map[index];
> +}

Use the array directly

> +
> +static int 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 -EFAULT;

EFAULT => "Bad address". Probably the error code should be slightly
different.

> +}
> +
> +static int dp_link_reduce_lane(struct dp_dev *dp)
> +{
> +	if (dp->link.cap.lanes == DP_LANE_NUM_1) {
> +		drm_err(dp->dev, "dp link training reduce lane failed, already reach minimum\n");
> +		return -EFAULT;
> +	}
> +
> +	/* currently only 1 lane */

You've probably meant "1 or 2 lanes". Still a switchcase might be
better.

> +	dp->link.cap.lanes = DP_LANE_NUM_1;
> +
> +	return 0;
> +}
> +
> +static int 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 >= DPCD_REVISION_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 = 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_err(dp->dev, "Update link training failed\n");
> +			return 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 dp_link_training_channel_eq(struct dp_dev *dp)
> +{
> +	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
> +	enum dp_pattern_e tps;
> +	u8 eq_tries;
> +	int ret;
> +
> +	if (dp->link.cap.is_tps4)
> +		tps = DP_PATTERN_TPS4;
> +	else if (dp->link.cap.is_tps3)
> +		tps = DP_PATTERN_TPS3;
> +	else
> +		tps = DP_PATTERN_TPS2;
> +
> +	ret = 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;
> +		}
> +
> +		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_err(dp->dev, "Update link training failed\n");
> +			break;
> +		}
> +	}
> +
> +	if (eq_tries == EQ_MAX_RETRY)
> +		drm_err(dp->dev, "channel equalization failed %u times\n", eq_tries);
> +
> +	dp_link_set_pattern(dp, DP_PATTERN_NO);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int dp_link_downgrade_training_cr(struct dp_dev *dp)
> +{
> +	if (dp_link_reduce_rate(dp))
> +		return dp_link_reduce_lane(dp);
> +
> +	return 0;
> +}
> +
> +static int dp_link_downgrade_training_eq(struct dp_dev *dp)
> +{
> +	if ((dp->link.status.clock_recovered && !dp->link.status.channel_equalized)) {
> +		if (!dp_link_reduce_lane(dp))
> +			return 0;
> +	}
> +
> +	return dp_link_reduce_rate(dp);
> +}
> +
> +int dp_link_training(struct dp_dev *dp)
> +{
> +	struct hibmc_dp_link *link = &dp->link;
> +	int ret;
> +
> +	while (true) {
> +		ret = dp_link_training_cr_pre(dp);
> +		if (ret)
> +			goto err;
> +
> +		ret = dp_link_training_cr(dp);
> +		if (ret)
> +			goto err;
> +
> +		if (!link->status.clock_recovered) {
> +			ret = dp_link_downgrade_training_cr(dp);
> +			if (ret)
> +				goto err;
> +			continue;
> +		}
> +
> +		ret = dp_link_training_channel_eq(dp);
> +		if (ret)
> +			goto err;
> +
> +		if (!link->status.channel_equalized) {
> +			ret = dp_link_downgrade_training_eq(dp);
> +			if (ret)
> +				goto err;
> +			continue;
> +		}
> +
> +		return 0;
> +	}
> +
> +err:
> +	dp_link_set_pattern(dp, DP_PATTERN_NO);
> +
> +	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..38877d8f473b
> --- /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 dp_link_training(struct dp_dev *dp);
> +u8 dp_get_link_rate(u8 index);
> +
> +#endif
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

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

[Yongbang Shi]

> On Tue, Oct 22, 2024 at 08:41:46PM +0800, Yongbang Shi wrote:
>> From: baihan li <libaihan@huawei.com>
>>
>> Add link training process functions in this moduel.
> We should probably have a bounty for a developer who finally writes a
> generic DP link training helpers.
>
>> Signed-off-by: baihan li <libaihan@huawei.com>
> Missing SoB
>
>> ---
>> ChangeLog:
>> 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 | 344 +++++++++++++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h |  25 ++
>>   3 files changed, 370 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..b02a536e0689
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
>> @@ -0,0 +1,344 @@
>> +// 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 dp_link_training_configure(struct dp_dev *dp)
>> +{
>> +	u8 buf[2];
>> +	int ret;
>> +
>> +	/* DP 2 lane */
>> +	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_LANE_DATA_EN,
>> +		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x3 : 0x1);
>> +	dp_write_bits(dp->base + DP_DPTX_GCTL0, DP_CFG_PHY_LANE_NUM,
>> +		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x1 : 0);
>> +
>> +	/* enhanced frame */
>> +	dp_write_bits(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_FRAME_MODE, 0x1);
>> +
>> +	/* set rate and lane count */
>> +	buf[0] = 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_err(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
>> +		return 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_err(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", 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 dp_link_pattern2dpcd(struct dp_dev *dp, enum dp_pattern_e pattern)
>> +{
>> +	switch (pattern) {
>> +	case DP_PATTERN_NO:
>> +		return DP_TRAINING_PATTERN_DISABLE;
>> +	case DP_PATTERN_TPS1:
>> +		return DP_TRAINING_PATTERN_1;
>> +	case DP_PATTERN_TPS2:
>> +		return DP_TRAINING_PATTERN_2;
>> +	case DP_PATTERN_TPS3:
>> +		return DP_TRAINING_PATTERN_3;
>> +	case DP_PATTERN_TPS4:
>> +		return DP_TRAINING_PATTERN_4;
>> +	default:
>> +		drm_err(dp->dev, "dp link unknown pattern %d\n", pattern);
>> +		return -EINVAL;
> Why do you need the extra defines / wrappers? Can you use
> DP_TRAINING_PATTERN_foo directly?

Hi Dmitry,
Thanks for your all of these good advices and questions. I will resply as soon as possible.
For this point, I also need this enum dp_pattern_e value to write in my dp reg at TPS stage, so I made this map here.

Thanks,
Baihan


>> +	}
>> +}
>> +
>> +static int dp_link_set_pattern(struct dp_dev *dp, enum dp_pattern_e pattern)
>> +{
>> +	int ret;
>> +	u8 buf;
>> +
>> +	ret = dp_link_pattern2dpcd(dp, pattern);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	buf = (u8)ret;
>> +	if (pattern != DP_TRAINING_PATTERN_DISABLE && pattern != DP_TRAINING_PATTERN_4) {
>> +		buf |= DP_LINK_SCRAMBLING_DISABLE;
>> +		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0x1);
>> +	} else {
>> +		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0);
>> +	}
>> +
>> +	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_PAT_SEL, pattern);
>> +
>> +	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
>> +	if (ret != sizeof(buf))
>> +		drm_err(dp->dev, "dp aux write training pattern set failed\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp_link_training_cr_pre(struct dp_dev *dp)
>> +{
>> +	u8 *train_set = dp->link.train_set;
>> +	int ret;
>> +	u8 i;
>> +
>> +	ret = dp_link_training_configure(dp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dp_link_set_pattern(dp, DP_PATTERN_TPS1);
>> +	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_err(dp->dev, "dp aux write training lane set failed\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static bool 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;
>> +
>> +	/* not support level 3 */
> ??
>
>> +	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 dp_get_link_rate(u8 index)
>> +{
>> +	return link_rate_map[index];
>> +}
> Use the array directly
>
>> +
>> +static int 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 -EFAULT;
> EFAULT => "Bad address". Probably the error code should be slightly
> different.
>
>> +}
>> +
>> +static int dp_link_reduce_lane(struct dp_dev *dp)
>> +{
>> +	if (dp->link.cap.lanes == DP_LANE_NUM_1) {
>> +		drm_err(dp->dev, "dp link training reduce lane failed, already reach minimum\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	/* currently only 1 lane */
> You've probably meant "1 or 2 lanes". Still a switchcase might be
> better.
>
>> +	dp->link.cap.lanes = DP_LANE_NUM_1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int 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 >= DPCD_REVISION_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 = 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_err(dp->dev, "Update link training failed\n");
>> +			return 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 dp_link_training_channel_eq(struct dp_dev *dp)
>> +{
>> +	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
>> +	enum dp_pattern_e tps;
>> +	u8 eq_tries;
>> +	int ret;
>> +
>> +	if (dp->link.cap.is_tps4)
>> +		tps = DP_PATTERN_TPS4;
>> +	else if (dp->link.cap.is_tps3)
>> +		tps = DP_PATTERN_TPS3;
>> +	else
>> +		tps = DP_PATTERN_TPS2;
>> +
>> +	ret = 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;
>> +		}
>> +
>> +		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_err(dp->dev, "Update link training failed\n");
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (eq_tries == EQ_MAX_RETRY)
>> +		drm_err(dp->dev, "channel equalization failed %u times\n", eq_tries);
>> +
>> +	dp_link_set_pattern(dp, DP_PATTERN_NO);
>> +
>> +	return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int dp_link_downgrade_training_cr(struct dp_dev *dp)
>> +{
>> +	if (dp_link_reduce_rate(dp))
>> +		return dp_link_reduce_lane(dp);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp_link_downgrade_training_eq(struct dp_dev *dp)
>> +{
>> +	if ((dp->link.status.clock_recovered && !dp->link.status.channel_equalized)) {
>> +		if (!dp_link_reduce_lane(dp))
>> +			return 0;
>> +	}
>> +
>> +	return dp_link_reduce_rate(dp);
>> +}
>> +
>> +int dp_link_training(struct dp_dev *dp)
>> +{
>> +	struct hibmc_dp_link *link = &dp->link;
>> +	int ret;
>> +
>> +	while (true) {
>> +		ret = dp_link_training_cr_pre(dp);
>> +		if (ret)
>> +			goto err;
>> +
>> +		ret = dp_link_training_cr(dp);
>> +		if (ret)
>> +			goto err;
>> +
>> +		if (!link->status.clock_recovered) {
>> +			ret = dp_link_downgrade_training_cr(dp);
>> +			if (ret)
>> +				goto err;
>> +			continue;
>> +		}
>> +
>> +		ret = dp_link_training_channel_eq(dp);
>> +		if (ret)
>> +			goto err;
>> +
>> +		if (!link->status.channel_equalized) {
>> +			ret = dp_link_downgrade_training_eq(dp);
>> +			if (ret)
>> +				goto err;
>> +			continue;
>> +		}
>> +
>> +		return 0;
>> +	}
>> +
>> +err:
>> +	dp_link_set_pattern(dp, DP_PATTERN_NO);
>> +
>> +	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..38877d8f473b
>> --- /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 dp_link_training(struct dp_dev *dp);
>> +u8 dp_get_link_rate(u8 index);
>> +
>> +#endif
>> -- 
>> 2.33.0
>>

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

[Dmitry Baryshkov]

On Tue, 29 Oct 2024 at 16:15, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
> > On Tue, Oct 22, 2024 at 08:41:46PM +0800, Yongbang Shi wrote:
> >> From: baihan li <libaihan@huawei.com>
> >>
> >> Add link training process functions in this moduel.
> > We should probably have a bounty for a developer who finally writes a
> > generic DP link training helpers.
> >
> >> Signed-off-by: baihan li <libaihan@huawei.com>
> > Missing SoB
> >
> >> ---
> >> ChangeLog:
> >> 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 | 344 +++++++++++++++++++
> >>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h |  25 ++
> >>   3 files changed, 370 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..b02a536e0689
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
> >> @@ -0,0 +1,344 @@
> >> +// 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 dp_link_training_configure(struct dp_dev *dp)
> >> +{
> >> +    u8 buf[2];
> >> +    int ret;
> >> +
> >> +    /* DP 2 lane */
> >> +    dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_LANE_DATA_EN,
> >> +                  dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x3 : 0x1);
> >> +    dp_write_bits(dp->base + DP_DPTX_GCTL0, DP_CFG_PHY_LANE_NUM,
> >> +                  dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x1 : 0);
> >> +
> >> +    /* enhanced frame */
> >> +    dp_write_bits(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_FRAME_MODE, 0x1);
> >> +
> >> +    /* set rate and lane count */
> >> +    buf[0] = 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_err(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
> >> +            return 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_err(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", 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 dp_link_pattern2dpcd(struct dp_dev *dp, enum dp_pattern_e pattern)
> >> +{
> >> +    switch (pattern) {
> >> +    case DP_PATTERN_NO:
> >> +            return DP_TRAINING_PATTERN_DISABLE;
> >> +    case DP_PATTERN_TPS1:
> >> +            return DP_TRAINING_PATTERN_1;
> >> +    case DP_PATTERN_TPS2:
> >> +            return DP_TRAINING_PATTERN_2;
> >> +    case DP_PATTERN_TPS3:
> >> +            return DP_TRAINING_PATTERN_3;
> >> +    case DP_PATTERN_TPS4:
> >> +            return DP_TRAINING_PATTERN_4;
> >> +    default:
> >> +            drm_err(dp->dev, "dp link unknown pattern %d\n", pattern);
> >> +            return -EINVAL;
> > Why do you need the extra defines / wrappers? Can you use
> > DP_TRAINING_PATTERN_foo directly?
>
> Hi Dmitry,
> Thanks for your all of these good advices and questions. I will resply as soon as possible.
> For this point, I also need this enum dp_pattern_e value to write in my dp reg at TPS stage, so I made this map here.

I'd suggest to do it other way around: use standard defines and when
necessary map them for the register write.

-- 
With best wishes
Dmitry

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

[Yongbang Shi]

> On Tue, Oct 22, 2024 at 08:41:46PM +0800, Yongbang Shi wrote:
>> From: baihan li <libaihan@huawei.com>
>>
>> Add link training process functions in this moduel.
> We should probably have a bounty for a developer who finally writes a
> generic DP link training helpers.
>
>> Signed-off-by: baihan li <libaihan@huawei.com>
> Missing SoB
>
>> ---
>> ChangeLog:
>> 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 | 344 +++++++++++++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.h |  25 ++
>>   3 files changed, 370 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..b02a536e0689
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
>> @@ -0,0 +1,344 @@
>> +// 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 dp_link_training_configure(struct dp_dev *dp)
>> +{
>> +	u8 buf[2];
>> +	int ret;
>> +
>> +	/* DP 2 lane */
>> +	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_LANE_DATA_EN,
>> +		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x3 : 0x1);
>> +	dp_write_bits(dp->base + DP_DPTX_GCTL0, DP_CFG_PHY_LANE_NUM,
>> +		      dp->link.cap.lanes == DP_LANE_NUM_2 ? 0x1 : 0);
>> +
>> +	/* enhanced frame */
>> +	dp_write_bits(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_FRAME_MODE, 0x1);
>> +
>> +	/* set rate and lane count */
>> +	buf[0] = 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_err(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
>> +		return 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_err(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", 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 dp_link_pattern2dpcd(struct dp_dev *dp, enum dp_pattern_e pattern)
>> +{
>> +	switch (pattern) {
>> +	case DP_PATTERN_NO:
>> +		return DP_TRAINING_PATTERN_DISABLE;
>> +	case DP_PATTERN_TPS1:
>> +		return DP_TRAINING_PATTERN_1;
>> +	case DP_PATTERN_TPS2:
>> +		return DP_TRAINING_PATTERN_2;
>> +	case DP_PATTERN_TPS3:
>> +		return DP_TRAINING_PATTERN_3;
>> +	case DP_PATTERN_TPS4:
>> +		return DP_TRAINING_PATTERN_4;
>> +	default:
>> +		drm_err(dp->dev, "dp link unknown pattern %d\n", pattern);
>> +		return -EINVAL;
> Why do you need the extra defines / wrappers? Can you use
> DP_TRAINING_PATTERN_foo directly?
>
>> +	}
>> +}
>> +
>> +static int dp_link_set_pattern(struct dp_dev *dp, enum dp_pattern_e pattern)
>> +{
>> +	int ret;
>> +	u8 buf;
>> +
>> +	ret = dp_link_pattern2dpcd(dp, pattern);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	buf = (u8)ret;
>> +	if (pattern != DP_TRAINING_PATTERN_DISABLE && pattern != DP_TRAINING_PATTERN_4) {
>> +		buf |= DP_LINK_SCRAMBLING_DISABLE;
>> +		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0x1);
>> +	} else {
>> +		dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_SCRAMBLE_EN, 0);
>> +	}
>> +
>> +	dp_write_bits(dp->base + DP_PHYIF_CTRL0, DP_CFG_PAT_SEL, pattern);
>> +
>> +	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
>> +	if (ret != sizeof(buf))
>> +		drm_err(dp->dev, "dp aux write training pattern set failed\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp_link_training_cr_pre(struct dp_dev *dp)
>> +{
>> +	u8 *train_set = dp->link.train_set;
>> +	int ret;
>> +	u8 i;
>> +
>> +	ret = dp_link_training_configure(dp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dp_link_set_pattern(dp, DP_PATTERN_TPS1);
>> +	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_err(dp->dev, "dp aux write training lane set failed\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static bool 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;
>> +
>> +	/* not support level 3 */
> ??
>
>> +	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 dp_get_link_rate(u8 index)
>> +{
>> +	return link_rate_map[index];
>> +}
> Use the array directly

Hi Dmitry,
Because we need to use this global array in other files, and I don't want to extern it directly.
Thanks,
Baihan


>> +
>> +static int 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 -EFAULT;
> EFAULT => "Bad address". Probably the error code should be slightly
> different.
>
>> +}
>> +
>> +static int dp_link_reduce_lane(struct dp_dev *dp)
>> +{
>> +	if (dp->link.cap.lanes == DP_LANE_NUM_1) {
>> +		drm_err(dp->dev, "dp link training reduce lane failed, already reach minimum\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	/* currently only 1 lane */
> You've probably meant "1 or 2 lanes". Still a switchcase might be
> better.
>
>> +	dp->link.cap.lanes = DP_LANE_NUM_1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int 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 >= DPCD_REVISION_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 = 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_err(dp->dev, "Update link training failed\n");
>> +			return 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 dp_link_training_channel_eq(struct dp_dev *dp)
>> +{
>> +	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
>> +	enum dp_pattern_e tps;
>> +	u8 eq_tries;
>> +	int ret;
>> +
>> +	if (dp->link.cap.is_tps4)
>> +		tps = DP_PATTERN_TPS4;
>> +	else if (dp->link.cap.is_tps3)
>> +		tps = DP_PATTERN_TPS3;
>> +	else
>> +		tps = DP_PATTERN_TPS2;
>> +
>> +	ret = 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;
>> +		}
>> +
>> +		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_err(dp->dev, "Update link training failed\n");
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (eq_tries == EQ_MAX_RETRY)
>> +		drm_err(dp->dev, "channel equalization failed %u times\n", eq_tries);
>> +
>> +	dp_link_set_pattern(dp, DP_PATTERN_NO);
>> +
>> +	return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int dp_link_downgrade_training_cr(struct dp_dev *dp)
>> +{
>> +	if (dp_link_reduce_rate(dp))
>> +		return dp_link_reduce_lane(dp);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp_link_downgrade_training_eq(struct dp_dev *dp)
>> +{
>> +	if ((dp->link.status.clock_recovered && !dp->link.status.channel_equalized)) {
>> +		if (!dp_link_reduce_lane(dp))
>> +			return 0;
>> +	}
>> +
>> +	return dp_link_reduce_rate(dp);
>> +}
>> +
>> +int dp_link_training(struct dp_dev *dp)
>> +{
>> +	struct hibmc_dp_link *link = &dp->link;
>> +	int ret;
>> +
>> +	while (true) {
>> +		ret = dp_link_training_cr_pre(dp);
>> +		if (ret)
>> +			goto err;
>> +
>> +		ret = dp_link_training_cr(dp);
>> +		if (ret)
>> +			goto err;
>> +
>> +		if (!link->status.clock_recovered) {
>> +			ret = dp_link_downgrade_training_cr(dp);
>> +			if (ret)
>> +				goto err;
>> +			continue;
>> +		}
>> +
>> +		ret = dp_link_training_channel_eq(dp);
>> +		if (ret)
>> +			goto err;
>> +
>> +		if (!link->status.channel_equalized) {
>> +			ret = dp_link_downgrade_training_eq(dp);
>> +			if (ret)
>> +				goto err;
>> +			continue;
>> +		}
>> +
>> +		return 0;
>> +	}
>> +
>> +err:
>> +	dp_link_set_pattern(dp, DP_PATTERN_NO);
>> +
>> +	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..38877d8f473b
>> --- /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 dp_link_training(struct dp_dev *dp);
>> +u8 dp_get_link_rate(u8 index);
>> +
>> +#endif
>> -- 
>> 2.33.0
>>

[PATCH V2 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>
---
ChangeLog:
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 +-
 .../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 +++
 4 files changed, 288 insertions(+), 1 deletion(-)
 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

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_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
new file mode 100644
index 000000000000..4846deb794ae
--- /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_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
new file mode 100644
index 000000000000..5ed3c9138790
--- /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 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 dp_set_tu(struct dp_dev *dp, struct drm_display_mode *mode)
+{
+	u32 tu_symbol_frac_size;
+	u32 tu_symbol_size;
+	u64 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 = 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_write_bits(dp->base + DP_VIDEO_PACKET,
+		      DP_CFG_STREAM_TU_SYMBOL_SIZE, tu_symbol_size);
+	dp_write_bits(dp->base + DP_VIDEO_PACKET,
+		      DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE, tu_symbol_frac_size);
+}
+
+static void 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 = 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)((u64)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 = (u64)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_write_bits(dp->base + DP_VIDEO_HORIZONTAL_SIZE, DP_CFG_STREAM_HTOTAL_SIZE, htotal_size);
+	dp_write_bits(dp->base + DP_VIDEO_HORIZONTAL_SIZE, DP_CFG_STREAM_HBLANK_SIZE, hblank_size);
+}
+
+static void 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_write_bits(dp->base + DP_TIMING_GEN_CONFIG0,
+		      DP_CFG_TIMING_GEN0_HBLANK, (mode->htotal - mode->hdisplay));
+	dp_write_bits(dp->base + DP_TIMING_GEN_CONFIG0,
+		      DP_CFG_TIMING_GEN0_HACTIVE, mode->hdisplay);
+
+	dp_write_bits(dp->base + DP_TIMING_GEN_CONFIG2,
+		      DP_CFG_TIMING_GEN0_VBLANK, vblank);
+	dp_write_bits(dp->base + DP_TIMING_GEN_CONFIG2,
+		      DP_CFG_TIMING_GEN0_VACTIVE, mode->vdisplay);
+	dp_write_bits(dp->base + DP_TIMING_GEN_CONFIG3,
+		      DP_CFG_TIMING_GEN0_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
+
+	dp_write_bits(dp->base + DP_VIDEO_CONFIG0,
+		      DP_CFG_STREAM_HACTIVE, mode->hdisplay);
+	dp_write_bits(dp->base + DP_VIDEO_CONFIG0,
+		      DP_CFG_STREAM_HBLANK, (mode->htotal - mode->hdisplay));
+	dp_write_bits(dp->base + DP_VIDEO_CONFIG2,
+		      DP_CFG_STREAM_HSYNC_WIDTH, (mode->hsync_end - mode->hsync_start));
+
+	dp_write_bits(dp->base + DP_VIDEO_CONFIG1,
+		      DP_CFG_STREAM_VACTIVE, mode->vdisplay);
+	dp_write_bits(dp->base + DP_VIDEO_CONFIG1,
+		      DP_CFG_STREAM_VBLANK, vblank);
+	dp_write_bits(dp->base + DP_VIDEO_CONFIG3,
+		      DP_CFG_STREAM_VFRONT_PORCH, (mode->vsync_start - mode->vdisplay));
+	dp_write_bits(dp->base + DP_VIDEO_CONFIG3,
+		      DP_CFG_STREAM_VSYNC_WIDTH, (mode->vsync_end - mode->vsync_start));
+
+	dp_write_bits(dp->base + DP_VIDEO_MSA0,
+		      DP_CFG_STREAM_VSTART, vstart);
+	dp_write_bits(dp->base + DP_VIDEO_MSA0,
+		      DP_CFG_STREAM_HSTART, hstart);
+
+	dp_write_bits(dp->base + DP_VIDEO_CTRL,
+		      DP_CFG_STREAM_VSYNC_POLARITY,
+		      mode->flags & DRM_MODE_FLAG_PVSYNC ? 1 : 0);
+	dp_write_bits(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);
+
+	dp_set_tu(dp, mode);
+
+	dp_write_bits(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_RGB_ENABLE, 0x1);
+	dp_write_bits(dp->base + DP_VIDEO_CTRL, DP_CFG_STREAM_VIDEO_MAPPING, 0);
+
+	/* divide 2: up even */
+	if (timing_delay - timing_delay / 2 * 2)
+		timing_delay++;
+
+	dp_write_bits(dp->base + DP_TIMING_MODEL_CTRL,
+		      DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1, timing_delay);
+
+	dp_set_sst(dp, mode);
+}
+
+int 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;
+
+	dp_aux_init(dp_dev);
+
+	ret = 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 dp_hw_uninit(struct hibmc_dp *dp)
+{
+	// keep this uninit interface in the future use
+}
+
+void dp_display_en(struct hibmc_dp *dp, bool enable)
+{
+	struct dp_dev *dp_dev = dp->dp_dev;
+
+	if (enable) {
+		dp_write_bits(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0x1);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+		dp_write_bits(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0x1);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+	} else {
+		dp_write_bits(dp_dev->base + DP_DPTX_GCTL0, BIT(10), 0);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+		dp_write_bits(dp_dev->base + DP_VIDEO_CTRL, BIT(0), 0);
+		writel(DP_SYNC_EN_MASK, dp_dev->base + DP_TIMING_SYNC_CTRL);
+	}
+
+	msleep(50);
+}
+
+int 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 = dp_link_training(dp_dev);
+		if (ret) {
+			drm_err(dp->drm_dev, "dp link training failed, ret: %d\n", ret);
+			return ret;
+		}
+	}
+
+	dp_display_en(dp, false);
+	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..f5394327dc99
--- /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 dp_hw_init(struct hibmc_dp *dp);
+void dp_hw_uninit(struct hibmc_dp *dp);
+int dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
+void dp_display_en(struct hibmc_dp *dp, bool enable);
+
+#endif
-- 
2.33.0

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

[kernel test robot]

Hi Yongbang,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on linus/master v6.12-rc4 next-20241024]
[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/20241022-204925
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20241022124148.1952761-4-shiyongbang%40huawei.com
patch subject: [PATCH V2 drm-dp 3/4] drm/hisilicon/hibmc: add dp hw moduel in hibmc
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241025/202410250931.UDQ9s66H-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410250931.UDQ9s66H-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/202410250931.UDQ9s66H-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/bridge/imx/imx-legacy-bridge.o
>> ERROR: modpost: "__divdi3" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined!
ERROR: modpost: "__udivdi3" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined!

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]

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

[PATCH V2 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>
---
ChangeLog:
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    | 148 ++++++++++++++++++
 .../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, 208 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..5d896dc56346
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#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;
+
+	dp_display_en(dp, false);
+
+	ret = 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_enable(struct hibmc_dp *dp)
+{
+	dp_display_en(dp, true);
+}
+
+static void hibmc_dp_disable(struct hibmc_dp *dp)
+{
+	dp_display_en(dp, false);
+}
+
+static int hibmc_dp_hw_init(struct hibmc_drm_private *priv)
+{
+	int ret;
+
+	ret = dp_hw_init(&priv->dp);
+	if (ret)
+		return ret;
+
+	dp_display_en(&priv->dp, false);
+
+	return 0;
+}
+
+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_enable(dp);
+}
+
+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_disable(dp);
+}
+
+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)
+{
+	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);
+	if (ret) {
+		drm_err(dev, "hibmc dp hw init failed: %d\n", ret);
+		return ret;
+	}
+
+	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:
+	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 V2 drm-dp 4/4] drm/hisilicon/hibmc: add dp module in hibmc

[kernel test robot]

Hi Yongbang,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on linus/master v6.12-rc4 next-20241024]
[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/20241022-204925
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20241022124148.1952761-5-shiyongbang%40huawei.com
patch subject: [PATCH V2 drm-dp 4/4] drm/hisilicon/hibmc: add dp module in hibmc
config: powerpc-randconfig-003-20241024 (https://download.01.org/0day-ci/archive/20241025/202410251136.1m7BlR68-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410251136.1m7BlR68-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/202410251136.1m7BlR68-lkp@intel.com/

All errors (new ones prefixed by >>):

   powerpc-linux-ld: drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.o: in function `dp_link_cfg':
>> dp_hw.c:(.text.dp_link_cfg+0x250): undefined reference to `__udivdi3'
>> powerpc-linux-ld: dp_hw.c:(.text.dp_link_cfg+0x340): undefined reference to `__udivdi3'
   powerpc-linux-ld: dp_hw.c:(.text.dp_link_cfg+0x384): undefined reference to `__udivdi3'

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