Hi Lucas,
On Wed, 2024-04-17 at 10:00 +0200, Lucas Stach wrote:
Hi Vitor,
Am Dienstag, dem 16.04.2024 um 17:08 +0100 schrieb Vitor Soares:
On Tue, 2024-04-16 at 11:53 +0100, Vitor Soares wrote:
++ Peng Fan peng.fan@nxp.com
Greetings,
On Wed, 2024-04-10 at 12:01 +0100, Vitor Soares wrote:
Hi Lucas,
On Tue, 2024-04-09 at 17:44 +0100, Vitor Soares wrote:
On Tue, 2024-04-09 at 16:36 +0200, Lucas Stach wrote:
Am Dienstag, dem 09.04.2024 um 14:22 +0100 schrieb Vitor Soares: > Hi Lucas, > > Thanks for your feedback. > > On Tue, 2024-04-09 at 11:13 +0200, Lucas Stach wrote: > > Hi Vitor, > > > > Am Dienstag, dem 09.04.2024 um 09:58 +0100 schrieb Vitor > > Soares: > > > From: Vitor Soares vitor.soares@toradex.com > > > > > > The pgc_vpu_* nodes miss the reference to the power domain > > > parent, > > > leading the system to hang during the resume. > > > > > This change is not correct. The vpumix domain is controlled > > through > > the > > imx8mm-vpu-blk-ctrl and must not be directly triggered by the > > child > > domains in order to guarantee proper power sequencing. > > > > If the sequencing is incorrect for resume, it needs to be > > fixed > > in > > the > > blk-ctrl driver. I'll happily assist if you have any > > questions > > about > > this intricate mix between GPC and blk-ctrl hardware/drivers. > > I'm new into the topic, so I tried to follow same approach as > in > imx8mp > DT. > That's a good hint, the 8MP VPU GPC node additions missed my radar. The direct dependency there between the GPC domains is equally wrong.
> I also checked the imx8mq DT and it only have one domain for > the > VPU in the GPC. It seem blk-ctrl also dependes on pgc_vpu_* to > work > properly. > > The blk-ctrl driver hangs on imx8m_blk_ctrl_power_on() when > access > the > ip registers for the soft reset. I tried to power-up the before > the > soft reset, but it didn't work. > The runtime_pm_get_sync() at the start of that function should ensure that bus GPC domain aka vpumix is powered up. Can you check if that is happening?
I checked bc->bus_power_dev->power.runtime_status and it is RPM_ACTIVE.
Am I looking to on the right thing? It is RPM_ACTIVE event before runtime_pm_get_sync().
During the probe I can see that bus_power_dev->power.runtime_status = RPM_SUSPENDED and then vpumix is powered up on GPC driver.
On resume routine I can't see this flow. bus_power_dev-
power.runtime_status = RPM_ACTIVE and vpumix end up not being powered-
up.
I checked the suspend flow and the GPC tries to poweroff vpumix.
My understanding is that when resuming the 38310000.video-codec, the vpumix isn't powered up. It happens because runtime_status and runtime_last_status = RPM_ACTIVE.
I tried to change blk-ctrl suspend routine to force the runtime_status = RPM_SUSPENDED, but the system ended up hanging on another device.
From the comment in blk-ctrl suspend, we rely on PM_SLEEP code that iterates over dpm_list for suspend/resume. I did look at the dpm_list, and it changes the order on every boot.
With all the tests, I also found that the system randomly hangs on dispblk-lcdif suspend. I have confirmed this device is in a different place in the dpm_list (not sure if it is the root cause). I haven't understood how blk-ctrl ensures the correct order there yet.
Random order of the DPM list seems like a good find to investigate further.
Taking the following dpm_list excerpt: idx - device
... 191 - imx-pgc-domain.7 192 - imx-pgc-domain.8 193 - imx-pgc-domain.9 194 - 38330000.blk-ctrl 195 - 38310000.video-codec 196 - 38300000.video-codec ... 205 - genpd:0:38330000.blk-ctrl 206 - genpd:1:38330000.blk-ctrl 207 - genpd:2:38330000.blk-ctrl 208 - genpd:3:38330000.blk-ctrl
Shouldn't genpd devices be before 38330000.blk-ctrl? As their power domain is GPC and the blk-ctrl power domain is genpd.
I did the following change to have genpd device before 38330000.blk-ctrl on dpm_list and it did work.
diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c index ca942d7929c2..0f1471dcd4e8 100644 --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c @@ -220,6 +220,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(bc->bus_power_dev), "failed to attach power domain "bus"\n"); } + device_move(dev, bc->bus_power_dev, DPM_ORDER_PARENT_BEFORE_DEV); for (i = 0; i < bc_data->num_domains; i++) { const struct imx8m_blk_ctrl_domain_data *data = &bc_data->domains[i]; @@ -268,6 +269,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) data->gpc_name); goto cleanup_pds; } + device_move(dev, domain->power_dev, DPM_ORDER_PARENT_BEFORE_DEV); domain->genpd.name = data->name; domain->genpd.power_on = imx8m_blk_ctrl_power_on;
any concern about this approach?
I'm a bit uncomfortable with calling such a low-level function from this driver. Also we don't really want to move the device to a new parent, but just want to ensure proper order on the dpm list. Adding a device_link between the devices seems like the better way to do so.
Thanks for your feedback.
I have tested with device_link_add() and it is working. I will prepare a new patch with this change.
Best regards, Vitor Soares
Regards, Lucas
Best regards, Vitor Soares
Regards, Lucas
> Do you have an idea how we can address this within blk-ctrl? > > Best regards, > Vitor