On Mon, Mar 03, 2025 at 04:59:50PM +0800, Yubing Zhang wrote:
> 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.
This can be compared in dw_dp_link_get_adjustments(), before / while
updating the request data.
>
> >
> > > + }
> > > +}
> > > +
> > > +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.
> >
> > > + 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
> > >
> >
>
--
With best wishes
Dmitry
