[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

[Magnus Damm]

iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

[PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y
[PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code
[PATCH 03/04] iommu/ipmmu-vmsa: Break out domain allocation code
[PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

This series updates the IPMMU driver with CONFIG_IOMMU_DMA=y support
which is present in mainline for 64-bit ARM while experimental support
has been posted for 32-bit ARM thanks to Marek Szyprowski:

[RFC 0/3] Unify IOMMU-based DMA-mapping code for ARM and ARM64

Most of the patches in this series based on code earlier included in
the series below but has been reworked to also fit on 32-bit ARM:

[PATCH/RFC 00/10] iommu/ipmmu-vmsa: Experimental r8a7795 support

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Built on top of next-20160315
 Depends on [PATCH v2 00/04] iommu/ipmmu-vmsa: IPMMU multi-arch update V2

 drivers/iommu/ipmmu-vmsa.c |  238 ++++++++++++++++++++++++++++++++------------
 1 file changed, 174 insertions(+), 64 deletions(-)

[PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Instead of assuming that CONFIG_ARM=y also means CONFIG_IOMMU_DMA=n,
convert the #ifdefs to take CONFIG_IOMMU_DMA into consideration
so 32-bit ARM can make use of CONFIG_IOMMU_DMA=y as well once those
bits are in place.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- 0007/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-03-15 21:05:40.590513000 +0900
@@ -22,7 +22,7 @@
 #include <linux/sizes.h>
 #include <linux/slab.h>
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 #include <asm/dma-iommu.h>
 #include <asm/pgalloc.h>
 #endif
@@ -40,7 +40,7 @@ struct ipmmu_vmsa_device {
 	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
 	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 	struct dma_iommu_mapping *mapping;
 #endif
 };
@@ -619,7 +619,7 @@ static int ipmmu_find_utlbs(struct ipmmu
 	return 0;
 }
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device *mmu)
 {
 	int ret;

[PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Break out the utlb parsing code and dev_data allocation into a
separate function. This is preparation for future code sharing.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |  125 ++++++++++++++++++++++++--------------------
 1 file changed, 70 insertions(+), 55 deletions(-)

--- 0008/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-03-15 21:08:20.180513000 +0900
@@ -619,6 +619,69 @@ static int ipmmu_find_utlbs(struct ipmmu
 	return 0;
 }
 
+static int ipmmu_init_platform_device(struct device *dev,
+				      struct iommu_group *group)
+{
+	struct ipmmu_vmsa_archdata *archdata;
+	struct ipmmu_vmsa_device *mmu;
+	unsigned int *utlbs;
+	unsigned int i;
+	int num_utlbs;
+	int ret = -ENODEV;
+
+	/* Find the master corresponding to the device. */
+
+	num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
+					       "#iommu-cells");
+	if (num_utlbs < 0)
+		return -ENODEV;
+
+	utlbs = kcalloc(num_utlbs, sizeof(*utlbs), GFP_KERNEL);
+	if (!utlbs)
+		return -ENOMEM;
+
+	spin_lock(&ipmmu_devices_lock);
+
+	list_for_each_entry(mmu, &ipmmu_devices, list) {
+		ret = ipmmu_find_utlbs(mmu, dev, utlbs, num_utlbs);
+		if (!ret) {
+			/*
+			 * TODO Take a reference to the MMU to protect
+			 * against device removal.
+			 */
+			break;
+		}
+	}
+
+	spin_unlock(&ipmmu_devices_lock);
+
+	if (ret < 0)
+		goto error;
+
+	for (i = 0; i < num_utlbs; ++i) {
+		if (utlbs[i] >= mmu->num_utlbs) {
+			ret = -EINVAL;
+			goto error;
+		}
+	}
+
+	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
+	if (!archdata) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	archdata->mmu = mmu;
+	archdata->utlbs = utlbs;
+	archdata->num_utlbs = num_utlbs;
+	dev->archdata.iommu = archdata;
+	return 0;
+
+error:
+	kfree(utlbs);
+	return ret;
+}
+
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device *mmu)
 {
@@ -675,13 +738,8 @@ static inline void ipmmu_release_mapping
 
 static int ipmmu_add_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata;
-	struct ipmmu_vmsa_device *mmu;
-	struct iommu_group *group = NULL;
-	unsigned int *utlbs;
-	unsigned int i;
-	int num_utlbs;
-	int ret = -ENODEV;
+	struct iommu_group *group;
+	int ret;
 
 	if (dev->archdata.iommu) {
 		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
@@ -689,42 +747,6 @@ static int ipmmu_add_device(struct devic
 		return -EINVAL;
 	}
 
-	/* Find the master corresponding to the device. */
-
-	num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
-					       "#iommu-cells");
-	if (num_utlbs < 0)
-		return -ENODEV;
-
-	utlbs = kcalloc(num_utlbs, sizeof(*utlbs), GFP_KERNEL);
-	if (!utlbs)
-		return -ENOMEM;
-
-	spin_lock(&ipmmu_devices_lock);
-
-	list_for_each_entry(mmu, &ipmmu_devices, list) {
-		ret = ipmmu_find_utlbs(mmu, dev, utlbs, num_utlbs);
-		if (!ret) {
-			/*
-			 * TODO Take a reference to the MMU to protect
-			 * against device removal.
-			 */
-			break;
-		}
-	}
-
-	spin_unlock(&ipmmu_devices_lock);
-
-	if (ret < 0)
-		return -ENODEV;
-
-	for (i = 0; i < num_utlbs; ++i) {
-		if (utlbs[i] >= mmu->num_utlbs) {
-			ret = -EINVAL;
-			goto error;
-		}
-	}
-
 	/* Create a device group and add the device to it. */
 	group = iommu_group_alloc();
 	if (IS_ERR(group)) {
@@ -742,27 +764,20 @@ static int ipmmu_add_device(struct devic
 		goto error;
 	}
 
-	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
-	if (!archdata) {
-		ret = -ENOMEM;
+	ret = ipmmu_init_platform_device(dev, group);
+	if (ret < 0) {
+		dev_err(dev, "Failed to init platform device\n");
 		goto error;
 	}
 
-	archdata->mmu = mmu;
-	archdata->utlbs = utlbs;
-	archdata->num_utlbs = num_utlbs;
-	dev->archdata.iommu = archdata;
-
-	ret = ipmmu_map_attach(dev, mmu);
+	ret = ipmmu_map_attach(dev, ((struct ipmmu_vmsa_archdata *)
+				     dev->archdata.iommu)->mmu);
 	if (ret < 0)
 		goto error;
 
 	return 0;
 
 error:
-	kfree(dev->archdata.iommu);
-	kfree(utlbs);
-
 	dev->archdata.iommu = NULL;
 
 	if (!IS_ERR_OR_NULL(group))

[PATCH 03/04] iommu/ipmmu-vmsa: Break out domain allocation code

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Break out the domain allocation code into a separate function.
This is preparation for future code sharing.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

--- 0014/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-14 19:23:22.210513000 +0900
@@ -457,13 +457,10 @@ static irqreturn_t ipmmu_domain_irq(int
  * IOMMU Operations
  */
 
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
 {
 	struct ipmmu_vmsa_domain *domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
@@ -473,6 +470,14 @@ static struct iommu_domain *ipmmu_domain
 	return &domain->io_domain;
 }
 
+static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+{
+	if (type != IOMMU_DOMAIN_UNMANAGED)
+		return NULL;
+
+	return __ipmmu_domain_alloc(type);
+}
+
 static void ipmmu_domain_free(struct iommu_domain *io_domain)
 {
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);

[PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Introduce a new set of iommu_ops suitable for 64-bit ARM
as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. The ->of_xlate()
callback is needed by the code exported by of_iommu.h and
it is wrapped in #ifdefs to also compile of x86_64.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |   94 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 2 deletions(-)

--- 0011/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-03-16 01:35:06.990513000 +0900
@@ -10,6 +10,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/delay.h>
+#include <linux/dma-iommu.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/export.h>
@@ -804,7 +805,7 @@ static void ipmmu_remove_device(struct d
 	dev->archdata.iommu = NULL;
 }
 
-static const struct iommu_ops ipmmu_ops = {
+static const struct iommu_ops __maybe_unused ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc,
 	.domain_free = ipmmu_domain_free,
 	.attach_dev = ipmmu_attach_device,
@@ -818,6 +819,92 @@ static const struct iommu_ops ipmmu_ops
 	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
 };
 
+static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
+{
+	struct iommu_domain *io_domain;
+
+	if (type != IOMMU_DOMAIN_DMA)
+		return NULL;
+
+	io_domain = __ipmmu_domain_alloc(type);
+	if (io_domain)
+		iommu_get_dma_cookie(io_domain);
+
+	return io_domain;
+}
+
+static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
+{
+	iommu_put_dma_cookie(io_domain);
+	ipmmu_domain_free(io_domain);
+}
+
+static int ipmmu_add_device_dma(struct device *dev)
+{
+	struct iommu_group *group;
+
+	/* only accept devices with iommus property */
+	if (of_count_phandle_with_args(dev->of_node, "iommus",
+				       "#iommu-cells") < 0)
+		return -ENODEV;
+
+	group = iommu_group_get_for_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	return 0;
+}
+
+static void ipmmu_remove_device_dma(struct device *dev)
+{
+	iommu_group_remove_device(dev);
+}
+
+static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
+{
+	struct iommu_group *group;
+	int ret;
+
+	group = generic_device_group(dev);
+	if (IS_ERR(group))
+		return group;
+
+	ret = ipmmu_init_platform_device(dev, group);
+	if (ret) {
+		iommu_group_put(group);
+		group = ERR_PTR(ret);
+	}
+
+	return group;
+}
+
+#ifdef CONFIG_OF_IOMMU
+static int ipmmu_of_xlate_dma(struct device *dev,
+			      struct of_phandle_args *spec)
+{
+	/* dummy callback to satisfy of_iommu_configure() */
+	return 0;
+}
+#endif
+
+static const struct iommu_ops __maybe_unused ipmmu_ops_dma = {
+	.domain_alloc = ipmmu_domain_alloc_dma,
+	.domain_free = ipmmu_domain_free_dma,
+	.attach_dev = ipmmu_attach_device,
+	.detach_dev = ipmmu_detach_device,
+	.map = ipmmu_map,
+	.unmap = ipmmu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.iova_to_phys = ipmmu_iova_to_phys,
+	.add_device = ipmmu_add_device_dma,
+	.remove_device = ipmmu_remove_device_dma,
+	.device_group = ipmmu_device_group_dma,
+	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
+#ifdef CONFIG_OF_IOMMU
+	.of_xlate = ipmmu_of_xlate_dma,
+#endif
+};
+
 /* -----------------------------------------------------------------------------
  * Probe/remove and init
  */
@@ -929,14 +1016,17 @@ static struct platform_driver ipmmu_driv
 
 static int __init ipmmu_init(void)
 {
+	const struct iommu_ops *ops;
 	int ret;
 
 	ret = platform_driver_register(&ipmmu_driver);
 	if (ret < 0)
 		return ret;
 
+	ops = IS_ENABLED(CONFIG_IOMMU_DMA) ? &ipmmu_ops_dma : &ipmmu_ops;
+
 	if (!iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
+		bus_set_iommu(&platform_bus_type, ops);
 
 	return 0;
 }

Re: [PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Wednesday 16 March 2016 02:05:10 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Introduce a new set of iommu_ops suitable for 64-bit ARM
> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. The ->of_xlate()
> callback is needed by the code exported by of_iommu.h and
> it is wrapped in #ifdefs to also compile of x86_64.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

Nice work, thanks a lot. With the two comments below addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  drivers/iommu/ipmmu-vmsa.c |   94 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 2 deletions(-)
> 
> --- 0011/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-03-16 01:35:06.990513000 +0900
> @@ -10,6 +10,7 @@
> 
>  #include <linux/bitmap.h>
>  #include <linux/delay.h>
> +#include <linux/dma-iommu.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
> @@ -804,7 +805,7 @@ static void ipmmu_remove_device(struct d
>  	dev->archdata.iommu = NULL;
>  }
> 
> -static const struct iommu_ops ipmmu_ops = {
> +static const struct iommu_ops __maybe_unused ipmmu_ops = {
>  	.domain_alloc = ipmmu_domain_alloc,
>  	.domain_free = ipmmu_domain_free,
>  	.attach_dev = ipmmu_attach_device,
> @@ -818,6 +819,92 @@ static const struct iommu_ops ipmmu_ops
>  	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
>  };
> 
> +static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
> +{
> +	struct iommu_domain *io_domain;
> +
> +	if (type != IOMMU_DOMAIN_DMA)
> +		return NULL;
> +
> +	io_domain = __ipmmu_domain_alloc(type);
> +	if (io_domain)
> +		iommu_get_dma_cookie(io_domain);
> +
> +	return io_domain;
> +}
> +
> +static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
> +{
> +	iommu_put_dma_cookie(io_domain);
> +	ipmmu_domain_free(io_domain);
> +}
> +
> +static int ipmmu_add_device_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +
> +	/* only accept devices with iommus property */

Nitpicking, the driver capitalizes the first letter of the sentence in 
comments and includes a period at the end. Could you please follow the same 
style for consistency ?

> +	if (of_count_phandle_with_args(dev->of_node, "iommus",
> +				       "#iommu-cells") < 0)
> +		return -ENODEV;

Given that the iommu_group_get_for_dev() call will call the .device_group() 
operation that ends up calling ipmmu_init_platform_device(), do we need this 
check here ?

> +	group = iommu_group_get_for_dev(dev);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> +
> +	return 0;
> +}
> +
> +static void ipmmu_remove_device_dma(struct device *dev)
> +{
> +	iommu_group_remove_device(dev);
> +}
> +
> +static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +	int ret;
> +
> +	group = generic_device_group(dev);
> +	if (IS_ERR(group))
> +		return group;
> +
> +	ret = ipmmu_init_platform_device(dev, group);
> +	if (ret) {
> +		iommu_group_put(group);
> +		group = ERR_PTR(ret);
> +	}
> +
> +	return group;
> +}
> +
> +#ifdef CONFIG_OF_IOMMU
> +static int ipmmu_of_xlate_dma(struct device *dev,
> +			      struct of_phandle_args *spec)
> +{
> +	/* dummy callback to satisfy of_iommu_configure() */
> +	return 0;
> +}
> +#endif
> +
> +static const struct iommu_ops __maybe_unused ipmmu_ops_dma = {
> +	.domain_alloc = ipmmu_domain_alloc_dma,
> +	.domain_free = ipmmu_domain_free_dma,
> +	.attach_dev = ipmmu_attach_device,
> +	.detach_dev = ipmmu_detach_device,
> +	.map = ipmmu_map,
> +	.unmap = ipmmu_unmap,
> +	.map_sg = default_iommu_map_sg,
> +	.iova_to_phys = ipmmu_iova_to_phys,
> +	.add_device = ipmmu_add_device_dma,
> +	.remove_device = ipmmu_remove_device_dma,
> +	.device_group = ipmmu_device_group_dma,
> +	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
> +#ifdef CONFIG_OF_IOMMU
> +	.of_xlate = ipmmu_of_xlate_dma,
> +#endif
> +};
> +
>  /*
> ---------------------------------------------------------------------------
> -- * Probe/remove and init
>   */
> @@ -929,14 +1016,17 @@ static struct platform_driver ipmmu_driv
> 
>  static int __init ipmmu_init(void)
>  {
> +	const struct iommu_ops *ops;
>  	int ret;
> 
>  	ret = platform_driver_register(&ipmmu_driver);
>  	if (ret < 0)
>  		return ret;
> 
> +	ops = IS_ENABLED(CONFIG_IOMMU_DMA) ? &ipmmu_ops_dma : &ipmmu_ops;
> +
>  	if (!iommu_present(&platform_bus_type))
> -		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
> +		bus_set_iommu(&platform_bus_type, ops);
> 
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Robin Murphy]

On 15/03/16 17:05, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Introduce a new set of iommu_ops suitable for 64-bit ARM
> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. The ->of_xlate()
> callback is needed by the code exported by of_iommu.h and
> it is wrapped in #ifdefs to also compile of x86_64.
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
>
>   drivers/iommu/ipmmu-vmsa.c |   94 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 92 insertions(+), 2 deletions(-)
>
> --- 0011/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-03-16 01:35:06.990513000 +0900
> @@ -10,6 +10,7 @@
>
>   #include <linux/bitmap.h>
>   #include <linux/delay.h>
> +#include <linux/dma-iommu.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/err.h>
>   #include <linux/export.h>
> @@ -804,7 +805,7 @@ static void ipmmu_remove_device(struct d
>   	dev->archdata.iommu = NULL;
>   }
>
> -static const struct iommu_ops ipmmu_ops = {
> +static const struct iommu_ops __maybe_unused ipmmu_ops = {
>   	.domain_alloc = ipmmu_domain_alloc,
>   	.domain_free = ipmmu_domain_free,
>   	.attach_dev = ipmmu_attach_device,
> @@ -818,6 +819,92 @@ static const struct iommu_ops ipmmu_ops
>   	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
>   };
>
> +static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
> +{
> +	struct iommu_domain *io_domain;
> +
> +	if (type != IOMMU_DOMAIN_DMA)
> +		return NULL;
> +
> +	io_domain = __ipmmu_domain_alloc(type);
> +	if (io_domain)
> +		iommu_get_dma_cookie(io_domain);
> +
> +	return io_domain;
> +}
> +
> +static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
> +{
> +	iommu_put_dma_cookie(io_domain);
> +	ipmmu_domain_free(io_domain);
> +}
> +
> +static int ipmmu_add_device_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +
> +	/* only accept devices with iommus property */
> +	if (of_count_phandle_with_args(dev->of_node, "iommus",
> +				       "#iommu-cells") < 0)
> +		return -ENODEV;

It seems a bit weird to deliberately ignore the information of_xlate 
gives you, only to go off poking the DT to dredge it up manually later. 
Why not use of_xlate to create the archdata, then simply check for it 
here? (Ideally that should also be common with ARM, but I think you 
still get called in the wrong order there)

> +
> +	group = iommu_group_get_for_dev(dev);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> +
> +	return 0;
> +}
> +
> +static void ipmmu_remove_device_dma(struct device *dev)
> +{
> +	iommu_group_remove_device(dev);
> +}
> +
> +static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +	int ret;
> +
> +	group = generic_device_group(dev);
> +	if (IS_ERR(group))
> +		return group;
> +
> +	ret = ipmmu_init_platform_device(dev, group);
> +	if (ret) {
> +		iommu_group_put(group);
> +		group = ERR_PTR(ret);
> +	}
> +
> +	return group;
> +}
> +
> +#ifdef CONFIG_OF_IOMMU
> +static int ipmmu_of_xlate_dma(struct device *dev,
> +			      struct of_phandle_args *spec)
> +{
> +	/* dummy callback to satisfy of_iommu_configure() */
> +	return 0;
> +}
> +#endif
> +
> +static const struct iommu_ops __maybe_unused ipmmu_ops_dma = {
> +	.domain_alloc = ipmmu_domain_alloc_dma,
> +	.domain_free = ipmmu_domain_free_dma,
> +	.attach_dev = ipmmu_attach_device,
> +	.detach_dev = ipmmu_detach_device,
> +	.map = ipmmu_map,
> +	.unmap = ipmmu_unmap,
> +	.map_sg = default_iommu_map_sg,
> +	.iova_to_phys = ipmmu_iova_to_phys,
> +	.add_device = ipmmu_add_device_dma,
> +	.remove_device = ipmmu_remove_device_dma,
> +	.device_group = ipmmu_device_group_dma,
> +	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
> +#ifdef CONFIG_OF_IOMMU
> +	.of_xlate = ipmmu_of_xlate_dma,
> +#endif

Yup, I definitely think Arnd is right about eradicating these #ifdefs[1].

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel/2177390

> +};
> +
>   /* -----------------------------------------------------------------------------
>    * Probe/remove and init
>    */
> @@ -929,14 +1016,17 @@ static struct platform_driver ipmmu_driv
>
>   static int __init ipmmu_init(void)
>   {
> +	const struct iommu_ops *ops;
>   	int ret;
>
>   	ret = platform_driver_register(&ipmmu_driver);
>   	if (ret < 0)
>   		return ret;
>
> +	ops = IS_ENABLED(CONFIG_IOMMU_DMA) ? &ipmmu_ops_dma : &ipmmu_ops;
> +
>   	if (!iommu_present(&platform_bus_type))
> -		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
> +		bus_set_iommu(&platform_bus_type, ops);
>
>   	return 0;
>   }
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

Re: [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

[Laurent Pinchart]

Hi Magnus,

Thank you for the patches.

On Wednesday 16 March 2016 02:04:31 Magnus Damm wrote:
> iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> 
> [PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y
> [PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code
> [PATCH 03/04] iommu/ipmmu-vmsa: Break out domain allocation code
> [PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
> 
> This series updates the IPMMU driver with CONFIG_IOMMU_DMA=y support
> which is present in mainline for 64-bit ARM while experimental support
> has been posted for 32-bit ARM thanks to Marek Szyprowski:
> 
> [RFC 0/3] Unify IOMMU-based DMA-mapping code for ARM and ARM64
> 
> Most of the patches in this series based on code earlier included in
> the series below but has been reworked to also fit on 32-bit ARM:
> 
> [PATCH/RFC 00/10] iommu/ipmmu-vmsa: Experimental r8a7795 support
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

Great ! Thanks a lot for the work, I'm really happy to see this being 
addressed.

For patches 1/4 to 3/4,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

For patch 4/4, please see my comments in the reply to the patch.

> ---
> 
>  Built on top of next-20160315
>  Depends on [PATCH v2 00/04] iommu/ipmmu-vmsa: IPMMU multi-arch update V2
> 
>  drivers/iommu/ipmmu-vmsa.c |  238 ++++++++++++++++++++++++++++++----------
>  1 file changed, 174 insertions(+), 64 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

[Joerg Roedel]

On Wed, Mar 16, 2016 at 02:04:31AM +0900, Magnus Damm wrote:
> iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> 
> [PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y
> [PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code
> [PATCH 03/04] iommu/ipmmu-vmsa: Break out domain allocation code
> [PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

These patches do not apply to v4.6-rc2. Can you please rebase them,
address the comments and add the Reviewed-by's?


	Joerg