[PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

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

As these PU domains are nested inside the vpumix domain, let's reference
it accordingly. After this change, the suspend/resume is working.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: <stable@vger.kernel.org>
Closes: https://lore.kernel.org/all/fccbb040330a706a4f7b34875db1d896a0bf81c8.camel@gmail.com/
Fixes: d39d4bb15310 ("arm64: dts: imx8mm: add GPC node")
Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 8a1b42b94dce..97d0c6d23ad8 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -739,16 +739,19 @@ pgc_vpumix: power-domain@6 {
 					pgc_vpu_g1: power-domain@7 {
 						#power-domain-cells = <0>;
 						reg = <IMX8MM_POWER_DOMAIN_VPUG1>;
+						power-domains = <&pgc_vpumix>;
 					};
 
 					pgc_vpu_g2: power-domain@8 {
 						#power-domain-cells = <0>;
 						reg = <IMX8MM_POWER_DOMAIN_VPUG2>;
+						power-domains = <&pgc_vpumix>;
 					};
 
 					pgc_vpu_h1: power-domain@9 {
 						#power-domain-cells = <0>;
 						reg = <IMX8MM_POWER_DOMAIN_VPUH1>;
+						power-domains = <&pgc_vpumix>;
 					};
 
 					pgc_dispmix: power-domain@10 {
-- 
2.34.1

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[Lucas Stach]

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.

Regards,
Lucas

> As these PU domains are nested inside the vpumix domain, let's reference
> it accordingly. After this change, the suspend/resume is working.
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: <stable@vger.kernel.org>
> Closes: https://lore.kernel.org/all/fccbb040330a706a4f7b34875db1d896a0bf81c8.camel@gmail.com/
> Fixes: d39d4bb15310 ("arm64: dts: imx8mm: add GPC node")
> Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 8a1b42b94dce..97d0c6d23ad8 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -739,16 +739,19 @@ pgc_vpumix: power-domain@6 {
>  					pgc_vpu_g1: power-domain@7 {
>  						#power-domain-cells = <0>;
>  						reg = <IMX8MM_POWER_DOMAIN_VPUG1>;
> +						power-domains = <&pgc_vpumix>;
>  					};
>  
>  					pgc_vpu_g2: power-domain@8 {
>  						#power-domain-cells = <0>;
>  						reg = <IMX8MM_POWER_DOMAIN_VPUG2>;
> +						power-domains = <&pgc_vpumix>;
>  					};
>  
>  					pgc_vpu_h1: power-domain@9 {
>  						#power-domain-cells = <0>;
>  						reg = <IMX8MM_POWER_DOMAIN_VPUH1>;
> +						power-domains = <&pgc_vpumix>;
>  					};
>  
>  					pgc_dispmix: power-domain@10 {

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

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

Do you have an idea how we can address this within blk-ctrl?

Best regards,
Vitor

> 
> Regards,
> Lucas
> 
> > As these PU domains are nested inside the vpumix domain, let's
> > reference
> > it accordingly. After this change, the suspend/resume is working.
> > 
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: <stable@vger.kernel.org>
> > Closes:
> > https://lore.kernel.org/all/fccbb040330a706a4f7b34875db1d896a0bf81c8.camel@gmail.com/
> > Fixes: d39d4bb15310 ("arm64: dts: imx8mm: add GPC node")
> > Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index 8a1b42b94dce..97d0c6d23ad8 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -739,16 +739,19 @@ pgc_vpumix: power-domain@6 {
> >                                         pgc_vpu_g1: power-domain@7
> > {
> >                                                 #power-domain-cells
> > = <0>;
> >                                                 reg =
> > <IMX8MM_POWER_DOMAIN_VPUG1>;
> > +                                               power-domains =
> > <&pgc_vpumix>;
> >                                         };
> >  
> >                                         pgc_vpu_g2: power-domain@8
> > {
> >                                                 #power-domain-cells
> > = <0>;
> >                                                 reg =
> > <IMX8MM_POWER_DOMAIN_VPUG2>;
> > +                                               power-domains =
> > <&pgc_vpumix>;
> >                                         };
> >  
> >                                         pgc_vpu_h1: power-domain@9
> > {
> >                                                 #power-domain-cells
> > = <0>;
> >                                                 reg =
> > <IMX8MM_POWER_DOMAIN_VPUH1>;
> > +                                               power-domains =
> > <&pgc_vpumix>;
> >                                         };
> >  
> >                                         pgc_dispmix:
> > power-domain@10 {
> 

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[Lucas Stach]

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?

Regards,
Lucas

> Do you have an idea how we can address this within blk-ctrl?
> 
> Best regards,
> Vitor
> 
> > 
> > Regards,
> > Lucas
> > 
> > > As these PU domains are nested inside the vpumix domain, let's
> > > reference
> > > it accordingly. After this change, the suspend/resume is working.
> > > 
> > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > Cc: <stable@vger.kernel.org>
> > > Closes:
> > > https://lore.kernel.org/all/fccbb040330a706a4f7b34875db1d896a0bf81c8.camel@gmail.com/
> > > Fixes: d39d4bb15310 ("arm64: dts: imx8mm: add GPC node")
> > > Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> > > ---
> > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > index 8a1b42b94dce..97d0c6d23ad8 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > @@ -739,16 +739,19 @@ pgc_vpumix: power-domain@6 {
> > >                                         pgc_vpu_g1: power-domain@7
> > > {
> > >                                                 #power-domain-cells
> > > = <0>;
> > >                                                 reg =
> > > <IMX8MM_POWER_DOMAIN_VPUG1>;
> > > +                                               power-domains =
> > > <&pgc_vpumix>;
> > >                                         };
> > >  
> > >                                         pgc_vpu_g2: power-domain@8
> > > {
> > >                                                 #power-domain-cells
> > > = <0>;
> > >                                                 reg =
> > > <IMX8MM_POWER_DOMAIN_VPUG2>;
> > > +                                               power-domains =
> > > <&pgc_vpumix>;
> > >                                         };
> > >  
> > >                                         pgc_vpu_h1: power-domain@9
> > > {
> > >                                                 #power-domain-cells
> > > = <0>;
> > >                                                 reg =
> > > <IMX8MM_POWER_DOMAIN_VPUH1>;
> > > +                                               power-domains =
> > > <&pgc_vpumix>;
> > >                                         };
> > >  
> > >                                         pgc_dispmix:
> > > power-domain@10 {
> > 
> 

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[Vitor Soares]

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


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

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[Vitor Soares]

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.


Best regards,
Vitor Soares

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

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[Vitor Soares]

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

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

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

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

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

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[Lucas Stach]

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.

Regards,
Lucas

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

Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent

[Vitor Soares]

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