[PATCH 0/5] iommu/rockchip: fix bugs; enable iommu for ARM64

[PATCH 0/5] iommu/rockchip: fix bugs; enable iommu for ARM64

[Shunqian Zheng]

Fix a few bugs for rockchip iommu and enable for ARM64.

Simon (5):
  iommu/rockchip: fix devm_request_irq and devm_free_irq parameter
  iommu/rockchip: add map_sg callback for rk_iommu_ops
  iommu/rockchip: add sanity check for virtual device
  iommu/rockchip: add ARM64 cache flush operation for iommu
  iommu/rockchip: enable rockchip iommu on ARM64 platform

 drivers/iommu/Kconfig          |  2 +-
 drivers/iommu/rockchip-iommu.c | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH 1/5] iommu/rockchip: fix devm_request_irq and devm_free_irq parameter

[Shunqian Zheng]

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

When rk_iommu_attach_device or rk_iommu_detach_device be called, the second
parameter "dev" represent the device who own the iommu, so it is not resonable
using "dev" for devm_request_irq's first parameter. To avoid potential error,
we must use iommu device itself "iommu->dev" instead, the same as devm_free_irq.

Signed-off-by: Simon <xxm@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

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

[Shunqian Zheng]

From: Simon <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 <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

[PATCH 3/5] iommu/rockchip: add sanity check for virtual device

[Shunqian Zheng]

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

We mark DRM as "virtual" device by checking it's group,
which may no longer true with the new rockchip drm drv in
https://patchwork.kernel.org/patch/8876331/.

Checking the group->iommu_data for virtual device.

Signed-off-by: Simon <xxm@rock-chips.com>
---
 drivers/iommu/rockchip-iommu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 3c16ec3..043d18c 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -764,6 +764,7 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
 	return unmap_size;
 }
 
+/* Return NULL for a virtual device like DRM */
 static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
 {
 	struct iommu_group *group;
@@ -774,6 +775,11 @@ static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
 	if (!group)
 		return NULL;
 	iommu_dev = iommu_group_get_iommudata(group);
+	if (!iommu_dev) {
+		dev_info(dev, "Possibly a virtual device\n");
+		return NULL;
+	}
+
 	rk_iommu = dev_get_drvdata(iommu_dev);
 	iommu_group_put(group);
 
@@ -791,7 +797,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 
 	/*
 	 * 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)
-- 
1.9.1

[PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu

[Shunqian Zheng]

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

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

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 043d18c..1741b65 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -95,12 +95,16 @@ struct rk_iommu {
 
 static inline void rk_table_flush(u32 *va, unsigned int count)
 {
+#if defined(CONFIG_ARM)
 	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;
 
 	__cpuc_flush_dcache_area(va, size);
 	outer_flush_range(pa_start, pa_end);
+#elif defined(CONFIG_ARM64)
+	__dma_flush_range(va, va + count);
+#endif
 }
 
 static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom)
-- 
1.9.1

[PATCH 5/5] iommu/rockchip: enable rockchip iommu on ARM64 platform

[Shunqian Zheng]

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

Signed-off-by: Simon <xxm@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 1/5] iommu/rockchip: fix devm_request_irq and devm_free_irq parameter

[Heiko Stuebner]

Am Montag, 23. Mai 2016, 09:37:15 schrieb Shunqian Zheng:
> From: Simon <xxm@rock-chips.com>

generally a "firstname surename <mail>" is expected, so a first name is not 
really enough.

> 
> When rk_iommu_attach_device or rk_iommu_detach_device be called, the
> second parameter "dev" represent the device who own the iommu, so it is
> not resonable using "dev" for devm_request_irq's first parameter. To
> avoid potential error, we must use iommu device itself "iommu->dev"
> instead, the same as devm_free_irq.
> 
> Signed-off-by: Simon <xxm@rock-chips.com>

same here, and the person sending in the patch also needs a signed-off-by 
line, so that should look like

Signed-off-by: Simon <xxm@rock-chips.com>
Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>

The same applies of course for all other affected patches of this series.


After looking at the iommu code, I think the change itself looks sane, so
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

although I wonder what we need the devm_* for if we request and free the 
irqs manually all the time anyway?


Heiko

> ---
>  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;

Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu

[Robin Murphy]

On 23/05/16 02:37, Shunqian Zheng wrote:
> From: Simon <xxm@rock-chips.com>
>
> Signed-off-by: Simon <xxm@rock-chips.com>
> ---
>   drivers/iommu/rockchip-iommu.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 043d18c..1741b65 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -95,12 +95,16 @@ struct rk_iommu {
>
>   static inline void rk_table_flush(u32 *va, unsigned int count)
>   {
> +#if defined(CONFIG_ARM)
>   	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;
>
>   	__cpuc_flush_dcache_area(va, size);
>   	outer_flush_range(pa_start, pa_end);
> +#elif defined(CONFIG_ARM64)
> +	__dma_flush_range(va, va + count);
> +#endif

Ugh, please don't use arch-private cache maintenance functions directly 
from a driver. Allocating/mapping page tables to be read by the IOMMU is 
still DMA, so using the DMA APIs is the correct way to manage them, 
*especially* if it needs to work across multiple architectures.

Robin.

>   }
>
>   static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom)
>

Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu

[Catalin Marinas]

On Mon, May 23, 2016 at 11:44:14AM +0100, Robin Murphy wrote:
> On 23/05/16 02:37, Shunqian Zheng wrote:
> >From: Simon <xxm@rock-chips.com>
> >
> >Signed-off-by: Simon <xxm@rock-chips.com>
> >---
> >  drivers/iommu/rockchip-iommu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> >index 043d18c..1741b65 100644
> >--- a/drivers/iommu/rockchip-iommu.c
> >+++ b/drivers/iommu/rockchip-iommu.c
> >@@ -95,12 +95,16 @@ struct rk_iommu {
> >
> >  static inline void rk_table_flush(u32 *va, unsigned int count)
> >  {
> >+#if defined(CONFIG_ARM)
> >  	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;
> >
> >  	__cpuc_flush_dcache_area(va, size);
> >  	outer_flush_range(pa_start, pa_end);
> >+#elif defined(CONFIG_ARM64)
> >+	__dma_flush_range(va, va + count);
> >+#endif
> 
> Ugh, please don't use arch-private cache maintenance functions directly from
> a driver. Allocating/mapping page tables to be read by the IOMMU is still
> DMA, so using the DMA APIs is the correct way to manage them, *especially*
> if it needs to work across multiple architectures.

I fully agree, these functions should not be used in drivers.

-- 
Catalin

Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu

[Shunqian Zheng]

Catalin, Robin,

On 2016年05月23日 21:35, Catalin Marinas wrote:
> On Mon, May 23, 2016 at 11:44:14AM +0100, Robin Murphy wrote:
>> On 23/05/16 02:37, Shunqian Zheng wrote:
>>> From: Simon <xxm@rock-chips.com>
>>>
>>> Signed-off-by: Simon <xxm@rock-chips.com>
>>> ---
>>>   drivers/iommu/rockchip-iommu.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>>> index 043d18c..1741b65 100644
>>> --- a/drivers/iommu/rockchip-iommu.c
>>> +++ b/drivers/iommu/rockchip-iommu.c
>>> @@ -95,12 +95,16 @@ struct rk_iommu {
>>>
>>>   static inline void rk_table_flush(u32 *va, unsigned int count)
>>>   {
>>> +#if defined(CONFIG_ARM)
>>>   	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;
>>>
>>>   	__cpuc_flush_dcache_area(va, size);
>>>   	outer_flush_range(pa_start, pa_end);
>>> +#elif defined(CONFIG_ARM64)
>>> +	__dma_flush_range(va, va + count);
>>> +#endif
>> Ugh, please don't use arch-private cache maintenance functions directly from
>> a driver. Allocating/mapping page tables to be read by the IOMMU is still
>> DMA, so using the DMA APIs is the correct way to manage them, *especially*
>> if it needs to work across multiple architectures.
It's easier for us if changing  the __dma_flush_range() to 
__flush_dcache_area() is acceptable here?

Thank you,
- shunqian
> I fully agree, these functions should not be used in drivers.

Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu

[Catalin Marinas]

On Tue, May 24, 2016 at 10:31:17AM +0800, Shunqian Zheng wrote:
> On 2016年05月23日 21:35, Catalin Marinas wrote:
> >On Mon, May 23, 2016 at 11:44:14AM +0100, Robin Murphy wrote:
> >>On 23/05/16 02:37, Shunqian Zheng wrote:
> >>>From: Simon <xxm@rock-chips.com>
> >>>
> >>>Signed-off-by: Simon <xxm@rock-chips.com>
> >>>---
> >>>  drivers/iommu/rockchip-iommu.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>>diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> >>>index 043d18c..1741b65 100644
> >>>--- a/drivers/iommu/rockchip-iommu.c
> >>>+++ b/drivers/iommu/rockchip-iommu.c
> >>>@@ -95,12 +95,16 @@ struct rk_iommu {
> >>>
> >>>  static inline void rk_table_flush(u32 *va, unsigned int count)
> >>>  {
> >>>+#if defined(CONFIG_ARM)
> >>>  	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;
> >>>
> >>>  	__cpuc_flush_dcache_area(va, size);
> >>>  	outer_flush_range(pa_start, pa_end);
> >>>+#elif defined(CONFIG_ARM64)
> >>>+	__dma_flush_range(va, va + count);
> >>>+#endif
> >>Ugh, please don't use arch-private cache maintenance functions directly from
> >>a driver. Allocating/mapping page tables to be read by the IOMMU is still
> >>DMA, so using the DMA APIs is the correct way to manage them, *especially*
> >>if it needs to work across multiple architectures.
> 
> It's easier for us if changing  the __dma_flush_range() to
> __flush_dcache_area() is acceptable here?

It's not really acceptable for arm64, nor for arm32. Please fix this
driver in a similar way to commit e3c971960fd4 ("iommu/tegra-smmu:
Convert to use DMA API").

The only place where we allowed __flush_dcache_area() is in the GICv3
driver and that's because it hasn't been wired as a platform device
(yet).

-- 
Catalin