Skip to contentSkip to conversation

Add no-hpd property to the cadence bridge

PATCH New Yashas D 2 participants 4 messages Oct 9

Subsystems (4)
lore
Share & tools
lore

Yashas D

author
#0 lore raw

v7 cover Add no-hpd property to the cadence bridge

This series adds 'no-hpd' device tree property support to the Cadence MHDP8546 bridge driver for boards where the HPD line cannot be used for hotplug detection.

On TI J721S2 EVMs, the DP0 HPD resistor is DNI from factory, so the HPD signal is not physically connected to SoC pin AA24 by default. AA24 must still be in DP0_HPD mux mode for the MHDP firmware to operate.

When 'no-hpd' is set, DRM_BRIDGE_OP_HPD is omitted and the framework falls back to polling .detect() every ~10 seconds. Presence is detected by reading DP_DPCD_REV (0x000) over AUX, with an early exit when the monitor is stably connected to avoid re-programming the stream each poll.

Changes since v6:
- Reverted back to dev_dbg instead of dev_err for the normal hpd case.
- Picked up Acked-by from Krzysztof Kozlowski from v6 on the bindings patch.

Link to v6: https://lore.kernel.org/all/[email protected]/

Rahul T R (2):
dt-bindings: display: bridge: cdns,mhdp8546: Add no-hpd property to
the cadence bridge
drm: bridge: cdns-mhdp8546: Add no-hpd property

 .../display/bridge/cdns,mhdp8546.yaml         | 11 +++
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 69 +++++++++++++++++--
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
 3 files changed, 75 insertions(+), 6 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v7 1/2 dt-bindings: display: bridge: cdns,mhdp8546: Add no-hpd property to the cadence bridge

From: Rahul T R <[email protected]>

The mhdp bridge can work without its HPD pin hooked up to the connector, but the current bridge driver throws an error when hpd line is not connected to the connector. For such cases, we need an indication for no-hpd, using which we can bypass the hpd detection and instead use the auxiliary channels connected to the DP connector to confirm the connection.
So add no-hpd property to the bindings, to disable hpd when not connected or cannot be used for hotplug detection.

---
 .../bindings/display/bridge/cdns,mhdp8546.yaml        | 11 +++++++++++
 1 file changed, 11 insertions(+)
Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml +11 -0
index c2b369456e4e2..56ce3f65ff495 100644
@@ -57,6 +57,17 @@ properties:
   interrupts:
     maxItems: 1
 
+  no-hpd:
+    type: boolean
+    description:
+      Set if the HPD line on the bridge isn't physically connected to the
+      DisplayPort connector or cannot be used for hotplug detection.
+
+      Valid use cases include HPD pin not routed to the connector on the PCB,
+      HPD signal muxed with another function on the SoC making it unavailable
+      for hotplug detection, or hardware design where HPD cannot reliably
+      detect monitor presence.
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH v7 2/2 drm: bridge: cdns-mhdp8546: Add no-hpd property

In reply to Yashas D

From: Rahul T R <[email protected]>

Add a 'no-hpd' boolean property to support boards where the HPD line cannot be used for hotplug detection due to hardware limitations.

On TI J721S2 EVMs, the DP0 HPD resistor is not populated from factory (DNI), so the HPD signal is not physically connected to SoC pin AA24 by default which makes HPD unavailable but AA24 must be in DP0_HPD mux mode for the MHDP firmware to operate.

When this property is set, the driver uses auxiliary channel (AUX) DPCD reads to detect monitor presence instead of hardware HPD signals. The DRM framework polls the connection status via the .detect() callback, providing hotplug detection without requiring the HPD pin.

Valid use cases:
- HPD pin not routed to connector on PCB
- HPD signal muxed with another function on SoC
- Hardware designs where HPD cannot reliably detect monitor presence

---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 69 +++++++++++++++++--
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
 2 files changed, 64 insertions(+), 6 deletions(-)
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +63 -6
index f47717bd10798..b02147a725a2d 100644
@@ -53,6 +53,8 @@
 #include "cdns-mhdp8546-hdcp.h"
 #include "cdns-mhdp8546-j721e.h"
 
+static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp);
+
 static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
@@ -698,7 +700,9 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
 	 * MHDP_HW_STOPPED happens only due to driver removal when
 	 * bridge should already be detached.
 	 */
-	cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
+
+	if (!mhdp->no_hpd)
+		cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
 
 	spin_unlock(&mhdp->start_lock);
 
@@ -739,7 +743,13 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
 	spin_lock(&mhdp->start_lock);
 	bridge_attached = mhdp->bridge_attached;
 	spin_unlock(&mhdp->start_lock);
-	if (bridge_attached)
+
+	if (!bridge_attached)
+		return;
+
+	if (mhdp->no_hpd)
+		cdns_mhdp_update_link_status(mhdp);
+	else
 		drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
 }
 
@@ -791,7 +801,6 @@ static ssize_t cdns_mhdp_transfer(struct drm_dp_aux *aux,
 			dev_dbg(mhdp->dev,
 				"Failed to read DPCD addr %u\n",
 				msg->address);
-
 			return ret;
 		}
 	}
@@ -1523,6 +1532,19 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
 
 	spin_unlock(&mhdp->start_lock);
 
+	if (mhdp->no_hpd) {
+		/*
+		 * In no-hpd mode there are no HPD interrupts to trigger
+		 * detection. If firmware is already ready, do the initial
+		 * AUX poll immediately. Otherwise fw_cb() will call
+		 * cdns_mhdp_update_link_status() once firmware finishes
+		 * loading and sees bridge_attached is true.
+		 */
+		if (hw_ready)
+			cdns_mhdp_update_link_status(mhdp);
+		return 0;
+	}
+
 	/* Enable SW event interrupts */
 	if (hw_ready)
 		cdns_mhdp_bridge_hpd_enable(bridge);
@@ -2012,6 +2034,16 @@ static enum drm_connector_status
 cdns_mhdp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
 {
 	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+	bool hw_ready;
+
+	if (mhdp->no_hpd) {
+		spin_lock(&mhdp->start_lock);
+		hw_ready = mhdp->hw_state == MHDP_HW_READY;
+		spin_unlock(&mhdp->start_lock);
+
+		if (hw_ready)
+			cdns_mhdp_update_link_status(mhdp);
+	}
 
 	return cdns_mhdp_detect(mhdp);
 }
@@ -2100,7 +2132,29 @@ static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp)
 
 	mutex_lock(&mhdp->link_mutex);
 
-	mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+	if (mhdp->no_hpd) {
+		u8 rev;
+
+		/*
+		 * Use a side-effect-free capability register for presence
+		 * detection. A successful AUX read means a sink is present;
+		 * we do not need link training status registers here.
+		 */
+		ret = drm_dp_dpcd_read(&mhdp->aux, DP_DPCD_REV, &rev, 1);
+		mhdp->plugged = (ret == 1);
+		ret = mhdp->plugged ? 0 : -EIO;
+		hpd_pulse = false;
+
+		/*
+		 * If the monitor is still connected and the link is already
+		 * up, there is nothing to do. Avoid falling through to
+		 * cdns_mhdp_sst_enable() on every poll cycle.
+		 */
+		if (mhdp->plugged && old_plugged && mhdp->link_up)
+			goto out;
+	} else {
+		mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+	}
 
 	if (!mhdp->plugged) {
 		cdns_mhdp_link_down(mhdp);
@@ -2288,6 +2342,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
 	mhdp->aux.dev = dev;
 	mhdp->aux.transfer = cdns_mhdp_transfer;
 
+	mhdp->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
+
 	mhdp->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(mhdp->regs)) {
 		dev_err(dev, "Failed to get memory resource\n");
@@ -2359,8 +2415,9 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
 	mhdp->display_fmt.bpc = 8;
 
 	mhdp->bridge.of_node = pdev->dev.of_node;
-	mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
-			   DRM_BRIDGE_OP_HPD;
+	mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+	if (!mhdp->no_hpd)
+		mhdp->bridge.ops |= DRM_BRIDGE_OP_HPD;
 	mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 
 	ret = phy_init(mhdp->phy);
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h +1 -0
index b53335b0d22c3..24ffb732a207f 100644
@@ -388,6 +388,7 @@ struct cdns_mhdp_device {
 
 	bool link_up;
 	bool plugged;
+	bool no_hpd;
 
 	/*
 	 * "start_lock" protects the access to bridge_attached and
Signature · 1 line
2.34.1
#3 lore raw

Re: [PATCH v7 2/2] drm: bridge: cdns-mhdp8546: Add no-hpd property

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/bridge: cdns-mhdp8546: no-hpd property inadvertently disables all SW event interrupts…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/bridge: cdns-mhdp8546: no-hpd property inadvertently disables all SW event interrupts
- [Medium] drm/bridge: cdns-mhdp8546: .detect polling forces link training when disabled --

commit 74d172bfd85fd1324471808ee819e4a124d26946
Author: Rahul T R <[email protected]>
Subject: drm: bridge: cdns-mhdp8546: Add no-hpd property

Adds a no-hpd boolean property for boards where the hardware HPD line is unavailable. When set, the driver uses DPCD AUX reads to poll monitor presence via the .detect() callback instead of relying on HPD interrupts.

Quoted 4 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +0 -0
index f47717bd10798..b02147a725a2d 100644

[ ... ]

drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c static int cdns_mhdp_fw_activate(const struct firmware *fw, · line 705

Quoted 8 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +3 -1
@@ -698,7 +700,9 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
 	 * MHDP_HW_STOPPED happens only due to driver removal when
 	 * bridge should already be detached.
 	 */
-	cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
+
+	if (!mhdp->no_hpd)
+		cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);

[Severity: High]
When no-hpd is set, cdns_mhdp_bridge_hpd_enable() is bypassed here.

Looking at cdns_mhdp_bridge_hpd_enable(), it unmasks all SW events, not just HPD:

static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)

{
        ...
        if (mhdp->bridge_attached)
                writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
                       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
                       mhdp->regs + CDNS_APB_INT_MASK);
}

Since this bit controls all software event notifications from the firmware, won't bypassing this cause features like HDCP to unconditionally time out when they wait for SW events?

Quoted 3 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +0 -0
 	spin_unlock(&mhdp->start_lock);
 

[ ... ]

Quoted 6 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +2 -0
@@ -1523,6 +1532,19 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
 
 	spin_unlock(&mhdp->start_lock);
 
+	if (mhdp->no_hpd) {
+		/*

drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c static int cdns_mhdp_attach(struct drm_bridge *bridge, · line 1546

Quoted 10 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +10 -0
+		 * In no-hpd mode there are no HPD interrupts to trigger
+		 * detection. If firmware is already ready, do the initial
+		 * AUX poll immediately. Otherwise fw_cb() will call
+		 * cdns_mhdp_update_link_status() once firmware finishes
+		 * loading and sees bridge_attached is true.
+		 */
+		if (hw_ready)
+			cdns_mhdp_update_link_status(mhdp);
+		return 0;
+	}

[Severity: High]
This early return also bypasses the SW event interrupt unmasking during bridge attach. Should CDNS_APB_INT_MASK_SW_EVENT_INT be unmasked independently of the HPD configuration?

Quoted 4 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +1 -0
+
 	/* Enable SW event interrupts */
 	if (hw_ready)
 		cdns_mhdp_bridge_hpd_enable(bridge);

[ ... ]

Quoted 15 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +10 -1
@@ -2100,7 +2132,29 @@ static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp)
 
 	mutex_lock(&mhdp->link_mutex);
 
-	mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+	if (mhdp->no_hpd) {
+		u8 rev;
+
+		/*
+		 * Use a side-effect-free capability register for presence
+		 * detection. A successful AUX read means a sink is present;
+		 * we do not need link training status registers here.
+		 */
+		ret = drm_dp_dpcd_read(&mhdp->aux, DP_DPCD_REV, &rev, 1);
+		mhdp->plugged = (ret == 1);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp) · line 2154

Quoted 10 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +10 -0
+		ret = mhdp->plugged ? 0 : -EIO;
+		hpd_pulse = false;
+
+		/*
+		 * If the monitor is still connected and the link is already
+		 * up, there is nothing to do. Avoid falling through to
+		 * cdns_mhdp_sst_enable() on every poll cycle.
+		 */
+		if (mhdp->plugged && old_plugged && mhdp->link_up)
+			goto out;

[Severity: Medium]
During DPMS off or atomic_disable, the driver correctly powers down the link and sets mhdp->link_up to false.

When the 10-second polling calls .detect() while the bridge is disabled, mhdp->link_up will be false. The short-circuit condition here evaluates to false, causing the code to fall through to:

	if (!mhdp->link_up) {
		ret = cdns_mhdp_link_up(mhdp);
		if (ret < 0)
			goto out;
	}

Will this unconditionally perform full hardware link training and issue DP_SET_POWER_D0 on every poll cycle, defeating power management while the bridge is logically disabled?

Quoted 6 lines
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +3 -0
+	} else {
+		mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+	}
 
 	if (!mhdp->plugged) {
 		cdns_mhdp_link_down(mhdp);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2