Re: [PATCH v3] drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver
From: Tomi Valkeinen
Date: Thu Jun 18 2026 - 04:55:08 EST
Hi,
On 01/06/2026 12:50, Abhash Kumar Jha wrote:
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.
It's not clear from the patch if this is a fix or improvement. It sounds a bit like a fix, but it doesn't mention any kind of issue in the driver. So, why is this patch needed?
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.
If you write "also" or "additionally" or such in a commit desc, you should stop and think if that part should actually be a separate patch. Also, why is that change needed?
Overall, this sounds fragile/hacky to me.
The first thing is that usually you shouldn't use system suspend/resume in a bridge driver. When a system suspend happend, the display pipeline will be disabled, so this driver will get an atomic_disable() call, and enable when resuming. You can use runtime PM hooks if you need resume/suspend hooks.
The second thing is the PD notifier. Is there really no way we can see the state from the MDHP IP registers?
Tomi
Signed-off-by: Abhash Kumar Jha <a-kumar2@xxxxxx>
---
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@xxxxxx/
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@xxxxxx/
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)