* [PATCH 0/2] OMAP IOMMU cleanups
@ 2014-09-09 15:45 Laurent Pinchart
[not found] ` <1410277545-32157-1-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2014-09-09 15:45 UTC (permalink / raw)
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
Hello,
Those two patches clean up the OMAP IOMMU driver. Please see individual commit
messages for more information.
Laurent Pinchart (2):
iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
iommu/omap: Remove omap_iommu unused owner field
drivers/iommu/omap-iommu-debug.c | 6 ++-
drivers/iommu/omap-iommu.c | 103 ++++++++++++---------------------------
drivers/iommu/omap-iommu.h | 11 +++--
drivers/iommu/omap-iommu2.c | 18 +------
4 files changed, 44 insertions(+), 94 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
[not found] ` <1410277545-32157-1-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
@ 2014-09-09 15:45 ` Laurent Pinchart
[not found] ` <1410277545-32157-2-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
2014-09-09 15:45 ` [PATCH 2/2] iommu/omap: Remove omap_iommu unused owner field Laurent Pinchart
1 sibling, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2014-09-09 15:45 UTC (permalink / raw)
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
The OMAP IOMMU driver supports both the OMAP1 and OMAP2+ IOMMU variants
by splitting the driver into a core module and a thin arch-specific
operations module.
(In practice only the OMAP2+ module omap-iommu2 is implemented, but
let's not denigrate the effort.)
The arch-specific operations module registers itself with the OMAP IOMMU
core module at initialization time. This initializes a module global
arch-specific operations pointer, used at runtime by the IOMMU
instances.
This scheme causes several issues. In addition to making it impossible
to support different OMAP IOMMU types in a single system (which in all
fairness is quite unlikely to happen), it also causes initialization
ordering issues by requiring the arch-specific operations module to be
loaded before any IOMMU user. This results in a probe breakage with the
OMAP3 ISP driver when not compiled as a module.
Fix the problem by inverting the dependency. Instead of having the
omap-iommu2 module register itself to iommu-omap, make the iommu-omap
retrieve the omap-iommu2 operations structure directly when probing the
IOMMU device. This ensures that a probed IOMMU will always have valid
arch-specific operations.
As the arch-specific operations pointer is now initialized at probe
time, this change requires turning it from a global variable into a
per-device variable.
Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
drivers/iommu/omap-iommu-debug.c | 6 ++-
drivers/iommu/omap-iommu.c | 94 ++++++++++++++--------------------------
drivers/iommu/omap-iommu.h | 10 ++++-
drivers/iommu/omap-iommu2.c | 18 +-------
4 files changed, 45 insertions(+), 83 deletions(-)
diff --git a/drivers/iommu/omap-iommu-debug.c b/drivers/iommu/omap-iommu-debug.c
index 531658d..35a2c3a 100644
--- a/drivers/iommu/omap-iommu-debug.c
+++ b/drivers/iommu/omap-iommu-debug.c
@@ -33,7 +33,9 @@ static struct dentry *iommu_debug_root;
static ssize_t debug_read_ver(struct file *file, char __user *userbuf,
size_t count, loff_t *ppos)
{
- u32 ver = omap_iommu_arch_version();
+ struct device *dev = file->private_data;
+ struct omap_iommu *obj = dev_to_omap_iommu(dev);
+ u32 ver = omap_iommu_arch_version(obj);
char buf[MAXCOLUMN], *p = buf;
p += sprintf(p, "H/W version: %d.%d\n", (ver >> 4) & 0xf , ver & 0xf);
@@ -117,7 +119,7 @@ static ssize_t debug_write_pagetable(struct file *file,
return -EINVAL;
}
- omap_iotlb_cr_to_e(&cr, &e);
+ omap_iotlb_cr_to_e(obj, &cr, &e);
err = omap_iopgtable_store_entry(obj, &e);
if (err)
dev_err(obj->dev, "%s: fail to store cr\n", __func__);
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index df579f8..192c367 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -76,45 +76,10 @@ struct iotlb_lock {
short vict;
};
-/* accommodate the difference between omap1 and omap2/3 */
-static const struct iommu_functions *arch_iommu;
-
static struct platform_driver omap_iommu_driver;
static struct kmem_cache *iopte_cachep;
/**
- * omap_install_iommu_arch - Install archtecure specific iommu functions
- * @ops: a pointer to architecture specific iommu functions
- *
- * There are several kind of iommu algorithm(tlb, pagetable) among
- * omap series. This interface installs such an iommu algorighm.
- **/
-int omap_install_iommu_arch(const struct iommu_functions *ops)
-{
- if (arch_iommu)
- return -EBUSY;
-
- arch_iommu = ops;
- return 0;
-}
-EXPORT_SYMBOL_GPL(omap_install_iommu_arch);
-
-/**
- * omap_uninstall_iommu_arch - Uninstall archtecure specific iommu functions
- * @ops: a pointer to architecture specific iommu functions
- *
- * This interface uninstalls the iommu algorighm installed previously.
- **/
-void omap_uninstall_iommu_arch(const struct iommu_functions *ops)
-{
- if (arch_iommu != ops)
- pr_err("%s: not your arch\n", __func__);
-
- arch_iommu = NULL;
-}
-EXPORT_SYMBOL_GPL(omap_uninstall_iommu_arch);
-
-/**
* omap_iommu_save_ctx - Save registers for pm off-mode support
* @dev: client device
**/
@@ -122,7 +87,7 @@ void omap_iommu_save_ctx(struct device *dev)
{
struct omap_iommu *obj = dev_to_omap_iommu(dev);
- arch_iommu->save_ctx(obj);
+ obj->arch_iommu->save_ctx(obj);
}
EXPORT_SYMBOL_GPL(omap_iommu_save_ctx);
@@ -134,16 +99,16 @@ void omap_iommu_restore_ctx(struct device *dev)
{
struct omap_iommu *obj = dev_to_omap_iommu(dev);
- arch_iommu->restore_ctx(obj);
+ obj->arch_iommu->restore_ctx(obj);
}
EXPORT_SYMBOL_GPL(omap_iommu_restore_ctx);
/**
* omap_iommu_arch_version - Return running iommu arch version
**/
-u32 omap_iommu_arch_version(void)
+u32 omap_iommu_arch_version(struct omap_iommu *obj)
{
- return arch_iommu->version;
+ return obj->arch_iommu->version;
}
EXPORT_SYMBOL_GPL(omap_iommu_arch_version);
@@ -153,7 +118,7 @@ static int iommu_enable(struct omap_iommu *obj)
struct platform_device *pdev = to_platform_device(obj->dev);
struct iommu_platform_data *pdata = pdev->dev.platform_data;
- if (!arch_iommu)
+ if (!obj->arch_iommu)
return -ENODEV;
if (pdata && pdata->deassert_reset) {
@@ -166,7 +131,7 @@ static int iommu_enable(struct omap_iommu *obj)
pm_runtime_get_sync(obj->dev);
- err = arch_iommu->enable(obj);
+ err = obj->arch_iommu->enable(obj);
return err;
}
@@ -176,7 +141,7 @@ static void iommu_disable(struct omap_iommu *obj)
struct platform_device *pdev = to_platform_device(obj->dev);
struct iommu_platform_data *pdata = pdev->dev.platform_data;
- arch_iommu->disable(obj);
+ obj->arch_iommu->disable(obj);
pm_runtime_put_sync(obj->dev);
@@ -187,20 +152,21 @@ static void iommu_disable(struct omap_iommu *obj)
/*
* TLB operations
*/
-void omap_iotlb_cr_to_e(struct cr_regs *cr, struct iotlb_entry *e)
+void omap_iotlb_cr_to_e(struct omap_iommu *obj, struct cr_regs *cr,
+ struct iotlb_entry *e)
{
BUG_ON(!cr || !e);
- arch_iommu->cr_to_e(cr, e);
+ obj->arch_iommu->cr_to_e(cr, e);
}
EXPORT_SYMBOL_GPL(omap_iotlb_cr_to_e);
-static inline int iotlb_cr_valid(struct cr_regs *cr)
+static inline int iotlb_cr_valid(struct omap_iommu *obj, struct cr_regs *cr)
{
if (!cr)
return -EINVAL;
- return arch_iommu->cr_valid(cr);
+ return obj->arch_iommu->cr_valid(cr);
}
static inline struct cr_regs *iotlb_alloc_cr(struct omap_iommu *obj,
@@ -209,22 +175,22 @@ static inline struct cr_regs *iotlb_alloc_cr(struct omap_iommu *obj,
if (!e)
return NULL;
- return arch_iommu->alloc_cr(obj, e);
+ return obj->arch_iommu->alloc_cr(obj, e);
}
-static u32 iotlb_cr_to_virt(struct cr_regs *cr)
+static u32 iotlb_cr_to_virt(struct omap_iommu *obj, struct cr_regs *cr)
{
- return arch_iommu->cr_to_virt(cr);
+ return obj->arch_iommu->cr_to_virt(cr);
}
-static u32 get_iopte_attr(struct iotlb_entry *e)
+static u32 get_iopte_attr(struct omap_iommu *obj, struct iotlb_entry *e)
{
- return arch_iommu->get_pte_attr(e);
+ return obj->arch_iommu->get_pte_attr(e);
}
static u32 iommu_report_fault(struct omap_iommu *obj, u32 *da)
{
- return arch_iommu->fault_isr(obj, da);
+ return obj->arch_iommu->fault_isr(obj, da);
}
static void iotlb_lock_get(struct omap_iommu *obj, struct iotlb_lock *l)
@@ -250,12 +216,12 @@ static void iotlb_lock_set(struct omap_iommu *obj, struct iotlb_lock *l)
static void iotlb_read_cr(struct omap_iommu *obj, struct cr_regs *cr)
{
- arch_iommu->tlb_read_cr(obj, cr);
+ obj->arch_iommu->tlb_read_cr(obj, cr);
}
static void iotlb_load_cr(struct omap_iommu *obj, struct cr_regs *cr)
{
- arch_iommu->tlb_load_cr(obj, cr);
+ obj->arch_iommu->tlb_load_cr(obj, cr);
iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
iommu_write_reg(obj, 1, MMU_LD_TLB);
@@ -272,7 +238,7 @@ static inline ssize_t iotlb_dump_cr(struct omap_iommu *obj, struct cr_regs *cr,
{
BUG_ON(!cr || !buf);
- return arch_iommu->dump_cr(obj, cr, buf);
+ return obj->arch_iommu->dump_cr(obj, cr, buf);
}
/* only used in iotlb iteration for-loop */
@@ -317,7 +283,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e)
struct cr_regs tmp;
for_each_iotlb_cr(obj, obj->nr_tlb_entries, i, tmp)
- if (!iotlb_cr_valid(&tmp))
+ if (!iotlb_cr_valid(obj, &tmp))
break;
if (i == obj->nr_tlb_entries) {
@@ -384,10 +350,10 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
u32 start;
size_t bytes;
- if (!iotlb_cr_valid(&cr))
+ if (!iotlb_cr_valid(obj, &cr))
continue;
- start = iotlb_cr_to_virt(&cr);
+ start = iotlb_cr_to_virt(obj, &cr);
bytes = iopgsz_to_bytes(cr.cam & 3);
if ((start <= da) && (da < start + bytes)) {
@@ -432,7 +398,7 @@ ssize_t omap_iommu_dump_ctx(struct omap_iommu *obj, char *buf, ssize_t bytes)
pm_runtime_get_sync(obj->dev);
- bytes = arch_iommu->dump_ctx(obj, buf, bytes);
+ bytes = obj->arch_iommu->dump_ctx(obj, buf, bytes);
pm_runtime_put_sync(obj->dev);
@@ -452,7 +418,7 @@ __dump_tlb_entries(struct omap_iommu *obj, struct cr_regs *crs, int num)
iotlb_lock_get(obj, &saved);
for_each_iotlb_cr(obj, num, i, tmp) {
- if (!iotlb_cr_valid(&tmp))
+ if (!iotlb_cr_valid(obj, &tmp))
continue;
*p++ = tmp;
}
@@ -666,7 +632,7 @@ iopgtable_store_entry_core(struct omap_iommu *obj, struct iotlb_entry *e)
break;
}
- prot = get_iopte_attr(e);
+ prot = get_iopte_attr(obj, e);
spin_lock(&obj->page_table_lock);
err = fn(obj, e->da, e->pa, prot);
@@ -873,8 +839,10 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
dev = driver_find_device(&omap_iommu_driver.driver, NULL,
(void *)name,
device_match_by_alias);
- if (!dev)
+ if (!dev) {
+ dev_err(dev, "%s: can't find IOMMU %s\n", __func__, name);
return ERR_PTR(-ENODEV);
+ }
obj = to_iommu(dev);
@@ -894,6 +862,7 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
flush_iotlb_all(obj);
if (!try_module_get(obj->owner)) {
+ dev_err(obj->dev, "%s: can't get owner\n", __func__);
err = -ENODEV;
goto err_module;
}
@@ -969,6 +938,7 @@ static int omap_iommu_probe(struct platform_device *pdev)
obj->dev = &pdev->dev;
obj->ctx = (void *)obj + sizeof(*obj);
+ obj->arch_iommu = &omap2_iommu_ops;
spin_lock_init(&obj->iommu_lock);
spin_lock_init(&obj->page_table_lock);
diff --git a/drivers/iommu/omap-iommu.h b/drivers/iommu/omap-iommu.h
index 1275a82..7a90800 100644
--- a/drivers/iommu/omap-iommu.h
+++ b/drivers/iommu/omap-iommu.h
@@ -46,6 +46,9 @@ struct omap_iommu {
int nr_tlb_entries;
+ /* accommodate the difference between omap1 and omap2/3 */
+ const struct iommu_functions *arch_iommu;
+
void *ctx; /* iommu context: registres saved area */
int has_bus_err_back;
@@ -193,9 +196,10 @@ static inline struct omap_iommu *dev_to_omap_iommu(struct device *dev)
/*
* global functions
*/
-extern u32 omap_iommu_arch_version(void);
+extern u32 omap_iommu_arch_version(struct omap_iommu *obj);
-extern void omap_iotlb_cr_to_e(struct cr_regs *cr, struct iotlb_entry *e);
+extern void omap_iotlb_cr_to_e(struct omap_iommu *obj, struct cr_regs *cr,
+ struct iotlb_entry *e);
extern int
omap_iopgtable_store_entry(struct omap_iommu *obj, struct iotlb_entry *e);
@@ -214,6 +218,8 @@ omap_iommu_dump_ctx(struct omap_iommu *obj, char *buf, ssize_t len);
extern size_t
omap_dump_tlb_entries(struct omap_iommu *obj, char *buf, ssize_t len);
+extern const struct iommu_functions omap2_iommu_ops;
+
/*
* register accessors
*/
diff --git a/drivers/iommu/omap-iommu2.c b/drivers/iommu/omap-iommu2.c
index 5e1ea3b..e72fe62 100644
--- a/drivers/iommu/omap-iommu2.c
+++ b/drivers/iommu/omap-iommu2.c
@@ -296,7 +296,7 @@ static void omap2_cr_to_e(struct cr_regs *cr, struct iotlb_entry *e)
e->mixed = cr->ram & MMU_RAM_MIXED;
}
-static const struct iommu_functions omap2_iommu_ops = {
+const struct iommu_functions omap2_iommu_ops = {
.version = IOMMU_ARCH_VERSION,
.enable = omap2_iommu_enable,
@@ -319,19 +319,3 @@ static const struct iommu_functions omap2_iommu_ops = {
.restore_ctx = omap2_iommu_restore_ctx,
.dump_ctx = omap2_iommu_dump_ctx,
};
-
-static int __init omap2_iommu_init(void)
-{
- return omap_install_iommu_arch(&omap2_iommu_ops);
-}
-module_init(omap2_iommu_init);
-
-static void __exit omap2_iommu_exit(void)
-{
- omap_uninstall_iommu_arch(&omap2_iommu_ops);
-}
-module_exit(omap2_iommu_exit);
-
-MODULE_AUTHOR("Hiroshi DOYU, Paul Mundt and Toshihiro Kobayashi");
-MODULE_DESCRIPTION("omap iommu: omap2/3 architecture specific functions");
-MODULE_LICENSE("GPL v2");
--
1.8.5.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] iommu/omap: Remove omap_iommu unused owner field
[not found] ` <1410277545-32157-1-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
2014-09-09 15:45 ` [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2 Laurent Pinchart
@ 2014-09-09 15:45 ` Laurent Pinchart
2014-09-09 21:41 ` Suman Anna
[not found] ` <1410277545-32157-3-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
1 sibling, 2 replies; 10+ messages in thread
From: Laurent Pinchart @ 2014-09-09 15:45 UTC (permalink / raw)
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
The owner field is never set. Remove it.
Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
drivers/iommu/omap-iommu.c | 11 -----------
drivers/iommu/omap-iommu.h | 1 -
2 files changed, 12 deletions(-)
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 192c367..fdfe732 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -861,20 +861,11 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
goto err_enable;
flush_iotlb_all(obj);
- if (!try_module_get(obj->owner)) {
- dev_err(obj->dev, "%s: can't get owner\n", __func__);
- err = -ENODEV;
- goto err_module;
- }
-
spin_unlock(&obj->iommu_lock);
dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name);
return obj;
-err_module:
- if (obj->refcount == 1)
- iommu_disable(obj);
err_enable:
obj->refcount--;
spin_unlock(&obj->iommu_lock);
@@ -895,8 +886,6 @@ static void omap_iommu_detach(struct omap_iommu *obj)
if (--obj->refcount == 0)
iommu_disable(obj);
- module_put(obj->owner);
-
obj->iopgd = NULL;
spin_unlock(&obj->iommu_lock);
diff --git a/drivers/iommu/omap-iommu.h b/drivers/iommu/omap-iommu.h
index 7a90800..2c3b85c 100644
--- a/drivers/iommu/omap-iommu.h
+++ b/drivers/iommu/omap-iommu.h
@@ -28,7 +28,6 @@ struct iotlb_entry {
struct omap_iommu {
const char *name;
- struct module *owner;
void __iomem *regbase;
struct device *dev;
void *isr_priv;
--
1.8.5.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
[not found] ` <1410277545-32157-2-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
@ 2014-09-09 21:33 ` Suman Anna
[not found] ` <540F7217.3070501-l0cyMroinI0@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Suman Anna @ 2014-09-09 21:33 UTC (permalink / raw)
To: Laurent Pinchart, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
Hi Laurent,
On 09/09/2014 10:45 AM, Laurent Pinchart wrote:
> The OMAP IOMMU driver supports both the OMAP1 and OMAP2+ IOMMU variants
> by splitting the driver into a core module and a thin arch-specific
> operations module.
>
> (In practice only the OMAP2+ module omap-iommu2 is implemented, but
> let's not denigrate the effort.)
Thank you for the patch. I had something similar in my list of cleanup
TODO items on the OMAP IOMMU driver, but I was intending to cut down
more and consolidate the omap-iommu.c and omap-iommu2.c files into a
single one.
This had been the case from the introduction of the driver going back to
v2.6.30, and OMAP1 was never added and I doubt it would be added anytime
in the foreseeable future.
>
> The arch-specific operations module registers itself with the OMAP IOMMU
> core module at initialization time. This initializes a module global
> arch-specific operations pointer, used at runtime by the IOMMU
> instances.
>
> This scheme causes several issues. In addition to making it impossible
> to support different OMAP IOMMU types in a single system (which in all
> fairness is quite unlikely to happen),
Yep, except for a few enhancements (like reporting Fault PC address on
OMAP4 DSPs, and dropping both endianness support), the core IOMMU
functionality hasn't changed much between OMAP2 and the latest OMAP4+
SoCs. So, my plan was to completely get rid of the iommu_functions (it
also eliminates the need for exporting most of the OMAP IOMMU API). So
while I am ok with the current patch, I prefer consolidation than
keeping the scalability alive, it can always be added if a need for that
arises. What do you think?
it also causes initialization
> ordering issues by requiring the arch-specific operations module to be
> loaded before any IOMMU user. This results in a probe breakage with the
> OMAP3 ISP driver when not compiled as a module.
This can be fixed if we make the current omap-iommu2.c as a
subsys_initcall as well, right?
regards
Suman
>
> Fix the problem by inverting the dependency. Instead of having the
> omap-iommu2 module register itself to iommu-omap, make the iommu-omap
> retrieve the omap-iommu2 operations structure directly when probing the
> IOMMU device. This ensures that a probed IOMMU will always have valid
> arch-specific operations.
>
> As the arch-specific operations pointer is now initialized at probe
> time, this change requires turning it from a global variable into a
> per-device variable.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
> drivers/iommu/omap-iommu-debug.c | 6 ++-
> drivers/iommu/omap-iommu.c | 94 ++++++++++++++--------------------------
> drivers/iommu/omap-iommu.h | 10 ++++-
> drivers/iommu/omap-iommu2.c | 18 +-------
> 4 files changed, 45 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu-debug.c b/drivers/iommu/omap-iommu-debug.c
> index 531658d..35a2c3a 100644
> --- a/drivers/iommu/omap-iommu-debug.c
> +++ b/drivers/iommu/omap-iommu-debug.c
> @@ -33,7 +33,9 @@ static struct dentry *iommu_debug_root;
> static ssize_t debug_read_ver(struct file *file, char __user *userbuf,
> size_t count, loff_t *ppos)
> {
> - u32 ver = omap_iommu_arch_version();
> + struct device *dev = file->private_data;
> + struct omap_iommu *obj = dev_to_omap_iommu(dev);
> + u32 ver = omap_iommu_arch_version(obj);
> char buf[MAXCOLUMN], *p = buf;
>
> p += sprintf(p, "H/W version: %d.%d\n", (ver >> 4) & 0xf , ver & 0xf);
> @@ -117,7 +119,7 @@ static ssize_t debug_write_pagetable(struct file *file,
> return -EINVAL;
> }
>
> - omap_iotlb_cr_to_e(&cr, &e);
> + omap_iotlb_cr_to_e(obj, &cr, &e);
> err = omap_iopgtable_store_entry(obj, &e);
> if (err)
> dev_err(obj->dev, "%s: fail to store cr\n", __func__);
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index df579f8..192c367 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -76,45 +76,10 @@ struct iotlb_lock {
> short vict;
> };
>
> -/* accommodate the difference between omap1 and omap2/3 */
> -static const struct iommu_functions *arch_iommu;
> -
> static struct platform_driver omap_iommu_driver;
> static struct kmem_cache *iopte_cachep;
>
> /**
> - * omap_install_iommu_arch - Install archtecure specific iommu functions
> - * @ops: a pointer to architecture specific iommu functions
> - *
> - * There are several kind of iommu algorithm(tlb, pagetable) among
> - * omap series. This interface installs such an iommu algorighm.
> - **/
> -int omap_install_iommu_arch(const struct iommu_functions *ops)
> -{
> - if (arch_iommu)
> - return -EBUSY;
> -
> - arch_iommu = ops;
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(omap_install_iommu_arch);
> -
> -/**
> - * omap_uninstall_iommu_arch - Uninstall archtecure specific iommu functions
> - * @ops: a pointer to architecture specific iommu functions
> - *
> - * This interface uninstalls the iommu algorighm installed previously.
> - **/
> -void omap_uninstall_iommu_arch(const struct iommu_functions *ops)
> -{
> - if (arch_iommu != ops)
> - pr_err("%s: not your arch\n", __func__);
> -
> - arch_iommu = NULL;
> -}
> -EXPORT_SYMBOL_GPL(omap_uninstall_iommu_arch);
> -
> -/**
> * omap_iommu_save_ctx - Save registers for pm off-mode support
> * @dev: client device
> **/
> @@ -122,7 +87,7 @@ void omap_iommu_save_ctx(struct device *dev)
> {
> struct omap_iommu *obj = dev_to_omap_iommu(dev);
>
> - arch_iommu->save_ctx(obj);
> + obj->arch_iommu->save_ctx(obj);
> }
> EXPORT_SYMBOL_GPL(omap_iommu_save_ctx);
>
> @@ -134,16 +99,16 @@ void omap_iommu_restore_ctx(struct device *dev)
> {
> struct omap_iommu *obj = dev_to_omap_iommu(dev);
>
> - arch_iommu->restore_ctx(obj);
> + obj->arch_iommu->restore_ctx(obj);
> }
> EXPORT_SYMBOL_GPL(omap_iommu_restore_ctx);
>
> /**
> * omap_iommu_arch_version - Return running iommu arch version
> **/
> -u32 omap_iommu_arch_version(void)
> +u32 omap_iommu_arch_version(struct omap_iommu *obj)
> {
> - return arch_iommu->version;
> + return obj->arch_iommu->version;
> }
> EXPORT_SYMBOL_GPL(omap_iommu_arch_version);
>
> @@ -153,7 +118,7 @@ static int iommu_enable(struct omap_iommu *obj)
> struct platform_device *pdev = to_platform_device(obj->dev);
> struct iommu_platform_data *pdata = pdev->dev.platform_data;
>
> - if (!arch_iommu)
> + if (!obj->arch_iommu)
> return -ENODEV;
>
> if (pdata && pdata->deassert_reset) {
> @@ -166,7 +131,7 @@ static int iommu_enable(struct omap_iommu *obj)
>
> pm_runtime_get_sync(obj->dev);
>
> - err = arch_iommu->enable(obj);
> + err = obj->arch_iommu->enable(obj);
>
> return err;
> }
> @@ -176,7 +141,7 @@ static void iommu_disable(struct omap_iommu *obj)
> struct platform_device *pdev = to_platform_device(obj->dev);
> struct iommu_platform_data *pdata = pdev->dev.platform_data;
>
> - arch_iommu->disable(obj);
> + obj->arch_iommu->disable(obj);
>
> pm_runtime_put_sync(obj->dev);
>
> @@ -187,20 +152,21 @@ static void iommu_disable(struct omap_iommu *obj)
> /*
> * TLB operations
> */
> -void omap_iotlb_cr_to_e(struct cr_regs *cr, struct iotlb_entry *e)
> +void omap_iotlb_cr_to_e(struct omap_iommu *obj, struct cr_regs *cr,
> + struct iotlb_entry *e)
> {
> BUG_ON(!cr || !e);
>
> - arch_iommu->cr_to_e(cr, e);
> + obj->arch_iommu->cr_to_e(cr, e);
> }
> EXPORT_SYMBOL_GPL(omap_iotlb_cr_to_e);
>
> -static inline int iotlb_cr_valid(struct cr_regs *cr)
> +static inline int iotlb_cr_valid(struct omap_iommu *obj, struct cr_regs *cr)
> {
> if (!cr)
> return -EINVAL;
>
> - return arch_iommu->cr_valid(cr);
> + return obj->arch_iommu->cr_valid(cr);
> }
>
> static inline struct cr_regs *iotlb_alloc_cr(struct omap_iommu *obj,
> @@ -209,22 +175,22 @@ static inline struct cr_regs *iotlb_alloc_cr(struct omap_iommu *obj,
> if (!e)
> return NULL;
>
> - return arch_iommu->alloc_cr(obj, e);
> + return obj->arch_iommu->alloc_cr(obj, e);
> }
>
> -static u32 iotlb_cr_to_virt(struct cr_regs *cr)
> +static u32 iotlb_cr_to_virt(struct omap_iommu *obj, struct cr_regs *cr)
> {
> - return arch_iommu->cr_to_virt(cr);
> + return obj->arch_iommu->cr_to_virt(cr);
> }
>
> -static u32 get_iopte_attr(struct iotlb_entry *e)
> +static u32 get_iopte_attr(struct omap_iommu *obj, struct iotlb_entry *e)
> {
> - return arch_iommu->get_pte_attr(e);
> + return obj->arch_iommu->get_pte_attr(e);
> }
>
> static u32 iommu_report_fault(struct omap_iommu *obj, u32 *da)
> {
> - return arch_iommu->fault_isr(obj, da);
> + return obj->arch_iommu->fault_isr(obj, da);
> }
>
> static void iotlb_lock_get(struct omap_iommu *obj, struct iotlb_lock *l)
> @@ -250,12 +216,12 @@ static void iotlb_lock_set(struct omap_iommu *obj, struct iotlb_lock *l)
>
> static void iotlb_read_cr(struct omap_iommu *obj, struct cr_regs *cr)
> {
> - arch_iommu->tlb_read_cr(obj, cr);
> + obj->arch_iommu->tlb_read_cr(obj, cr);
> }
>
> static void iotlb_load_cr(struct omap_iommu *obj, struct cr_regs *cr)
> {
> - arch_iommu->tlb_load_cr(obj, cr);
> + obj->arch_iommu->tlb_load_cr(obj, cr);
>
> iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
> iommu_write_reg(obj, 1, MMU_LD_TLB);
> @@ -272,7 +238,7 @@ static inline ssize_t iotlb_dump_cr(struct omap_iommu *obj, struct cr_regs *cr,
> {
> BUG_ON(!cr || !buf);
>
> - return arch_iommu->dump_cr(obj, cr, buf);
> + return obj->arch_iommu->dump_cr(obj, cr, buf);
> }
>
> /* only used in iotlb iteration for-loop */
> @@ -317,7 +283,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e)
> struct cr_regs tmp;
>
> for_each_iotlb_cr(obj, obj->nr_tlb_entries, i, tmp)
> - if (!iotlb_cr_valid(&tmp))
> + if (!iotlb_cr_valid(obj, &tmp))
> break;
>
> if (i == obj->nr_tlb_entries) {
> @@ -384,10 +350,10 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
> u32 start;
> size_t bytes;
>
> - if (!iotlb_cr_valid(&cr))
> + if (!iotlb_cr_valid(obj, &cr))
> continue;
>
> - start = iotlb_cr_to_virt(&cr);
> + start = iotlb_cr_to_virt(obj, &cr);
> bytes = iopgsz_to_bytes(cr.cam & 3);
>
> if ((start <= da) && (da < start + bytes)) {
> @@ -432,7 +398,7 @@ ssize_t omap_iommu_dump_ctx(struct omap_iommu *obj, char *buf, ssize_t bytes)
>
> pm_runtime_get_sync(obj->dev);
>
> - bytes = arch_iommu->dump_ctx(obj, buf, bytes);
> + bytes = obj->arch_iommu->dump_ctx(obj, buf, bytes);
>
> pm_runtime_put_sync(obj->dev);
>
> @@ -452,7 +418,7 @@ __dump_tlb_entries(struct omap_iommu *obj, struct cr_regs *crs, int num)
> iotlb_lock_get(obj, &saved);
>
> for_each_iotlb_cr(obj, num, i, tmp) {
> - if (!iotlb_cr_valid(&tmp))
> + if (!iotlb_cr_valid(obj, &tmp))
> continue;
> *p++ = tmp;
> }
> @@ -666,7 +632,7 @@ iopgtable_store_entry_core(struct omap_iommu *obj, struct iotlb_entry *e)
> break;
> }
>
> - prot = get_iopte_attr(e);
> + prot = get_iopte_attr(obj, e);
>
> spin_lock(&obj->page_table_lock);
> err = fn(obj, e->da, e->pa, prot);
> @@ -873,8 +839,10 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
> dev = driver_find_device(&omap_iommu_driver.driver, NULL,
> (void *)name,
> device_match_by_alias);
> - if (!dev)
> + if (!dev) {
> + dev_err(dev, "%s: can't find IOMMU %s\n", __func__, name);
> return ERR_PTR(-ENODEV);
> + }
>
> obj = to_iommu(dev);
>
> @@ -894,6 +862,7 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
> flush_iotlb_all(obj);
>
> if (!try_module_get(obj->owner)) {
> + dev_err(obj->dev, "%s: can't get owner\n", __func__);
> err = -ENODEV;
> goto err_module;
> }
> @@ -969,6 +938,7 @@ static int omap_iommu_probe(struct platform_device *pdev)
>
> obj->dev = &pdev->dev;
> obj->ctx = (void *)obj + sizeof(*obj);
> + obj->arch_iommu = &omap2_iommu_ops;
>
> spin_lock_init(&obj->iommu_lock);
> spin_lock_init(&obj->page_table_lock);
> diff --git a/drivers/iommu/omap-iommu.h b/drivers/iommu/omap-iommu.h
> index 1275a82..7a90800 100644
> --- a/drivers/iommu/omap-iommu.h
> +++ b/drivers/iommu/omap-iommu.h
> @@ -46,6 +46,9 @@ struct omap_iommu {
>
> int nr_tlb_entries;
>
> + /* accommodate the difference between omap1 and omap2/3 */
> + const struct iommu_functions *arch_iommu;
> +
> void *ctx; /* iommu context: registres saved area */
>
> int has_bus_err_back;
> @@ -193,9 +196,10 @@ static inline struct omap_iommu *dev_to_omap_iommu(struct device *dev)
> /*
> * global functions
> */
> -extern u32 omap_iommu_arch_version(void);
> +extern u32 omap_iommu_arch_version(struct omap_iommu *obj);
>
> -extern void omap_iotlb_cr_to_e(struct cr_regs *cr, struct iotlb_entry *e);
> +extern void omap_iotlb_cr_to_e(struct omap_iommu *obj, struct cr_regs *cr,
> + struct iotlb_entry *e);
>
> extern int
> omap_iopgtable_store_entry(struct omap_iommu *obj, struct iotlb_entry *e);
> @@ -214,6 +218,8 @@ omap_iommu_dump_ctx(struct omap_iommu *obj, char *buf, ssize_t len);
> extern size_t
> omap_dump_tlb_entries(struct omap_iommu *obj, char *buf, ssize_t len);
>
> +extern const struct iommu_functions omap2_iommu_ops;
> +
> /*
> * register accessors
> */
> diff --git a/drivers/iommu/omap-iommu2.c b/drivers/iommu/omap-iommu2.c
> index 5e1ea3b..e72fe62 100644
> --- a/drivers/iommu/omap-iommu2.c
> +++ b/drivers/iommu/omap-iommu2.c
> @@ -296,7 +296,7 @@ static void omap2_cr_to_e(struct cr_regs *cr, struct iotlb_entry *e)
> e->mixed = cr->ram & MMU_RAM_MIXED;
> }
>
> -static const struct iommu_functions omap2_iommu_ops = {
> +const struct iommu_functions omap2_iommu_ops = {
> .version = IOMMU_ARCH_VERSION,
>
> .enable = omap2_iommu_enable,
> @@ -319,19 +319,3 @@ static const struct iommu_functions omap2_iommu_ops = {
> .restore_ctx = omap2_iommu_restore_ctx,
> .dump_ctx = omap2_iommu_dump_ctx,
> };
> -
> -static int __init omap2_iommu_init(void)
> -{
> - return omap_install_iommu_arch(&omap2_iommu_ops);
> -}
> -module_init(omap2_iommu_init);
> -
> -static void __exit omap2_iommu_exit(void)
> -{
> - omap_uninstall_iommu_arch(&omap2_iommu_ops);
> -}
> -module_exit(omap2_iommu_exit);
> -
> -MODULE_AUTHOR("Hiroshi DOYU, Paul Mundt and Toshihiro Kobayashi");
> -MODULE_DESCRIPTION("omap iommu: omap2/3 architecture specific functions");
> -MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iommu/omap: Remove omap_iommu unused owner field
2014-09-09 15:45 ` [PATCH 2/2] iommu/omap: Remove omap_iommu unused owner field Laurent Pinchart
@ 2014-09-09 21:41 ` Suman Anna
[not found] ` <1410277545-32157-3-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
1 sibling, 0 replies; 10+ messages in thread
From: Suman Anna @ 2014-09-09 21:41 UTC (permalink / raw)
To: Laurent Pinchart, iommu; +Cc: linux-omap, Florian Vaussard, Joerg Roedel
Hi Laurent,
On 09/09/2014 10:45 AM, Laurent Pinchart wrote:
> The owner field is never set. Remove it.
Thanks, this seems to have been dead code since the days OMAP IOMMU has
been converted from building as modules to built-in as part of the IOMMU
API adoption. So,
Acked-by: Suman Anna <s-anna@ti.com>
regards
Suman
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/iommu/omap-iommu.c | 11 -----------
> drivers/iommu/omap-iommu.h | 1 -
> 2 files changed, 12 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 192c367..fdfe732 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -861,20 +861,11 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
> goto err_enable;
> flush_iotlb_all(obj);
>
> - if (!try_module_get(obj->owner)) {
> - dev_err(obj->dev, "%s: can't get owner\n", __func__);
> - err = -ENODEV;
> - goto err_module;
> - }
> -
> spin_unlock(&obj->iommu_lock);
>
> dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name);
> return obj;
>
> -err_module:
> - if (obj->refcount == 1)
> - iommu_disable(obj);
> err_enable:
> obj->refcount--;
> spin_unlock(&obj->iommu_lock);
> @@ -895,8 +886,6 @@ static void omap_iommu_detach(struct omap_iommu *obj)
> if (--obj->refcount == 0)
> iommu_disable(obj);
>
> - module_put(obj->owner);
> -
> obj->iopgd = NULL;
>
> spin_unlock(&obj->iommu_lock);
> diff --git a/drivers/iommu/omap-iommu.h b/drivers/iommu/omap-iommu.h
> index 7a90800..2c3b85c 100644
> --- a/drivers/iommu/omap-iommu.h
> +++ b/drivers/iommu/omap-iommu.h
> @@ -28,7 +28,6 @@ struct iotlb_entry {
>
> struct omap_iommu {
> const char *name;
> - struct module *owner;
> void __iomem *regbase;
> struct device *dev;
> void *isr_priv;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
[not found] ` <540F7217.3070501-l0cyMroinI0@public.gmane.org>
@ 2014-09-09 21:59 ` Laurent Pinchart
2014-09-09 22:31 ` Suman Anna
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2014-09-09 21:59 UTC (permalink / raw)
To: Suman Anna
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
Hi Suman,
On Tuesday 09 September 2014 16:33:11 Suman Anna wrote:
> On 09/09/2014 10:45 AM, Laurent Pinchart wrote:
> > The OMAP IOMMU driver supports both the OMAP1 and OMAP2+ IOMMU variants
> > by splitting the driver into a core module and a thin arch-specific
> > operations module.
> >
> > (In practice only the OMAP2+ module omap-iommu2 is implemented, but
> > let's not denigrate the effort.)
>
> Thank you for the patch. I had something similar in my list of cleanup
> TODO items on the OMAP IOMMU driver, but I was intending to cut down
> more and consolidate the omap-iommu.c and omap-iommu2.c files into a
> single one.
>
> This had been the case from the introduction of the driver going back to
> v2.6.30, and OMAP1 was never added and I doubt it would be added anytime
> in the foreseeable future.
>
> > The arch-specific operations module registers itself with the OMAP IOMMU
> > core module at initialization time. This initializes a module global
> > arch-specific operations pointer, used at runtime by the IOMMU
> > instances.
> >
> > This scheme causes several issues. In addition to making it impossible
> > to support different OMAP IOMMU types in a single system (which in all
> > fairness is quite unlikely to happen),
>
> Yep, except for a few enhancements (like reporting Fault PC address on
> OMAP4 DSPs, and dropping both endianness support), the core IOMMU
> functionality hasn't changed much between OMAP2 and the latest OMAP4+
> SoCs. So, my plan was to completely get rid of the iommu_functions (it
> also eliminates the need for exporting most of the OMAP IOMMU API). So
> while I am ok with the current patch, I prefer consolidation than
> keeping the scalability alive, it can always be added if a need for that
> arises. What do you think?
I agree with your approach, but in the meantime we have a problem to solve.
How about applying this patch now (it goes in the right direction anyway), and
then removing the iommu functions when you will have time ?
> it also causes initialization
>
> > ordering issues by requiring the arch-specific operations module to be
> > loaded before any IOMMU user. This results in a probe breakage with the
> > OMAP3 ISP driver when not compiled as a module.
>
> This can be fixed if we make the current omap-iommu2.c as a
> subsys_initcall as well, right?
>
> regards
> Suman
>
> > Fix the problem by inverting the dependency. Instead of having the
> > omap-iommu2 module register itself to iommu-omap, make the iommu-omap
> > retrieve the omap-iommu2 operations structure directly when probing the
> > IOMMU device. This ensures that a probed IOMMU will always have valid
> > arch-specific operations.
> >
> > As the arch-specific operations pointer is now initialized at probe
> > time, this change requires turning it from a global variable into a
> > per-device variable.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > ---
> >
> > drivers/iommu/omap-iommu-debug.c | 6 ++-
> > drivers/iommu/omap-iommu.c | 94 +++++++++++++----------------------
> > drivers/iommu/omap-iommu.h | 10 ++++-
> > drivers/iommu/omap-iommu2.c | 18 +-------
> > 4 files changed, 45 insertions(+), 83 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
2014-09-09 21:59 ` Laurent Pinchart
@ 2014-09-09 22:31 ` Suman Anna
[not found] ` <540F7FD0.4010109-l0cyMroinI0@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Suman Anna @ 2014-09-09 22:31 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: iommu, linux-omap, Florian Vaussard, Joerg Roedel
Hi Laurent,
> On Tuesday 09 September 2014 16:33:11 Suman Anna wrote:
>> On 09/09/2014 10:45 AM, Laurent Pinchart wrote:
>>> The OMAP IOMMU driver supports both the OMAP1 and OMAP2+ IOMMU variants
>>> by splitting the driver into a core module and a thin arch-specific
>>> operations module.
>>>
>>> (In practice only the OMAP2+ module omap-iommu2 is implemented, but
>>> let's not denigrate the effort.)
>>
>> Thank you for the patch. I had something similar in my list of cleanup
>> TODO items on the OMAP IOMMU driver, but I was intending to cut down
>> more and consolidate the omap-iommu.c and omap-iommu2.c files into a
>> single one.
>>
>> This had been the case from the introduction of the driver going back to
>> v2.6.30, and OMAP1 was never added and I doubt it would be added anytime
>> in the foreseeable future.
>>
>>> The arch-specific operations module registers itself with the OMAP IOMMU
>>> core module at initialization time. This initializes a module global
>>> arch-specific operations pointer, used at runtime by the IOMMU
>>> instances.
>>>
>>> This scheme causes several issues. In addition to making it impossible
>>> to support different OMAP IOMMU types in a single system (which in all
>>> fairness is quite unlikely to happen),
>>
>> Yep, except for a few enhancements (like reporting Fault PC address on
>> OMAP4 DSPs, and dropping both endianness support), the core IOMMU
>> functionality hasn't changed much between OMAP2 and the latest OMAP4+
>> SoCs. So, my plan was to completely get rid of the iommu_functions (it
>> also eliminates the need for exporting most of the OMAP IOMMU API). So
>> while I am ok with the current patch, I prefer consolidation than
>> keeping the scalability alive, it can always be added if a need for that
>> arises. What do you think?
>
> I agree with your approach, but in the meantime we have a problem to solve.
> How about applying this patch now (it goes in the right direction anyway), and
> then removing the iommu functions when you will have time ?
Can you give the subsys_initcall solution a try to see if that resolves
the problem at hand? That would be a much smaller change, if that
doesn't work we can go with this patch.
I will work on my cleanup list for 3.19.
regards
Suman
>
>> it also causes initialization
>>
>>> ordering issues by requiring the arch-specific operations module to be
>>> loaded before any IOMMU user. This results in a probe breakage with the
>>> OMAP3 ISP driver when not compiled as a module.
>>
>> This can be fixed if we make the current omap-iommu2.c as a
>> subsys_initcall as well, right?
>>
>> regards
>> Suman
>>
>>> Fix the problem by inverting the dependency. Instead of having the
>>> omap-iommu2 module register itself to iommu-omap, make the iommu-omap
>>> retrieve the omap-iommu2 operations structure directly when probing the
>>> IOMMU device. This ensures that a probed IOMMU will always have valid
>>> arch-specific operations.
>>>
>>> As the arch-specific operations pointer is now initialized at probe
>>> time, this change requires turning it from a global variable into a
>>> per-device variable.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>
>>> drivers/iommu/omap-iommu-debug.c | 6 ++-
>>> drivers/iommu/omap-iommu.c | 94 +++++++++++++----------------------
>>> drivers/iommu/omap-iommu.h | 10 ++++-
>>> drivers/iommu/omap-iommu2.c | 18 +-------
>>> 4 files changed, 45 insertions(+), 83 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
[not found] ` <540F7FD0.4010109-l0cyMroinI0@public.gmane.org>
@ 2014-09-20 13:12 ` Laurent Pinchart
2014-09-23 16:04 ` Suman Anna
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2014-09-20 13:12 UTC (permalink / raw)
To: Suman Anna
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
Hi Suman,
On Tuesday 09 September 2014 17:31:44 Suman Anna wrote:
> > On Tuesday 09 September 2014 16:33:11 Suman Anna wrote:
> >> On 09/09/2014 10:45 AM, Laurent Pinchart wrote:
> >>> The OMAP IOMMU driver supports both the OMAP1 and OMAP2+ IOMMU variants
> >>> by splitting the driver into a core module and a thin arch-specific
> >>> operations module.
> >>>
> >>> (In practice only the OMAP2+ module omap-iommu2 is implemented, but
> >>> let's not denigrate the effort.)
> >>
> >> Thank you for the patch. I had something similar in my list of cleanup
> >> TODO items on the OMAP IOMMU driver, but I was intending to cut down
> >> more and consolidate the omap-iommu.c and omap-iommu2.c files into a
> >> single one.
> >>
> >> This had been the case from the introduction of the driver going back to
> >> v2.6.30, and OMAP1 was never added and I doubt it would be added anytime
> >> in the foreseeable future.
> >>
> >>> The arch-specific operations module registers itself with the OMAP IOMMU
> >>> core module at initialization time. This initializes a module global
> >>> arch-specific operations pointer, used at runtime by the IOMMU
> >>> instances.
> >>>
> >>> This scheme causes several issues. In addition to making it impossible
> >>> to support different OMAP IOMMU types in a single system (which in all
> >>> fairness is quite unlikely to happen),
> >>
> >> Yep, except for a few enhancements (like reporting Fault PC address on
> >> OMAP4 DSPs, and dropping both endianness support), the core IOMMU
> >> functionality hasn't changed much between OMAP2 and the latest OMAP4+
> >> SoCs. So, my plan was to completely get rid of the iommu_functions (it
> >> also eliminates the need for exporting most of the OMAP IOMMU API). So
> >> while I am ok with the current patch, I prefer consolidation than
> >> keeping the scalability alive, it can always be added if a need for that
> >> arises. What do you think?
> >
> > I agree with your approach, but in the meantime we have a problem to
> > solve.
> > How about applying this patch now (it goes in the right direction anyway),
> > and then removing the iommu functions when you will have time ?
>
> Can you give the subsys_initcall solution a try to see if that resolves
> the problem at hand? That would be a much smaller change, if that
> doesn't work we can go with this patch.
It seems to work.
> I will work on my cleanup list for 3.19.
Does that schedule still hold ? If so I'll submit a simple subsys_initcall()
patch for v3.18.
> >> it also causes initialization
> >>
> >>> ordering issues by requiring the arch-specific operations module to be
> >>> loaded before any IOMMU user. This results in a probe breakage with the
> >>> OMAP3 ISP driver when not compiled as a module.
> >>
> >> This can be fixed if we make the current omap-iommu2.c as a
> >> subsys_initcall as well, right?
> >>
> >> regards
> >> Suman
> >>
> >>> Fix the problem by inverting the dependency. Instead of having the
> >>> omap-iommu2 module register itself to iommu-omap, make the iommu-omap
> >>> retrieve the omap-iommu2 operations structure directly when probing the
> >>> IOMMU device. This ensures that a probed IOMMU will always have valid
> >>> arch-specific operations.
> >>>
> >>> As the arch-specific operations pointer is now initialized at probe
> >>> time, this change requires turning it from a global variable into a
> >>> per-device variable.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> >>> ---
> >>>
> >>> drivers/iommu/omap-iommu-debug.c | 6 ++-
> >>> drivers/iommu/omap-iommu.c | 94 +++++++++++++--------------------
> >>> drivers/iommu/omap-iommu.h | 10 ++++-
> >>> drivers/iommu/omap-iommu2.c | 18 +-------
> >>> 4 files changed, 45 insertions(+), 83 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
2014-09-20 13:12 ` Laurent Pinchart
@ 2014-09-23 16:04 ` Suman Anna
0 siblings, 0 replies; 10+ messages in thread
From: Suman Anna @ 2014-09-23 16:04 UTC (permalink / raw)
To: Laurent Pinchart
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
Hi Laurent,
>
> On Tuesday 09 September 2014 17:31:44 Suman Anna wrote:
>>> On Tuesday 09 September 2014 16:33:11 Suman Anna wrote:
>>>> On 09/09/2014 10:45 AM, Laurent Pinchart wrote:
>>>>> The OMAP IOMMU driver supports both the OMAP1 and OMAP2+ IOMMU variants
>>>>> by splitting the driver into a core module and a thin arch-specific
>>>>> operations module.
>>>>>
>>>>> (In practice only the OMAP2+ module omap-iommu2 is implemented, but
>>>>> let's not denigrate the effort.)
>>>>
>>>> Thank you for the patch. I had something similar in my list of cleanup
>>>> TODO items on the OMAP IOMMU driver, but I was intending to cut down
>>>> more and consolidate the omap-iommu.c and omap-iommu2.c files into a
>>>> single one.
>>>>
>>>> This had been the case from the introduction of the driver going back to
>>>> v2.6.30, and OMAP1 was never added and I doubt it would be added anytime
>>>> in the foreseeable future.
>>>>
>>>>> The arch-specific operations module registers itself with the OMAP IOMMU
>>>>> core module at initialization time. This initializes a module global
>>>>> arch-specific operations pointer, used at runtime by the IOMMU
>>>>> instances.
>>>>>
>>>>> This scheme causes several issues. In addition to making it impossible
>>>>> to support different OMAP IOMMU types in a single system (which in all
>>>>> fairness is quite unlikely to happen),
>>>>
>>>> Yep, except for a few enhancements (like reporting Fault PC address on
>>>> OMAP4 DSPs, and dropping both endianness support), the core IOMMU
>>>> functionality hasn't changed much between OMAP2 and the latest OMAP4+
>>>> SoCs. So, my plan was to completely get rid of the iommu_functions (it
>>>> also eliminates the need for exporting most of the OMAP IOMMU API). So
>>>> while I am ok with the current patch, I prefer consolidation than
>>>> keeping the scalability alive, it can always be added if a need for that
>>>> arises. What do you think?
>>>
>>> I agree with your approach, but in the meantime we have a problem to
>>> solve.
>>> How about applying this patch now (it goes in the right direction anyway),
>>> and then removing the iommu functions when you will have time ?
>>
>> Can you give the subsys_initcall solution a try to see if that resolves
>> the problem at hand? That would be a much smaller change, if that
>> doesn't work we can go with this patch.
>
> It seems to work.
>
>> I will work on my cleanup list for 3.19.
>
> Does that schedule still hold ? If so I'll submit a simple subsys_initcall()
> patch for v3.18.
Yes, that would be great. I am working on the patches and will post them
in a couple of days.
regards
Suman
>
>>>> it also causes initialization
>>>>
>>>>> ordering issues by requiring the arch-specific operations module to be
>>>>> loaded before any IOMMU user. This results in a probe breakage with the
>>>>> OMAP3 ISP driver when not compiled as a module.
>>>>
>>>> This can be fixed if we make the current omap-iommu2.c as a
>>>> subsys_initcall as well, right?
>>>>
>>>> regards
>>>> Suman
>>>>
>>>>> Fix the problem by inverting the dependency. Instead of having the
>>>>> omap-iommu2 module register itself to iommu-omap, make the iommu-omap
>>>>> retrieve the omap-iommu2 operations structure directly when probing the
>>>>> IOMMU device. This ensures that a probed IOMMU will always have valid
>>>>> arch-specific operations.
>>>>>
>>>>> As the arch-specific operations pointer is now initialized at probe
>>>>> time, this change requires turning it from a global variable into a
>>>>> per-device variable.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>>>> ---
>>>>>
>>>>> drivers/iommu/omap-iommu-debug.c | 6 ++-
>>>>> drivers/iommu/omap-iommu.c | 94 +++++++++++++--------------------
>>>>> drivers/iommu/omap-iommu.h | 10 ++++-
>>>>> drivers/iommu/omap-iommu2.c | 18 +-------
>>>>> 4 files changed, 45 insertions(+), 83 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iommu/omap: Remove omap_iommu unused owner field
[not found] ` <1410277545-32157-3-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
@ 2014-09-25 13:57 ` Joerg Roedel
0 siblings, 0 replies; 10+ messages in thread
From: Joerg Roedel @ 2014-09-25 13:57 UTC (permalink / raw)
To: Laurent Pinchart
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard
On Tue, Sep 09, 2014 at 06:45:45PM +0300, Laurent Pinchart wrote:
> The owner field is never set. Remove it.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
> drivers/iommu/omap-iommu.c | 11 -----------
> drivers/iommu/omap-iommu.h | 1 -
> 2 files changed, 12 deletions(-)
Applied this one to arm/omap, thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-09-25 13:57 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-09 15:45 [PATCH 0/2] OMAP IOMMU cleanups Laurent Pinchart
[not found] ` <1410277545-32157-1-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
2014-09-09 15:45 ` [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2 Laurent Pinchart
[not found] ` <1410277545-32157-2-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
2014-09-09 21:33 ` Suman Anna
[not found] ` <540F7217.3070501-l0cyMroinI0@public.gmane.org>
2014-09-09 21:59 ` Laurent Pinchart
2014-09-09 22:31 ` Suman Anna
[not found] ` <540F7FD0.4010109-l0cyMroinI0@public.gmane.org>
2014-09-20 13:12 ` Laurent Pinchart
2014-09-23 16:04 ` Suman Anna
2014-09-09 15:45 ` [PATCH 2/2] iommu/omap: Remove omap_iommu unused owner field Laurent Pinchart
2014-09-09 21:41 ` Suman Anna
[not found] ` <1410277545-32157-3-git-send-email-laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
2014-09-25 13:57 ` Joerg Roedel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.