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

[Yongbang Shi <shiyongbang@huawei.com>]

> On Sat, Feb 22, 2025 at 10:50:56AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.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@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.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.


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