[PATCH v2 0/7] fix bugs; enable iommu for ARM64

[PATCH v2 0/7] fix bugs; enable iommu for ARM64

[Shunqian Zheng]

This series patches mainly for ARM64 supporting.
To do this, it first add virtual iommu slave device which DRM can attach to,
convert DRM driver to use common iommu API instead of the ARM32
functions, and then use DMA API in iommu driver to map, to flush cache.

The v2 patches make a lot changes vs v1, so please forget the v1.

Shunqian Zheng (4):
  iommu/rockchip: support virtual iommu slave device
  ARM: dts: rockchip: add virtual iommu for display
  drm: rockchip: use common iommu api to attach iommu
  iommu/rockchip: use DMA API to map, to flush cache

Simon Xue (3):
  iommu/rockchip: fix devm_{request,free}_irq parameter
  iommu/rockchip: add map_sg callback for rk_iommu_ops
  iommu/rockchip: enable rockchip iommu on ARM64 platform

 arch/arm/boot/dts/rk3288.dtsi               |   6 ++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 ++++++++++++++++--------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
 drivers/iommu/Kconfig                       |   2 +-
 drivers/iommu/rockchip-iommu.c              | 151 ++++++++++++++++++----------
 5 files changed, 193 insertions(+), 97 deletions(-)

-- 
1.9.1

[PATCH v2 1/7] iommu/rockchip: fix devm_{request,free}_irq parameter

[Shunqian Zheng]

From: Simon Xue <xxm@rock-chips.com>

Even though the iommu shares irq with its master, using the *dev of iommu
instead of master's *dev for devm_{request,free}_irq makes things clear.

Signed-off-by: Simon Xue <xxm@rock-chips.com>
Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
---
 drivers/iommu/rockchip-iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index c7d6156..ec0ce62 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -807,7 +807,7 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 
 	iommu->domain = domain;
 
-	ret = devm_request_irq(dev, iommu->irq, rk_iommu_irq,
+	ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq,
 			       IRQF_SHARED, dev_name(dev), iommu);
 	if (ret)
 		return ret;
@@ -860,7 +860,7 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
 	}
 	rk_iommu_disable_stall(iommu);
 
-	devm_free_irq(dev, iommu->irq, iommu);
+	devm_free_irq(iommu->dev, iommu->irq, iommu);
 
 	iommu->domain = NULL;
 
-- 
1.9.1

Re: [PATCH v2 1/7] iommu/rockchip: fix devm_{request,free}_irq parameter

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
> From: Simon Xue <xxm@rock-chips.com>
>
> Even though the iommu shares irq with its master, using the *dev of iommu
> instead of master's *dev for devm_{request,free}_irq makes things clear.
>
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> ---
>  drivers/iommu/rockchip-iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

[PATCH v2 2/7] iommu/rockchip: add map_sg callback for rk_iommu_ops

[Shunqian Zheng]

From: Simon Xue <xxm@rock-chips.com>

The iommu_dma_alloc() in iommu/dma-iommu.c calls iommu_map_sg()
that requires the callback iommu_ops .map_sg(). Adding the
default_iommu_map_sg() to rockchip iommu accordingly.

Signed-off-by: Simon Xue <xxm@rock-chips.com>
Signed-off-by: Shunqian Zheng <xxm@rock-chips.com>
---
 drivers/iommu/rockchip-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index ec0ce62..3c16ec3 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1022,6 +1022,7 @@ static const struct iommu_ops rk_iommu_ops = {
 	.detach_dev = rk_iommu_detach_device,
 	.map = rk_iommu_map,
 	.unmap = rk_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
 	.add_device = rk_iommu_add_device,
 	.remove_device = rk_iommu_remove_device,
 	.iova_to_phys = rk_iommu_iova_to_phys,
-- 
1.9.1

Re: [PATCH v2 2/7] iommu/rockchip: add map_sg callback for rk_iommu_ops

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
> From: Simon Xue <xxm@rock-chips.com>
>
> The iommu_dma_alloc() in iommu/dma-iommu.c calls iommu_map_sg()
> that requires the callback iommu_ops .map_sg(). Adding the
> default_iommu_map_sg() to rockchip iommu accordingly.
>
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> Signed-off-by: Shunqian Zheng <xxm@rock-chips.com>
> ---
>  drivers/iommu/rockchip-iommu.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

[PATCH v2 3/7] iommu/rockchip: support virtual iommu slave device

[Shunqian Zheng]

An virtual master device like DRM need to attach to iommu
domain to share the domain with VOP(the one with actual
iommu slave). We currently check the group is NULL to indicate
a virtual master, which is not true since we decide to use
the common iommu api to attach device in DRM.

With this patch, we can probe a virtual iommu device and
allow the DRM attaching to it. The virtual iommu is needed also
because we want convert to use DMA API for map/unmap, cache flush,
so that DRM buffer alloc still work even VOP is disabled.

Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
---
 drivers/iommu/rockchip-iommu.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 3c16ec3..d6c3051 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -75,6 +75,11 @@
 
 #define IOMMU_REG_POLL_COUNT_FAST 1000
 
+/* A virtual iommu in device-tree registered without reg or
+ * interrupts, so the num_mmu is zero.
+ */
+#define RK_IOMMU_IS_VIRTUAL(iommu) (iommu->num_mmu == 0)
+
 struct rk_iommu_domain {
 	struct list_head iommus;
 	u32 *dt; /* page directory table */
@@ -789,13 +794,13 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 	int ret, i;
 	phys_addr_t dte_addr;
 
-	/*
-	 * Allow 'virtual devices' (e.g., drm) to attach to domain.
-	 * Such a device does not belong to an iommu group.
-	 */
 	iommu = rk_iommu_from_dev(dev);
-	if (!iommu)
+
+	iommu->domain = domain;
+	if (RK_IOMMU_IS_VIRTUAL(iommu)) {
+		dev_dbg(dev, "Attach virtual device to iommu domain\n");
 		return 0;
+	}
 
 	ret = rk_iommu_enable_stall(iommu);
 	if (ret)
@@ -805,7 +810,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 	if (ret)
 		return ret;
 
-	iommu->domain = domain;
 
 	ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq,
 			       IRQF_SHARED, dev_name(dev), iommu);
@@ -842,10 +846,13 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
 	unsigned long flags;
 	int i;
 
-	/* Allow 'virtual devices' (eg drm) to detach from domain */
 	iommu = rk_iommu_from_dev(dev);
-	if (!iommu)
+
+	iommu->domain = NULL;
+	if (RK_IOMMU_IS_VIRTUAL(iommu)) {
+		dev_dbg(dev, "Master with virtual iommu detached from domain\n");
 		return;
+	}
 
 	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
 	list_del_init(&iommu->node);
@@ -862,8 +869,6 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
 
 	devm_free_irq(iommu->dev, iommu->irq, iommu);
 
-	iommu->domain = NULL;
-
 	dev_dbg(dev, "Detached from iommu domain\n");
 }
 
@@ -1034,6 +1039,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rk_iommu *iommu;
 	struct resource *res;
+	int num_res = pdev->num_resources;
 	int i;
 
 	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
@@ -1043,12 +1049,19 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, iommu);
 	iommu->dev = dev;
 	iommu->num_mmu = 0;
-	iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * iommu->num_mmu,
+
+	if (!num_res) {
+		iommu->bases = NULL;
+		dev_info(dev, "this is a virtual iommu\n");
+		return 0;
+	}
+
+	iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * num_res,
 				    GFP_KERNEL);
 	if (!iommu->bases)
 		return -ENOMEM;
 
-	for (i = 0; i < pdev->num_resources; i++) {
+	for (i = 0; i < num_res; i++) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
 		if (!res)
 			continue;
-- 
1.9.1

Re: [PATCH v2 3/7] iommu/rockchip: support virtual iommu slave device

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
> An virtual master device like DRM need to attach to iommu
> domain to share the domain with VOP(the one with actual
> iommu slave). We currently check the group is NULL to indicate
> a virtual master, which is not true since we decide to use
> the common iommu api to attach device in DRM.
>
> With this patch, we can probe a virtual iommu device and
> allow the DRM attaching to it. The virtual iommu is needed also
> because we want convert to use DMA API for map/unmap, cache flush,
> so that DRM buffer alloc still work even VOP is disabled.

I'm not really convinced that this is a good idea. This will require
creating fake devices in the system and generally looks really hacky.
Please see my alternative proposal inline.

>
> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> ---
>  drivers/iommu/rockchip-iommu.c | 37 +++++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 3c16ec3..d6c3051 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -75,6 +75,11 @@
>
>  #define IOMMU_REG_POLL_COUNT_FAST 1000
>
> +/* A virtual iommu in device-tree registered without reg or
> + * interrupts, so the num_mmu is zero.
> + */
> +#define RK_IOMMU_IS_VIRTUAL(iommu) (iommu->num_mmu == 0)
> +
>  struct rk_iommu_domain {
>         struct list_head iommus;
>         u32 *dt; /* page directory table */
> @@ -789,13 +794,13 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>         int ret, i;
>         phys_addr_t dte_addr;
>
> -       /*
> -        * Allow 'virtual devices' (e.g., drm) to attach to domain.
> -        * Such a device does not belong to an iommu group.
> -        */
>         iommu = rk_iommu_from_dev(dev);
> -       if (!iommu)

Could we instead allocate such virtual rk_iommu struct here (dev could
be used as iommu->dev for logging purposes and a fake group could be
allocated too)?

> +
> +       iommu->domain = domain;
> +       if (RK_IOMMU_IS_VIRTUAL(iommu)) {
> +               dev_dbg(dev, "Attach virtual device to iommu domain\n");
>                 return 0;
> +       }
>
>         ret = rk_iommu_enable_stall(iommu);
>         if (ret)
> @@ -805,7 +810,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>         if (ret)
>                 return ret;
>
> -       iommu->domain = domain;
>
>         ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq,
>                                IRQF_SHARED, dev_name(dev), iommu);
> @@ -842,10 +846,13 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
>         unsigned long flags;
>         int i;
>
> -       /* Allow 'virtual devices' (eg drm) to detach from domain */
>         iommu = rk_iommu_from_dev(dev);
> -       if (!iommu)
> +
> +       iommu->domain = NULL;

I don't think it's a good idea to set the domain to NULL before
disabling the real IOMMU. It might still trigger an interrupt at this
point and things won't behave correctly. I guess the original line
could be left as is and simply same assignment added under the if
below.

Best regards,
Tomasz

[PATCH v2 4/7] ARM: dts: rockchip: add virtual iommu for display

[Shunqian Zheng]

An virtual iommu without reg or interrupts for display.
Adding this according to iommu driver changes.

Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
---
 arch/arm/boot/dts/rk3288.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 7fa932f..4cd535f 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -219,9 +219,15 @@
 		clock-names = "timer", "pclk";
 	};
 
+	display_mmu: virtual-iommu {
+		compatible = "rockchip,iommu";
+		#iommu-cells = <0>;
+	};
+
 	display-subsystem {
 		compatible = "rockchip,display-subsystem";
 		ports = <&vopl_out>, <&vopb_out>;
+		iommus = <&display_mmu>;
 	};
 
 	sdmmc: dwmmc@ff0c0000 {
-- 
1.9.1

Re: [PATCH v2 4/7] ARM: dts: rockchip: add virtual iommu for display

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
> An virtual iommu without reg or interrupts for display.
> Adding this according to iommu driver changes.
>
> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 7fa932f..4cd535f 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -219,9 +219,15 @@
>                 clock-names = "timer", "pclk";
>         };
>
> +       display_mmu: virtual-iommu {
> +               compatible = "rockchip,iommu";
> +               #iommu-cells = <0>;
> +       };
> +

Device tree should describe real hardware and so it isn't really a
good idea to add such virtual iommu, especially using a compatible
string of a real device.

Please see my comments to patch 3/7 for an alternative idea.

Best regards,
Tomasz

[PATCH v2 5/7] drm: rockchip: use common iommu api to attach iommu

[Shunqian Zheng]

Rockchip DRM used the arm special API, arm_iommu_*(), to attach
iommu for ARM32 SoCs. This patch convert to common iommu API
so it would support ARM64 like RK3399.

The general idea is domain_alloc(), attach_device() and
arch_setup_dma_ops() to set dma_ops manually for DRM at the last.

Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 +++++++++++++++++++---------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
 2 files changed, 89 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f5a68fc..7965a66 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -14,8 +14,6 @@
  * GNU General Public License for more details.
  */
 
-#include <asm/dma-iommu.h>
-
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
@@ -24,6 +22,8 @@
 #include <linux/module.h>
 #include <linux/of_graph.h>
 #include <linux/component.h>
+#include <linux/dma-iommu.h>
+#include <linux/iommu.h>
 
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_fb.h"
@@ -46,7 +46,8 @@ static bool is_support_iommu = true;
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev)
 {
-	struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
+	struct rockchip_drm_private *private = drm_dev->dev_private;
+	struct iommu_domain *domain = private->domain;
 	int ret;
 
 	if (!is_support_iommu)
@@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 
 	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
-	return arm_iommu_attach_device(dev, mapping);
+	ret = iommu_attach_device(domain, dev);
+
+	if (ret) {
+		dev_err(dev, "Failed to attach iommu device\n");
+		return ret;
+	}
+	arch_setup_dma_ops(dev, 0x00000000, SZ_2G,
+			   (struct iommu_ops *)dev->bus->iommu_ops, false);
+	return 0;
 }
 
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 				    struct device *dev)
 {
-	if (!is_support_iommu)
-		return;
+	struct rockchip_drm_private *private = drm_dev->dev_private;
+	struct iommu_domain *domain = private->domain;
 
-	arm_iommu_detach_device(dev);
+	if (is_support_iommu)
+		iommu_detach_device(domain, dev);
 }
 
 int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
@@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct drm_device *dev,
 		priv->crtc_funcs[pipe]->disable_vblank(crtc);
 }
 
+static int rockchip_drm_init_iommu(struct drm_device *drm_dev)
+{
+	struct rockchip_drm_private *private = drm_dev->dev_private;
+	struct device *dev = drm_dev->dev;
+	int ret;
+
+	dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
+				      GFP_KERNEL);
+	if (!dev->dma_parms) {
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(dev, "Failed to set coherent mask\n");
+		return ret;
+	}
+
+	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
+
+	private->domain = iommu_domain_alloc(&platform_bus_type);
+	if (!private->domain)
+		return -ENOMEM;
+
+	ret = iommu_get_dma_cookie(private->domain);
+	if (ret) {
+		dev_err(dev, "Failed to get dma cookie\n");
+		goto err_free_domain;
+	}
+
+	ret = iommu_dma_init_domain(private->domain, 0x00000000, SZ_2G);
+	if (ret) {
+		dev_err(dev, "Failed to init domain\n");
+		goto err_put_cookie;
+	}
+
+	ret = rockchip_drm_dma_attach_device(drm_dev, dev);
+	if (ret) {
+		dev_err(dev, "Failed to attach device\n");
+		goto err_put_cookie;
+	}
+
+	return 0;
+
+err_put_cookie:
+	iommu_put_dma_cookie(private->domain);
+err_free_domain:
+	iommu_domain_free(private->domain);
+
+	return ret;
+}
+
+static void rockchip_iommu_cleanup(struct drm_device *drm_dev)
+{
+	struct rockchip_drm_private *private = drm_dev->dev_private;
+
+	iommu_put_dma_cookie(private->domain);
+	iommu_domain_free(private->domain);
+}
+
 static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags)
 {
 	struct rockchip_drm_private *private;
-	struct dma_iommu_mapping *mapping = NULL;
 	struct device *dev = drm_dev->dev;
 	struct drm_connector *connector;
 	int ret;
@@ -153,38 +223,18 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags)
 
 	rockchip_drm_mode_config_init(drm_dev);
 
-	dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
-				      GFP_KERNEL);
-	if (!dev->dma_parms) {
-		ret = -ENOMEM;
-		goto err_config_cleanup;
-	}
-
 	if (is_support_iommu) {
-		/* TODO(djkurtz): fetch the mapping start/size from somewhere */
-		mapping = arm_iommu_create_mapping(&platform_bus_type,
-						   0x00000000,
-						   SZ_2G);
-		if (IS_ERR(mapping)) {
-			ret = PTR_ERR(mapping);
-			goto err_config_cleanup;
-		}
-
-		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+		ret = rockchip_drm_init_iommu(drm_dev);
 		if (ret)
-			goto err_release_mapping;
-
-		dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
-
-		ret = arm_iommu_attach_device(dev, mapping);
-		if (ret)
-			goto err_release_mapping;
+			goto err_config_cleanup;
 	}
 
 	/* Try to bind all sub drivers. */
 	ret = component_bind_all(dev, drm_dev);
-	if (ret)
-		goto err_detach_device;
+	if (ret) {
+		dev_err(dev, "Failed to bind components\n");
+		goto err_iommu_cleanup;
+	}
 
 	/*
 	 * All components are now added, we can publish the connector sysfs
@@ -222,21 +272,17 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags)
 	if (ret)
 		goto err_vblank_cleanup;
 
-	if (is_support_iommu)
-		arm_iommu_release_mapping(mapping);
 	return 0;
+
 err_vblank_cleanup:
 	drm_vblank_cleanup(drm_dev);
 err_kms_helper_poll_fini:
 	drm_kms_helper_poll_fini(drm_dev);
 err_unbind:
 	component_unbind_all(dev, drm_dev);
-err_detach_device:
-	if (is_support_iommu)
-		arm_iommu_detach_device(dev);
-err_release_mapping:
+err_iommu_cleanup:
 	if (is_support_iommu)
-		arm_iommu_release_mapping(mapping);
+		rockchip_iommu_cleanup(drm_dev);
 err_config_cleanup:
 	drm_mode_config_cleanup(drm_dev);
 	drm_dev->dev_private = NULL;
@@ -252,7 +298,7 @@ static int rockchip_drm_unload(struct drm_device *drm_dev)
 	drm_kms_helper_poll_fini(drm_dev);
 	component_unbind_all(dev, drm_dev);
 	if (is_support_iommu)
-		arm_iommu_detach_device(dev);
+		rockchip_iommu_cleanup(drm_dev);
 	drm_mode_config_cleanup(drm_dev);
 	drm_dev->dev_private = NULL;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 56f43a3..1e2a666 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -70,6 +70,7 @@ struct rockchip_drm_private {
 	const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
 
 	struct rockchip_atomic_commit commit;
+	struct iommu_domain *domain;
 };
 
 void rockchip_drm_atomic_work(struct work_struct *work);
-- 
1.9.1

Re: [PATCH v2 5/7] drm: rockchip: use common iommu api to attach iommu

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
> Rockchip DRM used the arm special API, arm_iommu_*(), to attach
> iommu for ARM32 SoCs. This patch convert to common iommu API
> so it would support ARM64 like RK3399.
>
> The general idea is domain_alloc(), attach_device() and
> arch_setup_dma_ops() to set dma_ops manually for DRM at the last.
>
> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 +++++++++++++++++++---------
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
>  2 files changed, 89 insertions(+), 42 deletions(-)
>

Please see my comments inline.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index f5a68fc..7965a66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -14,8 +14,6 @@
>   * GNU General Public License for more details.
>   */
>
> -#include <asm/dma-iommu.h>
> -
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_helper.h>
> @@ -24,6 +22,8 @@
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
>  #include <linux/component.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/iommu.h>
>
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_fb.h"
> @@ -46,7 +46,8 @@ static bool is_support_iommu = true;
>  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>                                    struct device *dev)
>  {
> -       struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
> +       struct rockchip_drm_private *private = drm_dev->dev_private;
> +       struct iommu_domain *domain = private->domain;
>         int ret;
>
>         if (!is_support_iommu)
> @@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>
>         dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
>
> -       return arm_iommu_attach_device(dev, mapping);
> +       ret = iommu_attach_device(domain, dev);
> +

nit: Unnecessary blank line.

> +       if (ret) {
> +               dev_err(dev, "Failed to attach iommu device\n");
> +               return ret;
> +       }

nit: On the other hand, a blank line here would improve readability.

> +       arch_setup_dma_ops(dev, 0x00000000, SZ_2G,
> +                          (struct iommu_ops *)dev->bus->iommu_ops, false);

This is casting a const pointer to a non-const pointer. which isn't
really a good idea. I can see that arch_setup_dma_ops() requires a
writable pointer, though. Looking at the implementations of
arch_setup_dma_ops() around the platforms (namely arm and arm64...),
it makes me wonder if the prototype shouldn't be changed to const
instead.

> +       return 0;
>  }
>
>  void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
>                                     struct device *dev)
>  {
> -       if (!is_support_iommu)
> -               return;
> +       struct rockchip_drm_private *private = drm_dev->dev_private;
> +       struct iommu_domain *domain = private->domain;
>
> -       arm_iommu_detach_device(dev);
> +       if (is_support_iommu)
> +               iommu_detach_device(domain, dev);
>  }
>
>  int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
> @@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct drm_device *dev,
>                 priv->crtc_funcs[pipe]->disable_vblank(crtc);
>  }
>
> +static int rockchip_drm_init_iommu(struct drm_device *drm_dev)
> +{
> +       struct rockchip_drm_private *private = drm_dev->dev_private;
> +       struct device *dev = drm_dev->dev;
> +       int ret;
> +
> +       dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> +                                     GFP_KERNEL);
> +       if (!dev->dma_parms) {
> +               ret = -ENOMEM;
> +               return ret;

nit: return -ENOMEM;

> +       }
> +
> +       ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> +       if (ret) {
> +               dev_err(dev, "Failed to set coherent mask\n");
> +               return ret;
> +       }
> +
> +       dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
> +
> +       private->domain = iommu_domain_alloc(&platform_bus_type);
> +       if (!private->domain)
> +               return -ENOMEM;
> +
> +       ret = iommu_get_dma_cookie(private->domain);
> +       if (ret) {
> +               dev_err(dev, "Failed to get dma cookie\n");
> +               goto err_free_domain;
> +       }
> +
> +       ret = iommu_dma_init_domain(private->domain, 0x00000000, SZ_2G);

I guess djkurtz's TODO comment could be preserved here.

Best regards,
Tomasz

[PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Shunqian Zheng]

Use DMA API instead of architecture internal functions like
__cpuc_flush_dcache_area() etc.

To support the virtual device like DRM the virtual slave iommu
added in the previous patch, attaching to which the DRM can use
it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.

With this patch, this driver is available for ARM64 like RK3399.

Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
---
 drivers/iommu/rockchip-iommu.c | 113 ++++++++++++++++++++++++++---------------
 1 file changed, 71 insertions(+), 42 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index d6c3051..aafea6e 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -4,8 +4,6 @@
  * published by the Free Software Foundation.
  */
 
-#include <asm/cacheflush.h>
-#include <asm/pgtable.h>
 #include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -61,8 +59,7 @@
 #define RK_MMU_IRQ_BUS_ERROR     0x02  /* bus read error */
 #define RK_MMU_IRQ_MASK          (RK_MMU_IRQ_PAGE_FAULT | RK_MMU_IRQ_BUS_ERROR)
 
-#define NUM_DT_ENTRIES 1024
-#define NUM_PT_ENTRIES 1024
+#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */
 
 #define SPAGE_ORDER 12
 #define SPAGE_SIZE (1 << SPAGE_ORDER)
@@ -82,7 +79,9 @@
 
 struct rk_iommu_domain {
 	struct list_head iommus;
+	struct device *dev;
 	u32 *dt; /* page directory table */
+	dma_addr_t dt_dma;
 	spinlock_t iommus_lock; /* lock for iommus list */
 	spinlock_t dt_lock; /* lock for modifying page directory table */
 
@@ -98,14 +97,12 @@ struct rk_iommu {
 	struct iommu_domain *domain; /* domain to which iommu is attached */
 };
 
-static inline void rk_table_flush(u32 *va, unsigned int count)
+static inline void rk_table_flush(struct device *dev, dma_addr_t dma,
+				  unsigned int count)
 {
-	phys_addr_t pa_start = virt_to_phys(va);
-	phys_addr_t pa_end = virt_to_phys(va + count);
-	size_t size = pa_end - pa_start;
+	size_t size = count * 4;
 
-	__cpuc_flush_dcache_area(va, size);
-	outer_flush_range(pa_start, pa_end);
+	dma_sync_single_range_for_device(dev, dma, 0, size, DMA_TO_DEVICE);
 }
 
 static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom)
@@ -188,10 +185,9 @@ static inline bool rk_dte_is_pt_valid(u32 dte)
 	return dte & RK_DTE_PT_VALID;
 }
 
-static u32 rk_mk_dte(u32 *pt)
+static inline u32 rk_mk_dte(dma_addr_t pt_dma)
 {
-	phys_addr_t pt_phys = virt_to_phys(pt);
-	return (pt_phys & RK_DTE_PT_ADDRESS_MASK) | RK_DTE_PT_VALID;
+	return (pt_dma & RK_DTE_PT_ADDRESS_MASK) | RK_DTE_PT_VALID;
 }
 
 /*
@@ -609,12 +605,14 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
 				  dma_addr_t iova)
 {
 	u32 *page_table, *dte_addr;
+	u32 dte_index = rk_iova_dte_index(iova);
 	u32 dte;
 	phys_addr_t pt_phys;
+	dma_addr_t pt_dma;
 
 	assert_spin_locked(&rk_domain->dt_lock);
 
-	dte_addr = &rk_domain->dt[rk_iova_dte_index(iova)];
+	dte_addr = &rk_domain->dt[dte_index];
 	dte = *dte_addr;
 	if (rk_dte_is_pt_valid(dte))
 		goto done;
@@ -623,19 +621,27 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
 	if (!page_table)
 		return ERR_PTR(-ENOMEM);
 
-	dte = rk_mk_dte(page_table);
-	*dte_addr = dte;
+	pt_dma = dma_map_single(rk_domain->dev, page_table,
+				SPAGE_SIZE, DMA_TO_DEVICE);
+	if (dma_mapping_error(rk_domain->dev, pt_dma)) {
+		dev_err(rk_domain->dev, "dma mapping error\n");
+		free_page((unsigned long)page_table);
+		return ERR_PTR(-ENOMEM);
+	}
 
-	rk_table_flush(page_table, NUM_PT_ENTRIES);
-	rk_table_flush(dte_addr, 1);
+	dte = rk_mk_dte(pt_dma);
+	*dte_addr = dte;
 
+	rk_table_flush(rk_domain->dev, pt_dma, NUM_TLB_ENTRIES);
+	rk_table_flush(rk_domain->dev, rk_domain->dt_dma + dte_index * 4, 1);
 done:
 	pt_phys = rk_dte_pt_address(dte);
 	return (u32 *)phys_to_virt(pt_phys);
 }
 
 static size_t rk_iommu_unmap_iova(struct rk_iommu_domain *rk_domain,
-				  u32 *pte_addr, dma_addr_t iova, size_t size)
+				  u32 *pte_addr, dma_addr_t pte_dma,
+				  size_t size)
 {
 	unsigned int pte_count;
 	unsigned int pte_total = size / SPAGE_SIZE;
@@ -650,14 +656,14 @@ static size_t rk_iommu_unmap_iova(struct rk_iommu_domain *rk_domain,
 		pte_addr[pte_count] = rk_mk_pte_invalid(pte);
 	}
 
-	rk_table_flush(pte_addr, pte_count);
+	rk_table_flush(rk_domain->dev, pte_dma, pte_count);
 
 	return pte_count * SPAGE_SIZE;
 }
 
 static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr,
-			     dma_addr_t iova, phys_addr_t paddr, size_t size,
-			     int prot)
+			     dma_addr_t pte_dma, dma_addr_t iova,
+			     phys_addr_t paddr, size_t size, int prot)
 {
 	unsigned int pte_count;
 	unsigned int pte_total = size / SPAGE_SIZE;
@@ -676,7 +682,7 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr,
 		paddr += SPAGE_SIZE;
 	}
 
-	rk_table_flush(pte_addr, pte_count);
+	rk_table_flush(rk_domain->dev, pte_dma, pte_total);
 
 	/*
 	 * Zap the first and last iova to evict from iotlb any previously
@@ -689,7 +695,8 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr,
 	return 0;
 unwind:
 	/* Unmap the range of iovas that we just mapped */
-	rk_iommu_unmap_iova(rk_domain, pte_addr, iova, pte_count * SPAGE_SIZE);
+	rk_iommu_unmap_iova(rk_domain, pte_addr, pte_dma,
+			    pte_count * SPAGE_SIZE);
 
 	iova += pte_count * SPAGE_SIZE;
 	page_phys = rk_pte_page_address(pte_addr[pte_count]);
@@ -704,8 +711,9 @@ static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova,
 {
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
 	unsigned long flags;
-	dma_addr_t iova = (dma_addr_t)_iova;
+	dma_addr_t pte_dma, iova = (dma_addr_t)_iova;
 	u32 *page_table, *pte_addr;
+	u32 dte_index, pte_index;
 	int ret;
 
 	spin_lock_irqsave(&rk_domain->dt_lock, flags);
@@ -723,8 +731,13 @@ static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova,
 		return PTR_ERR(page_table);
 	}
 
-	pte_addr = &page_table[rk_iova_pte_index(iova)];
-	ret = rk_iommu_map_iova(rk_domain, pte_addr, iova, paddr, size, prot);
+	dte_index = rk_domain->dt[rk_iova_dte_index(iova)];
+	pte_index = rk_iova_pte_index(iova);
+	pte_addr = &page_table[pte_index];
+	pte_dma = rk_dte_pt_address(dte_index) + pte_index * 4;
+	ret = rk_iommu_map_iova(rk_domain, pte_addr, pte_dma, iova,
+				paddr, size, prot);
+
 	spin_unlock_irqrestore(&rk_domain->dt_lock, flags);
 
 	return ret;
@@ -735,7 +748,7 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
 {
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
 	unsigned long flags;
-	dma_addr_t iova = (dma_addr_t)_iova;
+	dma_addr_t pte_dma, iova = (dma_addr_t)_iova;
 	phys_addr_t pt_phys;
 	u32 dte;
 	u32 *pte_addr;
@@ -759,7 +772,8 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
 
 	pt_phys = rk_dte_pt_address(dte);
 	pte_addr = (u32 *)phys_to_virt(pt_phys) + rk_iova_pte_index(iova);
-	unmap_size = rk_iommu_unmap_iova(rk_domain, pte_addr, iova, size);
+	pte_dma = pt_phys + rk_iova_pte_index(iova) * 4;
+	unmap_size = rk_iommu_unmap_iova(rk_domain, pte_addr, pte_dma, size);
 
 	spin_unlock_irqrestore(&rk_domain->dt_lock, flags);
 
@@ -776,8 +790,6 @@ static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
 	struct rk_iommu *rk_iommu;
 
 	group = iommu_group_get(dev);
-	if (!group)
-		return NULL;
 	iommu_dev = iommu_group_get_iommudata(group);
 	rk_iommu = dev_get_drvdata(iommu_dev);
 	iommu_group_put(group);
@@ -792,9 +804,21 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
 	unsigned long flags;
 	int ret, i;
-	phys_addr_t dte_addr;
 
 	iommu = rk_iommu_from_dev(dev);
+	/* Set the domain dev to virtual one if exist */
+	if (RK_IOMMU_IS_VIRTUAL(iommu) || !rk_domain->dev)
+		rk_domain->dev = iommu->dev;
+
+	if (!rk_domain->dt_dma) {
+		rk_domain->dt_dma = dma_map_single(rk_domain->dev, rk_domain->dt,
+						   SPAGE_SIZE, DMA_TO_DEVICE);
+		if (dma_mapping_error(rk_domain->dev, rk_domain->dt_dma))
+			return -ENOMEM;
+
+		rk_table_flush(rk_domain->dev, rk_domain->dt_dma,
+			       NUM_TLB_ENTRIES);
+	}
 
 	iommu->domain = domain;
 	if (RK_IOMMU_IS_VIRTUAL(iommu)) {
@@ -804,28 +828,27 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 
 	ret = rk_iommu_enable_stall(iommu);
 	if (ret)
-		return ret;
+		goto unmap;
 
 	ret = rk_iommu_force_reset(iommu);
 	if (ret)
-		return ret;
+		goto unmap;
 
 
 	ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq,
 			       IRQF_SHARED, dev_name(dev), iommu);
 	if (ret)
-		return ret;
+		goto unmap;
 
-	dte_addr = virt_to_phys(rk_domain->dt);
 	for (i = 0; i < iommu->num_mmu; i++) {
-		rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, dte_addr);
+		rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, rk_domain->dt_dma);
 		rk_iommu_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE);
 		rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK);
 	}
 
 	ret = rk_iommu_enable_paging(iommu);
 	if (ret)
-		return ret;
+		goto unmap;
 
 	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
 	list_add_tail(&iommu->node, &rk_domain->iommus);
@@ -836,6 +859,10 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 	rk_iommu_disable_stall(iommu);
 
 	return 0;
+unmap:
+	dma_unmap_single(rk_domain->dev, rk_domain->dt_dma, SPAGE_SIZE,
+			 DMA_TO_DEVICE);
+	return ret;
 }
 
 static void rk_iommu_detach_device(struct iommu_domain *domain,
@@ -846,8 +873,10 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
 	unsigned long flags;
 	int i;
 
-	iommu = rk_iommu_from_dev(dev);
+	rk_domain->dev = NULL;
 
+	/* Allow 'virtual devices' (eg drm) to detach from domain */
+	iommu = rk_iommu_from_dev(dev);
 	iommu->domain = NULL;
 	if (RK_IOMMU_IS_VIRTUAL(iommu)) {
 		dev_dbg(dev, "Master with virtual iommu detached from domain\n");
@@ -883,6 +912,8 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
 	if (!rk_domain)
 		return NULL;
 
+	rk_domain->dev = NULL;
+	rk_domain->dt_dma = 0;
 	/*
 	 * rk32xx iommus use a 2 level pagetable.
 	 * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
@@ -892,8 +923,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
 	if (!rk_domain->dt)
 		goto err_dt;
 
-	rk_table_flush(rk_domain->dt, NUM_DT_ENTRIES);
-
 	spin_lock_init(&rk_domain->iommus_lock);
 	spin_lock_init(&rk_domain->dt_lock);
 	INIT_LIST_HEAD(&rk_domain->iommus);
@@ -912,7 +941,7 @@ static void rk_iommu_domain_free(struct iommu_domain *domain)
 
 	WARN_ON(!list_empty(&rk_domain->iommus));
 
-	for (i = 0; i < NUM_DT_ENTRIES; i++) {
+	for (i = 0; i < NUM_TLB_ENTRIES; i++) {
 		u32 dte = rk_domain->dt[i];
 		if (rk_dte_is_pt_valid(dte)) {
 			phys_addr_t pt_phys = rk_dte_pt_address(dte);
-- 
1.9.1

Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
> Use DMA API instead of architecture internal functions like
> __cpuc_flush_dcache_area() etc.
>
> To support the virtual device like DRM the virtual slave iommu
> added in the previous patch, attaching to which the DRM can use
> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>
> With this patch, this driver is available for ARM64 like RK3399.
>

Could we instead simply allocate coherent memory for page tables using
dma_alloc_coherent() and skip any flushing on CPU side completely? If
I'm looking correctly, the driver only reads back the page directory
when checking if there is a need to allocate new page table, so there
shouldn't be any significant penalty for disabling the cache.

Other than that, please see some comments inline.

> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> ---
>  drivers/iommu/rockchip-iommu.c | 113 ++++++++++++++++++++++++++---------------
>  1 file changed, 71 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index d6c3051..aafea6e 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -4,8 +4,6 @@
>   * published by the Free Software Foundation.
>   */
>
> -#include <asm/cacheflush.h>
> -#include <asm/pgtable.h>
>  #include <linux/compiler.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> @@ -61,8 +59,7 @@
>  #define RK_MMU_IRQ_BUS_ERROR     0x02  /* bus read error */
>  #define RK_MMU_IRQ_MASK          (RK_MMU_IRQ_PAGE_FAULT | RK_MMU_IRQ_BUS_ERROR)
>
> -#define NUM_DT_ENTRIES 1024
> -#define NUM_PT_ENTRIES 1024
> +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */

Is it necessary to change this in this patch? In general, it's not a
good idea to mix multiple logical changes together.

>
>  #define SPAGE_ORDER 12
>  #define SPAGE_SIZE (1 << SPAGE_ORDER)
> @@ -82,7 +79,9 @@
>
>  struct rk_iommu_domain {
>         struct list_head iommus;
> +       struct device *dev;
>         u32 *dt; /* page directory table */
> +       dma_addr_t dt_dma;
>         spinlock_t iommus_lock; /* lock for iommus list */
>         spinlock_t dt_lock; /* lock for modifying page directory table */
>
> @@ -98,14 +97,12 @@ struct rk_iommu {
>         struct iommu_domain *domain; /* domain to which iommu is attached */
>  };
>
> -static inline void rk_table_flush(u32 *va, unsigned int count)
> +static inline void rk_table_flush(struct device *dev, dma_addr_t dma,
> +                                 unsigned int count)
>  {
> -       phys_addr_t pa_start = virt_to_phys(va);
> -       phys_addr_t pa_end = virt_to_phys(va + count);
> -       size_t size = pa_end - pa_start;
> +       size_t size = count * 4;

It would be a good idea to specify what "count" is. I'm a bit confused
that before it meant bytes and now some multiple of 4?

Best regards,
Tomasz

Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Tomasz Figa]

On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng
<shunqian.zheng@gmail.com> wrote:
> Hi
>
> On 2016年06月10日 17:10, Tomasz Figa wrote:
>>
>> Hi,
>>
>> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com>
>> wrote:
>>>
>>> Use DMA API instead of architecture internal functions like
>>> __cpuc_flush_dcache_area() etc.
>>>
>>> To support the virtual device like DRM the virtual slave iommu
>>> added in the previous patch, attaching to which the DRM can use
>>> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>>>
>>> With this patch, this driver is available for ARM64 like RK3399.
>>>
>> Could we instead simply allocate coherent memory for page tables using
>> dma_alloc_coherent() and skip any flushing on CPU side completely? If
>> I'm looking correctly, the driver only reads back the page directory
>> when checking if there is a need to allocate new page table, so there
>> shouldn't be any significant penalty for disabling the cache.
>
> I try to use dma_alloc_coherent() to replace the dma_map_single(),
> but it doesn't work for me properly.
> Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after
> attaching
> to iommu, so when the iommu domain need to alloc a new page in
> rk_iommu_map(),
> it would call:
> rk_iommu_map()  --> dma_alloc_coherent()  --> ops->alloc()  --> iommu_map()
> --> rk_iommu_map()

It shouldn't call iommu_map(), because the IOMMU is not behind another
IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the
IOMMU struct device and not the DRM device?

Best regards,
Tomasz

Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Tomasz Figa]

On Mon, Jun 13, 2016 at 7:31 PM, Shunqian Zheng
<shunqian.zheng@gmail.com> wrote:
> HI,
>
>
> On 2016年06月13日 18:21, Tomasz Figa wrote:
>>
>> On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng
>> <shunqian.zheng@gmail.com> wrote:
>>>
>>> Hi
>>>
>>> On 2016年06月10日 17:10, Tomasz Figa wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com>
>>>> wrote:
>>>>>
>>>>> Use DMA API instead of architecture internal functions like
>>>>> __cpuc_flush_dcache_area() etc.
>>>>>
>>>>> To support the virtual device like DRM the virtual slave iommu
>>>>> added in the previous patch, attaching to which the DRM can use
>>>>> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>>>>>
>>>>> With this patch, this driver is available for ARM64 like RK3399.
>>>>>
>>>> Could we instead simply allocate coherent memory for page tables using
>>>> dma_alloc_coherent() and skip any flushing on CPU side completely? If
>>>> I'm looking correctly, the driver only reads back the page directory
>>>> when checking if there is a need to allocate new page table, so there
>>>> shouldn't be any significant penalty for disabling the cache.
>>>
>>> I try to use dma_alloc_coherent() to replace the dma_map_single(),
>>> but it doesn't work for me properly.
>>> Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after
>>> attaching
>>> to iommu, so when the iommu domain need to alloc a new page in
>>> rk_iommu_map(),
>>> it would call:
>>> rk_iommu_map()  --> dma_alloc_coherent()  --> ops->alloc()  -->
>>> iommu_map()
>>> --> rk_iommu_map()
>>
>> It shouldn't call iommu_map(), because the IOMMU is not behind another
>> IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the
>> IOMMU struct device and not the DRM device?
>
> I called dma_alloc_coherent() with DRM device but not the IOMMU device,
> because DRM didn't attach to any iommu. Even allocating an virtual one when
> attaching, the iommu->dev
> is DRM device though.
> Am I right here?

What I meant, is that even though rk_iommu_map() is called for DRM
device, the page table allocation happening inside should be called
for the IOMMU device itself, because it's the consumer of these page
tables.

Best regards,
Tomasz

Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Robin Murphy]

On 13/06/16 10:56, Shunqian Zheng wrote:
> Hi
>
> On 2016年06月10日 17:10, Tomasz Figa wrote:
>> Hi,
>>
>> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng
>> <zhengsq@rock-chips.com> wrote:
>>> Use DMA API instead of architecture internal functions like
>>> __cpuc_flush_dcache_area() etc.
>>>
>>> To support the virtual device like DRM the virtual slave iommu
>>> added in the previous patch, attaching to which the DRM can use
>>> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>>>
>>> With this patch, this driver is available for ARM64 like RK3399.
>>>
>> Could we instead simply allocate coherent memory for page tables using
>> dma_alloc_coherent() and skip any flushing on CPU side completely? If
>> I'm looking correctly, the driver only reads back the page directory
>> when checking if there is a need to allocate new page table, so there
>> shouldn't be any significant penalty for disabling the cache.
> I try to use dma_alloc_coherent() to replace the dma_map_single(),
> but it doesn't work for me properly.
> Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after
> attaching
> to iommu, so when the iommu domain need to alloc a new page in
> rk_iommu_map(),
> it would call:
> rk_iommu_map()  --> dma_alloc_coherent()  --> ops->alloc()  -->
> iommu_map() --> rk_iommu_map()

That sounds more like you're passing the wrong device around somewhere, 
since this approach is working fine for other IOMMUs; specifically, the 
flow goes:

dma_alloc_coherent(DRM dev)   // for buffer
--> ops->alloc(DRM dev)
     --> iommu_dma_alloc(DRM dev)
         --> iommu_map()
             --> dma_alloc_coherent(IOMMU dev)  // for pagetable
                 --> ops->alloc(IOMMU dev)
                     --> swiotlb_alloc(IOMMU dev)

There shouldn't be any need for this "virtual IOMMU" at all. I think the 
Exynos DRM driver is in a similar situation of having multiple devices 
(involving multiple IOMMUs) backing the virtual DRM device, and that 
doesn't seem to be doing anything this crazy so it's probably worth 
taking a look at.

Robin.

> Then I try to reserve memory for coherent so that, dma_alloc_coherent()
> calls dma_alloc_from_coherent()
> but not ops->alloc(). But it doesn't work too because when DRM request
> buffer it never uses iommu.
>>
>> Other than that, please see some comments inline.
>>
>>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
>>> ---
>>>   drivers/iommu/rockchip-iommu.c | 113
>>> ++++++++++++++++++++++++++---------------
>>>   1 file changed, 71 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/iommu/rockchip-iommu.c
>>> b/drivers/iommu/rockchip-iommu.c
>>> index d6c3051..aafea6e 100644
>>> --- a/drivers/iommu/rockchip-iommu.c
>>> +++ b/drivers/iommu/rockchip-iommu.c
>>> @@ -4,8 +4,6 @@
>>>    * published by the Free Software Foundation.
>>>    */
>>>
>>> -#include <asm/cacheflush.h>
>>> -#include <asm/pgtable.h>
>>>   #include <linux/compiler.h>
>>>   #include <linux/delay.h>
>>>   #include <linux/device.h>
>>> @@ -61,8 +59,7 @@
>>>   #define RK_MMU_IRQ_BUS_ERROR     0x02  /* bus read error */
>>>   #define RK_MMU_IRQ_MASK          (RK_MMU_IRQ_PAGE_FAULT |
>>> RK_MMU_IRQ_BUS_ERROR)
>>>
>>> -#define NUM_DT_ENTRIES 1024
>>> -#define NUM_PT_ENTRIES 1024
>>> +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */
>> Is it necessary to change this in this patch? In general, it's not a
>> good idea to mix multiple logical changes together.
> Sure, will restore changes in v3.
>>
>>>   #define SPAGE_ORDER 12
>>>   #define SPAGE_SIZE (1 << SPAGE_ORDER)
>>> @@ -82,7 +79,9 @@
>>>
>>>   struct rk_iommu_domain {
>>>          struct list_head iommus;
>>> +       struct device *dev;
>>>          u32 *dt; /* page directory table */
>>> +       dma_addr_t dt_dma;
>>>          spinlock_t iommus_lock; /* lock for iommus list */
>>>          spinlock_t dt_lock; /* lock for modifying page directory
>>> table */
>>>
>>> @@ -98,14 +97,12 @@ struct rk_iommu {
>>>          struct iommu_domain *domain; /* domain to which iommu is
>>> attached */
>>>   };
>>>
>>> -static inline void rk_table_flush(u32 *va, unsigned int count)
>>> +static inline void rk_table_flush(struct device *dev, dma_addr_t dma,
>>> +                                 unsigned int count)
>>>   {
>>> -       phys_addr_t pa_start = virt_to_phys(va);
>>> -       phys_addr_t pa_end = virt_to_phys(va + count);
>>> -       size_t size = pa_end - pa_start;
>>> +       size_t size = count * 4;
>> It would be a good idea to specify what "count" is. I'm a bit confused
>> that before it meant bytes and now some multiple of 4?
> "count" means PT/DT entry count to flush here. I would add some more
> comment on it.
>
> Thank you very much,
> Shunqian
>>
>> Best regards,
>> Tomasz
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 7/7] iommu/rockchip: enable rockchip iommu on ARM64 platform

[Shunqian Zheng]

From: Simon Xue <xxm@rock-chips.com>

Signed-off-by: Simon Xue <xxm@rock-chips.com>
Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
---
 drivers/iommu/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index ad08603..5572621 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -218,7 +218,7 @@ config OMAP_IOMMU_DEBUG
 
 config ROCKCHIP_IOMMU
 	bool "Rockchip IOMMU Support"
-	depends on ARM
+	depends on ARM || ARM64
 	depends on ARCH_ROCKCHIP || COMPILE_TEST
 	select IOMMU_API
 	select ARM_DMA_USE_IOMMU
-- 
1.9.1

Re: [PATCH v2 7/7] iommu/rockchip: enable rockchip iommu on ARM64 platform

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
> From: Simon Xue <xxm@rock-chips.com>
>

It is usual a good practice to include at least a single sentence
here, even if the patch is trivial. In this case it could say

"This patch makes it possible to compile the rockchip-iommu driver on
ARM64 platform to be used with 64-bit SoCs equipped with this type of
IOMMU."

> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> ---
>  drivers/iommu/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Assuming that the above is fixed:

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz