| Message ID | 1410277545-32157-2-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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(-) > > 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"); > -- 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
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@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(-)
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(-) > -- 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
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@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(-)
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@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(-) > -- 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
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");
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@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(-)