Messages in this thread | | | Subject | Re: [PATCH v5 17/27] iommu/mediatek: Add pm runtime callback | From | Yong Wu <> | Date | Tue, 29 Dec 2020 19:06:00 +0800 |
| |
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
| |