| 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: <bb603875-dc3f-4e3c-88eb-fdd9c7217383@huawei.com>
Date: Tue, 26 Nov 2024 09:38:00 +0800
From: Yongbang Shi <shiyongbang@...wei.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
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>,
<shiyongbang@...wei.com>
Subject: Re: [PATCH v5 drm-dp 1/5] drm/hisilicon/hibmc: add dp aux in hibmc
在 2024/11/22 9:42, Dmitry Baryshkov 写道:
> On Mon, Nov 18, 2024 at 10:28:01PM +0800, Yongbang Shi wrote:
>> From: baihan li <libaihan@...wei.com>
>>
>> Add dp aux read/write functions. They are basic functions
>> and will be used later.
>>
>> Signed-off-by: Baihan Li <libaihan@...wei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@...wei.com>
>> ---
>> ChangeLog:
>> v4 -> v5:
>> - fixing build errors reported by kernel test robot <lkp@...el.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202411131438.RZWYrWTE-lkp@intel.com/
>> v3 -> v4:
>> - retun error codes in result incorrect branch, suggested by Dmitry Baryshkov.
>> - replacing all ret= with returns, suggested by Dmitry Baryshkov.
>> - moving the comment below the judgment statement, suggested by Dmitry Baryshkov.
>> - moving definations to the source file and clearing headers, suggested by Dmitry Baryshkov.
>> - reanaming dp_prefix to hibmc_dp_prefix, suggested by Dmitry Baryshkov.
>> - changing hibmc_dp_reg_write_field to static inline and lock, suggested by Dmitry Baryshkov.
>> - moving some structs to later patch, suggested by Dmitry Baryshkov.
>> v2 -> v3:
>> - put the macro definations in latter patch where they are actually used, suggested by Dmitry Baryshkov.
>> - rename some macro definations to make them sensible, suggested by Dmitry Baryshkov.
>> - using FIELD_PREP and FIELD_GET, suggested by Dmitry Baryshkov.
>> - using DP_DPCD_REV_foo, suggested by Dmitry Baryshkov.
>> - fix build errors reported by kernel test robot <lkp@...el.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202410250305.UHKDhtxy-lkp@intel.com/
>> v1 -> v2:
>> - using drm_dp_aux frame implement dp aux read and write functions, suggested by Jani Nikula.
>> - using drm dp header files' dp macros instead, suggested by Andy Yan.
>> v1:https://lore.kernel.org/all/20240930100610.782363-1-shiyongbang@huawei.com/
>> ---
>> drivers/gpu/drm/hisilicon/hibmc/Makefile | 3 +-
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c | 164 +++++++++++++++++++
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h | 38 +++++
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h | 27 +++
>> 4 files changed, 231 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
>> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index d25c75e60d3d..8770ec6dfffd 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,4 +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
>> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>> + dp/dp_aux.o
>>
>> obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
>> new file mode 100644
>> index 000000000000..16bdfefbf255
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
>> @@ -0,0 +1,164 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-laterHIBMC_BYTES_IN_U32
>> +// Copyright (c) 2024 Hisilicon Limited.
>> +
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/minmax.h>
>> +#include <drm/drm_device.h>
>> +#include <drm/drm_print.h>
>> +#include "dp_comm.h"
>> +#include "dp_reg.h"
>> +
>> +#define HIBMC_AUX_CMD_REQ_LEN GENMASK(7, 4)
>> +#define HIBMC_AUX_CMD_ADDR GENMASK(27, 8)
>> +#define HIBMC_AUX_CMD_I2C_ADDR_ONLY BIT(28)
>> +#define HIBMC_BYTES_IN_U32 4
>> +#define HIBMC_AUX_I2C_WRITE_SUCCESS 0x1
>> +#define HIBMC_DP_MIN_PULSE_NUM 0x9
>> +#define BITS_IN_U8 8
>> +
>> +static inline void hibmc_dp_aux_reset(struct hibmc_dp_dev *dp)
>> +{
>> + hibmc_dp_reg_write_field(dp, HIBMC_DP_DPTX_RST_CTRL, HIBMC_DP_CFG_AUX_RST_N, 0x0);
>> + usleep_range(10, 15);
>> + hibmc_dp_reg_write_field(dp, HIBMC_DP_DPTX_RST_CTRL, HIBMC_DP_CFG_AUX_RST_N, 0x1);
>> +}
>> +
>> +static void hibmc_dp_aux_read_data(struct hibmc_dp_dev *dp, u8 *buf, u8 size)
>> +{
>> + u32 reg_num;
>> + u32 value;
>> + u32 num;
>> + u8 i, j;
>> +
>> + reg_num = DIV_ROUND_UP(size, HIBMC_BYTES_IN_U32);
>> + for (i = 0; i < reg_num; i++) {
>> + /* number of bytes read from a single register */
>> + num = min(size - i * HIBMC_BYTES_IN_U32, HIBMC_BYTES_IN_U32);
>> + value = readl(dp->base + HIBMC_DP_AUX_RD_DATA0 + i * HIBMC_BYTES_IN_U32);
>> + /* convert the 32-bit value of the register to the buffer. */
>> + for (j = 0; j < num; j++)
>> + buf[i * HIBMC_BYTES_IN_U32 + j] = value >> (j * BITS_IN_U8);
>> + }
>> +}
>> +
>> +static void hibmc_dp_aux_write_data(struct hibmc_dp_dev *dp, u8 *buf, u8 size)
>> +{
>> + u32 reg_num;
>> + u32 value;
>> + u32 num;
>> + u8 i, j;
>> +
>> + reg_num = DIV_ROUND_UP(size, HIBMC_BYTES_IN_U32);
>> + for (i = 0; i < reg_num; i++) {
>> + /* number of bytes written to a single register */
>> + num = min_t(u8, size - i * HIBMC_BYTES_IN_U32, HIBMC_BYTES_IN_U32);
>> + value = 0;
>> + /* obtain the 32-bit value written to a single register. */
>> + for (j = 0; j < num; j++)
>> + value |= buf[i * HIBMC_BYTES_IN_U32 + j] << (j * BITS_IN_U8);
>> + /* writing data to a single register */
>> + writel(value, dp->base + HIBMC_DP_AUX_WR_DATA0 + i * HIBMC_BYTES_IN_U32);
>> + }
>> +}
>> +
>> +static u32 hibmc_dp_aux_build_cmd(const struct drm_dp_aux_msg *msg)
>> +{
>> + u32 aux_cmd = msg->request;
>> +
>> + if (msg->size)
>> + aux_cmd |= FIELD_PREP(HIBMC_AUX_CMD_REQ_LEN, (msg->size - 1));
>> + else
>> + aux_cmd |= FIELD_PREP(HIBMC_AUX_CMD_I2C_ADDR_ONLY, 1);
>> +
>> + aux_cmd |= FIELD_PREP(HIBMC_AUX_CMD_ADDR, msg->address);
>> +
>> + return aux_cmd;
>> +}
>> +
>> +/* ret >= 0, ret is size; ret < 0, ret is err code */
>> +static int hibmc_dp_aux_parse_xfer(struct hibmc_dp_dev *dp, struct drm_dp_aux_msg *msg)
>> +{
>> + u32 buf_data_cnt;
>> + u32 aux_status;
>> +
>> + aux_status = readl(dp->base + HIBMC_DP_AUX_STATUS);
>> + msg->reply = FIELD_GET(HIBMC_DP_CFG_AUX_STATUS, aux_status);
>> +
>> + if (aux_status & HIBMC_DP_CFG_AUX_TIMEOUT)
>> + return -ETIMEDOUT;
>> +
>> + /* only address */
>> + if (!msg->size)
>> + return 0;
>> +
>> + if (msg->reply != DP_AUX_NATIVE_REPLY_ACK)
>> + return -EIO;
>> +
>> + buf_data_cnt = FIELD_GET(HIBMC_DP_CFG_AUX_READY_DATA_BYTE, aux_status);
>> +
>> + switch (msg->request) {
>> + case DP_AUX_NATIVE_WRITE:
>> + return msg->size;
>> + case DP_AUX_I2C_WRITE | DP_AUX_I2C_MOT:
>> + if (buf_data_cnt == HIBMC_AUX_I2C_WRITE_SUCCESS)
>> + return msg->size;
>> + else
>> + return FIELD_GET(HIBMC_DP_CFG_AUX, aux_status);
>> + case DP_AUX_NATIVE_READ:
>> + case DP_AUX_I2C_READ | DP_AUX_I2C_MOT:
>> + buf_data_cnt--;
>> + if (buf_data_cnt != msg->size) {
>> + /* only the successful part of data is read */
>> + return -EBUSY;
>> + }
>> +
>> + /* all data is successfully read */
>> + hibmc_dp_aux_read_data(dp, msg->buffer, msg->size);
>> + return msg->size;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +/* ret >= 0 ,ret is size; ret < 0, ret is err code */
>> +static ssize_t hibmc_dp_aux_xfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> +{
>> + struct hibmc_dp_dev *dp = container_of(aux, struct hibmc_dp_dev, aux);
>> + u32 aux_cmd;
>> + int ret;
>> + u32 val; /* val will be assigned at the beginning of readl_poll_timeout function */
>> +
>> + writel(0, dp->base + HIBMC_DP_AUX_WR_DATA0);
>> + writel(0, dp->base + HIBMC_DP_AUX_WR_DATA1);
>> + writel(0, dp->base + HIBMC_DP_AUX_WR_DATA2);
>> + writel(0, dp->base + HIBMC_DP_AUX_WR_DATA3);
>> +
>> + hibmc_dp_aux_write_data(dp, msg->buffer, msg->size);
>> +
>> + aux_cmd = hibmc_dp_aux_build_cmd(msg);
>> + writel(aux_cmd, dp->base + HIBMC_DP_AUX_CMD_ADDR);
>> +
>> + /* enable aux transfer */
>> + hibmc_dp_reg_write_field(dp, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_REQ, 0x1);
>> + ret = readl_poll_timeout(dp->base + HIBMC_DP_AUX_REQ, val,
>> + !(val & HIBMC_DP_CFG_AUX_REQ), 50, 5000);
>> + if (ret) {
>> + hibmc_dp_aux_reset(dp);
>> + return ret;
>> + }
>> +
>> + return hibmc_dp_aux_parse_xfer(dp, msg);
>> +}
>> +
>> +void hibmc_dp_aux_init(struct hibmc_dp_dev *dp)
>> +{
>> + hibmc_dp_reg_write_field(dp, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> + hibmc_dp_reg_write_field(dp, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> + hibmc_dp_reg_write_field(dp, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM,
>> + HIBMC_DP_MIN_PULSE_NUM);
>> +
>> + dp->aux.transfer = hibmc_dp_aux_xfer;
>> + dp->aux.is_remote = 0;
>> + drm_dp_aux_init(&dp->aux);
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> new file mode 100644
>> index 000000000000..ce3b6fa4ea9e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2024 Hisilicon Limited. */
>> +
>> +#ifndef DP_COMM_H
>> +#define DP_COMM_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/bitops.h>
>> +#include <linux/errno.h>
>> +#include <linux/mutex.h>
>> +#include <linux/kernel.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/io.h>
>> +#include <drm/display/drm_dp_helper.h>
>> +
>> +struct hibmc_dp_dev {
>> + struct drm_dp_aux aux;
>> + struct drm_device *dev;
>> + void __iomem *base;
>> + struct mutex lock; /* protects concurrent RW in hibmc_dp_reg_write_field() */
>> +};
>> +
>> +#define dp_field_modify(reg_value, mask, val) ({ \
>> + (reg_value) &= ~(mask); \
>> + (reg_value) |= FIELD_PREP(mask, val); })
> do { ... } while (0) or static inline. Or just inline into the calling
> function, if there is just one place where it is used.
>
>> +
>> +#define hibmc_dp_reg_write_field(dp, offset, mask, val) ({ \
>> + typeof(dp) _dp = dp; \
>> + typeof(_dp->base) addr = (_dp->base + (offset)); \
>> + mutex_lock(&_dp->lock); \
>> + u32 reg_value = readl(addr); \
>> + dp_field_modify(reg_value, mask, val); \
>> + writel(reg_value, addr); \
>> + mutex_unlock(&_dp->lock); })
> I'd prefer a static inline function. Other than that:
Dear Dmitry,
Thanks for your all advice and your very carefully reviewing! I will take your all other suggestion.
For this function, it will be used in multiple files. If we static inline it in header, there
will be kernel test robot's build error becuase FIELD_PREP()'s parameter is a variable.
Here's the error's link: https://lore.kernel.org/oe-kbuild-all/202411131438.RZWYrWTE-lkp@intel.com/
Thanks,
Baihan
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>
>
>> +
>> +void hibmc_dp_aux_init(struct hibmc_dp_dev *dp);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> new file mode 100644
>> index 000000000000..f3e6781e111a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2024 Hisilicon Limited. */
>> +
>> +#ifndef DP_REG_H
>> +#define DP_REG_H
>> +
>> +#define HIBMC_DP_AUX_CMD_ADDR 0x50
>> +#define HIBMC_DP_AUX_WR_DATA0 0x54
>> +#define HIBMC_DP_AUX_WR_DATA1 0x58
>> +#define HIBMC_DP_AUX_WR_DATA2 0x5c
>> +#define HIBMC_DP_AUX_WR_DATA3 0x60
>> +#define HIBMC_DP_AUX_RD_DATA0 0x64
>> +#define HIBMC_DP_AUX_REQ 0x74
>> +#define HIBMC_DP_AUX_STATUS 0x78
>> +#define HIBMC_DP_DPTX_RST_CTRL 0x700
>> +
>> +#define HIBMC_DP_CFG_AUX_SYNC_LEN_SEL BIT(1)
>> +#define HIBMC_DP_CFG_AUX_TIMER_TIMEOUT BIT(2)
>> +#define HIBMC_DP_CFG_AUX_MIN_PULSE_NUM GENMASK(13, 9)
>> +#define HIBMC_DP_CFG_AUX_REQ BIT(0)
>> +#define HIBMC_DP_CFG_AUX_RST_N BIT(4)
>> +#define HIBMC_DP_CFG_AUX_TIMEOUT BIT(0)
>> +#define HIBMC_DP_CFG_AUX_READY_DATA_BYTE GENMASK(16, 12)
>> +#define HIBMC_DP_CFG_AUX GENMASK(24, 17)
>> +#define HIBMC_DP_CFG_AUX_STATUS GENMASK(11, 4)
>> +
>> +#endif
>> --
>> 2.33.0
>>
Powered by blists - more mailing lists