| Message ID | 20201209080102.26626-19-yong.wu@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | MT8192 IOMMU support | expand |
On Wed, Dec 09, 2020 at 04:00:53PM +0800, Yong Wu wrote: > In the previous SoC, the M4U HW is in the EMI power domain which is > always on. the latest M4U is in the display power domain which may be > turned on/off, thus we have to add pm_runtime interface for it. > > When the engine work, the engine always enable the power and clocks for > smi-larb/smi-common, then the M4U's power will always be powered on > automatically via the device link with smi-common. > > Note: we don't enable the M4U power in iommu_map/unmap for tlb flush. > If its power already is on, of course it is ok. if the power is off, > the main tlb will be reset while M4U power on, thus the tlb flush while > m4u power off is unnecessary, just skip it. > > There will be one case that pm runctime status is not expected when tlb > flush. After boot, the display may call dma_alloc_attrs before it call > pm_runtime_get(disp-dev), then the m4u's pm status is not active inside > the dma_alloc_attrs. Since it only happens after boot, the tlb is clean > at that time, I also think this is ok. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 41 +++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 6fe3ee2b2bf5..0e9c03cbab32 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -184,6 +184,8 @@ static void mtk_iommu_tlb_flush_all(void *cookie) > struct mtk_iommu_data *data = cookie; > > for_each_m4u(data) { > + if (!pm_runtime_active(data->dev)) > + continue; Is it guaranteed that the status is active in the check above, but then the process is preempted and it goes down here? Shouldn't we do something like below? ret = pm_runtime_get_if_active(); if (!ret) continue; if (ret < 0) // handle error // Flush pm_runtime_put(); Similar comment to the other places being changed by this patch. > writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, > data->base + data->plat_data->inv_sel_reg); > writel_relaxed(F_ALL_INVLD, data->base + REG_MMU_INVALIDATE); > @@ -200,6 +202,10 @@ static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size, > u32 tmp; > > for_each_m4u(data) { > + /* skip tlb flush when pm is not active. */ > + if (!pm_runtime_active(data->dev)) > + continue; > + > spin_lock_irqsave(&data->tlb_lock, flags); > writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, > data->base + data->plat_data->inv_sel_reg); > @@ -384,6 +390,8 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, > { > struct mtk_iommu_data *data = dev_iommu_priv_get(dev); > struct mtk_iommu_domain *dom = to_mtk_domain(domain); > + struct device *m4udev = data->dev; > + bool pm_enabled = pm_runtime_enabled(m4udev); > int ret; > > if (!data) > @@ -391,12 +399,25 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, > > /* Update the pgtable base address register of the M4U HW */ > if (!data->m4u_dom) { > + if (pm_enabled) { > + ret = pm_runtime_get_sync(m4udev); > + if (ret < 0) { > + pm_runtime_put_noidle(m4udev); > + return ret; > + } > + } > ret = mtk_iommu_hw_init(data); > - if (ret) > + if (ret) { > + if (pm_enabled) > + pm_runtime_put(m4udev); > return ret; > + } > data->m4u_dom = dom; > writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, > data->base + REG_MMU_PT_BASE_ADDR); > + > + if (pm_enabled) > + pm_runtime_put(m4udev); > } > > mtk_iommu_config(data, dev, true); > @@ -747,10 +768,13 @@ static int mtk_iommu_probe(struct platform_device *pdev) > if (dev->pm_domain) { > struct device_link *link; > > + pm_runtime_enable(dev); > + > link = device_link_add(data->smicomm_dev, dev, > DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME); > if (!link) { > dev_err(dev, "Unable link %s.\n", dev_name(data->smicomm_dev)); > + pm_runtime_disable(dev); > return -EINVAL; > } > } > @@ -785,8 +809,10 @@ static int mtk_iommu_probe(struct platform_device *pdev) > out_sysfs_remove: > iommu_device_sysfs_remove(&data->iommu); > out_link_remove: > - if (dev->pm_domain) > + if (dev->pm_domain) { > device_link_remove(data->smicomm_dev, dev); > + pm_runtime_disable(dev); > + } > return ret; > } > > @@ -801,8 +827,10 @@ static int mtk_iommu_remove(struct platform_device *pdev) > bus_set_iommu(&platform_bus_type, NULL); > > clk_disable_unprepare(data->bclk); > - if (pdev->dev.pm_domain) > + if (pdev->dev.pm_domain) { > device_link_remove(data->smicomm_dev, &pdev->dev); > + pm_runtime_disable(&pdev->dev); > + } > devm_free_irq(&pdev->dev, data->irq, data); > component_master_del(&pdev->dev, &mtk_iommu_com_ops); > return 0; > @@ -834,6 +862,9 @@ static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev) > void __iomem *base = data->base; > int ret; > > + /* Avoid first resume to affect the default value of registers below. */ > + if (!m4u_dom) > + return 0; > ret = clk_prepare_enable(data->bclk); > if (ret) { > dev_err(data->dev, "Failed to enable clk(%d) in resume\n", ret); > @@ -847,9 +878,7 @@ static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev) > writel_relaxed(reg->int_main_control, base + REG_MMU_INT_MAIN_CONTROL); > writel_relaxed(reg->ivrp_paddr, base + REG_MMU_IVRP_PADDR); > writel_relaxed(reg->vld_pa_rng, base + REG_MMU_VLD_PA_RNG); > - if (m4u_dom) > - writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, > - base + REG_MMU_PT_BASE_ADDR); > + writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, base + REG_MMU_PT_BASE_ADDR); > return 0; > } > > -- > 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:36 +0900, Tomasz Figa wrote: > On Wed, Dec 09, 2020 at 04:00:53PM +0800, Yong Wu wrote: > > In the previous SoC, the M4U HW is in the EMI power domain which is > > always on. the latest M4U is in the display power domain which may be > > turned on/off, thus we have to add pm_runtime interface for it. > > > > When the engine work, the engine always enable the power and clocks for > > smi-larb/smi-common, then the M4U's power will always be powered on > > automatically via the device link with smi-common. > > > > Note: we don't enable the M4U power in iommu_map/unmap for tlb flush. > > If its power already is on, of course it is ok. if the power is off, > > the main tlb will be reset while M4U power on, thus the tlb flush while > > m4u power off is unnecessary, just skip it. > > > > There will be one case that pm runctime status is not expected when tlb > > flush. After boot, the display may call dma_alloc_attrs before it call > > pm_runtime_get(disp-dev), then the m4u's pm status is not active inside > > the dma_alloc_attrs. Since it only happens after boot, the tlb is clean > > at that time, I also think this is ok. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > drivers/iommu/mtk_iommu.c | 41 +++++++++++++++++++++++++++++++++------ > > 1 file changed, 35 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index 6fe3ee2b2bf5..0e9c03cbab32 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -184,6 +184,8 @@ static void mtk_iommu_tlb_flush_all(void *cookie) > > struct mtk_iommu_data *data = cookie; > > > > for_each_m4u(data) { > > + if (!pm_runtime_active(data->dev)) > > + continue; > > Is it guaranteed that the status is active in the check above, but then > the process is preempted and it goes down here? > > Shouldn't we do something like below? > > ret = pm_runtime_get_if_active(); > if (!ret) > continue; > if (ret < 0) > // handle error > > // Flush > > pm_runtime_put(); Make sense. Thanks. There is a comment in arm_smmu.c "avoid touching dev->power.lock in fastpaths". To avoid this here too(we have many SoC don't have power-domain). then the code will be like: bool has_pm = !!data->dev->pm_domain; if (has_pm) { if (pm_runtime_get_if_in_use(data->dev) <= 0) continue; } xxxx if (has_pm) pm_runtime_put(data->dev); > > Similar comment to the other places being changed by this patch. > > > writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, > > data->base + data->plat_data->inv_sel_reg); > > writel_relaxed(F_ALL_INVLD, data->base + REG_MMU_INVALIDATE); > > @@ -200,6 +202,10 @@ static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size, > > u32 tmp; > > > > for_each_m4u(data) { > > + /* skip tlb flush when pm is not active. */ > > + if (!pm_runtime_active(data->dev)) > > + continue; > > + > > spin_lock_irqsave(&data->tlb_lock, flags); > > writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, > > data->base + data->plat_data->inv_sel_reg); [snip]
On Tue, Dec 29, 2020 at 8:06 PM Yong Wu <yong.wu@mediatek.com> wrote: > > On Wed, 2020-12-23 at 17:36 +0900, Tomasz Figa wrote: > > On Wed, Dec 09, 2020 at 04:00:53PM +0800, Yong Wu wrote: > > > In the previous SoC, the M4U HW is in the EMI power domain which is > > > always on. the latest M4U is in the display power domain which may be > > > turned on/off, thus we have to add pm_runtime interface for it. > > > > > > When the engine work, the engine always enable the power and clocks for > > > smi-larb/smi-common, then the M4U's power will always be powered on > > > automatically via the device link with smi-common. > > > > > > Note: we don't enable the M4U power in iommu_map/unmap for tlb flush. > > > If its power already is on, of course it is ok. if the power is off, > > > the main tlb will be reset while M4U power on, thus the tlb flush while > > > m4u power off is unnecessary, just skip it. > > > > > > There will be one case that pm runctime status is not expected when tlb > > > flush. After boot, the display may call dma_alloc_attrs before it call > > > pm_runtime_get(disp-dev), then the m4u's pm status is not active inside > > > the dma_alloc_attrs. Since it only happens after boot, the tlb is clean > > > at that time, I also think this is ok. > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > > --- > > > drivers/iommu/mtk_iommu.c | 41 +++++++++++++++++++++++++++++++++------ > > > 1 file changed, 35 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > > index 6fe3ee2b2bf5..0e9c03cbab32 100644 > > > --- a/drivers/iommu/mtk_iommu.c > > > +++ b/drivers/iommu/mtk_iommu.c > > > @@ -184,6 +184,8 @@ static void mtk_iommu_tlb_flush_all(void *cookie) > > > struct mtk_iommu_data *data = cookie; > > > > > > for_each_m4u(data) { > > > + if (!pm_runtime_active(data->dev)) > > > + continue; > > > > Is it guaranteed that the status is active in the check above, but then > > the process is preempted and it goes down here? > > > > Shouldn't we do something like below? > > > > ret = pm_runtime_get_if_active(); > > if (!ret) > > continue; > > if (ret < 0) > > // handle error > > > > // Flush > > > > pm_runtime_put(); > > Make sense. Thanks. There is a comment in arm_smmu.c "avoid touching > dev->power.lock in fastpaths". To avoid this here too(we have many SoC > don't have power-domain). then the code will be like: > > bool has_pm = !!data->dev->pm_domain; > > if (has_pm) { > if (pm_runtime_get_if_in_use(data->dev) <= 0) > continue; > } > > xxxx > > if (has_pm) > pm_runtime_put(data->dev); Looks good to me, thanks. > > > > Similar comment to the other places being changed by this patch. > > > > > writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, > > > data->base + data->plat_data->inv_sel_reg); > > > writel_relaxed(F_ALL_INVLD, data->base + REG_MMU_INVALIDATE); > > > @@ -200,6 +202,10 @@ static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size, > > > u32 tmp; > > > > > > for_each_m4u(data) { > > > + /* skip tlb flush when pm is not active. */ > > > + if (!pm_runtime_active(data->dev)) > > > + continue; > > > + > > > spin_lock_irqsave(&data->tlb_lock, flags); > > > writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, > > > data->base + data->plat_data->inv_sel_reg); > [snip]
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c index 6fe3ee2b2bf5..0e9c03cbab32 100644 --- a/drivers/iommu/mtk_iommu.c +++ b/drivers/iommu/mtk_iommu.c @@ -184,6 +184,8 @@ static void mtk_iommu_tlb_flush_all(void *cookie) struct mtk_iommu_data *data = cookie; for_each_m4u(data) { + if (!pm_runtime_active(data->dev)) + continue; writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, data->base + data->plat_data->inv_sel_reg); writel_relaxed(F_ALL_INVLD, data->base + REG_MMU_INVALIDATE); @@ -200,6 +202,10 @@ static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size, u32 tmp; for_each_m4u(data) { + /* skip tlb flush when pm is not active. */ + if (!pm_runtime_active(data->dev)) + continue; + spin_lock_irqsave(&data->tlb_lock, flags); writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, data->base + data->plat_data->inv_sel_reg); @@ -384,6 +390,8 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, { struct mtk_iommu_data *data = dev_iommu_priv_get(dev); struct mtk_iommu_domain *dom = to_mtk_domain(domain); + struct device *m4udev = data->dev; + bool pm_enabled = pm_runtime_enabled(m4udev); int ret; if (!data) @@ -391,12 +399,25 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, /* Update the pgtable base address register of the M4U HW */ if (!data->m4u_dom) { + if (pm_enabled) { + ret = pm_runtime_get_sync(m4udev); + if (ret < 0) { + pm_runtime_put_noidle(m4udev); + return ret; + } + } ret = mtk_iommu_hw_init(data); - if (ret) + if (ret) { + if (pm_enabled) + pm_runtime_put(m4udev); return ret; + } data->m4u_dom = dom; writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, data->base + REG_MMU_PT_BASE_ADDR); + + if (pm_enabled) + pm_runtime_put(m4udev); } mtk_iommu_config(data, dev, true); @@ -747,10 +768,13 @@ static int mtk_iommu_probe(struct platform_device *pdev) if (dev->pm_domain) { struct device_link *link; + pm_runtime_enable(dev); + link = device_link_add(data->smicomm_dev, dev, DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME); if (!link) { dev_err(dev, "Unable link %s.\n", dev_name(data->smicomm_dev)); + pm_runtime_disable(dev); return -EINVAL; } } @@ -785,8 +809,10 @@ static int mtk_iommu_probe(struct platform_device *pdev) out_sysfs_remove: iommu_device_sysfs_remove(&data->iommu); out_link_remove: - if (dev->pm_domain) + if (dev->pm_domain) { device_link_remove(data->smicomm_dev, dev); + pm_runtime_disable(dev); + } return ret; } @@ -801,8 +827,10 @@ static int mtk_iommu_remove(struct platform_device *pdev) bus_set_iommu(&platform_bus_type, NULL); clk_disable_unprepare(data->bclk); - if (pdev->dev.pm_domain) + if (pdev->dev.pm_domain) { device_link_remove(data->smicomm_dev, &pdev->dev); + pm_runtime_disable(&pdev->dev); + } devm_free_irq(&pdev->dev, data->irq, data); component_master_del(&pdev->dev, &mtk_iommu_com_ops); return 0; @@ -834,6 +862,9 @@ static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev) void __iomem *base = data->base; int ret; + /* Avoid first resume to affect the default value of registers below. */ + if (!m4u_dom) + return 0; ret = clk_prepare_enable(data->bclk); if (ret) { dev_err(data->dev, "Failed to enable clk(%d) in resume\n", ret); @@ -847,9 +878,7 @@ static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev) writel_relaxed(reg->int_main_control, base + REG_MMU_INT_MAIN_CONTROL); writel_relaxed(reg->ivrp_paddr, base + REG_MMU_IVRP_PADDR); writel_relaxed(reg->vld_pa_rng, base + REG_MMU_VLD_PA_RNG); - if (m4u_dom) - writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, - base + REG_MMU_PT_BASE_ADDR); + writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, base + REG_MMU_PT_BASE_ADDR); return 0; }
In the previous SoC, the M4U HW is in the EMI power domain which is always on. the latest M4U is in the display power domain which may be turned on/off, thus we have to add pm_runtime interface for it. When the engine work, the engine always enable the power and clocks for smi-larb/smi-common, then the M4U's power will always be powered on automatically via the device link with smi-common. Note: we don't enable the M4U power in iommu_map/unmap for tlb flush. If its power already is on, of course it is ok. if the power is off, the main tlb will be reset while M4U power on, thus the tlb flush while m4u power off is unnecessary, just skip it. There will be one case that pm runctime status is not expected when tlb flush. After boot, the display may call dma_alloc_attrs before it call pm_runtime_get(disp-dev), then the m4u's pm status is not active inside the dma_alloc_attrs. Since it only happens after boot, the tlb is clean at that time, I also think this is ok. Signed-off-by: Yong Wu <yong.wu@mediatek.com> --- drivers/iommu/mtk_iommu.c | 41 +++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)