Re: [PATCH v2] clk: imx: imx8mp: Add delay after power up

[Frank Li <Frank.li@nxp.com>]

On Tue, May 07, 2024 at 09:44:19AM +0800, Shengjiu Wang wrote:
> On Tue, May 7, 2024 at 2:22 AM Frank Li <Frank.li@nxp.com> wrote:
> >
> > On Mon, May 06, 2024 at 11:35:02AM +0800, Shengjiu Wang wrote:
> > > According to comments in drivers/pmdomain/imx/gpcv2.c:
> > >
> > >       /* 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 = 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.
> > >                */
> > >       }
> > >
> > > The BLK-CTL module needs to add delay to wait for a handshake request finished
> > > before accessing registers, which is just after the enabling of the power domain.
> > >
> > > Otherwise there is error:
> > >
> > > [    2.181035] Kernel panic - not syncing: Asynchronous SError Interrupt
> > > [    2.181038] CPU: 1 PID: 48 Comm: kworker/u16:2 Not tainted 6.9.0-rc5-next-20240424-00003-g21cec88845c6 #171
> > > [    2.181047] Hardware name: NXP i.MX8MPlus EVK board (DT)
> > > [    2.181050] Workqueue: events_unbound deferred_probe_work_func
> > > [    2.181064] Call trace:
> > > [...]
> > > [    2.181142]  arm64_serror_panic+0x6c/0x78
> > > [    2.181149]  do_serror+0x3c/0x70
> > > [    2.181157]  el1h_64_error_handler+0x30/0x48
> > > [    2.181164]  el1h_64_error+0x64/0x68
> > > [    2.181171]  clk_imx8mp_audiomix_runtime_resume+0x34/0x44
> > > [    2.181183]  __genpd_runtime_resume+0x30/0x80
> > > [    2.181195]  genpd_runtime_resume+0x110/0x244
> > > [    2.181205]  __rpm_callback+0x48/0x1d8
> > > [    2.181213]  rpm_callback+0x68/0x74
> > > [    2.181224]  rpm_resume+0x468/0x6c0
> > > [    2.181234]  __pm_runtime_resume+0x50/0x94
> > > [    2.181243]  pm_runtime_get_suppliers+0x60/0x8c
> > > [    2.181258]  __driver_probe_device+0x48/0x12c
> > > [    2.181268]  driver_probe_device+0xd8/0x15c
> > > [    2.181278]  __device_attach_driver+0xb8/0x134
> > > [    2.181290]  bus_for_each_drv+0x84/0xe0
> > > [    2.181302]  __device_attach+0x9c/0x188
> > > [    2.181312]  device_initial_probe+0x14/0x20
> > > [    2.181323]  bus_probe_device+0xac/0xb0
> > > [    2.181334]  deferred_probe_work_func+0x88/0xc0
> > > [    2.181344]  process_one_work+0x150/0x290
> > > [    2.181357]  worker_thread+0x2f8/0x408
> > > [    2.181370]  kthread+0x110/0x114
> > > [    2.181381]  ret_from_fork+0x10/0x20
> > > [    2.181391] SMP: stopping secondary CPUs
> > >
> > > Fixes: 1496dd413b2e ("clk: imx: imx8mp: Add pm_runtime support for power saving")
> > > Reported-by: Francesco Dolcini <francesco@dolcini.it>
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > Revewied-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > > changes in v2:
> > > - reduce size of panic log in commit message
> > >
> > >  drivers/clk/imx/clk-imx8mp-audiomix.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
> > > index b381d6f784c8..ae2c0f254225 100644
> > > --- a/drivers/clk/imx/clk-imx8mp-audiomix.c
> > > +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
> > > @@ -6,6 +6,7 @@
> > >   */
> > >
> > >  #include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/device.h>
> > >  #include <linux/io.h>
> > >  #include <linux/mod_devicetable.h>
> > > @@ -360,6 +361,12 @@ static int clk_imx8mp_audiomix_runtime_suspend(struct device *dev)
> > >
> > >  static int clk_imx8mp_audiomix_runtime_resume(struct device *dev)
> > >  {
> > > +     /*
> > > +      * According to the drivers/pmdomain/imx/gpcv2.c
> > > +      * need to wait for handshake request to propagate
> > > +      */
> > > +     udelay(5);
> > > +
> >
> > Did you address the issue I comments at v1?
> > It should not fix at here, I think it should be gpcv2.c to delay 5us.
> 
> Other BLK CTRL drivers already delay 5us in its own drivers, if
> add delay in gpcv2.c, for these drivers, it will delay 10us totally.

We should go forward as correct direction. If udelay should be gpcv2.c,
it should be there and remove other udelay in BLK CTRL drivers gradually.

If sometime found 5us is not enough, need change to 6us, we just need
change at one place.

Frank

> 
> Best regards
> Shengjiu Wang
> 
> 
> 
> >
> > Frank
> >
> > >       clk_imx8mp_audiomix_save_restore(dev, false);
> > >
> > >       return 0;
> > > --
> > > 2.34.1
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel