Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
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.pinch...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
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.pinch...@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(-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
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.pinch...@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; }
Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
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.pinch...@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
Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
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.pinch...@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(-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2
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.pinch...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html