On Mon, Mar 03, 2025 at 04:59:50PM +0800, Yubing Zhang wrote:

Hi Dmitry,

On 2025/3/2 2:14, Dmitry Baryshkov wrote:

quoted

On Sun, Feb 23, 2025 at 07:30:25PM +0800, Andy Yan wrote:

quoted

From: Andy Yan <andy.yan@rock-chips.com>

The DW DP TX Controller is compliant with the DisplayPort Specification
Version 1.4 with the following features:

* DisplayPort 1.4a
* Main Link: 1/2/4 lanes
* Main Link Support 1.62Gbps, 2.7Gbps, 5.4Gbps and 8.1Gbps
* AUX channel 1Mbps
* Single Stream Transport(SST)
* Multistream Transport (MST)
*Type-C support (alternate mode)
* HDCP 2.2, HDCP 1.3
* Supports up to 8/10 bits per color component
* Supports RBG, YCbCr4:4:4, YCbCr4:2:2, YCbCr4:2:0
* Pixel clock up to 594MHz
* I2S, SPDIF audio interface

Add library with common helpers to make it can be shared with
other SoC.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

drm/bridge: cleanup

Stray line?

quoted

---

  drivers/gpu/drm/bridge/synopsys/Kconfig  |    7 +
  drivers/gpu/drm/bridge/synopsys/Makefile |    1 +
  drivers/gpu/drm/bridge/synopsys/dw-dp.c  | 2155 ++++++++++++++++++++++
  include/drm/bridge/dw_dp.h               |   19 +
  4 files changed, 2182 insertions(+)
  create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-dp.c

......

quoted

quoted

+
+static u8 dw_dp_voltage_max(u8 preemph)
+{
+	switch (preemph & DP_TRAIN_PRE_EMPHASIS_MASK) {
+	case DP_TRAIN_PRE_EMPH_LEVEL_0:
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
+	case DP_TRAIN_PRE_EMPH_LEVEL_1:
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
+	case DP_TRAIN_PRE_EMPH_LEVEL_2:
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_1;
+	case DP_TRAIN_PRE_EMPH_LEVEL_3:
+	default:
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
+	}
+}
+
+static void dw_dp_link_get_adjustments(struct dw_dp_link *link,
+				       u8 status[DP_LINK_STATUS_SIZE])
+{
+	struct dw_dp_link_train_set *adjust = &link->train.adjust;
+	u8 v = 0;
+	u8 p = 0;
+	unsigned int i;
+
+	for (i = 0; i < link->lanes; i++) {
+		v = drm_dp_get_adjust_request_voltage(status, i);
+		p = drm_dp_get_adjust_request_pre_emphasis(status, i);
+		if (p >=  DP_TRAIN_PRE_EMPH_LEVEL_3) {
+			adjust->pre_emphasis[i] = DP_TRAIN_PRE_EMPH_LEVEL_3 >>
+						  DP_TRAIN_PRE_EMPHASIS_SHIFT;
+			adjust->pre_max_reached[i] = true;
+		} else {
+			adjust->pre_emphasis[i] = p >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
+			adjust->pre_max_reached[i] = false;
+		}
+		v = min(v, dw_dp_voltage_max(p));
+		if (v >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3) {
+			adjust->voltage_swing[i] = DP_TRAIN_VOLTAGE_SWING_LEVEL_3 >>
+						   DP_TRAIN_VOLTAGE_SWING_SHIFT;
+			adjust->voltage_max_reached[i] = true;
+		} else {
+			adjust->voltage_swing[i] = v >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
+			adjust->voltage_max_reached[i] = false;
+		}
+	}
+}
+
+static void dw_dp_link_train_adjust(struct dw_dp_link_train *train)
+{
+	struct dw_dp_link_train_set *request = &train->request;
+	struct dw_dp_link_train_set *adjust = &train->adjust;
+	unsigned int i;
+
+	for (i = 0; i < 4; i++) {

Shouldn't it be a loop up to link->lanes?

quoted

+		if (request->voltage_swing[i] != adjust->voltage_swing[i])
+			request->voltage_swing[i] = adjust->voltage_swing[i];
+		if (request->voltage_max_reached[i] != adjust->voltage_max_reached[i])
+			request->voltage_max_reached[i] = adjust->voltage_max_reached[i];
+	}
+
+	for (i = 0; i < 4; i++) {
+		if (request->pre_emphasis[i] != adjust->pre_emphasis[i])
+			request->pre_emphasis[i] = adjust->pre_emphasis[i];
+		if (request->pre_max_reached[i] != adjust->pre_max_reached[i])
+			request->pre_max_reached[i] = adjust->pre_max_reached[i];

Why do you need separate request and adjustment structs?

During link training cr sequence, if dprx keep the LANEx_CR_DONE bit(s)
cleared, the request and adjustment structs are used to check the
old and new valud of ADJUST_REQUEST_LANEx_y register(s) is changed or not.

This can be compared in dw_dp_link_get_adjustments(), before / while
updating the request data.

quoted

quoted

+	}
+}
+
+static int dw_dp_link_clock_recovery(struct dw_dp *dp)
+{
+	struct dw_dp_link *link = &dp->link;
+	u8 status[DP_LINK_STATUS_SIZE];
+	unsigned int tries = 0;
+	int ret;
+
+	ret = dw_dp_link_train_set_pattern(dp, DP_TRAINING_PATTERN_1);
+	if (ret)
+		return ret;
+
+	for (;;) {
+		ret = dw_dp_link_train_update_vs_emph(dp);
+		if (ret)
+			return ret;
+
+		drm_dp_link_train_clock_recovery_delay(&dp->aux, link->dpcd);
+
+		ret = drm_dp_dpcd_read_link_status(&dp->aux, status);
+		if (ret < 0) {
+			dev_err(dp->dev, "failed to read link status: %d\n", ret);
+			return ret;
+		}
+
+		if (drm_dp_clock_recovery_ok(status, link->lanes)) {
+			link->train.clock_recovered = true;
+			break;
+		}
+
+		dw_dp_link_get_adjustments(link, status);
+
+		if (link->train.request.voltage_swing[0] ==
+		    link->train.adjust.voltage_swing[0])

Should this take all lanes to account? I think it might be posssible to
drop the adjust / request split and adjust tries in
dw_dp_link_get_adjustments() instead.

Yes, here shall compare both swing and pre-emphasis for all lanes. It's a
good idea to drop the adjust / request split. The swing and pre-emphasis
compare just need by cr sequence. But both cr and eq sequences use
dw_dp_link_get_adjustments(). It will be better to delete
dw_dp_link_train_adjust() and add a new function to adjust tries for cr
sequence.

SGTM.

quoted

quoted

+			tries++;
+		else
+			tries = 0;
+
+		if (tries == 5)
+			break;
+
+		dw_dp_link_train_adjust(&link->train);
+	}
+
+	return 0;
+}
+
+static int dw_dp_link_channel_equalization(struct dw_dp *dp)
+{
+	struct dw_dp_link *link = &dp->link;
+	u8 status[DP_LINK_STATUS_SIZE], pattern;
+	unsigned int tries;
+	int ret;
+
+	if (link->caps.tps4_supported)
+		pattern = DP_TRAINING_PATTERN_4;
+	else if (link->caps.tps3_supported)
+		pattern = DP_TRAINING_PATTERN_3;
+	else
+		pattern = DP_TRAINING_PATTERN_2;
+	ret = dw_dp_link_train_set_pattern(dp, pattern);
+	if (ret)
+		return ret;
+
+	for (tries = 1; tries < 5; tries++) {
+		ret = dw_dp_link_train_update_vs_emph(dp);
+		if (ret)
+			return ret;
+
+		drm_dp_link_train_channel_eq_delay(&dp->aux, link->dpcd);
+
+		ret = drm_dp_dpcd_read_link_status(&dp->aux, status);
+		if (ret < 0)
+			return ret;
+
+		if (!drm_dp_clock_recovery_ok(status, link->lanes)) {
+			dev_err(dp->dev, "clock recovery lost while equalizing channel\n");
+			link->train.clock_recovered = false;
+			break;
+		}
+
+		if (drm_dp_channel_eq_ok(status, link->lanes)) {
+			link->train.channel_equalized = true;
+			break;
+		}
+
+		dw_dp_link_get_adjustments(link, status);
+		dw_dp_link_train_adjust(&link->train);
+	}
+
+	return 0;
+}

......

quoted

quoted

+
+struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
+			 const struct dw_dp_plat_data *plat_data);
+#endif /* __DW_DP__ */
-- 
2.34.1

-- 
With best wishes
Dmitry