| Message ID | 20241125142108.1613016-1-carlos.song@nxp.com (mailing list archive) |
|---|---|
| State | In Next |
| Headers | show |
| Series | i2c: imx: make controller available until system suspend_noirq() and from resume_noirq() | expand |
On Mon, Nov 25, 2024 at 10:21:08PM +0800, carlos.song@nxp.com wrote: > From: Carlos Song <carlos.song@nxp.com> > > Put runtime pm to resume state between suspend() and suspend_noirq(), > resume_noirq() and resume(), because some I2C devices need controller > on to do communication during this period. > > The controller can't be wakeup once runtime pm is disabled and in > runtime autosuspended state. > > The problem can be easily reproduced on the I.MX8MQ platform: > PMIC needs to be used to enable regular when the system resumes. > When PMIC uses I2C controller, I2C runtime pm has not been enabled, > so in i2c xfer(), pm_runtime_resume_and_get() will return error, > which causes data transfer failed. Therefore, regulars can not > be enabled and hang system resumes. > Here is resume error log: > [ 53.888902] galcore 38000000.gpu3d: PM: calling genpd_resume_noirq @ 529, parent: platform > [ 53.897203] i2c_imx_xfer, pm_runtime_resume_and_get is -13 > [ 53.902713] imx-pgc imx-pgc-domain.5: failed to enable regulator: -EACCES > [ 53.909518] galcore 38000000.gpu3d: PM: genpd_resume_noirq returned 0 after 12331 usecs > [ 53.917545] mxc_hantro 38300000.vpu: PM: calling genpd_resume_noirq @ 529, parent: soc@0 > [ 53.925659] i2c_imx_xfer, pm_runtime_resume_and_get is -13 > [ 53.931157] imx-pgc imx-pgc-domain.6: failed to enable regulator: -EACCES > > I.MX8MQ system resume normally after applying the fix. Here is resume log: > [ 71.068807] galcore 38000000.gpu3d: PM: calling genpd_resume_noirq @ 530, parent: platform > [ 71.077103] i2c_imx_xfer, pm_runtime_resume_and_get is 0 > [ 71.083578] galcore 38000000.gpu3d: PM: genpd_resume_noirq returned 0 after 6490 usecs > [ 71.091526] mxc_hantro 38300000.vpu: PM: calling genpd_resume_noirq @ 530, parent: soc@0 > [ 71.099638] i2c_imx_xfer, pm_runtime_resume_and_get is 0 > [ 71.106091] mxc_hantro 38300000.vpu: PM: genpd_resume_noirq returned 0 after 6458 usecs > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- Reviewed-by: Frank Li <Frank.Li@nxp.com> > drivers/i2c/busses/i2c-imx.c | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index ee7070ee9e6e..c35092726465 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -1770,7 +1770,8 @@ static int i2c_imx_probe(struct platform_device *pdev) > goto rpm_disable; > > /* Request IRQ */ > - ret = request_irq(irq, i2c_imx_isr, IRQF_SHARED, pdev->name, i2c_imx); > + ret = request_irq(irq, i2c_imx_isr, IRQF_SHARED | IRQF_NO_SUSPEND, > + pdev->name, i2c_imx); > if (ret) { > dev_err(&pdev->dev, "can't claim irq %d\n", irq); > goto rpm_disable; > @@ -1894,7 +1895,36 @@ static int i2c_imx_runtime_resume(struct device *dev) > return ret; > } > > +static int i2c_imx_suspend(struct device *dev) > +{ > + /* > + * Some I2C devices may need I2C controller up during resume_noirq() > + * or suspend_noirq(), if the controller is autosuspended, there is > + * no way to wakeup it once runtime pm is disabled (in suspend_late()). > + * When system resume, I2C controller will be available until runtime pm > + * is enabled(in_resume_early()). But it is too late for some devices. > + * Wakeup the controller in suspend() callback while runtime pm is enabled, > + * I2C controller will be available until suspend_noirq() callback > + * (pm_runtime_force_suspend()) is called. During the resume, I2C controller > + * can be restored by resume_noirq() callback (pm_runtime_force_resume()). > + * Then resume() callback enables autosuspend. It will make I2C controller > + * available until system suspend_noirq() and from resume_noirq(). > + */ > + return pm_runtime_resume_and_get(dev); > +} > + > +static int i2c_imx_resume(struct device *dev) > +{ > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + > + return 0; > +} > + > static const struct dev_pm_ops i2c_imx_pm_ops = { > + NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > + SYSTEM_SLEEP_PM_OPS(i2c_imx_suspend, i2c_imx_resume) > RUNTIME_PM_OPS(i2c_imx_runtime_suspend, i2c_imx_runtime_resume, NULL) > }; > > -- > 2.34.1 >
Hi Carlos, ... > +static int i2c_imx_suspend(struct device *dev) > +{ > + /* > + * Some I2C devices may need I2C controller up during resume_noirq() > + * or suspend_noirq(), if the controller is autosuspended, there is > + * no way to wakeup it once runtime pm is disabled (in suspend_late()). > + * When system resume, I2C controller will be available until runtime pm > + * is enabled(in_resume_early()). But it is too late for some devices. > + * Wakeup the controller in suspend() callback while runtime pm is enabled, > + * I2C controller will be available until suspend_noirq() callback > + * (pm_runtime_force_suspend()) is called. During the resume, I2C controller > + * can be restored by resume_noirq() callback (pm_runtime_force_resume()). > + * Then resume() callback enables autosuspend. It will make I2C controller > + * available until system suspend_noirq() and from resume_noirq(). > + */ Just made some little adjustments to the comment above, please let me know if they are fine: /* * Some I2C devices may need the I2C controller to remain active * during resume_noirq() or suspend_noirq(). If the controller is * autosuspended, there is no way to wake it up once runtime PM is * disabled (in suspend_late()). * * During system resume, the I2C controller will be available only * after runtime PM is re-enabled (in resume_early()). However, this * may be too late for some devices. * * Wake up the controller in the suspend() callback while runtime PM * is still enabled. The I2C controller will remain available until * the suspend_noirq() callback (pm_runtime_force_suspend()) is * called. During resume, the I2C controller can be restored by the * resume_noirq() callback (pm_runtime_force_resume()). * * Finally, the resume() callback re-enables autosuspend, ensuring * the I2C controller remains available until the system enters * suspend_noirq() and from resume_noirq(). */ If so, I will take it in. Andi > + return pm_runtime_resume_and_get(dev); > +}
> -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Thursday, December 5, 2024 7:54 PM > To: Carlos Song <carlos.song@nxp.com> > Cc: o.rempel@pengutronix.de; kernel@pengutronix.de; shawnguo@kernel.org; > s.hauer@pengutronix.de; festevam@gmail.com; Frank Li <frank.li@nxp.com>; > linux-i2c@vger.kernel.org; imx@lists.linux.dev; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH] i2c: imx: make controller available until system > suspend_noirq() and from resume_noirq() > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > Hi Carlos, > > ... > > > +static int i2c_imx_suspend(struct device *dev) { > > + /* > > + * Some I2C devices may need I2C controller up during resume_noirq() > > + * or suspend_noirq(), if the controller is autosuspended, there is > > + * no way to wakeup it once runtime pm is disabled (in > suspend_late()). > > + * When system resume, I2C controller will be available until runtime > pm > > + * is enabled(in_resume_early()). But it is too late for some devices. > > + * Wakeup the controller in suspend() callback while runtime pm is > enabled, > > + * I2C controller will be available until suspend_noirq() callback > > + * (pm_runtime_force_suspend()) is called. During the resume, I2C > controller > > + * can be restored by resume_noirq() callback > (pm_runtime_force_resume()). > > + * Then resume() callback enables autosuspend. It will make I2C > controller > > + * available until system suspend_noirq() and from resume_noirq(). > > + */ > > Just made some little adjustments to the comment above, please let me know if > they are fine: > > /* > * Some I2C devices may need the I2C controller to remain active > * during resume_noirq() or suspend_noirq(). If the controller is > * autosuspended, there is no way to wake it up once runtime PM is > * disabled (in suspend_late()). > * > * During system resume, the I2C controller will be available only > * after runtime PM is re-enabled (in resume_early()). However, this > * may be too late for some devices. > * > * Wake up the controller in the suspend() callback while runtime PM > * is still enabled. The I2C controller will remain available until > * the suspend_noirq() callback (pm_runtime_force_suspend()) is > * called. During resume, the I2C controller can be restored by the > * resume_noirq() callback (pm_runtime_force_resume()). > * > * Finally, the resume() callback re-enables autosuspend, ensuring > * the I2C controller remains available until the system enters > * suspend_noirq() and from resume_noirq(). > */ > > If so, I will take it in. > Hi, Andi, That is so nice! > Andi > > > + return pm_runtime_resume_and_get(dev); }
Hi Carlos, On Mon, Nov 25, 2024 at 10:21:08PM +0800, carlos.song@nxp.com wrote: > From: Carlos Song <carlos.song@nxp.com> > > Put runtime pm to resume state between suspend() and suspend_noirq(), > resume_noirq() and resume(), because some I2C devices need controller > on to do communication during this period. > > The controller can't be wakeup once runtime pm is disabled and in > runtime autosuspended state. > > The problem can be easily reproduced on the I.MX8MQ platform: > PMIC needs to be used to enable regular when the system resumes. > When PMIC uses I2C controller, I2C runtime pm has not been enabled, > so in i2c xfer(), pm_runtime_resume_and_get() will return error, > which causes data transfer failed. Therefore, regulars can not > be enabled and hang system resumes. > Here is resume error log: > [ 53.888902] galcore 38000000.gpu3d: PM: calling genpd_resume_noirq @ 529, parent: platform > [ 53.897203] i2c_imx_xfer, pm_runtime_resume_and_get is -13 > [ 53.902713] imx-pgc imx-pgc-domain.5: failed to enable regulator: -EACCES > [ 53.909518] galcore 38000000.gpu3d: PM: genpd_resume_noirq returned 0 after 12331 usecs > [ 53.917545] mxc_hantro 38300000.vpu: PM: calling genpd_resume_noirq @ 529, parent: soc@0 > [ 53.925659] i2c_imx_xfer, pm_runtime_resume_and_get is -13 > [ 53.931157] imx-pgc imx-pgc-domain.6: failed to enable regulator: -EACCES > > I.MX8MQ system resume normally after applying the fix. Here is resume log: > [ 71.068807] galcore 38000000.gpu3d: PM: calling genpd_resume_noirq @ 530, parent: platform > [ 71.077103] i2c_imx_xfer, pm_runtime_resume_and_get is 0 > [ 71.083578] galcore 38000000.gpu3d: PM: genpd_resume_noirq returned 0 after 6490 usecs > [ 71.091526] mxc_hantro 38300000.vpu: PM: calling genpd_resume_noirq @ 530, parent: soc@0 > [ 71.099638] i2c_imx_xfer, pm_runtime_resume_and_get is 0 > [ 71.106091] mxc_hantro 38300000.vpu: PM: genpd_resume_noirq returned 0 after 6458 usecs > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> merged to i2c/i2c-host. Thanks, Andi
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index ee7070ee9e6e..c35092726465 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -1770,7 +1770,8 @@ static int i2c_imx_probe(struct platform_device *pdev) goto rpm_disable; /* Request IRQ */ - ret = request_irq(irq, i2c_imx_isr, IRQF_SHARED, pdev->name, i2c_imx); + ret = request_irq(irq, i2c_imx_isr, IRQF_SHARED | IRQF_NO_SUSPEND, + pdev->name, i2c_imx); if (ret) { dev_err(&pdev->dev, "can't claim irq %d\n", irq); goto rpm_disable; @@ -1894,7 +1895,36 @@ static int i2c_imx_runtime_resume(struct device *dev) return ret; } +static int i2c_imx_suspend(struct device *dev) +{ + /* + * Some I2C devices may need I2C controller up during resume_noirq() + * or suspend_noirq(), if the controller is autosuspended, there is + * no way to wakeup it once runtime pm is disabled (in suspend_late()). + * When system resume, I2C controller will be available until runtime pm + * is enabled(in_resume_early()). But it is too late for some devices. + * Wakeup the controller in suspend() callback while runtime pm is enabled, + * I2C controller will be available until suspend_noirq() callback + * (pm_runtime_force_suspend()) is called. During the resume, I2C controller + * can be restored by resume_noirq() callback (pm_runtime_force_resume()). + * Then resume() callback enables autosuspend. It will make I2C controller + * available until system suspend_noirq() and from resume_noirq(). + */ + return pm_runtime_resume_and_get(dev); +} + +static int i2c_imx_resume(struct device *dev) +{ + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + + return 0; +} + static const struct dev_pm_ops i2c_imx_pm_ops = { + NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) + SYSTEM_SLEEP_PM_OPS(i2c_imx_suspend, i2c_imx_resume) RUNTIME_PM_OPS(i2c_imx_runtime_suspend, i2c_imx_runtime_resume, NULL) };