linux-kernel - Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <564fa534b32f4a6e96da6752f531fc7447ec633d.camel@gmail.com>
Date: Tue, 16 Apr 2024 11:53:26 +0100
From: Vitor Soares <ivitro@...il.com>
To: Lucas Stach <l.stach@...gutronix.de>, Rob Herring <robh@...nel.org>, 
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,  Sascha Hauer
 <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Peng Fan <peng.fan@....com>
Cc: Vitor Soares <vitor.soares@...adex.com>, devicetree@...r.kernel.org, 
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power
 domain parent

++ Peng Fan <peng.fan@....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@...adex.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. 

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.

Best regards,
Vitor Soares

> 
> > 
> > 
> > > 
> > > Regards,
> > > Lucas
> > > 
> > > > Do you have an idea how we can address this within blk-ctrl?
> > > > 
> > > > Best regards,
> > > > Vitor
> > 
>