* [PATCH 1/5] iommu/rockchip: fix devm_request_irq and devm_free_irq parameter
2016-05-23 1:37 [PATCH 0/5] iommu/rockchip: fix bugs; enable iommu for ARM64 Shunqian Zheng
@ 2016-05-23 1:37 ` Shunqian Zheng
2016-05-23 9:28 ` Heiko Stuebner
2016-05-23 1:37 ` [PATCH 2/5] iommu/rockchip: add map_sg callback for rk_iommu_ops Shunqian Zheng
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Shunqian Zheng @ 2016-05-23 1:37 UTC (permalink / raw)
To: joro, heiko; +Cc: iommu, linux-kernel, linux-rockchip, Simon
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/5] iommu/rockchip: fix devm_request_irq and devm_free_irq parameter
2016-05-23 1:37 ` [PATCH 1/5] iommu/rockchip: fix devm_request_irq and devm_free_irq parameter Shunqian Zheng
@ 2016-05-23 9:28 ` Heiko Stuebner
0 siblings, 0 replies; 11+ messages in thread
From: Heiko Stuebner @ 2016-05-23 9:28 UTC (permalink / raw)
To: Shunqian Zheng; +Cc: joro, iommu, linux-kernel, linux-rockchip, Simon
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;
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/5] iommu/rockchip: add map_sg callback for rk_iommu_ops
2016-05-23 1:37 [PATCH 0/5] iommu/rockchip: fix bugs; enable iommu for ARM64 Shunqian Zheng
2016-05-23 1:37 ` [PATCH 1/5] iommu/rockchip: fix devm_request_irq and devm_free_irq parameter Shunqian Zheng
@ 2016-05-23 1:37 ` Shunqian Zheng
2016-05-23 1:37 ` [PATCH 3/5] iommu/rockchip: add sanity check for virtual device Shunqian Zheng
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Shunqian Zheng @ 2016-05-23 1:37 UTC (permalink / raw)
To: joro, heiko; +Cc: iommu, linux-kernel, linux-rockchip, Simon
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/5] iommu/rockchip: add sanity check for virtual device
2016-05-23 1:37 [PATCH 0/5] iommu/rockchip: fix bugs; enable iommu for ARM64 Shunqian Zheng
2016-05-23 1:37 ` [PATCH 1/5] iommu/rockchip: fix devm_request_irq and devm_free_irq parameter Shunqian Zheng
2016-05-23 1:37 ` [PATCH 2/5] iommu/rockchip: add map_sg callback for rk_iommu_ops Shunqian Zheng
@ 2016-05-23 1:37 ` Shunqian Zheng
2016-05-23 1:37 ` [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu Shunqian Zheng
2016-05-23 1:37 ` [PATCH 5/5] iommu/rockchip: enable rockchip iommu on ARM64 platform Shunqian Zheng
4 siblings, 0 replies; 11+ messages in thread
From: Shunqian Zheng @ 2016-05-23 1:37 UTC (permalink / raw)
To: joro, heiko; +Cc: iommu, linux-kernel, linux-rockchip, Simon
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu
2016-05-23 1:37 [PATCH 0/5] iommu/rockchip: fix bugs; enable iommu for ARM64 Shunqian Zheng
` (2 preceding siblings ...)
2016-05-23 1:37 ` [PATCH 3/5] iommu/rockchip: add sanity check for virtual device Shunqian Zheng
@ 2016-05-23 1:37 ` Shunqian Zheng
2016-05-23 10:44 ` Robin Murphy
2016-05-23 1:37 ` [PATCH 5/5] iommu/rockchip: enable rockchip iommu on ARM64 platform Shunqian Zheng
4 siblings, 1 reply; 11+ messages in thread
From: Shunqian Zheng @ 2016-05-23 1:37 UTC (permalink / raw)
To: joro, heiko; +Cc: iommu, linux-kernel, linux-rockchip, Simon
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu
2016-05-23 1:37 ` [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu Shunqian Zheng
@ 2016-05-23 10:44 ` Robin Murphy
2016-05-23 13:35 ` Catalin Marinas
0 siblings, 1 reply; 11+ messages in thread
From: Robin Murphy @ 2016-05-23 10:44 UTC (permalink / raw)
To: Shunqian Zheng, joro, heiko, Catalin Marinas, Mark Rutland
Cc: linux-rockchip, iommu, linux-kernel, Simon
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)
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu
2016-05-23 10:44 ` Robin Murphy
@ 2016-05-23 13:35 ` Catalin Marinas
2016-05-24 2:31 ` Shunqian Zheng
0 siblings, 1 reply; 11+ messages in thread
From: Catalin Marinas @ 2016-05-23 13:35 UTC (permalink / raw)
To: Robin Murphy
Cc: Shunqian Zheng, joro, heiko, Mark Rutland, linux-rockchip, iommu,
linux-kernel, Simon
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu
2016-05-23 13:35 ` Catalin Marinas
@ 2016-05-24 2:31 ` Shunqian Zheng
2016-05-24 9:59 ` Catalin Marinas
0 siblings, 1 reply; 11+ messages in thread
From: Shunqian Zheng @ 2016-05-24 2:31 UTC (permalink / raw)
To: Catalin Marinas, Robin Murphy
Cc: joro, heiko, Mark Rutland, linux-rockchip, iommu, linux-kernel,
Simon
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu
2016-05-24 2:31 ` Shunqian Zheng
@ 2016-05-24 9:59 ` Catalin Marinas
0 siblings, 0 replies; 11+ messages in thread
From: Catalin Marinas @ 2016-05-24 9:59 UTC (permalink / raw)
To: Shunqian Zheng
Cc: Robin Murphy, joro, heiko, Mark Rutland, linux-rockchip, iommu,
linux-kernel, Simon
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 5/5] iommu/rockchip: enable rockchip iommu on ARM64 platform
2016-05-23 1:37 [PATCH 0/5] iommu/rockchip: fix bugs; enable iommu for ARM64 Shunqian Zheng
` (3 preceding siblings ...)
2016-05-23 1:37 ` [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu Shunqian Zheng
@ 2016-05-23 1:37 ` Shunqian Zheng
4 siblings, 0 replies; 11+ messages in thread
From: Shunqian Zheng @ 2016-05-23 1:37 UTC (permalink / raw)
To: joro, heiko; +Cc: iommu, linux-kernel, linux-rockchip, Simon
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
^ permalink raw reply related [flat|nested] 11+ messages in thread