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

[Kumar, Abhash]

Hi,

On 10/1/2026 1:10 PM, Tomi Valkeinen wrote:
> Hi,
>
> On 29/06/2026 13:32, abhash wrote:
> > Hi Tomi,
> >
> > Thanks for the review.
> >
> > On 18/06/26 14:19, Tomi Valkeinen wrote:
> > > 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?
> >
> > The driver lacked support for suspend-resume as stated in the todo on 
> > the driver, So the patch adds this improvement.
>
> The patch doesn't remove any todo lines. Was that just a miss, or is 
> there more to add wrt. PM?
Yeah, i will remove the TODO in the next revision.
> What does it mean it didn't support PM? Does the driver not work after 
> suspend-resume cycle? Or does the driver prevent a proper suspend? 

When we resume, the cdns_mhdp_link_up function fails.

Failure log at resume:

[  55.329452] tidss 4a00000.dss: PM: calling tidss_resume [tidss] @ 
1053, parent: bus@100000

[ 183.381228] cdns-mhdp8546 a000000.bridge: Failed to read receiver 
capabilities

[ 191.391548] cdns-mhdp8546 a000000.bridge: get block[0] edid failed: -110

[ 193.392713] cdns-mhdp8546 a000000.bridge: Failed to read register

[ 261.415544] cdns-mhdp8546 a000000.bridge: Failed to read register

[ 263.946153] tidss 4a00000.dss: Timeout waiting for framedone on crtc 0

[ 391.991358] cdns-mhdp8546 a000000.bridge: Failed to read receiver 
capabilities

[ 392.026847] tidss 4a00000.dss: PM: tidss_resume [tidss] returned 0 
after 336689133 usecs

I guess it is fair to say that the driver does not work after 
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.
> > >
> > > 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? 
> >
> > The phy device could be powered off while resuming. So we are 
> > explicitly powering it on.
> >
> > The phy driver api also recommends to always call phy_init() first 
> > followed by a phy_power_on().
> >
> > "Some PHY drivers may not implement `phy_init` or `phy_power_on`, but 
> > controllers should always call these functions to be compatible with 
> > other PHYs"
>
> It still sounds like a separate patch to me: the current driver is 
> missing phy_power_on/off from the probe/remove functions.
I will add that as a separate patch
> > > 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.
> > Thanks for the suggestion, I will use the runtime PM instead.
> >
> > > The second thing is the PD notifier. Is there really no way we can 
> > > see the state from the MDHP IP registers? 
> >
> > The other way that i found was to read the MHDP KEEP_ALIVE_p register 
> > twice to know if the firmware is incrementing the counter.
> >
> > Based on that we can decide if the bridge is active or not. Do you 
> > think this approach would be okay over the PD notifier?
> I think it would be best to be able somehow to ask this from the HW to 
> find the true state, instead of guessing it second hand from the PD 
> notifier (which also doesn't tell us the initial HW state at probe).
>
> KEEP_ALIVE_p sounds fine. Or what does the mdhp IP do if you send a 
> message to the firmware when it's not up? Say, if you always do 
> cdns_mhdp_set_firmware_active, what happens if the FW has not been 
> loaded? I would guess that there's a timeout, and that could be used 
> to find out the FW is not up.
Yes, there is a 2 second timeout if we send a message and the firmware 
is not up. I would prefer the KEEP_ALIVE_p method, as that is a better 
indication of the

firmware being active.

> Also, if the IMEM is not accessible and you load the FW, what happens?

If IMEM is not accessible and we load the FW, the kernel crashes due to 
S-error.


Thanks,

Abhash