| Message ID | 20201209080102.26626-1-yong.wu@mediatek.com |
|---|---|
| Headers | show |
| Series | MT8192 IOMMU support | expand |
On Wed, Dec 09, 2020 at 04:00:44PM +0800, Yong Wu wrote: > MediaTek extend the bit5 in lvl1 and lvl2 descriptor as PA34. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > Acked-by: Will Deacon <will@kernel.org> > Reviewed-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/io-pgtable-arm-v7s.c | 9 +++++++-- > drivers/iommu/mtk_iommu.c | 2 +- > include/linux/io-pgtable.h | 4 ++-- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c > index e880745ab1e8..4d0aa079470f 100644 > --- a/drivers/iommu/io-pgtable-arm-v7s.c > +++ b/drivers/iommu/io-pgtable-arm-v7s.c > @@ -112,9 +112,10 @@ > #define ARM_V7S_TEX_MASK 0x7 > #define ARM_V7S_ATTR_TEX(val) (((val) & ARM_V7S_TEX_MASK) << ARM_V7S_TEX_SHIFT) > > -/* MediaTek extend the two bits for PA 32bit/33bit */ > +/* MediaTek extend the bits below for PA 32bit/33bit/34bit */ > #define ARM_V7S_ATTR_MTK_PA_BIT32 BIT(9) > #define ARM_V7S_ATTR_MTK_PA_BIT33 BIT(4) > +#define ARM_V7S_ATTR_MTK_PA_BIT34 BIT(5) > > /* *well, except for TEX on level 2 large pages, of course :( */ > #define ARM_V7S_CONT_PAGE_TEX_SHIFT 6 > @@ -194,6 +195,8 @@ static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int lvl, > pte |= ARM_V7S_ATTR_MTK_PA_BIT32; > if (paddr & BIT_ULL(33)) > pte |= ARM_V7S_ATTR_MTK_PA_BIT33; > + if (paddr & BIT_ULL(34)) > + pte |= ARM_V7S_ATTR_MTK_PA_BIT34; > return pte; > } > > @@ -218,6 +221,8 @@ static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl, > paddr |= BIT_ULL(32); > if (pte & ARM_V7S_ATTR_MTK_PA_BIT33) > paddr |= BIT_ULL(33); > + if (pte & ARM_V7S_ATTR_MTK_PA_BIT34) > + paddr |= BIT_ULL(34); > return paddr; > } > > @@ -754,7 +759,7 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg, > if (cfg->ias > ARM_V7S_ADDR_BITS) > return NULL; > > - if (cfg->oas > (arm_v7s_is_mtk_enabled(cfg) ? 34 : ARM_V7S_ADDR_BITS)) > + if (cfg->oas > (arm_v7s_is_mtk_enabled(cfg) ? 35 : ARM_V7S_ADDR_BITS)) > return NULL; > > if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 6451d83753e1..ec3c87d4b172 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -320,7 +320,7 @@ static int mtk_iommu_domain_finalise(struct mtk_iommu_domain *dom) > IO_PGTABLE_QUIRK_ARM_MTK_EXT, > .pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap, > .ias = 32, > - .oas = 34, > + .oas = 35, Shouldn't this be set according to the real hardware capabilities, instead of always setting it to 35? Best regards, Tomasz > .tlb = &mtk_iommu_flush_ops, > .iommu_dev = data->dev, > }; > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h > index 4cde111e425b..1ae0757f4f94 100644 > --- a/include/linux/io-pgtable.h > +++ b/include/linux/io-pgtable.h > @@ -77,8 +77,8 @@ struct io_pgtable_cfg { > * TLB maintenance when mapping as well as when unmapping. > * > * IO_PGTABLE_QUIRK_ARM_MTK_EXT: (ARM v7s format) MediaTek IOMMUs extend > - * to support up to 34 bits PA where the bit32 and bit33 are > - * encoded in the bit9 and bit4 of the PTE respectively. > + * to support up to 35 bits PA where the bit32, bit33 and bit34 are > + * encoded in the bit9, bit4 and bit5 of the PTE respectively. > * > * IO_PGTABLE_QUIRK_NON_STRICT: Skip issuing synchronous leaf TLBIs > * on unmap, for DMA domains using the flush queue mechanism for > -- > 2.18.0 > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Wed, Dec 09, 2020 at 04:00:50PM +0800, Yong Wu wrote: > Add fail handle for iommu_device_sysfs_add and iommu_device_register. > > Fixes: b16c0170b53c ("iommu/mediatek: Make use of iommu_device_register interface") > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 39478cfbe0f1..09c8c58feb78 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -746,7 +746,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) > > ret = iommu_device_register(&data->iommu); > if (ret) > - return ret; > + goto out_sysfs_remove; > > spin_lock_init(&data->tlb_lock); > list_add_tail(&data->list, &m4ulist); > @@ -754,7 +754,16 @@ static int mtk_iommu_probe(struct platform_device *pdev) > if (!iommu_present(&platform_bus_type)) > bus_set_iommu(&platform_bus_type, &mtk_iommu_ops); > > - return component_master_add_with_match(dev, &mtk_iommu_com_ops, match); > + ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, match); > + if (ret) > + goto out_dev_unreg; > + return ret; > + > +out_dev_unreg: Shouldn't other operations be undone as well? I can see that above bus_set_iommu() is set and an entry is added to m4ulist. > + iommu_device_unregister(&data->iommu); > +out_sysfs_remove: > + iommu_device_sysfs_remove(&data->iommu); > + return ret; > } > > static int mtk_iommu_remove(struct platform_device *pdev) > -- > 2.18.0 > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Wed, Dec 09, 2020 at 04:00:51PM +0800, Yong Wu wrote: > In the lastest SoC, M4U has its special power domain. thus, If the engine > begin to work, it should help enable the power for M4U firstly. > Currently if the engine work, it always enable the power/clocks for > smi-larbs/smi-common. This patch adds device_link for smi-common and M4U. > then, if smi-common power is enabled, the M4U power also is powered on > automatically. > > Normally M4U connect with several smi-larbs and their smi-common always > are the same, In this patch it get smi-common dev from the first smi-larb > device(i==0), then add the device_link only while m4u has power-domain. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 30 ++++++++++++++++++++++++++++-- > drivers/iommu/mtk_iommu.h | 1 + > 2 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 09c8c58feb78..5614015e5b96 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -20,6 +20,7 @@ > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #include <linux/regmap.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > @@ -706,7 +707,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) > return larb_nr; > > for (i = 0; i < larb_nr; i++) { > - struct device_node *larbnode; > + struct device_node *larbnode, *smicomm_node; > struct platform_device *plarbdev; > u32 id; > > @@ -732,6 +733,26 @@ static int mtk_iommu_probe(struct platform_device *pdev) > > component_match_add_release(dev, &match, release_of, > compare_of, larbnode); > + if (i != 0) > + continue; How about using the last larb instead and moving the code below outside of the loop? > + smicomm_node = of_parse_phandle(larbnode, "mediatek,smi", 0); > + if (!smicomm_node) > + return -EINVAL; > + > + plarbdev = of_find_device_by_node(smicomm_node); > + of_node_put(smicomm_node); > + data->smicomm_dev = &plarbdev->dev; > + } > + > + if (dev->pm_domain) { > + struct device_link *link; > + > + 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)); > + return -EINVAL; > + } > } > > platform_set_drvdata(pdev, data); > @@ -739,7 +760,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) > ret = iommu_device_sysfs_add(&data->iommu, dev, NULL, > "mtk-iommu.%pa", &ioaddr); > if (ret) > - return ret; > + goto out_link_remove; > > iommu_device_set_ops(&data->iommu, &mtk_iommu_ops); > iommu_device_set_fwnode(&data->iommu, &pdev->dev.of_node->fwnode); > @@ -763,6 +784,9 @@ static int mtk_iommu_probe(struct platform_device *pdev) > iommu_device_unregister(&data->iommu); > out_sysfs_remove: > iommu_device_sysfs_remove(&data->iommu); > +out_link_remove: > + if (dev->pm_domain) > + device_link_remove(data->smicomm_dev, dev); > return ret; > } > > @@ -777,6 +801,8 @@ 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) > + device_link_remove(data->smicomm_dev, &pdev->dev); > devm_free_irq(&pdev->dev, data->irq, data); > component_master_del(&pdev->dev, &mtk_iommu_com_ops); > return 0; > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h > index d0c93652bdbe..5e03a029c4dc 100644 > --- a/drivers/iommu/mtk_iommu.h > +++ b/drivers/iommu/mtk_iommu.h > @@ -68,6 +68,7 @@ struct mtk_iommu_data { > > struct iommu_device iommu; > const struct mtk_iommu_plat_data *plat_data; > + struct device *smicomm_dev; > > struct dma_iommu_mapping *mapping; /* For mtk_iommu_v1.c */ > > -- > 2.18.0 > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
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, 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:25 +0900, Tomasz Figa wrote: > On Wed, Dec 09, 2020 at 04:00:50PM +0800, Yong Wu wrote: > > Add fail handle for iommu_device_sysfs_add and iommu_device_register. > > > > Fixes: b16c0170b53c ("iommu/mediatek: Make use of iommu_device_register interface") > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > drivers/iommu/mtk_iommu.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index 39478cfbe0f1..09c8c58feb78 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -746,7 +746,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) > > > > ret = iommu_device_register(&data->iommu); > > if (ret) > > - return ret; > > + goto out_sysfs_remove; > > > > spin_lock_init(&data->tlb_lock); > > list_add_tail(&data->list, &m4ulist); > > @@ -754,7 +754,16 @@ static int mtk_iommu_probe(struct platform_device *pdev) > > if (!iommu_present(&platform_bus_type)) > > bus_set_iommu(&platform_bus_type, &mtk_iommu_ops); > > > > - return component_master_add_with_match(dev, &mtk_iommu_com_ops, match); > > + ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, match); > > + if (ret) > > + goto out_dev_unreg; > > + return ret; > > + > > +out_dev_unreg: > > Shouldn't other operations be undone as well? I can see that above > bus_set_iommu() is set and an entry is added to m4ulist. Oh. Yes. I will add them. and remove the fixes tag since they are not introduced by it. these error handle are not added in the first version. > > > + iommu_device_unregister(&data->iommu); > > +out_sysfs_remove: > > + iommu_device_sysfs_remove(&data->iommu); > > + return ret; > > } > > > > static int mtk_iommu_remove(struct platform_device *pdev) > > -- > > 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
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 Wed, 2020-12-23 at 17:20 +0900, Tomasz Figa wrote: > On Wed, Dec 09, 2020 at 04:00:44PM +0800, Yong Wu wrote: > > MediaTek extend the bit5 in lvl1 and lvl2 descriptor as PA34. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > Acked-by: Will Deacon <will@kernel.org> > > Reviewed-by: Robin Murphy <robin.murphy@arm.com> > > --- > > drivers/iommu/io-pgtable-arm-v7s.c | 9 +++++++-- > > drivers/iommu/mtk_iommu.c | 2 +- > > include/linux/io-pgtable.h | 4 ++-- > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c > > index e880745ab1e8..4d0aa079470f 100644 > > --- a/drivers/iommu/io-pgtable-arm-v7s.c > > +++ b/drivers/iommu/io-pgtable-arm-v7s.c > > @@ -112,9 +112,10 @@ > > #define ARM_V7S_TEX_MASK 0x7 > > #define ARM_V7S_ATTR_TEX(val) (((val) & ARM_V7S_TEX_MASK) << ARM_V7S_TEX_SHIFT) > > > > -/* MediaTek extend the two bits for PA 32bit/33bit */ > > +/* MediaTek extend the bits below for PA 32bit/33bit/34bit */ > > #define ARM_V7S_ATTR_MTK_PA_BIT32 BIT(9) > > #define ARM_V7S_ATTR_MTK_PA_BIT33 BIT(4) > > +#define ARM_V7S_ATTR_MTK_PA_BIT34 BIT(5) > > > > /* *well, except for TEX on level 2 large pages, of course :( */ > > #define ARM_V7S_CONT_PAGE_TEX_SHIFT 6 > > @@ -194,6 +195,8 @@ static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int lvl, > > pte |= ARM_V7S_ATTR_MTK_PA_BIT32; > > if (paddr & BIT_ULL(33)) > > pte |= ARM_V7S_ATTR_MTK_PA_BIT33; > > + if (paddr & BIT_ULL(34)) > > + pte |= ARM_V7S_ATTR_MTK_PA_BIT34; > > return pte; > > } > > > > @@ -218,6 +221,8 @@ static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl, > > paddr |= BIT_ULL(32); > > if (pte & ARM_V7S_ATTR_MTK_PA_BIT33) > > paddr |= BIT_ULL(33); > > + if (pte & ARM_V7S_ATTR_MTK_PA_BIT34) > > + paddr |= BIT_ULL(34); > > return paddr; > > } > > > > @@ -754,7 +759,7 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg, > > if (cfg->ias > ARM_V7S_ADDR_BITS) > > return NULL; > > > > - if (cfg->oas > (arm_v7s_is_mtk_enabled(cfg) ? 34 : ARM_V7S_ADDR_BITS)) > > + if (cfg->oas > (arm_v7s_is_mtk_enabled(cfg) ? 35 : ARM_V7S_ADDR_BITS)) > > return NULL; > > > > if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index 6451d83753e1..ec3c87d4b172 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -320,7 +320,7 @@ static int mtk_iommu_domain_finalise(struct mtk_iommu_domain *dom) > > IO_PGTABLE_QUIRK_ARM_MTK_EXT, > > .pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap, > > .ias = 32, > > - .oas = 34, > > + .oas = 35, > > Shouldn't this be set according to the real hardware capabilities, > instead of always setting it to 35? Here only make the code clean. 35 is ok for all the SoC. But you are right from the HW point, the logic is like this: if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE)) dom->cfg.oas = data->enable_4GB ? 33 : 32; else dom->cfg.oas = 35; I will use this in next version. > > Best regards, > Tomasz > > > .tlb = &mtk_iommu_flush_ops, > > .iommu_dev = data->dev, > > }; > > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h > > index 4cde111e425b..1ae0757f4f94 100644 > > --- a/include/linux/io-pgtable.h > > +++ b/include/linux/io-pgtable.h > > @@ -77,8 +77,8 @@ struct io_pgtable_cfg { > > * TLB maintenance when mapping as well as when unmapping. > > * > > * IO_PGTABLE_QUIRK_ARM_MTK_EXT: (ARM v7s format) MediaTek IOMMUs extend > > - * to support up to 34 bits PA where the bit32 and bit33 are > > - * encoded in the bit9 and bit4 of the PTE respectively. > > + * to support up to 35 bits PA where the bit32, bit33 and bit34 are > > + * encoded in the bit9, bit4 and bit5 of the PTE respectively. > > * > > * IO_PGTABLE_QUIRK_NON_STRICT: Skip issuing synchronous leaf TLBIs > > * on unmap, for DMA domains using the flush queue mechanism for > > -- > > 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:29 +0900, Tomasz Figa wrote: > On Wed, Dec 09, 2020 at 04:00:51PM +0800, Yong Wu wrote: > > In the lastest SoC, M4U has its special power domain. thus, If the engine > > begin to work, it should help enable the power for M4U firstly. > > Currently if the engine work, it always enable the power/clocks for > > smi-larbs/smi-common. This patch adds device_link for smi-common and M4U. > > then, if smi-common power is enabled, the M4U power also is powered on > > automatically. > > > > Normally M4U connect with several smi-larbs and their smi-common always > > are the same, In this patch it get smi-common dev from the first smi-larb > > device(i==0), then add the device_link only while m4u has power-domain. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > drivers/iommu/mtk_iommu.c | 30 ++++++++++++++++++++++++++++-- > > drivers/iommu/mtk_iommu.h | 1 + > > 2 files changed, 29 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index 09c8c58feb78..5614015e5b96 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -20,6 +20,7 @@ > > #include <linux/of_irq.h> > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > #include <linux/regmap.h> > > #include <linux/slab.h> > > #include <linux/spinlock.h> > > @@ -706,7 +707,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) > > return larb_nr; > > > > for (i = 0; i < larb_nr; i++) { > > - struct device_node *larbnode; > > + struct device_node *larbnode, *smicomm_node; > > struct platform_device *plarbdev; > > u32 id; > > > > @@ -732,6 +733,26 @@ static int mtk_iommu_probe(struct platform_device *pdev) > > > > component_match_add_release(dev, &match, release_of, > > compare_of, larbnode); > > + if (i != 0) > > + continue; > > How about using the last larb instead and moving the code below outside > of the loop? Of course OK. Thanks.
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]
This patch mainly adds support for mt8192 Multimedia IOMMU and SMI. mt8192 also is MTK IOMMU gen2 which uses ARM Short-Descriptor translation table format. The M4U-SMI HW diagram is as below: EMI | M4U | ------------ SMI Common ------------ | +-------+------+------+----------------------+-------+ | | | | ...... | | | | | | | | larb0 larb1 larb2 larb4 ...... larb19 larb20 disp0 disp1 mdp vdec IPE IPE All the connections are HW fixed, SW can NOT adjust it. Comparing with the preview SoC, this patchset mainly adds two new functions: a) add iova 34 bits support. b) add multi domains support since several HW has the special iova region requirement. change note: v5: a) Add a new patch for the header guard for smi-larb-port.h in [5/27]. b) Add a new patch for error handle for iommu_device_sysfs_add and iommu_device_register[15/27]. c) Add a flag for the iova "ias == 34" case. the previous SoC still keep 32bits to save 16KB*3 lvl1 pgtable memory[13/27]. d) Add include <linux/bitfield.h> for FIELD_GET build fail. e) In PM power domain patch, add a checking "pm_runtime_enabled" when call pm_runtime_get_sync for non power-domain case. and add a pm_runtime_put_noidle while pm_runtime_get_sync fail case. v4: https://lore.kernel.org/linux-iommu/20201111123838.15682-1-yong.wu@mediatek.com/ a) rebase on v5.10-rc1 b) Move the smi part to a independent patchset. c) Improve v7s code from Robin and Will. d) Add a mediatek iommu entry patch in MAINTAIN. v3: https://lore.kernel.org/linux-iommu/20200930070647.10188-1-yong.wu@mediatek.com/ a) Fix DT schema issue commented from Rob. b) Fix a v7s issue. Use "_lvl" instead of "_l" in the macro(ARM_V7S_PTES_PER_LVL) since it is called in ARM_V7S_LVL_IDX which has already used "_l". c) Fix a PM suspend issue: Avoid pm suspend in pm runtime case. v2: https://lore.kernel.org/linux-iommu/20200905080920.13396-1-yong.wu@mediatek.com/ a) Convert IOMMU/SMI dt-binding to DT schema. b) Fix some comment from Pi-Hsun and Nicolas. like use generic_iommu_put_resv_regions. c) Reword some comment, like add how to use domain-id. v1: https://lore.kernel.org/linux-iommu/20200711064846.16007-1-yong.wu@mediatek.com/ Yong Wu (27): dt-bindings: iommu: mediatek: Convert IOMMU to DT schema dt-bindings: memory: mediatek: Add a common larb-port header file dt-bindings: memory: mediatek: Extend LARB_NR_MAX to 32 dt-bindings: memory: mediatek: Add domain definition dt-bindings: memory: mediatek: Rename header guard for SMI header file dt-bindings: mediatek: Add binding for mt8192 IOMMU iommu/mediatek: Use the common mtk-smi-larb-port.h iommu/io-pgtable-arm-v7s: Use ias to check the valid iova in unmap iommu/io-pgtable-arm-v7s: Extend PA34 for MediaTek iommu/io-pgtable-arm-v7s: Clarify LVL_SHIFT/BITS macro iommu/io-pgtable-arm-v7s: Add cfg as a param in some macros iommu/io-pgtable-arm-v7s: Quad lvl1 pgtable for MediaTek iommu/mediatek: Add a flag for iova_34 bit case iommu/mediatek: Move hw_init into attach_device iommu/mediatek: Add fail handle for sysfs_add and device_register iommu/mediatek: Add device link for smi-common and m4u iommu/mediatek: Add pm runtime callback iommu/mediatek: Add power-domain operation iommu/mediatek: Add iova reserved function iommu/mediatek: Add single domain iommu/mediatek: Support master use iova over 32bit iommu/mediatek: Support up to 34bit iova in tlb flush iommu/mediatek: Support report iova 34bit translation fault in ISR iommu/mediatek: Add support for multi domain iommu/mediatek: Adjust the structure iommu/mediatek: Add mt8192 support MAINTAINERS: Add entry for MediaTek IOMMU .../bindings/iommu/mediatek,iommu.txt | 105 ------- .../bindings/iommu/mediatek,iommu.yaml | 183 +++++++++++ MAINTAINERS | 9 + drivers/iommu/io-pgtable-arm-v7s.c | 56 ++-- drivers/iommu/mtk_iommu.c | 289 +++++++++++++++--- drivers/iommu/mtk_iommu.h | 11 +- drivers/memory/mtk-smi.c | 8 + include/dt-bindings/memory/mt2701-larb-port.h | 4 +- include/dt-bindings/memory/mt2712-larb-port.h | 6 +- include/dt-bindings/memory/mt6779-larb-port.h | 6 +- include/dt-bindings/memory/mt8167-larb-port.h | 6 +- include/dt-bindings/memory/mt8173-larb-port.h | 6 +- include/dt-bindings/memory/mt8183-larb-port.h | 6 +- include/dt-bindings/memory/mt8192-larb-port.h | 240 +++++++++++++++ .../dt-bindings/memory/mtk-smi-larb-port.h | 22 ++ include/linux/io-pgtable.h | 4 +- include/soc/mediatek/smi.h | 3 +- 17 files changed, 764 insertions(+), 200 deletions(-) delete mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt create mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml create mode 100644 include/dt-bindings/memory/mt8192-larb-port.h create mode 100644 include/dt-bindings/memory/mtk-smi-larb-port.h