Re: [PATCH v3 4/4] OMAP3/4: iommu: adapt to runtime pm
Hi, On Fri, Nov 4, 2011 at 6:27 PM, Kevin Hilman khil...@ti.com wrote: @@ -821,9 +820,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); if (errs == 0) return IRQ_HANDLED; I'm not terribly familiar with this IOMMU code, but this one looks suspiciou because you're removing the clock calls but not replacing them with runtime PM get/put calls. I just want to make sure that's intentional. If so, you might want to add a comment about that to the changelog. Yes it is intentional, reason is that in order to get an interrupt, the device should be powered on in advance, right now it is working because the modules share a common clock so the users of the omap-iommu indirectly give power to it. However I made another change to do pm_runtime_get/put on attach/detach so it doesn't rely on others to keep the clocks on. I'll add the comment. Thanks, 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 v3 4/4] OMAP3/4: iommu: adapt to runtime pm
Omar Ramirez Luna omar.rami...@ti.com writes: Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling. Tidspbridge uses a macro removed with this patch, for now the value is hardcoded to avoid breaking compilation. Signed-off-by: Omar Ramirez Luna omar.rami...@ti.com Looks like a good cleanup. I agree with the comments from Myungjoo, and have a question below.. [...] @@ -821,9 +820,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); if (errs == 0) return IRQ_HANDLED; I'm not terribly familiar with this IOMMU code, but this one looks suspiciou because you're removing the clock calls but not replacing them with runtime PM get/put calls. I just want to make sure that's intentional. If so, you might want to add a comment about that to the changelog. Kevin -- 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 v3 4/4] OMAP3/4: iommu: adapt to runtime pm
On Wed, Nov 2, 2011 at 7:15 AM, Omar Ramirez Luna omar.rami...@ti.com wrote: Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling. Tidspbridge uses a macro removed with this patch, for now the value is hardcoded to avoid breaking compilation. Signed-off-by: Omar Ramirez Luna omar.rami...@ti.com --- arch/arm/mach-omap2/iommu2.c | 17 arch/arm/mach-omap2/omap-iommu.c | 1 - arch/arm/plat-omap/include/plat/iommu.h | 2 - arch/arm/plat-omap/include/plat/iommu2.h | 2 - drivers/iommu/omap-iommu.c | 46 - drivers/staging/tidspbridge/core/tiomap3430_pwr.c | 2 +- 6 files changed, 19 insertions(+), 51 deletions(-) diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index bbbf747..3c55be0 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -123,11 +123,11 @@ static int iommu_enable(struct omap_iommu *obj) if (!arch_iommu) return -ENODEV; - clk_enable(obj-clk); + pm_runtime_enable(obj-dev); + pm_runtime_get_sync(obj-dev); err = arch_iommu-enable(obj); - clk_disable(obj-clk); return err; } @@ -136,11 +136,10 @@ static void iommu_disable(struct omap_iommu *obj) if (!obj) return; - clk_enable(obj-clk); - arch_iommu-disable(obj); - clk_disable(obj-clk); + pm_runtime_put_sync(obj-dev); + pm_runtime_disable(obj-dev); } Hello Omar, I'm just curious here... Is there any reason to do pm_runtime_enable/disable at iommu_enable/iommu_disable which are called by iommu_attach/detach? I thought that normally, ideal locations of pm_runtime_enable/disable for such devices are in probe/remove() because it assures that the device is suspended after the probe. It seems that the device might be kept on after probe and before the first iommu_attach if it is default-on. Thanks, MyungJoo -- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics -- 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 v3 4/4] OMAP3/4: iommu: adapt to runtime pm
On Tue, Nov 01, 2011 at 05:15:52PM -0500, Omar Ramirez Luna wrote:
Use runtime PM functionality interfaced with hwmod enable/idle
functions, to replace direct clock operations, reset and sysconfig
handling.
Tidspbridge uses a macro removed with this patch, for now the value
is hardcoded to avoid breaking compilation.
You probably want to include people involved with power management on
this, so maybe the linux-pm mailing list, and those involved with
runtime-pm stuff (I think Rafael qualifies as the maintainer for this
stuff, even if he's not listed in MAINTAINERS.)
Signed-off-by: Omar Ramirez Luna omar.rami...@ti.com
---
arch/arm/mach-omap2/iommu2.c | 17
arch/arm/mach-omap2/omap-iommu.c |1 -
arch/arm/plat-omap/include/plat/iommu.h |2 -
arch/arm/plat-omap/include/plat/iommu2.h |2 -
drivers/iommu/omap-iommu.c| 46
-
drivers/staging/tidspbridge/core/tiomap3430_pwr.c |2 +-
6 files changed, 19 insertions(+), 51 deletions(-)
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 60e3363..5adad97 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -25,15 +25,6 @@
*/
#define IOMMU_ARCH_VERSION 0x0011
-/* SYSCONF */
-#define MMU_SYS_IDLE_SHIFT 3
-#define MMU_SYS_IDLE_FORCE (0 MMU_SYS_IDLE_SHIFT)
-#define MMU_SYS_IDLE_NONE(1 MMU_SYS_IDLE_SHIFT)
-#define MMU_SYS_IDLE_SMART (2 MMU_SYS_IDLE_SHIFT)
-#define MMU_SYS_IDLE_MASK(3 MMU_SYS_IDLE_SHIFT)
-
-#define MMU_SYS_AUTOIDLE 1
-
/* IRQSTATUS IRQENABLE */
#define MMU_IRQ_MULTIHITFAULT(1 4)
#define MMU_IRQ_TABLEWALKFAULT (1 3)
@@ -96,11 +87,6 @@ static int omap2_iommu_enable(struct omap_iommu *obj)
dev_info(obj-dev, %s: version %d.%d\n, obj-name,
(l 4) 0xf, l 0xf);
- l = iommu_read_reg(obj, MMU_SYSCONFIG);
- l = ~MMU_SYS_IDLE_MASK;
- l |= (MMU_SYS_IDLE_SMART | MMU_SYS_AUTOIDLE);
- iommu_write_reg(obj, l, MMU_SYSCONFIG);
-
iommu_write_reg(obj, pa, MMU_TTB);
__iommu_set_twl(obj, true);
@@ -114,7 +100,6 @@ static void omap2_iommu_disable(struct omap_iommu *obj)
l = ~MMU_CNTL_MASK;
iommu_write_reg(obj, l, MMU_CNTL);
- iommu_write_reg(obj, MMU_SYS_IDLE_FORCE, MMU_SYSCONFIG);
dev_dbg(obj-dev, %s is shutting down\n, obj-name);
}
@@ -244,8 +229,6 @@ omap2_iommu_dump_ctx(struct omap_iommu *obj, char *buf,
ssize_t len)
char *p = buf;
pr_reg(REVISION);
- pr_reg(SYSCONFIG);
- pr_reg(SYSSTATUS);
pr_reg(IRQSTATUS);
pr_reg(IRQENABLE);
pr_reg(WALKING_ST);
diff --git a/arch/arm/mach-omap2/omap-iommu.c
b/arch/arm/mach-omap2/omap-iommu.c
index 669fd07..cad98c7 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -34,7 +34,6 @@ static int __init omap_iommu_dev_init(struct omap_hwmod
*oh, void *unused)
static int i;
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/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index 01927a5..3842e99 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -28,7 +28,6 @@ struct iotlb_entry {
struct omap_iommu {
const char *name;
struct module *owner;
- struct clk *clk;
void __iomem*regbase;
struct device *dev;
void*isr_priv;
@@ -119,7 +118,6 @@ struct omap_mmu_dev_attr {
struct iommu_platform_data {
const char *name;
- const char *clk_name;
int nr_tlb_entries;
u32 da_start;
u32 da_end;
diff --git a/arch/arm/plat-omap/include/plat/iommu2.h
b/arch/arm/plat-omap/include/plat/iommu2.h
index d4116b5..1579694 100644
--- a/arch/arm/plat-omap/include/plat/iommu2.h
+++ b/arch/arm/plat-omap/include/plat/iommu2.h
@@ -19,8 +19,6 @@
* MMU Register offsets
*/
#define MMU_REVISION 0x00
-#define MMU_SYSCONFIG0x10
-#define MMU_SYSSTATUS0x14
#define MMU_IRQSTATUS0x18
#define MMU_IRQENABLE0x1c
#define MMU_WALKING_ST 0x40
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bbbf747..3c55be0 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -16,11 +16,11 @@
#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/mutex.h
#include linux/spinlock.h
+#include linux/pm_runtime.h
#include asm/cacheflush.h
@@ -123,11 +123,11 @@ static int iommu_enable(struct omap_iommu *obj)
Re: [PATCH v3 4/4] OMAP3/4: iommu: adapt to runtime pm
Hi MyungJoo, On Wed, Nov 2, 2011 at 5:16 AM, MyungJoo Ham myungjoo@gmail.com wrote: On Wed, Nov 2, 2011 at 7:15 AM, Omar Ramirez Luna omar.rami...@ti.com wrote: Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling. Tidspbridge uses a macro removed with this patch, for now the value is hardcoded to avoid breaking compilation. Signed-off-by: Omar Ramirez Luna omar.rami...@ti.com --- arch/arm/mach-omap2/iommu2.c | 17 arch/arm/mach-omap2/omap-iommu.c | 1 - arch/arm/plat-omap/include/plat/iommu.h | 2 - arch/arm/plat-omap/include/plat/iommu2.h | 2 - drivers/iommu/omap-iommu.c | 46 - drivers/staging/tidspbridge/core/tiomap3430_pwr.c | 2 +- 6 files changed, 19 insertions(+), 51 deletions(-) diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index bbbf747..3c55be0 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -123,11 +123,11 @@ static int iommu_enable(struct omap_iommu *obj) if (!arch_iommu) return -ENODEV; - clk_enable(obj-clk); + pm_runtime_enable(obj-dev); + pm_runtime_get_sync(obj-dev); err = arch_iommu-enable(obj); - clk_disable(obj-clk); return err; } @@ -136,11 +136,10 @@ static void iommu_disable(struct omap_iommu *obj) if (!obj) return; - clk_enable(obj-clk); - arch_iommu-disable(obj); - clk_disable(obj-clk); + pm_runtime_put_sync(obj-dev); + pm_runtime_disable(obj-dev); } I'm just curious here... Is there any reason to do pm_runtime_enable/disable at iommu_enable/iommu_disable which are called by iommu_attach/detach? I thought that normally, ideal locations of pm_runtime_enable/disable for such devices are in probe/remove() because it assures that the device is suspended after the probe. It seems that the device might be kept on after probe and before the first iommu_attach if it is default-on. The default state of these MMUs is on reset and needs to be deasserted to be used, but you're right, it makes more sense to move pm_runtime_enable/disable calls to probe/remove. I'll wait a bit, do this change and resubmit. Thanks for the comment, 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 v3 4/4] OMAP3/4: iommu: adapt to runtime pm
On Wed, Nov 2, 2011 at 10:21 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Nov 01, 2011 at 05:15:52PM -0500, Omar Ramirez Luna wrote: Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling. Tidspbridge uses a macro removed with this patch, for now the value is hardcoded to avoid breaking compilation. You probably want to include people involved with power management on this, so maybe the linux-pm mailing list, and those involved with runtime-pm stuff (I think Rafael qualifies as the maintainer for this stuff, even if he's not listed in MAINTAINERS.) Will do, I'll submit it again if no other comments are received. Regards, 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
[PATCH v3 4/4] OMAP3/4: iommu: adapt to runtime pm
Use runtime PM functionality interfaced with hwmod enable/idle
functions, to replace direct clock operations, reset and sysconfig
handling.
Tidspbridge uses a macro removed with this patch, for now the value
is hardcoded to avoid breaking compilation.
Signed-off-by: Omar Ramirez Luna omar.rami...@ti.com
---
arch/arm/mach-omap2/iommu2.c | 17
arch/arm/mach-omap2/omap-iommu.c |1 -
arch/arm/plat-omap/include/plat/iommu.h |2 -
arch/arm/plat-omap/include/plat/iommu2.h |2 -
drivers/iommu/omap-iommu.c| 46 -
drivers/staging/tidspbridge/core/tiomap3430_pwr.c |2 +-
6 files changed, 19 insertions(+), 51 deletions(-)
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 60e3363..5adad97 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -25,15 +25,6 @@
*/
#define IOMMU_ARCH_VERSION 0x0011
-/* SYSCONF */
-#define MMU_SYS_IDLE_SHIFT 3
-#define MMU_SYS_IDLE_FORCE (0 MMU_SYS_IDLE_SHIFT)
-#define MMU_SYS_IDLE_NONE (1 MMU_SYS_IDLE_SHIFT)
-#define MMU_SYS_IDLE_SMART (2 MMU_SYS_IDLE_SHIFT)
-#define MMU_SYS_IDLE_MASK (3 MMU_SYS_IDLE_SHIFT)
-
-#define MMU_SYS_AUTOIDLE 1
-
/* IRQSTATUS IRQENABLE */
#define MMU_IRQ_MULTIHITFAULT (1 4)
#define MMU_IRQ_TABLEWALKFAULT (1 3)
@@ -96,11 +87,6 @@ static int omap2_iommu_enable(struct omap_iommu *obj)
dev_info(obj-dev, %s: version %d.%d\n, obj-name,
(l 4) 0xf, l 0xf);
- l = iommu_read_reg(obj, MMU_SYSCONFIG);
- l = ~MMU_SYS_IDLE_MASK;
- l |= (MMU_SYS_IDLE_SMART | MMU_SYS_AUTOIDLE);
- iommu_write_reg(obj, l, MMU_SYSCONFIG);
-
iommu_write_reg(obj, pa, MMU_TTB);
__iommu_set_twl(obj, true);
@@ -114,7 +100,6 @@ static void omap2_iommu_disable(struct omap_iommu *obj)
l = ~MMU_CNTL_MASK;
iommu_write_reg(obj, l, MMU_CNTL);
- iommu_write_reg(obj, MMU_SYS_IDLE_FORCE, MMU_SYSCONFIG);
dev_dbg(obj-dev, %s is shutting down\n, obj-name);
}
@@ -244,8 +229,6 @@ omap2_iommu_dump_ctx(struct omap_iommu *obj, char *buf,
ssize_t len)
char *p = buf;
pr_reg(REVISION);
- pr_reg(SYSCONFIG);
- pr_reg(SYSSTATUS);
pr_reg(IRQSTATUS);
pr_reg(IRQENABLE);
pr_reg(WALKING_ST);
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 669fd07..cad98c7 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -34,7 +34,6 @@ static int __init omap_iommu_dev_init(struct omap_hwmod *oh,
void *unused)
static int i;
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/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index 01927a5..3842e99 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -28,7 +28,6 @@ struct iotlb_entry {
struct omap_iommu {
const char *name;
struct module *owner;
- struct clk *clk;
void __iomem*regbase;
struct device *dev;
void*isr_priv;
@@ -119,7 +118,6 @@ struct omap_mmu_dev_attr {
struct iommu_platform_data {
const char *name;
- const char *clk_name;
int nr_tlb_entries;
u32 da_start;
u32 da_end;
diff --git a/arch/arm/plat-omap/include/plat/iommu2.h
b/arch/arm/plat-omap/include/plat/iommu2.h
index d4116b5..1579694 100644
--- a/arch/arm/plat-omap/include/plat/iommu2.h
+++ b/arch/arm/plat-omap/include/plat/iommu2.h
@@ -19,8 +19,6 @@
* MMU Register offsets
*/
#define MMU_REVISION 0x00
-#define MMU_SYSCONFIG 0x10
-#define MMU_SYSSTATUS 0x14
#define MMU_IRQSTATUS 0x18
#define MMU_IRQENABLE 0x1c
#define MMU_WALKING_ST 0x40
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bbbf747..3c55be0 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -16,11 +16,11 @@
#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/mutex.h
#include linux/spinlock.h
+#include linux/pm_runtime.h
#include asm/cacheflush.h
@@ -123,11 +123,11 @@ static int iommu_enable(struct omap_iommu *obj)
if (!arch_iommu)
return -ENODEV;
- clk_enable(obj-clk);
+ pm_runtime_enable(obj-dev);
+ pm_runtime_get_sync(obj-dev);
err = arch_iommu-enable(obj);
- clk_disable(obj-clk);
return err;
}
@@ -136,11 +136,10 @@ static void iommu_disable(struct omap_iommu *obj)
if (!obj)
return;
- clk_enable(obj-clk);
-
