| Message ID | 1473770941-8337-3-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 13 September 2016 at 14:49, Marek Szyprowski <m.szyprowski@samsung.com> 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 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. Apologize for not reviewing earlier and if you find my questions/suggestions being silly. You may ignore them, if you don't think they deserves a proper answer. :-) I am not so familiar with the IOMMU subsystem, but I am wondering whether the issue you are solving, is similar to what can be observed for DMA and serial drivers. And of course also for other IOMMU drivers. In general the DMA/serial drivers requires to use the pm_runtime_irq_safe() option, to be able to easily deploy runtime PM support (of course there are some other workarounds as well). As we know, using the pm_runtime_irq_safe() option comes with some limitations, such as the runtime PM callbacks is not allowed to sleep. For a PM domain (genpd) that is attached to the device, this also means it must not be powered off. To solve this problem, I was thinking we could convert to use the asynchronous pm_runtime_get() API, when trying to runtime resume the device from atomic contexts. Of course when it turns out that the device isn't yet runtime resumed immediately after calling pm_runtime_get(), the request needs to be put on a request queue to be managed shortly after instead. Doing it like this, would remove the need to use the pm_runtime_irq_safe() option. I realize that such change needs to be implemented in common code for IOMMU drivers, if at all possible. Anyway, I hope you at least get the idea and I just wanted to mention that I have been exploring this option for DMA and serial drivers. Kind regards Uffe > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++------------------------- > 1 file changed, 94 insertions(+), 131 deletions(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index b0fa4d432e71..34717a0b1902 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); > @@ -389,7 +371,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; > @@ -435,40 +417,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) > @@ -485,17 +446,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 > @@ -506,48 +469,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, > @@ -556,7 +489,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); > @@ -657,40 +590,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 = { > @@ -794,9 +742,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); > @@ -830,31 +781,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, > @@ -865,7 +817,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; > @@ -873,29 +824,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, > @@ -1266,10 +1218,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_PERSISTENT | DEVICE_LINK_PM_RUNTIME); > + > return 0; > } > > -- > 1.9.1 > -- 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 Marek,
[auto build test ERROR on iommu/next]
[also build test ERROR on v4.8-rc6 next-20160913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/Marek-Szyprowski/Exynos-IOMMU-proper-runtime-PM-support-use-device-dependencies/20160913-205434
base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/iommu/exynos-iommu.c: In function 'exynos_iommu_of_xlate':
>> drivers/iommu/exynos-iommu.c:1232:2: error: implicit declaration of function 'device_link_add' [-Werror=implicit-function-declaration]
device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
^
>> drivers/iommu/exynos-iommu.c:1232:37: error: 'DEVICE_LINK_AVAILABLE' undeclared (first use in this function)
device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
^
drivers/iommu/exynos-iommu.c:1232:37: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/iommu/exynos-iommu.c:1233:4: error: 'DEVICE_LINK_PERSISTENT' undeclared (first use in this function)
DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
^
>> drivers/iommu/exynos-iommu.c:1233:29: error: 'DEVICE_LINK_PM_RUNTIME' undeclared (first use in this function)
DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
^
cc1: some warnings being treated as errors
vim +/device_link_add +1232 drivers/iommu/exynos-iommu.c
1226
1227 /*
1228 * SYSMMU will be runtime enabled via device link (dependency) to its
1229 * master device, so there are no direct calls to pm_runtime_get/put
1230 * in this driver.
1231 */
> 1232 device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
> 1233 DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
1234
1235 return 0;
1236 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Ulf, On 2016-09-13 16:20, Ulf Hansson wrote: > On 13 September 2016 at 14:49, Marek Szyprowski > <m.szyprowski@samsung.com> 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 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. > Apologize for not reviewing earlier and if you find my > questions/suggestions being silly. You may ignore them, if you don't > think they deserves a proper answer. :-) No problem. There are no silly questions, but there might be some silly answers ;) > I am not so familiar with the IOMMU subsystem, but I am wondering > whether the issue you are solving, is similar to what can be observed > for DMA and serial drivers. And of course also for other IOMMU > drivers. > > In general the DMA/serial drivers requires to use the > pm_runtime_irq_safe() option, to be able to easily deploy runtime PM > support (of course there are some other workarounds as well). There are some similarities between IOMMU and DMA engine devices (serial drivers are imho completely different case). Both hw blocks do their work on behalf of some other hardware block, which I will call master device. DMA engine performs some DMA transaction on master's device request, while IOMMU usually sits between system memory and master's device memory interface, remapping addresses of each DMA transaction according to its configuration and provided mapping tables (master device has some kind of internal DMA controller and performs DMA transactions on their own). IOMMU is usually used for a) mapping physically discontinuous memory into contiguous DMA addresses and b) isolating devices, so they can access only memory, which is dedicated or allocated for them. DMA engine devices provide explicit API for their master's device drivers, while IOMMU drivers are usually hidden behind DMA-mapping API (for most use cases, although it would be possible for master's device driver to call IOMMU API directly and some GPU/DRM drivers do that). However from runtime pm perspective the DMA engine and IOMMU devices are a bit different. DMA engine drivers have well defined start and end of operation (queuing dma request and irq from hw about having it finished). During that time the device has to be runtime active all the time. The problem with using current implementation of runtime pm is the fact that both start and end of operation can be triggered from atomic context, what is not really suitable for runtime pm. So the problem is mainly about API incompatibility and lack of something like dma_engine_prepare()/unprepare() (as an analogy to clocks api). In case of IOMMU the main problem is determining weather IOMMU controller has to be activated. There is no calls in IOMMU and DMA-mapping API, which would bracket all DMA transactions performed by the master device. Someone proposed to keep IOMMU runtime active when there exist at least one mapping created by the IOMMU/DMA-mapping layers. This however does not cover all the cases. In case of our IOMMU, when it is disabled or runtime suspended, it enters "pass-thought" mode, so master device can still perform DMA operations with identity mappings (so DMA address equals to physical memory address). Till now Exynos IOMMU called pm_runtime_get() on attaching to the iommu domain (what happens during initialization of dma-mapping structures for given master device) and kept it active all the time. This patch series tries to address Exynos IOMMU runtime pm issue by forcing IOMMU controller to follow runtime pm status of its master device. This way we ensure that anytime when master's device is runtime activated, the iommu will be also active and master device won't be able to bypass during its DMA transactions mappings created by the IOMMU layer. Quite long answer, but I hope I managed to give you a bit more background on this topic. > As we know, using the pm_runtime_irq_safe() option comes with some > limitations, such as the runtime PM callbacks is not allowed to sleep. > For a PM domain (genpd) that is attached to the device, this also > means it must not be powered off. Right, if possible I would like to avoid using pm_runtime_irq_safe() option, because it is really impractical. > To solve this problem, I was thinking we could convert to use the > asynchronous pm_runtime_get() API, when trying to runtime resume the > device from atomic contexts. I'm not sure if this will work for DMA engine devices. If I understand correctly some client's of DMA engine device might rely on the DMA engine being configured and operational after queuing a request and they might lock up if the DMA engine device activation if postponed because of async runtime pm activation. > Of course when it turns out that the device isn't yet runtime resumed > immediately after calling pm_runtime_get(), the request needs to be > put on a request queue to be managed shortly after instead. Doing it > like this, would remove the need to use the pm_runtime_irq_safe() > option. > > I realize that such change needs to be implemented in common code for > IOMMU drivers, if at all possible. > > Anyway, I hope you at least get the idea and I just wanted to mention > that I have been exploring this option for DMA and serial drivers. I also have runtime pm for serial driver on my todo list, but it doesn't have high priority. The other runtime pm integration subsystem that I want to work on first is pinctrl. It is needed to fully support Exynos 5433 SoCs, because registers of some audio related pins are in the audio power domain, what now prevent us from enabling support for audio power domain. Best regards
[...] > > > There are some similarities between IOMMU and DMA engine devices (serial > drivers are imho completely different case). Both hw blocks do their work > on behalf of some other hardware block, which I will call master device. > DMA engine performs some DMA transaction on master's device request, while > IOMMU usually sits between system memory and master's device memory > interface, remapping addresses of each DMA transaction according to its > configuration and provided mapping tables (master device has some kind > of internal DMA controller and performs DMA transactions on their own). > IOMMU is usually used for a) mapping physically discontinuous memory into > contiguous DMA addresses and b) isolating devices, so they can access > only memory, which is dedicated or allocated for them. > > DMA engine devices provide explicit API for their master's device drivers, > while IOMMU drivers are usually hidden behind DMA-mapping API (for most > use cases, although it would be possible for master's device driver to > call IOMMU API directly and some GPU/DRM drivers do that). > > However from runtime pm perspective the DMA engine and IOMMU devices are > a bit different. > > DMA engine drivers have well defined start and end of operation (queuing > dma request and irq from hw about having it finished). During that time > the device has to be runtime active all the time. The problem with using > current implementation of runtime pm is the fact that both start and end > of operation can be triggered from atomic context, what is not really > suitable for runtime pm. So the problem is mainly about API > incompatibility and lack of something like dma_engine_prepare()/unprepare() > (as an analogy to clocks api). That's also a viable option. Although, DMA clients would then need to invoke such APIs from non-atomic contexts. Typically that would be from client driver's runtime PM callbacks. Me personally would rather avoid such solution, as it would sprinkle lots of drivers to deal with this. Although, perhaps this is the only option that actually works. > > In case of IOMMU the main problem is determining weather IOMMU controller > has to be activated. There is no calls in IOMMU and DMA-mapping API, which > would bracket all DMA transactions performed by the master device. Someone > proposed to keep IOMMU runtime active when there exist at least one > mapping created by the IOMMU/DMA-mapping layers. This however does not > cover all the cases. In case of our IOMMU, when it is disabled or runtime > suspended, it enters "pass-thought" mode, so master device can still > perform DMA operations with identity mappings (so DMA address equals to > physical memory address). Till now Exynos IOMMU called pm_runtime_get() > on attaching to the iommu domain (what happens during initialization of > dma-mapping structures for given master device) and kept it active all > the time. > > This patch series tries to address Exynos IOMMU runtime pm issue by forcing > IOMMU controller to follow runtime pm status of its master device. This way > we ensure that anytime when master's device is runtime activated, the iommu > will be also active and master device won't be able to bypass during its > DMA transactions mappings created by the IOMMU layer. > > Quite long answer, but I hope I managed to give you a bit more background > on this topic. Yes, indeed. Thank you for taking the time to respond! > >> As we know, using the pm_runtime_irq_safe() option comes with some >> limitations, such as the runtime PM callbacks is not allowed to sleep. >> For a PM domain (genpd) that is attached to the device, this also >> means it must not be powered off. > > > Right, if possible I would like to avoid using pm_runtime_irq_safe() > option, because it is really impractical. > >> To solve this problem, I was thinking we could convert to use the >> asynchronous pm_runtime_get() API, when trying to runtime resume the >> device from atomic contexts. > > > I'm not sure if this will work for DMA engine devices. If I understand > correctly some client's of DMA engine device might rely on the DMA > engine being configured and operational after queuing a request and > they might lock up if the DMA engine device activation if postponed > because of async runtime pm activation. I didn't know about this. If you have an example from the top of your head, could you perhaps point me to it? [...] Kind regards Uffe -- 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 Ulf, On 2016-09-14 12:28, Ulf Hansson wrote: > [...] > >> There are some similarities between IOMMU and DMA engine devices (serial >> drivers are imho completely different case). Both hw blocks do their work >> on behalf of some other hardware block, which I will call master device. >> DMA engine performs some DMA transaction on master's device request, while >> IOMMU usually sits between system memory and master's device memory >> interface, remapping addresses of each DMA transaction according to its >> configuration and provided mapping tables (master device has some kind >> of internal DMA controller and performs DMA transactions on their own). >> IOMMU is usually used for a) mapping physically discontinuous memory into >> contiguous DMA addresses and b) isolating devices, so they can access >> only memory, which is dedicated or allocated for them. >> >> DMA engine devices provide explicit API for their master's device drivers, >> while IOMMU drivers are usually hidden behind DMA-mapping API (for most >> use cases, although it would be possible for master's device driver to >> call IOMMU API directly and some GPU/DRM drivers do that). >> >> However from runtime pm perspective the DMA engine and IOMMU devices are >> a bit different. >> >> DMA engine drivers have well defined start and end of operation (queuing >> dma request and irq from hw about having it finished). During that time >> the device has to be runtime active all the time. The problem with using >> current implementation of runtime pm is the fact that both start and end >> of operation can be triggered from atomic context, what is not really >> suitable for runtime pm. So the problem is mainly about API >> incompatibility and lack of something like dma_engine_prepare()/unprepare() >> (as an analogy to clocks api). > That's also a viable option. Although, DMA clients would then need to > invoke such APIs from non-atomic contexts. Typically that would be > from client driver's runtime PM callbacks. > > Me personally would rather avoid such solution, as it would sprinkle > lots of drivers to deal with this. Although, perhaps this is the only > option that actually works. One might also introduce optional functions and notify DMA engine core with some flag that the client driver wants to use them or not. If not core will prepare dma engine on initialization. This is not really nice from the API clearness perspective, but it would allow to have some time for transition to the new approach till all clients gets updated. >> In case of IOMMU the main problem is determining weather IOMMU controller >> has to be activated. There is no calls in IOMMU and DMA-mapping API, which >> would bracket all DMA transactions performed by the master device. Someone >> proposed to keep IOMMU runtime active when there exist at least one >> mapping created by the IOMMU/DMA-mapping layers. This however does not >> cover all the cases. In case of our IOMMU, when it is disabled or runtime >> suspended, it enters "pass-thought" mode, so master device can still >> perform DMA operations with identity mappings (so DMA address equals to >> physical memory address). Till now Exynos IOMMU called pm_runtime_get() >> on attaching to the iommu domain (what happens during initialization of >> dma-mapping structures for given master device) and kept it active all >> the time. >> >> This patch series tries to address Exynos IOMMU runtime pm issue by forcing >> IOMMU controller to follow runtime pm status of its master device. This way >> we ensure that anytime when master's device is runtime activated, the iommu >> will be also active and master device won't be able to bypass during its >> DMA transactions mappings created by the IOMMU layer. >> >> Quite long answer, but I hope I managed to give you a bit more background >> on this topic. > Yes, indeed. Thank you for taking the time to respond! > >>> As we know, using the pm_runtime_irq_safe() option comes with some >>> limitations, such as the runtime PM callbacks is not allowed to sleep. >>> For a PM domain (genpd) that is attached to the device, this also >>> means it must not be powered off. >> >> Right, if possible I would like to avoid using pm_runtime_irq_safe() >> option, because it is really impractical. >> >>> To solve this problem, I was thinking we could convert to use the >>> asynchronous pm_runtime_get() API, when trying to runtime resume the >>> device from atomic contexts. >> I'm not sure if this will work for DMA engine devices. If I understand >> correctly some client's of DMA engine device might rely on the DMA >> engine being configured and operational after queuing a request and >> they might lock up if the DMA engine device activation if postponed >> because of async runtime pm activation. > I didn't know about this. If you have an example from the top of your > head, could you perhaps point me to it? No, I don't have any example. This is just my fear that there might be some hardware which works this way. I can imagine a driver, which queue dma engine request and then triggers 'start' command to its hw block. HW blocks usually uses some additional signal lines for DMA, so the HW block might check if all needed signals from DMA engine HW are ready. If not it will fail to start avoid lockup of starting without DMA engine HW being ready. However I don't have much experience with DMA engine and drivers. I only helped in adding DMA engine support to Samsung UART driver and in the hardware manual there is information about additional lines between DMA controller and UART module, which are used in the DMA mode. Best regards
[...] >>> >>>> To solve this problem, I was thinking we could convert to use the >>>> asynchronous pm_runtime_get() API, when trying to runtime resume the >>>> device from atomic contexts. >>> >>> I'm not sure if this will work for DMA engine devices. If I understand >>> correctly some client's of DMA engine device might rely on the DMA >>> engine being configured and operational after queuing a request and >>> they might lock up if the DMA engine device activation if postponed >>> because of async runtime pm activation. >> >> I didn't know about this. If you have an example from the top of your >> head, could you perhaps point me to it? > > > No, I don't have any example. This is just my fear that there might be some > hardware which works this way. I can imagine a driver, which queue dma > engine > request and then triggers 'start' command to its hw block. HW blocks usually > uses some additional signal lines for DMA, so the HW block might check if > all > needed signals from DMA engine HW are ready. If not it will fail to start > avoid lockup of starting without DMA engine HW being ready. Well, I think this reasoning makes lots of sense! Clearly we wouldn't be able to guarantee that there's isn't a glitch in an undefined HW behaviour. I will drop my suggested approach and try out another one. > > However I don't have much experience with DMA engine and drivers. I only > helped in adding DMA engine support to Samsung UART driver and in the > hardware manual there is information about additional lines between DMA > controller and UART module, which are used in the DMA mode. Thanks for sharing your experience! Now, I should allow you to get back to the original review! :-) Kind regards Uffe -- 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 b0fa4d432e71..34717a0b1902 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); @@ -389,7 +371,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; @@ -435,40 +417,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) @@ -485,17 +446,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 @@ -506,48 +469,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, @@ -556,7 +489,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); @@ -657,40 +590,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 = { @@ -794,9 +742,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); @@ -830,31 +781,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, @@ -865,7 +817,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; @@ -873,29 +824,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, @@ -1266,10 +1218,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_PERSISTENT | 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(-)