[PATCH 0/6] omap: iommu: hwmod support and code reorganization

[PATCH 0/6] omap: iommu: hwmod support and code reorganization

[Omar Ramirez Luna]

Boot tested on omap 3430 and 3630, functionality on iva2 only.

Introduced hwmod data and support for omap3 (iva2 mmu & isp mmu) and
omap4 (ducati mmu & tesla mmu).

Minor cleanup due to this changes.

There is an issue (present without patches too):

Doing a cycle of insmod-rmmod to iommu module and then full
insmod of bridgedriver (with all dependencies) causes the next
read of iommu registers to dump an unhandled fault log; this,
because when rmmod of iommu is called it tries to clean the
iommu tables and flush any old entry prior to removing the driver,
however these reads/writes are attempted while the MMU is under
reset and will timeout on the L3 IVA target agent, which will leave
MMU unusable until the proper restore sequence to clear this L3
error is executed.

Fix would be to move page table allocation to iommu get and its
correspondent free to iommu put, however it will fall entirely
under iommu user responsibility, as it needs to clear the mmu
reset bit, before calling iommu functions that read/write into
mmu registers (which should apply only for first boot or on
baseimage reload); trading this restriction in order to keep
the same generic iommu code for all omap iommu devices.

This issue has been seen on omap3 iva2 mmu, same restriction should
apply for tesla mmu.

For discussion: should iommu handle mmu reset source directly?
This register is grouped into an IVA PRM register which handles
sequencer, iva2 mmu and dsp resets. As mentioned iommu handles
cam, iva2 mmu (OMAP3) and tesla, ducati mmu (OMAP4), only tesla
and iva2 should suffer from this restriction and according changes
should be needed to handle both cases on mach-omap2 code. This
also affects hwmod, since, if defined, it tries to read SYSC
registers at boot time when registering the hwmod and causing the
same issue.

Omar Ramirez Luna (6):
  omap: iommu: remove redundant clock usage
  OMAP3: hwmod data: Add mmu for iva2 and isp
  OMAP4: hwmod data: add mmu hwmod for ducati and tesla
  omap: iommu: intial hwmod support
  omap: iommu: hwmod device enable/disable routines
  omap: iommu: code reorganization and cleanup

 arch/arm/mach-omap2/omap-iommu.c           |  168 +++++++++-------------------
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |  103 +++++++++++++++++
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  105 +++++++++++++++++
 arch/arm/plat-omap/include/plat/iommu.h    |   19 +++-
 arch/arm/plat-omap/include/plat/omap34xx.h |    2 +
 arch/arm/plat-omap/iommu.c                 |   64 ++++-------
 6 files changed, 299 insertions(+), 162 deletions(-)

[PATCH 1/6] omap: iommu: remove redundant clock usage

[Omar Ramirez Luna]

iommu driver is meant to provide control of mmu hardware blocks
its current users (MMUs) are part of larger subsystems and do not
have a dedicated clock as the one they use is shared with the
entire subsystem, it doesn't make sense to enable/disable on each
register read/write operation as the driver using its interface
should also be handling the same clock.

iommu should only enable/disable the clock on mmu request/free from
the driver wanting to use it.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/plat-omap/iommu.c |   38 +++++---------------------------------
 1 files changed, 5 insertions(+), 33 deletions(-)

diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index 6cd151b..de992c8 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -108,7 +108,6 @@ static int iommu_enable(struct iommu *obj)
 
 	err = arch_iommu->enable(obj);
 
-	clk_disable(obj->clk);
 	return err;
 }
 
@@ -117,8 +116,6 @@ static void iommu_disable(struct iommu *obj)
 	if (!obj)
 		return;
 
-	clk_enable(obj->clk);
-
 	arch_iommu->disable(obj);
 
 	clk_disable(obj->clk);
@@ -237,20 +234,16 @@ static struct cr_regs __iotlb_read_cr(struct iommu *obj, int n)
  **/
 int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
 {
-	int err = 0;
 	struct iotlb_lock l;
 	struct cr_regs *cr;
 
 	if (!obj || !obj->nr_tlb_entries || !e)
 		return -EINVAL;
 
-	clk_enable(obj->clk);
-
 	iotlb_lock_get(obj, &l);
 	if (l.base == obj->nr_tlb_entries) {
 		dev_warn(obj->dev, "%s: preserve entries full\n", __func__);
-		err = -EBUSY;
-		goto out;
+		return -EBUSY;
 	}
 	if (!e->prsvd) {
 		int i;
@@ -262,8 +255,7 @@ int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
 
 		if (i == obj->nr_tlb_entries) {
 			dev_dbg(obj->dev, "%s: full: no entry\n", __func__);
-			err = -EBUSY;
-			goto out;
+			return -EBUSY;
 		}
 
 		iotlb_lock_get(obj, &l);
@@ -273,10 +265,8 @@ int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
 	}
 
 	cr = iotlb_alloc_cr(obj, e);
-	if (IS_ERR(cr)) {
-		clk_disable(obj->clk);
+	if (IS_ERR(cr))
 		return PTR_ERR(cr);
-	}
 
 	iotlb_load_cr(obj, cr);
 	kfree(cr);
@@ -287,9 +277,8 @@ int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
 	if (++l.vict == obj->nr_tlb_entries)
 		l.vict = l.base;
 	iotlb_lock_set(obj, &l);
-out:
-	clk_disable(obj->clk);
-	return err;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(load_iotlb_entry);
 
@@ -305,8 +294,6 @@ void flush_iotlb_page(struct iommu *obj, u32 da)
 	int i;
 	struct cr_regs cr;
 
-	clk_enable(obj->clk);
-
 	for_each_iotlb_cr(obj, obj->nr_tlb_entries, i, cr) {
 		u32 start;
 		size_t bytes;
@@ -324,7 +311,6 @@ void flush_iotlb_page(struct iommu *obj, u32 da)
 			iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
 		}
 	}
-	clk_disable(obj->clk);
 
 	if (i == obj->nr_tlb_entries)
 		dev_dbg(obj->dev, "%s: no page for %08x\n", __func__, da);
@@ -359,15 +345,11 @@ void flush_iotlb_all(struct iommu *obj)
 {
 	struct iotlb_lock l;
 
-	clk_enable(obj->clk);
-
 	l.base = 0;
 	l.vict = 0;
 	iotlb_lock_set(obj, &l);
 
 	iommu_write_reg(obj, 1, MMU_GFLUSH);
-
-	clk_disable(obj->clk);
 }
 EXPORT_SYMBOL_GPL(flush_iotlb_all);
 
@@ -382,9 +364,7 @@ EXPORT_SYMBOL_GPL(flush_iotlb_all);
  */
 void iommu_set_twl(struct iommu *obj, bool on)
 {
-	clk_enable(obj->clk);
 	arch_iommu->set_twl(obj, on);
-	clk_disable(obj->clk);
 }
 EXPORT_SYMBOL_GPL(iommu_set_twl);
 
@@ -395,12 +375,8 @@ ssize_t iommu_dump_ctx(struct iommu *obj, char *buf, ssize_t bytes)
 	if (!obj || !buf)
 		return -EINVAL;
 
-	clk_enable(obj->clk);
-
 	bytes = arch_iommu->dump_ctx(obj, buf, bytes);
 
-	clk_disable(obj->clk);
-
 	return bytes;
 }
 EXPORT_SYMBOL_GPL(iommu_dump_ctx);
@@ -412,7 +388,6 @@ static int __dump_tlb_entries(struct iommu *obj, struct cr_regs *crs, int num)
 	struct cr_regs tmp;
 	struct cr_regs *p = crs;
 
-	clk_enable(obj->clk);
 	iotlb_lock_get(obj, &saved);
 
 	for_each_iotlb_cr(obj, num, i, tmp) {
@@ -422,7 +397,6 @@ static int __dump_tlb_entries(struct iommu *obj, struct cr_regs *crs, int num)
 	}
 
 	iotlb_lock_set(obj, &saved);
-	clk_disable(obj->clk);
 
 	return  p - crs;
 }
@@ -795,9 +769,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
 	if (!err)
 		return IRQ_HANDLED;
 
-	clk_enable(obj->clk);
 	stat = iommu_report_fault(obj, &da);
-	clk_disable(obj->clk);
 	if (!stat)
 		return IRQ_HANDLED;
 
-- 
1.7.1

[PATCH 2/6] OMAP3: hwmod data: Add mmu for iva2 and isp

[Omar Ramirez Luna]

Add mmu hwmod data for iva2 and isp.

Plus a define for the iva2 base register.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |  103 ++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/iommu.h    |    8 ++
 arch/arm/plat-omap/include/plat/omap34xx.h |    2 +
 3 files changed, 113 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 60d977e..ff80efc 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -21,6 +21,7 @@
 #include <plat/l4_3xxx.h>
 #include <plat/i2c.h>
 #include <plat/omap34xx.h>
+#include <plat/iommu.h>
 
 #include "omap_hwmod_common_data.h"
 
@@ -801,6 +802,106 @@ static struct omap_hwmod omap3xxx_mailbox_hwmod = {
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
 };
 
+/* mmu */
+
+static struct omap_hwmod_class omap3xxx_mmu_hwmod_class = {
+	.name = "mmu",
+};
+
+/* isp mmu */
+
+static struct omap_hwmod omap3xxx_isp_mmu_hwmod;
+
+static struct omap_hwmod_addr_space omap3xxx_isp_mmu_addrs[] = {
+	{
+		.pa_start	= OMAP3430_ISP_MMU_BASE,
+		.pa_end		= OMAP3430_ISP_MMU_BASE + SZ_256 - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l4_core -> isp mmu */
+static struct omap_hwmod_ocp_if omap3xxx_l4_core__isp_mmu = {
+	.master		= &omap3xxx_l4_core_hwmod,
+	.slave		= &omap3xxx_isp_mmu_hwmod,
+	.addr		= omap3xxx_isp_mmu_addrs,
+	.clk		= "cam_ick",
+	.addr_cnt	= ARRAY_SIZE(omap3xxx_isp_mmu_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* isp mmu slave ports */
+static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = {
+	&omap3xxx_l4_core__isp_mmu,
+};
+
+static struct omap_hwmod_irq_info omap3xxx_isp_mmu_irqs[] = {
+	{ .name = "isp", .irq = INT_24XX_CAM_IRQ, },
+};
+
+static struct omap_mmu_dev_attr isp_mmu_dev_attr = {
+	.nr_tlb_entries = 8,
+};
+
+static struct omap_hwmod omap3xxx_isp_mmu_hwmod = {
+	.name		= "isp",
+	.class		= &omap3xxx_mmu_hwmod_class,
+	.mpu_irqs	= omap3xxx_isp_mmu_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap3xxx_isp_mmu_irqs),
+	.slaves		= omap3xxx_isp_mmu_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
+	.dev_attr	= &isp_mmu_dev_attr,
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.flags		= HWMOD_NO_IDLEST,
+};
+
+/* iva2 mmu */
+
+static struct omap_hwmod omap3xxx_iva2_mmu_hwmod;
+
+static struct omap_hwmod_addr_space omap3xxx_iva2_mmu_addrs[] = {
+	{
+		.pa_start	= OMAP34XX_IVA2_MMU_BASE,
+		.pa_end		= OMAP34XX_IVA2_MMU_BASE + SZ_256 - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l3_main -> iva2 mmu */
+static struct omap_hwmod_ocp_if omap3xxx_l3_main__iva2_mmu = {
+	.master		= &omap3xxx_l3_main_hwmod,
+	.slave		= &omap3xxx_iva2_mmu_hwmod,
+	.addr		= omap3xxx_iva2_mmu_addrs,
+	.addr_cnt	= ARRAY_SIZE(omap3xxx_iva2_mmu_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* iva2 mmu slave ports */
+static struct omap_hwmod_ocp_if *omap3xxx_iva2_mmu_slaves[] = {
+	&omap3xxx_l3_main__iva2_mmu,
+};
+
+static struct omap_hwmod_irq_info omap3xxx_iva2_mmu_irqs[] = {
+	{ .name = "iva2", .irq = INT_24XX_DSP_MMU, },
+};
+
+static struct omap_mmu_dev_attr iva2_mmu_dev_attr = {
+	.nr_tlb_entries = 32,
+};
+
+static struct omap_hwmod omap3xxx_iva2_mmu_hwmod = {
+	.name		= "iva2",
+	.class		= &omap3xxx_mmu_hwmod_class,
+	.mpu_irqs	= omap3xxx_iva2_mmu_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap3xxx_iva2_mmu_irqs),
+	.main_clk	= "iva2_ck",
+	.slaves		= omap3xxx_iva2_mmu_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap3xxx_iva2_mmu_slaves),
+	.dev_attr	= &iva2_mmu_dev_attr,
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.flags		= HWMOD_NO_IDLEST,
+};
+
 static __initdata struct omap_hwmod *omap3xxx_hwmods[] = {
 	&omap3xxx_l3_main_hwmod,
 	&omap3xxx_l4_core_hwmod,
@@ -817,6 +918,8 @@ static __initdata struct omap_hwmod *omap3xxx_hwmods[] = {
 	&omap3xxx_i2c2_hwmod,
 	&omap3xxx_i2c3_hwmod,
 	&omap3xxx_mailbox_hwmod,
+	&omap3xxx_isp_mmu_hwmod,
+	&omap3xxx_iva2_mmu_hwmod,
 	NULL,
 };
 
diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 33c7d41..91a75a5 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -99,6 +99,14 @@ struct iommu_functions {
 	ssize_t (*dump_ctx)(struct iommu *obj, char *buf, ssize_t len);
 };
 
+/* omap_mmu_dev_attr - OMAP mmu device attributes for omap_hwmod
+ * @nr_tlb_entries: number of entries supported by the translation look-aside
+ *                   buffer (TLB).
+ */
+struct omap_mmu_dev_attr {
+	int nr_tlb_entries;
+};
+
 struct iommu_platform_data {
 	const char *name;
 	const char *clk_name;
diff --git a/arch/arm/plat-omap/include/plat/omap34xx.h b/arch/arm/plat-omap/include/plat/omap34xx.h
index 98fc8b4..697f8f9 100644
--- a/arch/arm/plat-omap/include/plat/omap34xx.h
+++ b/arch/arm/plat-omap/include/plat/omap34xx.h
@@ -82,6 +82,8 @@
 
 #define OMAP34XX_MAILBOX_BASE		(L4_34XX_BASE + 0x94000)
 
+#define OMAP34XX_IVA2_MMU_BASE	0x5D000000
+
 /* Security */
 #define OMAP34XX_SEC_BASE	(L4_34XX_BASE + 0xA0000)
 #define OMAP34XX_SEC_SHA1MD5_BASE	(OMAP34XX_SEC_BASE + 0x23000)
-- 
1.7.1

[PATCH 3/6] OMAP4: hwmod data: add mmu hwmod for ducati and tesla

[Omar Ramirez Luna]

Add mmu hwmod data for ducati and tesla.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  105 ++++++++++++++++++++++++++++
 1 files changed, 105 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index f7525e3..1d5eace 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -22,6 +22,7 @@
 
 #include <plat/omap_hwmod.h>
 #include <plat/cpu.h>
+#include <plat/iommu.h>
 
 #include "omap_hwmod_common_data.h"
 
@@ -1103,6 +1104,106 @@ static struct omap_hwmod omap44xx_mailbox_hwmod = {
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
 };
 
+/* mmu */
+
+static struct omap_hwmod_class omap44xx_mmu_hwmod_class = {
+	.name = "mmu",
+};
+
+/* ducati mmu */
+
+static struct omap_hwmod omap44xx_ducati_mmu_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_ducati_mmu_addrs[] = {
+	{
+		.pa_start	= OMAP4_MMU1_BASE,
+		.pa_end		= OMAP4_MMU1_BASE + SZ_4K - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l3_main_1 -> ducati mmu */
+static struct omap_hwmod_ocp_if omap44xx_l3_main_1__ducati_mmu = {
+	.master		= &omap44xx_l3_main_1_hwmod,
+	.slave		= &omap44xx_ducati_mmu_hwmod,
+	.addr		= omap44xx_ducati_mmu_addrs,
+	.clk		= "dpll_mpu_m2_ck",
+	.addr_cnt	= ARRAY_SIZE(omap44xx_ducati_mmu_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* ducati mmu slave ports */
+static struct omap_hwmod_ocp_if *omap44xx_ducati_mmu_slaves[] = {
+	&omap44xx_l3_main_1__ducati_mmu,
+};
+
+static struct omap_hwmod_irq_info omap44xx_ducati_mmu_irqs[] = {
+	{ .name = "ducati", .irq = 100 + OMAP44XX_IRQ_GIC_START, },
+};
+
+static struct omap_mmu_dev_attr ducati_mmu_dev_attr = {
+	.nr_tlb_entries = 32,
+};
+
+static struct omap_hwmod omap44xx_ducati_mmu_hwmod = {
+	.name		= "ducati",
+	.class		= &omap44xx_mmu_hwmod_class,
+	.mpu_irqs	= omap44xx_ducati_mmu_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_ducati_mmu_irqs),
+	.slaves		= omap44xx_ducati_mmu_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap44xx_ducati_mmu_slaves),
+	.dev_attr	= &ducati_mmu_dev_attr,
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
+	.flags		= HWMOD_NO_IDLEST,
+};
+
+/* tesla mmu */
+
+static struct omap_hwmod omap44xx_tesla_mmu_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_tesla_mmu_addrs[] = {
+	{
+		.pa_start	= OMAP4_MMU2_BASE,
+		.pa_end		= OMAP4_MMU2_BASE + SZ_4K - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l3_main_1 -> tesla mmu */
+static struct omap_hwmod_ocp_if omap44xx_l3_main_1__tesla_mmu = {
+	.master		= &omap44xx_l3_main_1_hwmod,
+	.slave		= &omap44xx_tesla_mmu_hwmod,
+	.addr		= omap44xx_tesla_mmu_addrs,
+	.addr_cnt	= ARRAY_SIZE(omap44xx_tesla_mmu_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* tesla mmu slave ports */
+static struct omap_hwmod_ocp_if *omap44xx_tesla_mmu_slaves[] = {
+	&omap44xx_l3_main_1__tesla_mmu,
+};
+
+static struct omap_hwmod_irq_info omap44xx_tesla_mmu_irqs[] = {
+	{ .name = "tesla", .irq = 28 + OMAP44XX_IRQ_GIC_START, },
+};
+
+static struct omap_mmu_dev_attr tesla_mmu_dev_attr = {
+	.nr_tlb_entries = 32,
+};
+
+static struct omap_hwmod omap44xx_tesla_mmu_hwmod = {
+	.name		= "tesla",
+	.class		= &omap44xx_mmu_hwmod_class,
+	.mpu_irqs	= omap44xx_tesla_mmu_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_tesla_mmu_irqs),
+	.main_clk	= "dsp_fck",
+	.slaves		= omap44xx_tesla_mmu_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap44xx_tesla_mmu_slaves),
+	.dev_attr	= &tesla_mmu_dev_attr,
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
+	.flags		= HWMOD_NO_IDLEST,
+};
+
 static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
 	/* dmm class */
 	&omap44xx_dmm_hwmod,
@@ -1140,6 +1241,10 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
 
 	/* mailbox */
 	&omap44xx_mailbox_hwmod,
+
+	/* mmu */
+	&omap44xx_ducati_mmu_hwmod,
+	&omap44xx_tesla_mmu_hwmod,
 	NULL,
 };
 
-- 
1.7.1

[PATCH 4/6] omap: iommu: intial hwmod support

[Omar Ramirez Luna]

Use the defined hwmod data according to the devices
present on omap3 and omap4.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap-iommu.c        |   77 ++++++++-----------------------
 arch/arm/plat-omap/include/plat/iommu.h |    2 +-
 arch/arm/plat-omap/iommu.c              |    2 +-
 3 files changed, 21 insertions(+), 60 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index f5a1aad..65460ef 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -14,12 +14,11 @@
 
 #include <plat/iommu.h>
 #include <plat/irqs.h>
+#include <plat/omap_hwmod.h>
+#include <plat/omap_device.h>
 
 struct iommu_device {
-	resource_size_t base;
-	int irq;
 	struct iommu_platform_data pdata;
-	struct resource res[2];
 };
 static struct iommu_device *devices;
 static int num_iommu_devices;
@@ -27,128 +26,90 @@ static int num_iommu_devices;
 #ifdef CONFIG_ARCH_OMAP3
 static struct iommu_device omap3_devices[] = {
 	{
-		.base = 0x480bd400,
-		.irq = 24,
 		.pdata = {
 			.name = "isp",
-			.nr_tlb_entries = 8,
 			.clk_name = "cam_ick",
 		},
 	},
 #if defined(CONFIG_MPU_BRIDGE_IOMMU)
 	{
-		.base = 0x5d000000,
-		.irq = 28,
 		.pdata = {
 			.name = "iva2",
-			.nr_tlb_entries = 32,
 			.clk_name = "iva2_ck",
 		},
 	},
 #endif
 };
 #define NR_OMAP3_IOMMU_DEVICES ARRAY_SIZE(omap3_devices)
-static struct platform_device *omap3_iommu_pdev[NR_OMAP3_IOMMU_DEVICES];
 #else
 #define omap3_devices		NULL
 #define NR_OMAP3_IOMMU_DEVICES	0
-#define omap3_iommu_pdev	NULL
 #endif
 
 #ifdef CONFIG_ARCH_OMAP4
 static struct iommu_device omap4_devices[] = {
 	{
-		.base = OMAP4_MMU1_BASE,
-		.irq = OMAP44XX_IRQ_DUCATI_MMU,
 		.pdata = {
 			.name = "ducati",
-			.nr_tlb_entries = 32,
 			.clk_name = "ducati_ick",
 		},
 	},
 #if defined(CONFIG_MPU_TESLA_IOMMU)
 	{
-		.base = OMAP4_MMU2_BASE,
-		.irq = INT_44XX_DSP_MMU,
 		.pdata = {
 			.name = "tesla",
-			.nr_tlb_entries = 32,
 			.clk_name = "tesla_ick",
 		},
 	},
 #endif
 };
 #define NR_OMAP4_IOMMU_DEVICES ARRAY_SIZE(omap4_devices)
-static struct platform_device *omap4_iommu_pdev[NR_OMAP4_IOMMU_DEVICES];
 #else
 #define omap4_devices		NULL
 #define NR_OMAP4_IOMMU_DEVICES	0
-#define omap4_iommu_pdev	NULL
 #endif
 
-static struct platform_device **omap_iommu_pdev;
-
 static int __init omap_iommu_init(void)
 {
-	int i, err;
-	struct resource res[] = {
-		{ .flags = IORESOURCE_MEM },
-		{ .flags = IORESOURCE_IRQ },
-	};
+	int i;
 
 	if (cpu_is_omap34xx()) {
 		devices = omap3_devices;
-		omap_iommu_pdev = omap3_iommu_pdev;
 		num_iommu_devices = NR_OMAP3_IOMMU_DEVICES;
 	} else if (cpu_is_omap44xx()) {
 		devices = omap4_devices;
-		omap_iommu_pdev = omap4_iommu_pdev;
 		num_iommu_devices = NR_OMAP4_IOMMU_DEVICES;
 	} else
 		return -ENODEV;
 
 	for (i = 0; i < num_iommu_devices; i++) {
-		struct platform_device *pdev;
-		const struct iommu_device *d = &devices[i];
+		struct omap_hwmod *oh;
+		struct omap_device *od;
 
-		pdev = platform_device_alloc("omap-iommu", i);
-		if (!pdev) {
-			err = -ENOMEM;
-			goto err_out;
+		oh = omap_hwmod_lookup(devices[i].pdata.name);
+		if (!oh) {
+			pr_err("%s: hwmod not found\n", __func__);
+			return -ENODEV;
 		}
 
-		res[0].start = d->base;
-		res[0].end = d->base + MMU_REG_SIZE - 1;
-		res[1].start = res[1].end = d->irq;
+		devices[i].pdata.mmu_attr =
+				(struct omap_mmu_dev_attr *)oh->dev_attr;
 
-		err = platform_device_add_resources(pdev, res,
-						    ARRAY_SIZE(res));
-		if (err)
-			goto err_out;
-		err = platform_device_add_data(pdev, &d->pdata,
-					       sizeof(d->pdata));
-		if (err)
-			goto err_out;
-		err = platform_device_add(pdev);
-		if (err)
-			goto err_out;
-		omap_iommu_pdev[i] = pdev;
+		od = omap_device_build("omap-iommu", i, oh,
+				&devices[i].pdata, sizeof(devices[i].pdata),
+				NULL, 0,
+				0);
+		if (!od) {
+			pr_err("%s: error device build failed\n", __func__);
+			return -EPERM;
+		}
 	}
 	return 0;
-
-err_out:
-	while (i--)
-		platform_device_put(omap_iommu_pdev[i]);
-	return err;
 }
 module_init(omap_iommu_init);
 
 static void __exit omap_iommu_exit(void)
 {
-	int i;
-
-	for (i = 0; i < num_iommu_devices; i++)
-		platform_device_unregister(omap_iommu_pdev[i]);
 }
 module_exit(omap_iommu_exit);
 
diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 91a75a5..9650309 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -110,7 +110,7 @@ struct omap_mmu_dev_attr {
 struct iommu_platform_data {
 	const char *name;
 	const char *clk_name;
-	const int nr_tlb_entries;
+	struct omap_mmu_dev_attr *mmu_attr;
 };
 
 #if defined(CONFIG_ARCH_OMAP1)
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index de992c8..0fc9d90 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -890,7 +890,7 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
 	if (IS_ERR(obj->clk))
 		goto err_clk;
 
-	obj->nr_tlb_entries = pdata->nr_tlb_entries;
+	obj->nr_tlb_entries = pdata->mmu_attr->nr_tlb_entries;
 	obj->name = pdata->name;
 	obj->dev = &pdev->dev;
 	obj->ctx = (void *)obj + sizeof(*obj);
-- 
1.7.1

[PATCH 5/6] omap: iommu: hwmod device enable/disable routines

[Omar Ramirez Luna]

Use omap device enable/disable routines.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap-iommu.c        |   16 +++++++++++-----
 arch/arm/plat-omap/include/plat/iommu.h |    7 +++++--
 arch/arm/plat-omap/iommu.c              |   24 +++++++++++++++---------
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 65460ef..0a76bce 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -28,14 +28,12 @@ static struct iommu_device omap3_devices[] = {
 	{
 		.pdata = {
 			.name = "isp",
-			.clk_name = "cam_ick",
 		},
 	},
 #if defined(CONFIG_MPU_BRIDGE_IOMMU)
 	{
 		.pdata = {
 			.name = "iva2",
-			.clk_name = "iva2_ck",
 		},
 	},
 #endif
@@ -51,14 +49,12 @@ static struct iommu_device omap4_devices[] = {
 	{
 		.pdata = {
 			.name = "ducati",
-			.clk_name = "ducati_ick",
 		},
 	},
 #if defined(CONFIG_MPU_TESLA_IOMMU)
 	{
 		.pdata = {
 			.name = "tesla",
-			.clk_name = "tesla_ick",
 		},
 	},
 #endif
@@ -69,6 +65,14 @@ static struct iommu_device omap4_devices[] = {
 #define NR_OMAP4_IOMMU_DEVICES	0
 #endif
 
+static struct omap_device_pm_latency iommu_latencies[] = {
+	[0] = {
+		.activate_func = omap_device_enable_clocks,
+		.deactivate_func = omap_device_enable_clocks,
+		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST
+	},
+};
+
 static int __init omap_iommu_init(void)
 {
 	int i;
@@ -94,10 +98,12 @@ static int __init omap_iommu_init(void)
 
 		devices[i].pdata.mmu_attr =
 				(struct omap_mmu_dev_attr *)oh->dev_attr;
+		devices[i].pdata.device_enable = omap_device_enable;
+		devices[i].pdata.device_disable = omap_device_idle;
 
 		od = omap_device_build("omap-iommu", i, oh,
 				&devices[i].pdata, sizeof(devices[i].pdata),
-				NULL, 0,
+				iommu_latencies, ARRAY_SIZE(iommu_latencies),
 				0);
 		if (!od) {
 			pr_err("%s: error device build failed\n", __func__);
diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 9650309..fd8ffeb 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -13,6 +13,8 @@
 #ifndef __MACH_IOMMU_H
 #define __MACH_IOMMU_H
 
+struct platform_device;
+
 struct iotlb_entry {
 	u32 da;
 	u32 pa;
@@ -28,7 +30,6 @@ struct iotlb_entry {
 struct iommu {
 	const char	*name;
 	struct module	*owner;
-	struct clk	*clk;
 	void __iomem	*regbase;
 	struct device	*dev;
 
@@ -109,8 +110,10 @@ struct omap_mmu_dev_attr {
 
 struct iommu_platform_data {
 	const char *name;
-	const char *clk_name;
 	struct omap_mmu_dev_attr *mmu_attr;
+
+	int (*device_enable)(struct platform_device *pdev);
+	int (*device_disable)(struct platform_device *pdev);
 };
 
 #if defined(CONFIG_ARCH_OMAP1)
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index 0fc9d90..36b1b63 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -100,11 +100,17 @@ EXPORT_SYMBOL_GPL(iommu_arch_version);
 static int iommu_enable(struct iommu *obj)
 {
 	int err;
+	struct iommu_platform_data *pdata;
+	struct platform_device *pdev;
 
 	if (!obj)
 		return -EINVAL;
 
-	clk_enable(obj->clk);
+	pdev = container_of(obj->dev, struct platform_device, dev);
+	pdata = obj->dev->platform_data;
+
+	if (pdata->device_enable)
+		pdata->device_enable(pdev);
 
 	err = arch_iommu->enable(obj);
 
@@ -113,12 +119,19 @@ static int iommu_enable(struct iommu *obj)
 
 static void iommu_disable(struct iommu *obj)
 {
+	struct iommu_platform_data *pdata;
+	struct platform_device *pdev;
+
 	if (!obj)
 		return;
 
 	arch_iommu->disable(obj);
 
-	clk_disable(obj->clk);
+	pdev = container_of(obj->dev, struct platform_device, dev);
+	pdata = obj->dev->platform_data;
+
+	if (pdata->device_enable)
+		pdata->device_disable(pdev);
 }
 
 /*
@@ -886,10 +899,6 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
 	if (!obj)
 		return -ENOMEM;
 
-	obj->clk = clk_get(&pdev->dev, pdata->clk_name);
-	if (IS_ERR(obj->clk))
-		goto err_clk;
-
 	obj->nr_tlb_entries = pdata->mmu_attr->nr_tlb_entries;
 	obj->name = pdata->name;
 	obj->dev = &pdev->dev;
@@ -949,8 +958,6 @@ err_irq:
 	release_mem_region(res->start, resource_size(res));
 	iounmap(obj->regbase);
 err_mem:
-	clk_put(obj->clk);
-err_clk:
 	kfree(obj);
 	return err;
 }
@@ -972,7 +979,6 @@ static int __devexit omap_iommu_remove(struct platform_device *pdev)
 	release_mem_region(res->start, resource_size(res));
 	iounmap(obj->regbase);
 
-	clk_put(obj->clk);
 	dev_info(&pdev->dev, "%s removed\n", obj->name);
 	kfree(obj);
 	return 0;
-- 
1.7.1

[PATCH 6/6] omap: iommu: code reorganization and cleanup

[Omar Ramirez Luna]

Since omap-iommu is now using hwmod, there are structures and
arrays that can be cleaned up to increase the readability of
the code.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap-iommu.c        |   95 +++++++++++--------------------
 arch/arm/plat-omap/include/plat/iommu.h |    2 +-
 2 files changed, 34 insertions(+), 63 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 0a76bce..135474b 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -17,53 +17,17 @@
 #include <plat/omap_hwmod.h>
 #include <plat/omap_device.h>
 
-struct iommu_device {
-	struct iommu_platform_data pdata;
+static char *omap3_devices[] = {
+	"isp",
+	"iva2",
+	NULL,
 };
-static struct iommu_device *devices;
-static int num_iommu_devices;
-
-#ifdef CONFIG_ARCH_OMAP3
-static struct iommu_device omap3_devices[] = {
-	{
-		.pdata = {
-			.name = "isp",
-		},
-	},
-#if defined(CONFIG_MPU_BRIDGE_IOMMU)
-	{
-		.pdata = {
-			.name = "iva2",
-		},
-	},
-#endif
-};
-#define NR_OMAP3_IOMMU_DEVICES ARRAY_SIZE(omap3_devices)
-#else
-#define omap3_devices		NULL
-#define NR_OMAP3_IOMMU_DEVICES	0
-#endif
-
-#ifdef CONFIG_ARCH_OMAP4
-static struct iommu_device omap4_devices[] = {
-	{
-		.pdata = {
-			.name = "ducati",
-		},
-	},
-#if defined(CONFIG_MPU_TESLA_IOMMU)
-	{
-		.pdata = {
-			.name = "tesla",
-		},
-	},
-#endif
+
+static char *omap4_devices[] = {
+	"ducati",
+	"tesla",
+	NULL,
 };
-#define NR_OMAP4_IOMMU_DEVICES ARRAY_SIZE(omap4_devices)
-#else
-#define omap4_devices		NULL
-#define NR_OMAP4_IOMMU_DEVICES	0
-#endif
 
 static struct omap_device_pm_latency iommu_latencies[] = {
 	[0] = {
@@ -73,36 +37,28 @@ static struct omap_device_pm_latency iommu_latencies[] = {
 	},
 };
 
-static int __init omap_iommu_init(void)
+static int __init omap_iommu_add(char **devices)
 {
 	int i;
 
-	if (cpu_is_omap34xx()) {
-		devices = omap3_devices;
-		num_iommu_devices = NR_OMAP3_IOMMU_DEVICES;
-	} else if (cpu_is_omap44xx()) {
-		devices = omap4_devices;
-		num_iommu_devices = NR_OMAP4_IOMMU_DEVICES;
-	} else
-		return -ENODEV;
-
-	for (i = 0; i < num_iommu_devices; i++) {
+	for (i = 0; devices[i]; i++) {
 		struct omap_hwmod *oh;
 		struct omap_device *od;
+		struct iommu_platform_data pdata;
 
-		oh = omap_hwmod_lookup(devices[i].pdata.name);
+		oh = omap_hwmod_lookup(devices[i]);
 		if (!oh) {
 			pr_err("%s: hwmod not found\n", __func__);
 			return -ENODEV;
 		}
 
-		devices[i].pdata.mmu_attr =
-				(struct omap_mmu_dev_attr *)oh->dev_attr;
-		devices[i].pdata.device_enable = omap_device_enable;
-		devices[i].pdata.device_disable = omap_device_idle;
+		pdata.name = devices[i];
+		pdata.mmu_attr = (struct omap_mmu_dev_attr *)oh->dev_attr;
+		pdata.device_enable = omap_device_enable;
+		pdata.device_disable = omap_device_idle;
 
 		od = omap_device_build("omap-iommu", i, oh,
-				&devices[i].pdata, sizeof(devices[i].pdata),
+				&pdata, sizeof(pdata),
 				iommu_latencies, ARRAY_SIZE(iommu_latencies),
 				0);
 		if (!od) {
@@ -110,8 +66,23 @@ static int __init omap_iommu_init(void)
 			return -EPERM;
 		}
 	}
+
 	return 0;
 }
+
+static int __init omap_iommu_init(void)
+{
+	int err;
+
+	if (cpu_is_omap34xx())
+		err = omap_iommu_add(omap3_devices);
+	else if (cpu_is_omap44xx())
+		err = omap_iommu_add(omap4_devices);
+	else
+		return -ENODEV;
+
+	return err;
+}
 module_init(omap_iommu_init);
 
 static void __exit omap_iommu_exit(void)
diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index fd8ffeb..2205c3c 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -109,7 +109,7 @@ struct omap_mmu_dev_attr {
 };
 
 struct iommu_platform_data {
-	const char *name;
+	char *name;
 	struct omap_mmu_dev_attr *mmu_attr;
 
 	int (*device_enable)(struct platform_device *pdev);
-- 
1.7.1

Re: [PATCH 0/6] omap: iommu: hwmod support and code reorganization

[Ramirez Luna, Omar]

On Fri, Nov 5, 2010 at 8:19 PM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
> Boot tested on omap 3430 and 3630, functionality on iva2 only.

I meant functionally tested on iva2 only.

Regards,

Omar

Re: [PATCH 6/6] omap: iommu: code reorganization and cleanup

[Felipe Contreras]

On Sat, Nov 6, 2010 at 3:19 AM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
> Since omap-iommu is now using hwmod, there are structures and
> arrays that can be cleaned up to increase the readability of
> the code.
>
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>

NAK.

> ---
>  arch/arm/mach-omap2/omap-iommu.c        |   95 +++++++++++--------------------
>  arch/arm/plat-omap/include/plat/iommu.h |    2 +-
>  2 files changed, 34 insertions(+), 63 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index 0a76bce..135474b 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -17,53 +17,17 @@
>  #include <plat/omap_hwmod.h>
>  #include <plat/omap_device.h>
>
> -struct iommu_device {
> -       struct iommu_platform_data pdata;
> +static char *omap3_devices[] = {

Shouldn't this be __init?

> +       "isp",

#if defined(CONFIG_MPU_BRIDGE_IOMMU)

> +       "iva2",

#endif

> +       NULL,
>  };

If you want to get rid of CONFIG_MPU_BRIDGE_IOMMU, do it in a separate patch.

> -static int num_iommu_devices;
> -
> -#ifdef CONFIG_ARCH_OMAP3
> -static struct iommu_device omap3_devices[] = {
> -       {
> -               .pdata = {
> -                       .name = "isp",
> -               },
> -       },
> -#if defined(CONFIG_MPU_BRIDGE_IOMMU)
> -       {
> -               .pdata = {
> -                       .name = "iva2",
> -               },
> -       },
> -#endif
> -};
> -#define NR_OMAP3_IOMMU_DEVICES ARRAY_SIZE(omap3_devices)
> -#else
> -#define omap3_devices          NULL
> -#define NR_OMAP3_IOMMU_DEVICES 0
> -#endif
> -
> -#ifdef CONFIG_ARCH_OMAP4
> -static struct iommu_device omap4_devices[] = {
> -       {
> -               .pdata = {
> -                       .name = "ducati",
> -               },
> -       },
> -#if defined(CONFIG_MPU_TESLA_IOMMU)
> -       {
> -               .pdata = {
> -                       .name = "tesla",
> -               },
> -       },
> -#endif
> +
> +static char *omap4_devices[] = {
> +       "ducati",
> +       "tesla",
> +       NULL,
>  };
> -#define NR_OMAP4_IOMMU_DEVICES ARRAY_SIZE(omap4_devices)
> -#else
> -#define omap4_devices          NULL
> -#define NR_OMAP4_IOMMU_DEVICES 0
> -#endif
>
>  static struct omap_device_pm_latency iommu_latencies[] = {
>        [0] = {
> @@ -73,36 +37,28 @@ static struct omap_device_pm_latency iommu_latencies[] = {
>        },
>  };
>
> -static int __init omap_iommu_init(void)
> +static int __init omap_iommu_add(char **devices)
>  {
>        int i;
>
> -       if (cpu_is_omap34xx()) {
> -               devices = omap3_devices;
> -               num_iommu_devices = NR_OMAP3_IOMMU_DEVICES;
> -       } else if (cpu_is_omap44xx()) {
> -               devices = omap4_devices;
> -               num_iommu_devices = NR_OMAP4_IOMMU_DEVICES;
> -       } else
> -               return -ENODEV;
> -
> -       for (i = 0; i < num_iommu_devices; i++) {
> +       for (i = 0; devices[i]; i++) {
>                struct omap_hwmod *oh;
>                struct omap_device *od;
> +               struct iommu_platform_data pdata;
>
> -               oh = omap_hwmod_lookup(devices[i].pdata.name);
> +               oh = omap_hwmod_lookup(devices[i]);
>                if (!oh) {
>                        pr_err("%s: hwmod not found\n", __func__);
>                        return -ENODEV;
>                }
>
> -               devices[i].pdata.mmu_attr =
> -                               (struct omap_mmu_dev_attr *)oh->dev_attr;
> -               devices[i].pdata.device_enable = omap_device_enable;
> -               devices[i].pdata.device_disable = omap_device_idle;
> +               pdata.name = devices[i];
> +               pdata.mmu_attr = (struct omap_mmu_dev_attr *)oh->dev_attr;

No need for casting on a 'void *'.

> +               pdata.device_enable = omap_device_enable;
> +               pdata.device_disable = omap_device_idle;
>
>                od = omap_device_build("omap-iommu", i, oh,
> -                               &devices[i].pdata, sizeof(devices[i].pdata),
> +                               &pdata, sizeof(pdata),
>                                iommu_latencies, ARRAY_SIZE(iommu_latencies),
>                                0);
>                if (!od) {
> @@ -110,8 +66,23 @@ static int __init omap_iommu_init(void)
>                        return -EPERM;
>                }
>        }
> +
>        return 0;
>  }
> +
> +static int __init omap_iommu_init(void)
> +{
> +       int err;
> +
> +       if (cpu_is_omap34xx())
> +               err = omap_iommu_add(omap3_devices);
> +       else if (cpu_is_omap44xx())
> +               err = omap_iommu_add(omap4_devices);
> +       else
> +               return -ENODEV;
> +
> +       return err;
> +}

I don't see what's the point of having a separate omap_iommu_add. The
code of omap_iommu_add() can be moved to the end of omap_iommu_init()
very easily.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 0/6] omap: iommu: hwmod support and code reorganization

[Cousson, Benoit]

s/ducati/ipu/
s/tesla/dsp/

Please do not use internal codename for the changelog or even for the code.

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> Boot tested on omap 3430 and 3630, functionality on iva2 only.
>
> Introduced hwmod data and support for omap3 (iva2 mmu&  isp mmu) and
> omap4 (ducati mmu&  tesla mmu).
>
> Minor cleanup due to this changes.
>
> There is an issue (present without patches too):
>
> Doing a cycle of insmod-rmmod to iommu module and then full
> insmod of bridgedriver (with all dependencies) causes the next
> read of iommu registers to dump an unhandled fault log; this,
> because when rmmod of iommu is called it tries to clean the
> iommu tables and flush any old entry prior to removing the driver,
> however these reads/writes are attempted while the MMU is under
> reset and will timeout on the L3 IVA target agent, which will leave
> MMU unusable until the proper restore sequence to clear this L3
> error is executed.
>
> Fix would be to move page table allocation to iommu get and its
> correspondent free to iommu put, however it will fall entirely
> under iommu user responsibility, as it needs to clear the mmu
> reset bit, before calling iommu functions that read/write into
> mmu registers (which should apply only for first boot or on
> baseimage reload); trading this restriction in order to keep
> the same generic iommu code for all omap iommu devices.
>
> This issue has been seen on omap3 iva2 mmu, same restriction should

I'm still not really understanding that issue.
In theory, the reset is deasserted when the omap_device is enabled and 
will be re-asserted when the omap_device will be disable.
Why is the mmu already under reset when the iommu is trying to clean the 
tables?

Could please describe the complete sequence?

Benoit

> apply for tesla mmu.
>
> For discussion: should iommu handle mmu reset source directly?
> This register is grouped into an IVA PRM register which handles
> sequencer, iva2 mmu and dsp resets. As mentioned iommu handles
> cam, iva2 mmu (OMAP3) and tesla, ducati mmu (OMAP4), only tesla
> and iva2 should suffer from this restriction and according changes
> should be needed to handle both cases on mach-omap2 code. This
> also affects hwmod, since, if defined, it tries to read SYSC
> registers at boot time when registering the hwmod and causing the
> same issue.
>
> Omar Ramirez Luna (6):
>    omap: iommu: remove redundant clock usage
>    OMAP3: hwmod data: Add mmu for iva2 and isp
>    OMAP4: hwmod data: add mmu hwmod for ducati and tesla
>    omap: iommu: intial hwmod support
>    omap: iommu: hwmod device enable/disable routines
>    omap: iommu: code reorganization and cleanup
>
>   arch/arm/mach-omap2/omap-iommu.c           |  168 +++++++++-------------------
>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |  103 +++++++++++++++++
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  105 +++++++++++++++++
>   arch/arm/plat-omap/include/plat/iommu.h    |   19 +++-
>   arch/arm/plat-omap/include/plat/omap34xx.h |    2 +
>   arch/arm/plat-omap/iommu.c                 |   64 ++++-------
>   6 files changed, 299 insertions(+), 162 deletions(-)
>

Re: [PATCH 0/6] omap: iommu: hwmod support and code reorganization

[Cousson, Benoit]

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> Boot tested on omap 3430 and 3630, functionality on iva2 only.
>
> Introduced hwmod data and support for omap3 (iva2 mmu&  isp mmu) and
> omap4 (ducati mmu&  tesla mmu).
>
> Minor cleanup due to this changes.
>
> There is an issue (present without patches too):
>
> Doing a cycle of insmod-rmmod to iommu module and then full
> insmod of bridgedriver (with all dependencies) causes the next
> read of iommu registers to dump an unhandled fault log; this,
> because when rmmod of iommu is called it tries to clean the
> iommu tables and flush any old entry prior to removing the driver,
> however these reads/writes are attempted while the MMU is under
> reset and will timeout on the L3 IVA target agent, which will leave
> MMU unusable until the proper restore sequence to clear this L3
> error is executed.
>
> Fix would be to move page table allocation to iommu get and its
> correspondent free to iommu put, however it will fall entirely
> under iommu user responsibility, as it needs to clear the mmu
> reset bit, before calling iommu functions that read/write into
> mmu registers (which should apply only for first boot or on
> baseimage reload); trading this restriction in order to keep
> the same generic iommu code for all omap iommu devices.
>
> This issue has been seen on omap3 iva2 mmu, same restriction should
> apply for tesla mmu.
>
> For discussion: should iommu handle mmu reset source directly?
> This register is grouped into an IVA PRM register which handles
> sequencer, iva2 mmu and dsp resets. As mentioned iommu handles
> cam, iva2 mmu (OMAP3) and tesla, ducati mmu (OMAP4), only tesla
> and iva2 should suffer from this restriction and according changes
> should be needed to handle both cases on mach-omap2 code. This
> also affects hwmod, since, if defined, it tries to read SYSC
> registers at boot time when registering the hwmod and causing the
> same issue.
>
> Omar Ramirez Luna (6):
>    omap: iommu: remove redundant clock usage
>    OMAP3: hwmod data: Add mmu for iva2 and isp
>    OMAP4: hwmod data: add mmu hwmod for ducati and tesla
>    omap: iommu: intial hwmod support
>    omap: iommu: hwmod device enable/disable routines
>    omap: iommu: code reorganization and cleanup

Could you please try to use a consistent naming in the subjects?
OMAP3, OMAP4, OMAP2 --> OMAP and not omap.

Thanks,
Benoit

>
>   arch/arm/mach-omap2/omap-iommu.c           |  168 +++++++++-------------------
>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |  103 +++++++++++++++++
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  105 +++++++++++++++++
>   arch/arm/plat-omap/include/plat/iommu.h    |   19 +++-
>   arch/arm/plat-omap/include/plat/omap34xx.h |    2 +
>   arch/arm/plat-omap/iommu.c                 |   64 ++++-------
>   6 files changed, 299 insertions(+), 162 deletions(-)
>

Re: [PATCH 1/6] omap: iommu: remove redundant clock usage

[Cousson, Benoit]

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> iommu driver is meant to provide control of mmu hardware blocks

A dot is missing here, and a capital letter should follow.

> its current users (MMUs) are part of larger subsystems and do not
> have a dedicated clock as the one they use is shared with the
> entire subsystem, it doesn't make sense to enable/disable on each
> register read/write operation as the driver using its interface
> should also be handling the same clock.
>
> iommu should only enable/disable the clock on mmu request/free from
> the driver wanting to use it.

Mmm, I'm not necessarily convinced by that explanation.
If in a next revision, we change the clock partitioning and provide a 
dedicated clock for the mmu, it will not work anymore.
I don't thing you should assume anything about the current partitioning.

That being said, when you will change that code to switch to runtime PM, 
you will end up managing the module state in the ISR context.

Regards,
Benoit


>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/plat-omap/iommu.c |   38 +++++---------------------------------
>   1 files changed, 5 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index 6cd151b..de992c8 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -108,7 +108,6 @@ static int iommu_enable(struct iommu *obj)
>
>   	err = arch_iommu->enable(obj);
>
> -	clk_disable(obj->clk);
>   	return err;
>   }
>
> @@ -117,8 +116,6 @@ static void iommu_disable(struct iommu *obj)
>   	if (!obj)
>   		return;
>
> -	clk_enable(obj->clk);
> -
>   	arch_iommu->disable(obj);
>
>   	clk_disable(obj->clk);
> @@ -237,20 +234,16 @@ static struct cr_regs __iotlb_read_cr(struct iommu *obj, int n)
>    **/
>   int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
>   {
> -	int err = 0;
>   	struct iotlb_lock l;
>   	struct cr_regs *cr;
>
>   	if (!obj || !obj->nr_tlb_entries || !e)
>   		return -EINVAL;
>
> -	clk_enable(obj->clk);
> -
>   	iotlb_lock_get(obj,&l);
>   	if (l.base == obj->nr_tlb_entries) {
>   		dev_warn(obj->dev, "%s: preserve entries full\n", __func__);
> -		err = -EBUSY;
> -		goto out;
> +		return -EBUSY;
>   	}
>   	if (!e->prsvd) {
>   		int i;
> @@ -262,8 +255,7 @@ int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
>
>   		if (i == obj->nr_tlb_entries) {
>   			dev_dbg(obj->dev, "%s: full: no entry\n", __func__);
> -			err = -EBUSY;
> -			goto out;
> +			return -EBUSY;
>   		}
>
>   		iotlb_lock_get(obj,&l);
> @@ -273,10 +265,8 @@ int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
>   	}
>
>   	cr = iotlb_alloc_cr(obj, e);
> -	if (IS_ERR(cr)) {
> -		clk_disable(obj->clk);
> +	if (IS_ERR(cr))
>   		return PTR_ERR(cr);
> -	}
>
>   	iotlb_load_cr(obj, cr);
>   	kfree(cr);
> @@ -287,9 +277,8 @@ int load_iotlb_entry(struct iommu *obj, struct iotlb_entry *e)
>   	if (++l.vict == obj->nr_tlb_entries)
>   		l.vict = l.base;
>   	iotlb_lock_set(obj,&l);
> -out:
> -	clk_disable(obj->clk);
> -	return err;
> +
> +	return 0;
>   }
>   EXPORT_SYMBOL_GPL(load_iotlb_entry);
>
> @@ -305,8 +294,6 @@ void flush_iotlb_page(struct iommu *obj, u32 da)
>   	int i;
>   	struct cr_regs cr;
>
> -	clk_enable(obj->clk);
> -
>   	for_each_iotlb_cr(obj, obj->nr_tlb_entries, i, cr) {
>   		u32 start;
>   		size_t bytes;
> @@ -324,7 +311,6 @@ void flush_iotlb_page(struct iommu *obj, u32 da)
>   			iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
>   		}
>   	}
> -	clk_disable(obj->clk);
>
>   	if (i == obj->nr_tlb_entries)
>   		dev_dbg(obj->dev, "%s: no page for %08x\n", __func__, da);
> @@ -359,15 +345,11 @@ void flush_iotlb_all(struct iommu *obj)
>   {
>   	struct iotlb_lock l;
>
> -	clk_enable(obj->clk);
> -
>   	l.base = 0;
>   	l.vict = 0;
>   	iotlb_lock_set(obj,&l);
>
>   	iommu_write_reg(obj, 1, MMU_GFLUSH);
> -
> -	clk_disable(obj->clk);
>   }
>   EXPORT_SYMBOL_GPL(flush_iotlb_all);
>
> @@ -382,9 +364,7 @@ EXPORT_SYMBOL_GPL(flush_iotlb_all);
>    */
>   void iommu_set_twl(struct iommu *obj, bool on)
>   {
> -	clk_enable(obj->clk);
>   	arch_iommu->set_twl(obj, on);
> -	clk_disable(obj->clk);
>   }
>   EXPORT_SYMBOL_GPL(iommu_set_twl);
>
> @@ -395,12 +375,8 @@ ssize_t iommu_dump_ctx(struct iommu *obj, char *buf, ssize_t bytes)
>   	if (!obj || !buf)
>   		return -EINVAL;
>
> -	clk_enable(obj->clk);
> -
>   	bytes = arch_iommu->dump_ctx(obj, buf, bytes);
>
> -	clk_disable(obj->clk);
> -
>   	return bytes;
>   }
>   EXPORT_SYMBOL_GPL(iommu_dump_ctx);
> @@ -412,7 +388,6 @@ static int __dump_tlb_entries(struct iommu *obj, struct cr_regs *crs, int num)
>   	struct cr_regs tmp;
>   	struct cr_regs *p = crs;
>
> -	clk_enable(obj->clk);
>   	iotlb_lock_get(obj,&saved);
>
>   	for_each_iotlb_cr(obj, num, i, tmp) {
> @@ -422,7 +397,6 @@ static int __dump_tlb_entries(struct iommu *obj, struct cr_regs *crs, int num)
>   	}
>
>   	iotlb_lock_set(obj,&saved);
> -	clk_disable(obj->clk);
>
>   	return  p - crs;
>   }
> @@ -795,9 +769,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
>   	if (!err)
>   		return IRQ_HANDLED;
>
> -	clk_enable(obj->clk);
>   	stat = iommu_report_fault(obj,&da);
> -	clk_disable(obj->clk);
>   	if (!stat)
>   		return IRQ_HANDLED;
>

Re: [PATCH 2/6] OMAP3: hwmod data: Add mmu for iva2 and isp

[Cousson, Benoit]

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> Add mmu hwmod data for iva2 and isp.

s/iva2/iva/

If you do need the version information, you should use the rev field in 
the hwmod class.

All the comments about the structure fields order I did on your mailbox 
series will apply here as well.

Benoit

>
> Plus a define for the iva2 base register.
>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |  103 ++++++++++++++++++++++++++++
>   arch/arm/plat-omap/include/plat/iommu.h    |    8 ++
>   arch/arm/plat-omap/include/plat/omap34xx.h |    2 +
>   3 files changed, 113 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index 60d977e..ff80efc 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -21,6 +21,7 @@
>   #include<plat/l4_3xxx.h>
>   #include<plat/i2c.h>
>   #include<plat/omap34xx.h>
> +#include<plat/iommu.h>
>
>   #include "omap_hwmod_common_data.h"
>
> @@ -801,6 +802,106 @@ static struct omap_hwmod omap3xxx_mailbox_hwmod = {
>   	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>   };
>
> +/* mmu */
> +
> +static struct omap_hwmod_class omap3xxx_mmu_hwmod_class = {
> +	.name = "mmu",
> +};
> +
> +/* isp mmu */
> +
> +static struct omap_hwmod omap3xxx_isp_mmu_hwmod;
> +
> +static struct omap_hwmod_addr_space omap3xxx_isp_mmu_addrs[] = {
> +	{
> +		.pa_start	= OMAP3430_ISP_MMU_BASE,
> +		.pa_end		= OMAP3430_ISP_MMU_BASE + SZ_256 - 1,
> +		.flags		= ADDR_TYPE_RT,
> +	},
> +};
> +
> +/* l4_core ->  isp mmu */
> +static struct omap_hwmod_ocp_if omap3xxx_l4_core__isp_mmu = {
> +	.master		=&omap3xxx_l4_core_hwmod,
> +	.slave		=&omap3xxx_isp_mmu_hwmod,
> +	.addr		= omap3xxx_isp_mmu_addrs,
> +	.clk		= "cam_ick",
> +	.addr_cnt	= ARRAY_SIZE(omap3xxx_isp_mmu_addrs),
> +	.user		= OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> +/* isp mmu slave ports */
> +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = {
> +	&omap3xxx_l4_core__isp_mmu,
> +};
> +
> +static struct omap_hwmod_irq_info omap3xxx_isp_mmu_irqs[] = {
> +	{ .name = "isp", .irq = INT_24XX_CAM_IRQ, },
> +};
> +
> +static struct omap_mmu_dev_attr isp_mmu_dev_attr = {
> +	.nr_tlb_entries = 8,
> +};
> +
> +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = {
> +	.name		= "isp",
> +	.class		=&omap3xxx_mmu_hwmod_class,
> +	.mpu_irqs	= omap3xxx_isp_mmu_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap3xxx_isp_mmu_irqs),
> +	.slaves		= omap3xxx_isp_mmu_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
> +	.dev_attr	=&isp_mmu_dev_attr,
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.flags		= HWMOD_NO_IDLEST,
> +};
> +
> +/* iva2 mmu */
> +
> +static struct omap_hwmod omap3xxx_iva2_mmu_hwmod;
> +
> +static struct omap_hwmod_addr_space omap3xxx_iva2_mmu_addrs[] = {
> +	{
> +		.pa_start	= OMAP34XX_IVA2_MMU_BASE,
> +		.pa_end		= OMAP34XX_IVA2_MMU_BASE + SZ_256 - 1,
> +		.flags		= ADDR_TYPE_RT,
> +	},
> +};
> +
> +/* l3_main ->  iva2 mmu */
> +static struct omap_hwmod_ocp_if omap3xxx_l3_main__iva2_mmu = {
> +	.master		=&omap3xxx_l3_main_hwmod,
> +	.slave		=&omap3xxx_iva2_mmu_hwmod,
> +	.addr		= omap3xxx_iva2_mmu_addrs,
> +	.addr_cnt	= ARRAY_SIZE(omap3xxx_iva2_mmu_addrs),
> +	.user		= OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> +/* iva2 mmu slave ports */
> +static struct omap_hwmod_ocp_if *omap3xxx_iva2_mmu_slaves[] = {
> +	&omap3xxx_l3_main__iva2_mmu,
> +};
> +
> +static struct omap_hwmod_irq_info omap3xxx_iva2_mmu_irqs[] = {
> +	{ .name = "iva2", .irq = INT_24XX_DSP_MMU, },
> +};
> +
> +static struct omap_mmu_dev_attr iva2_mmu_dev_attr = {
> +	.nr_tlb_entries = 32,
> +};
> +
> +static struct omap_hwmod omap3xxx_iva2_mmu_hwmod = {
> +	.name		= "iva2",
> +	.class		=&omap3xxx_mmu_hwmod_class,
> +	.mpu_irqs	= omap3xxx_iva2_mmu_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap3xxx_iva2_mmu_irqs),
> +	.main_clk	= "iva2_ck",
> +	.slaves		= omap3xxx_iva2_mmu_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap3xxx_iva2_mmu_slaves),
> +	.dev_attr	=&iva2_mmu_dev_attr,
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.flags		= HWMOD_NO_IDLEST,
> +};
> +
>   static __initdata struct omap_hwmod *omap3xxx_hwmods[] = {
>   	&omap3xxx_l3_main_hwmod,
>   	&omap3xxx_l4_core_hwmod,
> @@ -817,6 +918,8 @@ static __initdata struct omap_hwmod *omap3xxx_hwmods[] = {
>   	&omap3xxx_i2c2_hwmod,
>   	&omap3xxx_i2c3_hwmod,
>   	&omap3xxx_mailbox_hwmod,
> +	&omap3xxx_isp_mmu_hwmod,
> +	&omap3xxx_iva2_mmu_hwmod,
>   	NULL,
>   };
>
> diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
> index 33c7d41..91a75a5 100644
> --- a/arch/arm/plat-omap/include/plat/iommu.h
> +++ b/arch/arm/plat-omap/include/plat/iommu.h
> @@ -99,6 +99,14 @@ struct iommu_functions {
>   	ssize_t (*dump_ctx)(struct iommu *obj, char *buf, ssize_t len);
>   };
>
> +/* omap_mmu_dev_attr - OMAP mmu device attributes for omap_hwmod
> + * @nr_tlb_entries: number of entries supported by the translation look-aside
> + *                   buffer (TLB).
> + */
> +struct omap_mmu_dev_attr {
> +	int nr_tlb_entries;
> +};
> +
>   struct iommu_platform_data {
>   	const char *name;
>   	const char *clk_name;
> diff --git a/arch/arm/plat-omap/include/plat/omap34xx.h b/arch/arm/plat-omap/include/plat/omap34xx.h
> index 98fc8b4..697f8f9 100644
> --- a/arch/arm/plat-omap/include/plat/omap34xx.h
> +++ b/arch/arm/plat-omap/include/plat/omap34xx.h
> @@ -82,6 +82,8 @@
>
>   #define OMAP34XX_MAILBOX_BASE		(L4_34XX_BASE + 0x94000)
>
> +#define OMAP34XX_IVA2_MMU_BASE	0x5D000000
> +
>   /* Security */
>   #define OMAP34XX_SEC_BASE	(L4_34XX_BASE + 0xA0000)
>   #define OMAP34XX_SEC_SHA1MD5_BASE	(OMAP34XX_SEC_BASE + 0x23000)

Re: [PATCH 3/6] OMAP4: hwmod data: add mmu hwmod for ducati and tesla

[Cousson, Benoit]

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> Add mmu hwmod data for ducati and tesla.

s/ducati/ipu/
s/tesla/dsp/

Please do not use internal codename.

Here again, you completely changed the omap4 existing data for (almost) 
nothing.

I agree, the original code was not considering the mmu as a hwmod but 
only the core of the subsystem: mmu + cache.

But as far as I can see, you just added a new mmu class, a dev_attr, and 
the hwmod remain almost the same.
Otherwise, you replaced the proper names by the bad one, and you removed 
important data (hw reset for ex).

Please start from the original code and fix what is missing or wrong but 
do not re-write everything.

>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  105 ++++++++++++++++++++++++++++
>   1 files changed, 105 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index f7525e3..1d5eace 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -22,6 +22,7 @@
>
>   #include<plat/omap_hwmod.h>
>   #include<plat/cpu.h>
> +#include<plat/iommu.h>
>
>   #include "omap_hwmod_common_data.h"
>
> @@ -1103,6 +1104,106 @@ static struct omap_hwmod omap44xx_mailbox_hwmod = {
>   	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
>   };
>
> +/* mmu */
> +
> +static struct omap_hwmod_class omap44xx_mmu_hwmod_class = {
> +	.name = "mmu",
> +};

That change is OK. The remaining part seems to be completely broken.

> +
> +/* ducati mmu */
> +
> +static struct omap_hwmod omap44xx_ducati_mmu_hwmod;
> +
> +static struct omap_hwmod_addr_space omap44xx_ducati_mmu_addrs[] = {
> +	{
> +		.pa_start	= OMAP4_MMU1_BASE,
> +		.pa_end		= OMAP4_MMU1_BASE + SZ_4K - 1,
> +		.flags		= ADDR_TYPE_RT,
> +	},
> +};
> +
> +/* l3_main_1 ->  ducati mmu */
> +static struct omap_hwmod_ocp_if omap44xx_l3_main_1__ducati_mmu = {
> +	.master		=&omap44xx_l3_main_1_hwmod,
> +	.slave		=&omap44xx_ducati_mmu_hwmod,
> +	.addr		= omap44xx_ducati_mmu_addrs,
> +	.clk		= "dpll_mpu_m2_ck",

Are you sure of that?

Benoit

> +	.addr_cnt	= ARRAY_SIZE(omap44xx_ducati_mmu_addrs),
> +	.user		= OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> +/* ducati mmu slave ports */
> +static struct omap_hwmod_ocp_if *omap44xx_ducati_mmu_slaves[] = {
> +	&omap44xx_l3_main_1__ducati_mmu,
> +};
> +
> +static struct omap_hwmod_irq_info omap44xx_ducati_mmu_irqs[] = {
> +	{ .name = "ducati", .irq = 100 + OMAP44XX_IRQ_GIC_START, },
> +};
> +
> +static struct omap_mmu_dev_attr ducati_mmu_dev_attr = {
> +	.nr_tlb_entries = 32,
> +};
> +
> +static struct omap_hwmod omap44xx_ducati_mmu_hwmod = {
> +	.name		= "ducati",
> +	.class		=&omap44xx_mmu_hwmod_class,
> +	.mpu_irqs	= omap44xx_ducati_mmu_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_ducati_mmu_irqs),
> +	.slaves		= omap44xx_ducati_mmu_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap44xx_ducati_mmu_slaves),
> +	.dev_attr	=&ducati_mmu_dev_attr,
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> +	.flags		= HWMOD_NO_IDLEST,
> +};
> +
> +/* tesla mmu */
> +
> +static struct omap_hwmod omap44xx_tesla_mmu_hwmod;
> +
> +static struct omap_hwmod_addr_space omap44xx_tesla_mmu_addrs[] = {
> +	{
> +		.pa_start	= OMAP4_MMU2_BASE,
> +		.pa_end		= OMAP4_MMU2_BASE + SZ_4K - 1,
> +		.flags		= ADDR_TYPE_RT,
> +	},
> +};
> +
> +/* l3_main_1 ->  tesla mmu */
> +static struct omap_hwmod_ocp_if omap44xx_l3_main_1__tesla_mmu = {
> +	.master		=&omap44xx_l3_main_1_hwmod,
> +	.slave		=&omap44xx_tesla_mmu_hwmod,
> +	.addr		= omap44xx_tesla_mmu_addrs,
> +	.addr_cnt	= ARRAY_SIZE(omap44xx_tesla_mmu_addrs),
> +	.user		= OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> +/* tesla mmu slave ports */
> +static struct omap_hwmod_ocp_if *omap44xx_tesla_mmu_slaves[] = {
> +	&omap44xx_l3_main_1__tesla_mmu,
> +};
> +
> +static struct omap_hwmod_irq_info omap44xx_tesla_mmu_irqs[] = {
> +	{ .name = "tesla", .irq = 28 + OMAP44XX_IRQ_GIC_START, },
> +};
> +
> +static struct omap_mmu_dev_attr tesla_mmu_dev_attr = {
> +	.nr_tlb_entries = 32,
> +};
> +
> +static struct omap_hwmod omap44xx_tesla_mmu_hwmod = {
> +	.name		= "tesla",
> +	.class		=&omap44xx_mmu_hwmod_class,
> +	.mpu_irqs	= omap44xx_tesla_mmu_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_tesla_mmu_irqs),
> +	.main_clk	= "dsp_fck",
> +	.slaves		= omap44xx_tesla_mmu_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap44xx_tesla_mmu_slaves),
> +	.dev_attr	=&tesla_mmu_dev_attr,
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> +	.flags		= HWMOD_NO_IDLEST,
> +};
> +
>   static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
>   	/* dmm class */
>   	&omap44xx_dmm_hwmod,
> @@ -1140,6 +1241,10 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
>
>   	/* mailbox */
>   	&omap44xx_mailbox_hwmod,
> +
> +	/* mmu */
> +	&omap44xx_ducati_mmu_hwmod,
> +	&omap44xx_tesla_mmu_hwmod,
>   	NULL,
>   };
>

Re: [PATCH 4/6] omap: iommu: intial hwmod support

[Cousson, Benoit]

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> Use the defined hwmod data according to the devices
> present on omap3 and omap4.
>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/omap-iommu.c        |   77 ++++++++-----------------------
>   arch/arm/plat-omap/include/plat/iommu.h |    2 +-
>   arch/arm/plat-omap/iommu.c              |    2 +-
>   3 files changed, 21 insertions(+), 60 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index f5a1aad..65460ef 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -14,12 +14,11 @@
>
>   #include<plat/iommu.h>
>   #include<plat/irqs.h>
> +#include<plat/omap_hwmod.h>
> +#include<plat/omap_device.h>
>
>   struct iommu_device {
> -	resource_size_t base;
> -	int irq;
>   	struct iommu_platform_data pdata;
> -	struct resource res[2];
>   };
>   static struct iommu_device *devices;
>   static int num_iommu_devices;
> @@ -27,128 +26,90 @@ static int num_iommu_devices;
>   #ifdef CONFIG_ARCH_OMAP3
>   static struct iommu_device omap3_devices[] = {

You should not need that at all, you are just duplicating the data that 
are already in hwmod, and then you are going to create real platform_device.

>   	{
> -		.base = 0x480bd400,
> -		.irq = 24,
>   		.pdata = {
>   			.name = "isp",
> -			.nr_tlb_entries = 8,
>   			.clk_name = "cam_ick",
>   		},
>   	},
>   #if defined(CONFIG_MPU_BRIDGE_IOMMU)
>   	{
> -		.base = 0x5d000000,
> -		.irq = 28,
>   		.pdata = {
>   			.name = "iva2",
> -			.nr_tlb_entries = 32,
>   			.clk_name = "iva2_ck",
>   		},
>   	},
>   #endif
>   };
>   #define NR_OMAP3_IOMMU_DEVICES ARRAY_SIZE(omap3_devices)
> -static struct platform_device *omap3_iommu_pdev[NR_OMAP3_IOMMU_DEVICES];
>   #else
>   #define omap3_devices		NULL
>   #define NR_OMAP3_IOMMU_DEVICES	0
> -#define omap3_iommu_pdev	NULL
>   #endif
>
>   #ifdef CONFIG_ARCH_OMAP4
>   static struct iommu_device omap4_devices[] = {
>   	{
> -		.base = OMAP4_MMU1_BASE,
> -		.irq = OMAP44XX_IRQ_DUCATI_MMU,
>   		.pdata = {
>   			.name = "ducati",
> -			.nr_tlb_entries = 32,
>   			.clk_name = "ducati_ick",
>   		},
>   	},
>   #if defined(CONFIG_MPU_TESLA_IOMMU)
>   	{
> -		.base = OMAP4_MMU2_BASE,
> -		.irq = INT_44XX_DSP_MMU,
>   		.pdata = {
>   			.name = "tesla",
> -			.nr_tlb_entries = 32,
>   			.clk_name = "tesla_ick",
>   		},
>   	},
>   #endif
>   };
>   #define NR_OMAP4_IOMMU_DEVICES ARRAY_SIZE(omap4_devices)
> -static struct platform_device *omap4_iommu_pdev[NR_OMAP4_IOMMU_DEVICES];
>   #else
>   #define omap4_devices		NULL
>   #define NR_OMAP4_IOMMU_DEVICES	0
> -#define omap4_iommu_pdev	NULL
>   #endif

Most of that previous code should not exist anymore with hwmod.
All the information you will need should be in hwmod data.

>
> -static struct platform_device **omap_iommu_pdev;
> -
>   static int __init omap_iommu_init(void)
>   {
> -	int i, err;
> -	struct resource res[] = {
> -		{ .flags = IORESOURCE_MEM },
> -		{ .flags = IORESOURCE_IRQ },
> -	};
> +	int i;
>
>   	if (cpu_is_omap34xx()) {
>   		devices = omap3_devices;
> -		omap_iommu_pdev = omap3_iommu_pdev;
>   		num_iommu_devices = NR_OMAP3_IOMMU_DEVICES;
>   	} else if (cpu_is_omap44xx()) {
>   		devices = omap4_devices;
> -		omap_iommu_pdev = omap4_iommu_pdev;
>   		num_iommu_devices = NR_OMAP4_IOMMU_DEVICES;
>   	} else
>   		return -ENODEV;
>
>   	for (i = 0; i<  num_iommu_devices; i++) {

Don't do that, just iterate over the hwmod class using 
omap_hwmod_for_each_by_class. Please have a look at the drivers with 
multiple devices that are already using hwmod (i2c, uart...).

> -		struct platform_device *pdev;
> -		const struct iommu_device *d =&devices[i];
> +		struct omap_hwmod *oh;
> +		struct omap_device *od;
>
> -		pdev = platform_device_alloc("omap-iommu", i);
> -		if (!pdev) {
> -			err = -ENOMEM;
> -			goto err_out;
> +		oh = omap_hwmod_lookup(devices[i].pdata.name);
> +		if (!oh) {
> +			pr_err("%s: hwmod not found\n", __func__);
> +			return -ENODEV;
>   		}
>
> -		res[0].start = d->base;
> -		res[0].end = d->base + MMU_REG_SIZE - 1;
> -		res[1].start = res[1].end = d->irq;
> +		devices[i].pdata.mmu_attr =
> +				(struct omap_mmu_dev_attr *)oh->dev_attr;

You need to create your pdata here, and attached it to the device.

>
> -		err = platform_device_add_resources(pdev, res,
> -						    ARRAY_SIZE(res));
> -		if (err)
> -			goto err_out;
> -		err = platform_device_add_data(pdev,&d->pdata,
> -					       sizeof(d->pdata));
> -		if (err)
> -			goto err_out;
> -		err = platform_device_add(pdev);
> -		if (err)
> -			goto err_out;
> -		omap_iommu_pdev[i] = pdev;
> +		od = omap_device_build("omap-iommu", i, oh,
> +				&devices[i].pdata, sizeof(devices[i].pdata),
> +				NULL, 0,
> +				0);
> +		if (!od) {
> +			pr_err("%s: error device build failed\n", __func__);
> +			return -EPERM;
> +		}
>   	}
>   	return 0;
> -
> -err_out:
> -	while (i--)
> -		platform_device_put(omap_iommu_pdev[i]);
> -	return err;
>   }
>   module_init(omap_iommu_init);
>
>   static void __exit omap_iommu_exit(void)
>   {
> -	int i;
> -
> -	for (i = 0; i<  num_iommu_devices; i++)
> -		platform_device_unregister(omap_iommu_pdev[i]);
>   }
>   module_exit(omap_iommu_exit);
>
> diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
> index 91a75a5..9650309 100644
> --- a/arch/arm/plat-omap/include/plat/iommu.h
> +++ b/arch/arm/plat-omap/include/plat/iommu.h
> @@ -110,7 +110,7 @@ struct omap_mmu_dev_attr {
>   struct iommu_platform_data {
>   	const char *name;
>   	const char *clk_name;
> -	const int nr_tlb_entries;
> +	struct omap_mmu_dev_attr *mmu_attr;

Except nr_tlb_entries, all the fields seems useless to me.

Regards,
Benoit

>   };
>
>   #if defined(CONFIG_ARCH_OMAP1)
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index de992c8..0fc9d90 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -890,7 +890,7 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
>   	if (IS_ERR(obj->clk))
>   		goto err_clk;
>
> -	obj->nr_tlb_entries = pdata->nr_tlb_entries;
> +	obj->nr_tlb_entries = pdata->mmu_attr->nr_tlb_entries;
>   	obj->name = pdata->name;
>   	obj->dev =&pdev->dev;
>   	obj->ctx = (void *)obj + sizeof(*obj);

Re: [PATCH 5/6] omap: iommu: hwmod device enable/disable routines

[Cousson, Benoit]

You should use runtime PM directly. That omap_device step is useless.

Moreover, this patch should be merged with the previous one.

Benoit

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> Use omap device enable/disable routines.
>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/omap-iommu.c        |   16 +++++++++++-----
>   arch/arm/plat-omap/include/plat/iommu.h |    7 +++++--
>   arch/arm/plat-omap/iommu.c              |   24 +++++++++++++++---------
>   3 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index 65460ef..0a76bce 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -28,14 +28,12 @@ static struct iommu_device omap3_devices[] = {
>   	{
>   		.pdata = {
>   			.name = "isp",
> -			.clk_name = "cam_ick",
>   		},
>   	},
>   #if defined(CONFIG_MPU_BRIDGE_IOMMU)
>   	{
>   		.pdata = {
>   			.name = "iva2",
> -			.clk_name = "iva2_ck",
>   		},
>   	},
>   #endif
> @@ -51,14 +49,12 @@ static struct iommu_device omap4_devices[] = {
>   	{
>   		.pdata = {
>   			.name = "ducati",
> -			.clk_name = "ducati_ick",
>   		},
>   	},
>   #if defined(CONFIG_MPU_TESLA_IOMMU)
>   	{
>   		.pdata = {
>   			.name = "tesla",
> -			.clk_name = "tesla_ick",
>   		},
>   	},
>   #endif
> @@ -69,6 +65,14 @@ static struct iommu_device omap4_devices[] = {
>   #define NR_OMAP4_IOMMU_DEVICES	0
>   #endif
>
> +static struct omap_device_pm_latency iommu_latencies[] = {
> +	[0] = {
> +		.activate_func = omap_device_enable_clocks,
> +		.deactivate_func = omap_device_enable_clocks,
> +		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST
> +	},
> +};
> +
>   static int __init omap_iommu_init(void)
>   {
>   	int i;
> @@ -94,10 +98,12 @@ static int __init omap_iommu_init(void)
>
>   		devices[i].pdata.mmu_attr =
>   				(struct omap_mmu_dev_attr *)oh->dev_attr;
> +		devices[i].pdata.device_enable = omap_device_enable;
> +		devices[i].pdata.device_disable = omap_device_idle;
>
>   		od = omap_device_build("omap-iommu", i, oh,
>   				&devices[i].pdata, sizeof(devices[i].pdata),
> -				NULL, 0,
> +				iommu_latencies, ARRAY_SIZE(iommu_latencies),
>   				0);
>   		if (!od) {
>   			pr_err("%s: error device build failed\n", __func__);
> diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
> index 9650309..fd8ffeb 100644
> --- a/arch/arm/plat-omap/include/plat/iommu.h
> +++ b/arch/arm/plat-omap/include/plat/iommu.h
> @@ -13,6 +13,8 @@
>   #ifndef __MACH_IOMMU_H
>   #define __MACH_IOMMU_H
>
> +struct platform_device;
> +
>   struct iotlb_entry {
>   	u32 da;
>   	u32 pa;
> @@ -28,7 +30,6 @@ struct iotlb_entry {
>   struct iommu {
>   	const char	*name;
>   	struct module	*owner;
> -	struct clk	*clk;
>   	void __iomem	*regbase;
>   	struct device	*dev;
>
> @@ -109,8 +110,10 @@ struct omap_mmu_dev_attr {
>
>   struct iommu_platform_data {
>   	const char *name;
> -	const char *clk_name;
>   	struct omap_mmu_dev_attr *mmu_attr;
> +
> +	int (*device_enable)(struct platform_device *pdev);
> +	int (*device_disable)(struct platform_device *pdev);
>   };
>
>   #if defined(CONFIG_ARCH_OMAP1)
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index 0fc9d90..36b1b63 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -100,11 +100,17 @@ EXPORT_SYMBOL_GPL(iommu_arch_version);
>   static int iommu_enable(struct iommu *obj)
>   {
>   	int err;
> +	struct iommu_platform_data *pdata;
> +	struct platform_device *pdev;
>
>   	if (!obj)
>   		return -EINVAL;
>
> -	clk_enable(obj->clk);
> +	pdev = container_of(obj->dev, struct platform_device, dev);
> +	pdata = obj->dev->platform_data;
> +
> +	if (pdata->device_enable)
> +		pdata->device_enable(pdev);
>
>   	err = arch_iommu->enable(obj);
>
> @@ -113,12 +119,19 @@ static int iommu_enable(struct iommu *obj)
>
>   static void iommu_disable(struct iommu *obj)
>   {
> +	struct iommu_platform_data *pdata;
> +	struct platform_device *pdev;
> +
>   	if (!obj)
>   		return;
>
>   	arch_iommu->disable(obj);
>
> -	clk_disable(obj->clk);
> +	pdev = container_of(obj->dev, struct platform_device, dev);
> +	pdata = obj->dev->platform_data;
> +
> +	if (pdata->device_enable)
> +		pdata->device_disable(pdev);
>   }
>
>   /*
> @@ -886,10 +899,6 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
>   	if (!obj)
>   		return -ENOMEM;
>
> -	obj->clk = clk_get(&pdev->dev, pdata->clk_name);
> -	if (IS_ERR(obj->clk))
> -		goto err_clk;
> -
>   	obj->nr_tlb_entries = pdata->mmu_attr->nr_tlb_entries;
>   	obj->name = pdata->name;
>   	obj->dev =&pdev->dev;
> @@ -949,8 +958,6 @@ err_irq:
>   	release_mem_region(res->start, resource_size(res));
>   	iounmap(obj->regbase);
>   err_mem:
> -	clk_put(obj->clk);
> -err_clk:
>   	kfree(obj);
>   	return err;
>   }
> @@ -972,7 +979,6 @@ static int __devexit omap_iommu_remove(struct platform_device *pdev)
>   	release_mem_region(res->start, resource_size(res));
>   	iounmap(obj->regbase);
>
> -	clk_put(obj->clk);
>   	dev_info(&pdev->dev, "%s removed\n", obj->name);
>   	kfree(obj);
>   	return 0;

Re: [PATCH 6/6] omap: iommu: code reorganization and cleanup

[Cousson, Benoit]

On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
> Since omap-iommu is now using hwmod, there are structures and
> arrays that can be cleaned up to increase the readability of
> the code.

This patch should be merged with the previous one as well.

I do not see the need to split in 3 patches these changes.
It will be much readable and will avoid people, like me, doing comment 
on a piece of code you will remove 2 patches later.
That cleanup must be done when the hwmod is introduced since that code 
was already useless at that time.

I can understand the phased approach when you have huge changes to do, 
but in that case, that does not worth it. It make the review even more 
painful.


> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/omap-iommu.c        |   95 +++++++++++--------------------
>   arch/arm/plat-omap/include/plat/iommu.h |    2 +-
>   2 files changed, 34 insertions(+), 63 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index 0a76bce..135474b 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -17,53 +17,17 @@
>   #include<plat/omap_hwmod.h>
>   #include<plat/omap_device.h>
>
> -struct iommu_device {
> -	struct iommu_platform_data pdata;
> +static char *omap3_devices[] = {
> +	"isp",
> +	"iva2",
> +	NULL,
>   };
> -static struct iommu_device *devices;
> -static int num_iommu_devices;
> -
> -#ifdef CONFIG_ARCH_OMAP3
> -static struct iommu_device omap3_devices[] = {
> -	{
> -		.pdata = {
> -			.name = "isp",
> -		},
> -	},
> -#if defined(CONFIG_MPU_BRIDGE_IOMMU)
> -	{
> -		.pdata = {
> -			.name = "iva2",
> -		},
> -	},
> -#endif
> -};
> -#define NR_OMAP3_IOMMU_DEVICES ARRAY_SIZE(omap3_devices)
> -#else
> -#define omap3_devices		NULL
> -#define NR_OMAP3_IOMMU_DEVICES	0
> -#endif
> -
> -#ifdef CONFIG_ARCH_OMAP4
> -static struct iommu_device omap4_devices[] = {
> -	{
> -		.pdata = {
> -			.name = "ducati",
> -		},
> -	},
> -#if defined(CONFIG_MPU_TESLA_IOMMU)
> -	{
> -		.pdata = {
> -			.name = "tesla",
> -		},
> -	},
> -#endif
> +
> +static char *omap4_devices[] = {
> +	"ducati",
> +	"tesla",
> +	NULL,

Not needed if you iterate over the class.

Benoit

Re: [PATCH 0/6] omap: iommu: hwmod support and code reorganization

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 1:31 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> s/ducati/ipu/
> s/tesla/dsp/
>
> Please do not use internal codename for the changelog or even for the code.

I picked this terminology from the driver, I didn't want to cause
confusion, agree to the change.

> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>
>> Boot tested on omap 3430 and 3630, functionality on iva2 only.
>>
>> Introduced hwmod data and support for omap3 (iva2 mmu&  isp mmu) and
>> omap4 (ducati mmu&  tesla mmu).
>>
>> Minor cleanup due to this changes.
>>
>> There is an issue (present without patches too):
>>
>> Doing a cycle of insmod-rmmod to iommu module and then full
>> insmod of bridgedriver (with all dependencies) causes the next
>> read of iommu registers to dump an unhandled fault log; this,
>> because when rmmod of iommu is called it tries to clean the
>> iommu tables and flush any old entry prior to removing the driver,
>> however these reads/writes are attempted while the MMU is under
>> reset and will timeout on the L3 IVA target agent, which will leave
>> MMU unusable until the proper restore sequence to clear this L3
>> error is executed.
>>
>> Fix would be to move page table allocation to iommu get and its
>> correspondent free to iommu put, however it will fall entirely
>> under iommu user responsibility, as it needs to clear the mmu
>> reset bit, before calling iommu functions that read/write into
>> mmu registers (which should apply only for first boot or on
>> baseimage reload); trading this restriction in order to keep
>> the same generic iommu code for all omap iommu devices.
>>
>> This issue has been seen on omap3 iva2 mmu, same restriction should
>
> I'm still not really understanding that issue.
> In theory, the reset is deasserted when the omap_device is enabled and will
> be re-asserted when the omap_device will be disable.

Probably I'm missing the bits telling hwmod to handle its reset bit, I
will recheck.

> Why is the mmu already under reset when the iommu is trying to clean the
> tables?

However, the driver disables the device on a call named iommu_put
(where, from your comment, reset should be re-asserted ), and cleans
the TLBs and page tables on removal of the driver (after iommu_put).

> Could please describe the complete sequence?

I'm assuming the dependencies are installed.

on boot up:
1. insmod iommu.ko
- device_enable is not called, unless a call to iommu_get is given,
this will mean that the driver has one user and thus device_enable is
called (however right now it doesn't matter if you call iommu_get or
not)

2. rmmod iommu.ko
- remove function will try to cleanup the TLBs writing into MMU
registers, since MMU is on reset it will trigger a timeout on its TA.

3. insmod bridgedriver.ko
- it is dependent on iommu.ko so it should have been installed before.
Whenever bridge calls iommu_get, the external abort will be caused due
to a read to MMU.

As I commented, cleaning the page tables could be moved from driver's
remove function to iommu_put, if omap device enable/disable is
handling the reset bit probably this issue shouldn't be hit.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] omap: iommu: remove redundant clock usage

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 2:11 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>
>> iommu driver is meant to provide control of mmu hardware blocks
>
> A dot is missing here, and a capital letter should follow.

Actually it is a comma, it is meant to be part of the same paragraph.

>
>> its current users (MMUs) are part of larger subsystems and do not
>> have a dedicated clock as the one they use is shared with the
>> entire subsystem, it doesn't make sense to enable/disable on each
>> register read/write operation as the driver using its interface
>> should also be handling the same clock.
>>
>> iommu should only enable/disable the clock on mmu request/free from
>> the driver wanting to use it.
>
> Mmm, I'm not necessarily convinced by that explanation.
> If in a next revision, we change the clock partitioning and provide a
> dedicated clock for the mmu, it will not work anymore.
> I don't thing you should assume anything about the current partitioning.

HW wise, only one clock feeds the mmu, iva2_ck is used to generate
three clocks but this inside the IVA2 module. I'm assuming omap4 dsp
is the same.

ISP also depends on cam_ick (among others), and mmu is inside ISP module afaik.

>From what I have checked on omap4 TRM it is the same, mmu doesn't have
a direct clock, it relies on the main ipu clock to function, but it is
my first glance to omap4 and I might be wrong.

> That being said, when you will change that code to switch to runtime PM, you
> will end up managing the module state in the ISR context.

Mainly this patch was sent on my laziness to add device enable/disable
calls on all the parts the code is using clk disable/enable, given
that they are not really needed.

Regards,

Omar

Re: [PATCH 2/6] OMAP3: hwmod data: Add mmu for iva2 and isp

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 2:15 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>
>> Add mmu hwmod data for iva2 and isp.
>
> s/iva2/iva/

Where this terminology can be found? I'm basing this naming on what
was there or TRM. For omap3 there is no such thing as iva, it is iva2.

I can change it but I just wanted to know.

> If you do need the version information, you should use the rev field in the
> hwmod class.

I don't.

> All the comments about the structure fields order I did on your mailbox
> series will apply here as well.

Same for my comments.

Regards,

Omar

Re: [PATCH 3/6] OMAP4: hwmod data: add mmu hwmod for ducati and tesla

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 3:47 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>
>> Add mmu hwmod data for ducati and tesla.
>
> s/ducati/ipu/
> s/tesla/dsp/
>
> Please do not use internal codename.

Tried to avoid confusion with what was originally in the driver, agree
with the change.

> Here again, you completely changed the omap4 existing data for (almost)
> nothing.
>
> I agree, the original code was not considering the mmu as a hwmod but only
> the core of the subsystem: mmu + cache.
>
> But as far as I can see, you just added a new mmu class, a dev_attr, and the
> hwmod remain almost the same.
> Otherwise, you replaced the proper names by the bad one, and you removed
> important data (hw reset for ex).
>
> Please start from the original code and fix what is missing or wrong but do
> not re-write everything.

I wrote this one from scratch, I didn't see that there were pieces to
handle some stuff since the code is buried in a private tree.

I cared to dug up the mailbox one, but completely missed this one.

>> +/* mmu */
>> +
>> +static struct omap_hwmod_class omap44xx_mmu_hwmod_class = {
>> +       .name = "mmu",
>> +};
>
> That change is OK. The remaining part seems to be completely broken.
>
>> +
>> +/* ducati mmu */
>> +
>> +static struct omap_hwmod omap44xx_ducati_mmu_hwmod;
>> +
>> +static struct omap_hwmod_addr_space omap44xx_ducati_mmu_addrs[] = {
>> +       {
>> +               .pa_start       = OMAP4_MMU1_BASE,
>> +               .pa_end         = OMAP4_MMU1_BASE + SZ_4K - 1,
>> +               .flags          = ADDR_TYPE_RT,
>> +       },
>> +};
>> +
>> +/* l3_main_1 ->  ducati mmu */
>> +static struct omap_hwmod_ocp_if omap44xx_l3_main_1__ducati_mmu = {
>> +       .master         =&omap44xx_l3_main_1_hwmod,
>> +       .slave          =&omap44xx_ducati_mmu_hwmod,
>> +       .addr           = omap44xx_ducati_mmu_addrs,
>> +       .clk            = "dpll_mpu_m2_ck",
>
> Are you sure of that?

No, this was supposed to be the hwmod main_clk... the ocp_if clk
should be l3_div.

I will add these changes and the ones you mention as "mmu + cache",
and see how it goes from there.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] omap: iommu: intial hwmod support

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 4:05 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>>  static struct iommu_device omap3_devices[] = {
>
> You should not need that at all, you are just duplicating the data that are
> already in hwmod, and then you are going to create real platform_device.

ok, will try that.

>>        for (i = 0; i<  num_iommu_devices; i++) {
>
> Don't do that, just iterate over the hwmod class using
> omap_hwmod_for_each_by_class. Please have a look at the drivers with
> multiple devices that are already using hwmod (i2c, uart...).

And this too.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] omap: iommu: hwmod device enable/disable routines

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 4:17 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> You should use runtime PM directly. That omap_device step is useless.
>
> Moreover, this patch should be merged with the previous one.

Agree, will check on runtime PM.

Regards,

Omar

Re: [PATCH 6/6] omap: iommu: code reorganization and cleanup

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 4:28 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>
>> Since omap-iommu is now using hwmod, there are structures and
>> arrays that can be cleaned up to increase the readability of
>> the code.
>
> This patch should be merged with the previous one as well.

No problem.

> I do not see the need to split in 3 patches these changes.
> It will be much readable and will avoid people, like me, doing comment on a
> piece of code you will remove 2 patches later.
> That cleanup must be done when the hwmod is introduced since that code was
> already useless at that time.

Yes, I was trying to avoid mixing changes so the review could be
focused on the replacements only, it seems it backfired.

Will do it in one patch and see how it looks.

>> +static char *omap4_devices[] = {
>> +       "ducati",
>> +       "tesla",
>> +       NULL,
>
> Not needed if you iterate over the class.

Ok.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] omap: iommu: code reorganization and cleanup

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 3:34 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Nov 6, 2010 at 3:19 AM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
>> Since omap-iommu is now using hwmod, there are structures and
>> arrays that can be cleaned up to increase the readability of
>> the code.
>>
>> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
>
> NAK.

Mostly agree, no need to discuss as this is going to change, will keep
CONFIG_MPU_BRIDGE_IOMMU.

Regards,

Omar

Re: [PATCH 0/6] omap: iommu: hwmod support and code reorganization

[Cousson, Benoit]

On 11/7/2010 10:43 AM, Ramirez Luna, Omar wrote:
> On Sat, Nov 6, 2010 at 1:31 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> s/ducati/ipu/
>> s/tesla/dsp/
>>
>> Please do not use internal codename for the changelog or even for the code.
>
> I picked this terminology from the driver, I didn't want to cause
> confusion, agree to the change.
>
>> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>>
>>> Boot tested on omap 3430 and 3630, functionality on iva2 only.
>>>
>>> Introduced hwmod data and support for omap3 (iva2 mmu&    isp mmu) and
>>> omap4 (ducati mmu&    tesla mmu).
>>>
>>> Minor cleanup due to this changes.
>>>
>>> There is an issue (present without patches too):
>>>
>>> Doing a cycle of insmod-rmmod to iommu module and then full
>>> insmod of bridgedriver (with all dependencies) causes the next
>>> read of iommu registers to dump an unhandled fault log; this,
>>> because when rmmod of iommu is called it tries to clean the
>>> iommu tables and flush any old entry prior to removing the driver,
>>> however these reads/writes are attempted while the MMU is under
>>> reset and will timeout on the L3 IVA target agent, which will leave
>>> MMU unusable until the proper restore sequence to clear this L3
>>> error is executed.
>>>
>>> Fix would be to move page table allocation to iommu get and its
>>> correspondent free to iommu put, however it will fall entirely
>>> under iommu user responsibility, as it needs to clear the mmu
>>> reset bit, before calling iommu functions that read/write into
>>> mmu registers (which should apply only for first boot or on
>>> baseimage reload); trading this restriction in order to keep
>>> the same generic iommu code for all omap iommu devices.
>>>
>>> This issue has been seen on omap3 iva2 mmu, same restriction should
>>
>> I'm still not really understanding that issue.
>> In theory, the reset is deasserted when the omap_device is enabled and will
>> be re-asserted when the omap_device will be disable.
>
> Probably I'm missing the bits telling hwmod to handle its reset bit, I
> will recheck.

After reading the other patches of that series, this is what I realized 
as well. You need the entries with hw reset information to make that works.

>
>> Why is the mmu already under reset when the iommu is trying to clean the
>> tables?
>
> However, the driver disables the device on a call named iommu_put
> (where, from your comment, reset should be re-asserted ), and cleans
> the TLBs and page tables on removal of the driver (after iommu_put).
>
>> Could please describe the complete sequence?
>
> I'm assuming the dependencies are installed.
>
> on boot up:
> 1. insmod iommu.ko
> - device_enable is not called, unless a call to iommu_get is given,
> this will mean that the driver has one user and thus device_enable is
> called (however right now it doesn't matter if you call iommu_get or
> not)
>
> 2. rmmod iommu.ko
> - remove function will try to cleanup the TLBs writing into MMU
> registers, since MMU is on reset it will trigger a timeout on its TA.
>
> 3. insmod bridgedriver.ko
> - it is dependent on iommu.ko so it should have been installed before.
> Whenever bridge calls iommu_get, the external abort will be caused due
> to a read to MMU.
>
> As I commented, cleaning the page tables could be moved from driver's
> remove function to iommu_put, if omap device enable/disable is
> handling the reset bit probably this issue shouldn't be hit.

Yep, I think so as well.

Regards,
Benoit

Re: [PATCH 2/6] OMAP3: hwmod data: Add mmu for iva2 and isp

[Cousson, Benoit]

On 11/7/2010 11:00 AM, Ramirez Luna, Omar wrote:
> On Sat, Nov 6, 2010 at 2:15 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>>
>>> Add mmu hwmod data for iva2 and isp.
>>
>> s/iva2/iva/
>
> Where this terminology can be found? I'm basing this naming on what
> was there or TRM. For omap3 there is no such thing as iva, it is iva2.

IVA2 means IVA v2, before we had an IVA (v1) then IVA v1.1...

The goal is to use the same name for the IP with similar functionality 
across OMAPs version in order to have generic code in the driver.

IVA, IVA2, IVAHD are all doing similar things, so the only way to 
identify the same functionality is by using the same name.
Thanks to the rev field, you can differentiate the various version 
during device creation.
That will allow you to query the hwmod on every version of OMAPs using 
the same "iva" name. Otherwise you will have to use the cpu_is_omap_XXX 
to select iva, iva2 or ivahd depending of the SoC.

Does that make sense to you?

Benoit

Re: [PATCH 3/6] OMAP4: hwmod data: add mmu hwmod for ducati and tesla

[Cousson, Benoit]

On 11/7/2010 11:18 AM, Ramirez Luna, Omar wrote:
> On Sat, Nov 6, 2010 at 3:47 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> On 11/5/2010 9:19 PM, Ramirez Luna, Omar wrote:
>>>
>>> Add mmu hwmod data for ducati and tesla.
>>
>> s/ducati/ipu/
>> s/tesla/dsp/
>>
>> Please do not use internal codename.
>
> Tried to avoid confusion with what was originally in the driver, agree
> with the change.
>
>> Here again, you completely changed the omap4 existing data for (almost)
>> nothing.
>>
>> I agree, the original code was not considering the mmu as a hwmod but only
>> the core of the subsystem: mmu + cache.
>>
>> But as far as I can see, you just added a new mmu class, a dev_attr, and the
>> hwmod remain almost the same.
>> Otherwise, you replaced the proper names by the bad one, and you removed
>> important data (hw reset for ex).
>>
>> Please start from the original code and fix what is missing or wrong but do
>> not re-write everything.
>
> I wrote this one from scratch, I didn't see that there were pieces to
> handle some stuff since the code is buried in a private tree.

Not true at all... It was sent to l-o:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg32854.html

And stored in a supposedly private tree, which appears to be public:
http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=summary

> I cared to dug up the mailbox one, but completely missed this one.
>
>>> +/* mmu */
>>> +
>>> +static struct omap_hwmod_class omap44xx_mmu_hwmod_class = {
>>> +       .name = "mmu",
>>> +};
>>
>> That change is OK. The remaining part seems to be completely broken.
>>
>>> +
>>> +/* ducati mmu */
>>> +
>>> +static struct omap_hwmod omap44xx_ducati_mmu_hwmod;
>>> +
>>> +static struct omap_hwmod_addr_space omap44xx_ducati_mmu_addrs[] = {
>>> +       {
>>> +               .pa_start       = OMAP4_MMU1_BASE,
>>> +               .pa_end         = OMAP4_MMU1_BASE + SZ_4K - 1,
>>> +               .flags          = ADDR_TYPE_RT,
>>> +       },
>>> +};
>>> +
>>> +/* l3_main_1 ->    ducati mmu */
>>> +static struct omap_hwmod_ocp_if omap44xx_l3_main_1__ducati_mmu = {
>>> +       .master         =&omap44xx_l3_main_1_hwmod,
>>> +       .slave          =&omap44xx_ducati_mmu_hwmod,
>>> +       .addr           = omap44xx_ducati_mmu_addrs,
>>> +       .clk            = "dpll_mpu_m2_ck",
>>
>> Are you sure of that?
>
> No, this was supposed to be the hwmod main_clk... the ocp_if clk
> should be l3_div.
>
> I will add these changes and the ones you mention as "mmu + cache",
> and see how it goes from there.

Please do not do any change on that code base, just use the original 
code and update it if needed.

Benoit

Re: [PATCH 3/6] OMAP4: hwmod data: add mmu hwmod for ducati and tesla

[Ramirez Luna, Omar]

On Mon, Nov 8, 2010 at 5:21 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>> I wrote this one from scratch, I didn't see that there were pieces to
>> handle some stuff since the code is buried in a private tree.
>
> Not true at all... It was sent to l-o:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg32854.html

I did review v3 of this patch set, care to point the exact place for
mmu hwmod on this patch set. It seems [v2 5/7] had mailbox, but I
don't think v3 made it to the list.

> And stored in a supposedly private tree, which appears to be public:
> http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=summary

I took whatever was in:

http://dev.omapzoom.org/?p=integration/kernel-omap4.git;a=shortlog;h=refs/heads/L24.11

> Please do not do any change on that code base, just use the original code
> and update it if needed.

Will do, as taken from the tree you mention.

Regards,

Omar

Re: [PATCH 2/6] OMAP3: hwmod data: Add mmu for iva2 and isp

[Ramirez Luna, Omar]

On Mon, Nov 8, 2010 at 5:05 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>>>> Add mmu hwmod data for iva2 and isp.
>>>
>>> s/iva2/iva/
>>
>> Where this terminology can be found? I'm basing this naming on what
>> was there or TRM. For omap3 there is no such thing as iva, it is iva2.
>
> IVA2 means IVA v2, before we had an IVA (v1) then IVA v1.1...
>
> The goal is to use the same name for the IP with similar functionality
> across OMAPs version in order to have generic code in the driver.
>
> IVA, IVA2, IVAHD are all doing similar things, so the only way to identify
> the same functionality is by using the same name.
> Thanks to the rev field, you can differentiate the various version during
> device creation.
> That will allow you to query the hwmod on every version of OMAPs using the
> same "iva" name. Otherwise you will have to use the cpu_is_omap_XXX to
> select iva, iva2 or ivahd depending of the SoC.
>
> Does that make sense to you?

That makes sense, thanks for the explanation.

Omar

Re: [PATCH 3/6] OMAP4: hwmod data: add mmu hwmod for ducati and tesla

[Cousson, Benoit]

On 11/9/2010 12:48 AM, Ramirez Luna, Omar wrote:
> On Mon, Nov 8, 2010 at 5:21 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>>> I wrote this one from scratch, I didn't see that there were pieces to
>>> handle some stuff since the code is buried in a private tree.
>>
>> Not true at all... It was sent to l-o:
>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg32854.html
>
> I did review v3 of this patch set, care to point the exact place for
> mmu hwmod on this patch set. It seems [v2 5/7] had mailbox, but I
> don't think v3 made it to the list.

The patch itself may did not reach the list due to his size, that's why 
I added the link to the GIT.

Both dsp and ipu contain the mmu + cache related information.

static struct omap_hwmod omap44xx_dsp_hwmod
static struct omap_hwmod omap44xx_ipu_hwmod

At that time I didn't know how that stuff was supposed to be used by the 
driver. I already changed the structure a little bit based on Hari's 
comment, so if you do need mmu entry only, since that structure seems to 
handle only that, it might makes sense to rename them in order to 
consider them part of the mmu class.
It should be then mmu_dsp and mmu_ipu, because in that case, it will 
represent the mmu class instances for the dsp and the ipu.

I'm perfectly fine to update that part if needed.

Regards,
Benoit

>> And stored in a supposedly private tree, which appears to be public:
>> http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=summary
>
> I took whatever was in:
>
> http://dev.omapzoom.org/?p=integration/kernel-omap4.git;a=shortlog;h=refs/heads/L24.11

>
>> Please do not do any change on that code base, just use the original code
>> and update it if needed.
>
> Will do, as taken from the tree you mention.
>
> Regards,
>
> Omar