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@xxxxxxxxxxx>
> ---
> 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);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
