Message-ID: <8621221.DvuYhMxLoT@steina-w>
Date:   Fri, 13 Oct 2023 09:28:54 +0200
From:   Alexander Stein <alexander.stein@...tq-group.com>
To:     dmitry.baryshkov@...aro.org, andrzej.hajda@...el.com,
        neil.armstrong@...aro.org, Laurent.pinchart@...asonboard.com,
        jonas@...boo.se, jernej.skrabec@...il.com, airlied@...il.com,
        daniel@...ll.ch, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, festevam@...il.com, vkoul@...nel.org,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-phy@...ts.infradead.org, Sandor Yu <Sandor.yu@....com>,
        Sandor.yu@....com
Cc:     kernel@...gutronix.de, linux-imx@....com, oliver.brown@....com,
        sam@...nborg.org
Subject: Re: [PATCH v10 1/7] drm: bridge: Cadence: Creat mhdp helper driver

Hi Sandor,

thanks for the updated series.

Am Freitag, 13. Oktober 2023, 05:24:20 CEST schrieb Sandor Yu:
> MHDP8546 mailbox access functions will be share to other mhdp driver
> and Cadence HDP-TX HDMI/DP PHY drivers.
> Create a new mhdp helper driver and move all those functions into.
> 
> 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 created with the general command ID
> GENERAL_REGISTER_WRITE.
> 
> Signed-off-by: Sandor Yu <Sandor.yu@....com>
> ---
> v9->v10:
>  *use mhdp helper driver to replace macro functions,
>  move maibox access function and mhdp hdmi/dp common API
>  functions into the driver.
> 
>  drivers/gpu/drm/bridge/cadence/Kconfig        |   4
>  drivers/gpu/drm/bridge/cadence/Makefile       |   1 +
>  .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 306 ++++++++++++++
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 383 +++---------------
>  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  44 +-
>  include/drm/bridge/cdns-mhdp-helper.h         |  96 +++++
>  6 files changed, 473 insertions(+), 361 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 ec35215a20034..0b7b4626a7af0
> 100644
> --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> @@ -20,6 +20,9 @@ config DRM_CDNS_DSI_J721E
>  	  the routing of the DSS DPI signal to the Cadence DSI.
>  endif
> 
> +config CDNS_MHDP_HELPER
> +	tristate
> +
>  config DRM_CDNS_MHDP8546
>  	tristate "Cadence DPI/DP bridge"
>  	select DRM_DISPLAY_DP_HELPER
> @@ -27,6 +30,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..2e3eee40494f0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> @@ -0,0 +1,306 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 NXP Semiconductor, Inc.
> + *
> + */
> +#include <drm/bridge/cdns-mhdp-helper.h>
> +#include <linux/dev_printk.h>
> +#include <linux/module.h>
> +
> +/* Mailbox helper functions */
> +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base)
> +{
> +	int ret, empty;
> +
> +	WARN_ON(!mutex_is_locked(base->mbox_mutex));
> +
> +	ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_EMPTY,
> +				 empty, !empty, MAILBOX_RETRY_US,
> +				 MAILBOX_TIMEOUT_US);
> +	if (ret < 0)
> +		return ret;
> +
> +	return readl(base->regs + CDNS_MAILBOX_RX_DATA) & 0xff;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_read);

No need to export this. You can make this function actually static.

> +
> +int cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val)
> +{
> +	int ret, full;
> +
> +	WARN_ON(!mutex_is_locked(base->mbox_mutex));
> +
> +	ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_FULL,
> +				 full, !full, MAILBOX_RETRY_US,
> +				 MAILBOX_TIMEOUT_US);
> +	if (ret < 0)
> +		return ret;
> +
> +	writel(val, base->regs + CDNS_MAILBOX_TX_DATA);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_write);

No need to export that one as well. You can make this function actually static 
as these two functions are only called from the helper itself.

> +
> +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> +				  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 = cdns_mhdp_mailbox_read(base);
> +		if (ret < 0)
> +			return ret;
> +
> +		header[i] = ret;
> +	}
> +
> +	mbox_size = get_unaligned_be16(header + 2);
> +
> +	if (opcode != header[0] || module_id != header[1] ||
> +	    req_size != mbox_size) {
> +		/*
> +		 * 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 (cdns_mhdp_mailbox_read(base) < 0)
> +				break;
> +
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_header);
> +
> +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> +				u8 *buff, u16 buff_size)
> +{
> +	u32 i;
> +	int ret;
> +
> +	for (i = 0; i < buff_size; i++) {
> +		ret = cdns_mhdp_mailbox_read(base);
> +		if (ret < 0)
> +			return ret;
> +
> +		buff[i] = ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_data);
> +
> +int cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, 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 = cdns_mhdp_mailbox_write(base, header[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < size; i++) {
> +		ret = cdns_mhdp_mailbox_write(base, message[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send);
> +
> +/* General 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);
> +
> +	mutex_lock(base->mbox_mutex);
> +
> +	ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> +				     GENERAL_REGISTER_READ,
> +				     sizeof(msg), msg);
> +	if (ret)
> +		goto out;
> +
> +	ret = cdns_mhdp_mailbox_recv_header(base, MB_MODULE_ID_GENERAL,
> +					    GENERAL_REGISTER_READ,
> +					    sizeof(resp));
> +	if (ret)
> +		goto out;
> +
> +	ret = cdns_mhdp_mailbox_recv_data(base, resp, sizeof(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:
> +	mutex_unlock(base->mbox_mutex);
> +	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];
> +	int ret;
> +
> +	put_unaligned_be32(addr, msg);
> +	put_unaligned_be32(val, msg + 4);
> +
> +	mutex_lock(base->mbox_mutex);
> +
> +	ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> +				     GENERAL_REGISTER_WRITE,
> +				     sizeof(msg), msg);
> +
> +	mutex_unlock(base->mbox_mutex);
> +
> +	return ret;
> +}
> +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];
> +	int ret;
> +
> +	put_unaligned_be16(addr, msg);
> +	put_unaligned_be32(val, msg + 2);
> +
> +	mutex_lock(base->mbox_mutex);
> +
> +	ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> +				     DPTX_WRITE_REGISTER, sizeof(msg), 
msg);
> +
> +	mutex_unlock(base->mbox_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write);
> +
> +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];
> +	int ret;
> +
> +	put_unaligned_be16(addr, field);
> +	field[2] = start_bit;
> +	field[3] = bits_no;
> +	put_unaligned_be32(val, field + 4);
> +
> +	mutex_lock(base->mbox_mutex);
> +
> +	ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> +				     DPTX_WRITE_FIELD, sizeof(field), 
field);
> +
> +	mutex_unlock(base->mbox_mutex);
> +
> +	return ret;
> +}
> +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];
> +	int ret;
> +
> +	put_unaligned_be16(len, msg);
> +	put_unaligned_be24(addr, msg + 2);
> +
> +	mutex_lock(base->mbox_mutex);
> +
> +	ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> +				     DPTX_READ_DPCD, sizeof(msg), 
msg);
> +	if (ret)
> +		goto out;
> +
> +	ret = cdns_mhdp_mailbox_recv_header(base, MB_MODULE_ID_DP_TX,
> +					    DPTX_READ_DPCD,
> +					    sizeof(reg) + len);
> +	if (ret)
> +		goto out;
> +
> +	ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> +	if (ret)
> +		goto out;
> +
> +	ret = cdns_mhdp_mailbox_recv_data(base, data, len);
> +
> +out:
> +	mutex_unlock(base->mbox_mutex);
> +
> +	return ret;
> +}
> +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;
> +
> +	mutex_lock(base->mbox_mutex);
> +
> +	ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> +				     DPTX_WRITE_DPCD, sizeof(msg), 
msg);
> +	if (ret)
> +		goto out;
> +
> +	ret = cdns_mhdp_mailbox_recv_header(base, MB_MODULE_ID_DP_TX,
> +					    DPTX_WRITE_DPCD, 
sizeof(reg));
> +	if (ret)
> +		goto out;
> +
> +	ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> +	if (ret)
> +		goto out;
> +
> +	if (addr != get_unaligned_be24(reg + 2))
> +		ret = -EINVAL;
> +
> +out:
> +	mutex_unlock(base->mbox_mutex);
> +
> +	if (ret)
> +		dev_err(base->dev, "dpcd write failed: %d\n", ret);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_write);
> +
> +MODULE_DESCRIPTION("Cadence MHDP Helper driver");
> +MODULE_AUTHOR("Sandor Yu <Sandor.yu@....com>");
> +MODULE_LICENSE("GPL");
> [...]
> diff --git a/include/drm/bridge/cdns-mhdp-helper.h
> b/include/drm/bridge/cdns-mhdp-helper.h new file mode 100644
> index 0000000000000..b89db9e842266
> --- /dev/null
> +++ b/include/drm/bridge/cdns-mhdp-helper.h
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 NXP Semiconductor, Inc.
> + */
> +#ifndef __CDNS_MHDP_HELPER_H__
> +#define __CDNS_MHDP_HELPER_H__
> +
> +#include <asm/unaligned.h>
> +#include <linux/iopoll.h>
> +
> +/* mailbox regs offset */
> +#define CDNS_MAILBOX_FULL			0x00008
> +#define CDNS_MAILBOX_EMPTY			0x0000c
> +#define CDNS_MAILBOX_TX_DATA			0x00010
> +#define CDNS_MAILBOX_RX_DATA			0x00014
> +
> +#define MAILBOX_RETRY_US			1000
> +#define MAILBOX_TIMEOUT_US			2000000
> +
> +/* Module ID Code */
> +#define MB_MODULE_ID_DP_TX			0x01
> +#define MB_MODULE_ID_HDMI_TX			0x03
> +#define MB_MODULE_ID_HDCP_TX			0x07
> +#define MB_MODULE_ID_HDCP_RX			0x08
> +#define MB_MODULE_ID_HDCP_GENERAL		0x09
> +#define MB_MODULE_ID_GENERAL			0x0A
> +
> +/* General Commands */
> +#define GENERAL_MAIN_CONTROL			0x01
> +#define GENERAL_TEST_ECHO			0x02
> +#define GENERAL_BUS_SETTINGS			0x03
> +#define GENERAL_TEST_ACCESS			0x04
> +#define GENERAL_REGISTER_WRITE			0x05
> +#define GENERAL_WRITE_FIELD			0x06
> +#define GENERAL_REGISTER_READ			0x07
> +#define GENERAL_GET_HPD_STATE			0x11
> +
> +/* DPTX Commands */
> +#define DPTX_SET_POWER_MNG			0x00
> +#define DPTX_SET_HOST_CAPABILITIES		0x01
> +#define DPTX_GET_EDID				0x02
> +#define DPTX_READ_DPCD				0x03
> +#define DPTX_WRITE_DPCD				0x04
> +#define DPTX_ENABLE_EVENT			0x05
> +#define DPTX_WRITE_REGISTER			0x06
> +#define DPTX_READ_REGISTER			0x07
> +#define DPTX_WRITE_FIELD			0x08
> +#define DPTX_TRAINING_CONTROL			0x09
> +#define DPTX_READ_EVENT				0x0a
> +#define DPTX_READ_LINK_STAT			0x0b
> +#define DPTX_SET_VIDEO				0x0c
> +#define DPTX_SET_AUDIO				0x0d
> +#define DPTX_GET_LAST_AUX_STAUS			0x0e
> +#define DPTX_SET_LINK_BREAK_POINT		0x0f
> +#define DPTX_FORCE_LANES			0x10
> +#define DPTX_HPD_STATE				0x11
> +#define DPTX_ADJUST_LT				0x12
> +
> +/* HDMI TX Commands */
> +#define HDMI_TX_READ				0x00
> +#define HDMI_TX_WRITE				0x01
> +#define HDMI_TX_UPDATE_READ			0x02
> +#define HDMI_TX_EDID				0x03
> +#define HDMI_TX_EVENTS				0x04
> +#define HDMI_TX_HPD_STATUS			0x05
> +
> +struct cdns_mhdp_base {
> +	struct device *dev;
> +	void __iomem *regs;
> +	/* protect mailbox communications with the firmware */
> +	struct mutex *mbox_mutex;
> +};
> +
> +/* Mailbox helper functions */
> +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base);
> +int cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val);

You can remove these two declarations.

Best regards,
Alexander

Best regards,
> +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> +				  u8 module_id, u8 opcode, u16 
req_size);
> +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> +				u8 *buff, u16 buff_size);
> +int cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8 module_id,
> +			   u8 opcode, u16 size, u8 *message);
> +
> +/* General commands helper functions */
> +int cdns_mhdp_reg_read(struct cdns_mhdp_base *base, u32 addr, u32 *value);
> +int cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32 val);
> +
> +/* DPTX commands helper functions */
> +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr, u32 val);
> +int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16 addr, +	
		
>       u8 start_bit, u8 bits_no, u32 val);
> +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> +			u32 addr, u8 *data, u16 len);
> +int cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8 value);
> +
> +#endif /* __CDNS_MHDP_HELPER_H__ */

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider

Powered by blists - more mailing lists