| Message ID | 20180301101837.27969-14-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jeffy, On Thu, Mar 1, 2018 at 7:18 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: > When the power domain is powered off, the IOMMU cannot be accessed and > register programming must be deferred until the power domain becomes > enabled. > > Add runtime PM support, and use runtime PM device link from IOMMU to > master to startup and shutdown IOMMU. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > Changes in v6: None > Changes in v5: > Avoid race about pm_runtime_get_if_in_use() and pm_runtime_enabled(). > > Changes in v4: None > Changes in v3: > Only call startup() and shutdown() when iommu attached. > Remove pm_mutex. > Check runtime PM disabled. > Check pm_runtime in rk_iommu_irq(). > > Changes in v2: None > > drivers/iommu/rockchip-iommu.c | 181 +++++++++++++++++++++++++++++++---------- > 1 file changed, 140 insertions(+), 41 deletions(-) > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index 2448a0528e39..0e0a42f41818 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -22,6 +22,7 @@ > #include <linux/of_iommu.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > > @@ -106,6 +107,7 @@ struct rk_iommu { > }; > > struct rk_iommudata { > + struct device_link *link; /* runtime PM link from IOMMU to master */ Kerneldoc comment for the struct could be added instead. > struct rk_iommu *iommu; > }; > > @@ -518,7 +520,12 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id) > u32 int_status; > dma_addr_t iova; > irqreturn_t ret = IRQ_NONE; > - int i; > + int i, err, need_runtime_put; nit: need_runtime_put could be a bool. > + > + err = pm_runtime_get_if_in_use(iommu->dev); > + if (err <= 0 && err != -EINVAL) > + return ret; > + need_runtime_put = err > 0; Generally something must be really wrong if we end up with err == 0 here, because the IOMMU must be powered on to signal an interrupt. The only case this could happen would be if the IRQ signal was shared with some device from another power domain. Is it possible on Rockchip SoCs? If not, perhaps we should have a WARN_ON() here for such case. > > WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); > > @@ -570,6 +577,9 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id) > > clk_bulk_disable(iommu->num_clocks, iommu->clocks); > > + if (need_runtime_put) > + pm_runtime_put(iommu->dev); > + > return ret; > } > > @@ -611,10 +621,20 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain, > spin_lock_irqsave(&rk_domain->iommus_lock, flags); > list_for_each(pos, &rk_domain->iommus) { > struct rk_iommu *iommu; > + int ret; > + > iommu = list_entry(pos, struct rk_iommu, node); > - WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); > - rk_iommu_zap_lines(iommu, iova, size); > - clk_bulk_disable(iommu->num_clocks, iommu->clocks); > + > + /* Only zap TLBs of IOMMUs that are powered on. */ > + ret = pm_runtime_get_if_in_use(iommu->dev); > + if (ret > 0 || ret == -EINVAL) { > + WARN_ON(clk_bulk_enable(iommu->num_clocks, > + iommu->clocks)); > + rk_iommu_zap_lines(iommu, iova, size); > + clk_bulk_disable(iommu->num_clocks, iommu->clocks); > + } > + if (ret > 0) > + pm_runtime_put(iommu->dev); > } > spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); > } > @@ -817,22 +837,30 @@ static struct rk_iommu *rk_iommu_from_dev(struct device *dev) > return data ? data->iommu : NULL; > } > > -static int rk_iommu_attach_device(struct iommu_domain *domain, > - struct device *dev) > +/* Must be called with iommu powered on and attached */ > +static void rk_iommu_shutdown(struct rk_iommu *iommu) > { > - struct rk_iommu *iommu; > + int i; > + > + /* Ignore error while disabling, just keep going */ > + WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); > + rk_iommu_enable_stall(iommu); > + rk_iommu_disable_paging(iommu); > + for (i = 0; i < iommu->num_mmu; i++) { > + rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, 0); > + rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, 0); > + } > + rk_iommu_disable_stall(iommu); > + clk_bulk_disable(iommu->num_clocks, iommu->clocks); > +} > + > +/* Must be called with iommu powered on and attached */ > +static int rk_iommu_startup(struct rk_iommu *iommu) > +{ > + struct iommu_domain *domain = iommu->domain; > struct rk_iommu_domain *rk_domain = to_rk_domain(domain); > - unsigned long flags; > int ret, i; > > - /* > - * Allow 'virtual devices' (e.g., drm) to attach to domain. > - * Such a device does not belong to an iommu group. > - */ > - iommu = rk_iommu_from_dev(dev); > - if (!iommu) > - return 0; > - > ret = clk_bulk_enable(iommu->num_clocks, iommu->clocks); > if (ret) > return ret; > @@ -845,8 +873,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, > if (ret) > goto out_disable_stall; > > - iommu->domain = domain; > - > for (i = 0; i < iommu->num_mmu; i++) { > rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, > rk_domain->dt_dma); > @@ -855,14 +881,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, > } > > ret = rk_iommu_enable_paging(iommu); > - if (ret) > - goto out_disable_stall; > - > - spin_lock_irqsave(&rk_domain->iommus_lock, flags); > - list_add_tail(&iommu->node, &rk_domain->iommus); > - spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); > - > - dev_dbg(dev, "Attached to iommu domain\n"); > > out_disable_stall: > rk_iommu_disable_stall(iommu); > @@ -877,31 +895,76 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, > struct rk_iommu *iommu; > struct rk_iommu_domain *rk_domain = to_rk_domain(domain); > unsigned long flags; > - int i; > + int ret; > > /* Allow 'virtual devices' (eg drm) to detach from domain */ > iommu = rk_iommu_from_dev(dev); > if (!iommu) > return; > > + dev_dbg(dev, "Detaching from iommu domain\n"); > + > + /* iommu already detached */ > + if (iommu->domain != domain) > + return; > + > + iommu->domain = NULL; > + > spin_lock_irqsave(&rk_domain->iommus_lock, flags); > list_del_init(&iommu->node); > spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); > > - /* Ignore error while disabling, just keep going */ > - WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); > - rk_iommu_enable_stall(iommu); > - rk_iommu_disable_paging(iommu); > - for (i = 0; i < iommu->num_mmu; i++) { > - rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, 0); > - rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, 0); > - } > - rk_iommu_disable_stall(iommu); > - clk_bulk_disable(iommu->num_clocks, iommu->clocks); > + ret = pm_runtime_get_if_in_use(iommu->dev); > + if (ret > 0 || ret == -EINVAL) > + rk_iommu_shutdown(iommu); > + if (ret > 0) > + pm_runtime_put(iommu->dev); > +} > > - iommu->domain = NULL; > +static int rk_iommu_attach_device(struct iommu_domain *domain, > + struct device *dev) > +{ > + struct rk_iommu *iommu; > + struct rk_iommu_domain *rk_domain = to_rk_domain(domain); > + unsigned long flags; > + int ret, need_runtime_put; nit: need_runtime_put could be bool Best regards, Tomasz
On 05/03/18 13:49, Tomasz Figa wrote: [...] >> @@ -518,7 +520,12 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id) >> u32 int_status; >> dma_addr_t iova; >> irqreturn_t ret = IRQ_NONE; >> - int i; >> + int i, err, need_runtime_put; > > nit: need_runtime_put could be a bool. > >> + >> + err = pm_runtime_get_if_in_use(iommu->dev); >> + if (err <= 0 && err != -EINVAL) >> + return ret; >> + need_runtime_put = err > 0; > > Generally something must be really wrong if we end up with err == 0 > here, because the IOMMU must be powered on to signal an interrupt. The > only case this could happen would be if the IRQ signal was shared with > some device from another power domain. Is it possible on Rockchip > SoCs? If not, perhaps we should have a WARN_ON() here for such case. In general, there's almost certainly some time window between the interrupt level being latched at the GIC and the IRQ actually being taken by its target CPU, in which potentially the power could be removed and/or the clocks gated - especially if there are higher-priority IRQs pending at the same time and the racing PM call is on some other CPU. Sure, it's probably unlikely, but I wouldn't necessarily consider it completely erroneous. Robin.
On Mon, Mar 5, 2018 at 11:13 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 05/03/18 13:49, Tomasz Figa wrote: > [...] >>> >>> @@ -518,7 +520,12 @@ static irqreturn_t rk_iommu_irq(int irq, void >>> *dev_id) >>> u32 int_status; >>> dma_addr_t iova; >>> irqreturn_t ret = IRQ_NONE; >>> - int i; >>> + int i, err, need_runtime_put; >> >> >> nit: need_runtime_put could be a bool. >> >>> + >>> + err = pm_runtime_get_if_in_use(iommu->dev); >>> + if (err <= 0 && err != -EINVAL) >>> + return ret; >>> + need_runtime_put = err > 0; >> >> >> Generally something must be really wrong if we end up with err == 0 >> here, because the IOMMU must be powered on to signal an interrupt. The >> only case this could happen would be if the IRQ signal was shared with >> some device from another power domain. Is it possible on Rockchip >> SoCs? If not, perhaps we should have a WARN_ON() here for such case. > > > In general, there's almost certainly some time window between the interrupt > level being latched at the GIC and the IRQ actually being taken by its > target CPU, in which potentially the power could be removed and/or the > clocks gated - especially if there are higher-priority IRQs pending at the > same time and the racing PM call is on some other CPU. Sure, it's probably > unlikely, but I wouldn't necessarily consider it completely erroneous. Clocks are not a problem here, since the handler re-enables them and clk_enable() is IRQ-safe. However, runtime PM might need sleeping, so we can't just get_sync() from the handler. I guess, we should just bail out in such case, since the power off would probably clear any internal interrupt state anyway. Also, the interrupt would be basically a page fault, during which the master device would be stalled, so it's rather unlikely that we see its driver putting the runtime PM, which would only happen after the master device resumes and competes (or something times out). So probably WARN_ON() isn't such bad idea still.
Hi Jeffy, On Tue, Mar 6, 2018 at 10:57 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote: > Hi Tomasz, > > Thanks for your reply. > > On 03/05/2018 09:49 PM, Tomasz Figa wrote: >>> >>> > struct rk_iommudata { >>> >+ struct device_link *link; /* runtime PM link from IOMMU to >>> > master */ >> >> Kerneldoc comment for the struct could be added instead. > > i saw this in the kerneldoc: > > * An MMU device exists alongside a busmaster device, both are in the same > power domain. The MMU implements DMA address translation for the > busmaster > device and shall be runtime resumed and kept active whenever and as long > as the busmaster device is active. The busmaster device's driver shall > not bind before the MMU is bound. To achieve this, a device link with > runtime PM integration is added from the busmaster device (consumer) > to the MMU device (supplier). The effect with regards to runtime PM > is the same as if the MMU was the parent of the master device. > > > maybe we can use something like: > device link with runtime PM integration from the master (consumer) to the > IOMMU (supplier). Sorry, I meant that kerneldoc comment style could be used to document this struct. https://www.kernel.org/doc/html/v4.15/doc-guide/kernel-doc.html#writing-kernel-doc-comments Best regards, Tomasz
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 2448a0528e39..0e0a42f41818 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -22,6 +22,7 @@ #include <linux/of_iommu.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/spinlock.h> @@ -106,6 +107,7 @@ struct rk_iommu { }; struct rk_iommudata { + struct device_link *link; /* runtime PM link from IOMMU to master */ struct rk_iommu *iommu; }; @@ -518,7 +520,12 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id) u32 int_status; dma_addr_t iova; irqreturn_t ret = IRQ_NONE; - int i; + int i, err, need_runtime_put; + + err = pm_runtime_get_if_in_use(iommu->dev); + if (err <= 0 && err != -EINVAL) + return ret; + need_runtime_put = err > 0; WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); @@ -570,6 +577,9 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id) clk_bulk_disable(iommu->num_clocks, iommu->clocks); + if (need_runtime_put) + pm_runtime_put(iommu->dev); + return ret; } @@ -611,10 +621,20 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain, spin_lock_irqsave(&rk_domain->iommus_lock, flags); list_for_each(pos, &rk_domain->iommus) { struct rk_iommu *iommu; + int ret; + iommu = list_entry(pos, struct rk_iommu, node); - WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); - rk_iommu_zap_lines(iommu, iova, size); - clk_bulk_disable(iommu->num_clocks, iommu->clocks); + + /* Only zap TLBs of IOMMUs that are powered on. */ + ret = pm_runtime_get_if_in_use(iommu->dev); + if (ret > 0 || ret == -EINVAL) { + WARN_ON(clk_bulk_enable(iommu->num_clocks, + iommu->clocks)); + rk_iommu_zap_lines(iommu, iova, size); + clk_bulk_disable(iommu->num_clocks, iommu->clocks); + } + if (ret > 0) + pm_runtime_put(iommu->dev); } spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); } @@ -817,22 +837,30 @@ static struct rk_iommu *rk_iommu_from_dev(struct device *dev) return data ? data->iommu : NULL; } -static int rk_iommu_attach_device(struct iommu_domain *domain, - struct device *dev) +/* Must be called with iommu powered on and attached */ +static void rk_iommu_shutdown(struct rk_iommu *iommu) { - struct rk_iommu *iommu; + int i; + + /* Ignore error while disabling, just keep going */ + WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); + rk_iommu_enable_stall(iommu); + rk_iommu_disable_paging(iommu); + for (i = 0; i < iommu->num_mmu; i++) { + rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, 0); + rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, 0); + } + rk_iommu_disable_stall(iommu); + clk_bulk_disable(iommu->num_clocks, iommu->clocks); +} + +/* Must be called with iommu powered on and attached */ +static int rk_iommu_startup(struct rk_iommu *iommu) +{ + struct iommu_domain *domain = iommu->domain; struct rk_iommu_domain *rk_domain = to_rk_domain(domain); - unsigned long flags; int ret, i; - /* - * Allow 'virtual devices' (e.g., drm) to attach to domain. - * Such a device does not belong to an iommu group. - */ - iommu = rk_iommu_from_dev(dev); - if (!iommu) - return 0; - ret = clk_bulk_enable(iommu->num_clocks, iommu->clocks); if (ret) return ret; @@ -845,8 +873,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, if (ret) goto out_disable_stall; - iommu->domain = domain; - for (i = 0; i < iommu->num_mmu; i++) { rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, rk_domain->dt_dma); @@ -855,14 +881,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, } ret = rk_iommu_enable_paging(iommu); - if (ret) - goto out_disable_stall; - - spin_lock_irqsave(&rk_domain->iommus_lock, flags); - list_add_tail(&iommu->node, &rk_domain->iommus); - spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); - - dev_dbg(dev, "Attached to iommu domain\n"); out_disable_stall: rk_iommu_disable_stall(iommu); @@ -877,31 +895,76 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, struct rk_iommu *iommu; struct rk_iommu_domain *rk_domain = to_rk_domain(domain); unsigned long flags; - int i; + int ret; /* Allow 'virtual devices' (eg drm) to detach from domain */ iommu = rk_iommu_from_dev(dev); if (!iommu) return; + dev_dbg(dev, "Detaching from iommu domain\n"); + + /* iommu already detached */ + if (iommu->domain != domain) + return; + + iommu->domain = NULL; + spin_lock_irqsave(&rk_domain->iommus_lock, flags); list_del_init(&iommu->node); spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); - /* Ignore error while disabling, just keep going */ - WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)); - rk_iommu_enable_stall(iommu); - rk_iommu_disable_paging(iommu); - for (i = 0; i < iommu->num_mmu; i++) { - rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, 0); - rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, 0); - } - rk_iommu_disable_stall(iommu); - clk_bulk_disable(iommu->num_clocks, iommu->clocks); + ret = pm_runtime_get_if_in_use(iommu->dev); + if (ret > 0 || ret == -EINVAL) + rk_iommu_shutdown(iommu); + if (ret > 0) + pm_runtime_put(iommu->dev); +} - iommu->domain = NULL; +static int rk_iommu_attach_device(struct iommu_domain *domain, + struct device *dev) +{ + struct rk_iommu *iommu; + struct rk_iommu_domain *rk_domain = to_rk_domain(domain); + unsigned long flags; + int ret, need_runtime_put; + + /* + * Allow 'virtual devices' (e.g., drm) to attach to domain. + * Such a device does not belong to an iommu group. + */ + iommu = rk_iommu_from_dev(dev); + if (!iommu) + return 0; + + dev_dbg(dev, "Attaching to iommu domain\n"); + + /* iommu already attached */ + if (iommu->domain == domain) + return 0; + + if (iommu->domain) + rk_iommu_detach_device(iommu->domain, dev); + + iommu->domain = domain; + + spin_lock_irqsave(&rk_domain->iommus_lock, flags); + list_add_tail(&iommu->node, &rk_domain->iommus); + spin_unlock_irqrestore(&rk_domain->iommus_lock, flags); + + ret = pm_runtime_get_if_in_use(iommu->dev); + if (ret <= 0 && ret != -EINVAL) + return 0; + need_runtime_put = ret > 0; + + ret = rk_iommu_startup(iommu); + if (ret) + rk_iommu_detach_device(iommu->domain, dev); + + if (need_runtime_put) + pm_runtime_put(iommu->dev); - dev_dbg(dev, "Detached from iommu domain\n"); + return ret; } static struct iommu_domain *rk_iommu_domain_alloc(unsigned type) @@ -989,17 +1052,21 @@ static int rk_iommu_add_device(struct device *dev) { struct iommu_group *group; struct rk_iommu *iommu; + struct rk_iommudata *data; - iommu = rk_iommu_from_dev(dev); - if (!iommu) + data = dev->archdata.iommu; + if (!data) return -ENODEV; + iommu = rk_iommu_from_dev(dev); + group = iommu_group_get_for_dev(dev); if (IS_ERR(group)) return PTR_ERR(group); iommu_group_put(group); iommu_device_link(&iommu->iommu, dev); + data->link = device_link_add(dev, iommu->dev, DL_FLAG_PM_RUNTIME); return 0; } @@ -1007,9 +1074,11 @@ static int rk_iommu_add_device(struct device *dev) static void rk_iommu_remove_device(struct device *dev) { struct rk_iommu *iommu; + struct rk_iommudata *data = dev->archdata.iommu; iommu = rk_iommu_from_dev(dev); + device_link_del(data->link); iommu_device_unlink(&iommu->iommu, dev); iommu_group_remove_device(dev); } @@ -1135,6 +1204,8 @@ static int rk_iommu_probe(struct platform_device *pdev) bus_set_iommu(&platform_bus_type, &rk_iommu_ops); + pm_runtime_enable(dev); + return 0; err_remove_sysfs: iommu_device_sysfs_remove(&iommu->iommu); @@ -1143,6 +1214,33 @@ static int rk_iommu_probe(struct platform_device *pdev) return err; } +static int __maybe_unused rk_iommu_suspend(struct device *dev) +{ + struct rk_iommu *iommu = dev_get_drvdata(dev); + + if (!iommu->domain) + return 0; + + rk_iommu_shutdown(iommu); + return 0; +} + +static int __maybe_unused rk_iommu_resume(struct device *dev) +{ + struct rk_iommu *iommu = dev_get_drvdata(dev); + + if (!iommu->domain) + return 0; + + return rk_iommu_startup(iommu); +} + +static const struct dev_pm_ops rk_iommu_pm_ops = { + SET_RUNTIME_PM_OPS(rk_iommu_suspend, rk_iommu_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) +}; + static const struct of_device_id rk_iommu_dt_ids[] = { { .compatible = "rockchip,iommu" }, { /* sentinel */ } @@ -1154,6 +1252,7 @@ static struct platform_driver rk_iommu_driver = { .driver = { .name = "rk_iommu", .of_match_table = rk_iommu_dt_ids, + .pm = &rk_iommu_pm_ops, .suppress_bind_attrs = true, }, };
When the power domain is powered off, the IOMMU cannot be accessed and register programming must be deferred until the power domain becomes enabled. Add runtime PM support, and use runtime PM device link from IOMMU to master to startup and shutdown IOMMU. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- Changes in v6: None Changes in v5: Avoid race about pm_runtime_get_if_in_use() and pm_runtime_enabled(). Changes in v4: None Changes in v3: Only call startup() and shutdown() when iommu attached. Remove pm_mutex. Check runtime PM disabled. Check pm_runtime in rk_iommu_irq(). Changes in v2: None drivers/iommu/rockchip-iommu.c | 181 +++++++++++++++++++++++++++++++---------- 1 file changed, 140 insertions(+), 41 deletions(-)