[PATCH v3] drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver

[PATCH v3] drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver

[Abhash Kumar Jha]

Add system suspend and resume hooks to the cdns-mhdp8546 bridge driver.

While resuming we either load the firmware or activate it. Firmware
is loaded only when resuming from a successful suspend-resume cycle.

If resuming due to an aborted suspend, loading the firmware is not
possible because the uCPU's IMEM is only accessible after a reset and the
bridge has not gone through a reset in this case. Hence, Activate the
firmware that is already loaded.

Use genpd_notifier to get the power domain status of the bridge and
accordingly load the firmware.

Additionally, introduce phy_power_off/on to control the power to the phy.

Signed-off-by: Abhash Kumar Jha <a-kumar2@ti.com>
---
Hi,

Changes in v3:
- Register genpd_notifier callback only when genpd is available.
- Mark mhdp->powered_off = false after resume is successful.
- Use SIMPLE_DEV_PM_OPS instead of SET_SYSTEM_SLEEP_PM_OPS.
- Set mhdp->hw_state appropriately in error paths.
- Link to v2: https://lore.kernel.org/all/20260205085233.81678-1-a-kumar2@ti.com/

Changes in v2:
- Fixed defined but not used [-Wunused-function] warning for suspend and resume calls.
- Link to v1: https://lore.kernel.org/all/20260129112016.2448037-1-a-kumar2@ti.com/

Thanks and Regards,
Abhash

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 138 +++++++++++++++++-
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |   4 +
 2 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 99888f8d55736..90d8c037a2220 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -32,6 +32,7 @@
 #include <linux/phy/phy.h>
 #include <linux/phy/phy-dp.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/slab.h>
 #include <linux/wait.h>
 
@@ -2287,6 +2288,121 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
 	drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
 }
 
+static int cdns_mhdp_resume(struct device *dev)
+{
+	struct cdns_mhdp_device *mhdp = dev_get_drvdata(dev);
+	unsigned long rate;
+	int ret;
+
+	ret = clk_prepare_enable(mhdp->clk);
+	if (ret)
+		return ret;
+
+	rate = clk_get_rate(mhdp->clk);
+	writel(rate % 1000000, mhdp->regs + CDNS_SW_CLK_L);
+	writel(rate / 1000000, mhdp->regs + CDNS_SW_CLK_H);
+	writel(~0, mhdp->regs + CDNS_APB_INT_MASK);
+
+	ret = phy_init(mhdp->phy);
+	if (ret) {
+		dev_err(mhdp->dev, "Failed to initialize PHY: %d\n", ret);
+		goto disable_clk;
+	}
+	ret = phy_power_on(mhdp->phy);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "Failed to power on PHY: %d\n", ret);
+		goto error;
+	}
+
+	if (mhdp->powered_off) {
+		ret = cdns_mhdp_load_firmware(mhdp);
+		if (ret)
+			goto phy_off;
+
+		ret = wait_event_timeout(mhdp->fw_load_wq,
+					mhdp->hw_state == MHDP_HW_READY,
+					msecs_to_jiffies(1000));
+		if (ret == 0) {
+			dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n",
+				__func__);
+			ret = -ETIMEDOUT;
+			goto phy_off;
+		}
+	} else {
+		ret = cdns_mhdp_set_firmware_active(mhdp, true);
+		if (ret) {
+			dev_err(mhdp->dev, "Failed to activate firmware (%pe)\n", ERR_PTR(ret));
+			goto phy_off;
+		}
+	}
+
+	mhdp->powered_off = false;
+	return 0;
+
+phy_off:
+	phy_power_off(mhdp->phy);
+error:
+	phy_exit(mhdp->phy);
+disable_clk:
+	clk_disable_unprepare(mhdp->clk);
+
+	return ret;
+}
+
+static int cdns_mhdp_suspend(struct device *dev)
+{
+	struct cdns_mhdp_device *mhdp = dev_get_drvdata(dev);
+	unsigned long timeout = msecs_to_jiffies(100);
+	int ret = 0;
+
+	cancel_work_sync(&mhdp->hpd_work);
+	ret = wait_event_timeout(mhdp->fw_load_wq,
+				 mhdp->hw_state == MHDP_HW_READY,
+				 timeout);
+
+	spin_lock(&mhdp->start_lock);
+	if (mhdp->hw_state != MHDP_HW_READY) {
+		spin_unlock(&mhdp->start_lock);
+		dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", __func__);
+		return -ETIMEDOUT;
+	}
+	mhdp->hw_state = MHDP_HW_STOPPED;
+	spin_unlock(&mhdp->start_lock);
+
+	ret = cdns_mhdp_set_firmware_active(mhdp, false);
+	if (ret) {
+		dev_err(mhdp->dev, "Failed to stop firmware (%pe)\n", ERR_PTR(ret));
+		spin_lock(&mhdp->start_lock);
+		mhdp->hw_state = MHDP_HW_READY;
+		spin_unlock(&mhdp->start_lock);
+		goto error;
+	}
+
+	phy_power_off(mhdp->phy);
+	phy_exit(mhdp->phy);
+	clk_disable_unprepare(mhdp->clk);
+
+	/* if no power domain available, always reload firmware on resume */
+	if (!mhdp->dev->pm_domain)
+		mhdp->powered_off = true;
+
+error:
+	return ret;
+}
+
+static int mhdp_pd_notifier_cb(struct notifier_block *nb,
+			unsigned long action, void *data)
+{
+	struct cdns_mhdp_device *mhdp = container_of(nb, struct cdns_mhdp_device, pd_nb);
+
+	if (action == GENPD_NOTIFY_OFF)
+		mhdp->powered_off = true;
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(cdns_mhdp_pm_ops, cdns_mhdp_suspend, cdns_mhdp_resume);
+
 static int cdns_mhdp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -2401,6 +2517,11 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
 		dev_err(mhdp->dev, "Failed to initialize PHY: %d\n", ret);
 		goto plat_fini;
 	}
+	ret = phy_power_on(mhdp->phy);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "Failed to power on PHY: %d\n", ret);
+		goto phy_exit;
+	}
 
 	/* Initialize the work for modeset in case of link train failure */
 	INIT_WORK(&mhdp->modeset_retry_work, cdns_mhdp_modeset_retry_fn);
@@ -2411,15 +2532,26 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
 
 	ret = cdns_mhdp_load_firmware(mhdp);
 	if (ret)
-		goto phy_exit;
+		goto power_off;
 
 	if (mhdp->hdcp_supported)
 		cdns_mhdp_hdcp_init(mhdp);
 
-	drm_bridge_add(&mhdp->bridge);
+	mhdp->powered_off = false;
+	if (mhdp->dev->pm_domain) {
+		mhdp->pd_nb.notifier_call = mhdp_pd_notifier_cb;
+		ret = dev_pm_genpd_add_notifier(mhdp->dev, &mhdp->pd_nb);
+		if (ret) {
+			dev_err_probe(dev, ret, "failed to add power domain notifier\n");
+			goto power_off;
+		}
+	}
 
+	drm_bridge_add(&mhdp->bridge);
 	return 0;
 
+power_off:
+	phy_power_off(mhdp->phy);
 phy_exit:
 	phy_exit(mhdp->phy);
 plat_fini:
@@ -2457,6 +2589,7 @@ static void cdns_mhdp_remove(struct platform_device *pdev)
 				ERR_PTR(ret));
 	}
 
+	phy_power_off(mhdp->phy);
 	phy_exit(mhdp->phy);
 
 	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->exit)
@@ -2488,6 +2621,7 @@ static struct platform_driver mhdp_driver = {
 	.driver	= {
 		.name		= "cdns-mhdp8546",
 		.of_match_table	= mhdp_ids,
+		.pm = pm_sleep_ptr(&cdns_mhdp_pm_ops),
 	},
 	.probe	= cdns_mhdp_probe,
 	.remove = cdns_mhdp_remove,
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
index 2041ffbc019a5..31dbeaf33ccb6 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -413,6 +413,10 @@ struct cdns_mhdp_device {
 
 	struct cdns_mhdp_hdcp hdcp;
 	bool hdcp_supported;
+
+	/* Power domain status notifier */
+	struct notifier_block pd_nb;
+	bool powered_off;
 };
 
 #define connector_to_mhdp(x) container_of(x, struct cdns_mhdp_device, connector)
-- 
2.34.1

Claude review: drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver
Author: Abhash Kumar Jha <a-kumar2@ti.com>
Patches: 1
Reviewed: 2026-06-04T14:19:35.342050

---

This is a v3 single patch adding system suspend/resume support to the Cadence MHDP8546 DisplayPort bridge driver. The approach uses a genpd notifier to track whether the power domain was actually powered off during suspend, which determines whether firmware needs to be fully reloaded (after power loss) or just reactivated (after an aborted suspend). The patch also adds `phy_power_on`/`phy_power_off` calls that were previously missing from probe/remove.

The general design is reasonable, but there are several issues ranging from a missing cleanup path (notifier removal) to clock management conflicts with `devm_clk_get_enabled`, a missing `hw_state` update on the resume error path, and inconsistent error handling compared to the existing remove code.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver

[Claude Code Review Bot]

Patch Review

**Critical: Missing notifier removal in remove path**

The notifier is registered in probe via `dev_pm_genpd_add_notifier()`, but `cdns_mhdp_remove()` never calls `dev_pm_genpd_remove_notifier()`. This will leave a dangling notifier pointing at freed memory after the driver is removed.

**Critical: Clock management conflicts with `devm_clk_get_enabled()`**

The probe function at line 2275 of the current tree uses `devm_clk_get_enabled()`, which means the clock is managed by devres and will be automatically disabled/unprepared at device removal. But `cdns_mhdp_suspend()` calls `clk_disable_unprepare(mhdp->clk)` and `cdns_mhdp_resume()` calls `clk_prepare_enable(mhdp->clk)`. This creates a conflict: on driver removal after a resume, devres will try to disable an already-managed clock, and the enable/disable count will be unbalanced. The driver should either switch to `devm_clk_get()` (not the `_enabled` variant) and manage clock enable/disable explicitly, or find another approach for suspend/resume clock handling.

**Bug: `hw_state` not set to `MHDP_HW_STOPPED` on resume error paths**

In `cdns_mhdp_resume()`, if `cdns_mhdp_load_firmware()` or `wait_event_timeout` or `cdns_mhdp_set_firmware_active()` fails, the function returns an error but `mhdp->hw_state` is never updated. The suspend function sets `hw_state = MHDP_HW_STOPPED`, and it stays that way on resume failure, but the comment block in the header file (line 317) says:
```
MHDP_HW_READY <-> MHDP_HW_STOPPED
```
If resume partially fails (e.g. firmware loaded but timeout waiting for ready), the state machine is left in an inconsistent position. At minimum, on resume failure, `hw_state` should be explicitly set to `MHDP_HW_STOPPED` so a subsequent suspend/resume cycle doesn't malfunction.

**Bug: `wait_event_timeout` return value mishandled in suspend**

```c
ret = wait_event_timeout(mhdp->fw_load_wq,
			 mhdp->hw_state == MHDP_HW_READY,
			 timeout);

spin_lock(&mhdp->start_lock);
if (mhdp->hw_state != MHDP_HW_READY) {
```

`wait_event_timeout` returns 0 on timeout and >0 on success. Here `ret` is set to the timeout return value (positive on success), but then later `ret` is overwritten by `cdns_mhdp_set_firmware_active()`. However, if the wait succeeds but `hw_state == MHDP_HW_READY`, then the code proceeds and `ret` is still the positive jiffies remaining value. Although it gets overwritten by `cdns_mhdp_set_firmware_active()` which returns 0 on success, it would be cleaner to reset `ret = 0` after the wait succeeds, as the existing `int ret = 0` initialization is dead. Same issue exists in `cdns_mhdp_resume()`:

```c
ret = wait_event_timeout(mhdp->fw_load_wq,
			mhdp->hw_state == MHDP_HW_READY,
			msecs_to_jiffies(1000));
if (ret == 0) {
	...
	ret = -ETIMEDOUT;
	goto phy_off;
}
```

If `wait_event_timeout` returns a positive value (success), `ret` is now some positive jiffies count but the success path does `mhdp->powered_off = false; return 0;` — so this particular case is fine. But it is fragile and inconsistent. In the suspend case, if the success path after `cdns_mhdp_set_firmware_active` had any error checks using `ret`, the stale positive value could leak through.

**Issue: `cancel_work_sync(&mhdp->hpd_work)` in suspend but not `modeset_retry_work`**

The suspend function cancels `hpd_work` but not `modeset_retry_work`. The remove function cancels both (`cancel_work_sync(&mhdp->modeset_retry_work)` and `flush_work(&mhdp->hpd_work)`). If a modeset retry is pending during suspend, it could fire after the firmware is deactivated and the PHY is powered off, causing failures.

**Issue: No bridge state management during suspend/resume**

The suspend/resume doesn't interact with the DRM bridge state at all. If the display is active when suspend happens, resume will reload/activate firmware but won't restore the display mode, link training, or any active stream state. While this might be handled at a higher level by the DRM framework's atomic resume, it's worth considering whether `cdns_mhdp_bridge_hpd_enable()` should be called in resume (as `cdns_mhdp_fw_activate()` does at line 701) to re-enable HPD interrupt handling.

**Minor: `DEFINE_SIMPLE_DEV_PM_OPS` placement**

The `DEFINE_SIMPLE_DEV_PM_OPS` macro is placed between the suspend/resume functions and `cdns_mhdp_probe()`. This is fine functionally, but conventionally PM ops definitions are placed closer to the driver struct.

**Minor: Missing `NOTIFY_DONE` return in notifier**

```c
static int mhdp_pd_notifier_cb(struct notifier_block *nb,
			unsigned long action, void *data)
{
	struct cdns_mhdp_device *mhdp = container_of(nb, struct cdns_mhdp_device, pd_nb);

	if (action == GENPD_NOTIFY_OFF)
		mhdp->powered_off = true;

	return 0;
}
```

Returning `0` is equivalent to `NOTIFY_DONE`, so it works, but using the symbolic constant `NOTIFY_DONE` is conventional and clearer.

**Minor: Race on `powered_off` flag**

`mhdp->powered_off` is a plain `bool` accessed from the genpd notifier callback (which may run in arbitrary context) and from the suspend/resume paths. There is no locking around it. While it's likely benign since the writes are not concurrent in practice (the notifier fires during the power-off sequence, and resume reads it after), it would be more robust to use `start_lock` or at minimum make it atomic.

---
Generated by Claude Code Patch Reviewer