| Message ID | 20240823095319.3917639-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pmdomain: imx: imx93-blk-ctrl: fix power up domain fail during early noriq resume | expand |
On Fri, 23 Aug 2024 at 11:44, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Dong Aisheng <aisheng.dong@nxp.com> > > After disabling PXP and having no displays connected, we met the following > suspend/resume hang issue on MX93 EVK board. > > Enabling non-boot CPUs ... > Detected VIPT I-cache on CPU1 > GICv3: CPU1: found redistributor 100 region 0:0x0000000048060000 > CPU1: Booted secondary processor 0x0000000100 [0x412fd050] > CPU1 is up > imx93-blk-ctrl 4ac10000.system-controller: failed to power up domain: -13 > imx93-blk-ctrl 4ac10000.system-controller: failed to power up domain: -13 > imx93-blk-ctrl 4ac10000.system-controller: failed to power up domain: -13 > ... > > The issue was introduced since the commit c24efa673278 > ("PM: runtime: Capture device status before disabling runtime PM") > which will also check the power.last_status must be RPM_ACTIVE before > pm_runtime_get_sync() can return 1 (means already active) even pm_runtime > is disabled during no_irq resume stage. > > However, the pm_runtime_set_active() we called ahead of > pm_runtime_get_sync() will not update power.last_status which probably like > a upstream kernel issue. But that's another issue which may worth an > extra fix. I think this is confusing, I don't see any calls to pm_runtime_set_active() anywhere? Are you referring to some old code? > > This patch refers to the solution in the exist similar imx8m blkctrl > driver[1] that it will power up upstream domains during blkctl suspend > first in order to make sure the power.last_status to be RPM_ACTIVE. Then we > can support calling pm_runtime_get_sync in noirq resume stage. > > After fixing, no need extra calling of pm_runtime_set_active() ahead. > > 1. drivers/pmdomain/imx/imx8m-blk-ctrl.c > > Fixes: e9aa77d413c9 ("soc: imx: add i.MX93 media blk ctrl driver") > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/pmdomain/imx/imx93-blk-ctrl.c | 29 +++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/pmdomain/imx/imx93-blk-ctrl.c b/drivers/pmdomain/imx/imx93-blk-ctrl.c > index 904ffa55b8f4..34ac7b722b90 100644 > --- a/drivers/pmdomain/imx/imx93-blk-ctrl.c > +++ b/drivers/pmdomain/imx/imx93-blk-ctrl.c > @@ -424,6 +424,34 @@ static const struct imx93_blk_ctrl_data imx93_media_blk_ctl_dev_data = { > .reg_access_table = &imx93_media_blk_ctl_access_table, > }; > > +static int imx93_blk_ctrl_suspend(struct device *dev) > +{ > + struct imx93_blk_ctrl *bc = dev_get_drvdata(dev); > + > + /* > + * This may look strange, but is done so the generic PM_SLEEP code > + * can power down our domains and more importantly power them up again > + * after resume, without tripping over our usage of runtime PM to > + * control the upstream GPC domains. Things happen in the right order > + * in the system suspend/resume paths due to the device parent/child > + * hierarchy. > + */ > + return pm_runtime_resume_and_get(bc->dev); The reason why we *don't* use a regular parent/child setup of the PM domains (genpds) to control power-on/off, is because there seems to be a specific sequence that needs to be followed. So, instead we are using runtime PM to control the power for the parent PM domain. See the comment in imx93_blk_ctrl_probe(). I have to admit, it all looks strange to me and seems also very fragile. That said, why doesn't the sequence matter any longer during system suspend/resume. Or maybe the sequence doesn't really matter after all? > +} > + > +static int imx93_blk_ctrl_resume(struct device *dev) > +{ > + struct imx93_blk_ctrl *bc = dev_get_drvdata(dev); > + > + pm_runtime_put(bc->dev); > + > + return 0; > +} > + > +static const struct dev_pm_ops imx93_blk_ctrl_pm_ops = { > + SYSTEM_SLEEP_PM_OPS(imx93_blk_ctrl_suspend, imx93_blk_ctrl_resume) > +}; > + > static const struct of_device_id imx93_blk_ctrl_of_match[] = { > { > .compatible = "fsl,imx93-media-blk-ctrl", > @@ -439,6 +467,7 @@ static struct platform_driver imx93_blk_ctrl_driver = { > .remove_new = imx93_blk_ctrl_remove, > .driver = { > .name = "imx93-blk-ctrl", > + .pm = pm_sleep_ptr(&imx93_blk_ctrl_pm_ops), > .of_match_table = imx93_blk_ctrl_of_match, > }, > }; > -- > 2.37.1 > Kind regards Uffe
> Subject: Re: [PATCH] pmdomain: imx: imx93-blk-ctrl: fix power up > domain fail during early noriq resume > > On Fri, 23 Aug 2024 at 11:44, Peng Fan (OSS) <peng.fan@oss.nxp.com> > wrote: > > > > From: Dong Aisheng <aisheng.dong@nxp.com> > > > > After disabling PXP and having no displays connected, we met the > > following suspend/resume hang issue on MX93 EVK board. > > > > Enabling non-boot CPUs ... > > Detected VIPT I-cache on CPU1 > > GICv3: CPU1: found redistributor 100 region > 0:0x0000000048060000 > > CPU1: Booted secondary processor 0x0000000100 [0x412fd050] > > CPU1 is up > > imx93-blk-ctrl 4ac10000.system-controller: failed to power up > domain: > > -13 imx93-blk-ctrl 4ac10000.system-controller: failed to power up > > domain: -13 imx93-blk-ctrl 4ac10000.system-controller: failed to > > power up domain: -13 ... > > > > The issue was introduced since the commit c24efa673278 > > ("PM: runtime: Capture device status before disabling runtime PM") > > which will also check the power.last_status must be RPM_ACTIVE > before > > pm_runtime_get_sync() can return 1 (means already active) even > > pm_runtime is disabled during no_irq resume stage. > > > > However, the pm_runtime_set_active() we called ahead of > > pm_runtime_get_sync() will not update power.last_status which > probably > > like a upstream kernel issue. But that's another issue which may > worth > > an extra fix. > > I think this is confusing, I don't see any calls to > pm_runtime_set_active() anywhere? Are you referring to some old > code? oh. I forgot to update commit message when moving this patch to upstream > > > > > This patch refers to the solution in the exist similar imx8m blkctrl > > driver[1] that it will power up upstream domains during blkctl > suspend > > first in order to make sure the power.last_status to be RPM_ACTIVE. > > Then we can support calling pm_runtime_get_sync in noirq resume > stage. > > > > After fixing, no need extra calling of pm_runtime_set_active() ahead. > > > > 1. drivers/pmdomain/imx/imx8m-blk-ctrl.c > > > > Fixes: e9aa77d413c9 ("soc: imx: add i.MX93 media blk ctrl driver") > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/pmdomain/imx/imx93-blk-ctrl.c | 29 > > +++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/pmdomain/imx/imx93-blk-ctrl.c > > b/drivers/pmdomain/imx/imx93-blk-ctrl.c > > index 904ffa55b8f4..34ac7b722b90 100644 > > --- a/drivers/pmdomain/imx/imx93-blk-ctrl.c > > +++ b/drivers/pmdomain/imx/imx93-blk-ctrl.c > > @@ -424,6 +424,34 @@ static const struct imx93_blk_ctrl_data > imx93_media_blk_ctl_dev_data = { > > .reg_access_table = &imx93_media_blk_ctl_access_table, > > }; > > > > +static int imx93_blk_ctrl_suspend(struct device *dev) { > > + struct imx93_blk_ctrl *bc = dev_get_drvdata(dev); > > + > > + /* > > + * This may look strange, but is done so the generic PM_SLEEP > code > > + * can power down our domains and more importantly power > them up again > > + * after resume, without tripping over our usage of runtime PM > to > > + * control the upstream GPC domains. Things happen in the > right order > > + * in the system suspend/resume paths due to the device > parent/child > > + * hierarchy. > > + */ > > + return pm_runtime_resume_and_get(bc->dev); > > The reason why we *don't* use a regular parent/child setup of the PM > domains (genpds) to control power-on/off, is because there seems to > be a specific sequence that needs to be followed. So, instead we are > using runtime PM to control the power for the parent PM domain. See > the comment in imx93_blk_ctrl_probe(). This was to follow i.MX8M implementation, but parent/child here should be better here. After rethinking about this, i.MX93 not has the HW limitation as i.MX8M, so parent/child could be used here. > > I have to admit, it all looks strange to me and seems also very fragile. > > That said, why doesn't the sequence matter any longer during system > suspend/resume. Or maybe the sequence doesn't really matter after all? It matters, the blk ctrl should be child of GPC power domain. Thanks, Peng. > > > +} > > + > > +static int imx93_blk_ctrl_resume(struct device *dev) { > > + struct imx93_blk_ctrl *bc = dev_get_drvdata(dev); > > + > > + pm_runtime_put(bc->dev); > > + > > + return 0; > > +} > > + > > +static const struct dev_pm_ops imx93_blk_ctrl_pm_ops = { > > + SYSTEM_SLEEP_PM_OPS(imx93_blk_ctrl_suspend, > > +imx93_blk_ctrl_resume) }; > > + > > static const struct of_device_id imx93_blk_ctrl_of_match[] = { > > { > > .compatible = "fsl,imx93-media-blk-ctrl", @@ -439,6 > > +467,7 @@ static struct platform_driver imx93_blk_ctrl_driver = { > > .remove_new = imx93_blk_ctrl_remove, > > .driver = { > > .name = "imx93-blk-ctrl", > > + .pm = pm_sleep_ptr(&imx93_blk_ctrl_pm_ops), > > .of_match_table = imx93_blk_ctrl_of_match, > > }, > > }; > > -- > > 2.37.1 > > > > Kind regards > Uffe
diff --git a/drivers/pmdomain/imx/imx93-blk-ctrl.c b/drivers/pmdomain/imx/imx93-blk-ctrl.c index 904ffa55b8f4..34ac7b722b90 100644 --- a/drivers/pmdomain/imx/imx93-blk-ctrl.c +++ b/drivers/pmdomain/imx/imx93-blk-ctrl.c @@ -424,6 +424,34 @@ static const struct imx93_blk_ctrl_data imx93_media_blk_ctl_dev_data = { .reg_access_table = &imx93_media_blk_ctl_access_table, }; +static int imx93_blk_ctrl_suspend(struct device *dev) +{ + struct imx93_blk_ctrl *bc = dev_get_drvdata(dev); + + /* + * This may look strange, but is done so the generic PM_SLEEP code + * can power down our domains and more importantly power them up again + * after resume, without tripping over our usage of runtime PM to + * control the upstream GPC domains. Things happen in the right order + * in the system suspend/resume paths due to the device parent/child + * hierarchy. + */ + return pm_runtime_resume_and_get(bc->dev); +} + +static int imx93_blk_ctrl_resume(struct device *dev) +{ + struct imx93_blk_ctrl *bc = dev_get_drvdata(dev); + + pm_runtime_put(bc->dev); + + return 0; +} + +static const struct dev_pm_ops imx93_blk_ctrl_pm_ops = { + SYSTEM_SLEEP_PM_OPS(imx93_blk_ctrl_suspend, imx93_blk_ctrl_resume) +}; + static const struct of_device_id imx93_blk_ctrl_of_match[] = { { .compatible = "fsl,imx93-media-blk-ctrl", @@ -439,6 +467,7 @@ static struct platform_driver imx93_blk_ctrl_driver = { .remove_new = imx93_blk_ctrl_remove, .driver = { .name = "imx93-blk-ctrl", + .pm = pm_sleep_ptr(&imx93_blk_ctrl_pm_ops), .of_match_table = imx93_blk_ctrl_of_match, }, };