| Message ID | 1715396125-3724-1-git-send-email-shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] pmdomain: imx: gpcv2: Add delay after power up handshake | expand |
On Fri, May 10, 2024 at 10:15 PM Shengjiu Wang <shengjiu.wang@nxp.com> wrote: > > AudioMix BLK-CTRL on i.MX8MP encountered an accessing register issue > after power up. > > [ 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 > The imx8mp-beacon board suffers from this as well, and I can confirm the patch also fixes it. It might be a coincidence, but the etnaviv NPU also enumerates more reliably now too. adam > According to comments in power up handshake: > > /* 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. > For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS clk-en > bit, it is better to add delay in this driver, as the BLK-CTL module doesn't > need to care about how it is powered up. > > regmap_read_bypassed() is to make sure the above write IO transaction already > reaches target before udelay(). > > Fixes: 1496dd413b2e ("clk: imx: imx8mp: Add pm_runtime support for power saving") > Reported-by: Francesco Dolcini <francesco@dolcini.it> > Closes: https://lore.kernel.org/all/66293535.170a0220.21fe.a2e7@mx.google.com/ > Suggested-by: Frank Li <frank.li@nxp.com> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> Tested-by: Adam Ford <aford173@gmail.com> > --- > changes in v3: > - move change to gpcv2.c, as it is more reasonable to let power driver > to handle such power issue, suggested by Frank Li > > changes in v2: > - reduce size of panic log in commit message > > drivers/pmdomain/imx/gpcv2.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pmdomain/imx/gpcv2.c b/drivers/pmdomain/imx/gpcv2.c > index 4b828d74a606..856eaac0ec14 100644 > --- a/drivers/pmdomain/imx/gpcv2.c > +++ b/drivers/pmdomain/imx/gpcv2.c > @@ -393,6 +393,17 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd) > * automatically there. Just add a delay and suppose the handshake finish > * after that. > */ > + > + /* > + * For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS > + * clk-en bit, it is better to add delay here, as the BLK-CTL module > + * doesn't need to care about how it is powered up. > + * > + * regmap_read_bypassed() is to make sure the above write IO transaction > + * already reaches target before udelay() > + */ > + regmap_read_bypassed(domain->regmap, domain->regs->hsk, ®_val); > + udelay(5); > } > > /* Disable reset clocks for all devices in the domain */ > -- > 2.34.1 > >
Hi, Am Mittwoch, 15. Mai 2024, 00:08:21 CEST schrieb Adam Ford: > On Fri, May 10, 2024 at 10:15 PM Shengjiu Wang <shengjiu.wang@nxp.com> wrote: > > > > AudioMix BLK-CTRL on i.MX8MP encountered an accessing register issue > > after power up. > > > > [ 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 > > > > The imx8mp-beacon board suffers from this as well, and I can confirm > the patch also fixes it. It might be a coincidence, but the etnaviv > NPU also enumerates more reliably now too. I had a similar local hack/workaround for NPU startup. This patch address both issues. Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Hello Stephen and Ulf, On Sat, May 11, 2024 at 10:55:25AM +0800, Shengjiu Wang wrote: > AudioMix BLK-CTRL on i.MX8MP encountered an accessing register issue > after power up. > > [ 2.181035] Kernel panic - not syncing: Asynchronous SError Interrupt ... > Fixes: 1496dd413b2e ("clk: imx: imx8mp: Add pm_runtime support for power saving") > Reported-by: Francesco Dolcini <francesco@dolcini.it> Any chances to get this into mainline with some priority? If in your opinion the fix is not correct I can just send a revert of the buggy commit. I reported the bug 4 weeks ago, the same day the broken commit was merged into -next [1], now we have a regression in mainline that is preventing booting with a kernel panic and because of that our whole CI is not able to test anything and therefore preventing us to look into any other regression. Thanks, Francesco [1] https://lore.kernel.org/all/20240424164725.GA18760@francesco-nb/
On Sat, 11 May 2024 at 05:15, Shengjiu Wang <shengjiu.wang@nxp.com> wrote: > > AudioMix BLK-CTRL on i.MX8MP encountered an accessing register issue > after power up. > > [ 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 > > According to comments in power up handshake: > > /* 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. > For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS clk-en > bit, it is better to add delay in this driver, as the BLK-CTL module doesn't > need to care about how it is powered up. > > regmap_read_bypassed() is to make sure the above write IO transaction already > reaches target before udelay(). > > Fixes: 1496dd413b2e ("clk: imx: imx8mp: Add pm_runtime support for power saving") > Reported-by: Francesco Dolcini <francesco@dolcini.it> > Closes: https://lore.kernel.org/all/66293535.170a0220.21fe.a2e7@mx.google.com/ > Suggested-by: Frank Li <frank.li@nxp.com> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> Sorry for the delay! Applied for fixes, thanks! Kind regards Uffe > --- > changes in v3: > - move change to gpcv2.c, as it is more reasonable to let power driver > to handle such power issue, suggested by Frank Li > > changes in v2: > - reduce size of panic log in commit message > > drivers/pmdomain/imx/gpcv2.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pmdomain/imx/gpcv2.c b/drivers/pmdomain/imx/gpcv2.c > index 4b828d74a606..856eaac0ec14 100644 > --- a/drivers/pmdomain/imx/gpcv2.c > +++ b/drivers/pmdomain/imx/gpcv2.c > @@ -393,6 +393,17 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd) > * automatically there. Just add a delay and suppose the handshake finish > * after that. > */ > + > + /* > + * For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS > + * clk-en bit, it is better to add delay here, as the BLK-CTL module > + * doesn't need to care about how it is powered up. > + * > + * regmap_read_bypassed() is to make sure the above write IO transaction > + * already reaches target before udelay() > + */ > + regmap_read_bypassed(domain->regmap, domain->regs->hsk, ®_val); > + udelay(5); > } > > /* Disable reset clocks for all devices in the domain */ > -- > 2.34.1 >
On Tue, May 21, 2024 at 12:22:41PM +0200, Ulf Hansson wrote: > On Sat, 11 May 2024 at 05:15, Shengjiu Wang <shengjiu.wang@nxp.com> wrote: > > The BLK-CTL module needs to add delay to wait for a handshake request finished. > > For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS clk-en > > bit, it is better to add delay in this driver, as the BLK-CTL module doesn't > > need to care about how it is powered up. > Sorry for the delay! > Applied for fixes, thanks! I see this is in -next but mainline is currently broken for i.MX8 platforms - could we get it in for -rc2?
On Mon, 27 May 2024 at 14:49, Mark Brown <broonie@kernel.org> wrote: > > On Tue, May 21, 2024 at 12:22:41PM +0200, Ulf Hansson wrote: > > On Sat, 11 May 2024 at 05:15, Shengjiu Wang <shengjiu.wang@nxp.com> wrote: > > > > The BLK-CTL module needs to add delay to wait for a handshake request finished. > > > For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS clk-en > > > bit, it is better to add delay in this driver, as the BLK-CTL module doesn't > > > need to care about how it is powered up. > > > Sorry for the delay! > > > Applied for fixes, thanks! > > I see this is in -next but mainline is currently broken for i.MX8 > platforms - could we get it in for -rc2? Yes, I am preparing a pull-request as of right now! Kind regards Uffe
diff --git a/drivers/pmdomain/imx/gpcv2.c b/drivers/pmdomain/imx/gpcv2.c index 4b828d74a606..856eaac0ec14 100644 --- a/drivers/pmdomain/imx/gpcv2.c +++ b/drivers/pmdomain/imx/gpcv2.c @@ -393,6 +393,17 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd) * automatically there. Just add a delay and suppose the handshake finish * after that. */ + + /* + * For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS + * clk-en bit, it is better to add delay here, as the BLK-CTL module + * doesn't need to care about how it is powered up. + * + * regmap_read_bypassed() is to make sure the above write IO transaction + * already reaches target before udelay() + */ + regmap_read_bypassed(domain->regmap, domain->regs->hsk, ®_val); + udelay(5); } /* Disable reset clocks for all devices in the domain */
AudioMix BLK-CTRL on i.MX8MP encountered an accessing register issue after power up. [ 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 According to comments in power up handshake: /* 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. For some BLK-CTL module (eg. AudioMix on i.MX8MP) doesn't have BUS clk-en bit, it is better to add delay in this driver, as the BLK-CTL module doesn't need to care about how it is powered up. regmap_read_bypassed() is to make sure the above write IO transaction already reaches target before udelay(). Fixes: 1496dd413b2e ("clk: imx: imx8mp: Add pm_runtime support for power saving") Reported-by: Francesco Dolcini <francesco@dolcini.it> Closes: https://lore.kernel.org/all/66293535.170a0220.21fe.a2e7@mx.google.com/ Suggested-by: Frank Li <frank.li@nxp.com> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- changes in v3: - move change to gpcv2.c, as it is more reasonable to let power driver to handle such power issue, suggested by Frank Li changes in v2: - reduce size of panic log in commit message drivers/pmdomain/imx/gpcv2.c | 11 +++++++++++ 1 file changed, 11 insertions(+)