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

[Yubing Zhang <yubing.zhang@rock-chips.com>]

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