On Tue, Oct 29, 2024 at 02:02:09PM +0800, Sandor Yu wrote:
> Mailbox access functions in MHDP8546 will be share to other MHDP driver
> and Cadence HDP-TX HDMI/DP PHY drivers.
>
> Create a new MHDP helper driver and move all mailbox access functions into.
> According the mailbox access sequence and type of security.
> Six mailbox access API functions are introduced.
> Three APIs for non-secure mailbox access:
> - cdns_mhdp_mailbox_send()
> - cdns_mhdp_mailbox_send_recv()
> - cdns_mhdp_mailbox_send_recv_multi()
> The other three APIs for secure mailbox access:
> - cdns_mhdp_secure_mailbox_send()
> - cdns_mhdp_secure_mailbox_send_recv()
> - cdns_mhdp_secure_mailbox_send_recv_multi()
>
> All MHDP commands that need to be passed through the mailbox
> have been rewritten using those new API functions.
>
> The register read/write and DP DPCD read/write command functions
> are also included in this new helper driver.
>
> Function cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(),
> because it use the DPTX command ID DPTX_WRITE_REGISTER.
> New cdns_mhdp_reg_write() is added with the general command ID
> GENERAL_REGISTER_WRITE.
>
> Signed-off-by: Sandor Yu <Sandor.yu@xxxxxxx>
> ---
> v17->v18:
> - Create three ordinary mailbox access APIs
> cdns_mhdp_mailbox_send
> cdns_mhdp_mailbox_send_recv
> cdns_mhdp_mailbox_send_recv_multi
> - Create three secure mailbox access APIs
> cdns_mhdp_secure_mailbox_send
> cdns_mhdp_secure_mailbox_send_recv
> cdns_mhdp_secure_mailbox_send_recv_multi
> - MHDP8546 DP and HDCP commands that need access mailbox are rewrited
> with above 6 API functions.
>
> v16->v17:
> - Replaces the local mutex mbox_mutex with a global mutex mhdp_mailbox_mutex
>
> v12->v16:
> *No change.
>
> drivers/gpu/drm/bridge/cadence/Kconfig | 4 +
> drivers/gpu/drm/bridge/cadence/Makefile | 1 +
> .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 414 +++++++++++++++
> .../drm/bridge/cadence/cdns-mhdp8546-core.c | 487 +++---------------
> .../drm/bridge/cadence/cdns-mhdp8546-core.h | 47 +-
> .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 236 +--------
> .../drm/bridge/cadence/cdns-mhdp8546-hdcp.h | 18 +-
> include/drm/bridge/cdns-mhdp-helper.h | 130 +++++
> 8 files changed, 658 insertions(+), 679 deletions(-)
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
>
> diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig b/drivers/gpu/drm/bridge/cadence/Kconfig
> index cced81633ddcd..e0973339e9e33 100644
> --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> @@ -21,6 +21,9 @@ config DRM_CDNS_DSI_J721E
> the routing of the DSS DPI signal to the Cadence DSI.
> endif
>
> +config CDNS_MHDP_HELPER
> + tristate
> +
I'm not sure if the placement for the helper is suitable. Especially if
you want to reuse the helper by some other subsystem. As far as I don't
like it, maybe drivers/soc is a better option.
> config DRM_CDNS_MHDP8546
> tristate "Cadence DPI/DP bridge"
> select DRM_DISPLAY_DP_HELPER
> @@ -28,6 +31,7 @@ config DRM_CDNS_MHDP8546
> select DRM_DISPLAY_HELPER
> select DRM_KMS_HELPER
> select DRM_PANEL_BRIDGE
> + select CDNS_MHDP_HELPER
> depends on OF
> help
> Support Cadence DPI to DP bridge. This is an internal
> diff --git a/drivers/gpu/drm/bridge/cadence/Makefile b/drivers/gpu/drm/bridge/cadence/Makefile
> index c95fd5b81d137..087dc074820d7 100644
> --- a/drivers/gpu/drm/bridge/cadence/Makefile
> +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> cdns-dsi-y := cdns-dsi-core.o
> cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
> +obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
> obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> cdns-mhdp8546-y := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) += cdns-mhdp8546-j721e.o
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> new file mode 100644
> index 0000000000000..f39228a78c7cb
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> @@ -0,0 +1,414 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023, 2024 NXP Semiconductor, Inc.
> + *
> + */
> +#include <drm/bridge/cdns-mhdp-helper.h>
> +#include <linux/dev_printk.h>
> +#include <linux/module.h>
> +
> +/* Protects mailbox communications with the firmware */
> +static DEFINE_MUTEX(mhdp_mailbox_mutex);
> +
> +/* Mailbox helper functions */
> +static int mhdp_mailbox_read(void __iomem *regs)
> +{
> + int ret, empty;
> +
> + WARN_ON(!mutex_is_locked(&mhdp_mailbox_mutex));
> +
> + ret = readx_poll_timeout(readl, regs + CDNS_MAILBOX_EMPTY,
> + empty, !empty, MAILBOX_RETRY_US,
> + MAILBOX_TIMEOUT_US);
> + if (ret < 0)
> + return ret;
> +
> + return readl(regs + CDNS_MAILBOX_RX_DATA) & 0xff;
> +}
> +
> +static int mhdp_mailbox_write(void __iomem *regs, u8 val)
> +{
> + int ret, full;
> +
> + WARN_ON(!mutex_is_locked(&mhdp_mailbox_mutex));
> +
> + ret = readx_poll_timeout(readl, regs + CDNS_MAILBOX_FULL,
> + full, !full, MAILBOX_RETRY_US,
> + MAILBOX_TIMEOUT_US);
> + if (ret < 0)
> + return ret;
> +
> + writel(val, regs + CDNS_MAILBOX_TX_DATA);
> +
> + return 0;
> +}
> +
> +static int mhdp_mailbox_recv_header(void __iomem *regs,
> + u8 module_id, u8 opcode,
> + u16 req_size)
> +{
> + u32 mbox_size, i;
> + u8 header[4];
> + int ret;
> +
> + /* read the header of the message */
> + for (i = 0; i < sizeof(header); i++) {
> + ret = mhdp_mailbox_read(regs);
> + if (ret < 0)
> + return ret;
> +
> + header[i] = ret;
> + }
> +
> + mbox_size = get_unaligned_be16(header + 2);
> +
> + if (opcode != header[0] || module_id != header[1] ||
> + (opcode != HDCP_TRAN_IS_REC_ID_VALID && req_size != mbox_size)) {
Could you please explain the second clause?
> + /*
> + * If the message in mailbox is not what we want, we need to
> + * clear the mailbox by reading its contents.
> + */
> + for (i = 0; i < mbox_size; i++)
> + if (mhdp_mailbox_read(regs) < 0)
> + break;
> +
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int mhdp_mailbox_recv_data(void __iomem *regs,
> + u8 *buff, u16 buff_size)
> +{
> + u32 i;
> + int ret;
> +
> + for (i = 0; i < buff_size; i++) {
> + ret = mhdp_mailbox_read(regs);
> + if (ret < 0)
> + return ret;
> +
> + buff[i] = ret;
> + }
> +
> + return 0;
> +}
> +
> +static int mhdp_mailbox_send(void __iomem *regs, u8 module_id,
> + u8 opcode, u16 size, u8 *message)
> +{
> + u8 header[4];
> + int ret, i;
> +
> + header[0] = opcode;
> + header[1] = module_id;
> + put_unaligned_be16(size, header + 2);
> +
> + for (i = 0; i < sizeof(header); i++) {
> + ret = mhdp_mailbox_write(regs, header[i]);
> + if (ret)
> + return ret;
> + }
> +
> + for (i = 0; i < size; i++) {
> + ret = mhdp_mailbox_write(regs, message[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
Please add kerneldocs for all new API. It's obvious for the simple ones,
but not for the more complex functions.
> +int cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8 module_id,
> + u8 opcode, u16 size, u8 *message)
> +{
> + int ret;
> +
> + mutex_lock(&mhdp_mailbox_mutex);
guard(mutex)(&mhdp_mailbox_mutex);
return mhdp_mailbox_send(...);
Similar changes can be applied further on.
> +
> + ret = mhdp_mailbox_send(base->regs, module_id, opcode, size, message);
> +
> + mutex_unlock(&mhdp_mailbox_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send);
> +
> +int cdns_mhdp_mailbox_send_recv(struct cdns_mhdp_base *base,
> + u8 module_id, u8 opcode,
> + u16 msg_size, u8 *msg,
> + u16 resp_size, u8 *resp)
> +{
> + int ret;
> +
> + mutex_lock(&mhdp_mailbox_mutex);
> +
> + ret = mhdp_mailbox_send(base->regs, module_id,
> + opcode, msg_size, msg);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_header(base->regs, module_id,
> + opcode, resp_size);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_data(base->regs, resp, resp_size);
> +out:
> + mutex_unlock(&mhdp_mailbox_mutex);
> +
> + if (ret)
> + dev_err(base->dev, "ModuleID=%d, CMD=%d failed: %d\n",
> + module_id, opcode, ret);
Using guard() would allow you to use precise messages, e.g. failed to
send, failed to received header, failed to receive data.
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send_recv);
> +
Like what is _multi? Why do you need to fetch into two separate buffers?
Can you fetch into one and then parse it accordingly? Please don't make
the API more complicated than necessary.
> +int cdns_mhdp_mailbox_send_recv_multi(struct cdns_mhdp_base *base,
> + u8 module_id, u8 opcode,
> + u16 msg_size, u8 *msg,
> + u8 opcode_resp,
> + u16 resp1_size, u8 *resp1,
> + u16 resp2_size, u8 *resp2)
> +{
> + int ret;
> +
> + mutex_lock(&mhdp_mailbox_mutex);
> +
> + ret = mhdp_mailbox_send(base->regs, module_id,
> + opcode, msg_size, msg);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_header(base->regs, module_id, opcode_resp,
> + resp1_size + resp2_size);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_data(base->regs, resp1, resp1_size);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_data(base->regs, resp2, resp2_size);
> +out:
> + mutex_unlock(&mhdp_mailbox_mutex);
> +
> + if (ret)
> + dev_err(base->dev, "ModuleID=%d, MSG_CMD=%d Resp_CMD=%d failed: %d\n",
> + module_id, opcode, opcode_resp, ret);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send_recv_multi);
> +
> +/*
> + * Secure mailbox access functions:
> + * These functions handle secure mailbox communication, which differs
> + * from non-secure mailbox access. They use the secure address.
> + */
> +int cdns_mhdp_secure_mailbox_send(struct cdns_mhdp_base *base, u8 module_id,
> + u8 opcode, u16 size, u8 *message)
> +{
> + int ret;
> +
> + mutex_lock(&mhdp_mailbox_mutex);
> +
> + ret = mhdp_mailbox_send(base->sapb_regs, module_id, opcode, size, message);
> +
> + mutex_unlock(&mhdp_mailbox_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_secure_mailbox_send);
> +
> +int cdns_mhdp_secure_mailbox_send_recv(struct cdns_mhdp_base *base,
> + u8 module_id, u8 opcode,
> + u16 msg_size, u8 *msg,
> + u16 resp_size, u8 *resp)
> +{
> + int ret;
> +
> + mutex_lock(&mhdp_mailbox_mutex);
> +
> + ret = mhdp_mailbox_send(base->sapb_regs, module_id,
> + opcode, msg_size, msg);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_header(base->sapb_regs, module_id,
> + opcode, resp_size);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_data(base->sapb_regs, resp, resp_size);
> +out:
> + mutex_unlock(&mhdp_mailbox_mutex);
> +
> + if (ret)
> + dev_err(base->dev, "ModuleID=%d, CMD=%d failed: %d\n",
> + module_id, opcode, ret);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_secure_mailbox_send_recv);
> +
> +int cdns_mhdp_secure_mailbox_send_recv_multi(struct cdns_mhdp_base *base,
> + u8 module_id, u8 opcode,
> + u16 msg_size, u8 *msg,
> + u8 opcode_resp,
> + u16 resp1_size, u8 *resp1,
> + u16 resp2_size, u8 *resp2)
> +{
> + int ret;
> +
> + mutex_lock(&mhdp_mailbox_mutex);
> +
> + ret = mhdp_mailbox_send(base->sapb_regs, module_id,
> + opcode, msg_size, msg);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_header(base->sapb_regs, module_id,
> + opcode_resp,
> + resp1_size + resp2_size);
> + if (ret)
> + goto out;
> +
> + ret = mhdp_mailbox_recv_data(base->sapb_regs, resp1, resp1_size);
> + if (ret)
> + goto out;
> +
> + if (module_id == MB_MODULE_ID_HDCP_TX &&
> + opcode == HDCP_TRAN_IS_REC_ID_VALID)
comment, please.
> + ret = mhdp_mailbox_recv_data(base->sapb_regs, resp2, 5 * resp1[0]);
> + else
> + ret = mhdp_mailbox_recv_data(base->sapb_regs, resp2, resp2_size);
> +out:
> + mutex_unlock(&mhdp_mailbox_mutex);
> +
> + if (ret)
> + dev_err(base->dev, "ModuleID=%d, CMD=%d failed: %d\n",
> + module_id, opcode, ret);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_secure_mailbox_send_recv_multi);
> +
> +/* General read/write helper functions */
> +int cdns_mhdp_reg_read(struct cdns_mhdp_base *base, u32 addr, u32 *value)
> +{
> + u8 msg[4], resp[8];
> + int ret;
> +
> + put_unaligned_be32(addr, msg);
> +
> + ret = cdns_mhdp_mailbox_send_recv(base, MB_MODULE_ID_GENERAL,
> + GENERAL_REGISTER_READ,
> + sizeof(msg), msg, sizeof(resp), resp);
> + if (ret)
> + goto out;
> +
> + /* Returned address value should be the same as requested */
> + if (memcmp(msg, resp, sizeof(msg))) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *value = get_unaligned_be32(resp + 4);
> +out:
> + if (ret) {
> + dev_err(base->dev, "Failed to read register\n");
> + *value = 0;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_read);
> +
> +int cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32 val)
> +{
> + u8 msg[8];
> +
> + put_unaligned_be32(addr, msg);
> + put_unaligned_be32(val, msg + 4);
> +
> + return cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> + GENERAL_REGISTER_WRITE,
> + sizeof(msg), msg);
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_write);
> +
> +/* DPTX helper functions */
> +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr, u32 val)
> +{
> + u8 msg[6];
> +
> + put_unaligned_be16(addr, msg);
> + put_unaligned_be32(val, msg + 2);
> +
> + return cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> + DPTX_WRITE_REGISTER, sizeof(msg), msg);
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write);
Please don't add API without a user. If the function is required in the
following patch, add it there.
> +
> +int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16 addr,
> + u8 start_bit, u8 bits_no, u32 val)
> +{
> + u8 field[8];
> +
> + put_unaligned_be16(addr, field);
> + field[2] = start_bit;
> + field[3] = bits_no;
> + put_unaligned_be32(val, field + 4);
> +
> + return cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> + DPTX_WRITE_FIELD, sizeof(field), field);
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write_bit);
> +
> +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> + u32 addr, u8 *data, u16 len)
> +{
> + u8 msg[5], reg[5];
> +
> + put_unaligned_be16(len, msg);
> + put_unaligned_be24(addr, msg + 2);
> +
> + return cdns_mhdp_mailbox_send_recv_multi(base,
> + MB_MODULE_ID_DP_TX,
> + DPTX_READ_DPCD,
> + sizeof(msg), msg,
> + DPTX_READ_DPCD,
> + sizeof(reg), reg,
> + len, data);
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_read);
> +
> +int cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8 value)
> +{
> + u8 msg[6], reg[5];
> + int ret;
> +
> + put_unaligned_be16(1, msg);
> + put_unaligned_be24(addr, msg + 2);
> + msg[5] = value;
> +
> + ret = cdns_mhdp_mailbox_send_recv(base, MB_MODULE_ID_DP_TX,
> + DPTX_WRITE_DPCD,
> + sizeof(msg), msg, sizeof(reg), reg);
> + if (ret) {
> + dev_err(base->dev, "dpcd write failed: %d\n", ret);
> + return ret;
> + }
> +
> + if (addr != get_unaligned_be24(reg + 2)) {
> + dev_err(base->dev, "Invalid response: expected address 0x%06x, got 0x%06x\n",
> + addr, get_unaligned_be24(reg + 2));
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_write);
> +
> +MODULE_DESCRIPTION("Cadence MHDP Helper driver");
> +MODULE_AUTHOR("Sandor Yu <Sandor.yu@xxxxxxx>");
> +MODULE_LICENSE("GPL");
--
With best wishes
Dmitry
