| Message ID | 1242468350-23190-4-git-send-email-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
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. -- 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
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.
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
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.
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
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)
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.
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 */
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(-)