From: Frank Li <Frank.li@nxp.com>
To: Shengjiu Wang <shengjiu.wang@gmail.com>
Cc: Shengjiu Wang <shengjiu.wang@nxp.com>,
abelvesa@kernel.org, peng.fan@nxp.com, mturquette@baylibre.com,
sboyd@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com, imx@lists.linux.dev,
linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] clk: imx: imx8mp: Add delay after power up
Date: Fri, 10 May 2024 14:38:41 -0400 [thread overview]
Message-ID: <Zj5psV6ZIFZ/OPth@lizhi-Precision-Tower-5810> (raw)
In-Reply-To: <CAA+D8ANuNtaC90fHtGoYiofPTLQHcyCm0p_dcsYTVgT7gsKtMg@mail.gmail.com>
On Tue, May 07, 2024 at 04:04:14PM +0800, Shengjiu Wang wrote:
> On Tue, May 7, 2024 at 12:02 PM Frank Li <Frank.li@nxp.com> wrote:
> >
> > On Tue, May 07, 2024 at 11:44:32AM +0800, Shengjiu Wang wrote:
> > > On Tue, May 7, 2024 at 11:41 AM Frank Li <Frank.li@nxp.com> wrote:
> > > >
> > > > 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.
> > > >
> > > With Peng's reply:
> > >
> > > "No. Because BLK CTRL enable BUS_EN, before enable BUS_EN, udelay does
> > > not help. For the audiomix, move to gpcv2 would work, but gpcv2 is
> > > not only for i.MX8MP audiomix. For mixes, default not enable BUS_EN
> > > after power on, the udelay must be in blk ctrl driver."
> > >
> > > So gpcv2.c is not correct place for all BLK CTRL drivers.
> >
> > where BLK-CTRL driver source code?
>
> drivers/pmdomain/imx/imx8m-blk-ctrl.c
> drivers/pmdomain/imx/imx8mp-blk-ctrl.c
> drivers/pmdomain/imx/imx93-blk-ctrl.c
I still think it should put in gpcv2.c. Call power_on/off happen at very
low frequency. Even there are additional 5us delay for other BLK-CTRL
drivers, it will tiny impact to system performance. It is not worth to add
additonal software check to disingiush these two cases.
But correct power on is more important.
So readl() follow a udelay(5) is more important then additional 5us delay
for other BLK-CTRL driver since there are many 5us delay already in gpcv2.
Frank
>
> Best regards
> Shengjiu Wang
> >
> > even if put clk_imx8mp_audiomix_runtime_resume(), it need read any
> > register before udelay. all regiser read and write is strong ordered.
> > when get value from a register, all previous write must be done.
> >
> > all udelay (5) in gpcv2 may not delay 5us at all.
> >
> > Frank
> > >
> > > Best regards
> > > Shengjiu Wang
> > >
> > > > 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
> > > > > > >
next prev parent reply other threads:[~2024-05-10 18:38 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 3:35 [PATCH v2] clk: imx: imx8mp: Add delay after power up Shengjiu Wang
2024-05-06 18:21 ` Frank Li
2024-05-07 1:44 ` Shengjiu Wang
2024-05-07 3:41 ` Frank Li
2024-05-07 3:44 ` Shengjiu Wang
2024-05-07 4:02 ` Frank Li
2024-05-07 8:04 ` Shengjiu Wang
2024-05-10 18:38 ` Frank Li [this message]
2024-05-11 3:09 ` Shengjiu Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zj5psV6ZIFZ/OPth@lizhi-Precision-Tower-5810 \
--to=frank.li@nxp.com \
--cc=abelvesa@kernel.org \
--cc=festevam@gmail.com \
--cc=imx@lists.linux.dev \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=peng.fan@nxp.com \
--cc=s.hauer@pengutronix.de \
--cc=sboyd@kernel.org \
--cc=shawnguo@kernel.org \
--cc=shengjiu.wang@gmail.com \
--cc=shengjiu.wang@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).