Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <CAFqH_50Gn7SuzJo2mDjGhsKC6_H=xMVKCbr2BM82geAyZaDu+A@mail.gmail.com>
References: <1526895424-22894-1-git-send-email-hl@rock-chips.com>
 <1526895424-22894-5-git-send-email-hl@rock-chips.com> <CAFqH_50w7NoUTaYpmCFDsuafie2a4v5Dirs5KbsV26ff5xEpGg@mail.gmail.com>
 <f3d7e6c7-0291-b909-4f0e-a55bc0cad603@rock-chips.com> <CAFqH_50Gn7SuzJo2mDjGhsKC6_H=xMVKCbr2BM82geAyZaDu+A@mail.gmail.com>
From:   Enric Balletbo Serra <eballetbo@gmail.com>
Date:   Tue, 22 May 2018 19:06:58 +0200
Message-ID: <CAFqH_52A6n9fuCFLbDaaAZB1YjyWLMVMViam5BvfYyA75eYr5w@mail.gmail.com>
Subject: Re: [PATCH v6 5/5] drm/rockchip: support dp training outside dp firmware
To:     hl <hl@rock-chips.com>
Cc:     Sean Paul <seanpaul@chromium.org>, David Airlie <airlied@linux.ie>,
        Chris Zhong <zyw@rock-chips.com>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Doug Anderson <dianders@chromium.org>,
        Brian Norris <briannorris@chromium.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        =?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>,
        daniel.vetter@intel.com, jani.nikula@linux.intel.com,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Lin,

2018-05-22 9:41 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> Hi Lin
>
> 2018-05-22 3:08 GMT+02:00 hl <hl@rock-chips.com>:
>> Hi Enric,
>>
>>
>>
>>
>> On Monday, May 21, 2018 11:22 PM, Enric Balletbo Serra wrote:
>>>
>>> Hi Lin,
>>>
>>> 2018-05-21 11:37 GMT+02:00 Lin Huang <hl@rock-chips.com>:
>>>>
>>>> DP firmware uses fixed phy config values to do training, but some
>>>> boards need to adjust these values to fit for their unique hardware
>>>> design. So get phy config values from dts and use software link training
>>>> instead of relying on firmware, if software training fail, keep firmware
>>>> training as a fallback if sw training fails.
>>>>
>>>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>>>> ---
>>>> Changes in v2:
>>>> - update patch following Enric suggest
>>>> Changes in v3:
>>>> - use variable fw_training instead sw_training_success
>>>> - base on DP SPCE, if training fail use lower link rate to retry training
>>>> Changes in v4:
>>>> - improve cdn_dp_get_lower_link_rate() and cdn_dp_software_train_link()
>>>> follow Sean suggest
>>>> Changes in v5:
>>>> - fix some whitespcae issue
>>>> Changes in v6:
>>>> - None
>>>>
>>>>   drivers/gpu/drm/rockchip/Makefile               |   3 +-
>>>>   drivers/gpu/drm/rockchip/cdn-dp-core.c          |  24 +-
>>>>   drivers/gpu/drm/rockchip/cdn-dp-core.h          |   2 +
>>>>   drivers/gpu/drm/rockchip/cdn-dp-link-training.c | 420
>>>> ++++++++++++++++++++++++
>>>>   drivers/gpu/drm/rockchip/cdn-dp-reg.c           |  31 +-
>>>>   drivers/gpu/drm/rockchip/cdn-dp-reg.h           |  38 ++-
>>>>   6 files changed, 505 insertions(+), 13 deletions(-)
>>>>   create mode 100644 drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/Makefile
>>>> b/drivers/gpu/drm/rockchip/Makefile
>>>> index a314e21..b932f62 100644
>>>> --- a/drivers/gpu/drm/rockchip/Makefile
>>>> +++ b/drivers/gpu/drm/rockchip/Makefile
>>>> @@ -9,7 +9,8 @@ rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>>>>   rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>>>>
>>>>   rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>>>> -rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
>>>> +rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o \
>>>> +                                       cdn-dp-link-training.o
>>>>   rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
>>>>   rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
>>>>   rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>> index cce64c1..d9d0d4d 100644
>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>> @@ -629,11 +629,13 @@ static void cdn_dp_encoder_enable(struct
>>>> drm_encoder *encoder)
>>>>                          goto out;
>>>>                  }
>>>>          }
>>>> -
>>>> -       ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
>>>> -       if (ret) {
>>>> -               DRM_DEV_ERROR(dp->dev, "Failed to idle video %d\n", ret);
>>>> -               goto out;
>>>> +       if (dp->use_fw_training == true) {
>>>
>>> You don't need to compare to true. Simply do:
>>>
>>> if (dp->use_fw_training)
>>>
>>>> +               ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
>>>> +               if (ret) {
>>>> +                       DRM_DEV_ERROR(dp->dev,
>>>> +                                     "Failed to idle video %d\n", ret);
>>>> +                       goto out;
>>>> +               }
>>>>          }
>>>>
>>>>          ret = cdn_dp_config_video(dp);
>>>> @@ -642,11 +644,15 @@ static void cdn_dp_encoder_enable(struct
>>>> drm_encoder *encoder)
>>>>                  goto out;
>>>>          }
>>>>
>>>> -       ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
>>>> -       if (ret) {
>>>> -               DRM_DEV_ERROR(dp->dev, "Failed to valid video %d\n",
>>>> ret);
>>>> -               goto out;
>>>> +       if (dp->use_fw_training == true) {
>>>
>>> if (dp->use_fw_training)
>>>
>>>> +               ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
>>>> +               if (ret) {
>>>> +                       DRM_DEV_ERROR(dp->dev,
>>>> +                               "Failed to valid video %d\n", ret);
>>>> +                       goto out;
>>>> +               }
>>>>          }
>>>> +
>>>>   out:
>>>>          mutex_unlock(&dp->lock);
>>>>   }
>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>> index 46159b2..77a9793 100644
>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>> @@ -84,6 +84,7 @@ struct cdn_dp_device {
>>>>          bool connected;
>>>>          bool active;
>>>>          bool suspended;
>>>> +       bool use_fw_training;
>>>>
>>>>          const struct firmware *fw;      /* cdn dp firmware */
>>>>          unsigned int fw_version;        /* cdn fw version */
>>>> @@ -106,6 +107,7 @@ struct cdn_dp_device {
>>>>          u8 ports;
>>>>          u8 lanes;
>>>>          int active_port;
>>>> +       u8 train_set[4];
>>>>
>>>>          u8 dpcd[DP_RECEIVER_CAP_SIZE];
>>>>          bool sink_has_audio;
>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>> b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>> new file mode 100644
>>>> index 0000000..73c3290
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>> @@ -0,0 +1,420 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
>>>> + * Author: Chris Zhong <zyw@rock-chips.com>
>>>> + */

Oh, I just noticed that this is not the preferred format for .c files,
see the discussion here

https://lkml.org/lkml/2017/11/25/133

>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/phy/phy.h>
>>>> +#include <soc/rockchip/rockchip_phy_typec.h>
>>>> +
>>>> +#include "cdn-dp-core.h"
>>>> +#include "cdn-dp-reg.h"
>>>> +
>>>> +static void cdn_dp_set_signal_levels(struct cdn_dp_device *dp)
>>>> +{
>>>> +       struct cdn_dp_port *port = dp->port[dp->active_port];
>>>> +       struct rockchip_typec_phy *tcphy = phy_get_drvdata(port->phy);
>>>> +
>>>> +       int rate = drm_dp_bw_code_to_link_rate(dp->link.rate);
>>>> +       u8 swing = (dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) >>
>>>> +                  DP_TRAIN_VOLTAGE_SWING_SHIFT;
>>>> +       u8 pre_emphasis = (dp->train_set[0] & DP_TRAIN_PRE_EMPHASIS_MASK)
>>>> +                         >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
>>>> +
>>>> +       tcphy->typec_phy_config(port->phy, rate, dp->link.num_lanes,
>>>> +                               swing, pre_emphasis);
>>>> +}
>>>> +
>>>> +static int cdn_dp_set_pattern(struct cdn_dp_device *dp, uint8_t
>>>> dp_train_pat)
>>>> +{
>>>> +       u32 phy_config, global_config;
>>>> +       int ret;
>>>> +       uint8_t pattern = dp_train_pat & DP_TRAINING_PATTERN_MASK;
>>>> +
>>>> +       global_config = NUM_LANES(dp->link.num_lanes - 1) | SST_MODE |
>>>> +                       GLOBAL_EN | RG_EN | ENC_RST_DIS | WR_VHSYNC_FALL;
>>>> +
>>>> +       phy_config = DP_TX_PHY_ENCODER_BYPASS(0) |
>>>> +                    DP_TX_PHY_SKEW_BYPASS(0) |
>>>> +                    DP_TX_PHY_DISPARITY_RST(0) |
>>>> +                    DP_TX_PHY_LANE0_SKEW(0) |
>>>> +                    DP_TX_PHY_LANE1_SKEW(1) |
>>>> +                    DP_TX_PHY_LANE2_SKEW(2) |
>>>> +                    DP_TX_PHY_LANE3_SKEW(3) |
>>>> +                    DP_TX_PHY_10BIT_ENABLE(0);
>>>> +
>>>> +       if (pattern != DP_TRAINING_PATTERN_DISABLE) {
>>>> +               global_config |= NO_VIDEO;
>>>> +               phy_config |= DP_TX_PHY_TRAINING_ENABLE(1) |
>>>> +                             DP_TX_PHY_SCRAMBLER_BYPASS(1) |
>>>> +                             DP_TX_PHY_TRAINING_PATTERN(pattern);
>>>> +       }
>>>> +
>>>> +       ret = cdn_dp_reg_write(dp, DP_FRAMER_GLOBAL_CONFIG,
>>>> global_config);
>>>> +       if (ret) {
>>>> +               DRM_ERROR("fail to set DP_FRAMER_GLOBAL_CONFIG, error:
>>>> %d\n",
>>>> +                         ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       ret = cdn_dp_reg_write(dp, DP_TX_PHY_CONFIG_REG, phy_config);
>>>> +       if (ret) {
>>>> +               DRM_ERROR("fail to set DP_TX_PHY_CONFIG_REG, error:
>>>> %d\n",
>>>> +                         ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       ret = cdn_dp_reg_write(dp, DPTX_LANE_EN, BIT(dp->link.num_lanes)
>>>> - 1);
>>>> +       if (ret) {
>>>> +               DRM_ERROR("fail to set DPTX_LANE_EN, error: %d\n", ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       if (drm_dp_enhanced_frame_cap(dp->dpcd))
>>>> +               ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 1);
>>>> +       else
>>>> +               ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 0);
>>>> +       if (ret)
>>>> +               DRM_ERROR("failed to set DPTX_ENHNCD, error: %x\n", ret);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static u8 cdn_dp_pre_emphasis_max(u8 voltage_swing)
>>>> +{
>>>> +       switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> +       case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> +               return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> +       case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> +               return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> +       case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> +               return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> +       default:
>>>> +               return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> +       }
>>>> +}
>>>> +
>>>> +static void cdn_dp_get_adjust_train(struct cdn_dp_device *dp,
>>>> +                                   uint8_t
>>>> link_status[DP_LINK_STATUS_SIZE])
>>>> +{
>>>> +       int i;
>>>> +       uint8_t v = 0, p = 0;
>>>> +       uint8_t preemph_max;
>>>> +
>>>> +       for (i = 0; i < dp->link.num_lanes; i++) {
>>>> +               v = max(v, drm_dp_get_adjust_request_voltage(link_status,
>>>> i));
>>>> +               p = max(p,
>>>> drm_dp_get_adjust_request_pre_emphasis(link_status,
>>>> +                                                                 i));
>>>> +       }
>>>> +
>>>> +       if (v >= VOLTAGE_LEVEL_2)
>>>> +               v = VOLTAGE_LEVEL_2 | DP_TRAIN_MAX_SWING_REACHED;
>>>> +
>>>> +       preemph_max = cdn_dp_pre_emphasis_max(v);
>>>> +       if (p >= preemph_max)
>>>> +               p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>>>> +
>>>> +       for (i = 0; i < dp->link.num_lanes; i++)
>>>> +               dp->train_set[i] = v | p;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Pick training pattern for channel equalization. Training Pattern 3
>>>> for HBR2
>>>> + * or 1.2 devices that support it, Training Pattern 2 otherwise.
>>>> + */
>>>> +static u32 cdn_dp_select_chaneq_pattern(struct cdn_dp_device *dp)
>>>> +{
>>>> +       u32 training_pattern = DP_TRAINING_PATTERN_2;
>>>> +
>>>> +       /*
>>>> +        * cdn dp support HBR2 also support TPS3. TPS3 support is also
>>>> mandatory
>>>> +        * for downstream devices that support HBR2. However, not all
>>>> sinks
>>>> +        * follow the spec.
>>>> +        */
>>>> +       if (drm_dp_tps3_supported(dp->dpcd))
>>>> +               training_pattern = DP_TRAINING_PATTERN_3;
>>>> +       else
>>>> +               DRM_DEBUG_KMS("5.4 Gbps link rate without sink TPS3
>>>> support\n");
>>>> +
>>>> +       return training_pattern;
>>>> +}
>>>> +
>>>> +
>>>> +static bool cdn_dp_link_max_vswing_reached(struct cdn_dp_device *dp)
>>>> +{
>>>> +       int lane;
>>>> +
>>>> +       for (lane = 0; lane < dp->link.num_lanes; lane++)
>>>> +               if ((dp->train_set[lane] & DP_TRAIN_MAX_SWING_REACHED) ==
>>>> 0)
>>>> +                       return false;
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>> +static int cdn_dp_update_link_train(struct cdn_dp_device *dp)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       cdn_dp_set_signal_levels(dp);
>>>> +
>>>> +       ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
>>>> +                               dp->train_set, dp->link.num_lanes);
>>>> +       if (ret != dp->link.num_lanes)
>>>> +               return -EINVAL;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int cdn_dp_set_link_train(struct cdn_dp_device *dp,
>>>> +                                 uint8_t dp_train_pat)
>>>> +{
>>>> +       uint8_t buf[sizeof(dp->train_set) + 1];
>>>> +       int ret, len;
>>>> +
>>>> +       buf[0] = dp_train_pat;
>>>> +       if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) ==
>>>> +           DP_TRAINING_PATTERN_DISABLE) {
>>>> +               /* don't write DP_TRAINING_LANEx_SET on disable */
>>>> +               len = 1;
>>>> +       } else {
>>>> +               /* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET
>>>> */
>>>> +               memcpy(buf + 1, dp->train_set, dp->link.num_lanes);
>>>> +               len = dp->link.num_lanes + 1;
>>>> +       }
>>>> +
>>>> +       ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET,
>>>> +                               buf, len);
>>>> +       if (ret != len)
>>>> +               return -EINVAL;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int cdn_dp_reset_link_train(struct cdn_dp_device *dp,
>>>> +                                   uint8_t dp_train_pat)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       memset(dp->train_set, 0, sizeof(dp->train_set));
>>>> +
>>>> +       cdn_dp_set_signal_levels(dp);
>>>> +
>>>> +       ret = cdn_dp_set_pattern(dp, dp_train_pat);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       return cdn_dp_set_link_train(dp, dp_train_pat);
>>>> +}
>>>> +
>>>> +/* Enable corresponding port and start training pattern 1 */
>>>> +static int cdn_dp_link_training_clock_recovery(struct cdn_dp_device *dp)
>>>> +{
>>>> +       u8 voltage;
>>>> +       u8 link_status[DP_LINK_STATUS_SIZE];
>>>> +       u32 voltage_tries, max_vswing_tries;
>>>> +       int ret;
>>>> +
>>>> +       /* clock recovery */
>>>> +       ret = cdn_dp_reset_link_train(dp, DP_TRAINING_PATTERN_1 |
>>>> +                                         DP_LINK_SCRAMBLING_DISABLE);
>>>> +       if (ret) {
>>>> +               DRM_ERROR("failed to start link train\n");
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       voltage_tries = 1;
>>>> +       max_vswing_tries = 0;
>>>> +       for (;;) {
>>>> +               drm_dp_link_train_clock_recovery_delay(dp->dpcd);
>>>> +               if (drm_dp_dpcd_read_link_status(&dp->aux, link_status)
>>>> !=
>>>> +                   DP_LINK_STATUS_SIZE) {
>>>> +                       DRM_ERROR("failed to get link status\n");
>>>> +                       return -EINVAL;
>>>> +               }
>>>> +
>>>> +               if (drm_dp_clock_recovery_ok(link_status,
>>>> dp->link.num_lanes)) {
>>>> +                       DRM_DEBUG_KMS("clock recovery OK\n");
>>>> +                       return 0;
>>>> +               }
>>>> +
>>>> +               if (voltage_tries >= 5) {
>>>> +                       DRM_DEBUG_KMS("Same voltage tried 5 times\n");
>>>> +                       return -EINVAL;
>>>> +               }
>>>> +
>>>> +               if (max_vswing_tries >= 1) {
>>>> +                       DRM_DEBUG_KMS("Max Voltage Swing reached\n");
>>>> +                       return -EINVAL;
>>>> +               }
>>>> +
>>>> +               voltage = dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
>>>> +
>>>> +               /* Update training set as requested by target */
>>>> +               cdn_dp_get_adjust_train(dp, link_status);
>>>> +               if (cdn_dp_update_link_train(dp)) {
>>>> +                       DRM_ERROR("failed to update link training\n");
>>>> +                       return -EINVAL;
>>>> +               }
>>>> +
>>>> +               if ((dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) ==
>>>> +                   voltage)
>>>> +                       ++voltage_tries;
>>>> +               else
>>>> +                       voltage_tries = 1;
>>>> +
>>>> +               if (cdn_dp_link_max_vswing_reached(dp))
>>>> +                       ++max_vswing_tries;
>>>> +       }
>>>> +}
>>>> +
>>>> +static int cdn_dp_link_training_channel_equalization(struct
>>>> cdn_dp_device *dp)
>>>> +{
>>>> +       int tries, ret;
>>>> +       u32 training_pattern;
>>>> +       uint8_t link_status[DP_LINK_STATUS_SIZE];
>>>> +
>>>> +       training_pattern = cdn_dp_select_chaneq_pattern(dp);
>>>> +       training_pattern |= DP_LINK_SCRAMBLING_DISABLE;
>>>> +
>>>> +       ret = cdn_dp_set_pattern(dp, training_pattern);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       ret = cdn_dp_set_link_train(dp, training_pattern);
>>>> +       if (ret) {
>>>> +               DRM_ERROR("failed to start channel equalization\n");
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       for (tries = 0; tries < 5; tries++) {
>>>> +               drm_dp_link_train_channel_eq_delay(dp->dpcd);
>>>> +               if (drm_dp_dpcd_read_link_status(&dp->aux, link_status)
>>>> !=
>>>> +                   DP_LINK_STATUS_SIZE) {
>>>> +                       DRM_ERROR("failed to get link status\n");
>>>> +                       break;
>>>> +               }
>>>> +
>>>> +               /* Make sure clock is still ok */
>>>> +               if (!drm_dp_clock_recovery_ok(link_status,
>>>> +                                             dp->link.num_lanes)) {
>>>> +                       DRM_DEBUG_KMS("Clock recovery check failed\n");
>>>> +                       break;
>>>> +               }
>>>> +
>>>> +               if (drm_dp_channel_eq_ok(link_status,
>>>> dp->link.num_lanes)) {
>>>> +                       DRM_DEBUG_KMS("Channel EQ done\n");
>>>> +                       return 0;
>>>> +               }
>>>> +
>>>> +               /* Update training set as requested by target */
>>>> +               cdn_dp_get_adjust_train(dp, link_status);
>>>> +               if (cdn_dp_update_link_train(dp)) {
>>>> +                       DRM_ERROR("failed to update link training\n");
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       /* Try 5 times, else fail and try at lower BW */
>>>> +       if (tries == 5)
>>>> +               DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +
>>>> +static int cdn_dp_stop_link_train(struct cdn_dp_device *dp)
>>>> +{
>>>> +       int ret = cdn_dp_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
>>>> +
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       return cdn_dp_set_link_train(dp, DP_TRAINING_PATTERN_DISABLE);
>>>> +}
>>>> +
>>>> +static int cdn_dp_get_lower_link_rate(struct cdn_dp_device *dp)
>>>> +{
>>>> +       switch (dp->link.rate) {
>>>> +       case DP_LINK_BW_1_62:
>>>> +               return -EINVAL;
>>>> +       case DP_LINK_BW_2_7:
>>>> +               dp->link.rate = DP_LINK_BW_1_62;
>>>> +               break;
>>>> +       case DP_LINK_BW_5_4:
>>>> +               dp->link.rate = DP_LINK_BW_2_7;
>>>> +               break;
>>>> +       default:
>>>> +               dp->link.rate = DP_LINK_BW_5_4;
>>>> +               break;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +int cdn_dp_software_train_link(struct cdn_dp_device *dp)
>>>> +{
>>>> +       int ret, stop_err;
>>>> +       u8 link_config[2];
>>>> +       u32 rate, sink_max, source_max;
>>>> +
>>>> +       ret = drm_dp_dpcd_read(&dp->aux, DP_DPCD_REV, dp->dpcd,
>>>> +                              sizeof(dp->dpcd));
>>>> +       if (ret < 0) {
>>>> +               DRM_DEV_ERROR(dp->dev, "Failed to get caps %d\n", ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       source_max = dp->lanes;
>>>> +       sink_max = drm_dp_max_lane_count(dp->dpcd);
>>>> +       dp->link.num_lanes = min(source_max, sink_max);
>>>> +
>>>> +       source_max = drm_dp_bw_code_to_link_rate(CDN_DP_MAX_LINK_RATE);
>>>> +       sink_max = drm_dp_max_link_rate(dp->dpcd);
>>>> +       rate = min(source_max, sink_max);
>>>> +       dp->link.rate = drm_dp_link_rate_to_bw_code(rate);
>>>> +
>>>> +       link_config[0] = 0;
>>>> +       link_config[1] = 0;
>>>> +       if (dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & 0x01)
>>>> +               link_config[1] = DP_SET_ANSI_8B10B;
>>>> +       drm_dp_dpcd_write(&dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>>>> +
>>>> +       while (true) {
>>>> +
>>>> +               /* Write the link configuration data */
>>>> +               link_config[0] = dp->link.rate;
>>>> +               link_config[1] = dp->link.num_lanes;
>>>> +               if (drm_dp_enhanced_frame_cap(dp->dpcd))
>>>> +                       link_config[1] |=
>>>> DP_LANE_COUNT_ENHANCED_FRAME_EN;
>>>> +               drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, link_config,
>>>> 2);
>>>> +
>>>> +               ret = cdn_dp_link_training_clock_recovery(dp);
>>>> +               if (ret) {
>>>> +                       if (!cdn_dp_get_lower_link_rate(dp))
>>>> +                               continue;
>>>> +
>>>> +                       DRM_ERROR("training clock recovery failed: %d\n",
>>>> ret);
>>>> +                       break;
>>>> +               }
>>>> +
>>>> +               ret = cdn_dp_link_training_channel_equalization(dp);
>>>> +               if (ret) {
>>>> +                       if (!cdn_dp_get_lower_link_rate(dp))
>>>> +                               continue;
>>>> +
>>>> +                       DRM_ERROR("training channel eq failed: %d\n",
>>>> ret);
>>>> +                       break;
>>>> +               }
>>>> +
>>>> +               break;
>>>> +       }
>>>> +
>>>> +       stop_err = cdn_dp_stop_link_train(dp);
>>>> +       if (stop_err) {
>>>> +               DRM_ERROR("stop training fail, error: %d\n", stop_err);
>>>> +               return stop_err;
>>>> +       }
>>>> +
>>>> +       return ret;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>> b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>> index 979355d..e1273e6 100644
>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>> @@ -17,7 +17,9 @@
>>>>   #include <linux/delay.h>
>>>>   #include <linux/io.h>
>>>>   #include <linux/iopoll.h>
>>>> +#include <linux/phy/phy.h>
>>>>   #include <linux/reset.h>
>>>> +#include <soc/rockchip/rockchip_phy_typec.h>
>>>>
>>>>   #include "cdn-dp-core.h"
>>>>   #include "cdn-dp-reg.h"
>>>> @@ -189,7 +191,7 @@ static int cdn_dp_mailbox_send(struct cdn_dp_device
>>>> *dp, u8 module_id,
>>>>          return 0;
>>>>   }
>>>>
>>>> -static int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
>>>> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
>>>>   {
>>>>          u8 msg[6];
>>>>
>>>> @@ -609,6 +611,31 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
>>>>   {
>>>>          int ret;
>>>>
>>>> +       /*
>>>> +        * DP firmware uses fixed phy config values to do training, but
>>>> some
>>>> +        * boards need to adjust these values to fit for their unique
>>>> hardware
>>>> +        * design. So if the phy is using custom config values, do
>>>> software
>>>> +        * link training instead of relying on firmware, if software
>>>> training
>>>> +        * fail, keep firmware training as a fallback if sw training
>>>> fails.
>>>> +        */
>>>> +       ret = cdn_dp_software_train_link(dp);
>>>> +       if (ret) {
>>>> +               DRM_DEV_ERROR(dp->dev,
>>>> +                       "Failed to do software training %d\n", ret);
>>>> +               goto do_fw_training;
>>>> +       }
>>>
>>> If I understand correctly you are changing current behavior. Before
>>> this patch, we use always firmware link training, and after this
>>> patch, we always use software link training. If fails we use the
>>> firmware link training.
>>>
>>> AFAIK my Samsung Chromebook Plus works well with firmware link
>>> training, so there are any benefits of use software link training
>>> instead of firmware link training?
>>>
>>> Looks to me that we should only do software link training on these
>>> platforms that need it, so on those that define the phy-config
>>> property in their DT and use by default firmware link training.
>>
>> Sean and me have discussed about that, and we all agree to use sw training
>> instead the
>> fw training to keep training process consistent, and we do not need add a
>> varialbe in
>> struct rockchip_typec_phy(like use_sw_training before) to distinguish sw and
>> fw training.
>> If training process implement correctly, sw training not different with fw
>> training, and as my
>> test so far, sw training work well on Kevin, ofcourse, we need keep testing
>> it.
>>
>
> Ok, I can also confirm that software training works well on kevin.
> Could you mention this change of behavior in the commit message? I
> think that after these patches the firmware training is unlikely to
> happen.
>
> Thanks,
>  Enric
>
>
>>>> +       ret = cdn_dp_reg_write(dp, SOURCE_HDTX_CAR, 0xf);
>>>> +       if (ret) {
>>>> +               DRM_DEV_ERROR(dp->dev,
>>>> +               "Failed to write SOURCE_HDTX_CAR register %d\n", ret);
>>>> +               goto do_fw_training;
>>>> +       }
>>>> +       dp->use_fw_training = false;
>>>> +       return 0;
>>>> +
>>>> +do_fw_training:
>>>> +       dp->use_fw_training = true;
>>>> +       DRM_DEV_DEBUG_KMS(dp->dev, "use fw training\n");
>>>>          ret = cdn_dp_training_start(dp);
>>>>          if (ret) {
>>>>                  DRM_DEV_ERROR(dp->dev, "Failed to start training %d\n",
>>>> ret);
>>>> @@ -623,7 +650,7 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
>>>>
>>>>          DRM_DEV_DEBUG_KMS(dp->dev, "rate:0x%x, lanes:%d\n",
>>>> dp->link.rate,
>>>>                            dp->link.num_lanes);
>>>> -       return ret;
>>>> +       return 0;
>>>>   }
>>>>
>>>>   int cdn_dp_set_video_status(struct cdn_dp_device *dp, int active)
>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>> b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>> index 6580b11..3420771 100644
>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>> @@ -137,7 +137,7 @@
>>>>   #define HPD_EVENT_MASK                 0x211c
>>>>   #define HPD_EVENT_DET                  0x2120
>>>>
>>>> -/* dpyx framer addr */
>>>> +/* dptx framer addr */
>>>>   #define DP_FRAMER_GLOBAL_CONFIG                0x2200
>>>>   #define DP_SW_RESET                    0x2204
>>>>   #define DP_FRAMER_TU                   0x2208
>>>> @@ -431,6 +431,40 @@
>>>>   /* Reference cycles when using lane clock as reference */
>>>>   #define LANE_REF_CYC                           0x8000
>>>>
>>>> +/* register CM_VID_CTRL */
>>>> +#define LANE_VID_REF_CYC(x)                    (((x) & (BIT(24) - 1)) <<
>>>> 0)
>>>> +#define NMVID_MEAS_TOLERANCE(x)                        (((x) & 0xf) <<
>>>> 24)
>>>> +
>>>> +/* register DP_TX_PHY_CONFIG_REG */
>>>> +#define DP_TX_PHY_TRAINING_ENABLE(x)           ((x) & 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_PRBS7          (0 << 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS1           (1 << 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS2           (2 << 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS3           (3 << 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS4           (4 << 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_PLTPAT         (5 << 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_D10_2          (6 << 1)
>>>> +#define DP_TX_PHY_TRAINING_TYPE_HBR2CPAT       (8 << 1)
>>>> +#define DP_TX_PHY_TRAINING_PATTERN(x)          ((x) << 1)
>>>> +#define DP_TX_PHY_SCRAMBLER_BYPASS(x)          (((x) & 1) << 5)
>>>> +#define DP_TX_PHY_ENCODER_BYPASS(x)            (((x) & 1) << 6)
>>>> +#define DP_TX_PHY_SKEW_BYPASS(x)               (((x) & 1) << 7)
>>>> +#define DP_TX_PHY_DISPARITY_RST(x)             (((x) & 1) << 8)
>>>> +#define DP_TX_PHY_LANE0_SKEW(x)                (((x) & 7) << 9)
>>>> +#define DP_TX_PHY_LANE1_SKEW(x)                (((x) & 7) << 12)
>>>> +#define DP_TX_PHY_LANE2_SKEW(x)                (((x) & 7) << 15)
>>>> +#define DP_TX_PHY_LANE3_SKEW(x)                (((x) & 7) << 18)
>>>> +#define DP_TX_PHY_10BIT_ENABLE(x)              (((x) & 1) << 21)
>>>> +
>>>> +/* register DP_FRAMER_GLOBAL_CONFIG */
>>>> +#define NUM_LANES(x)           ((x) & 3)
>>>> +#define SST_MODE               (0 << 2)
>>>> +#define RG_EN                  (0 << 4)
>>>> +#define GLOBAL_EN              BIT(3)
>>>> +#define NO_VIDEO               BIT(5)
>>>> +#define ENC_RST_DIS            BIT(6)
>>>> +#define WR_VHSYNC_FALL         BIT(7)
>>>> +
>>>>   enum voltage_swing_level {
>>>>          VOLTAGE_LEVEL_0,
>>>>          VOLTAGE_LEVEL_1,
>>>> @@ -476,6 +510,7 @@ int cdn_dp_set_host_cap(struct cdn_dp_device *dp, u8
>>>> lanes, bool flip);
>>>>   int cdn_dp_event_config(struct cdn_dp_device *dp);
>>>>   u32 cdn_dp_get_event(struct cdn_dp_device *dp);
>>>>   int cdn_dp_get_hpd_status(struct cdn_dp_device *dp);
>>>> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val);
>>>>   ssize_t cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr,
>>>>                            u8 *data, u16 len);
>>>>   ssize_t cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr,
>>>> @@ -489,4 +524,5 @@ int cdn_dp_config_video(struct cdn_dp_device *dp);
>>>>   int cdn_dp_audio_stop(struct cdn_dp_device *dp, struct audio_info
>>>> *audio);
>>>>   int cdn_dp_audio_mute(struct cdn_dp_device *dp, bool enable);
>>>>   int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info
>>>> *audio);
>>>> +int cdn_dp_software_train_link(struct cdn_dp_device *dp);
>>>>   #endif /* _CDN_DP_REG_H */
>>>> --
>>>> 2.7.4
>>>>
>>>
>>>
>>
>>