Received-SPF: pass (google.com: domain of linux-kernel+bounces-158208-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <1711026842-7268-1-git-send-email-shengjiu.wang@nxp.com>
 <20240424164725.GA18760@francesco-nb> <CAA+D8AMxcAOdeSUhLnFdX2tjXSiWKg8-oxfZ8oT06-qQPnNKqA@mail.gmail.com>
 <20240425083214.g6xplxql4g32zxud@pengutronix.de>
In-Reply-To: <20240425083214.g6xplxql4g32zxud@pengutronix.de>
From: Shengjiu Wang <shengjiu.wang@gmail.com>
Date: Thu, 25 Apr 2024 16:43:57 +0800
Message-ID: <CAA+D8ANyQwmS+sp-mo22zi4imSL9APHg5dY3-9yXMQSY_MMziA@mail.gmail.com>
Subject: Re: [PATCH v4] clk: imx: imx8mp: Add pm_runtime support for power saving
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: Francesco Dolcini <francesco@dolcini.it>, peng.fan@nxp.com, s.hauer@pengutronix.de, 
	sboyd@kernel.org, festevam@gmail.com, mturquette@baylibre.com, 
	linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, linux-imx@nxp.com, 
	kernel@pengutronix.de, shawnguo@kernel.org, 
	Shengjiu Wang <shengjiu.wang@nxp.com>, linux-arm-kernel@lists.infradead.org, 
	abelvesa@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 25, 2024 at 4:32=E2=80=AFPM Marco Felsch <m.felsch@pengutronix.=
de> wrote:
>
> On 24-04-25, Shengjiu Wang wrote:
> > On Thu, Apr 25, 2024 at 12:47=E2=80=AFAM Francesco Dolcini <francesco@d=
olcini.it> wrote:
> > >
> > > On Thu, Mar 21, 2024 at 09:14:02PM +0800, Shengjiu Wang wrote:
> > > > Add pm_runtime support for power saving. In pm runtime suspend
> > > > state the registers will be reseted, so add registers save
> > > > in pm runtime suspend and restore them in pm runtime resume.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > >
> > > Is this introducing a regression?
> > >
> > >   800 13:50:19.713052  <6>[   16.531134] clk: Disabling unused clocks
> > >   801 13:50:19.727524  <2>[   16.535413] SError Interrupt on CPU2, co=
de 0x00000000bf000002 -- SError
> > >   802 13:50:19.731400  <4>[   16.535421] CPU: 2 PID: 1 Comm: swapper/=
0 Not tainted 6.9.0-rc5-next-20240424 #1
> > >   803 13:50:19.742514  <4>[   16.535428] Hardware name: Toradex Verdi=
n iMX8M Plus on Dahlia Board (DT)
> > >   804 13:50:19.747157  <4>[   16.535431] pstate: 80000005 (Nzcv daif =
-PAN -UAO -TCO -DIT -SSBS BTYPE=3D--)
> > >   805 13:50:19.758468  <4>[   16.535442] pc : clk_imx8mp_audiomix_run=
time_resume+0x24/0x48
> > >   806 13:50:19.759372  <4>[   16.535456] lr : pm_generic_runtime_resu=
me+0x2c/0x44
> > >   807 13:50:19.759587  <4>[   16.535465] sp : ffff800082b8bb90
> > >   808 13:50:19.774512  <4>[   16.535468] x29: ffff800082b8bb90 x28: 0=
000000000000000 x27: 0000000000000000
> > >   809 13:50:19.775367  <4>[   16.535482] x26: 0000000000000000 x25: 0=
000000000000000 x24: 0000000000000000
> > >   810 13:50:19.790567  <4>[   16.535495] x23: ffff0000c0f7a4e0 x22: f=
fff0000c26922f8 x21: 0000000000000000
> > >   811 13:50:19.791308  <4>[   16.535508] x20: ffff0000c2692000 x19: f=
fff0000c0e30c10 x18: 0000000000000000
> > >   812 13:50:19.794834  <4>[   16.535521] x17: 000000007e4712cb x16: f=
fff80008296f800 x15: 0000000000000030
> > >   813 13:50:19.807341  <4>[   16.535532] x14: ffff0000c00b8080 x13: 0=
0000000000003f9 x12: 0000000000000000
> > >   814 13:50:19.810740  <4>[   16.535545] x11: 0000000000000000 x10: 0=
000000000000aa0 x9 : ffff800082b8bb20
> > >   815 13:50:19.822528  <4>[   16.535559] x8 : ffff0000c00b8b00 x7 : 0=
000000000000000 x6 : ffff0000c00b8000
> > >   816 13:50:19.827173  <4>[   16.535570] x5 : ffff8000836b0000 x4 : f=
fff0000c2f3a488 x3 : ffff8000813660d0
> > >   817 13:50:19.838446  <4>[   16.535583] x2 : 0000000000000004 x1 : 0=
000000000000001 x0 : 00000000ff777777
> > >   818 13:50:19.839321  <0>[   16.535597] Kernel panic - not syncing: =
Asynchronous SError Interrupt
> > >   819 13:50:19.839983  Matched prompt #9: Kernel panic - not syncing
> > >   820 13:50:19.840155  Setting prompt string to ['end Kernel panic[^\=
\r]*\\r', '/ #', 'Login timed out', 'Login incorrect']
> > >   821 13:50:19.854524  <4>[   16.535601] CPU: 2 PID: 1 Comm: swapper/=
0 Not tainted 6.9.0-rc5-next-20240424 #1
> > >   822 13:50:19.855261  <4>[   16.535609] Hardware name: Toradex Verdi=
n iMX8M Plus on Dahlia Board (DT)
> > >   823 13:50:19.858660  <4>[   16.535613] Call trace:
> > >   824 13:50:19.870455  <4>[   16.535616]  dump_backtrace+0x94/0xec
> > >   825 13:50:19.870763  <4>[   16.535626]  show_stack+0x18/0x24
> > >   826 13:50:19.871258  <4>[   16.535635]  dump_stack_lvl+0x38/0x90
> > >   827 13:50:19.874714  <4>[   16.535647]  dump_stack+0x18/0x24
> > >   828 13:50:19.874964  <4>[   16.535656]  panic+0x388/0x3c8
> > >   829 13:50:19.886551  <4>[   16.535667]  nmi_panic+0x48/0x94
> > >   830 13:50:19.888318  <4>[   16.535679]  arm64_serror_panic+0x6c/0x7=
8
> > >   831 13:50:19.888531  <4>[   16.535688]  do_serror+0x3c/0x78
> > >   832 13:50:19.892592  <4>[   16.535693]  el1h_64_error_handler+0x30/=
0x48
> > >   833 13:50:19.902540  <4>[   16.535703]  el1h_64_error+0x64/0x68
> > >   834 13:50:19.903437  <4>[   16.535709]  clk_imx8mp_audiomix_runtime=
_resume+0x24/0x48
> > >   835 13:50:19.907712  <4>[   16.535719]  __genpd_runtime_resume+0x30=
/0xa8
> > >   836 13:50:19.918505  <4>[   16.535729]  genpd_runtime_resume+0xb4/0=
x29c
> > >   837 13:50:19.918770  <4>[   16.535741]  __rpm_callback+0x48/0x198
> > >   838 13:50:19.919372  <4>[   16.535749]  rpm_callback+0x68/0x74
> > >   839 13:50:19.922715  <4>[   16.535754]  rpm_resume+0x3cc/0x680
> > >   840 13:50:19.934495  <4>[   16.535762]  __pm_runtime_resume+0x4c/0x=
90
> > >   841 13:50:19.934784  <4>[   16.535769]  clk_pm_runtime_get_all+0x58=
/0x164
> > >   842 13:50:19.935344  <4>[   16.535780]  clk_disable_unused+0x2c/0x1=
78
> > >   843 13:50:19.938873  <4>[   16.535793]  do_one_initcall+0x6c/0x1b0
> > >   844 13:50:19.950539  <4>[   16.535799]  kernel_init_freeable+0x1c8/=
0x290
> > >   845 13:50:19.951360  <4>[   16.535812]  kernel_init+0x20/0x1dc
> > >   846 13:50:19.951585  <4>[   16.535821]  ret_from_fork+0x10/0x20
> > >   847 13:50:19.954803  <2>[   16.535831] SMP: stopping secondary CPUs
> > >   848 13:50:19.966688  <0>[   16.535838] Kernel Offset: disabled
> > >   849 13:50:19.967221  <0>[   16.535841] CPU features: 0x0,00000040,0=
0100000,4200421b
> > >   850 13:50:19.967360  <0>[   16.535845] Memory Limit: none
> > >   851 13:50:19.985117  <0>[   16.788060] ---[ end Kernel panic - not =
syncing: Asynchronous SError Interrupt ]---
> > >
> > > from
> > >
> > > https://storage.kernelci.org/next/master/next-20240424/arm64/defconfi=
g/gcc-10/lab-broonie/baseline-imx8mp-verdin-nonwifi-dahlia.html
> > > https://lore.kernel.org/all/66293535.170a0220.21fe.a2e7@mx.google.com=
/
> > >
> >
> > Sorry that I didn't use a clean community kernel for the test.
>
> :/ I have asked you if you have tested this feature since I was aware of
> bugs regarding PM.

But the issue you encountered is the "clock-prepare lock" as I remember.
I think it is a different one.

>
> > On my local side I added delay in drivers/pmdomain/imx/gpcv2.c
> > so there was no such issue.
> >
> > But according to drivers/pmdomain/imx/gpcv2.c, seems that I need
> > to add delay in this driver, like this:
>
> "Seems" shouldn't be really a "The root cause for this is".

I should not use 'Seems':)

below is I find in drivers/pmdomain/imx/gpcv2.c.  the commented code:

        /* request the ADB400 to power up */
        if (domain->bits.hskreq) {
                regmap_update_bits(domain->regmap, domain->regs->hsk,
                                   domain->bits.hskreq, domain->bits.hskreq=
);

                /*
                 * ret =3D regmap_read_poll_timeout(domain->regmap,
domain->regs->hsk, reg_val,
                 *                                (reg_val &
domain->bits.hskack), 0,
                 *                                USEC_PER_MSEC);
                 * Technically we need the commented code to wait
handshake. But that needs
                 * the BLK-CTL module BUS clk-en bit being set.
                 *
                 * There is a separate BLK-CTL module and we will have
such a driver for it,
                 * that driver will set the BUS clk-en bit and
handshake will be triggered
                 * automatically there. Just add a delay and suppose
the handshake finish
                 * after that.
                 */
        }

Best regards
Shengjiu Wang
>
> Regards,
>   Marco
>
> >  static int clk_imx8mp_audiomix_runtime_resume(struct device *dev)
> >  {
> > +       /*
> > +        * According to the drivers/pmdomain/imx/gpcv2.c
> > +        * need to wait for reset to propagate
> > +        */
> > +       udelay(5);
> > +
> >
> > I will submit a patch for it.
> >
> > Thanks for reporting it
> >
> > Best regards
> > Shengjiu Wang
> >
> >
> >
> > > Francesco
> > >
> >
> >