| Message ID | 1421399592-7482-18-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marek, Thank you for the patch. On Friday 16 January 2015 10:13:11 Marek Szyprowski wrote: > This patch introduces IOMMU_OF_DECLARE-based initialization to the > driver, which replaces subsys_initcall-based procedure. > exynos_iommu_of_setup ensures that each sysmmu controller is probed > before its master device. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index c53cc8f61176..ea2659159e63 100644 > --- a/drivers/iommu/exynos-iommu.c > +++ b/drivers/iommu/exynos-iommu.c > @@ -13,16 +13,21 @@ > #endif > > #include <linux/clk.h> > +#include <linux/dma-mapping.h> > #include <linux/err.h> > #include <linux/io.h> > #include <linux/iommu.h> > #include <linux/interrupt.h> > #include <linux/list.h> > +#include <linux/of.h> > +#include <linux/of_iommu.h> > +#include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/slab.h> > > #include <asm/cacheflush.h> > +#include <asm/dma-iommu.h> > #include <asm/pgtable.h> > > typedef u32 sysmmu_iova_t; > @@ -1084,6 +1089,8 @@ static const struct iommu_ops exynos_iommu_ops = { > .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, > }; > > +static int init_done; > + > static int __init exynos_iommu_init(void) > { > int ret; > @@ -1116,6 +1123,8 @@ static int __init exynos_iommu_init(void) > goto err_set_iommu; > } > > + init_done = true; > + > return 0; > err_set_iommu: > kmem_cache_free(lv2table_kmem_cache, zero_lv2_table); > @@ -1125,4 +1134,21 @@ err_reg_driver: > kmem_cache_destroy(lv2table_kmem_cache); > return ret; > } > -subsys_initcall(exynos_iommu_init); > + > +static int __init exynos_iommu_of_setup(struct device_node *np) > +{ > + struct platform_device *pdev; > + > + if (!init_done) > + exynos_iommu_init(); > + > + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); > + if (IS_ERR(pdev)) > + return PTR_ERR(pdev); This feels like a hack to me. What happens here is that you're using the IOMMU_OF_DECLARE mechanism to make sure that the iommu platform device will be created and registered before the normal OF bus populate mechanism kicks in, thus ensuring that the iommu gets probed before other devices. In practice this is pretty similar to using different init levels, which is what Will's patch set was trying to avoid in the first place. Creating a new kind of init levels mechanism doesn't sound very good to me. The existing exynos-iommu driver is based on classic instantiation of a platform device from DT, using the normal device probing mechanism. As such it relies on the availability of a struct device for various helper functions. I thus understand why you want a struct device being registered for the iommu, instead of initializing the device right from the exynos_iommu_of_setup() function without a corresponding struct device being registered. This leads me to question whether we should really introduce IOMMU_OF_DECLARE. Using regular deferred probing seems more and more like a better solution to me. > + of_iommu_set_ops(np, &exynos_iommu_ops); > + return 0; > +} > + > +IOMMU_OF_DECLARE(exynos_iommu_of, "samsung,exynos-sysmmu", > + exynos_iommu_of_setup);
On Mon, Jan 19, 2015 at 01:11:07AM +0000, Laurent Pinchart wrote: > On Friday 16 January 2015 10:13:11 Marek Szyprowski wrote: > > This patch introduces IOMMU_OF_DECLARE-based initialization to the > > driver, which replaces subsys_initcall-based procedure. > > exynos_iommu_of_setup ensures that each sysmmu controller is probed > > before its master device. [...] > > +static int __init exynos_iommu_of_setup(struct device_node *np) > > +{ > > + struct platform_device *pdev; > > + > > + if (!init_done) > > + exynos_iommu_init(); > > + > > + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); > > + if (IS_ERR(pdev)) > > + return PTR_ERR(pdev); > > This feels like a hack to me. What happens here is that you're using the > IOMMU_OF_DECLARE mechanism to make sure that the iommu platform device will be > created and registered before the normal OF bus populate mechanism kicks in, > thus ensuring that the iommu gets probed before other devices. In practice > this is pretty similar to using different init levels, which is what Will's > patch set was trying to avoid in the first place. Creating a new kind of init > levels mechanism doesn't sound very good to me. > > The existing exynos-iommu driver is based on classic instantiation of a > platform device from DT, using the normal device probing mechanism. As such it > relies on the availability of a struct device for various helper functions. I > thus understand why you want a struct device being registered for the iommu, > instead of initializing the device right from the exynos_iommu_of_setup() > function without a corresponding struct device being registered. > > This leads me to question whether we should really introduce IOMMU_OF_DECLARE. > Using regular deferred probing seems more and more like a better solution to > me. We seem to be going round and round on this argument. I said before that I'm not against changing this [1], but somebody would need to propose patches, which hasn't happened in recent history. Arnd also makes some good arguments against using probing [2], which would need further discussion. Basically, it looks like there are two sides to this argument and I don't see anything changing without patch proposals. The only thing that the current discussions seem to be achieving is blocking people like Marek, who are trying to make use of what we have in mainline today! Will [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310783.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310992.html
Hi Will, On Monday 19 January 2015 11:33:31 Will Deacon wrote: > On Mon, Jan 19, 2015 at 01:11:07AM +0000, Laurent Pinchart wrote: > > On Friday 16 January 2015 10:13:11 Marek Szyprowski wrote: > >> This patch introduces IOMMU_OF_DECLARE-based initialization to the > >> driver, which replaces subsys_initcall-based procedure. > >> exynos_iommu_of_setup ensures that each sysmmu controller is probed > >> before its master device. > > [...] > > >> +static int __init exynos_iommu_of_setup(struct device_node *np) > >> +{ > >> + struct platform_device *pdev; > >> + > >> + if (!init_done) > >> + exynos_iommu_init(); > >> + > >> + pdev = of_platform_device_create(np, NULL, > >> platform_bus_type.dev_root); > >> + if (IS_ERR(pdev)) > >> + return PTR_ERR(pdev); > > > > This feels like a hack to me. What happens here is that you're using the > > IOMMU_OF_DECLARE mechanism to make sure that the iommu platform device > > will be created and registered before the normal OF bus populate > > mechanism kicks in, thus ensuring that the iommu gets probed before other > > devices. In practice this is pretty similar to using different init > > levels, which is what Will's patch set was trying to avoid in the first > > place. Creating a new kind of init levels mechanism doesn't sound very > > good to me. > > > > The existing exynos-iommu driver is based on classic instantiation of a > > platform device from DT, using the normal device probing mechanism. As > > such it relies on the availability of a struct device for various helper > > functions. I thus understand why you want a struct device being > > registered for the iommu, instead of initializing the device right from > > the exynos_iommu_of_setup() function without a corresponding struct > > device being registered. > > > > This leads me to question whether we should really introduce > > IOMMU_OF_DECLARE. Using regular deferred probing seems more and more like > > a better solution to me. > > We seem to be going round and round on this argument. I said before that > I'm not against changing this [1], but somebody would need to propose > patches, which hasn't happened in recent history. > > Arnd also makes some good arguments against using probing [2], which would > need further discussion. > > Basically, it looks like there are two sides to this argument and I don't > see anything changing without patch proposals. The only thing that the > current discussions seem to be achieving is blocking people like Marek, > who are trying to make use of what we have in mainline today! To be perfectly clear, I won't block patches here without submitting a counterproposal (unless there's something fundamentally wrong of course). I still believe the deferred probe approach should be given at least a try, but as I don't have time to implement that myself now, I won't try to block anything. > [1] > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310783. > html > [2] > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310992. > html
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index c53cc8f61176..ea2659159e63 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -13,16 +13,21 @@ #endif #include <linux/clk.h> +#include <linux/dma-mapping.h> #include <linux/err.h> #include <linux/io.h> #include <linux/iommu.h> #include <linux/interrupt.h> #include <linux/list.h> +#include <linux/of.h> +#include <linux/of_iommu.h> +#include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/slab.h> #include <asm/cacheflush.h> +#include <asm/dma-iommu.h> #include <asm/pgtable.h> typedef u32 sysmmu_iova_t; @@ -1084,6 +1089,8 @@ static const struct iommu_ops exynos_iommu_ops = { .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, }; +static int init_done; + static int __init exynos_iommu_init(void) { int ret; @@ -1116,6 +1123,8 @@ static int __init exynos_iommu_init(void) goto err_set_iommu; } + init_done = true; + return 0; err_set_iommu: kmem_cache_free(lv2table_kmem_cache, zero_lv2_table); @@ -1125,4 +1134,21 @@ err_reg_driver: kmem_cache_destroy(lv2table_kmem_cache); return ret; } -subsys_initcall(exynos_iommu_init); + +static int __init exynos_iommu_of_setup(struct device_node *np) +{ + struct platform_device *pdev; + + if (!init_done) + exynos_iommu_init(); + + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); + if (IS_ERR(pdev)) + return PTR_ERR(pdev); + + of_iommu_set_ops(np, &exynos_iommu_ops); + return 0; +} + +IOMMU_OF_DECLARE(exynos_iommu_of, "samsung,exynos-sysmmu", + exynos_iommu_of_setup);
This patch introduces IOMMU_OF_DECLARE-based initialization to the driver, which replaces subsys_initcall-based procedure. exynos_iommu_of_setup ensures that each sysmmu controller is probed before its master device. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)