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

["Kumar, Abhash" <a-kumar2@ti.com>]

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