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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi,

On 06/10/2026 11:49, Kumar, Abhash wrote:
> 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

But that's a driver imposed timeout, and it sounds rather arbitrary (and 
bad). How quickly does the firmware usually reply? What would be the 
situation where the firmware takes e.g. 1.5s to reply? I would guess 
that if the FW updates the KEEP_ALIVE_p every N us, it would also reply 
to us in that time.

In any case, changing MAILBOX_TIMEOUT_US would be another patch. 
KEEP_ALIVE_p sounds fine if you know how often the FW updates it.

  Tomi