mbox series

[v5,00/10] Initial support for Cadence MHDP(HDMI/DP) for i.MX8MQ

Message ID cover.1669620155.git.Sandor.yu@nxp.com
Headers show
Series Initial support for Cadence MHDP(HDMI/DP) for i.MX8MQ | expand

Message

Sandor Yu Nov. 28, 2022, 7:36 a.m. UTC
The patch set initial support for Cadence MHDP(HDMI/DP) DRM bridge
drivers and Cadence HDP-TX PHY(HDMI/DP) drivers for iMX8MQ.

The patch set compose of DRM bridge drivers and PHY drivers.
Both of them need the followed two patches to pass build.
  drm: bridge: cadence: convert mailbox functions to macro functions
  phy: Add HDMI configuration options

DRM bridges driver patches:
  dts-bingings: display: bridge: Add MHDP HDMI bindings for i.MX8MQ
  drm: bridge: cadence: Add MHDP DP driver for i.MX8MQ
  dts-bindings: display: bridge: Add MHDP DP bindings for i.MX8MQ
  drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ

PHY driver patches:
  dts-bindings: phy: Add Cadence HDP-TX DP PHY bindings
  phy: cadence: Add driver for HDP-TX DisplyPort PHY
  dts-bindings: phy: Add Cadence HDP-TX HDMI PHY bindings
  phy: cadence: Add driver for HDP-TX HDMI PHY

v4->v5:
- Drop "clk" suffix in clock name.
- Add output port property in the example of hdmi/dp.

v3->v4:
dt-bindings:
- Correct dt-bindings coding style and address review comments.
- Add apb_clk description.
- Add output port for HDMI/DP connector
PHY:
- Alphabetically sorted in Kconfig and Makefile for DP and HDMI PHY
- Remove unused registers define from HDMI and DP PHY drivers.
- More description in phy_hdmi.h.
- Add apb_clk to HDMI and DP phy driver.
HDMI/DP:
- Use get_unaligned_le32() to replace hardcode type conversion
  in HDMI AVI infoframe data fill function.
- Add mailbox mutex lock in HDMI/DP driver for phy functions
  to reslove race conditions between HDMI/DP and PHY drivers.
- Add apb_clk to both HDMI and DP driver.
- Rename some function names and add prefix with "cdns_hdmi/cdns_dp".
- Remove bpc 12 and 16 optional that not supported.

v2->v3:
Address comments for dt-bindings files.
- Correct dts-bindings file names 
  Rename phy-cadence-hdptx-dp.yaml to cdns,mhdp-imx8mq-dp.yaml
  Rename phy-cadence-hdptx-hdmi.yaml to cdns,mhdp-imx8mq-hdmi.yaml
- Drop redundant words and descriptions.
- Correct hdmi/dp node name.

v2 is a completely different version compared to v1.
Previous v1 can be available here [1].

v1->v2:
- Reuse Cadence mailbox access functions from mhdp8546 instead of
  rockchip DP.
- Mailbox access functions be convert to marco functions
  that will be referenced by HDP-TX PHY(HDMI/DP) driver too.
- Plain bridge instead of component driver.
- Standalone Cadence HDP-TX PHY(HDMI/DP) driver.
- Audio driver are removed from the patch set, it will be add in another
  patch set later.

[1] https://patchwork.kernel.org/project/linux-rockchip/cover/cover.1590982881.git.Sandor.yu@nxp.com/

Sandor Yu (10):
  drm: bridge: cadence: convert mailbox functions to macro functions
  dt-bindings: display: bridge: Add MHDP DP for i.MX8MQ
  drm: bridge: cadence: Add MHDP DP driver for i.MX8MQ
  phy: Add HDMI configuration options
  dt-bindings: display: bridge: Add MHDP HDMI for i.MX8MQ
  drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ
  dt-bindings: phy: Add Cadence HDP-TX DP PHY
  phy: cadence: Add driver for HDP-TX DisplyPort PHY
  dt-bindings: phy: Add Cadence HDP-TX HDMI PHY
  phy: cadence: Add driver for HDP-TX HDMI PHY

 .../display/bridge/cdns,mhdp-imx8mq-dp.yaml   |  102 ++
 .../display/bridge/cdns,mhdp-imx8mq-hdmi.yaml |  102 ++
 .../bindings/phy/cdns,hdptx-dp-phy.yaml       |   68 ++
 .../bindings/phy/cdns,hdptx-hdmi-phy.yaml     |   52 +
 drivers/gpu/drm/bridge/cadence/Kconfig        |   25 +
 drivers/gpu/drm/bridge/cadence/Makefile       |    3 +
 drivers/gpu/drm/bridge/cadence/cdns-dp-core.c | 1071 +++++++++++++++++
 .../gpu/drm/bridge/cadence/cdns-hdmi-core.c   | 1018 ++++++++++++++++
 .../gpu/drm/bridge/cadence/cdns-mhdp-common.h |  400 ++++++
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   |  197 +--
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |    1 -
 drivers/phy/cadence/Kconfig                   |   16 +
 drivers/phy/cadence/Makefile                  |    2 +
 drivers/phy/cadence/phy-cadence-hdptx-dp.c    |  737 ++++++++++++
 drivers/phy/cadence/phy-cadence-hdptx-hdmi.c  |  891 ++++++++++++++
 include/drm/bridge/cdns-mhdp-mailbox.h        |  240 ++++
 include/linux/phy/phy-hdmi.h                  |   38 +
 include/linux/phy/phy.h                       |    7 +-
 18 files changed, 4773 insertions(+), 197 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp-imx8mq-dp.yaml
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp-imx8mq-hdmi.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/cdns,hdptx-dp-phy.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/cdns,hdptx-hdmi-phy.yaml
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-dp-core.c
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
 create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-common.h
 create mode 100644 drivers/phy/cadence/phy-cadence-hdptx-dp.c
 create mode 100644 drivers/phy/cadence/phy-cadence-hdptx-hdmi.c
 create mode 100644 include/drm/bridge/cdns-mhdp-mailbox.h
 create mode 100644 include/linux/phy/phy-hdmi.h

Comments

Lucas Stach Dec. 9, 2022, 11:29 a.m. UTC | #1
Hi Sandor,

Am Montag, dem 28.11.2022 um 15:36 +0800 schrieb Sandor Yu:
> Mailbox access functions could be share to other mhdp driver and
> HDP-TX HDMI/DP PHY drivers, move those functions to head file
> include/drm/bridge/cdns-mhdp-mailbox.h and convert them to
> macro functions.
> 

This does not look right. If those functions need to be called from
other units and implemented in the header, they should simply be static
inline functions rather than macros. Those macros obfuscate the code a
lot.

However, I don't believe those should be called by other units
directly. From what I can see this is mostly used by the PHY drivers,
to interact with the mailbox and read/write PHY registers. However,
there is no locking between the bridge and the core driver in any form,
so they could corrupt the mailbox state. The PHY drivers have mailbox
mutexes, but as far as I can see they aren't used and even if they were
they would not guard against concurrent access to the mailbox between
the bridge and the PHY driver.

I think the right thing to do here would be to have a regmap
implementation in the bridge driver, which uses the mailbox functions
to implement the register access. Then this regmap could be passed down
from the bridge driver to the PHY, which should simply be a child node
of the bridge in DT.

Regards,
Lucas

> Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 197 +-------------
>  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |   1 -
>  include/drm/bridge/cdns-mhdp-mailbox.h        | 240 ++++++++++++++++++
>  3 files changed, 242 insertions(+), 196 deletions(-)
>  create mode 100644 include/drm/bridge/cdns-mhdp-mailbox.h
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 31442a922502..b77b0ddcc9b3 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -36,6 +36,7 @@
>  #include <linux/slab.h>
>  #include <linux/wait.h>
>  
> +#include <drm/bridge/cdns-mhdp-mailbox.h>
>  #include <drm/display/drm_dp_helper.h>
>  #include <drm/display/drm_hdcp_helper.h>
>  #include <drm/drm_atomic.h>
> @@ -55,200 +56,6 @@
>  #include "cdns-mhdp8546-hdcp.h"
>  #include "cdns-mhdp8546-j721e.h"
>  
> -static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
> -{
> -	int ret, empty;
> -
> -	WARN_ON(!mutex_is_locked(&mhdp->mbox_mutex));
> -
> -	ret = readx_poll_timeout(readl, mhdp->regs + CDNS_MAILBOX_EMPTY,
> -				 empty, !empty, MAILBOX_RETRY_US,
> -				 MAILBOX_TIMEOUT_US);
> -	if (ret < 0)
> -		return ret;
> -
> -	return readl(mhdp->regs + CDNS_MAILBOX_RX_DATA) & 0xff;
> -}
> -
> -static int cdns_mhdp_mailbox_write(struct cdns_mhdp_device *mhdp, u8 val)
> -{
> -	int ret, full;
> -
> -	WARN_ON(!mutex_is_locked(&mhdp->mbox_mutex));
> -
> -	ret = readx_poll_timeout(readl, mhdp->regs + CDNS_MAILBOX_FULL,
> -				 full, !full, MAILBOX_RETRY_US,
> -				 MAILBOX_TIMEOUT_US);
> -	if (ret < 0)
> -		return ret;
> -
> -	writel(val, mhdp->regs + CDNS_MAILBOX_TX_DATA);
> -
> -	return 0;
> -}
> -
> -static int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_device *mhdp,
> -					 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(mhdp);
> -		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(mhdp) < 0)
> -				break;
> -
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -static int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_device *mhdp,
> -				       u8 *buff, u16 buff_size)
> -{
> -	u32 i;
> -	int ret;
> -
> -	for (i = 0; i < buff_size; i++) {
> -		ret = cdns_mhdp_mailbox_read(mhdp);
> -		if (ret < 0)
> -			return ret;
> -
> -		buff[i] = ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int cdns_mhdp_mailbox_send(struct cdns_mhdp_device *mhdp, 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(mhdp, header[i]);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	for (i = 0; i < size; i++) {
> -		ret = cdns_mhdp_mailbox_write(mhdp, message[i]);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static
> -int cdns_mhdp_reg_read(struct cdns_mhdp_device *mhdp, u32 addr, u32 *value)
> -{
> -	u8 msg[4], resp[8];
> -	int ret;
> -
> -	put_unaligned_be32(addr, msg);
> -
> -	mutex_lock(&mhdp->mbox_mutex);
> -
> -	ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_GENERAL,
> -				     GENERAL_REGISTER_READ,
> -				     sizeof(msg), msg);
> -	if (ret)
> -		goto out;
> -
> -	ret = cdns_mhdp_mailbox_recv_header(mhdp, MB_MODULE_ID_GENERAL,
> -					    GENERAL_REGISTER_READ,
> -					    sizeof(resp));
> -	if (ret)
> -		goto out;
> -
> -	ret = cdns_mhdp_mailbox_recv_data(mhdp, 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(&mhdp->mbox_mutex);
> -	if (ret) {
> -		dev_err(mhdp->dev, "Failed to read register\n");
> -		*value = 0;
> -	}
> -
> -	return ret;
> -}
> -
> -static
> -int cdns_mhdp_reg_write(struct cdns_mhdp_device *mhdp, u16 addr, u32 val)
> -{
> -	u8 msg[6];
> -	int ret;
> -
> -	put_unaligned_be16(addr, msg);
> -	put_unaligned_be32(val, msg + 2);
> -
> -	mutex_lock(&mhdp->mbox_mutex);
> -
> -	ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_DP_TX,
> -				     DPTX_WRITE_REGISTER, sizeof(msg), msg);
> -
> -	mutex_unlock(&mhdp->mbox_mutex);
> -
> -	return ret;
> -}
> -
> -static
> -int cdns_mhdp_reg_write_bit(struct cdns_mhdp_device *mhdp, 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(&mhdp->mbox_mutex);
> -
> -	ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_DP_TX,
> -				     DPTX_WRITE_FIELD, sizeof(field), field);
> -
> -	mutex_unlock(&mhdp->mbox_mutex);
> -
> -	return ret;
> -}
> -
>  static
>  int cdns_mhdp_dpcd_read(struct cdns_mhdp_device *mhdp,
>  			u32 addr, u8 *data, u16 len)
> @@ -2058,7 +1865,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>  				     struct drm_bridge_state *bridge_state)
>  {
>  	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> -	u32 resp;
> +	u32 resp = 0;
>  
>  	dev_dbg(mhdp->dev, "%s\n", __func__);
>  
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index bedddd510d17..10c878bf0e63 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -212,7 +212,6 @@ struct phy;
>  #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
>  
>  /* firmware and opcodes */
>  #define FW_NAME					"cadence/mhdp8546.bin"
> diff --git a/include/drm/bridge/cdns-mhdp-mailbox.h b/include/drm/bridge/cdns-mhdp-mailbox.h
> new file mode 100644
> index 000000000000..0249322a74b0
> --- /dev/null
> +++ b/include/drm/bridge/cdns-mhdp-mailbox.h
> @@ -0,0 +1,240 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Cadence MHDP Firmware Access API function by Malibox.
> + *
> + * Copyright (C) 2022 NXP Semiconductor, Inc.
> + *
> + */
> +#ifndef CDNS_MHDP_MAILBOX_H
> +#define CDNS_MHDP_MAILBOX_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_GENERAL		0x0A
> +#define MB_MODULE_ID_DP_TX			0x01
> +
> +/* General Commands */
> +#define GENERAL_REGISTER_WRITE		0x05
> +#define GENERAL_REGISTER_READ		0x07
> +
> +/* DP TX Command */
> +#define DPTX_WRITE_FIELD			0x08
> +
> +/* MHDP Firmware access functions by Mailbox */
> +#define cdns_mhdp_mailbox_read(__mhdp) \
> +({ \
> +	int ret, empty, val; \
> +\
> +	WARN_ON(!mutex_is_locked(&(__mhdp)->mbox_mutex)); \
> +\
> +	do {  \
> +		ret = readx_poll_timeout(readl, (__mhdp)->regs + CDNS_MAILBOX_EMPTY,  \
> +					 empty, !empty, MAILBOX_RETRY_US,  \
> +					 MAILBOX_TIMEOUT_US);  \
> +		if (ret < 0)  \
> +			break;  \
> +\
> +		val = readl((__mhdp)->regs + CDNS_MAILBOX_RX_DATA) & 0xff; \
> +	} while (0);  \
> +\
> +	(ret < 0) ? ret : val;  \
> +})
> +
> +#define cdns_mhdp_mailbox_write(__mhdp, __val) \
> +({ \
> +	int ret, full;  \
> +\
> +	WARN_ON(!mutex_is_locked(&(__mhdp)->mbox_mutex)); \
> +\
> +	do {  \
> +		ret = readx_poll_timeout(readl, (__mhdp)->regs + CDNS_MAILBOX_FULL,  \
> +					 full, !full, MAILBOX_RETRY_US,  \
> +					 MAILBOX_TIMEOUT_US);  \
> +		if (ret < 0)  \
> +			break;  \
> +\
> +		writel((__val), (__mhdp)->regs + CDNS_MAILBOX_TX_DATA); \
> +	} while (0);  \
> +\
> +	ret; \
> +})
> +
> +#define  cdns_mhdp_mailbox_recv_header(__mhdp, __module_id, __opcode, __req_size) \
> +({  \
> +	u32 mbox_size, i;  \
> +	u8 header[4];  \
> +	int ret;  \
> +\
> +	do {  \
> +		/* read the header of the message */ \
> +		for (i = 0; i < sizeof(header); i++) {  \
> +			ret = cdns_mhdp_mailbox_read(__mhdp);  \
> +			if (ret < 0)  \
> +				break;  \
> +\
> +			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(__mhdp) < 0)  \
> +					break;  \
> +\
> +			ret = -EINVAL;  \
> +		}  \
> +\
> +		ret = 0; \
> +\
> +	} while (0);  \
> +\
> +	ret;  \
> +})
> +
> +#define cdns_mhdp_mailbox_recv_data(mhdp, buff, buff_size)  \
> +({  \
> +	u32 i;  \
> +	int ret;  \
> +\
> +	do {  \
> +		for (i = 0; i < buff_size; i++) {  \
> +			ret = cdns_mhdp_mailbox_read(mhdp);  \
> +			if (ret < 0)  \
> +				break;  \
> +\
> +			((u8 *)buff)[i] = ret;  \
> +		}  \
> +\
> +		ret = 0;  \
> +\
> +	} while (0);  \
> +\
> +	ret; \
> +})
> +
> +#define cdns_mhdp_mailbox_send(mhdp, module_id, opcode, size, message)  \
> +({  \
> +	u8 header[4];  \
> +	int ret, i;  \
> +\
> +	header[0] = opcode;  \
> +	header[1] = module_id;  \
> +	put_unaligned_be16(size, header + 2);  \
> +\
> +	do {  \
> +		for (i = 0; i < sizeof(header); i++) {  \
> +			ret = cdns_mhdp_mailbox_write(mhdp, header[i]);  \
> +			if (ret < 0)  \
> +				break;  \
> +		}  \
> +\
> +		for (i = 0; i < size; i++) {  \
> +			ret = cdns_mhdp_mailbox_write(mhdp, ((u8 *)message)[i]);  \
> +			if (ret < 0)  \
> +				break;;  \
> +		}  \
> +		ret = 0;  \
> +	} while (0);  \
> +\
> +	ret;  \
> +})
> +
> +#define cdns_mhdp_reg_read(mhdp, addr, value)  \
> +({  \
> +	u8 msg[4], resp[8];  \
> +	int ret;  \
> +\
> +	put_unaligned_be32(addr, msg);  \
> +\
> +	mutex_lock(&mhdp->mbox_mutex);  \
> +\
> +	do {  \
> +		ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_GENERAL,  \
> +					     GENERAL_REGISTER_READ,  \
> +					     sizeof(msg), msg);  \
> +		if (ret < 0)  \
> +			break;  \
> +\
> +		ret = cdns_mhdp_mailbox_recv_header(mhdp, MB_MODULE_ID_GENERAL,  \
> +						    GENERAL_REGISTER_READ,  \
> +						    sizeof(resp));  \
> +		if (ret < 0)  \
> +			break;  \
> +\
> +		ret = cdns_mhdp_mailbox_recv_data(mhdp, resp, sizeof(resp));  \
> +		if (ret < 0)  \
> +			break;  \
> +\
> +		/* Returned address value should be the same as requested */  \
> +		if (memcmp(msg, resp, sizeof(msg))) {  \
> +			ret = -EINVAL;  \
> +			break;  \
> +		}  \
> +\
> +		*((u32 *)value) = get_unaligned_be32(resp + 4);  \
> +			ret = 0;  \
> +	} while (0);  \
> +\
> +	mutex_unlock(&mhdp->mbox_mutex);  \
> +	if (ret < 0) {  \
> +		dev_err(mhdp->dev, "Failed to read register\n");  \
> +		*((u32 *)value) = 0;  \
> +	}  \
> +\
> +	ret;  \
> +})
> +
> +#define cdns_mhdp_reg_write(mhdp, addr, val)  \
> +({  \
> +	u8 msg[8];  \
> +	int ret;  \
> +\
> +	put_unaligned_be32(addr, msg);  \
> +	put_unaligned_be32(val, msg + 4);  \
> +\
> +	mutex_lock(&mhdp->mbox_mutex);  \
> +\
> +	ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_GENERAL,  \
> +				      GENERAL_REGISTER_WRITE, sizeof(msg), msg);  \
> +\
> +	mutex_unlock(&mhdp->mbox_mutex);  \
> +\
> +	ret;  \
> +})
> +
> +#define cdns_mhdp_reg_write_bit(mhdp, addr, start_bit, bits_no, 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(&mhdp->mbox_mutex);  \
> +\
> +	ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_DP_TX, \
> +				     DPTX_WRITE_FIELD, sizeof(field), field);  \
> +\
> +	mutex_unlock(&mhdp->mbox_mutex);  \
> +\
> +	ret; \
> +})
> +
> +#endif
Sandor Yu Dec. 12, 2022, 8:02 a.m. UTC | #2
Hi Lucas,

Thanks your comments,

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年12月9日 19:30
> To: Sandor Yu <sandor.yu@nxp.com>; andrzej.hajda@intel.com;
> neil.armstrong@linaro.org; robert.foss@linaro.org;
> Laurent.pinchart@ideasonboard.com; jonas@kwiboo.se;
> jernej.skrabec@gmail.com; airlied@gmail.com; daniel@ffwll.ch;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> vkoul@kernel.org; dri-devel@lists.freedesktop.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-phy@lists.infradead.org
> Cc: Oliver Brown <oliver.brown@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de
> Subject: [EXT] Re: [PATCH v5 01/10] drm: bridge: cadence: convert mailbox
> functions to macro functions
> 
> Caution: EXT Email
> 
> Hi Sandor,
> 
> Am Montag, dem 28.11.2022 um 15:36 +0800 schrieb Sandor Yu:
> > Mailbox access functions could be share to other mhdp driver and
> > HDP-TX HDMI/DP PHY drivers, move those functions to head file
> > include/drm/bridge/cdns-mhdp-mailbox.h and convert them to macro
> > functions.
> >
> 
> This does not look right. If those functions need to be called from other units
> and implemented in the header, they should simply be static inline functions
> rather than macros. Those macros obfuscate the code a lot.
> 
> However, I don't believe those should be called by other units directly. From
> what I can see this is mostly used by the PHY drivers, to interact with the
> mailbox and read/write PHY registers. However, there is no locking between
> the bridge and the core driver in any form, so they could corrupt the mailbox
> state. The PHY drivers have mailbox mutexes, but as far as I can see they
> aren't used and even if they were they would not guard against concurrent
> access to the mailbox between the bridge and the PHY driver.
Both PHY and bridge have its owner mutex to lock mailbox read/write.
In bridge driver, this mutex also lock for PHY API function access to avoid PHY and bridge driver race condition.

> 
> I think the right thing to do here would be to have a regmap implementation
> in the bridge driver, which uses the mailbox functions to implement the
> register access. Then this regmap could be passed down from the bridge
> driver to the PHY, which should simply be a child node of the bridge in DT.
> 
I'm not sure got your point " regmap implementation in the bridge driver".
Those mailbox functions also should be reused by mhdp8546.
Could you please explain detail or provide a sample?

Regards,
Sandor

> Regards,
> Lucas
> 
> > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > ---
> >  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 197 +-------------
> >  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |   1 -
> >  include/drm/bridge/cdns-mhdp-mailbox.h        | 240
> ++++++++++++++++++
> >  3 files changed, 242 insertions(+), 196 deletions(-)  create mode
> > 100644 include/drm/bridge/cdns-mhdp-mailbox.h
> >
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > index 31442a922502..b77b0ddcc9b3 100644
> > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > @@ -36,6 +36,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/wait.h>
> >
> > +#include <drm/bridge/cdns-mhdp-mailbox.h>
> >  #include <drm/display/drm_dp_helper.h>  #include
> > <drm/display/drm_hdcp_helper.h>  #include <drm/drm_atomic.h> @@
> > -55,200 +56,6 @@  #include "cdns-mhdp8546-hdcp.h"
> >  #include "cdns-mhdp8546-j721e.h"
> >
> > -static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp) -{
> > -     int ret, empty;
> > -
> > -     WARN_ON(!mutex_is_locked(&mhdp->mbox_mutex));
> > -
> > -     ret = readx_poll_timeout(readl, mhdp->regs +
> CDNS_MAILBOX_EMPTY,
> > -                              empty, !empty, MAILBOX_RETRY_US,
> > -                              MAILBOX_TIMEOUT_US);
> > -     if (ret < 0)
> > -             return ret;
> > -
> > -     return readl(mhdp->regs + CDNS_MAILBOX_RX_DATA) & 0xff;
> > -}
> > -
> > -static int cdns_mhdp_mailbox_write(struct cdns_mhdp_device *mhdp, u8
> > val) -{
> > -     int ret, full;
> > -
> > -     WARN_ON(!mutex_is_locked(&mhdp->mbox_mutex));
> > -
> > -     ret = readx_poll_timeout(readl, mhdp->regs +
> CDNS_MAILBOX_FULL,
> > -                              full, !full, MAILBOX_RETRY_US,
> > -                              MAILBOX_TIMEOUT_US);
> > -     if (ret < 0)
> > -             return ret;
> > -
> > -     writel(val, mhdp->regs + CDNS_MAILBOX_TX_DATA);
> > -
> > -     return 0;
> > -}
> > -
> > -static int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_device
> *mhdp,
> > -                                      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(mhdp);
> > -             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(mhdp) < 0)
> > -                             break;
> > -
> > -             return -EINVAL;
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> > -static int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_device
> *mhdp,
> > -                                    u8 *buff, u16 buff_size)
> > -{
> > -     u32 i;
> > -     int ret;
> > -
> > -     for (i = 0; i < buff_size; i++) {
> > -             ret = cdns_mhdp_mailbox_read(mhdp);
> > -             if (ret < 0)
> > -                     return ret;
> > -
> > -             buff[i] = ret;
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> > -static int cdns_mhdp_mailbox_send(struct cdns_mhdp_device *mhdp, 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(mhdp, header[i]);
> > -             if (ret)
> > -                     return ret;
> > -     }
> > -
> > -     for (i = 0; i < size; i++) {
> > -             ret = cdns_mhdp_mailbox_write(mhdp, message[i]);
> > -             if (ret)
> > -                     return ret;
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> > -static
> > -int cdns_mhdp_reg_read(struct cdns_mhdp_device *mhdp, u32 addr, u32
> > *value) -{
> > -     u8 msg[4], resp[8];
> > -     int ret;
> > -
> > -     put_unaligned_be32(addr, msg);
> > -
> > -     mutex_lock(&mhdp->mbox_mutex);
> > -
> > -     ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_GENERAL,
> > -                                  GENERAL_REGISTER_READ,
> > -                                  sizeof(msg), msg);
> > -     if (ret)
> > -             goto out;
> > -
> > -     ret = cdns_mhdp_mailbox_recv_header(mhdp,
> MB_MODULE_ID_GENERAL,
> > -
> GENERAL_REGISTER_READ,
> > -                                         sizeof(resp));
> > -     if (ret)
> > -             goto out;
> > -
> > -     ret = cdns_mhdp_mailbox_recv_data(mhdp, 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(&mhdp->mbox_mutex);
> > -     if (ret) {
> > -             dev_err(mhdp->dev, "Failed to read register\n");
> > -             *value = 0;
> > -     }
> > -
> > -     return ret;
> > -}
> > -
> > -static
> > -int cdns_mhdp_reg_write(struct cdns_mhdp_device *mhdp, u16 addr, u32
> > val) -{
> > -     u8 msg[6];
> > -     int ret;
> > -
> > -     put_unaligned_be16(addr, msg);
> > -     put_unaligned_be32(val, msg + 2);
> > -
> > -     mutex_lock(&mhdp->mbox_mutex);
> > -
> > -     ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_DP_TX,
> > -                                  DPTX_WRITE_REGISTER,
> sizeof(msg), msg);
> > -
> > -     mutex_unlock(&mhdp->mbox_mutex);
> > -
> > -     return ret;
> > -}
> > -
> > -static
> > -int cdns_mhdp_reg_write_bit(struct cdns_mhdp_device *mhdp, 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(&mhdp->mbox_mutex);
> > -
> > -     ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_DP_TX,
> > -                                  DPTX_WRITE_FIELD, sizeof(field),
> field);
> > -
> > -     mutex_unlock(&mhdp->mbox_mutex);
> > -
> > -     return ret;
> > -}
> > -
> >  static
> >  int cdns_mhdp_dpcd_read(struct cdns_mhdp_device *mhdp,
> >                       u32 addr, u8 *data, u16 len) @@ -2058,7
> +1865,7
> > @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
> >                                    struct drm_bridge_state
> > *bridge_state)  {
> >       struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> > -     u32 resp;
> > +     u32 resp = 0;
> >
> >       dev_dbg(mhdp->dev, "%s\n", __func__);
> >
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> > index bedddd510d17..10c878bf0e63 100644
> > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> > @@ -212,7 +212,6 @@ struct phy;
> >  #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
> >
> >  /* firmware and opcodes */
> >  #define FW_NAME
> "cadence/mhdp8546.bin"
> > diff --git a/include/drm/bridge/cdns-mhdp-mailbox.h
> > b/include/drm/bridge/cdns-mhdp-mailbox.h
> > new file mode 100644
> > index 000000000000..0249322a74b0
> > --- /dev/null
> > +++ b/include/drm/bridge/cdns-mhdp-mailbox.h
> > @@ -0,0 +1,240 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Cadence MHDP Firmware Access API function by Malibox.
> > + *
> > + * Copyright (C) 2022 NXP Semiconductor, Inc.
> > + *
> > + */
> > +#ifndef CDNS_MHDP_MAILBOX_H
> > +#define CDNS_MHDP_MAILBOX_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_GENERAL         0x0A
> > +#define MB_MODULE_ID_DP_TX                   0x01
> > +
> > +/* General Commands */
> > +#define GENERAL_REGISTER_WRITE               0x05
> > +#define GENERAL_REGISTER_READ                0x07
> > +
> > +/* DP TX Command */
> > +#define DPTX_WRITE_FIELD                     0x08
> > +
> > +/* MHDP Firmware access functions by Mailbox */ #define
> > +cdns_mhdp_mailbox_read(__mhdp) \ ({ \
> > +     int ret, empty, val; \
> > +\
> > +     WARN_ON(!mutex_is_locked(&(__mhdp)->mbox_mutex)); \ \
> > +     do {  \
> > +             ret = readx_poll_timeout(readl, (__mhdp)->regs +
> CDNS_MAILBOX_EMPTY,  \
> > +                                      empty, !empty,
> MAILBOX_RETRY_US,  \
> > +                                      MAILBOX_TIMEOUT_US);
> \
> > +             if (ret < 0)  \
> > +                     break;  \
> > +\
> > +             val = readl((__mhdp)->regs + CDNS_MAILBOX_RX_DATA)
> & 0xff; \
> > +     } while (0);  \
> > +\
> > +     (ret < 0) ? ret : val;  \
> > +})
> > +
> > +#define cdns_mhdp_mailbox_write(__mhdp, __val) \ ({ \
> > +     int ret, full;  \
> > +\
> > +     WARN_ON(!mutex_is_locked(&(__mhdp)->mbox_mutex)); \ \
> > +     do {  \
> > +             ret = readx_poll_timeout(readl, (__mhdp)->regs +
> CDNS_MAILBOX_FULL,  \
> > +                                      full, !full,
> MAILBOX_RETRY_US,  \
> > +                                      MAILBOX_TIMEOUT_US);
> \
> > +             if (ret < 0)  \
> > +                     break;  \
> > +\
> > +             writel((__val), (__mhdp)->regs +
> CDNS_MAILBOX_TX_DATA); \
> > +     } while (0);  \
> > +\
> > +     ret; \
> > +})
> > +
> > +#define  cdns_mhdp_mailbox_recv_header(__mhdp, __module_id,
> __opcode,
> > +__req_size) \ ({  \
> > +     u32 mbox_size, i;  \
> > +     u8 header[4];  \
> > +     int ret;  \
> > +\
> > +     do {  \
> > +             /* read the header of the message */ \
> > +             for (i = 0; i < sizeof(header); i++) {  \
> > +                     ret = cdns_mhdp_mailbox_read(__mhdp);  \
> > +                     if (ret < 0)  \
> > +                             break;  \ \
> > +                     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(__mhdp)
> < 0)  \
> > +                                     break;  \ \
> > +                     ret = -EINVAL;  \
> > +             }  \
> > +\
> > +             ret = 0; \
> > +\
> > +     } while (0);  \
> > +\
> > +     ret;  \
> > +})
> > +
> > +#define cdns_mhdp_mailbox_recv_data(mhdp, buff, buff_size)  \ ({  \
> > +     u32 i;  \
> > +     int ret;  \
> > +\
> > +     do {  \
> > +             for (i = 0; i < buff_size; i++) {  \
> > +                     ret = cdns_mhdp_mailbox_read(mhdp);  \
> > +                     if (ret < 0)  \
> > +                             break;  \ \
> > +                     ((u8 *)buff)[i] = ret;  \
> > +             }  \
> > +\
> > +             ret = 0;  \
> > +\
> > +     } while (0);  \
> > +\
> > +     ret; \
> > +})
> > +
> > +#define cdns_mhdp_mailbox_send(mhdp, module_id, opcode, size,
> > +message)  \ ({  \
> > +     u8 header[4];  \
> > +     int ret, i;  \
> > +\
> > +     header[0] = opcode;  \
> > +     header[1] = module_id;  \
> > +     put_unaligned_be16(size, header + 2);  \ \
> > +     do {  \
> > +             for (i = 0; i < sizeof(header); i++) {  \
> > +                     ret = cdns_mhdp_mailbox_write(mhdp,
> header[i]);  \
> > +                     if (ret < 0)  \
> > +                             break;  \
> > +             }  \
> > +\
> > +             for (i = 0; i < size; i++) {  \
> > +                     ret = cdns_mhdp_mailbox_write(mhdp, ((u8
> *)message)[i]);  \
> > +                     if (ret < 0)  \
> > +                             break;;  \
> > +             }  \
> > +             ret = 0;  \
> > +     } while (0);  \
> > +\
> > +     ret;  \
> > +})
> > +
> > +#define cdns_mhdp_reg_read(mhdp, addr, value)  \ ({  \
> > +     u8 msg[4], resp[8];  \
> > +     int ret;  \
> > +\
> > +     put_unaligned_be32(addr, msg);  \ \
> > +     mutex_lock(&mhdp->mbox_mutex);  \ \
> > +     do {  \
> > +             ret = cdns_mhdp_mailbox_send(mhdp,
> MB_MODULE_ID_GENERAL,  \
> > +
> GENERAL_REGISTER_READ,  \
> > +                                          sizeof(msg), msg);  \
> > +             if (ret < 0)  \
> > +                     break;  \
> > +\
> > +             ret = cdns_mhdp_mailbox_recv_header(mhdp,
> MB_MODULE_ID_GENERAL,  \
> > +
> GENERAL_REGISTER_READ,  \
> > +                                                 sizeof(resp));
> \
> > +             if (ret < 0)  \
> > +                     break;  \
> > +\
> > +             ret = cdns_mhdp_mailbox_recv_data(mhdp, resp,
> sizeof(resp));  \
> > +             if (ret < 0)  \
> > +                     break;  \
> > +\
> > +             /* Returned address value should be the same as
> requested */  \
> > +             if (memcmp(msg, resp, sizeof(msg))) {  \
> > +                     ret = -EINVAL;  \
> > +                     break;  \
> > +             }  \
> > +\
> > +             *((u32 *)value) = get_unaligned_be32(resp + 4);  \
> > +                     ret = 0;  \
> > +     } while (0);  \
> > +\
> > +     mutex_unlock(&mhdp->mbox_mutex);  \
> > +     if (ret < 0) {  \
> > +             dev_err(mhdp->dev, "Failed to read register\n");  \
> > +             *((u32 *)value) = 0;  \
> > +     }  \
> > +\
> > +     ret;  \
> > +})
> > +
> > +#define cdns_mhdp_reg_write(mhdp, addr, val)  \ ({  \
> > +     u8 msg[8];  \
> > +     int ret;  \
> > +\
> > +     put_unaligned_be32(addr, msg);  \
> > +     put_unaligned_be32(val, msg + 4);  \ \
> > +     mutex_lock(&mhdp->mbox_mutex);  \ \
> > +     ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_GENERAL,
> \
> > +                                   GENERAL_REGISTER_WRITE,
> > +sizeof(msg), msg);  \ \
> > +     mutex_unlock(&mhdp->mbox_mutex);  \ \
> > +     ret;  \
> > +})
> > +
> > +#define cdns_mhdp_reg_write_bit(mhdp, addr, start_bit, bits_no, 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(&mhdp->mbox_mutex);  \ \
> > +     ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_DP_TX, \
> > +                                  DPTX_WRITE_FIELD,
> sizeof(field),
> > +field);  \ \
> > +     mutex_unlock(&mhdp->mbox_mutex);  \ \
> > +     ret; \
> > +})
> > +
> > +#endif
>