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
