[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA8EJpoU9Pq4ZpvXj1hzpAgm+Vb002Q=AdTKo2ix4dcAaHNe4Q@mail.gmail.com>
Date: Tue, 5 Nov 2024 10:50:14 +0000
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/4] drm/hisilicon/hibmc: add dp hw moduel in hibmc
On Tue, 5 Nov 2024 at 06:06, Yongbang Shi <shiyongbang@...wei.com> wrote:
>
> > On Fri, Nov 01, 2024 at 06:50:27PM +0800, Yongbang Shi wrote:
> >> From: baihan li <libaihan@...wei.com>
> >>
> >> Build a dp level that hibmc driver can enable dp by
> >> calling their functions.
> >>
> >> Signed-off-by: baihan li <libaihan@...wei.com>
> >> Signed-off-by: yongbang shi <shiyongbang@...wei.com>
> >> ---
> >> ChangeLog:
> >> v2 -> v3:
> >> - fix build errors reported by kernel test robot <lkp@...el.com>
> >> Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@intel.com/
> >> v1 -> v2:
> >> - changed some defines and functions to former patch, suggested by Dmitry Baryshkov.
> >> - sorting the headers including in dp_hw.h and hibmc_drm_drv.c files, suggested by Dmitry Baryshkov.
> >> - deleting struct dp_mode and dp_mode_cfg function, suggested by Dmitry Baryshkov.
> >> - fix build errors reported by kernel test robot <lkp@...el.com>
> >> Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@intel.com/
> >> v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
> >> ---
> >> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +-
> >> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 237 ++++++++++++++++++++
> >> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h | 31 +++
> >> drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h | 41 ++++
> >> 4 files changed, 310 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> >> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> >>
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> >> index 94d77da88bbf..214228052ccf 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_aux.o dp/dp_link.o dp/dp_hw.o
> >>
> >> obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> >> new file mode 100644
> >> index 000000000000..214897798bdb
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> >> @@ -0,0 +1,237 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +// Copyright (c) 2024 Hisilicon Limited.
> >> +
> >> +#include <linux/io.h>
> >> +#include <linux/delay.h>
> >> +#include "dp_config.h"
> >> +#include "dp_comm.h"
> >> +#include "dp_reg.h"
> >> +#include "dp_hw.h"
> >> +#include "dp_link.h"
> >> +#include "dp_aux.h"
> >> +
> >> +static int hibmc_dp_link_init(struct dp_dev *dp)
> >> +{
> >> + dp->link.cap.lanes = 2;
> >> + dp->link.train_set = devm_kzalloc(dp->dev->dev,
> >> + dp->link.cap.lanes * sizeof(u8), GFP_KERNEL);
> > Can you replace it just with an array, removing a need for an additional
> > allocation?
> >
> >> + if (!dp->link.train_set)
> >> + return -ENOMEM;
> >> +
> >> + dp->link.cap.link_rate = 1;
> > Ok, this is why I don't link using indices for link rates. Which rate is
> > this? Unlike cap.lanes this is pure magic number. I think it should be
> > handled other way around: store actual link rate and convert to the
> > register value when required.
> >
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void hibmc_dp_set_tu(struct dp_dev *dp, struct drm_display_mode *mode)
> >> +{
> >> + u32 tu_symbol_frac_size;
> >> + u32 tu_symbol_size;
> >> + u32 rate_ks;
> >> + u8 lane_num;
> >> + u32 value;
> >> + u32 bpp;
> >> +
> >> + lane_num = dp->link.cap.lanes;
> >> + if (lane_num == 0) {
> >> + drm_err(dp->dev, "set tu failed, lane num cannot be 0!\n");
> >> + return;
> >> + }
> >> +
> >> + bpp = DP_BPP;
> > Where is this defined? Is it hibmc-specific or a generic value?
> >
> >> + rate_ks = hibmc_dp_get_link_rate(dp->link.cap.link_rate) * DP_LINK_RATE_CAL;
> > same question
>
> Hi Dmitry,
> Thanks for your detailed suggestions and questions. These two are defined in dp_config.h.
Please move defines to the corresponding patch, when the values are
being used. Also if these defines are HIBMC-specific, please use the
corresponding prefix (when one sees DP_foo they expect a constant
defined in the standard, not a driver-specific value).
--
With best wishes
Dmitry
Powered by blists - more mailing lists