[RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Felipe Contreras]

This patch series cleanups up a bit the opap3-iommu device registration and
then allows registration from other parts of the code.

Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
used as-is with the current omap iommu. By allowing devies to be registered
externaly (either isp, or iva2) this problem goes away.

Felipe Contreras (3):
  omap3-iommu: reorganize
  omap3-iommu: split init function into omap_iommu_add
  omap3-iommu: remote registration

 arch/arm/mach-omap2/omap3-iommu.c       |  130 ++++++++++++++-----------------
 arch/arm/plat-omap/include/mach/iommu.h |    2 +
 2 files changed, 62 insertions(+), 70 deletions(-)

[RFC/PATCH 1/3] omap3-iommu: reorganize

[Felipe Contreras]

From: Felipe Contreras <felipe.contreras@nokia.com>

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@nokia.com>
---
 arch/arm/mach-omap2/omap3-iommu.c |   72 ++++++++++++++++++------------------
 1 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index 367f36a..f4232ec 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -14,43 +14,31 @@
 
 #include <mach/iommu.h>
 
-#define OMAP3_MMU1_BASE	0x480bd400
-#define OMAP3_MMU2_BASE	0x5d000000
-#define OMAP3_MMU1_IRQ	24
-#define OMAP3_MMU2_IRQ	28
-
-static struct resource omap3_iommu_res[] = {
-	{ /* Camera ISP MMU */
-		.start		= OMAP3_MMU1_BASE,
-		.end		= OMAP3_MMU1_BASE + MMU_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= OMAP3_MMU1_IRQ,
-		.flags		= IORESOURCE_IRQ,
-	},
-	{ /* IVA2.2 MMU */
-		.start		= OMAP3_MMU2_BASE,
-		.end		= OMAP3_MMU2_BASE + MMU_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= OMAP3_MMU2_IRQ,
-		.flags		= IORESOURCE_IRQ,
-	},
+struct iommu_device {
+	resource_size_t base;
+	resource_size_t irq;
+	struct iommu_platform_data pdata;
+	struct resource res[2];
 };
-#define NR_IOMMU_RES (ARRAY_SIZE(omap3_iommu_res) / 2)
 
-static const struct iommu_platform_data omap3_iommu_pdata[] __initconst = {
+static struct iommu_device devices[] = {
 	{
-		.name = "isp",
-		.nr_tlb_entries = 8,
-		.clk_name = "cam_ick",
+		.base = 0x480bd400,
+		.irq = 24,
+		.pdata = {
+			.name = "isp",
+			.nr_tlb_entries = 8,
+			.clk_name = "cam_ick",
+		},
 	},
 	{
-		.name = "iva2",
-		.nr_tlb_entries = 32,
-		.clk_name = "iva2_ck",
+		.base = 0x5d000000,
+		.irq = 28,
+		.pdata = {
+			.name = "iva2",
+			.nr_tlb_entries = 32,
+			.clk_name = "iva2_ck",
+		},
 	},
 };
 #define NR_IOMMU_DEVICES ARRAY_SIZE(omap3_iommu_pdata)
@@ -63,23 +51,35 @@ static int __init omap3_iommu_init(void)
 
 	for (i = 0; i < NR_IOMMU_DEVICES; i++) {
 		struct platform_device *pdev;
+		const struct iommu_device *d = &devices[i];
+		struct resource res[] = {
+			{ .flags = IORESOURCE_MEM },
+			{ .flags = IORESOURCE_IRQ },
+		};
 
 		pdev = platform_device_alloc("omap-iommu", i + 1);
 		if (!pdev) {
 			err = -ENOMEM;
 			goto err_out;
 		}
-		err = platform_device_add_resources(pdev,
-				    &omap3_iommu_res[2 * i], NR_IOMMU_RES);
+
+		res[0].start = d->base;
+		res[0].end = d->base + MMU_REG_SIZE - 1;
+
+		res[1].start = d->irq;
+
+		err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
 		if (err)
 			goto err_out;
-		err = platform_device_add_data(pdev, &omap3_iommu_pdata[i],
-					       sizeof(omap3_iommu_pdata[0]));
+
+		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;
+
 		omap3_iommu_pdev[i] = pdev;
 	}
 	return 0;
-- 
1.6.3.1

[RFC/PATCH 2/3] omap3-iommu: split init function into omap_iommu_add

[Felipe Contreras]

In preparation for external registration.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mach-omap2/omap3-iommu.c |   76 ++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index f4232ec..149c624 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -45,43 +45,65 @@ static struct iommu_device devices[] = {
 
 static struct platform_device *omap3_iommu_pdev[NR_IOMMU_DEVICES];
 
-static int __init omap3_iommu_init(void)
+static struct platform_device *omap_iommu_add(const char *name)
 {
-	int i, err;
-
-	for (i = 0; i < NR_IOMMU_DEVICES; i++) {
-		struct platform_device *pdev;
-		const struct iommu_device *d = &devices[i];
-		struct resource res[] = {
-			{ .flags = IORESOURCE_MEM },
-			{ .flags = IORESOURCE_IRQ },
-		};
-
-		pdev = platform_device_alloc("omap-iommu", i + 1);
-		if (!pdev) {
-			err = -ENOMEM;
-			goto err_out;
+	struct platform_device *pdev;
+	const struct iommu_device *d = NULL;
+	struct resource res[] = {
+		{ .flags = IORESOURCE_MEM },
+		{ .flags = IORESOURCE_IRQ },
+	};
+	int err, i;
+
+	for (i = 0; i < ARRAY_SIZE(devices); i++)
+		if (strcmp(devices[i].pdata.name, name) == 0) {
+			d = &devices[i];
+			break;
 		}
 
-		res[0].start = d->base;
-		res[0].end = d->base + MMU_REG_SIZE - 1;
+	if (!d)
+		return NULL;
 
-		res[1].start = d->irq;
+	pdev = platform_device_alloc("omap-iommu", i + 1);
+	if (!pdev)
+		return NULL;
 
-		err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
-		if (err)
-			goto err_out;
+	res[0].start = d->base;
+	res[0].end = d->base + MMU_REG_SIZE - 1;
 
-		err = platform_device_add_data(pdev, &d->pdata, sizeof(d->pdata));
-		if (err)
-			goto err_out;
+	res[1].start = d->irq;
 
-		err = platform_device_add(pdev);
-		if (err)
-			goto err_out;
+	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;
+
+	return pdev;
 
+err_out:
+	platform_device_put(pdev);
+	return NULL;
+}
+
+static int __init omap3_iommu_init(void)
+{
+	struct platform_device *pdev;
+	int i, err;
+
+	for (i = 0; i < ARRAY_SIZE(devices); i++) {
+		pdev = omap_iommu_add(devices[i].pdata.name);
+		if (!pdev)
+			goto err_out;
 		omap3_iommu_pdev[i] = pdev;
 	}
+
 	return 0;
 
 err_out:
-- 
1.6.3.1

[RFC/PATCH 3/3] omap3-iommu: remote registration

[Felipe Contreras]

This allows devices to be registered only when they are used. The
current dsp-bridge driver for example is not using iommu so registering
the iommu iva2 device would conflict. By allowing remote registration
the dsp-bridge can decide when the iommu iva2 device is registered.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mach-omap2/omap3-iommu.c       |   36 +-----------------------------
 arch/arm/plat-omap/include/mach/iommu.h |    2 +
 2 files changed, 4 insertions(+), 34 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index 149c624..8380cd5 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -41,11 +41,8 @@ static struct iommu_device devices[] = {
 		},
 	},
 };
-#define NR_IOMMU_DEVICES ARRAY_SIZE(omap3_iommu_pdata)
 
-static struct platform_device *omap3_iommu_pdev[NR_IOMMU_DEVICES];
-
-static struct platform_device *omap_iommu_add(const char *name)
+struct platform_device *omap_iommu_add(const char *name)
 {
 	struct platform_device *pdev;
 	const struct iommu_device *d = NULL;
@@ -91,36 +88,7 @@ err_out:
 	platform_device_put(pdev);
 	return NULL;
 }
-
-static int __init omap3_iommu_init(void)
-{
-	struct platform_device *pdev;
-	int i, err;
-
-	for (i = 0; i < ARRAY_SIZE(devices); i++) {
-		pdev = omap_iommu_add(devices[i].pdata.name);
-		if (!pdev)
-			goto err_out;
-		omap3_iommu_pdev[i] = pdev;
-	}
-
-	return 0;
-
-err_out:
-	while (i--)
-		platform_device_put(omap3_iommu_pdev[i]);
-	return err;
-}
-module_init(omap3_iommu_init);
-
-static void __exit omap3_iommu_exit(void)
-{
-	int i;
-
-	for (i = 0; i < NR_IOMMU_DEVICES; i++)
-		platform_device_unregister(omap3_iommu_pdev[i]);
-}
-module_exit(omap3_iommu_exit);
+EXPORT_SYMBOL_GPL(omap_iommu_add);
 
 MODULE_AUTHOR("Hiroshi DOYU");
 MODULE_DESCRIPTION("omap iommu: omap3 device registration");
diff --git a/arch/arm/plat-omap/include/mach/iommu.h b/arch/arm/plat-omap/include/mach/iommu.h
index 769b00b..e22a4a4 100644
--- a/arch/arm/plat-omap/include/mach/iommu.h
+++ b/arch/arm/plat-omap/include/mach/iommu.h
@@ -165,4 +165,6 @@ extern int foreach_iommu_device(void *data,
 extern ssize_t iommu_dump_ctx(struct iommu *obj, char *buf);
 extern size_t dump_tlb_entries(struct iommu *obj, char *buf);
 
+struct platform_device *omap_iommu_add(const char *name);
+
 #endif /* __MACH_IOMMU_H */
-- 
1.6.3.1

Re: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Russell King - ARM Linux]

On Sat, May 16, 2009 at 01:05:50PM +0300, Felipe Contreras wrote:
> This allows devices to be registered only when they are used. The
> current dsp-bridge driver for example is not using iommu so registering
> the iommu iva2 device would conflict. By allowing remote registration
> the dsp-bridge can decide when the iommu iva2 device is registered.

I don't think that this is a good idea - what happens if two people
call omap_iommu_add() for the same IOMMU device independently?

The real problem here seems to be the TI DSP bridge code, and if that's
the case why can't we just avoid registering IVA2 if the TI DSP bridge
code is enabled.  That solves your stated problem without creating
additional management issues.

Re: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Felipe Contreras]

On Mon, May 18, 2009 at 3:11 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, May 16, 2009 at 01:05:50PM +0300, Felipe Contreras wrote:
>> This allows devices to be registered only when they are used. The
>> current dsp-bridge driver for example is not using iommu so registering
>> the iommu iva2 device would conflict. By allowing remote registration
>> the dsp-bridge can decide when the iommu iva2 device is registered.
>
> I don't think that this is a good idea - what happens if two people
> call omap_iommu_add() for the same IOMMU device independently?

Hmm, right, some extra checks would be needed to see if the device is
already registered and then increase some refcount. That's more
complicated that I was hoping for.

> The real problem here seems to be the TI DSP bridge code, and if that's
> the case why can't we just avoid registering IVA2 if the TI DSP bridge
> code is enabled.  That solves your stated problem without creating
> additional management issues.

The bridgedriver is expected to move and use iommu eventually, but not
right now, so I guess the iva2 device should be registered only if
MPU_BRIDGE_IOMMU is defined.

But then what's the point of having the isp iommu device if the camera
driver is disabled? Wouldn't that be wasting resources? Then if CAMERA
is not defined the isp device should not be registered either.

And finally if none of the two are enabled then you don't really
iommu. By having omap_iommu_add all the dependencies would be handled
automatically, right? 'modprobe bridgedriver' would load iommu.

-- 
Felipe Contreras
--
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: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Russell King - ARM Linux]

On Mon, May 18, 2009 at 03:46:07PM +0300, Felipe Contreras wrote:
> On Mon, May 18, 2009 at 3:11 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > The real problem here seems to be the TI DSP bridge code, and if that's
> > the case why can't we just avoid registering IVA2 if the TI DSP bridge
> > code is enabled.  That solves your stated problem without creating
> > additional management issues.
> 
> The bridgedriver is expected to move and use iommu eventually, but not
> right now, so I guess the iva2 device should be registered only if
> MPU_BRIDGE_IOMMU is defined.
> 
> But then what's the point of having the isp iommu device if the camera
> driver is disabled? Wouldn't that be wasting resources? Then if CAMERA
> is not defined the isp device should not be registered either.

So have something like:

config OMAP_IOMMU
	tristate

and then have both MPU_BRIDGE_IOMMU and the camera support (and whatever
else) select it.  That way, you only end up with the IOMMU support code
built into the kernel if you have users of it.

These low-level internal services drivers really don't need to be
publically visible in the configuration system.

As for the run-time size, that's truely minimal.

> And finally if none of the two are enabled then you don't really
> iommu. By having omap_iommu_add all the dependencies would be handled
> automatically, right? 'modprobe bridgedriver' would load iommu.

Think about it - the dependencies _already_ have to be there to use
the iommu services.
--
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: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Felipe Contreras]

On Mon, May 18, 2009 at 4:02 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, May 18, 2009 at 03:46:07PM +0300, Felipe Contreras wrote:
>> On Mon, May 18, 2009 at 3:11 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > The real problem here seems to be the TI DSP bridge code, and if that's
>> > the case why can't we just avoid registering IVA2 if the TI DSP bridge
>> > code is enabled.  That solves your stated problem without creating
>> > additional management issues.
>>
>> The bridgedriver is expected to move and use iommu eventually, but not
>> right now, so I guess the iva2 device should be registered only if
>> MPU_BRIDGE_IOMMU is defined.
>>
>> But then what's the point of having the isp iommu device if the camera
>> driver is disabled? Wouldn't that be wasting resources? Then if CAMERA
>> is not defined the isp device should not be registered either.
>
> So have something like:
>
> config OMAP_IOMMU
>        tristate
>
> and then have both MPU_BRIDGE_IOMMU and the camera support (and whatever
> else) select it.  That way, you only end up with the IOMMU support code
> built into the kernel if you have users of it.
>
> These low-level internal services drivers really don't need to be
> publically visible in the configuration system.

Yeap, that needs to be done too.

> As for the run-time size, that's truely minimal.

I thought creating iommu devices involved some kind of overhead,
allocating some resources probably. That's why the iva2 iommu device
conflicts with tidspbridge custom mmu.

>> And finally if none of the two are enabled then you don't really
>> iommu. By having omap_iommu_add all the dependencies would be handled
>> automatically, right? 'modprobe bridgedriver' would load iommu.
>
> Think about it - the dependencies _already_ have to be there to use
> the iommu services.

Ok, yes, for iommu, but not for omap3-iommu which is a separate module.

-- 
Felipe Contreras
--
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: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Russell King - ARM Linux]

On Mon, May 18, 2009 at 04:21:19PM +0300, Felipe Contreras wrote:
> On Mon, May 18, 2009 at 4:02 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, May 18, 2009 at 03:46:07PM +0300, Felipe Contreras wrote:
> >> On Mon, May 18, 2009 at 3:11 PM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > The real problem here seems to be the TI DSP bridge code, and if that's
> >> > the case why can't we just avoid registering IVA2 if the TI DSP bridge
> >> > code is enabled.  That solves your stated problem without creating
> >> > additional management issues.
> >>
> >> The bridgedriver is expected to move and use iommu eventually, but not
> >> right now, so I guess the iva2 device should be registered only if
> >> MPU_BRIDGE_IOMMU is defined.
> >>
> >> But then what's the point of having the isp iommu device if the camera
> >> driver is disabled? Wouldn't that be wasting resources? Then if CAMERA
> >> is not defined the isp device should not be registered either.
> >
> > So have something like:
> >
> > config OMAP_IOMMU
> >        tristate
> >
> > and then have both MPU_BRIDGE_IOMMU and the camera support (and whatever
> > else) select it.  That way, you only end up with the IOMMU support code
> > built into the kernel if you have users of it.
> >
> > These low-level internal services drivers really don't need to be
> > publically visible in the configuration system.
> 
> Yeap, that needs to be done too.
> 
> > As for the run-time size, that's truely minimal.
> 
> I thought creating iommu devices involved some kind of overhead,
> allocating some resources probably. That's why the iva2 iommu device
> conflicts with tidspbridge custom mmu.

I believe I've already said how to handle that - in fact it's in the
quoted messages at the top of this mail.

> >> And finally if none of the two are enabled then you don't really
> >> iommu. By having omap_iommu_add all the dependencies would be handled
> >> automatically, right? 'modprobe bridgedriver' would load iommu.
> >
> > Think about it - the dependencies _already_ have to be there to use
> > the iommu services.
> 
> Ok, yes, for iommu, but not for omap3-iommu which is a separate module.

That is a point, but I think it's a relatively minor one.  We could
get around that by ensuring that omap3-iommu is always built-in if
we have the possibility of iommu support, and leave iommu as a module.
--
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: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Felipe Contreras]

On Mon, May 18, 2009 at 4:40 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, May 18, 2009 at 04:21:19PM +0300, Felipe Contreras wrote:
>> On Mon, May 18, 2009 at 4:02 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, May 18, 2009 at 03:46:07PM +0300, Felipe Contreras wrote:
>> >> On Mon, May 18, 2009 at 3:11 PM, Russell King - ARM Linux
>> >> <linux@arm.linux.org.uk> wrote:
>> >> > The real problem here seems to be the TI DSP bridge code, and if that's
>> >> > the case why can't we just avoid registering IVA2 if the TI DSP bridge
>> >> > code is enabled.  That solves your stated problem without creating
>> >> > additional management issues.
>> >>
>> >> The bridgedriver is expected to move and use iommu eventually, but not
>> >> right now, so I guess the iva2 device should be registered only if
>> >> MPU_BRIDGE_IOMMU is defined.
>> >>
>> >> But then what's the point of having the isp iommu device if the camera
>> >> driver is disabled? Wouldn't that be wasting resources? Then if CAMERA
>> >> is not defined the isp device should not be registered either.
>> >
>> > So have something like:
>> >
>> > config OMAP_IOMMU
>> >        tristate
>> >
>> > and then have both MPU_BRIDGE_IOMMU and the camera support (and whatever
>> > else) select it.  That way, you only end up with the IOMMU support code
>> > built into the kernel if you have users of it.
>> >
>> > These low-level internal services drivers really don't need to be
>> > publically visible in the configuration system.
>>
>> Yeap, that needs to be done too.
>>
>> > As for the run-time size, that's truely minimal.
>>
>> I thought creating iommu devices involved some kind of overhead,
>> allocating some resources probably. That's why the iva2 iommu device
>> conflicts with tidspbridge custom mmu.
>
> I believe I've already said how to handle that - in fact it's in the
> quoted messages at the top of this mail.
>
>> >> And finally if none of the two are enabled then you don't really
>> >> iommu. By having omap_iommu_add all the dependencies would be handled
>> >> automatically, right? 'modprobe bridgedriver' would load iommu.
>> >
>> > Think about it - the dependencies _already_ have to be there to use
>> > the iommu services.
>>
>> Ok, yes, for iommu, but not for omap3-iommu which is a separate module.
>
> That is a point, but I think it's a relatively minor one.  We could
> get around that by ensuring that omap3-iommu is always built-in if
> we have the possibility of iommu support, and leave iommu as a module.

You mean omap3-iommu will be built-in in the iommu module?

That looks ok to me, *if* it's true that iommu devices don't waste any
significant resources if nobody is using them.

-- 
Felipe Contreras
--
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: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Hiroshi DOYU]

[-- Attachment #1: Type: Text/Plain, Size: 966 bytes --]

From: ext Russell King - ARM Linux <linux@arm.linux.org.uk>
Subject: Re: [RFC/PATCH 3/3] omap3-iommu: remote registration
Date: Mon, 18 May 2009 14:11:13 +0200

> On Sat, May 16, 2009 at 01:05:50PM +0300, Felipe Contreras wrote:
> > This allows devices to be registered only when they are used. The
> > current dsp-bridge driver for example is not using iommu so registering
> > the iommu iva2 device would conflict. By allowing remote registration
> > the dsp-bridge can decide when the iommu iva2 device is registered.
> 
> I don't think that this is a good idea - what happens if two people
> call omap_iommu_add() for the same IOMMU device independently?
> 
> The real problem here seems to be the TI DSP bridge code, and if that's
> the case why can't we just avoid registering IVA2 if the TI DSP bridge
> code is enabled.  That solves your stated problem without creating
> additional management issues.

How about the attached patch? I think this is enough.


[-- Attachment #2: 0001-omap-iommu-disable-iva2-iommu-device-registration-i.patch --]
[-- Type: Application/Octet-Stream, Size: 1052 bytes --]

From 0658efa31d68904998308a169247e87270110a30 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
Date: Mon, 18 May 2009 16:31:35 +0300
Subject: [PATCH 1/1] omap iommu: disable iva2 iommu device registration if tidspbridge enabled

tidspbridge will use omap iommu eventually. Until it's ready, disable it.

Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
---
 arch/arm/mach-omap2/omap3-iommu.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index 91ee38a..964990b 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -36,11 +36,13 @@ static const struct iommu_platform_data omap3_iommu_pdata[] __initconst = {
 		.nr_tlb_entries = 8,
 		.clk_name = "cam_ick",
 	},
+#if !defined(CONFIG_MPU_BRIDGE) && !defined(CONFIG_OMAP_DSP_MODULE)
 	{
 		.name = "iva2",
 		.nr_tlb_entries = 32,
 		.clk_name = "iva2_ck",
 	},
+#endif
 };
 #define NR_IOMMU_DEVICES ARRAY_SIZE(omap3_iommu_pdata)
 
-- 
1.6.0.4

Re: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Felipe Contreras]

On Mon, May 18, 2009 at 4:35 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Russell King - ARM Linux <linux@arm.linux.org.uk>
> Subject: Re: [RFC/PATCH 3/3] omap3-iommu: remote registration
> Date: Mon, 18 May 2009 14:11:13 +0200
>
>> On Sat, May 16, 2009 at 01:05:50PM +0300, Felipe Contreras wrote:
>> > This allows devices to be registered only when they are used. The
>> > current dsp-bridge driver for example is not using iommu so registering
>> > the iommu iva2 device would conflict. By allowing remote registration
>> > the dsp-bridge can decide when the iommu iva2 device is registered.
>>
>> I don't think that this is a good idea - what happens if two people
>> call omap_iommu_add() for the same IOMMU device independently?
>>
>> The real problem here seems to be the TI DSP bridge code, and if that's
>> the case why can't we just avoid registering IVA2 if the TI DSP bridge
>> code is enabled.  That solves your stated problem without creating
>> additional management issues.
>
> How about the attached patch? I think this is enough.

That wouldn't work when bridgedriver moves to iommu, how about:

+#if !defined(CONFIG_MPU_BRIDGE) && !defined(CONFIG_OMAP_DSP_MODULE)
+#if defined(CONFIG_MPU_BRIDGE_IOMMU)

-- 
Felipe Contreras
--
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: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [RFC/PATCH 3/3] omap3-iommu: remote registration
Date: Mon, 18 May 2009 15:52:59 +0200

> On Mon, May 18, 2009 at 4:35 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> > From: ext Russell King - ARM Linux <linux@arm.linux.org.uk>
> > Subject: Re: [RFC/PATCH 3/3] omap3-iommu: remote registration
> > Date: Mon, 18 May 2009 14:11:13 +0200
> >
> >> On Sat, May 16, 2009 at 01:05:50PM +0300, Felipe Contreras wrote:
> >> > This allows devices to be registered only when they are used. The
> >> > current dsp-bridge driver for example is not using iommu so registering
> >> > the iommu iva2 device would conflict. By allowing remote registration
> >> > the dsp-bridge can decide when the iommu iva2 device is registered.
> >>
> >> I don't think that this is a good idea - what happens if two people
> >> call omap_iommu_add() for the same IOMMU device independently?
> >>
> >> The real problem here seems to be the TI DSP bridge code, and if that's
> >> the case why can't we just avoid registering IVA2 if the TI DSP bridge
> >> code is enabled.  That solves your stated problem without creating
> >> additional management issues.
> >
> > How about the attached patch? I think this is enough.
> 
> That wouldn't work when bridgedriver moves to iommu, how about:
> 
> +#if !defined(CONFIG_MPU_BRIDGE) && !defined(CONFIG_OMAP_DSP_MODULE)
> +#if defined(CONFIG_MPU_BRIDGE_IOMMU)

Just now I read Russell's proposal. This would be better. Agreed.

How about Hari? I think that he's been working on this for a while....
--
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: [RFC/PATCH 3/3] omap3-iommu: remote registration

[Hiroshi DOYU]

[-- Attachment #1: Type: Text/Plain, Size: 1204 bytes --]

From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
Subject: Re: [RFC/PATCH 3/3] omap3-iommu: remote registration
Date: Mon, 18 May 2009 16:35:33 +0300 (EEST)

> From: ext Russell King - ARM Linux <linux@arm.linux.org.uk>
> Subject: Re: [RFC/PATCH 3/3] omap3-iommu: remote registration
> Date: Mon, 18 May 2009 14:11:13 +0200
> 
> > On Sat, May 16, 2009 at 01:05:50PM +0300, Felipe Contreras wrote:
> > > This allows devices to be registered only when they are used. The
> > > current dsp-bridge driver for example is not using iommu so registering
> > > the iommu iva2 device would conflict. By allowing remote registration
> > > the dsp-bridge can decide when the iommu iva2 device is registered.
> > 
> > I don't think that this is a good idea - what happens if two people
> > call omap_iommu_add() for the same IOMMU device independently?
> > 
> > The real problem here seems to be the TI DSP bridge code, and if that's
> > the case why can't we just avoid registering IVA2 if the TI DSP bridge
> > code is enabled.  That solves your stated problem without creating
> > additional management issues.
> 
> How about the attached patch? I think this is enough.

Oops, CONFIG_NAME was wrong. Attached new one.


[-- Attachment #2: 0001-omap-iommu-disable-iva2-iommu-device-registration-i.patch --]
[-- Type: Application/Octet-Stream, Size: 1054 bytes --]

From 37886d3d85d3b42b62edaa7fac22e001895b7617 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
Date: Mon, 18 May 2009 16:31:35 +0300
Subject: [PATCH 1/1] omap iommu: disable iva2 iommu device registration if tidspbridge enabled

tidspbridge will use omap iommu eventually. Until it's ready, disable it.

Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
---
 arch/arm/mach-omap2/omap3-iommu.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index 91ee38a..7fd8e4e 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -36,11 +36,13 @@ static const struct iommu_platform_data omap3_iommu_pdata[] __initconst = {
 		.nr_tlb_entries = 8,
 		.clk_name = "cam_ick",
 	},
+#if !defined(CONFIG_MPU_BRIDGE) && !defined(CONFIG_MPU_BRIDGE_MODULE)
 	{
 		.name = "iva2",
 		.nr_tlb_entries = 32,
 		.clk_name = "iva2_ck",
 	},
+#endif
 };
 #define NR_IOMMU_DEVICES ARRAY_SIZE(omap3_iommu_pdata)
 
-- 
1.6.0.4

Re: [RFC/PATCH 1/3] omap3-iommu: reorganize

[Russell King - ARM Linux]

On Sat, May 16, 2009 at 01:05:48PM +0300, Felipe Contreras wrote:
> +struct iommu_device {
> +	resource_size_t base;
> +	resource_size_t irq;
> +	struct iommu_platform_data pdata;
> +	struct resource res[2];
>  };

The data which is needed per device is:

- base address
- IRQ (no need for this to be resource_size_t - int will do)
- platform data

There's no need for that res[2] being there.

> @@ -63,23 +51,35 @@ static int __init omap3_iommu_init(void)
>  
>  	for (i = 0; i < NR_IOMMU_DEVICES; i++) {
>  		struct platform_device *pdev;
> +		const struct iommu_device *d = &devices[i];
> +		struct resource res[] = {
> +			{ .flags = IORESOURCE_MEM },
> +			{ .flags = IORESOURCE_IRQ },
> +		};

This initialization doesn't buy you anything, in fact quite the opposite.
The compiler actually interprets this as:

static struct resource __initial_res[] = {
	{ .flags = IORESOURCE_MEM },
	{ .flags = IORESOURCE_IRQ },
};
...
	for () {
		struct resource res[...];
		memcpy(res, __initial_res, sizeof(__initial_res));

> +
> +		res[0].start = d->base;
> +		res[0].end = d->base + MMU_REG_SIZE - 1;
> +
> +		res[1].start = d->irq;

It would be far better to initialize the flags element here for both.
And please also set res[1].end as I did in my patch.

> +
> +		err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
>  		if (err)
>  			goto err_out;
> -		err = platform_device_add_data(pdev, &omap3_iommu_pdata[i],
> -					       sizeof(omap3_iommu_pdata[0]));
> +
> +		err = platform_device_add_data(pdev, &d->pdata, sizeof(d->pdata));

This will fail checkpatch.

Re: [RFC/PATCH 1/3] omap3-iommu: reorganize

[Felipe Contreras]

On Mon, May 18, 2009 at 3:07 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, May 16, 2009 at 01:05:48PM +0300, Felipe Contreras wrote:
>> +struct iommu_device {
>> +     resource_size_t base;
>> +     resource_size_t irq;
>> +     struct iommu_platform_data pdata;
>> +     struct resource res[2];
>>  };
>
> The data which is needed per device is:
>
> - base address
> - IRQ (no need for this to be resource_size_t - int will do)
> - platform data
>
> There's no need for that res[2] being there.

Right, I was using 'res' but not any more; I forgot to remove it.

So resource_size_t is ok for 'base'?

>> @@ -63,23 +51,35 @@ static int __init omap3_iommu_init(void)
>>
>>       for (i = 0; i < NR_IOMMU_DEVICES; i++) {
>>               struct platform_device *pdev;
>> +             const struct iommu_device *d = &devices[i];
>> +             struct resource res[] = {
>> +                     { .flags = IORESOURCE_MEM },
>> +                     { .flags = IORESOURCE_IRQ },
>> +             };
>
> This initialization doesn't buy you anything, in fact quite the opposite.
> The compiler actually interprets this as:
>
> static struct resource __initial_res[] = {
>        { .flags = IORESOURCE_MEM },
>        { .flags = IORESOURCE_IRQ },
> };
> ...
>        for () {
>                struct resource res[...];
>                memcpy(res, __initial_res, sizeof(__initial_res));
>
>> +
>> +             res[0].start = d->base;
>> +             res[0].end = d->base + MMU_REG_SIZE - 1;
>> +
>> +             res[1].start = d->irq;
>
> It would be far better to initialize the flags element here for both.
> And please also set res[1].end as I did in my patch.

I think I saw that res initialization somewhere and I found it much
easier to read.

Ok. Will do.

>> +
>> +             err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
>>               if (err)
>>                       goto err_out;
>> -             err = platform_device_add_data(pdev, &omap3_iommu_pdata[i],
>> -                                            sizeof(omap3_iommu_pdata[0]));
>> +
>> +             err = platform_device_add_data(pdev, &d->pdata, sizeof(d->pdata));
>
> This will fail checkpatch.

I'll make sure it passes.

Cheers.

-- 
Felipe Contreras
--
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: [RFC/PATCH 1/3] omap3-iommu: reorganize

[Russell King - ARM Linux]

On Mon, May 18, 2009 at 03:59:42PM +0300, Felipe Contreras wrote:
> So resource_size_t is ok for 'base'?

Yes.

Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Russell King - ARM Linux]

On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
> This patch series cleanups up a bit the opap3-iommu device registration and
> then allows registration from other parts of the code.
> 
> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
> used as-is with the current omap iommu. By allowing devies to be registered
> externaly (either isp, or iva2) this problem goes away.

Hmm, so does this mean that the iommu patchset is going to grow by three
patches?  Hope not.

Also, please ensure that you address my other comments, some of which do
seem applicable to your cleanups as well - eg, the __initdata one.

Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Felipe Contreras]

On Sat, May 16, 2009 at 7:36 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
>> This patch series cleanups up a bit the opap3-iommu device registration and
>> then allows registration from other parts of the code.
>>
>> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
>> used as-is with the current omap iommu. By allowing devies to be registered
>> externaly (either isp, or iva2) this problem goes away.
>
> Hmm, so does this mean that the iommu patchset is going to grow by three
> patches?  Hope not.

I hope Hiroshi integrates my patches.

> Also, please ensure that you address my other comments, some of which do
> seem applicable to your cleanups as well - eg, the __initdata one.

Right, I can improve my cleanup patch with your comments, but my final
objective was to be able to register iommu devices independently, and
for that __initdata on the 'devices' array didn't work.

-- 
Felipe Contreras
--
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: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
Date: Sat, 16 May 2009 20:32:10 +0200

> On Sat, May 16, 2009 at 7:36 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
> >> This patch series cleanups up a bit the opap3-iommu device registration and
> >> then allows registration from other parts of the code.
> >>
> >> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
> >> used as-is with the current omap iommu. By allowing devies to be registered
> >> externaly (either isp, or iva2) this problem goes away.
> >
> > Hmm, so does this mean that the iommu patchset is going to grow by three
> > patches?  Hope not.

This series has been posted for a long time and I want to get this in
this time with minor fixes.

> I hope Hiroshi integrates my patches.

I think that the problem of yours is that it's not necesary to allow
device registration around kernel anywhere by "omap_iommu_add()" at
all, but enough only in "omap3-iommu.c". Since eventually
"tidspbridge" will use this iommu framework, no need for this
flexibility. Keeping same kind of device registration in one place is
good thing from SoC perspective. So I want to avoid adding unnecessary
flexibility to the code. I'll follow Russell's call, anyway.

> > Also, please ensure that you address my other comments, some of which do
> > seem applicable to your cleanups as well - eg, the __initdata one.
> 
> Right, I can improve my cleanup patch with your comments, but my final
> objective was to be able to register iommu devices independently, and
> for that __initdata on the 'devices' array didn't work.
--
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: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Hiroshi DOYU]

From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
Date: Mon, 18 May 2009 08:16:27 +0300 (EEST)

> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
> Date: Sat, 16 May 2009 20:32:10 +0200
> 
> > On Sat, May 16, 2009 at 7:36 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
> > >> This patch series cleanups up a bit the opap3-iommu device registration and
> > >> then allows registration from other parts of the code.
> > >>
> > >> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
> > >> used as-is with the current omap iommu. By allowing devies to be registered
> > >> externaly (either isp, or iva2) this problem goes away.
> > >
> > > Hmm, so does this mean that the iommu patchset is going to grow by three
> > > patches?  Hope not.
> 
> This series has been posted for a long time and I want to get this in
> this time with minor fixes.
> 
> > I hope Hiroshi integrates my patches.
> 
> I think that the problem of yours is that it's not necesary to allow
> device registration around kernel anywhere by "omap_iommu_add()" at
> all, but enough only in "omap3-iommu.c". Since eventually
> "tidspbridge" will use this iommu framework, no need for this
> flexibility. Keeping same kind of device registration in one place is
> good thing from SoC perspective. So I want to avoid adding unnecessary
> flexibility to the code. I'll follow Russell's call, anyway.

The other thing is about introducing a new structure type, "struct
iommu_device". I don't want to define new structure type just for this
device registration. Since device registration part is kind of
conventional one in kernel, I want to keep things simple/conventional
as much as possible.

> 
> > > Also, please ensure that you address my other comments, some of which do
> > > seem applicable to your cleanups as well - eg, the __initdata one.
> > 
> > Right, I can improve my cleanup patch with your comments, but my final
> > objective was to be able to register iommu devices independently, and
> > for that __initdata on the 'devices' array didn't work.
--
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: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Felipe Contreras]

On Mon, May 18, 2009 at 8:33 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
> Date: Mon, 18 May 2009 08:16:27 +0300 (EEST)
>
>> From: ext Felipe Contreras <felipe.contreras@gmail.com>
>> Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
>> Date: Sat, 16 May 2009 20:32:10 +0200
>>
>> > On Sat, May 16, 2009 at 7:36 PM, Russell King - ARM Linux
>> > <linux@arm.linux.org.uk> wrote:
>> > > On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
>> > >> This patch series cleanups up a bit the opap3-iommu device registration and
>> > >> then allows registration from other parts of the code.
>> > >>
>> > >> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
>> > >> used as-is with the current omap iommu. By allowing devies to be registered
>> > >> externaly (either isp, or iva2) this problem goes away.
>> > >
>> > > Hmm, so does this mean that the iommu patchset is going to grow by three
>> > > patches?  Hope not.
>>
>> This series has been posted for a long time and I want to get this in
>> this time with minor fixes.
>>
>> > I hope Hiroshi integrates my patches.
>>
>> I think that the problem of yours is that it's not necesary to allow
>> device registration around kernel anywhere by "omap_iommu_add()" at
>> all, but enough only in "omap3-iommu.c". Since eventually
>> "tidspbridge" will use this iommu framework, no need for this
>> flexibility. Keeping same kind of device registration in one place is
>> good thing from SoC perspective. So I want to avoid adding unnecessary
>> flexibility to the code. I'll follow Russell's call, anyway.
>
> The other thing is about introducing a new structure type, "struct
> iommu_device". I don't want to define new structure type just for this
> device registration. Since device registration part is kind of
> conventional one in kernel, I want to keep things simple/conventional
> as much as possible.

It is an *internal* structure, just like you local 'omap3_iommu_pdata'
nobody has not know about it outside omap3-iommu.c.

The exported function is:
struct platform_device *omap_iommu_add(const char *name);

Nothing custom there.

-- 
Felipe Contreras
--
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: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Felipe Contreras]

On Mon, May 18, 2009 at 8:16 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
> Date: Sat, 16 May 2009 20:32:10 +0200
>
>> On Sat, May 16, 2009 at 7:36 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
>> >> This patch series cleanups up a bit the opap3-iommu device registration and
>> >> then allows registration from other parts of the code.
>> >>
>> >> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
>> >> used as-is with the current omap iommu. By allowing devies to be registered
>> >> externaly (either isp, or iva2) this problem goes away.
>> >
>> > Hmm, so does this mean that the iommu patchset is going to grow by three
>> > patches?  Hope not.
>
> This series has been posted for a long time and I want to get this in
> this time with minor fixes.
>
>> I hope Hiroshi integrates my patches.
>
> I think that the problem of yours is that it's not necesary to allow
> device registration around kernel anywhere by "omap_iommu_add()" at
> all, but enough only in "omap3-iommu.c". Since eventually
> "tidspbridge" will use this iommu framework, no need for this
> flexibility. Keeping same kind of device registration in one place is
> good thing from SoC perspective. So I want to avoid adding unnecessary
> flexibility to the code. I'll follow Russell's call, anyway.

The first patch has nothing to do with omap_iommu_add, it's just
cleanups, and you haven't provided any valid reason to reject them.

The rest of the patches are *necessary* as of right now. IMHO iommu
should not be merged in it's current state because it will break
tidspbridge. Maybe after tidspbridge has been fully converted to use
iommu.

I think there will probably be a conversion period where the
tidspbridge will use iommu optionally, in that period iommu would need
to be patched and add a #ifdef MPU_BRIDGE_IOMMU, that would be ugly.

Also you haven't answered my questions: what happens if you disable
MPU_BRIDGE? What's the point of iommu to register the iva2 iommu
device?

My patches solves all of those issues.

-- 
Felipe Contreras
--
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: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
Date: Mon, 18 May 2009 13:48:09 +0200

> On Mon, May 18, 2009 at 8:16 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> > From: ext Felipe Contreras <felipe.contreras@gmail.com>
> > Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
> > Date: Sat, 16 May 2009 20:32:10 +0200
> >
> >> On Sat, May 16, 2009 at 7:36 PM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
> >> >> This patch series cleanups up a bit the opap3-iommu device registration and
> >> >> then allows registration from other parts of the code.
> >> >>
> >> >> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
> >> >> used as-is with the current omap iommu. By allowing devies to be registered
> >> >> externaly (either isp, or iva2) this problem goes away.
> >> >
> >> > Hmm, so does this mean that the iommu patchset is going to grow by three
> >> > patches?  Hope not.
> >
> > This series has been posted for a long time and I want to get this in
> > this time with minor fixes.
> >
> >> I hope Hiroshi integrates my patches.
> >
> > I think that the problem of yours is that it's not necesary to allow
> > device registration around kernel anywhere by "omap_iommu_add()" at
> > all, but enough only in "omap3-iommu.c". Since eventually
> > "tidspbridge" will use this iommu framework, no need for this
> > flexibility. Keeping same kind of device registration in one place is
> > good thing from SoC perspective. So I want to avoid adding unnecessary
> > flexibility to the code. I'll follow Russell's call, anyway.
> 
> The first patch has nothing to do with omap_iommu_add, it's just
> cleanups, and you haven't provided any valid reason to reject them.

Your code doesn't work since the other members of "struct resource"
haven't been initilialized like:

+		memset(res, 0,  sizeof(res));

Please refer to:

http://marc.info/?l=linux-omap&m=124263966231599&w=2


> The rest of the patches are *necessary* as of right now. IMHO iommu
> should not be merged in it's current state because it will break
> tidspbridge. Maybe after tidspbridge has been fully converted to use
> iommu.
> 
> I think there will probably be a conversion period where the
> tidspbridge will use iommu optionally, in that period iommu would need
> to be patched and add a #ifdef MPU_BRIDGE_IOMMU, that would be ugly.

How about:

http://lists.arm.linux.org.uk/lurker/attach/3@20090518.133533.487a3dd4.attach

> 
> Also you haven't answered my questions: what happens if you disable
> MPU_BRIDGE? What's the point of iommu to register the iva2 iommu
> device?

enough small to ignore.

> 
> My patches solves all of those issues.
> 
> -- 
> Felipe Contreras
--
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: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Felipe Contreras]

On Mon, May 18, 2009 at 4:48 PM, Hiroshi DOYU<Hiroshi.DOYU@nokia.com> wrote:
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
> Date: Mon, 18 May 2009 13:48:09 +0200
>
>> On Mon, May 18, 2009 at 8:16 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> > From: ext Felipe Contreras <felipe.contreras@gmail.com>
>> > Subject: Re: [RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration
>> > Date: Sat, 16 May 2009 20:32:10 +0200
>> >
>> >> On Sat, May 16, 2009 at 7:36 PM, Russell King - ARM Linux
>> >> <linux@arm.linux.org.uk> wrote:
>> >> > On Sat, May 16, 2009 at 01:05:47PM +0300, Felipe Contreras wrote:
>> >> >> This patch series cleanups up a bit the opap3-iommu device registration and
>> >> >> then allows registration from other parts of the code.
>> >> >>
>> >> >> Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
>> >> >> used as-is with the current omap iommu. By allowing devies to be registered
>> >> >> externaly (either isp, or iva2) this problem goes away.
>> >> >
>> >> > Hmm, so does this mean that the iommu patchset is going to grow by three
>> >> > patches?  Hope not.
>> >
>> > This series has been posted for a long time and I want to get this in
>> > this time with minor fixes.
>> >
>> >> I hope Hiroshi integrates my patches.
>> >
>> > I think that the problem of yours is that it's not necesary to allow
>> > device registration around kernel anywhere by "omap_iommu_add()" at
>> > all, but enough only in "omap3-iommu.c". Since eventually
>> > "tidspbridge" will use this iommu framework, no need for this
>> > flexibility. Keeping same kind of device registration in one place is
>> > good thing from SoC perspective. So I want to avoid adding unnecessary
>> > flexibility to the code. I'll follow Russell's call, anyway.
>>
>> The first patch has nothing to do with omap_iommu_add, it's just
>> cleanups, and you haven't provided any valid reason to reject them.
>
> Your code doesn't work since the other members of "struct resource"
> haven't been initilialized like:
>
> +               memset(res, 0,  sizeof(res));

For the record, that's not true:

+               struct resource res[] = {
+                       { .flags = IORESOURCE_MEM },
+                       { .flags = IORESOURCE_IRQ },
+               };

Is perfectly fine because according to the C standard in section 6.7.8 #19:

The initialization shall occur in initializer list order, each
initializer provided for a
particular subobject overriding any previously listed initializer for
the same subobject;130)
all subobjects that are not initialized explicitly shall be
initialized implicitly the same as
objects that have static storage duration.

So the rest of the fields are initialized to 0.

Cheers.

-- 
Felipe Contreras
--
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 3/6] omap iommu: omap3 iommu device registration

[Felipe Contreras]

On Wed, May 6, 2009 at 9:00 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> Subject: Re: [PATCH 3/6] omap iommu: omap3 iommu device registration
> Date: Tue, 5 May 2009 21:32:34 +0200
>
>> On Tue, May 5, 2009 at 3:47 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> > Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
>> > ---
>> >
>> >  arch/arm/mach-omap2/omap3-iommu.c |  105 +++++++++++++++++++++++++++++++++++++
>> >  1 files changed, 105 insertions(+), 0 deletions(-)
>> >  create mode 100644 arch/arm/mach-omap2/omap3-iommu.c
>> >
>> > diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
>> > new file mode 100644
>> > index 0000000..367f36a
>> > --- /dev/null
>> > +++ b/arch/arm/mach-omap2/omap3-iommu.c
>> > @@ -0,0 +1,105 @@
>> > +/*
>> > + * omap iommu: omap3 device registration
>> > + *
>> > + * Copyright (C) 2008-2009 Nokia Corporation
>> > + *
>> > + * Written by Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + */
>> > +
>> > +#include <linux/platform_device.h>
>> > +
>> > +#include <mach/iommu.h>
>> > +
>> > +#define OMAP3_MMU1_BASE        0x480bd400
>> > +#define OMAP3_MMU2_BASE        0x5d000000
>> > +#define OMAP3_MMU1_IRQ 24
>> > +#define OMAP3_MMU2_IRQ 28
>> > +
>> > +static struct resource omap3_iommu_res[] = {
>> > +       { /* Camera ISP MMU */
>> > +               .start          = OMAP3_MMU1_BASE,
>> > +               .end            = OMAP3_MMU1_BASE + MMU_REG_SIZE - 1,
>> > +               .flags          = IORESOURCE_MEM,
>> > +       },
>> > +       {
>> > +               .start          = OMAP3_MMU1_IRQ,
>> > +               .flags          = IORESOURCE_IRQ,
>> > +       },
>> > +       { /* IVA2.2 MMU */
>> > +               .start          = OMAP3_MMU2_BASE,
>> > +               .end            = OMAP3_MMU2_BASE + MMU_REG_SIZE - 1,
>> > +               .flags          = IORESOURCE_MEM,
>> > +       },
>> > +       {
>> > +               .start          = OMAP3_MMU2_IRQ,
>> > +               .flags          = IORESOURCE_IRQ,
>> > +       },
>> > +};
>>
>> This will break TI's bridgedriver, right?
>
> Working around is easy, but let's go forward, correcting TI's bridgedriver;-)
>
> http://marc.info/?l=linux-omap&m=124148814309478&w=2

*sigh*

I can't recall how many times I've asked for this, I'm sending a patch
series (RFC) that allows exactly what I want.

-- 
Felipe Contreras
--
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

[RFC/PATCH 0/3] omap3-iommu: cleanups and remote registration

[Felipe Contreras]

This patch series cleanups up a bit the opap3-iommu device registration and
then allows registration from other parts of the code.

Currently the iva2 code (tidspbridge) is not using iommu, therefore it can't be
used as-is with the current omap iommu. By allowing devies to be registered
externaly (either isp, or iva2) this problem goes away.

Felipe Contreras (3):
  omap3-iommu: reorganize
  omap3-iommu: split init function into omap_iommu_add
  omap3-iommu: remote registration

 arch/arm/mach-omap2/omap3-iommu.c       |  130 ++++++++++++++-----------------
 arch/arm/plat-omap/include/mach/iommu.h |    2 +
 2 files changed, 62 insertions(+), 70 deletions(-)

[RFC/PATCH 1/3] omap3-iommu: reorganize

[Felipe Contreras]

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mach-omap2/omap3-iommu.c |   72 ++++++++++++++++++------------------
 1 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index 367f36a..f4232ec 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -14,43 +14,31 @@
 
 #include <mach/iommu.h>
 
-#define OMAP3_MMU1_BASE	0x480bd400
-#define OMAP3_MMU2_BASE	0x5d000000
-#define OMAP3_MMU1_IRQ	24
-#define OMAP3_MMU2_IRQ	28
-
-static struct resource omap3_iommu_res[] = {
-	{ /* Camera ISP MMU */
-		.start		= OMAP3_MMU1_BASE,
-		.end		= OMAP3_MMU1_BASE + MMU_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= OMAP3_MMU1_IRQ,
-		.flags		= IORESOURCE_IRQ,
-	},
-	{ /* IVA2.2 MMU */
-		.start		= OMAP3_MMU2_BASE,
-		.end		= OMAP3_MMU2_BASE + MMU_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= OMAP3_MMU2_IRQ,
-		.flags		= IORESOURCE_IRQ,
-	},
+struct iommu_device {
+	resource_size_t base;
+	resource_size_t irq;
+	struct iommu_platform_data pdata;
+	struct resource res[2];
 };
-#define NR_IOMMU_RES (ARRAY_SIZE(omap3_iommu_res) / 2)
 
-static const struct iommu_platform_data omap3_iommu_pdata[] __initconst = {
+static struct iommu_device devices[] = {
 	{
-		.name = "isp",
-		.nr_tlb_entries = 8,
-		.clk_name = "cam_ick",
+		.base = 0x480bd400,
+		.irq = 24,
+		.pdata = {
+			.name = "isp",
+			.nr_tlb_entries = 8,
+			.clk_name = "cam_ick",
+		},
 	},
 	{
-		.name = "iva2",
-		.nr_tlb_entries = 32,
-		.clk_name = "iva2_ck",
+		.base = 0x5d000000,
+		.irq = 28,
+		.pdata = {
+			.name = "iva2",
+			.nr_tlb_entries = 32,
+			.clk_name = "iva2_ck",
+		},
 	},
 };
 #define NR_IOMMU_DEVICES ARRAY_SIZE(omap3_iommu_pdata)
@@ -63,23 +51,35 @@ static int __init omap3_iommu_init(void)
 
 	for (i = 0; i < NR_IOMMU_DEVICES; i++) {
 		struct platform_device *pdev;
+		const struct iommu_device *d = &devices[i];
+		struct resource res[] = {
+			{ .flags = IORESOURCE_MEM },
+			{ .flags = IORESOURCE_IRQ },
+		};
 
 		pdev = platform_device_alloc("omap-iommu", i + 1);
 		if (!pdev) {
 			err = -ENOMEM;
 			goto err_out;
 		}
-		err = platform_device_add_resources(pdev,
-				    &omap3_iommu_res[2 * i], NR_IOMMU_RES);
+
+		res[0].start = d->base;
+		res[0].end = d->base + MMU_REG_SIZE - 1;
+
+		res[1].start = d->irq;
+
+		err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
 		if (err)
 			goto err_out;
-		err = platform_device_add_data(pdev, &omap3_iommu_pdata[i],
-					       sizeof(omap3_iommu_pdata[0]));
+
+		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;
+
 		omap3_iommu_pdev[i] = pdev;
 	}
 	return 0;
-- 
1.6.3.GIT

[RFC/PATCH 2/3] omap3-iommu: split init function into omap_iommu_add

[Felipe Contreras]

In preparation for external registration.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mach-omap2/omap3-iommu.c |   76 ++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index f4232ec..149c624 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -45,43 +45,65 @@ static struct iommu_device devices[] = {
 
 static struct platform_device *omap3_iommu_pdev[NR_IOMMU_DEVICES];
 
-static int __init omap3_iommu_init(void)
+static struct platform_device *omap_iommu_add(const char *name)
 {
-	int i, err;
-
-	for (i = 0; i < NR_IOMMU_DEVICES; i++) {
-		struct platform_device *pdev;
-		const struct iommu_device *d = &devices[i];
-		struct resource res[] = {
-			{ .flags = IORESOURCE_MEM },
-			{ .flags = IORESOURCE_IRQ },
-		};
-
-		pdev = platform_device_alloc("omap-iommu", i + 1);
-		if (!pdev) {
-			err = -ENOMEM;
-			goto err_out;
+	struct platform_device *pdev;
+	const struct iommu_device *d = NULL;
+	struct resource res[] = {
+		{ .flags = IORESOURCE_MEM },
+		{ .flags = IORESOURCE_IRQ },
+	};
+	int err, i;
+
+	for (i = 0; i < ARRAY_SIZE(devices); i++)
+		if (strcmp(devices[i].pdata.name, name) == 0) {
+			d = &devices[i];
+			break;
 		}
 
-		res[0].start = d->base;
-		res[0].end = d->base + MMU_REG_SIZE - 1;
+	if (!d)
+		return NULL;
 
-		res[1].start = d->irq;
+	pdev = platform_device_alloc("omap-iommu", i + 1);
+	if (!pdev)
+		return NULL;
 
-		err = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
-		if (err)
-			goto err_out;
+	res[0].start = d->base;
+	res[0].end = d->base + MMU_REG_SIZE - 1;
 
-		err = platform_device_add_data(pdev, &d->pdata, sizeof(d->pdata));
-		if (err)
-			goto err_out;
+	res[1].start = d->irq;
 
-		err = platform_device_add(pdev);
-		if (err)
-			goto err_out;
+	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;
+
+	return pdev;
 
+err_out:
+	platform_device_put(pdev);
+	return NULL;
+}
+
+static int __init omap3_iommu_init(void)
+{
+	struct platform_device *pdev;
+	int i, err;
+
+	for (i = 0; i < ARRAY_SIZE(devices); i++) {
+		pdev = omap_iommu_add(devices[i].pdata.name);
+		if (!pdev)
+			goto err_out;
 		omap3_iommu_pdev[i] = pdev;
 	}
+
 	return 0;
 
 err_out:
-- 
1.6.3.GIT

[RFC/PATCH 3/3] omap3-iommu: remote registration

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mach-omap2/omap3-iommu.c       |   36 +-----------------------------
 arch/arm/plat-omap/include/mach/iommu.h |    2 +
 2 files changed, 4 insertions(+), 34 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-iommu.c b/arch/arm/mach-omap2/omap3-iommu.c
index 149c624..8380cd5 100644
--- a/arch/arm/mach-omap2/omap3-iommu.c
+++ b/arch/arm/mach-omap2/omap3-iommu.c
@@ -41,11 +41,8 @@ static struct iommu_device devices[] = {
 		},
 	},
 };
-#define NR_IOMMU_DEVICES ARRAY_SIZE(omap3_iommu_pdata)
 
-static struct platform_device *omap3_iommu_pdev[NR_IOMMU_DEVICES];
-
-static struct platform_device *omap_iommu_add(const char *name)
+struct platform_device *omap_iommu_add(const char *name)
 {
 	struct platform_device *pdev;
 	const struct iommu_device *d = NULL;
@@ -91,36 +88,7 @@ err_out:
 	platform_device_put(pdev);
 	return NULL;
 }
-
-static int __init omap3_iommu_init(void)
-{
-	struct platform_device *pdev;
-	int i, err;
-
-	for (i = 0; i < ARRAY_SIZE(devices); i++) {
-		pdev = omap_iommu_add(devices[i].pdata.name);
-		if (!pdev)
-			goto err_out;
-		omap3_iommu_pdev[i] = pdev;
-	}
-
-	return 0;
-
-err_out:
-	while (i--)
-		platform_device_put(omap3_iommu_pdev[i]);
-	return err;
-}
-module_init(omap3_iommu_init);
-
-static void __exit omap3_iommu_exit(void)
-{
-	int i;
-
-	for (i = 0; i < NR_IOMMU_DEVICES; i++)
-		platform_device_unregister(omap3_iommu_pdev[i]);
-}
-module_exit(omap3_iommu_exit);
+EXPORT_SYMBOL_GPL(omap_iommu_add);
 
 MODULE_AUTHOR("Hiroshi DOYU");
 MODULE_DESCRIPTION("omap iommu: omap3 device registration");
diff --git a/arch/arm/plat-omap/include/mach/iommu.h b/arch/arm/plat-omap/include/mach/iommu.h
index 769b00b..e22a4a4 100644
--- a/arch/arm/plat-omap/include/mach/iommu.h
+++ b/arch/arm/plat-omap/include/mach/iommu.h
@@ -165,4 +165,6 @@ extern int foreach_iommu_device(void *data,
 extern ssize_t iommu_dump_ctx(struct iommu *obj, char *buf);
 extern size_t dump_tlb_entries(struct iommu *obj, char *buf);
 
+struct platform_device *omap_iommu_add(const char *name);
+
 #endif /* __MACH_IOMMU_H */
-- 
1.6.3.GIT