linux-kernel - Re: [PATCH v3 drm-dp 3/8] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d77bfuqor2rsd5tg4abywkckiqwy7j5xr43p73to2ofwpirws3@4m4g7hkvji2a>
Date: Sat, 22 Feb 2025 07:31:07 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Yongbang Shi <shiyongbang@...wei.com>
Cc: xinliang.liu@...aro.org, tiantao6@...ilicon.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, 
	daniel@...ll.ch, kong.kongxinwei@...ilicon.com, liangjian010@...wei.com, 
	chenjianmin@...wei.com, lidongming5@...wei.com, libaihan@...wei.com, 
	shenjian15@...wei.com, shaojijie@...wei.com, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 drm-dp 3/8] drm/hisilicon/hibmc: Add dp serdes cfg in
 dp process

On Sat, Feb 22, 2025 at 10:50:56AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@...wei.com>
> 
> Add dp serdes cfg in link training process, and related adapting
> and modificating. Change some init values about training,
> because we want completely to negotiation process, so we start with
> the maximum rate and the electrical characteristic level is 0.
> 
> Signed-off-by: Baihan Li <libaihan@...wei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@...wei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - change commit to an imperative sentence, suggested by Dmitry Baryshkov.
>   - put HIBMC_DP_HOST_SERDES_CTRL in dp_serdes.h, suggested by Dmitry Baryshkov.
> v1 -> v2:
>   - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
> ---
>  .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  5 ++-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
>  .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    |  6 ++++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
>  6 files changed, 43 insertions(+), 11 deletions(-)
> 

Mostly LGTM.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> index 812d0794543c..e0537cc9af41 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> @@ -4,12 +4,15 @@
>  #ifndef DP_SERDES_H
>  #define DP_SERDES_H
>  
> +#include "dp_comm.h"

No, please include it directly, where required. This simplifies possible
inter-header dependencies.

> +
>  #define HIBMC_DP_HOST_OFFSET		0x10000
>  #define HIBMC_DP_LANE0_RATE_OFFSET	0x4
>  #define HIBMC_DP_LANE1_RATE_OFFSET	0xc
>  #define HIBMC_DP_LANE_STATUS_OFFSET	0x10
>  #define HIBMC_DP_PMA_LANE0_OFFSET	0x18
>  #define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
> +#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
>  #define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
>  
>  /* dp serdes TX-Deempth Configuration */
> @@ -24,6 +27,9 @@
>  #define DP_SERDES_VOL2_PRE1		0x4500
>  #define DP_SERDES_VOL3_PRE0		0x600
>  #define DP_SERDES_BW_8_1		0x3
> +#define DP_SERDES_BW_5_4		0x2
> +#define DP_SERDES_BW_2_7		0x1
> +#define DP_SERDES_BW_1_62		0x0
>  
>  #define DP_SERDES_DONE			0x3
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index e6de6d5edf6b..67d39e258cac 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -28,9 +28,7 @@
>  #include "hibmc_drm_drv.h"
>  #include "hibmc_drm_regs.h"
>  
> -#define HIBMC_DP_HOST_SERDES_CTRL		0x1f001c
> -#define HIBMC_DP_HOST_SERDES_CTRL_VAL		0x8a00
> -#define HIBMC_DP_HOST_SERDES_CTRL_MASK		0x7ffff
> +#include "dp/dp_serdes.h"
>  
>  DEFINE_DRM_GEM_FOPS(hibmc_fops);
>  
> @@ -122,8 +120,8 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>  	}
>  
>  	/* if DP existed, init DP */
> -	if ((readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL) &
> -	     HIBMC_DP_HOST_SERDES_CTRL_MASK) == HIBMC_DP_HOST_SERDES_CTRL_VAL) {
> +	ret = readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL);
> +	if (ret) {

Why?

>  		ret = hibmc_dp_init(priv);
>  		if (ret)
>  			drm_err(dev, "failed to init dp: %d\n", ret);
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry