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

2018-05-11 Thread Sean Paul 
On Wed, May 09, 2018 at 06:22:44PM +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 
> Signed-off-by: Lin Huang 

FTR, I've previously reviewed this patch at
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/985573

> ---
> Changes in v2:
> - update patch following Enric suggest
> 
>  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 | 391 
> 
>  drivers/gpu/drm/rockchip/cdn-dp-reg.c   |  34 ++-
>  drivers/gpu/drm/rockchip/cdn-dp-reg.h   |  38 ++-
>  6 files changed, 479 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/gpu/drm/rockchip/cdn-dp-link-training.c
> 

/snip

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h 
> b/drivers/gpu/drm/rockchip/cdn-dp-core.h
> index 46159b2..c6050ab 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 sw_training_success;

Can you change this to use_fw_training instead? So it will be false if sw
training succeeds, and true if sw training fails and needs to fallback to fw.

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

/snip

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c 
> b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> index afdfda0..bd0aed5 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> @@ -17,7 +17,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  
>  #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];
>  
> @@ -606,7 +608,35 @@ static int cdn_dp_get_training_status(struct 
> cdn_dp_device *dp)
>  int cdn_dp_train_link(struct cdn_dp_device *dp)
>  {
>   int ret;
> + struct cdn_dp_port *port = dp->port[dp->active_port];
> + struct rockchip_typec_phy *tcphy = phy_get_drvdata(port->phy);
>  
> + /*
> +  * 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.
> +  */
> + if (tcphy->need_software_training) {

As discussed in the downstream review, I'd rather default to sw training and
fallback to fw training if we must.

> + 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->sw_training_success = true;
> + return 0;
> + }
> +
> +do_fw_training:
> + dp->sw_training_success = false;

Can you please add a DRM_DEBUG_KMS log message here?

>   ret = cdn_dp_training_start(dp);
>   if (ret) {
>   DRM_DEV_ERROR(dp->dev, "Failed to start training %d\n", ret);
> @@ -621,7 +651,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
> ---

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

2018-05-11 Thread Sean Paul 
On Wed, May 09, 2018 at 06:22:44PM +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 
> Signed-off-by: Lin Huang 

FTR, I've previously reviewed this patch at
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/985573

> ---
> Changes in v2:
> - update patch following Enric suggest
> 
>  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 | 391 
> 
>  drivers/gpu/drm/rockchip/cdn-dp-reg.c   |  34 ++-
>  drivers/gpu/drm/rockchip/cdn-dp-reg.h   |  38 ++-
>  6 files changed, 479 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/gpu/drm/rockchip/cdn-dp-link-training.c
> 

/snip

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h 
> b/drivers/gpu/drm/rockchip/cdn-dp-core.h
> index 46159b2..c6050ab 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 sw_training_success;

Can you change this to use_fw_training instead? So it will be false if sw
training succeeds, and true if sw training fails and needs to fallback to fw.

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

/snip

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c 
> b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> index afdfda0..bd0aed5 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> @@ -17,7 +17,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  
>  #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];
>  
> @@ -606,7 +608,35 @@ static int cdn_dp_get_training_status(struct 
> cdn_dp_device *dp)
>  int cdn_dp_train_link(struct cdn_dp_device *dp)
>  {
>   int ret;
> + struct cdn_dp_port *port = dp->port[dp->active_port];
> + struct rockchip_typec_phy *tcphy = phy_get_drvdata(port->phy);
>  
> + /*
> +  * 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.
> +  */
> + if (tcphy->need_software_training) {

As discussed in the downstream review, I'd rather default to sw training and
fallback to fw training if we must.

> + 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->sw_training_success = true;
> + return 0;
> + }
> +
> +do_fw_training:
> + dp->sw_training_success = false;

Can you please add a DRM_DEBUG_KMS log message here?

>   ret = cdn_dp_training_start(dp);
>   if (ret) {
>   DRM_DEV_ERROR(dp->dev, "Failed to start training %d\n", ret);
> @@ -621,7 +651,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
> +++

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

2018-05-09 Thread Lin Huang 
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 
Signed-off-by: Lin Huang 
---
Changes in v2:
- update patch following Enric suggest

 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 | 391 
 drivers/gpu/drm/rockchip/cdn-dp-reg.c   |  34 ++-
 drivers/gpu/drm/rockchip/cdn-dp-reg.h   |  38 ++-
 6 files changed, 479 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..2359d82 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->sw_training_success == false) {
+   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->sw_training_success == false) {
+   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(>lock);
 }
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h 
b/drivers/gpu/drm/rockchip/cdn-dp-core.h
index 46159b2..c6050ab 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 sw_training_success;
 
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 000..25282de
--- /dev/null
+++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
@@ -0,0 +1,391 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
+ * Author: Chris Zhong 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#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)
+

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

2018-05-09 Thread Lin Huang 
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 
Signed-off-by: Lin Huang 
---
Changes in v2:
- update patch following Enric suggest

 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 | 391 
 drivers/gpu/drm/rockchip/cdn-dp-reg.c   |  34 ++-
 drivers/gpu/drm/rockchip/cdn-dp-reg.h   |  38 ++-
 6 files changed, 479 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..2359d82 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->sw_training_success == false) {
+   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->sw_training_success == false) {
+   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(>lock);
 }
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h 
b/drivers/gpu/drm/rockchip/cdn-dp-core.h
index 46159b2..c6050ab 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 sw_training_success;
 
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 000..25282de
--- /dev/null
+++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
@@ -0,0 +1,391 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
+ * Author: Chris Zhong 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#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;
+
+