Hi,
The following APIs are needed for us to support the legacy Tegra memory manager for devices("NvMap") with *DMA mapping API*.
New API:
->iova_alloc(): To allocate IOVA area. ->iova_alloc_at(): To allocate IOVA area at specific address. ->iova_free(): To free IOVA area.
->map_page_at(): To map page at specific IOVA.
misc: ->iova_get_free_total(): To return how much IOVA is available totally. ->iova_get_free_max(): To return the size of biggest IOVA area.
Although NvMap itself will be replaced soon, there are cases for the above API where we need to specify IOVA explicitly.
(1) HWAs may require the address for special purpose, like reset vector. (2) IOVA linear mapping: ex: [RFC 5/5] ARM: dma-mapping: Introduce dma_map_linear_attrs() for IOVA linear map (3) To support different heaps. To have allocation and mapping independently.
Some of them could be supported with creating different mappings, but currently a device can have a single contiguous mapping, and we cannot specifiy any address inside of a map since all IOVA alloction is done implicitly now.
This is the revised version of:
http://lists.linaro.org/pipermail/linaro-mm-sig/2012-May/001947.html http://lists.linaro.org/pipermail/linaro-mm-sig/2012-May/001948.html http://lists.linaro.org/pipermail/linaro-mm-sig/2012-May/001949.html
Any comment would be really appreciated.
Hiroshi Doyu (5): ARM: dma-mapping: New dma_map_ops->iova_get_free_{total,max} functions ARM: dma-mapping: New dma_map_ops->iova_{alloc,free}() functions ARM: dma-mapping: New dma_map_ops->iova_alloc*_at* function ARM: dma-mapping: New dma_map_ops->map_page*_at* function ARM: dma-mapping: Introduce dma_map_linear_attrs() for IOVA linear map
arch/arm/include/asm/dma-mapping.h | 55 +++++++++++++ arch/arm/mm/dma-mapping.c | 124 ++++++++++++++++++++++++++++++ include/asm-generic/dma-mapping-common.h | 20 +++++ include/linux/dma-mapping.h | 14 ++++ 4 files changed, 213 insertions(+), 0 deletions(-)
->iova>_get_free_total() returns the sum of available free areas. ->iova>_get_free_max() returns the largest available free area size.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/include/asm/dma-mapping.h | 16 ++++++++++ arch/arm/mm/dma-mapping.c | 54 ++++++++++++++++++++++++++++++++++++ include/linux/dma-mapping.h | 3 ++ 3 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index 2300484..1cbd279 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -170,6 +170,22 @@ static inline void dma_free_attrs(struct device *dev, size_t size, ops->free(dev, size, cpu_addr, dma_handle, attrs); }
+static inline size_t dma_iova_get_free_total(struct device *dev) +{ + struct dma_map_ops *ops = get_dma_ops(dev); + BUG_ON(!ops); + + return ops->iova_get_free_total(dev); +} + +static inline size_t dma_iova_get_free_max(struct device *dev) +{ + struct dma_map_ops *ops = get_dma_ops(dev); + BUG_ON(!ops); + + return ops->iova_get_free_max(dev); +} + /** * arm_dma_mmap - map a coherent DMA allocation into user space * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index e4746b7..db17338 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1001,6 +1001,57 @@ fs_initcall(dma_debug_do_init);
/* IOMMU */
+static size_t arm_iommu_iova_get_free_total(struct device *dev) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + unsigned long flags; + size_t size = 0; + unsigned long start = 0; + + BUG_ON(!dev); + BUG_ON(!mapping); + + spin_lock_irqsave(&mapping->lock, flags); + while (1) { + unsigned long end; + + start = bitmap_find_next_zero_area(mapping->bitmap, + mapping->bits, start, 1, 0); + if (start > mapping->bits) + break; + + end = find_next_bit(mapping->bitmap, mapping->bits, start); + size += end - start; + start = end; + } + spin_unlock_irqrestore(&mapping->lock, flags); + return size << (mapping->order + PAGE_SHIFT); +} + +static size_t arm_iommu_iova_get_free_max(struct device *dev) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + unsigned long flags; + size_t max_free = 0; + unsigned long start = 0; + + spin_lock_irqsave(&mapping->lock, flags); + while (1) { + unsigned long end; + + start = bitmap_find_next_zero_area(mapping->bitmap, + mapping->bits, start, 1, 0); + if (start > mapping->bits) + break; + + end = find_next_bit(mapping->bitmap, mapping->bits, start); + max_free = max_t(size_t, max_free, end - start); + start = end; + } + spin_unlock_irqrestore(&mapping->lock, flags); + return max_free << (mapping->order + PAGE_SHIFT); +} + static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping, size_t size) { @@ -1721,6 +1772,9 @@ struct dma_map_ops iommu_ops = { .unmap_sg = arm_iommu_unmap_sg, .sync_sg_for_cpu = arm_iommu_sync_sg_for_cpu, .sync_sg_for_device = arm_iommu_sync_sg_for_device, + + .iova_get_free_total = arm_iommu_iova_get_free_total, + .iova_get_free_max = arm_iommu_iova_get_free_max, };
struct dma_map_ops iommu_coherent_ops = { diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 94af418..0337182 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -53,6 +53,9 @@ struct dma_map_ops { #ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK u64 (*get_required_mask)(struct device *dev); #endif + size_t (*iova_get_free_total)(struct device *dev); + size_t (*iova_get_free_max)(struct device *dev); + int is_phys; };
There are some cases that IOVA allocation and mapping have to be done seperately, especially for perf optimization reasons. This patch allows client modules to {alloc,free} IOVA space without backing up actual pages for that area.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/include/asm/dma-mapping.h | 17 +++++++++++++++++ arch/arm/mm/dma-mapping.c | 17 +++++++++++++++++ include/linux/dma-mapping.h | 2 ++ 3 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index 1cbd279..5b86600 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -170,6 +170,23 @@ static inline void dma_free_attrs(struct device *dev, size_t size, ops->free(dev, size, cpu_addr, dma_handle, attrs); }
+static inline dma_addr_t dma_iova_alloc(struct device *dev, size_t size) +{ + struct dma_map_ops *ops = get_dma_ops(dev); + BUG_ON(!ops); + + return ops->iova_alloc(dev, size); +} + +static inline void dma_iova_free(struct device *dev, dma_addr_t addr, + size_t size) +{ + struct dma_map_ops *ops = get_dma_ops(dev); + BUG_ON(!ops); + + ops->iova_free(dev, addr, size); +} + static inline size_t dma_iova_get_free_total(struct device *dev) { struct dma_map_ops *ops = get_dma_ops(dev); diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index db17338..c18522a 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1080,6 +1080,13 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping, return mapping->base + (start << (mapping->order + PAGE_SHIFT)); }
+static dma_addr_t arm_iommu_iova_alloc(struct device *dev, size_t size) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + + return __alloc_iova(mapping, size); +} + static inline void __free_iova(struct dma_iommu_mapping *mapping, dma_addr_t addr, size_t size) { @@ -1094,6 +1101,14 @@ static inline void __free_iova(struct dma_iommu_mapping *mapping, spin_unlock_irqrestore(&mapping->lock, flags); }
+static void arm_iommu_iova_free(struct device *dev, dma_addr_t addr, + size_t size) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + + __free_iova(mapping, addr, size); +} + static struct page **__iommu_alloc_buffer(struct device *dev, size_t size, gfp_t gfp) { struct page **pages; @@ -1773,6 +1788,8 @@ struct dma_map_ops iommu_ops = { .sync_sg_for_cpu = arm_iommu_sync_sg_for_cpu, .sync_sg_for_device = arm_iommu_sync_sg_for_device,
+ .iova_alloc = arm_iommu_iova_alloc, + .iova_free = arm_iommu_iova_free, .iova_get_free_total = arm_iommu_iova_get_free_total, .iova_get_free_max = arm_iommu_iova_get_free_max, }; diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 0337182..e85aa04 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -53,6 +53,8 @@ struct dma_map_ops { #ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK u64 (*get_required_mask)(struct device *dev); #endif + dma_addr_t (*iova_alloc)(struct device *dev, size_t size); + void (*iova_free)(struct device *dev, dma_addr_t addr, size_t size); size_t (*iova_get_free_total)(struct device *dev); size_t (*iova_get_free_max)(struct device *dev);
To allocate IOVA area at specified address
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/include/asm/dma-mapping.h | 9 +++++++++ arch/arm/mm/dma-mapping.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/dma-mapping.h | 2 ++ 3 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index 5b86600..f04a533 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -187,6 +187,15 @@ static inline void dma_iova_free(struct device *dev, dma_addr_t addr, ops->iova_free(dev, addr, size); }
+static inline dma_addr_t dma_iova_alloc_at(struct device *dev, dma_addr_t addr, + size_t size) +{ + struct dma_map_ops *ops = get_dma_ops(dev); + BUG_ON(!ops); + + return ops->iova_alloc_at(dev, addr, size); +} + static inline size_t dma_iova_get_free_total(struct device *dev) { struct dma_map_ops *ops = get_dma_ops(dev); diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index c18522a..8ca2d1a 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1080,6 +1080,40 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping, return mapping->base + (start << (mapping->order + PAGE_SHIFT)); }
+static dma_addr_t __alloc_iova_at(struct dma_iommu_mapping *mapping, + dma_addr_t iova, size_t size) +{ + unsigned int count, start, orig; + unsigned long flags; + + count = ((PAGE_ALIGN(size) >> PAGE_SHIFT) + + (1 << mapping->order) - 1) >> mapping->order; + + spin_lock_irqsave(&mapping->lock, flags); + + orig = (iova - mapping->base) >> (mapping->order + PAGE_SHIFT); + start = bitmap_find_next_zero_area(mapping->bitmap, mapping->bits, + orig, count, 0); + + if ((start > mapping->bits) || (orig != start)) { + spin_unlock_irqrestore(&mapping->lock, flags); + return DMA_ERROR_CODE; + } + + bitmap_set(mapping->bitmap, start, count); + spin_unlock_irqrestore(&mapping->lock, flags); + + return mapping->base + (start << (mapping->order + PAGE_SHIFT)); +} + +static dma_addr_t arm_iommu_iova_alloc_at(struct device *dev, dma_addr_t iova, + size_t size) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + + return __alloc_iova_at(mapping, iova, size); +} + static dma_addr_t arm_iommu_iova_alloc(struct device *dev, size_t size) { struct dma_iommu_mapping *mapping = dev->archdata.mapping; @@ -1789,6 +1823,7 @@ struct dma_map_ops iommu_ops = { .sync_sg_for_device = arm_iommu_sync_sg_for_device,
.iova_alloc = arm_iommu_iova_alloc, + .iova_alloc_at = arm_iommu_iova_alloc_at, .iova_free = arm_iommu_iova_free, .iova_get_free_total = arm_iommu_iova_get_free_total, .iova_get_free_max = arm_iommu_iova_get_free_max, diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index e85aa04..4cf4427 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -54,6 +54,8 @@ struct dma_map_ops { u64 (*get_required_mask)(struct device *dev); #endif dma_addr_t (*iova_alloc)(struct device *dev, size_t size); + dma_addr_t (*iova_alloc_at)(struct device *dev, dma_addr_t dma_addr, + size_t size); void (*iova_free)(struct device *dev, dma_addr_t addr, size_t size); size_t (*iova_get_free_total)(struct device *dev); size_t (*iova_get_free_max)(struct device *dev);
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/mm/dma-mapping.c | 18 ++++++++++++++++++ include/asm-generic/dma-mapping-common.h | 19 +++++++++++++++++++ include/linux/dma-mapping.h | 7 +++++++ 3 files changed, 44 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 8ca2d1a..242289f 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1715,6 +1715,23 @@ static dma_addr_t arm_iommu_map_page(struct device *dev, struct page *page, return arm_coherent_iommu_map_page(dev, page, offset, size, dir, attrs); }
+static dma_addr_t arm_iommu_map_page_at(struct device *dev, struct page *page, + dma_addr_t dma_addr, unsigned long offset, size_t size, + enum dma_data_direction dir, struct dma_attrs *attrs) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + int ret, len = PAGE_ALIGN(size + offset); + + if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs)) + __dma_page_cpu_to_dev(page, offset, size, dir); + + ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0); + if (ret < 0) + return DMA_ERROR_CODE; + + return dma_addr + offset; +} + /** * arm_coherent_iommu_unmap_page * @dev: valid struct device pointer @@ -1813,6 +1830,7 @@ struct dma_map_ops iommu_ops = { .get_sgtable = arm_iommu_get_sgtable,
.map_page = arm_iommu_map_page, + .map_page_at = arm_iommu_map_page_at, .unmap_page = arm_iommu_unmap_page, .sync_single_for_cpu = arm_iommu_sync_single_for_cpu, .sync_single_for_device = arm_iommu_sync_single_for_device, diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h index de8bf89..eada2d8 100644 --- a/include/asm-generic/dma-mapping-common.h +++ b/include/asm-generic/dma-mapping-common.h @@ -26,6 +26,23 @@ static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr, return addr; }
+static inline dma_addr_t dma_map_single_at_attrs(struct device *dev, void *ptr, + dma_addr_t handle, + size_t size, + enum dma_data_direction dir, + struct dma_attrs *attrs) +{ + struct dma_map_ops *ops = get_dma_ops(dev); + dma_addr_t addr; + + kmemcheck_mark_initialized(ptr, size); + BUG_ON(!valid_dma_direction(dir)); + addr = ops->map_page_at(dev, virt_to_page(ptr), handle, + (unsigned long)ptr & ~PAGE_MASK, size, + dir, attrs); + return addr; +} + static inline void dma_unmap_single_attrs(struct device *dev, dma_addr_t addr, size_t size, enum dma_data_direction dir, @@ -172,6 +189,8 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, }
#define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, NULL) +#define dma_map_single_at(d, a, h, s, r) \ + dma_map_single_at_attrs(d, a, h, s, r, NULL) #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, NULL) #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, NULL) #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, NULL) diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 4cf4427..218bee3 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -25,6 +25,13 @@ struct dma_map_ops { unsigned long offset, size_t size, enum dma_data_direction dir, struct dma_attrs *attrs); + + dma_addr_t (*map_page_at)(struct device *dev, struct page *page, + dma_addr_t dma_handle, + unsigned long offset, size_t size, + enum dma_data_direction dir, + struct dma_attrs *attrs); + void (*unmap_page)(struct device *dev, dma_addr_t dma_handle, size_t size, enum dma_data_direction dir, struct dma_attrs *attrs);
Introduce a helper function, dma_map_linear(_attrs)() to create IOVA linear map, where IOVA and kernel virtual addresses are mapped at the same address linearly. This is useful to support legacy device drivers which expects no IOMMU.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/include/asm/dma-mapping.h | 13 +++++++++++++ include/asm-generic/dma-mapping-common.h | 1 + 2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index f04a533..7a78dd4 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -212,6 +212,19 @@ static inline size_t dma_iova_get_free_max(struct device *dev) return ops->iova_get_free_max(dev); }
+static inline dma_addr_t dma_map_linear_attrs(struct device *dev, void *va, + size_t size, enum dma_data_direction dir, + struct dma_attrs *attrs) +{ + dma_addr_t da; + + da = dma_iova_alloc_at(dev, (dma_addr_t)va, size); + if (da == DMA_ERROR_CODE) + return DMA_ERROR_CODE; + + return dma_map_single_at_attrs(dev, va, da, size, dir, attrs); +} + /** * arm_dma_mmap - map a coherent DMA allocation into user space * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h index eada2d8..4564bf0 100644 --- a/include/asm-generic/dma-mapping-common.h +++ b/include/asm-generic/dma-mapping-common.h @@ -191,6 +191,7 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, NULL) #define dma_map_single_at(d, a, h, s, r) \ dma_map_single_at_attrs(d, a, h, s, r, NULL) +#define dma_map_linear(d, a, s, r) dma_map_linear_attrs(d, a, s, r, NULL) #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, NULL) #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, NULL) #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, NULL)
On Wed, Aug 29, 2012 at 09:55:30AM +0300, Hiroshi Doyu wrote:
The following APIs are needed for us to support the legacy Tegra memory manager for devices("NvMap") with *DMA mapping API*.
Maybe I am not understanding the need completly. Can you elaborate on why this is needed for legacy Tegra?
New API:
->iova_alloc(): To allocate IOVA area. ->iova_alloc_at(): To allocate IOVA area at specific address. ->iova_free(): To free IOVA area.
->map_page_at(): To map page at specific IOVA.
This sounds like a layering violation. The situation today is as follows:
DMA-API : Handle DMA-addresses including an address allocator IOMMU-API : Full control over DMA address space, no address allocator
So what you want to do add to the DMA-API is already part of the IOMMU-API.
Here is my suggestion what you can do instead of extending the DMA-API. You can use the IOMMU-API to initialize the device address space with any mappings at the IOVAs you need the mappings. In the end you allocate another free range in the device address space and use that to satisfy DMA-API allocations. Any reason why that could not work?
Regards,
Joerg
Hi Joerg,
On Tue, 18 Sep 2012 14:49:18 +0200 Joerg Roedel joerg.roedel@amd.com wrote:
On Wed, Aug 29, 2012 at 09:55:30AM +0300, Hiroshi Doyu wrote:
The following APIs are needed for us to support the legacy Tegra memory manager for devices("NvMap") with *DMA mapping API*.
Maybe I am not understanding the need completly. Can you elaborate on why this is needed for legacy Tegra?
Actually not for legacy but it's necessary to replace homebrewed in-kernel API(not upstreamed) with the standard ones. The homebrewed in-kernel API has been used for the abvoe nvmap as its backend. The homebrewed ones are being replaced with the standard ones, IOMMU-API, DMA-API and dma-buf, mainly for transition purpose. I found that some missing features in DMA-API for that. I posted since other SoCs may have the similiar requirements, (1) To specify IOVA address at allocation, and (2) To have IOVA allocation and mapping separately.
New API:
->iova_alloc(): To allocate IOVA area. ->iova_alloc_at(): To allocate IOVA area at specific address. ->iova_free(): To free IOVA area.
->map_page_at(): To map page at specific IOVA.
This sounds like a layering violation. The situation today is as follows:
DMA-API : Handle DMA-addresses including an address allocator IOMMU-API : Full control over DMA address space, no address allocator
So what you want to do add to the DMA-API is already part of the IOMMU-API.
Here is my suggestion what you can do instead of extending the DMA-API. You can use the IOMMU-API to initialize the device address space with any mappings at the IOVAs you need the mappings. In the end you allocate another free range in the device address space and use that to satisfy DMA-API allocations. Any reason why that could not work?
I guess that it would work. Originally I thought that using DMA-API and IOMMU-API together in driver might be kind of layering violation since IOMMU-API itself is used in DMA-API. Only DMA-API used in driver might be cleaner. Considering that DMA API traditionally handling *anonymous* {bus,iova} address only, introducing the concept of specific address in DMA API may not be so encouraged, though.
It would be nice to listen how other SoCs have solved similar needs.
On Wednesday 19 September 2012, Hiroshi Doyu wrote:
I guess that it would work. Originally I thought that using DMA-API and IOMMU-API together in driver might be kind of layering violation since IOMMU-API itself is used in DMA-API. Only DMA-API used in driver might be cleaner. Considering that DMA API traditionally handling anonymous {bus,iova} address only, introducing the concept of specific address in DMA API may not be so encouraged, though.
It would be nice to listen how other SoCs have solved similar needs.
In general, I would recommend using only the IOMMU API when you have a device driver that needs to control the bus virtual address space and that manages a device that resides in its own IOMMU context. I would recommend using only the dma-mapping API when you have a device that lives in a shared bus virtual address space with other devices, and then never ask for a specific bus virtual address.
Can you explain what devices you see that don't fit in one of those two categories?
Arnd
Hi Arnd,
On Wed, 19 Sep 2012 09:59:45 +0200 Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 19 September 2012, Hiroshi Doyu wrote:
I guess that it would work. Originally I thought that using DMA-API and IOMMU-API together in driver might be kind of layering violation since IOMMU-API itself is used in DMA-API. Only DMA-API used in driver might be cleaner. Considering that DMA API traditionally handling anonymous {bus,iova} address only, introducing the concept of specific address in DMA API may not be so encouraged, though.
It would be nice to listen how other SoCs have solved similar needs.
In general, I would recommend using only the IOMMU API when you have a device driver that needs to control the bus virtual address space and that manages a device that resides in its own IOMMU context. I would recommend using only the dma-mapping API when you have a device that lives in a shared bus virtual address space with other devices, and then never ask for a specific bus virtual address.
Can you explain what devices you see that don't fit in one of those two categories?
I think that the above fis, but I'll continue to explain our case a little bit more below:
In Tegra, there's a few dozen of IOMMU'able devices. Each of them can be configured to enable/disable IOMMU. Also some IOMMU Address Space IDs(ASID) can be assigned to each device respectively. Some of devices are just traditional ones to use traditional dma-mapping API only, like normal SD/MMC. Some of devices require some specific IOVA address for reset vector and MMIO. For example, Tegra has another ARM(ARM7) as such. For traditional devices, dma mapping API is so nice that driver doesn't have to be aware of IOMMU. The same dma mapping API works with/without IOMMU.
If both devices are attached to the same mapping, IOMMU-API and dma-mapping API would be used together from different devices. Technically this can be avoided to assign different maps to each device, though.
On Wed, Sep 19, 2012 at 07:59:45AM +0000, Arnd Bergmann wrote:
On Wednesday 19 September 2012, Hiroshi Doyu wrote:
I guess that it would work. Originally I thought that using DMA-API and IOMMU-API together in driver might be kind of layering violation since IOMMU-API itself is used in DMA-API. Only DMA-API used in driver might be cleaner. Considering that DMA API traditionally handling anonymous {bus,iova} address only, introducing the concept of specific address in DMA API may not be so encouraged, though.
It would be nice to listen how other SoCs have solved similar needs.
In general, I would recommend using only the IOMMU API when you have a device driver that needs to control the bus virtual address space and that manages a device that resides in its own IOMMU context. I would recommend using only the dma-mapping API when you have a device that lives in a shared bus virtual address space with other devices, and then never ask for a specific bus virtual address.
Can you explain what devices you see that don't fit in one of those two categories?
Well, I don't think that a driver should limit to one of these 2 APIs. A driver can very well use the IOMMU-API during initialization (for example to map the firmware to an address the device expects it to be) and use the DMA-API later during normal operation to exchange data with the device.
When a device driver would only use the IOMMU-API and needs small DMA-able areas it has to re-implement something like the DMA-API (basically an address allocator) for that. So I don't see a reason why both can't be used in a device driver.
Regards,
Joerg
When a device driver would only use the IOMMU-API and needs small DMA- able areas it has to re-implement something like the DMA-API (basically an address allocator) for that. So I don't see a reason why both can't be used in a device driver.
On Tegra, the following use cases need specific IOVA mapping. 1. Few MMIO blocks need IOVA=PA mapping setup. 2. CPU side loads the firmware into physical memory, which has to be mapped to a specific IOVA address, as firmware is statically linked based on specific IOVA address.
DMA api's allow specifying only one address space per platform device.
For #1, DMA API can't be used as it doesn't allow mapping specific IOVA to PA. IOMMU API can be used for mapping specific IOVA to PA. But, in order to use IOMMU API, the driver has to dereference the dev pointer, get domain ptr, take lock, and allocate memory from dma_iommu_mapping. This breaks the abstraction for struct device. Each device driver that need IOVA=PA has to do this, which is redundant.
For #2, physical memory allocations alone can be done through DMA as it also allocates IOVA space Implicitly. Even after allocating physical memory through DMA API's, it would have same problem as #1 for IOVA to PA mapping.
If a fake device is expected to be created for specific IOVA allocation, then it may lead to creating multiple fake devices per specific IOVA and per ASID(unique IOVA address space). As domain init would be done based on device name, the fake device should have the same name as of original platform device.
If DMA API allows allocating specific IOVA address and mapping IOVA to specific PA, device driver don't need to know any details of struct device and specifying one mapping per device is enough and no need for fake devices.
Comments are much appreciated.
-KR
-----Original Message----- From: Joerg Roedel [mailto:joerg.roedel@amd.com] Sent: Wednesday, September 19, 2012 5:50 AM To: Arnd Bergmann Cc: Hiroshi Doyu; m.szyprowski@samsung.com; linux@arm.linux.org.uk; minchan@kernel.org; chunsang.jeong@linaro.org; linux- kernel@vger.kernel.org; subashrp@gmail.com; linaro-mm-sig@lists.linaro.org; linux-mm@kvack.org; iommu@lists.linux-foundation.org; Krishna Reddy; linux- tegra@vger.kernel.org; kyungmin.park@samsung.com; pullip.cho@samsung.com; linux-arm-kernel@lists.infradead.org Subject: Re: [RFC 0/5] ARM: dma-mapping: New dma_map_ops to control IOVA more precisely
On Wed, Sep 19, 2012 at 07:59:45AM +0000, Arnd Bergmann wrote:
On Wednesday 19 September 2012, Hiroshi Doyu wrote:
I guess that it would work. Originally I thought that using DMA-API and IOMMU-API together in driver might be kind of layering violation since IOMMU-API itself is used in DMA-API. Only DMA-API used in driver might be cleaner. Considering that DMA API traditionally handling anonymous {bus,iova} address only, introducing the concept of specific address in DMA API may not be so encouraged, though.
It would be nice to listen how other SoCs have solved similar needs.
In general, I would recommend using only the IOMMU API when you have a device driver that needs to control the bus virtual address space and that manages a device that resides in its own IOMMU context. I would recommend using only the dma-mapping API when you have a device that lives in a shared bus virtual address space with other devices, and then never ask for a specific bus virtual address.
Can you explain what devices you see that don't fit in one of those two categories?
Well, I don't think that a driver should limit to one of these 2 APIs. A driver can very well use the IOMMU-API during initialization (for example to map the firmware to an address the device expects it to be) and use the DMA-API later during normal operation to exchange data with the device.
When a device driver would only use the IOMMU-API and needs small DMA- able areas it has to re-implement something like the DMA-API (basically an address allocator) for that. So I don't see a reason why both can't be used in a device driver.
Regards,
Joerg
-- AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
On 09/19/2012 07:44 PM, Krishna Reddy wrote:
When a device driver would only use the IOMMU-API and needs small DMA- able areas it has to re-implement something like the DMA-API (basically an address allocator) for that. So I don't see a reason why both can't be used in a device driver.
On Tegra, the following use cases need specific IOVA mapping.
- Few MMIO blocks need IOVA=PA mapping setup.
In that case, why would we enable the IOMMU for that one device; IOMMU disabled means VA==PA, right? Perhaps isolation of the device so it can only access certain PA ranges for security?
On Tegra, the following use cases need specific IOVA mapping.
- Few MMIO blocks need IOVA=PA mapping setup.
In that case, why would we enable the IOMMU for that one device; IOMMU disabled means VA==PA, right? Perhaps isolation of the device so it can only access certain PA ranges for security?
The device(H/W controller) need to access few special memory blocks(IOVA==PA) and DRAM as well. If IOMMU is disabled, then it has to handle memory fragmentation, which defeats the purpose of IOMMU support. There is also a case where frame buffer memory is passed from BootLoader to Kernel and display H/W continues to access it with IOMMU enabled. To support this, the one to one mapping has to be setup before enabling IOMMU.
-KR
On 09/20/2012 12:40 AM, Krishna Reddy wrote:
On Tegra, the following use cases need specific IOVA mapping.
- Few MMIO blocks need IOVA=PA mapping setup.
In that case, why would we enable the IOMMU for that one device; IOMMU disabled means VA==PA, right? Perhaps isolation of the device so it can only access certain PA ranges for security?
The device(H/W controller) need to access few special memory blocks(IOVA==PA) and DRAM as well.
OK, so only /some/ of the VA space is VA==PA, and some is remapped; that's a little different that what you originally implied above.
BTW, which HW module is this; AVP/COP or something else. This sounds like an odd requirement.
There is also a case where frame buffer memory is passed from BootLoader to Kernel and display H/W continues to access it with IOMMU enabled. To support this, the one to one mapping has to be setup before enabling IOMMU.
Yes, that makes sense.
The device(H/W controller) need to access few special memory blocks(IOVA==PA) and DRAM as well.
OK, so only /some/ of the VA space is VA==PA, and some is remapped; that's a little different that what you originally implied above.
BTW, which HW module is this; AVP/COP or something else. This sounds like an odd requirement.
This is not specific to ARM7. There are protected memory regions on Tegra that can be accessed by some controllers like display, 2D, 3D, VDE, HDA. These are DRAM regions configured as protected by BootRom. These memory regions are not exposed to and not managed by OS page allocator. The H/W controller accesses to these regions still to go through IOMMU. The IOMMU view for all the H/W controllers is not uniform on Tegra. Some Controllers see entire 4GB IOVA space. i.e all accesses go though IOMMU. Some controllers see the IOVA Space that don't overlap with MMIO space. i.e The MMIO address access bypass IOMMU and directly go to MMIO space. Tegra IOMMU can support multiple address spaces as well. To hide controller Specific behavior, the drivers should take care of one to one mapping and remove inaccessible iova spaces in their address space's based platform device info.
In my initial mail, I referred protected memory regions as MMIO blocks, which is incorrect.
-KR
On Fri, 21 Sep 2012 20:16:00 +0200 Krishna Reddy vdumpa@nvidia.com wrote:
The device(H/W controller) need to access few special memory blocks(IOVA==PA) and DRAM as well.
OK, so only /some/ of the VA space is VA==PA, and some is remapped; that's a little different that what you originally implied above.
BTW, which HW module is this; AVP/COP or something else. This sounds like an odd requirement.
This is not specific to ARM7. There are protected memory regions on Tegra that can be accessed by some controllers like display, 2D, 3D, VDE, HDA. These are DRAM regions configured as protected by BootRom. These memory regions are not exposed to and not managed by OS page allocator. The H/W controller accesses to these regions still to go through IOMMU. The IOMMU view for all the H/W controllers is not uniform on Tegra. Some Controllers see entire 4GB IOVA space. i.e all accesses go though IOMMU. Some controllers see the IOVA Space that don't overlap with MMIO space. i.e The MMIO address access bypass IOMMU and directly go to MMIO space. Tegra IOMMU can support multiple address spaces as well. To hide controller Specific behavior, the drivers should take care of one to one mapping and remove inaccessible iova spaces in their address space's based platform device info.
The above is also related to another issue, how to specify IOMMU'able devices in DT.
As mentioned above, some IOVA mapping may be unique to some devices, and the number of IOMMU'able device are quite many nowadays, a few dozen in Tegra30 now. Basically they are seen as just normal platform devices from CPU even if they belong to different busses in H/W. IOW, their IOMMU'ability just depend on a platfrom bus from _S/W_ POV. Doing each registration(create a map & attach device) in board files isn't so nice. Currently we register them at "platform_device_add()" at once with just a HACK(*1), but this could/should be done based on the info passed from DT. For tegra, those parameter could be, "ASID" and "address range"(start, size, alignment). For example in DT:
deviceA { "start" "size" "align" iommu = <0x12340000 0x00400000 0x0000000>; # exclusively specify "start" or "align" iommu = <0x00000000 0x00400000 0x0010000>; iommu = <0x12340000 0x00040000 0x12380000 0x00040000>; # "start", "size" could be repeated... asid = 3; # if needed
or dma_range = <0x12340000 0x00400000 0x0000000>; # if iommu is considered as one implementation of dma..... };
Is there any way to specify each IOMMU'able _platform device_ and specify its map in DT?
The above ASID may be specific to Tegra, though. If we can specify the above info in DT and the info is passed to kernel, some platform common code would register them as IOMMU'able device automatically. It would be really covenient if this is done in platform_device/IOMMU common code. If the above attribute is implemented specific to Tegra/platform, we have to call attach_device quite many times somewhere in device initializations.
Any comment would be really appreciated.
*1:
From dd4dd6532d705c7bba0914b54c819d8d735c2fad Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu hdoyu@nvidia.com Date: Thu, 22 Mar 2012 16:06:27 +0200 Subject: [RFC 1/1] platform: IOMMU'able platform_device w/ PLATFORM_ENABLE_IOMMU
Introduced a new kernel config option, PLATFORM_ENABLE_IOMMU. With this, all platform devices can be converted to be IOMMU'able if platform_bus has non-null dma_iommu_map, where H/Ws always see its IO virtual address and virt_to_phys() doesn't work from H/W POV.
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/mm/dma-mapping.c | 7 +++++++ drivers/base/Kconfig | 4 ++++ drivers/base/platform.c | 17 +++++++++++++++-- drivers/iommu/Kconfig | 2 +- include/linux/device.h | 5 ++++- 5 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 242289f..28ca7c2 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1899,6 +1899,13 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, mapping->order = order; spin_lock_init(&mapping->lock);
+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU + if (WARN_ON(bus->map)) + goto err3; + + bus->map = mapping; +#endif + mapping->domain = iommu_domain_alloc(bus); if (!mapping->domain) goto err3; diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 3df339c..0f45311 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -308,4 +308,8 @@ config CMA_AREAS
endif
+config PLATFORM_ENABLE_IOMMU + bool "Make platform devices iommuable" + depends on IOMMU_API + endmenu diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h> + #include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev); - if (ret == 0) - return ret; + if (ret) + goto failed; + +#ifdef CONFIG_PLATFORM_ENABLE_IOMMU + if (platform_bus_type.map && !pdev->dev.archdata.mapping) { + ret = arm_iommu_attach_device(&pdev->dev, + platform_bus_type.map); + if (ret) + goto failed; + } +#endif + + return 0;
failed: while (--i >= 0) { diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index b736809..8b7eca1 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -164,7 +164,7 @@ config TEGRA_IOMMU_SMMU
config TEGRA_IOMMU_SMMU_LINEAR bool "Physical RAM IOVA Liner Mapping Support" - depends on TEGRA_IOMMU_SMMU + depends on TEGRA_IOMMU_SMMU && !PLATFORM_ENABLE_IOMMU default y help Enables support for linear mapping between physical address diff --git a/include/linux/device.h b/include/linux/device.h index e339929..3dcb501 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -35,6 +35,7 @@ struct subsys_private; struct bus_type; struct device_node; struct iommu_ops; +struct dma_iommu_mapping;
struct bus_attribute { struct attribute attr; @@ -106,7 +107,9 @@ struct bus_type { const struct dev_pm_ops *pm;
struct iommu_ops *iommu_ops; - +#ifdef CONFIG_PLATFORM_ENABLE_IOMMU + struct dma_iommu_mapping *map; +#endif struct subsys_private *p; };
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h>
#include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
if (ret == 0)return ret;
if (ret)goto failed;+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU
if (platform_bus_type.map && !pdev->dev.archdata.mapping) {ret = arm_iommu_attach_device(&pdev->dev,platform_bus_type.map);if (ret)goto failed;
This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
James
Hi James,
On Mon, 24 Sep 2012 11:28:01 +0200 James Bottomley James.Bottomley@HansenPartnership.com wrote:
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h>
#include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
if (ret == 0)return ret;
if (ret)goto failed;+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU
if (platform_bus_type.map && !pdev->dev.archdata.mapping) {ret = arm_iommu_attach_device(&pdev->dev,platform_bus_type.map);if (ret)goto failed;This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
As mentioned in the original, this is a heck to explain what is needed. I am looking for some generic solution for how to specify IOMMU info for each platform devices. I'm guessing that some other SoC may have the similar requirements on the above. As you mentioned, this solution should be a generic, not arch specific.
Hello,
On Monday, September 24, 2012 11:45 AM Hiroshi Doyu wrote:
On Mon, 24 Sep 2012 11:28:01 +0200 James Bottomley James.Bottomley@HansenPartnership.com wrote:
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h>
#include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
if (ret == 0)return ret;
if (ret)goto failed;+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU
if (platform_bus_type.map && !pdev->dev.archdata.mapping) {ret = arm_iommu_attach_device(&pdev->dev,platform_bus_type.map);if (ret)goto failed;This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
As mentioned in the original, this is a heck to explain what is needed. I am looking for some generic solution for how to specify IOMMU info for each platform devices. I'm guessing that some other SoC may have the similar requirements on the above. As you mentioned, this solution should be a generic, not arch specific.
Please read more about bus notifiers. IMHO a good example is provided in the following thread: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html
Best regards
Hi Marek,
Marek Szyprowski m.szyprowski@samsung.com wrote @ Mon, 24 Sep 2012 13:14:51 +0200:
Hello,
On Monday, September 24, 2012 11:45 AM Hiroshi Doyu wrote:
On Mon, 24 Sep 2012 11:28:01 +0200 James Bottomley James.Bottomley@HansenPartnership.com wrote:
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h>
#include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
if (ret == 0)return ret;
if (ret)goto failed;+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU
if (platform_bus_type.map && !pdev->dev.archdata.mapping) {ret = arm_iommu_attach_device(&pdev->dev,platform_bus_type.map);if (ret)goto failed;This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
As mentioned in the original, this is a heck to explain what is needed. I am looking for some generic solution for how to specify IOMMU info for each platform devices. I'm guessing that some other SoC may have the similar requirements on the above. As you mentioned, this solution should be a generic, not arch specific.
Please read more about bus notifiers. IMHO a good example is provided in the following thread: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html
This bus notifier seems enough flexible to afford the variation of IOMMU map info, like Tegra ASID, which could be platform-specific, and the other could be common too. There's already iommu_bus_notifier too. I'll try to implement something base on this.
Thanks for the good info.
Hiroshi Doyu hdoyu@nvidia.com wrote @ Mon, 24 Sep 2012 14:50:14 +0300 (EEST): ...
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h>
#include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
if (ret == 0)return ret;
if (ret)goto failed;+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU
if (platform_bus_type.map && !pdev->dev.archdata.mapping) {ret = arm_iommu_attach_device(&pdev->dev,platform_bus_type.map);if (ret)goto failed;This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
As mentioned in the original, this is a heck to explain what is needed. I am looking for some generic solution for how to specify IOMMU info for each platform devices. I'm guessing that some other SoC may have the similar requirements on the above. As you mentioned, this solution should be a generic, not arch specific.
Please read more about bus notifiers. IMHO a good example is provided in the following thread: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html
This bus notifier seems enough flexible to afford the variation of IOMMU map info, like Tegra ASID, which could be platform-specific, and the other could be common too. There's already iommu_bus_notifier too. I'll try to implement something base on this.
Experimentally implemented as below. With the followig patch, each device could specify its own map in DT, and automatically the device would be attached to the map.
There is a case that some devices share a map. This patch doesn't suppor such case yet.
From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu hdoyu@nvidia.com Date: Wed, 28 Nov 2012 14:47:04 +0200 Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices
platform_bus notifier registers IOMMU devices if dma-window is specified.
Its format is: dma-window = <"start" "size">; ex) dma-window = <0x12345000 0x8000>;
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com --- arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c index a2b6cf1..570d718 100644 --- a/arch/arm/mach-tegra/board-dt-tegra30.c +++ b/arch/arm/mach-tegra/board-dt-tegra30.c @@ -30,9 +30,11 @@ #include <linux/of_fdt.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/of_iommu.h>
#include <asm/mach/arch.h> #include <asm/hardware/gic.h> +#include <asm/dma-iommu.h>
#include "board.h" #include "clock.h" @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { { NULL, NULL, 0, 0}, };
+#ifdef CONFIG_ARM_DMA_USE_IOMMU +static int tegra_iommu_device_notifier(struct notifier_block *nb, + unsigned long event, void *_dev) +{ + struct dma_iommu_mapping *map = NULL; + struct device *dev = _dev; + dma_addr_t base; + size_t size; + int err; + + switch (event) { + case BUS_NOTIFY_ADD_DEVICE: + err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, + &size); + if (!err) + map = arm_iommu_create_mapping(&platform_bus_type, + base, size, 0); + if (IS_ERR_OR_NULL(map)) + break; + if (arm_iommu_attach_device(dev, map)) + dev_err(dev, "Failed to attach %s\n", dev_name(dev)); + dev_dbg(dev, "Attached %s to map %p\n", dev_name(dev), map); + break; + } + return NOTIFY_DONE; +} +#else +#define tegra_iommu_device_notifier NULL +#endif + +static struct notifier_block tegra_iommu_device_nb = { + .notifier_call = tegra_iommu_device_notifier, +}; + static void __init tegra30_dt_init(void) { tegra_clk_init_from_table(tegra_dt_clk_init_table);
+ if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) + bus_register_notifier(&platform_bus_type, + &tegra_iommu_device_nb); + of_platform_populate(NULL, of_default_bus_match_table, tegra30_auxdata_lookup, NULL); }
On 11/28/2012 06:48 AM, Hiroshi Doyu wrote:
Hiroshi Doyu hdoyu@nvidia.com wrote @ Mon, 24 Sep 2012 14:50:14 +0300 (EEST): ...
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h>
#include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
if (ret == 0)return ret;
if (ret)goto failed;+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU
if (platform_bus_type.map && !pdev->dev.archdata.mapping) {ret = arm_iommu_attach_device(&pdev->dev,platform_bus_type.map);if (ret)goto failed;This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
As mentioned in the original, this is a heck to explain what is needed. I am looking for some generic solution for how to specify IOMMU info for each platform devices. I'm guessing that some other SoC may have the similar requirements on the above. As you mentioned, this solution should be a generic, not arch specific.
Please read more about bus notifiers. IMHO a good example is provided in the following thread: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html
This bus notifier seems enough flexible to afford the variation of IOMMU map info, like Tegra ASID, which could be platform-specific, and the other could be common too. There's already iommu_bus_notifier too. I'll try to implement something base on this.
Experimentally implemented as below. With the followig patch, each device could specify its own map in DT, and automatically the device would be attached to the map.
There is a case that some devices share a map. This patch doesn't suppor such case yet.
From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 From: Hiroshi Doyu hdoyu@nvidia.com Date: Wed, 28 Nov 2012 14:47:04 +0200 Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices
platform_bus notifier registers IOMMU devices if dma-window is specified.
Its format is: dma-window = <"start" "size">; ex) dma-window = <0x12345000 0x8000>;
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++
Shouldn't this patch be to the IOMMU driver itself, not the core Tegra code?
On Wed, 28 Nov 2012 19:07:46 +0100 Stephen Warren swarren@wwwdotorg.org wrote: ......
Please read more about bus notifiers. IMHO a good example is provided in the following thread: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html
This bus notifier seems enough flexible to afford the variation of IOMMU map info, like Tegra ASID, which could be platform-specific, and the other could be common too. There's already iommu_bus_notifier too. I'll try to implement something base on this.
Experimentally implemented as below. With the followig patch, each device could specify its own map in DT, and automatically the device would be attached to the map.
There is a case that some devices share a map. This patch doesn't suppor such case yet.
From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 From: Hiroshi Doyu hdoyu@nvidia.com Date: Wed, 28 Nov 2012 14:47:04 +0200 Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices
platform_bus notifier registers IOMMU devices if dma-window is specified.
Its format is: dma-window = <"start" "size">; ex) dma-window = <0x12345000 0x8000>;
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++
Shouldn't this patch be to the IOMMU driver itself, not the core Tegra code?
That could be possible and cleaner. I'll check if it works.
On Wed, Nov 28, 2012 at 02:48:32PM +0100, Hiroshi Doyu wrote: [...]
From: Hiroshi Doyu hdoyu@nvidia.com Date: Wed, 28 Nov 2012 14:47:04 +0200 Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices
platform_bus notifier registers IOMMU devices if dma-window is specified.
Its format is: dma-window = <"start" "size">; ex) dma-window = <0x12345000 0x8000>;
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c index a2b6cf1..570d718 100644 --- a/arch/arm/mach-tegra/board-dt-tegra30.c +++ b/arch/arm/mach-tegra/board-dt-tegra30.c @@ -30,9 +30,11 @@ #include <linux/of_fdt.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/of_iommu.h> #include <asm/mach/arch.h> #include <asm/hardware/gic.h> +#include <asm/dma-iommu.h> #include "board.h" #include "clock.h" @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { { NULL, NULL, 0, 0}, }; +#ifdef CONFIG_ARM_DMA_USE_IOMMU +static int tegra_iommu_device_notifier(struct notifier_block *nb,
unsigned long event, void *_dev)+{
- struct dma_iommu_mapping *map = NULL;
- struct device *dev = _dev;
- dma_addr_t base;
- size_t size;
- int err;
- switch (event) {
- case BUS_NOTIFY_ADD_DEVICE:
err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base,&size);if (!err)map = arm_iommu_create_mapping(&platform_bus_type,base, size, 0);if (IS_ERR_OR_NULL(map))break;if (arm_iommu_attach_device(dev, map))dev_err(dev, "Failed to attach %s\n", dev_name(dev));dev_dbg(dev, "Attached %s to map %p\n", dev_name(dev), map);break;- }
- return NOTIFY_DONE;
+} +#else +#define tegra_iommu_device_notifier NULL +#endif
+static struct notifier_block tegra_iommu_device_nb = {
- .notifier_call = tegra_iommu_device_notifier,
+};
You don't need this extra protection since you use IS_ENABLED below and these are all static variables. The whole point of IS_ENABLED is to allow full compile coverage while leaving it up to the compiler to eliminate dead code.
Thierry
On 11/28/2012 09:48 PM, Hiroshi Doyu wrote:
Hiroshi Doyu hdoyu@nvidia.com wrote @ Mon, 24 Sep 2012 14:50:14 +0300 (EEST): ...
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index a1a7225..9eae3be 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -21,6 +21,8 @@ #include <linux/slab.h> #include <linux/pm_runtime.h>
+#include <asm/dma-iommu.h>
#include "base.h"
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device *pdev) dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
if (ret == 0)return ret;
if (ret)goto failed;+#ifdef CONFIG_PLATFORM_ENABLE_IOMMU
if (platform_bus_type.map && !pdev->dev.archdata.mapping) {ret = arm_iommu_attach_device(&pdev->dev,platform_bus_type.map);if (ret)goto failed;This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
As mentioned in the original, this is a heck to explain what is needed. I am looking for some generic solution for how to specify IOMMU info for each platform devices. I'm guessing that some other SoC may have the similar requirements on the above. As you mentioned, this solution should be a generic, not arch specific.
Please read more about bus notifiers. IMHO a good example is provided in the following thread: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html
This bus notifier seems enough flexible to afford the variation of IOMMU map info, like Tegra ASID, which could be platform-specific, and the other could be common too. There's already iommu_bus_notifier too. I'll try to implement something base on this.
Experimentally implemented as below. With the followig patch, each device could specify its own map in DT, and automatically the device would be attached to the map.
There is a case that some devices share a map. This patch doesn't suppor such case yet.
From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 From: Hiroshi Doyu hdoyu@nvidia.com Date: Wed, 28 Nov 2012 14:47:04 +0200 Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices
platform_bus notifier registers IOMMU devices if dma-window is specified.
Its format is: dma-window = <"start" "size">; ex) dma-window = <0x12345000 0x8000>;
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c index a2b6cf1..570d718 100644 --- a/arch/arm/mach-tegra/board-dt-tegra30.c +++ b/arch/arm/mach-tegra/board-dt-tegra30.c @@ -30,9 +30,11 @@ #include <linux/of_fdt.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/of_iommu.h> #include <asm/mach/arch.h> #include <asm/hardware/gic.h> +#include <asm/dma-iommu.h> #include "board.h" #include "clock.h" @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { { NULL, NULL, 0, 0}, }; +#ifdef CONFIG_ARM_DMA_USE_IOMMU +static int tegra_iommu_device_notifier(struct notifier_block *nb,
unsigned long event, void *_dev)+{
- struct dma_iommu_mapping *map = NULL;
- struct device *dev = _dev;
- dma_addr_t base;
- size_t size;
- int err;
- switch (event) {
- case BUS_NOTIFY_ADD_DEVICE:
err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base,&size);if (!err)map = arm_iommu_create_mapping(&platform_bus_type,base, size, 0);if (IS_ERR_OR_NULL(map))break;if (arm_iommu_attach_device(dev, map))
Add "arm_iommu_release_mapping" here.
And finally we see this patch, that's great. :)
dev_err(dev, "Failed to attach %s\n", dev_name(dev));dev_dbg(dev, "Attached %s to map %p\n", dev_name(dev), map);break;- }
- return NOTIFY_DONE;
+} +#else +#define tegra_iommu_device_notifier NULL +#endif
+static struct notifier_block tegra_iommu_device_nb = {
- .notifier_call = tegra_iommu_device_notifier,
+};
static void __init tegra30_dt_init(void) { tegra_clk_init_from_table(tegra_dt_clk_init_table);
- if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU))
bus_register_notifier(&platform_bus_type,&tegra_iommu_device_nb);- of_platform_populate(NULL, of_default_bus_match_table, tegra30_auxdata_lookup, NULL);
}
Mark Zhang nvmarkzhang@gmail.com wrote @ Fri, 30 Nov 2012 05:59:32 +0100:
On 11/28/2012 09:48 PM, Hiroshi Doyu wrote:
Hiroshi Doyu hdoyu@nvidia.com wrote @ Mon, 24 Sep 2012 14:50:14 +0300 (EEST): ...
On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote: > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index a1a7225..9eae3be 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -21,6 +21,8 @@ > #include <linux/slab.h> > #include <linux/pm_runtime.h> > > +#include <asm/dma-iommu.h> > + > #include "base.h" > > #define to_platform_driver(drv) (container_of((drv), struct > platform_driver, \ > @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device > *pdev) > dev_name(&pdev->dev), dev_name(pdev->dev.parent)); > > ret = device_add(&pdev->dev); > - if (ret == 0) > - return ret; > + if (ret) > + goto failed; > + > +#ifdef CONFIG_PLATFORM_ENABLE_IOMMU > + if (platform_bus_type.map && !pdev->dev.archdata.mapping) { > + ret = arm_iommu_attach_device(&pdev->dev, > + platform_bus_type.map); > + if (ret) > + goto failed;
This is horrible ... you're adding an architecture specific callback into our generic code; that's really a no-no. If the concept of CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this could become a generic callback.
As mentioned in the original, this is a heck to explain what is needed. I am looking for some generic solution for how to specify IOMMU info for each platform devices. I'm guessing that some other SoC may have the similar requirements on the above. As you mentioned, this solution should be a generic, not arch specific.
Please read more about bus notifiers. IMHO a good example is provided in the following thread: http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html
This bus notifier seems enough flexible to afford the variation of IOMMU map info, like Tegra ASID, which could be platform-specific, and the other could be common too. There's already iommu_bus_notifier too. I'll try to implement something base on this.
Experimentally implemented as below. With the followig patch, each device could specify its own map in DT, and automatically the device would be attached to the map.
There is a case that some devices share a map. This patch doesn't suppor such case yet.
From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 From: Hiroshi Doyu hdoyu@nvidia.com Date: Wed, 28 Nov 2012 14:47:04 +0200 Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices
platform_bus notifier registers IOMMU devices if dma-window is specified.
Its format is: dma-window = <"start" "size">; ex) dma-window = <0x12345000 0x8000>;
Signed-off-by: Hiroshi Doyu hdoyu@nvidia.com
arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c index a2b6cf1..570d718 100644 --- a/arch/arm/mach-tegra/board-dt-tegra30.c +++ b/arch/arm/mach-tegra/board-dt-tegra30.c @@ -30,9 +30,11 @@ #include <linux/of_fdt.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/of_iommu.h> #include <asm/mach/arch.h> #include <asm/hardware/gic.h> +#include <asm/dma-iommu.h> #include "board.h" #include "clock.h" @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { { NULL, NULL, 0, 0}, }; +#ifdef CONFIG_ARM_DMA_USE_IOMMU +static int tegra_iommu_device_notifier(struct notifier_block *nb,
unsigned long event, void *_dev)+{
- struct dma_iommu_mapping *map = NULL;
- struct device *dev = _dev;
- dma_addr_t base;
- size_t size;
- int err;
- switch (event) {
- case BUS_NOTIFY_ADD_DEVICE:
err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base,&size);if (!err)map = arm_iommu_create_mapping(&platform_bus_type,base, size, 0);if (IS_ERR_OR_NULL(map))break;if (arm_iommu_attach_device(dev, map))Add "arm_iommu_release_mapping" here.
Yes.
And finally we see this patch, that's great. :)
I'll move the location of patch to drivers/iommu/tegra-smmu.c and repost.
linaro-mm-sig@lists.linaro.org
-
Arnd Bergmann -
Hiroshi Doyu -
James Bottomley -
Joerg Roedel -
Krishna Reddy -
Marek Szyprowski -
Mark Zhang -
Stephen Warren -
Thierry Reding