| Message ID | 20201209080102.26626-18-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MT8192 IOMMU support | expand |
On Wed, Dec 09, 2020 at 04:00:52PM +0800, Yong Wu wrote: > This patch adds pm runtime callback. > > In pm runtime case, all the registers backup/restore and bclk are > controlled in the pm_runtime callback, then pm_suspend is not needed in > this case. > > runtime PM is disabled when suspend, thus we call > pm_runtime_status_suspended instead of pm_runtime_suspended. > > And, m4u doesn't have its special pm runtime domain in previous SoC, in > this case dev->power.runtime_status is RPM_SUSPENDED defaultly, This sounds wrong and could lead to hard to debug errors when the driver is changed in the future. Would it be possible to make the behavior consistent across the SoCs instead, so that runtime PM status is ACTIVE when needed, even on SoCs without an IOMMU PM domain? > thus add > a "dev->pm_domain" checking for the SoC that has pm runtime domain. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 5614015e5b96..6fe3ee2b2bf5 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -808,7 +808,7 @@ static int mtk_iommu_remove(struct platform_device *pdev) > return 0; > } > > -static int __maybe_unused mtk_iommu_suspend(struct device *dev) > +static int __maybe_unused mtk_iommu_runtime_suspend(struct device *dev) > { > struct mtk_iommu_data *data = dev_get_drvdata(dev); > struct mtk_iommu_suspend_reg *reg = &data->reg; > @@ -826,7 +826,7 @@ static int __maybe_unused mtk_iommu_suspend(struct device *dev) > return 0; > } > > -static int __maybe_unused mtk_iommu_resume(struct device *dev) > +static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev) > { > struct mtk_iommu_data *data = dev_get_drvdata(dev); > struct mtk_iommu_suspend_reg *reg = &data->reg; > @@ -853,7 +853,25 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev) > return 0; > } > > +static int __maybe_unused mtk_iommu_suspend(struct device *dev) > +{ > + /* runtime PM is disabled when suspend in pm_runtime case. */ > + if (dev->pm_domain && pm_runtime_status_suspended(dev)) > + return 0; > + > + return mtk_iommu_runtime_suspend(dev); > +} > + > +static int __maybe_unused mtk_iommu_resume(struct device *dev) > +{ > + if (dev->pm_domain && pm_runtime_status_suspended(dev)) > + return 0; > + > + return mtk_iommu_runtime_resume(dev); > +} Wouldn't it be enough to just use pm_runtime_force_suspend() and pm_runtime_force_resume() as system sleep ops? > + > static const struct dev_pm_ops mtk_iommu_pm_ops = { > + SET_RUNTIME_PM_OPS(mtk_iommu_runtime_suspend, mtk_iommu_runtime_resume, NULL) > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_iommu_suspend, mtk_iommu_resume) > }; > > -- > 2.18.0 > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Wed, 2020-12-23 at 17:32 +0900, Tomasz Figa wrote: > On Wed, Dec 09, 2020 at 04:00:52PM +0800, Yong Wu wrote: > > This patch adds pm runtime callback. > > > > In pm runtime case, all the registers backup/restore and bclk are > > controlled in the pm_runtime callback, then pm_suspend is not needed in > > this case. > > > > runtime PM is disabled when suspend, thus we call > > pm_runtime_status_suspended instead of pm_runtime_suspended. > > > > And, m4u doesn't have its special pm runtime domain in previous SoC, in > > this case dev->power.runtime_status is RPM_SUSPENDED defaultly, > > This sounds wrong and could lead to hard to debug errors when the driver > is changed in the future. Would it be possible to make the behavior > consistent across the SoCs instead, so that runtime PM status is ACTIVE > when needed, even on SoCs without an IOMMU PM domain? Appreciate the reviewing so detailly. I have tested this. a) always call pm_runtime_enable. b) always add device_link with smi_common. Then, the runtime PM status meet expectation. We don't call pm_runtime_get_sync so often, thus, we don't always touch dev->power.lock. this is ok for us. I will use this in the next version. > > > thus add > > a "dev->pm_domain" checking for the SoC that has pm runtime domain. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > drivers/iommu/mtk_iommu.c | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index 5614015e5b96..6fe3ee2b2bf5 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -808,7 +808,7 @@ static int mtk_iommu_remove(struct platform_device *pdev) > > return 0; > > } > > > > -static int __maybe_unused mtk_iommu_suspend(struct device *dev) > > +static int __maybe_unused mtk_iommu_runtime_suspend(struct device *dev) > > { > > struct mtk_iommu_data *data = dev_get_drvdata(dev); > > struct mtk_iommu_suspend_reg *reg = &data->reg; > > @@ -826,7 +826,7 @@ static int __maybe_unused mtk_iommu_suspend(struct device *dev) > > return 0; > > } > > > > -static int __maybe_unused mtk_iommu_resume(struct device *dev) > > +static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev) > > { > > struct mtk_iommu_data *data = dev_get_drvdata(dev); > > struct mtk_iommu_suspend_reg *reg = &data->reg; > > @@ -853,7 +853,25 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev) > > return 0; > > } > > > > +static int __maybe_unused mtk_iommu_suspend(struct device *dev) > > +{ > > + /* runtime PM is disabled when suspend in pm_runtime case. */ > > + if (dev->pm_domain && pm_runtime_status_suspended(dev)) > > + return 0; > > + > > + return mtk_iommu_runtime_suspend(dev); > > +} > > + > > +static int __maybe_unused mtk_iommu_resume(struct device *dev) > > +{ > > + if (dev->pm_domain && pm_runtime_status_suspended(dev)) > > + return 0; > > + > > + return mtk_iommu_runtime_resume(dev); > > +} > > Wouldn't it be enough to just use pm_runtime_force_suspend() and > pm_runtime_force_resume() as system sleep ops? After above solution, this is ok. Thanks. > > > + > > static const struct dev_pm_ops mtk_iommu_pm_ops = { > > + SET_RUNTIME_PM_OPS(mtk_iommu_runtime_suspend, mtk_iommu_runtime_resume, NULL) > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_iommu_suspend, mtk_iommu_resume) > > }; > > > > -- > > 2.18.0 > > > > _______________________________________________ > > iommu mailing list > > iommu@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/iommu
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c index 5614015e5b96..6fe3ee2b2bf5 100644 --- a/drivers/iommu/mtk_iommu.c +++ b/drivers/iommu/mtk_iommu.c @@ -808,7 +808,7 @@ static int mtk_iommu_remove(struct platform_device *pdev) return 0; } -static int __maybe_unused mtk_iommu_suspend(struct device *dev) +static int __maybe_unused mtk_iommu_runtime_suspend(struct device *dev) { struct mtk_iommu_data *data = dev_get_drvdata(dev); struct mtk_iommu_suspend_reg *reg = &data->reg; @@ -826,7 +826,7 @@ static int __maybe_unused mtk_iommu_suspend(struct device *dev) return 0; } -static int __maybe_unused mtk_iommu_resume(struct device *dev) +static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev) { struct mtk_iommu_data *data = dev_get_drvdata(dev); struct mtk_iommu_suspend_reg *reg = &data->reg; @@ -853,7 +853,25 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev) return 0; } +static int __maybe_unused mtk_iommu_suspend(struct device *dev) +{ + /* runtime PM is disabled when suspend in pm_runtime case. */ + if (dev->pm_domain && pm_runtime_status_suspended(dev)) + return 0; + + return mtk_iommu_runtime_suspend(dev); +} + +static int __maybe_unused mtk_iommu_resume(struct device *dev) +{ + if (dev->pm_domain && pm_runtime_status_suspended(dev)) + return 0; + + return mtk_iommu_runtime_resume(dev); +} + static const struct dev_pm_ops mtk_iommu_pm_ops = { + SET_RUNTIME_PM_OPS(mtk_iommu_runtime_suspend, mtk_iommu_runtime_resume, NULL) SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_iommu_suspend, mtk_iommu_resume) };
This patch adds pm runtime callback. In pm runtime case, all the registers backup/restore and bclk are controlled in the pm_runtime callback, then pm_suspend is not needed in this case. runtime PM is disabled when suspend, thus we call pm_runtime_status_suspended instead of pm_runtime_suspended. And, m4u doesn't have its special pm runtime domain in previous SoC, in this case dev->power.runtime_status is RPM_SUSPENDED defaultly, thus add a "dev->pm_domain" checking for the SoC that has pm runtime domain. Signed-off-by: Yong Wu <yong.wu@mediatek.com> --- drivers/iommu/mtk_iommu.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)