| Message ID | 1475136751-31340-3-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: > This patch uses recently introduced device links to track the runtime pm > state of the master's device. This way each SYSMMU controller is runtime > activated when its master's device is active instead of? BTW what is the master device of a SYSMMU? I have no clue about these IOMMU devices here. > and can save/restore its state instead of being enabled all the time. I take it this means currently even if the master device is disabled (whatever that is) all SYSMMU controllers are kept enabled, is that right? The issue here is this wastes power? Or what is the issue? > This way SYSMMU controllers no > longer prevents respective power domains to be turned off when master's > device is not used. So when the master device is idle we want to also remove power from the controllers ? How much power does this save on a typical device in the market BTW ? > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++------------------------- > 1 file changed, 94 insertions(+), 131 deletions(-) I'm reviewing the device link patches now but since this is a demo of use of that I'll note the changes here are pretty large and it makes it terribly difficult for review. Is there any way this patch can be split up in to logical atomic pieces that only do one task upon change ? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Luis On 2016-10-06 19:37, Luis R. Rodriguez wrote: > On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: >> This patch uses recently introduced device links to track the runtime pm >> state of the master's device. This way each SYSMMU controller is runtime >> activated when its master's device is active > instead of? instead of keeping SYSMMU controller runtime active all the time. > BTW what is the master device of a SYSMMU? I have no clue about these > IOMMU devices here. Here is a more detailed description of IOMMU hardware I wrote a few days ago for Ulf: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231006.html In short: there is a SYSMMU controller and its master device - a device, which performs DMA operations. That SYSMMU sits in between system memory and the master device, so it performs mapping of DMA addresses to physical memory addresses on each DMA operation. > >> and can save/restore its state instead of being enabled all the time. > I take it this means currently even if the master device is disabled > (whatever that is) all SYSMMU controllers are kept enabled, is that right? > The issue here is this wastes power? Or what is the issue? Yes, the issue here is the fact that SYSMMU is kept active all the time, what in turn prevent the power domain for turning off even if master device doesn't do anything and is already suspended. This directly (some clocks enabled) and in-directly (leakage current) causes power looses. > >> This way SYSMMU controllers no >> longer prevents respective power domains to be turned off when master's >> device is not used. > So when the master device is idle we want to also remove power from the > controllers ? How much power does this save on a typical device in the > market BTW ? The main purpose of this patchset is to let power domains to be turned off, because with the current code all domains are all the time turned on, because SYSMMU controllers prevent them from turning them off. If you want I can measure the power consumption of the idle board with all domains enabled and disabled if you want to see the numbers. On the other board disabling most power domains in idle state (the clocks were already disabled) gave me about 20mA savings (at 3.7V), what is a significant value for the battery powered device. > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++------------------------- >> 1 file changed, 94 insertions(+), 131 deletions(-) > I'm reviewing the device link patches now but since this is a demo of > use of that I'll note the changes here are pretty large and it makes > it terribly difficult for review. Is there any way this patch can be split > up in to logical atomic pieces that only do one task upon change ? I will try to split it a bit, but I cannot promise that much can be done to improve readability for someone not very familiar with the driver internals. Best regards
On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote: > Hi Luis > > > On 2016-10-06 19:37, Luis R. Rodriguez wrote: > > On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: > > > This patch uses recently introduced device links to track the runtime pm > > > state of the master's device. This way each SYSMMU controller is runtime > > > activated when its master's device is active > > instead of? > > instead of keeping SYSMMU controller runtime active all the time. I thought Rafael's work was for suspend/resume, not for runtime suspend. Is it for both ? Because as far as I can tell this was painted to help with suspend/resume ? > > BTW what is the master device of a SYSMMU? I have no clue about these > > IOMMU devices here. > > Here is a more detailed description of IOMMU hardware I wrote a few days ago > for Ulf: > http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231006.html > > In short: there is a SYSMMU controller and its master device - a device, > which performs DMA operations. That SYSMMU sits in between system memory > and the master device, so it performs mapping of DMA addresses to physical > memory addresses on each DMA operation. So you seek a run time power optimization ? Or a fix on suspend? Or both? > > > and can save/restore its state instead of being enabled all the time. > > I take it this means currently even if the master device is disabled > > (whatever that is) all SYSMMU controllers are kept enabled, is that right? > > The issue here is this wastes power? Or what is the issue? > > Yes, the issue here is the fact that SYSMMU is kept active all the time, > what in turn prevent the power domain for turning off even if master device > doesn't do anything and is already suspended. This directly (some clocks > enabled) and in-directly (leakage current) causes power looses. Thanks for the confirmation so really the biggest concern here was run time PM. > > > This way SYSMMU controllers no > > > longer prevents respective power domains to be turned off when master's > > > device is not used. > > So when the master device is idle we want to also remove power from the > > controllers ? How much power does this save on a typical device in the > > market BTW ? > > The main purpose of this patchset is to let power domains to be turned off, > because with the current code all domains are all the time turned on, > because > SYSMMU controllers prevent them from turning them off. I see.. I think the audio folks already addressed this with DAPM, but granted this was for audio. Then I was also referred to the DRM / Audio component framework, has this been looked into? v4l folks have v4l async stuff but its not clear if that help with run time PM. I'm mentioning these given it'd be silly to re-invent the wheel, additionally if we now have a generic solution everyone can jump on board with there is quite a bit of work we can do to dump a lot of old legacy crap. > If you want I can measure the power consumption of the idle board with all > domains enabled and disabled if you want to see the numbers. On the other > board > disabling most power domains in idle state (the clocks were already > disabled) > gave me about 20mA savings (at 3.7V), what is a significant value for the > battery powered device. Thanks, this means nothing to me, however it would be value-add to the commit log as anyone reviewing this can understand what the goal / savings was for exactly. > > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > --- > > > drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++------------------------- > > > 1 file changed, 94 insertions(+), 131 deletions(-) > > I'm reviewing the device link patches now but since this is a demo of > > use of that I'll note the changes here are pretty large and it makes > > it terribly difficult for review. Is there any way this patch can be split > > up in to logical atomic pieces that only do one task upon change ? > > I will try to split it a bit, but I cannot promise that much can be done > to improve readability for someone not very familiar with the driver > internals. I've heard this before, I don't buy it but lets see! Luis -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Luis, On 2016-11-08 23:14, Luis R. Rodriguez wrote: > On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote: >> Hi Luis >> >> >> On 2016-10-06 19:37, Luis R. Rodriguez wrote: >>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: >>>> This patch uses recently introduced device links to track the runtime pm >>>> state of the master's device. This way each SYSMMU controller is runtime >>>> activated when its master's device is active >>> instead of? >> instead of keeping SYSMMU controller runtime active all the time. > I thought Rafael's work was for suspend/resume, not for runtime suspend. > Is it for both ? Yes, it solves both problems, although the suspend/resume was easy to workaround just by using LATE_SLEEP_OPS. > Because as far as I can tell this was painted to help > with suspend/resume ? It also helped to remove the suspend/resume workaround. >>> BTW what is the master device of a SYSMMU? I have no clue about these >>> IOMMU devices here. >> Here is a more detailed description of IOMMU hardware I wrote a few days ago >> for Ulf: >> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231006.html >> >> In short: there is a SYSMMU controller and its master device - a device, >> which performs DMA operations. That SYSMMU sits in between system memory >> and the master device, so it performs mapping of DMA addresses to physical >> memory addresses on each DMA operation. > So you seek a run time power optimization ? Or a fix on suspend? Or both? The main reason for using device links was to implement proper runtime power optimization. >>>> and can save/restore its state instead of being enabled all the time. >>> I take it this means currently even if the master device is disabled >>> (whatever that is) all SYSMMU controllers are kept enabled, is that right? >>> The issue here is this wastes power? Or what is the issue? >> Yes, the issue here is the fact that SYSMMU is kept active all the time, >> what in turn prevent the power domain for turning off even if master device >> doesn't do anything and is already suspended. This directly (some clocks >> enabled) and in-directly (leakage current) causes power looses. > Thanks for the confirmation so really the biggest concern here was run time PM. > >>>> This way SYSMMU controllers no >>>> longer prevents respective power domains to be turned off when master's >>>> device is not used. >>> So when the master device is idle we want to also remove power from the >>> controllers ? How much power does this save on a typical device in the >>> market BTW ? >> The main purpose of this patchset is to let power domains to be turned off, >> because with the current code all domains are all the time turned on, >> because >> SYSMMU controllers prevent them from turning them off. > I see.. I think the audio folks already addressed this with DAPM, but granted > this was for audio. Then I was also referred to the DRM / Audio component > framework, has this been looked into? v4l folks have v4l async stuff but > its not clear if that help with run time PM. I'm mentioning these given it'd be > silly to re-invent the wheel, additionally if we now have a generic solution > everyone can jump on board with there is quite a bit of work we can do to > dump a lot of old legacy crap. Right, probably some workarounds here and there can be removed. However components and v4l-async solutions are for resolving only probe and registration issues and they are some kind of pool for grouping devices and triggering special callback once all requested devices in the pool have probed. >> If you want I can measure the power consumption of the idle board with all >> domains enabled and disabled if you want to see the numbers. On the other >> board >> disabling most power domains in idle state (the clocks were already >> disabled) >> gave me about 20mA savings (at 3.7V), what is a significant value for the >> battery powered device. > Thanks, this means nothing to me, however it would be value-add to the commit log > as anyone reviewing this can understand what the goal / savings was for exactly. > >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++------------------------- >>>> 1 file changed, 94 insertions(+), 131 deletions(-) >>> I'm reviewing the device link patches now but since this is a demo of >>> use of that I'll note the changes here are pretty large and it makes >>> it terribly difficult for review. Is there any way this patch can be split >>> up in to logical atomic pieces that only do one task upon change ? >> I will try to split it a bit, but I cannot promise that much can be done >> to improve readability for someone not very familiar with the driver >> internals. > I've heard this before, I don't buy it but lets see! Somehow I managed to split this all-in-one patch into several smaller changes in v5 and v6 was posted yesterday ago. Best regards
On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hi Luis, > > > On 2016-11-08 23:14, Luis R. Rodriguez wrote: >> >> On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote: >>> >>> Hi Luis >>> >>> >>> On 2016-10-06 19:37, Luis R. Rodriguez wrote: >>>> >>>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: >>>>> >>>>> This patch uses recently introduced device links to track the runtime >>>>> pm >>>>> state of the master's device. This way each SYSMMU controller is >>>>> runtime >>>>> activated when its master's device is active >>>> >>>> instead of? >>> >>> instead of keeping SYSMMU controller runtime active all the time. >> >> I thought Rafael's work was for suspend/resume, not for runtime suspend. >> Is it for both ? > > > Yes, it solves both problems, although the suspend/resume was easy to > workaround just by using LATE_SLEEP_OPS. Right, but that's just in this particular case, because the dependency chain is of length 2. :-) If you had a longer chain, you might in theory use the _noirq() stage somehow, but that has limitations. >> Because as far as I can tell this was painted to help >> with suspend/resume ? It helps with three things, (async) suspend/resume, runtime PM and shutdown (that last part is the hardest to figure out). The ordering in which all of these are carried out is analogous and cannot be determined correctly by the device registration ordering itself in general (which has been a known fact for years, but some localized workarounds were put in some places to work around that). Moreover, even if the list ordering (of dpm_list, for instance) is correct, it still doesn't guarantee the right ordering of actions that are carried out asynchronously. They are all started in the list order, but they may be running in parallel with each other and complete at different times. For this reason, there needs to be a way to ensure that, say, the suspend operations for consumer devices complete before their suppliers will become unavailable and so on. Both runtime PM and system suspend/resume have this problem. It is not present in the system shutdown case, but it still helps to get a correct list ordering (ie. such that won't cause supplier devices to be shut down before their consumers) in this case too. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 10, 2016 at 01:36:29AM +0100, Rafael J. Wysocki wrote: > On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > > Hi Luis, > > > > > > On 2016-11-08 23:14, Luis R. Rodriguez wrote: > >> > >> On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote: > >>> > >>> Hi Luis > >>> > >>> > >>> On 2016-10-06 19:37, Luis R. Rodriguez wrote: > >>>> > >>>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: > >>>>> > >>>>> This patch uses recently introduced device links to track the runtime > >>>>> pm > >>>>> state of the master's device. This way each SYSMMU controller is > >>>>> runtime > >>>>> activated when its master's device is active > >>>> > >>>> instead of? > >>> > >>> instead of keeping SYSMMU controller runtime active all the time. > >> > >> I thought Rafael's work was for suspend/resume, not for runtime suspend. > >> Is it for both ? > > > > > > Yes, it solves both problems, although the suspend/resume was easy to > > workaround just by using LATE_SLEEP_OPS. > > Right, but that's just in this particular case, because the dependency > chain is of length 2. :-) > > If you had a longer chain, you might in theory use the _noirq() stage > somehow, but that has limitations. > > >> Because as far as I can tell this was painted to help > >> with suspend/resume ? > > It helps with three things, (async) suspend/resume, runtime PM and > shutdown (that last part is the hardest to figure out). The ordering > in which all of these are carried out is analogous and cannot be > determined correctly by the device registration ordering itself in > general (which has been a known fact for years, but some localized > workarounds were put in some places to work around that). Thanks for the clarification, this is due to the implicit sort you had explained (and I provided notes for on ksummit-discuss) right? Can you itemize a few of the workarounds that are used today? As you clarify below, getting this order of device registration correct does not necessarily guarantee devices will wait for their provider to be ready. > Moreover, even if the list ordering (of dpm_list, for instance) is > correct, it still doesn't guarantee the right ordering of actions that > are carried out asynchronously. They are all started in the list > order, but they may be running in parallel with each other and > complete at different times. For this reason, there needs to be a way > to ensure that, say, the suspend operations for consumer devices > complete before their suppliers will become unavailable and so on. Thanks this helps as well! > Both runtime PM and system suspend/resume have this problem. It is > not present in the system shutdown case, but it still helps to get a > correct list ordering (ie. such that won't cause supplier devices to > be shut down before their consumers) in this case too. Is the fact that its not on shutdown just because we don't care about being sloppy about shutdown ? Shouldn't some devices care about that order ? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index dfb44034b4ee..c8926e030713 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -206,6 +206,7 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = { struct exynos_iommu_owner { struct list_head controllers; /* list of sysmmu_drvdata.owner_node */ struct iommu_domain *domain; /* domain this device is attached */ + struct mutex rpm_lock; /* for runtime pm of all sysmmus */ }; /* @@ -237,8 +238,8 @@ struct sysmmu_drvdata { struct clk *aclk; /* SYSMMU's aclk clock */ struct clk *pclk; /* SYSMMU's pclk clock */ struct clk *clk_master; /* master's device clock */ - int activations; /* number of calls to sysmmu_enable */ spinlock_t lock; /* lock for modyfying state */ + int active; /* current status */ struct exynos_iommu_domain *domain; /* domain we belong to */ struct list_head domain_node; /* node for domain clients list */ struct list_head owner_node; /* node for owner controllers list */ @@ -251,25 +252,6 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom) return container_of(dom, struct exynos_iommu_domain, domain); } -static bool set_sysmmu_active(struct sysmmu_drvdata *data) -{ - /* return true if the System MMU was not active previously - and it needs to be initialized */ - return ++data->activations == 1; -} - -static bool set_sysmmu_inactive(struct sysmmu_drvdata *data) -{ - /* return true if the System MMU is needed to be disabled */ - BUG_ON(data->activations < 1); - return --data->activations == 0; -} - -static bool is_sysmmu_active(struct sysmmu_drvdata *data) -{ - return data->activations > 0; -} - static void sysmmu_unblock(struct sysmmu_drvdata *data) { writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL); @@ -388,7 +370,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id) unsigned short reg_status, reg_clear; int ret = -ENOSYS; - WARN_ON(!is_sysmmu_active(data)); + WARN_ON(!data->active); if (MMU_MAJ_VER(data->version) < 5) { reg_status = REG_INT_STATUS; @@ -434,40 +416,19 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id) return IRQ_HANDLED; } -static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data) +static void __sysmmu_disable(struct sysmmu_drvdata *data) { + unsigned long flags; + clk_enable(data->clk_master); + spin_lock_irqsave(&data->lock, flags); writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL); writel(0, data->sfrbase + REG_MMU_CFG); - - __sysmmu_disable_clocks(data); -} - -static bool __sysmmu_disable(struct sysmmu_drvdata *data) -{ - bool disabled; - unsigned long flags; - - spin_lock_irqsave(&data->lock, flags); - - disabled = set_sysmmu_inactive(data); - - if (disabled) { - data->pgtable = 0; - data->domain = NULL; - - __sysmmu_disable_nocount(data); - - dev_dbg(data->sysmmu, "Disabled\n"); - } else { - dev_dbg(data->sysmmu, "%d times left to disable\n", - data->activations); - } - + data->active = false; spin_unlock_irqrestore(&data->lock, flags); - return disabled; + __sysmmu_disable_clocks(data); } static void __sysmmu_init_config(struct sysmmu_drvdata *data) @@ -484,17 +445,19 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data) writel(cfg, data->sfrbase + REG_MMU_CFG); } -static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data) +static void __sysmmu_enable(struct sysmmu_drvdata *data) { + unsigned long flags; + __sysmmu_enable_clocks(data); + spin_lock_irqsave(&data->lock, flags); writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL); - __sysmmu_init_config(data); - __sysmmu_set_ptbase(data, data->pgtable); - writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL); + data->active = true; + spin_unlock_irqrestore(&data->lock, flags); /* * SYSMMU driver keeps master's clock enabled only for the short @@ -505,48 +468,18 @@ static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data) clk_disable(data->clk_master); } -static int __sysmmu_enable(struct sysmmu_drvdata *data, phys_addr_t pgtable, - struct exynos_iommu_domain *domain) -{ - int ret = 0; - unsigned long flags; - - spin_lock_irqsave(&data->lock, flags); - if (set_sysmmu_active(data)) { - data->pgtable = pgtable; - data->domain = domain; - - __sysmmu_enable_nocount(data); - - dev_dbg(data->sysmmu, "Enabled\n"); - } else { - ret = (pgtable == data->pgtable) ? 1 : -EBUSY; - - dev_dbg(data->sysmmu, "already enabled\n"); - } - - if (WARN_ON(ret < 0)) - set_sysmmu_inactive(data); /* decrement count */ - - spin_unlock_irqrestore(&data->lock, flags); - - return ret; -} - static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data, sysmmu_iova_t iova) { unsigned long flags; - spin_lock_irqsave(&data->lock, flags); - if (is_sysmmu_active(data) && data->version >= MAKE_MMU_VER(3, 3)) { + if (data->active && data->version >= MAKE_MMU_VER(3, 3)) { clk_enable(data->clk_master); __sysmmu_tlb_invalidate_entry(data, iova, 1); clk_disable(data->clk_master); } spin_unlock_irqrestore(&data->lock, flags); - } static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data, @@ -555,7 +488,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data, unsigned long flags; spin_lock_irqsave(&data->lock, flags); - if (is_sysmmu_active(data)) { + if (data->active) { unsigned int num_inv = 1; clk_enable(data->clk_master); @@ -656,40 +589,55 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev) } pm_runtime_enable(dev); - of_iommu_set_ops(dev->of_node, &exynos_iommu_ops); return 0; } -#ifdef CONFIG_PM_SLEEP static int exynos_sysmmu_suspend(struct device *dev) { struct sysmmu_drvdata *data = dev_get_drvdata(dev); + struct exynos_iommu_owner *owner; - dev_dbg(dev, "suspend\n"); - if (is_sysmmu_active(data)) { - __sysmmu_disable_nocount(data); - pm_runtime_put(dev); + if (!data->master) + return 0; + + owner = data->master->archdata.iommu; + + mutex_lock(&owner->rpm_lock); + if (data->domain) { + dev_dbg(data->sysmmu, "saving state\n"); + __sysmmu_disable(data); } + mutex_unlock(&owner->rpm_lock); + return 0; } static int exynos_sysmmu_resume(struct device *dev) { struct sysmmu_drvdata *data = dev_get_drvdata(dev); + struct exynos_iommu_owner *owner; - dev_dbg(dev, "resume\n"); - if (is_sysmmu_active(data)) { - pm_runtime_get_sync(dev); - __sysmmu_enable_nocount(data); + if (!data->master) + return 0; + + owner = data->master->archdata.iommu; + + mutex_lock(&owner->rpm_lock); + if (data->domain) { + dev_dbg(data->sysmmu, "restoring state\n"); + __sysmmu_enable(data); } + mutex_unlock(&owner->rpm_lock); + return 0; } -#endif static const struct dev_pm_ops sysmmu_pm_ops = { - SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume) + SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL) + SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) }; static const struct of_device_id sysmmu_of_match[] __initconst = { @@ -793,9 +741,12 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain) spin_lock_irqsave(&domain->lock, flags); list_for_each_entry_safe(data, next, &domain->clients, domain_node) { - if (__sysmmu_disable(data)) - data->master = NULL; + spin_lock(&data->lock); + __sysmmu_disable(data); + data->pgtable = 0; + data->domain = NULL; list_del_init(&data->domain_node); + spin_unlock(&data->lock); } spin_unlock_irqrestore(&domain->lock, flags); @@ -829,31 +780,32 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain, phys_addr_t pagetable = virt_to_phys(domain->pgtable); struct sysmmu_drvdata *data, *next; unsigned long flags; - bool found = false; if (!has_sysmmu(dev) || owner->domain != iommu_domain) return; + mutex_lock(&owner->rpm_lock); + + list_for_each_entry(data, &owner->controllers, owner_node) { + if (pm_runtime_active(data->sysmmu)) + __sysmmu_disable(data); + } + spin_lock_irqsave(&domain->lock, flags); list_for_each_entry_safe(data, next, &domain->clients, domain_node) { - if (data->master == dev) { - if (__sysmmu_disable(data)) { - data->master = NULL; - list_del_init(&data->domain_node); - } - pm_runtime_put(data->sysmmu); - found = true; - } + spin_lock(&data->lock); + data->pgtable = 0; + data->domain = NULL; + list_del_init(&data->domain_node); + spin_unlock(&data->lock); } + owner->domain = NULL; spin_unlock_irqrestore(&domain->lock, flags); - owner->domain = NULL; + mutex_unlock(&owner->rpm_lock); - if (found) - dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", - __func__, &pagetable); - else - dev_err(dev, "%s: No IOMMU is attached\n", __func__); + dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__, + &pagetable); } static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, @@ -864,7 +816,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, struct sysmmu_drvdata *data; phys_addr_t pagetable = virt_to_phys(domain->pgtable); unsigned long flags; - int ret = -ENODEV; if (!has_sysmmu(dev)) return -ENODEV; @@ -872,29 +823,30 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, if (owner->domain) exynos_iommu_detach_device(owner->domain, dev); + mutex_lock(&owner->rpm_lock); + + spin_lock_irqsave(&domain->lock, flags); list_for_each_entry(data, &owner->controllers, owner_node) { - pm_runtime_get_sync(data->sysmmu); - ret = __sysmmu_enable(data, pagetable, domain); - if (ret >= 0) { - data->master = dev; - - spin_lock_irqsave(&domain->lock, flags); - list_add_tail(&data->domain_node, &domain->clients); - spin_unlock_irqrestore(&domain->lock, flags); - } + spin_lock(&data->lock); + data->pgtable = pagetable; + data->domain = domain; + list_add_tail(&data->domain_node, &domain->clients); + spin_unlock(&data->lock); } + owner->domain = iommu_domain; + spin_unlock_irqrestore(&domain->lock, flags); - if (ret < 0) { - dev_err(dev, "%s: Failed to attach IOMMU with pgtable %pa\n", - __func__, &pagetable); - return ret; + list_for_each_entry(data, &owner->controllers, owner_node) { + if (pm_runtime_active(data->sysmmu)) + __sysmmu_enable(data); } - owner->domain = iommu_domain; - dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa %s\n", - __func__, &pagetable, (ret == 0) ? "" : ", again"); + mutex_unlock(&owner->rpm_lock); - return ret; + dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, + &pagetable); + + return 0; } static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain, @@ -1265,10 +1217,21 @@ static int exynos_iommu_of_xlate(struct device *dev, return -ENOMEM; INIT_LIST_HEAD(&owner->controllers); + mutex_init(&owner->rpm_lock); dev->archdata.iommu = owner; } list_add_tail(&data->owner_node, &owner->controllers); + data->master = dev; + + /* + * SYSMMU will be runtime enabled via device link (dependency) to its + * master device, so there are no direct calls to pm_runtime_get/put + * in this driver. + */ + device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE, + DEVICE_LINK_PM_RUNTIME); + return 0; }
This patch uses recently introduced device links to track the runtime pm state of the master's device. This way each SYSMMU controller is runtime activated when its master's device is active and can save/restore its state instead of being enabled all the time. This way SYSMMU controllers no longer prevents respective power domains to be turned off when master's device is not used. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++------------------------- 1 file changed, 94 insertions(+), 131 deletions(-)