On Tue, 5 Nov 2024 at 06:06, Yongbang Shi <shiyongbang@xxxxxxxxxx> wrote: > > > On Fri, Nov 01, 2024 at 06:50:27PM +0800, Yongbang Shi wrote: > >> From: baihan li <libaihan@xxxxxxxxxx> > >> > >> Build a dp level that hibmc driver can enable dp by > >> calling their functions. > >> > >> Signed-off-by: baihan li <libaihan@xxxxxxxxxx> > >> Signed-off-by: yongbang shi <shiyongbang@xxxxxxxxxx> > >> --- > >> ChangeLog: > >> v2 -> v3: > >> - fix build errors reported by kernel test robot <lkp@xxxxxxxxx> > >> Closes: https://lore.kernel.org/oe-kbuild-all/202410250931.UDQ9s66H-lkp@xxxxxxxxx/ > >> 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@xxxxxxxxx> > >> Closes: https://lore.kernel.org/oe-kbuild-all/202410040328.VeVxM9yB-lkp@xxxxxxxxx/ > >> v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@xxxxxxxxxx/ > >> --- > >> 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
