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: <scjsgypzxx7pkd255qf3t45nnqytp7q2urdvwo6zdffubawsa3@p6duklhgw3rv>
Date: Mon, 24 Feb 2025 04:15:54 +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 06:32:35PM +0800, Yongbang Shi wrote:
> > 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.
> 
> Okay.
> 
> 
> > > +
> > >   #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?
> 
> Becuase this is a serdes deempth configuration which is changed if we start cfg serdes.
> The HIBMC_DP_HOST_SERDES_CTRL_VAL is default value, but we may change it. We can just
> check if it is not zero. If it's not zero, it has dp block and we can init it.

=> commit message, please.

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

-- 
With best wishes
Dmitry