Re: [PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm
Hi Ohad, On 14 November 2012 03:54, Ohad Ben-Cohen o...@wizery.com wrote: Hi Omar, On Wed, Nov 14, 2012 at 4:34 AM, Omar Ramirez Luna omar.l...@linaro.org wrote: Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations and sysconfig handling. Dues to reset sequence, pm_runtime_put_sync must be used, to avoid possible operations with the module under reset. There are some changes here that might not be trivial to understand in hindsight; any chance you can add more explanations (even only in the commit log) regarding: I have discussed exactly the same changes in the list with Felipe, but yes I did forget to add the explanations (I thought I did in some version of the patch or cover-letter), but will update this description. Below is the discussion just in case, I'll be replying to your comments anyway ;) https://patchwork.kernel.org/patch/1585741/ @@ -160,11 +160,10 @@ static int iommu_enable(struct omap_iommu *obj) ... - clk_enable(obj-clk); + pm_runtime_get_sync(obj-dev); err = arch_iommu-enable(obj); - clk_disable(obj-clk); return err; } Why do we remove clk_disable here (instead of replacing it with a _put variant) ? Basically, with the previous clk management, the iommu driver assumes that its clock is shared with its client, which is the case for ipu and dsp, but I don't like that assumption. So by doing clock_enable/disable, the functional clock required for translations it is indirectly provided by the user of the iommu (let's say ipu). E.g. IPU enables the iommu and maps, at the end of the maps the clock will be disabled, but given that ipu clock is the same the mmu stays functional. By changing this to get_sync only, the mmu stays enabled as long as the iommu has been requested (except for the power transitions). @@ -306,7 +303,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e) if (!obj || !obj-nr_tlb_entries || !e) return -EINVAL; - clk_enable(obj-clk); + pm_runtime_get_sync(obj-dev); If iommu_enable no longer disables obj-clk before returning, do we really need to call -get here (and in all the other similar instances) ? You can access this paths through debugfs, some of them doesn't work if the module is not enabled first, but in future if you just want to idle the iommu without freeing, these are needed to debug. @@ -816,9 +813,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) if (!obj-refcount) return IRQ_NONE; - clk_enable(obj-clk); errs = iommu_report_fault(obj, da); - clk_disable(obj-clk); Why do we remove the clk_ invocations here (instead of replacing them with get/put variants) ? Because in order to get an interrupt from the mmu device it implies that the mmu was functional already (with a clock), so I don't see how clk_enable/disable are needed here. Even if you rely on the IPU to maintain the clock enabled. Most of the above questions imply this patch not only converts the iommu to runtime PM, but may carry additional changes that may imply previous implementation is sub-optimal. I hope we can clearly document the motivation behind these changes too (maybe even consider extracting them to a different patch ?). I didn't want to extract this changes into different patches since they could be included in this one, otherwise it would look like lines adding and then deleting runtime pm functions. I do agree description is missing, again I thought I had done this somewhere but looks like I didn't, will update these. If you think these should be different patches please let me know, otherwise I would like to keep a single *documented* patch. @@ -990,6 +981,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev) goto err_irq; platform_set_drvdata(pdev, obj); + pm_runtime_irq_safe(obj-dev); Let's also document why _irq_safe is needed here ? Ok. Thanks for the comments, Omar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm
Hi Omar, On Thu, Nov 15, 2012 at 6:53 PM, Omar Ramirez Luna omar.l...@linaro.org wrote: On 14 November 2012 03:54, Ohad Ben-Cohen o...@wizery.com wrote: Most of the above questions imply this patch not only converts the iommu to runtime PM, but may carry additional changes that may imply previous implementation is sub-optimal. I hope we can clearly document the motivation behind these changes too (maybe even consider extracting them to a different patch ?). I didn't want to extract this changes into different patches since they could be included in this one, otherwise it would look like lines adding and then deleting runtime pm functions. I do agree description is missing, again I thought I had done this somewhere but looks like I didn't, will update these. If you think these should be different patches please let me know, otherwise I would like to keep a single *documented* patch. It seems like there are 3 different logical changes here: 1. remove clk_* invocations from iommu_fault_handler() 2. keep clocks enabled as long as iommu is enabled 3. convert to runtime pm If we split this to three patches in this order, you won't have to add and remove runtime pm functions. Let's do it, please. It's a small nuisance now, but may be really helpful in the future when someone tries to debug stuff and understand the motivation behind these changes. It'd make it much easier for you to document the changes, too: you have an entire commit log per change. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm
On 15 November 2012 11:39, Ohad Ben-Cohen o...@wizery.com wrote: I do agree description is missing, again I thought I had done this somewhere but looks like I didn't, will update these. If you think these should be different patches please let me know, otherwise I would like to keep a single *documented* patch. It seems like there are 3 different logical changes here: 1. remove clk_* invocations from iommu_fault_handler() 2. keep clocks enabled as long as iommu is enabled 3. convert to runtime pm If we split this to three patches in this order, you won't have to add and remove runtime pm functions. Let's do it, please. It's a small nuisance now, but may be really helpful in the future when someone tries to debug stuff and understand the motivation behind these changes. It'd make it much easier for you to document the changes, too: you have an entire commit log per change. Ok, not a problem then. Cheers, Omar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm
Hi Omar, On Wed, Nov 14, 2012 at 4:34 AM, Omar Ramirez Luna omar.l...@linaro.org wrote: Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations and sysconfig handling. Dues to reset sequence, pm_runtime_put_sync must be used, to avoid possible operations with the module under reset. There are some changes here that might not be trivial to understand in hindsight; any chance you can add more explanations (even only in the commit log) regarding: @@ -160,11 +160,10 @@ static int iommu_enable(struct omap_iommu *obj) ... - clk_enable(obj-clk); + pm_runtime_get_sync(obj-dev); err = arch_iommu-enable(obj); - clk_disable(obj-clk); return err; } Why do we remove clk_disable here (instead of replacing it with a _put variant) ? @@ -306,7 +303,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e) if (!obj || !obj-nr_tlb_entries || !e) return -EINVAL; - clk_enable(obj-clk); + pm_runtime_get_sync(obj-dev); If iommu_enable no longer disables obj-clk before returning, do we really need to call -get here (and in all the other similar instances) ? @@ -816,9 +813,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) if (!obj-refcount) return IRQ_NONE; - clk_enable(obj-clk); errs = iommu_report_fault(obj, da); - clk_disable(obj-clk); Why do we remove the clk_ invocations here (instead of replacing them with get/put variants) ? Most of the above questions imply this patch not only converts the iommu to runtime PM, but may carry additional changes that may imply previous implementation is sub-optimal. I hope we can clearly document the motivation behind these changes too (maybe even consider extracting them to a different patch ?). @@ -990,6 +981,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev) goto err_irq; platform_set_drvdata(pdev, obj); + pm_runtime_irq_safe(obj-dev); Let's also document why _irq_safe is needed here ? Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm
Use runtime PM functionality interfaced with hwmod enable/idle
functions, to replace direct clock operations and sysconfig
handling.
Dues to reset sequence, pm_runtime_put_sync must be used, to avoid
possible operations with the module under reset.
Signed-off-by: Omar Ramirez Luna omar.l...@linaro.org
---
arch/arm/mach-omap2/omap-iommu.c |1 -
drivers/iommu/omap-iommu.c | 45 -
drivers/iommu/omap-iommu.h |3 --
drivers/iommu/omap-iommu2.c | 17 ---
include/linux/platform_data/iommu-omap.h |1 -
5 files changed, 19 insertions(+), 48 deletions(-)
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 02726a6..7642fc4 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -31,7 +31,6 @@ static int __init omap_iommu_dev_init(struct omap_hwmod *oh,
void *unused)
return -ENOMEM;
pdata-name = oh-name;
- pdata-clk_name = oh-main_clk;
pdata-nr_tlb_entries = a-nr_tlb_entries;
pdata-da_start = a-da_start;
pdata-da_end = a-da_end;
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 0a6a901..b42b3d1 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -16,13 +16,13 @@
#include linux/slab.h
#include linux/interrupt.h
#include linux/ioport.h
-#include linux/clk.h
#include linux/platform_device.h
#include linux/iommu.h
#include linux/omap-iommu.h
#include linux/mutex.h
#include linux/spinlock.h
#include linux/io.h
+#include linux/pm_runtime.h
#include asm/cacheflush.h
@@ -160,11 +160,10 @@ static int iommu_enable(struct omap_iommu *obj)
}
}
- clk_enable(obj-clk);
+ pm_runtime_get_sync(obj-dev);
err = arch_iommu-enable(obj);
- clk_disable(obj-clk);
return err;
}
@@ -176,11 +175,9 @@ static void iommu_disable(struct omap_iommu *obj)
if (!obj || !pdata)
return;
- clk_enable(obj-clk);
-
arch_iommu-disable(obj);
- clk_disable(obj-clk);
+ pm_runtime_put_sync(obj-dev);
if (pdata-assert_reset)
pdata-assert_reset(pdev, pdata-reset_name);
@@ -306,7 +303,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct
iotlb_entry *e)
if (!obj || !obj-nr_tlb_entries || !e)
return -EINVAL;
- clk_enable(obj-clk);
+ pm_runtime_get_sync(obj-dev);
iotlb_lock_get(obj, l);
if (l.base == obj-nr_tlb_entries) {
@@ -336,7 +333,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct
iotlb_entry *e)
cr = iotlb_alloc_cr(obj, e);
if (IS_ERR(cr)) {
- clk_disable(obj-clk);
+ pm_runtime_put_sync(obj-dev);
return PTR_ERR(cr);
}
@@ -350,7 +347,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct
iotlb_entry *e)
l.vict = l.base;
iotlb_lock_set(obj, l);
out:
- clk_disable(obj-clk);
+ pm_runtime_put_sync(obj-dev);
return err;
}
@@ -380,7 +377,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
int i;
struct cr_regs cr;
- clk_enable(obj-clk);
+ pm_runtime_get_sync(obj-dev);
for_each_iotlb_cr(obj, obj-nr_tlb_entries, i, cr) {
u32 start;
@@ -399,7 +396,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
}
}
- clk_disable(obj-clk);
+ pm_runtime_put_sync(obj-dev);
if (i == obj-nr_tlb_entries)
dev_dbg(obj-dev, %s: no page for %08x\n, __func__, da);
@@ -413,7 +410,7 @@ static void flush_iotlb_all(struct omap_iommu *obj)
{
struct iotlb_lock l;
- clk_enable(obj-clk);
+ pm_runtime_get_sync(obj-dev);
l.base = 0;
l.vict = 0;
@@ -421,7 +418,7 @@ static void flush_iotlb_all(struct omap_iommu *obj)
iommu_write_reg(obj, 1, MMU_GFLUSH);
- clk_disable(obj-clk);
+ pm_runtime_put_sync(obj-dev);
}
#if defined(CONFIG_OMAP_IOMMU_DEBUG) || defined(CONFIG_OMAP_IOMMU_DEBUG_MODULE)
@@ -431,11 +428,11 @@ ssize_t omap_iommu_dump_ctx(struct omap_iommu *obj, char
*buf, ssize_t bytes)
if (!obj || !buf)
return -EINVAL;
- clk_enable(obj-clk);
+ pm_runtime_get_sync(obj-dev);
bytes = arch_iommu-dump_ctx(obj, buf, bytes);
- clk_disable(obj-clk);
+ pm_runtime_put_sync(obj-dev);
return bytes;
}
@@ -449,7 +446,7 @@ __dump_tlb_entries(struct omap_iommu *obj, struct cr_regs
*crs, int num)
struct cr_regs tmp;
struct cr_regs *p = crs;
- clk_enable(obj-clk);
+ pm_runtime_get_sync(obj-dev);
iotlb_lock_get(obj, saved);
for_each_iotlb_cr(obj, num, i, tmp) {
@@ -459,7 +456,7 @@
