Re: [PATCH 2/6] drm/bridge: synopsys: Add DW DPTX Controller support library

From: Yubing Zhang
Date: Mon Mar 03 2025 - 04:00:16 EST

Hi Dmitry,

On 2025/3/2 2:14, Dmitry Baryshkov wrote:
On Sun, Feb 23, 2025 at 07:30:25PM +0800, Andy Yan wrote:
From: Andy Yan <andy.yan@xxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxx>

drm/bridge: cleanup

Stray line?


---

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

......

+
+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?

+ 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.


+ }
+}
+
+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.

+ 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;
+}
......
+
+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