Re: [PATCH v3 4/4] drm/rockchip: support dp training outside dp firmware

From: Sean Paul
Date: Mon May 14 2018 - 11:18:44 EST

Next message: Christian Hansen (chansen3): "Re: [PATCH] arm64: allowing mmap to be traced"
Previous message: Jiri Pirko: "Re: [PATCH 04/14] net: sched: implement unlocked action init API"
In reply to: Lin Huang: "[PATCH v3 4/4] drm/rockchip: support dp training outside dp firmware"
Next in thread: Enric Balletbo Serra: "Re: [PATCH v3 1/4] drm/rockchip: add transfer function for cdn-dp"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, May 14, 2018 at 05:53:55PM +0800, Lin Huang wrote:
> 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.
>
> Signed-off-by: Chris Zhong <zyw@xxxxxxxxxxxxxx>
> Signed-off-by: Lin Huang <hl@xxxxxxxxxxxxxx>
> ---
> 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
>
> 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 | 416 ++++++++++++++++++++++++
> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 31 +-
> drivers/gpu/drm/rockchip/cdn-dp-reg.h | 38 ++-
> 6 files changed, 501 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) {
> + 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) {
> + 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..b8fd5bc
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c

/snip

> +static int cdn_dp_get_lower_link_rate(struct cdn_dp_device *dp)
> +{
> + if (dp->link.rate == DP_LINK_BW_1_62)
> + return -EINVAL;
> + else if (dp->link.rate == DP_LINK_BW_2_7)
> + dp->link.rate = DP_LINK_BW_1_62;

Extra indent

> + else
> + dp->link.rate = DP_LINK_BW_2_7;

This is better expressed as a switch statement:

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;
default:
dp->link.rate = DP_LINK_BW_2_7:
break;
}

You might also consider adding an additional case since there are rates higher
than 5.4GHz. ie:
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);
> +
> +retry:
> + /* 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);
> +
> + 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);
> +
> + ret = cdn_dp_link_training_clock_recovery(dp);
> + if (ret) {
> + if (cdn_dp_get_lower_link_rate(dp)) {
> + DRM_ERROR("training clock recovery fail, err: %d\n",
> + ret);
> + goto stop_training;
> + }
> +
> + /* use lower link rate to retraining */
> + goto retry;
> + }
> +
> + ret = cdn_dp_link_training_channel_equalization(dp);
> + if (ret) {
> + if (cdn_dp_get_lower_link_rate(dp)) {
> + DRM_ERROR("training channel equalization fail, err: %d\n",
> + ret);
> + goto stop_training;
> + }
> +
> + /* use lower link rate to retraining */
> + goto retry;
> + }
> +
> +stop_training:
> + 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;

Using labels to do loops reduces readability, it seems like you can also pull
out the downspread control write.

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

return 0;
}

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;

> +
> +stop_training:
> + 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

This comment is no longer accurate.

> + * 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;
> + }
> + 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)
>

/snip

--
Sean Paul, Software Engineer, Google / Chromium OS

Next message: Christian Hansen (chansen3): "Re: [PATCH] arm64: allowing mmap to be traced"
Previous message: Jiri Pirko: "Re: [PATCH 04/14] net: sched: implement unlocked action init API"
In reply to: Lin Huang: "[PATCH v3 4/4] drm/rockchip: support dp training outside dp firmware"
Next in thread: Enric Balletbo Serra: "Re: [PATCH v3 1/4] drm/rockchip: add transfer function for cdn-dp"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]