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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Signed-off-by: Yongbang Shi <shiyongbang@xxxxxxxxxx>
> > > ---
> > > 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
