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

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

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?

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?
>>> 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.

>> 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.

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

  Tomi