| 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
| ||
|
Message-ID: <dmq3iyxffg4jo7eup37zk4uqkqjtbn7wj2jmyl6ww5clksmgvq@jdslhlsbxrdr>
Date: Sat, 22 Feb 2025 07:25:46 +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 2/8] drm/hisilicon/hibmc: Add dp serdes cfg to
adjust serdes rate, voltage and pre-emphasis
On Sat, Feb 22, 2025 at 10:50:55AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@...wei.com>
>
> This dp controller need features of digital-to-analog conversion and
> high-speed transmission in chip by its extern serdes controller. Our
> serdes cfg is relatively simple, just need two register configurations.
> Don't need too much functions, like: power on/off, initialize, and some
> complex configurations, so I'm not going to use the phy framework.
> This serdes is inited and configured in dp initialization, and also
> integrating them into link training process.
>
> For rate changing, we can change from 1.62-8.2Gpbs by cfg reg.
> For voltage and pre-emphasis levels changing, we can cfg different
> serdes ffe value.
>
> Signed-off-by: Baihan Li <libaihan@...wei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@...wei.com>
> ---
> ChangeLog:
> v2 - > v3:
> - add commit log about dp serdes, suggested by Dmitry Baryshkov.
> - return value in hibmc_dp_serdes_init(), suggested by Dmitry Baryshkov.
> - add static const in the array of serdes_tx_cfg[], suggested by Dmitry Baryshkov.
> - change drm_warn to drm_dbg_dp, suggested by Dmitry Baryshkov.
> - add explanations about dp serdes macros, suggested by Dmitry Baryshkov.
> v1 -> v2:
> - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
> - changing all names of dp phy to dp serdes.
> ---
> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +-
> drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h | 1 +
> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 6 ++
> .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c | 72 +++++++++++++++++++
> .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h | 34 +++++++++
> 5 files changed, 114 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index 95a4ed599d98..43de077d6769 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0-only
> hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> - dp/dp_aux.o dp/dp_link.o dp/dp_hw.o hibmc_drm_dp.o
> + dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
>
> obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> index 2c52a4476c4d..e7746bc4b592 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> @@ -38,6 +38,7 @@ struct hibmc_dp_dev {
> struct mutex lock; /* protects concurrent RW in hibmc_dp_reg_write_field() */
> struct hibmc_dp_link link;
> u8 dpcd[DP_RECEIVER_CAP_SIZE];
> + void __iomem *serdes_base;
> };
>
> #define dp_field_modify(reg_value, mask, val) \
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index a8d543881c09..59078483ec55 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -7,6 +7,7 @@
> #include "dp_comm.h"
> #include "dp_reg.h"
> #include "dp_hw.h"
> +#include "dp_serdes.h"
>
> static void hibmc_dp_set_tu(struct hibmc_dp_dev *dp, struct drm_display_mode *mode)
> {
> @@ -151,6 +152,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
> {
> struct drm_device *drm_dev = dp->drm_dev;
> struct hibmc_dp_dev *dp_dev;
> + int ret;
>
> dp_dev = devm_kzalloc(drm_dev->dev, sizeof(struct hibmc_dp_dev), GFP_KERNEL);
> if (!dp_dev)
> @@ -165,6 +167,10 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>
> hibmc_dp_aux_init(dp_dev);
>
> + ret = hibmc_dp_serdes_init(dp_dev);
> + if (ret)
> + return ret;
> +
> dp_dev->link.cap.lanes = 0x2;
> dp_dev->link.cap.link_rate = DP_LINK_BW_2_7;
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> new file mode 100644
> index 000000000000..241b9ef782b0
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2025 Hisilicon Limited.
> +
> +#include <linux/delay.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_print.h>
> +#include "dp_comm.h"
> +#include "dp_config.h"
> +#include "dp_reg.h"
> +#include "dp_serdes.h"
> +
> +int hibmc_dp_serdes_set_tx_cfg(struct hibmc_dp_dev *dp, u8 train_set[HIBMC_DP_LANE_NUM_MAX])
> +{
> + static const u32 serdes_tx_cfg[4][4] = { {DP_SERDES_VOL0_PRE0, DP_SERDES_VOL0_PRE1,
> + DP_SERDES_VOL0_PRE2, DP_SERDES_VOL0_PRE3},
> + {DP_SERDES_VOL1_PRE0, DP_SERDES_VOL1_PRE1,
> + DP_SERDES_VOL1_PRE2}, {DP_SERDES_VOL2_PRE0,
> + DP_SERDES_VOL2_PRE1}, {DP_SERDES_VOL3_PRE0}};
> + int cfg[2];
> + int i;
> +
> + for (i = 0; i < HIBMC_DP_LANE_NUM_MAX; i++) {
> + cfg[i] = serdes_tx_cfg[(train_set[i] & 0x3)]
> + [(train_set[i] << DP_TRAIN_PRE_EMPHASIS_SHIFT & 0x3)];
I think this will not work as expected. There should be no need to shift
train_set[i], otherwise first '&' is executed, reducing array index to
(train_set[i]) << 3), which then is out-of-boundaries for the array.
Most likely you meant:
cfg[i] = serdes_tx_cfg[FIELD_GET(DP_TRAIN_VOLTAGE_SWING_MASK, train_set[i]]
[FIELD_GET(DP_TRAIN_PRE_EMPHASIS_MASK, train_set[i]];
> + if (!cfg[i])
> + return -EINVAL;
> +
> + /* lane1 offset is 4 */
> + writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, cfg[i]),
> + dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET + i * 4);
> + }
> +
> + usleep_range(300, 500);
> +
> + if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
> + drm_dbg_dp(dp->dev, "dp serdes cfg failed\n");
> + return -EAGAIN;
> + }
> +
> + return 0;
> +}
> +
> +int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
> +{
> + writel(rate, dp->serdes_base + HIBMC_DP_LANE0_RATE_OFFSET);
> + writel(rate, dp->serdes_base + HIBMC_DP_LANE1_RATE_OFFSET);
> +
> + usleep_range(300, 500);
> +
> + if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
> + drm_dbg_dp(dp->dev, "dp serdes rate switching failed\n");
> + return -EAGAIN;
> + }
> +
> + if (rate < DP_SERDES_BW_8_1)
> + drm_dbg_dp(dp->dev, "reducing serdes rate to :%d\n",
> + rate ? rate * HIBMC_DP_LINK_RATE_CAL * 10 : 162);
> +
> + return 0;
> +}
> +
> +int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp)
> +{
> + dp->serdes_base = dp->base + HIBMC_DP_HOST_OFFSET;
> +
> + writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
> + dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET);
> + writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
> + dp->serdes_base + HIBMC_DP_PMA_LANE1_OFFSET);
> +
> + return hibmc_dp_serdes_rate_switch(DP_SERDES_BW_8_1, dp);
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> new file mode 100644
> index 000000000000..812d0794543c
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2025 Hisilicon Limited. */
> +
> +#ifndef DP_SERDES_H
> +#define DP_SERDES_H
> +
Why all these values are not a part of the dp_reg.h?
> +#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_PMA_TXDEEMPH GENMASK(18, 1)
> +
> +/* dp serdes TX-Deempth Configuration */
> +#define DP_SERDES_VOL0_PRE0 0x280
> +#define DP_SERDES_VOL0_PRE1 0x2300
> +#define DP_SERDES_VOL0_PRE2 0x53c0
> +#define DP_SERDES_VOL0_PRE3 0x8400
> +#define DP_SERDES_VOL1_PRE0 0x380
> +#define DP_SERDES_VOL1_PRE1 0x3440
> +#define DP_SERDES_VOL1_PRE2 0x6480
> +#define DP_SERDES_VOL2_PRE0 0x500
> +#define DP_SERDES_VOL2_PRE1 0x4500
> +#define DP_SERDES_VOL3_PRE0 0x600
> +#define DP_SERDES_BW_8_1 0x3
> +
> +#define DP_SERDES_DONE 0x3
> +
> +int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp);
> +int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp);
> +int hibmc_dp_serdes_set_tx_cfg(struct hibmc_dp_dev *dp, u8 train_set[HIBMC_DP_LANE_NUM_MAX]);
> +
> +#endif
> --
> 2.33.0
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists