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
