[PATCHv7 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[PATCHv7 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Niklas Söderlund]

Hi,

[In this v7 series I have tried to address the questions raised by Christoph 
Hellwig and I hope it can awnser your concernes regarding dma-debug.]

This series tries to solve the problem with DMA with device registers
(MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
recent patch '9575632 (dmaengine: make slave address physical)'
clarifies that DMA slave address provided by clients is the physical
address. This puts the task of mapping the DMA slave address from a
phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
the same and no special care is needed. However if you have a IOMMU you
need to map the DMA slave phys_addr_t to a dma_addr_t using something
like this.

This series is based on top of v4.7-rc1.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the
ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with
/dev/mmcblk1, i2c and the serial console which are devices behind the
iommu.

Furthermore I have audited to the best of my ability all call paths
involved to make sure that the dma_addr_t obtained from
dma_map_resource() to is not used in a way where it would be expected
for the mapping to be RAM (have a struct page). Many thanks to Christoph
Hellwig and Laurent Pinchart for there input in this effort.

  * drivers/dma/sh/rcar-dmac.c
    Once the phys_addr_t is mapped to a dma_addr_t using
    dma_map_resource() it is only used to check that the transferee do not
    cross 4GB boundaries and then only directly written to HW registers.

  * drivers/iommu/iommu.c
    - iommu_map()
      Check that it's align to min page size or return -EINVAL then calls
      domain->ops->map()

  * drivers/iommu/ipmmu-vmsa.c
    - ipmmu_map()
      No logic only calls domain->ops->map()

  * drivers/iommu/io-pgtable-arm.c
    - arm_lpae_map()
      No logic only calls __arm_lpae_map()
    - __arm_lpae_map()
      No logic only calls arm_lpae_init_pte()
    - arm_lpae_init_pte()
      Used to get a pte:
        pte |= pfn_to_iopte(paddr >> data->pg_shift, data);

  * drivers/iommu/io-pgtable-arm-v7s.c
    - arm_v7s_map()
      No logic only calls __arm_v7s_map()
    - __arm_v7s_map()
      No logic only calls arm_v7s_init_pte()
    - arm_v7s_init_pte
      Used to get a pte:
        pte |= paddr & ARM_V7S_LVL_MASK(lvl);

  * ARM dma-mapping
    - dma_unmap_*
      Only valid unmap is dma_unmap_resource() all others are an invalid
      use case.
    - dma_sync_single_*
      Invalid use case, memory that is mapped is device memory
    - dma_common_mmap() and dma_mmap_attrs()
      Invalid use case
    - dma_common_get_sgtable() and dma_get_sgtable_attrs()
      Invalid use case, only for dma_alloc_* allocated memory,
    - dma_mapping_error()
      OK

* Changes since v6
- Use offset_in_page() and __pfn_to_phys(). This fixed a bug in the 
  lib/dma-debug.c. Thanks to Konrad Rzeszutek Wilk for finding it and Robin 
  Murphy for suggesting offset_in_page().
- Rebased on top of v4.7-rc1.
- Dropped DT patches which enabled the IPMMU on Renesas Koelsch and Lager. Will 
  post them separately at a later time.

* Changes since v5
- Add dma-debug work which adds a new mapping type for the resource
  mapping which correctly can be translated to a physical address.
- Drop patches from Robin Murphy since they now are accepted in the
  iommu repository and base the series on that tree instead.
- Add a review tag from Laurent.

* Changes since v4
- Move the mapping from phys_addr_t to dma_addr_t from slave_config to the
  prepare calls. This way we know the direction of the mapping and don't have
  to use DMA_BIDIRECTIONAL. Thanks Vinod for suggesting this.
- To be clear that the data type for slave addresses are changed add a patch
  that only changes the data type to phys_addr_t.
- Fixed up commit messages.

* Changes since v3
- Folded in a fix from Robin to his patch.
- Added a check to make sure dma_map_resource can not be used to map RAM as
  pointed out by Robin. I use BUG_ON to enforce this. It might not be the best
  method but I saw no other good way since DMA_ERROR_CODE might not be defined
  on all platforms.
- Added comment about that DTS changes will disable 2 DMA channels due to a HW
  (?) but in the DMAC.
- Dropped the use of dma_attrs, no longer needed.
- Collected Acked-by and Reviewed-by from Laurent.
- Various indentation fix ups.

* Changes since v2
- Drop patch to add dma_{map,unmap}_page_attrs.
- Add dma_{map,unmap}_resource to handle the mapping without involving a
  'struct page'. Thanks Laurent and Robin for pointing this out.
- Use size instead of address to keep track of if a mapping exist or not
  since addr == 0 is valid. Thanks Laurent.
- Pick up patch from Robin with Laurents ack (hope it's OK for me to
  attach the ack?) to add IOMMU_MMIO.
- Fix bug in rcar_dmac_device_config where the error check where
  inverted.
- Use DMA_BIDIRECTIONAL in rcar_dmac_device_config since we at that
  point can't be sure what direction the mapping is going to be used.

* Changes since v1
- Add and use a dma_{map,unmap}_page_attrs to be able to map the page
  using attributes DMA_ATTR_NO_KERNEL_MAPPING and
  DMA_ATTR_SKIP_CPU_SYNC. Thanks Laurent.
- Drop check if dmac is part of a iommu group or not, let the DMA
  mapping api handle it.
- Move slave configuration data around in rcar-dmac to avoid code
  duplication.
- Fix build issue reported by 'kbuild test robot' regarding phys_to_page
  not availability on some configurations.
- Add DT information for r8a7791.

* Changes since RFC
- Switch to use the dma-mapping api instead of using the iommu_map()
  directly. Turns out the dma-mapper is much smarter then me...
- Dropped the patch to expose domain->ops->pgsize_bitmap from within the
  iommu api.
- Dropped the patch showing how I tested the RFC.


Niklas S?derlund (6):
  dma-mapping: add {map,unmap}_resource to dma_map_ops
  dma-debug: add support for resource mappings
  dma-mapping: add dma_{map,unmap}_resource
  arm: dma-mapping: add {map,unmap}_resource for iommu ops
  dmaengine: rcar-dmac: group slave configuration
  dmaengine: rcar-dmac: add iommu support for slave transfers

 Documentation/DMA-API.txt   |  22 +++++++--
 arch/arm/mm/dma-mapping.c   |  63 ++++++++++++++++++++++++
 drivers/dma/sh/rcar-dmac.c  | 116 +++++++++++++++++++++++++++++++++++---------
 include/linux/dma-debug.h   |  19 ++++++++
 include/linux/dma-mapping.h |  42 ++++++++++++++++
 lib/dma-debug.c             |  52 +++++++++++++++++++-
 6 files changed, 285 insertions(+), 29 deletions(-)

-- 
2.8.2

[PATCHv7 1/6] dma-mapping: add {map,unmap}_resource to dma_map_ops

[Niklas Söderlund]

Add methods to handle mapping of device resources from a physical
address. This is needed for example to be able to map MMIO FIFO
registers to a IOMMU.

Signed-off-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/linux/dma-mapping.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index c980a92..a11ff9d 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -49,6 +49,12 @@ struct dma_map_ops {
 			 struct scatterlist *sg, int nents,
 			 enum dma_data_direction dir,
 			 struct dma_attrs *attrs);
+	dma_addr_t (*map_resource)(struct device *dev, phys_addr_t phys_addr,
+			       size_t size, enum dma_data_direction dir,
+			       struct dma_attrs *attrs);
+	void (*unmap_resource)(struct device *dev, dma_addr_t dma_handle,
+			   size_t size, enum dma_data_direction dir,
+			   struct dma_attrs *attrs);
 	void (*sync_single_for_cpu)(struct device *dev,
 				    dma_addr_t dma_handle, size_t size,
 				    enum dma_data_direction dir);
-- 
2.8.2

[PATCHv7 2/6] dma-debug: add support for resource mappings

[Niklas Söderlund]

A MMIO mapped resource can not be represented by a struct page so a new
debug type is needed to handle this. This patch add such type and
functionality to add/remove entries and how to translate them to a
physical address.

Signed-off-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
---
 include/linux/dma-debug.h | 19 +++++++++++++++++
 lib/dma-debug.c           | 52 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index fe8cb61..c7d844f 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -56,6 +56,13 @@ extern void debug_dma_alloc_coherent(struct device *dev, size_t size,
 extern void debug_dma_free_coherent(struct device *dev, size_t size,
 				    void *virt, dma_addr_t addr);
 
+extern void debug_dma_map_resource(struct device *dev, phys_addr_t addr,
+				   size_t size, int direction,
+				   dma_addr_t dma_addr);
+
+extern void debug_dma_unmap_resource(struct device *dev, dma_addr_t dma_addr,
+				     size_t size, int direction);
+
 extern void debug_dma_sync_single_for_cpu(struct device *dev,
 					  dma_addr_t dma_handle, size_t size,
 					  int direction);
@@ -141,6 +148,18 @@ static inline void debug_dma_free_coherent(struct device *dev, size_t size,
 {
 }
 
+static inline void debug_dma_map_resource(struct device *dev, phys_addr_t addr,
+					  size_t size, int direction,
+					  dma_addr_t dma_addr)
+{
+}
+
+static inline void debug_dma_unmap_resource(struct device *dev,
+					    dma_addr_t dma_addr, size_t size,
+					    int direction)
+{
+}
+
 static inline void debug_dma_sync_single_for_cpu(struct device *dev,
 						 dma_addr_t dma_handle,
 						 size_t size, int direction)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 51a76af..ce75fba 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -43,6 +43,7 @@ enum {
 	dma_debug_page,
 	dma_debug_sg,
 	dma_debug_coherent,
+	dma_debug_resource,
 };
 
 enum map_err_types {
@@ -150,8 +151,9 @@ static const char *const maperr2str[] = {
 	[MAP_ERR_CHECKED] = "dma map error checked",
 };
 
-static const char *type2name[4] = { "single", "page",
-				    "scather-gather", "coherent" };
+static const char *type2name[5] = { "single", "page",
+				    "scather-gather", "coherent",
+				    "resource" };
 
 static const char *dir2name[4] = { "DMA_BIDIRECTIONAL", "DMA_TO_DEVICE",
 				   "DMA_FROM_DEVICE", "DMA_NONE" };
@@ -397,6 +399,9 @@ static void hash_bucket_del(struct dma_debug_entry *entry)
 
 static unsigned long long phys_addr(struct dma_debug_entry *entry)
 {
+	if (entry->type == dma_debug_resource)
+		return __pfn_to_phys(entry->pfn) + entry->offset;
+
 	return page_to_phys(pfn_to_page(entry->pfn)) + entry->offset;
 }
 
@@ -1493,6 +1498,49 @@ void debug_dma_free_coherent(struct device *dev, size_t size,
 }
 EXPORT_SYMBOL(debug_dma_free_coherent);
 
+void debug_dma_map_resource(struct device *dev, phys_addr_t addr, size_t size,
+			    int direction, dma_addr_t dma_addr)
+{
+	struct dma_debug_entry *entry;
+
+	if (unlikely(dma_debug_disabled()))
+		return;
+
+	entry = dma_entry_alloc();
+	if (!entry)
+		return;
+
+	entry->type		= dma_debug_resource;
+	entry->dev		= dev;
+	entry->pfn		= __phys_to_pfn(addr);
+	entry->offset		= offset_in_page(addr);
+	entry->size		= size;
+	entry->dev_addr		= dma_addr;
+	entry->direction	= direction;
+	entry->map_err_type	= MAP_ERR_NOT_CHECKED;
+
+	add_dma_entry(entry);
+}
+EXPORT_SYMBOL(debug_dma_map_resource);
+
+void debug_dma_unmap_resource(struct device *dev, dma_addr_t dma_addr,
+			      size_t size, int direction)
+{
+	struct dma_debug_entry ref = {
+		.type           = dma_debug_resource,
+		.dev            = dev,
+		.dev_addr       = dma_addr,
+		.size           = size,
+		.direction      = direction,
+	};
+
+	if (unlikely(dma_debug_disabled()))
+		return;
+
+	check_unmap(&ref);
+}
+EXPORT_SYMBOL(debug_dma_unmap_resource);
+
 void debug_dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle,
 				   size_t size, int direction)
 {
-- 
2.8.2

[PATCHv7 3/6] dma-mapping: add dma_{map,unmap}_resource

[Niklas Söderlund]

Map/Unmap a device MMIO resource from a physical address. If no dma_map_ops
method is available the operation is a no-op.

Signed-off-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
---
 Documentation/DMA-API.txt   | 22 +++++++++++++++++-----
 include/linux/dma-mapping.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 45ef3f2..c7e5f99 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -277,14 +277,26 @@ and <size> parameters are provided to do partial page mapping, it is
 recommended that you never use these unless you really know what the
 cache width is.
 
+dma_addr_t
+dma_map_resource(struct device *dev, phys_addr_t phys_addr, size_t size,
+		 enum dma_data_direction dir, struct dma_attrs *attrs)
+
+void
+dma_unmap_resource(struct device *dev, dma_addr_t addr, size_t size,
+		   enum dma_data_direction dir, struct dma_attrs *attrs)
+
+API for mapping and unmapping for MMIO resources. All the notes and
+warnings for the other mapping APIs apply here. The API should only be
+used to map device MMIO resources, mapping of RAM is not permitted.
+
 int
 dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
 
-In some circumstances dma_map_single() and dma_map_page() will fail to create
-a mapping. A driver can check for these errors by testing the returned
-DMA address with dma_mapping_error(). A non-zero return value means the mapping
-could not be created and the driver should take appropriate action (e.g.
-reduce current DMA mapping usage or delay and try again later).
+In some circumstances dma_map_single(), dma_map_page() and dma_map_resource()
+will fail to create a mapping. A driver can check for these errors by testing
+the returned DMA address with dma_mapping_error(). A non-zero return value
+means the mapping could not be created and the driver should take appropriate
+action (e.g. reduce current DMA mapping usage or delay and try again later).
 
 	int
 	dma_map_sg(struct device *dev, struct scatterlist *sg,
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index a11ff9d..4c381ba 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -218,6 +218,42 @@ static inline void dma_unmap_page(struct device *dev, dma_addr_t addr,
 	debug_dma_unmap_page(dev, addr, size, dir, false);
 }
 
+static inline dma_addr_t dma_map_resource(struct device *dev,
+					  phys_addr_t phys_addr,
+					  size_t size,
+					  enum dma_data_direction dir,
+					  struct dma_attrs *attrs)
+{
+	struct dma_map_ops *ops = get_dma_ops(dev);
+	unsigned long pfn = __phys_to_pfn(phys_addr);
+	dma_addr_t addr;
+
+	BUG_ON(!valid_dma_direction(dir));
+
+	/* Don't allow RAM to be mapped */
+	BUG_ON(pfn_valid(pfn));
+
+	addr = phys_addr;
+	if (ops->map_resource)
+		addr = ops->map_resource(dev, phys_addr, size, dir, attrs);
+
+	debug_dma_map_resource(dev, phys_addr, size, dir, addr);
+
+	return addr;
+}
+
+static inline void dma_unmap_resource(struct device *dev, dma_addr_t addr,
+				      size_t size, enum dma_data_direction dir,
+				      struct dma_attrs *attrs)
+{
+	struct dma_map_ops *ops = get_dma_ops(dev);
+
+	BUG_ON(!valid_dma_direction(dir));
+	if (ops->unmap_resource)
+		ops->unmap_resource(dev, addr, size, dir, attrs);
+	debug_dma_unmap_resource(dev, addr, size, dir);
+}
+
 static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
 					   size_t size,
 					   enum dma_data_direction dir)
-- 
2.8.2

[PATCHv7 4/6] arm: dma-mapping: add {map, unmap}_resource for iommu ops

[Niklas Söderlund]

Add methods to map/unmap device resources addresses for dma_map_ops that
are IOMMU aware. This is needed to map a device MMIO register from a
physical address.

Signed-off-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm/mm/dma-mapping.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ff7ed56..8f12ec8 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1951,6 +1951,63 @@ static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle,
 	__free_iova(mapping, iova, len);
 }
 
+/**
+ * arm_iommu_map_resource - map a device resource for DMA
+ * @dev: valid struct device pointer
+ * @phys_addr: physical address of resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static dma_addr_t arm_iommu_map_resource(struct device *dev,
+		phys_addr_t phys_addr, size_t size,
+		enum dma_data_direction dir, struct dma_attrs *attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t dma_addr;
+	int ret, prot;
+	phys_addr_t addr = phys_addr & PAGE_MASK;
+	int offset = phys_addr & ~PAGE_MASK;
+	int len = PAGE_ALIGN(size + offset);
+
+	dma_addr = __alloc_iova(mapping, size);
+	if (dma_addr == DMA_ERROR_CODE)
+		return dma_addr;
+
+	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
+
+	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
+	if (ret < 0)
+		goto fail;
+
+	return dma_addr + offset;
+fail:
+	__free_iova(mapping, dma_addr, size);
+	return DMA_ERROR_CODE;
+}
+
+/**
+ * arm_iommu_unmap_resource - unmap a device DMA resource
+ * @dev: valid struct device pointer
+ * @dma_handle: DMA address to resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t dma_handle,
+		size_t size, enum dma_data_direction dir,
+		struct dma_attrs *attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t iova = dma_handle & PAGE_MASK;
+	int offset = dma_handle & ~PAGE_MASK;
+	int len = PAGE_ALIGN(size + offset);
+
+	if (!iova)
+		return;
+
+	iommu_unmap(mapping->domain, iova, len);
+	__free_iova(mapping, iova, len);
+}
+
 static void arm_iommu_sync_single_for_cpu(struct device *dev,
 		dma_addr_t handle, size_t size, enum dma_data_direction dir)
 {
@@ -1994,6 +2051,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,
+
+	.map_resource		= arm_iommu_map_resource,
+	.unmap_resource		= arm_iommu_unmap_resource,
 };
 
 struct dma_map_ops iommu_coherent_ops = {
@@ -2007,6 +2067,9 @@ struct dma_map_ops iommu_coherent_ops = {
 
 	.map_sg		= arm_coherent_iommu_map_sg,
 	.unmap_sg	= arm_coherent_iommu_unmap_sg,
+
+	.map_resource	= arm_iommu_map_resource,
+	.unmap_resource	= arm_iommu_unmap_resource,
 };
 
 /**
-- 
2.8.2

[PATCHv7 4/6] arm: dma-mapping: add {map, unmap}_resource for iommu ops

[Russell King - ARM Linux]

On Wed, Jun 01, 2016 at 05:22:27PM +0200, Niklas S?derlund wrote:
> +static dma_addr_t arm_iommu_map_resource(struct device *dev,
> +		phys_addr_t phys_addr, size_t size,
> +		enum dma_data_direction dir, struct dma_attrs *attrs)
> +{
> +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> +	dma_addr_t dma_addr;
> +	int ret, prot;
> +	phys_addr_t addr = phys_addr & PAGE_MASK;
> +	int offset = phys_addr & ~PAGE_MASK;
> +	int len = PAGE_ALIGN(size + offset);

Shouldn't both of these be unsigned - preferably size_t for len?

> +
> +	dma_addr = __alloc_iova(mapping, size);

Is this really correct?  What if size = 4095 and offset = 10?  Do we
really only need one IOVA page for such a mapping (I count two pages.)
Shouldn't this be "len" ?

> +	if (dma_addr == DMA_ERROR_CODE)
> +		return dma_addr;
> +
> +	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
> +
> +	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
> +	if (ret < 0)
> +		goto fail;
> +
> +	return dma_addr + offset;
> +fail:
> +	__free_iova(mapping, dma_addr, size);

Shouldn't this be "len" as well?

> +	return DMA_ERROR_CODE;
> +}
> +
> +/**
> + * arm_iommu_unmap_resource - unmap a device DMA resource
> + * @dev: valid struct device pointer
> + * @dma_handle: DMA address to resource
> + * @size: size of resource to map
> + * @dir: DMA transfer direction
> + */
> +static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t dma_handle,
> +		size_t size, enum dma_data_direction dir,
> +		struct dma_attrs *attrs)
> +{
> +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> +	dma_addr_t iova = dma_handle & PAGE_MASK;
> +	int offset = dma_handle & ~PAGE_MASK;
> +	int len = PAGE_ALIGN(size + offset);

unsigned/size_t again.

> +
> +	if (!iova)
> +		return;
> +
> +	iommu_unmap(mapping->domain, iova, len);
> +	__free_iova(mapping, iova, len);

Here, you free "len" bytes of iova, which is different from above.

> +}
> +
>  static void arm_iommu_sync_single_for_cpu(struct device *dev,
>  		dma_addr_t handle, size_t size, enum dma_data_direction dir)
>  {
> @@ -1994,6 +2051,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,
> +
> +	.map_resource		= arm_iommu_map_resource,
> +	.unmap_resource		= arm_iommu_unmap_resource,
>  };
>  
>  struct dma_map_ops iommu_coherent_ops = {
> @@ -2007,6 +2067,9 @@ struct dma_map_ops iommu_coherent_ops = {
>  
>  	.map_sg		= arm_coherent_iommu_map_sg,
>  	.unmap_sg	= arm_coherent_iommu_unmap_sg,
> +
> +	.map_resource	= arm_iommu_map_resource,
> +	.unmap_resource	= arm_iommu_unmap_resource,
>  };
>  
>  /**
> -- 
> 2.8.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCHv7 4/6] arm: dma-mapping: add {map, unmap}_resource for iommu ops

[Niklas Söderlund]

Hi Russell,

Thanks for your feedback.

On 2016-06-01 17:16:06 +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 01, 2016 at 05:22:27PM +0200, Niklas S?derlund wrote:
> > +static dma_addr_t arm_iommu_map_resource(struct device *dev,
> > +		phys_addr_t phys_addr, size_t size,
> > +		enum dma_data_direction dir, struct dma_attrs *attrs)
> > +{
> > +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> > +	dma_addr_t dma_addr;
> > +	int ret, prot;
> > +	phys_addr_t addr = phys_addr & PAGE_MASK;
> > +	int offset = phys_addr & ~PAGE_MASK;
> > +	int len = PAGE_ALIGN(size + offset);
> 
> Shouldn't both of these be unsigned - preferably size_t for len?

I have looked at arm_coherent_iommu_map_page() when writing this where 
len is int. But I do agree that it should probably be size_t and offset 
should be unsigned. Will fix this.

> 
> > +
> > +	dma_addr = __alloc_iova(mapping, size);
> 
> Is this really correct?  What if size = 4095 and offset = 10?  Do we
> really only need one IOVA page for such a mapping (I count two pages.)
> Shouldn't this be "len" ?

Wops, you are correct it should be len not size.

> 
> > +	if (dma_addr == DMA_ERROR_CODE)
> > +		return dma_addr;
> > +
> > +	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
> > +
> > +	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
> > +	if (ret < 0)
> > +		goto fail;
> > +
> > +	return dma_addr + offset;
> > +fail:
> > +	__free_iova(mapping, dma_addr, size);
> 
> Shouldn't this be "len" as well?

Yes.

> 
> > +	return DMA_ERROR_CODE;
> > +}
> > +
> > +/**
> > + * arm_iommu_unmap_resource - unmap a device DMA resource
> > + * @dev: valid struct device pointer
> > + * @dma_handle: DMA address to resource
> > + * @size: size of resource to map
> > + * @dir: DMA transfer direction
> > + */
> > +static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t dma_handle,
> > +		size_t size, enum dma_data_direction dir,
> > +		struct dma_attrs *attrs)
> > +{
> > +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> > +	dma_addr_t iova = dma_handle & PAGE_MASK;
> > +	int offset = dma_handle & ~PAGE_MASK;
> > +	int len = PAGE_ALIGN(size + offset);
> 
> unsigned/size_t again.

Will fix.

> 
> > +
> > +	if (!iova)
> > +		return;
> > +
> > +	iommu_unmap(mapping->domain, iova, len);
> > +	__free_iova(mapping, iova, len);
> 
> Here, you free "len" bytes of iova, which is different from above.

Yes you are correct. By using len instead of size in 
arm_iommu_map_resource() the sizes do match.

> 
> > +}
> > +
> >  static void arm_iommu_sync_single_for_cpu(struct device *dev,
> >  		dma_addr_t handle, size_t size, enum dma_data_direction dir)
> >  {
> > @@ -1994,6 +2051,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,
> > +
> > +	.map_resource		= arm_iommu_map_resource,
> > +	.unmap_resource		= arm_iommu_unmap_resource,
> >  };
> >  
> >  struct dma_map_ops iommu_coherent_ops = {
> > @@ -2007,6 +2067,9 @@ struct dma_map_ops iommu_coherent_ops = {
> >  
> >  	.map_sg		= arm_coherent_iommu_map_sg,
> >  	.unmap_sg	= arm_coherent_iommu_unmap_sg,
> > +
> > +	.map_resource	= arm_iommu_map_resource,
> > +	.unmap_resource	= arm_iommu_unmap_resource,
> >  };
> >  
> >  /**
> > -- 
> > 2.8.2
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

-- 
Regards,
Niklas S?derlund

[PATCHv7 5/6] dmaengine: rcar-dmac: group slave configuration

[Niklas Söderlund]

Group slave address and transfer size in own structs for source and
destination. This is in preparation for hooking up the dma-mapping API
to the slave addresses.

Signed-off-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/dma/sh/rcar-dmac.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index dfb1792..b0c3bb2 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -118,14 +118,22 @@ struct rcar_dmac_desc_page {
 	sizeof(struct rcar_dmac_xfer_chunk))
 
 /*
+ * struct rcar_dmac_chan_slave - Slave configuration
+ * @slave_addr: slave memory address
+ * @xfer_size: size (in bytes) of hardware transfers
+ */
+struct rcar_dmac_chan_slave {
+	phys_addr_t slave_addr;
+	unsigned int xfer_size;
+};
+
+/*
  * struct rcar_dmac_chan - R-Car Gen2 DMA Controller Channel
  * @chan: base DMA channel object
  * @iomem: channel I/O memory base
  * @index: index of this channel in the controller
- * @src_xfer_size: size (in bytes) of hardware transfers on the source side
- * @dst_xfer_size: size (in bytes) of hardware transfers on the destination side
- * @src_slave_addr: slave source memory address
- * @dst_slave_addr: slave destination memory address
+ * @src: slave memory address and size on the source side
+ * @dst: slave memory address and size on the destination side
  * @mid_rid: hardware MID/RID for the DMA client using this channel
  * @lock: protects the channel CHCR register and the desc members
  * @desc.free: list of free descriptors
@@ -142,10 +150,8 @@ struct rcar_dmac_chan {
 	void __iomem *iomem;
 	unsigned int index;
 
-	unsigned int src_xfer_size;
-	unsigned int dst_xfer_size;
-	dma_addr_t src_slave_addr;
-	dma_addr_t dst_slave_addr;
+	struct rcar_dmac_chan_slave src;
+	struct rcar_dmac_chan_slave dst;
 	int mid_rid;
 
 	spinlock_t lock;
@@ -793,13 +799,13 @@ static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
 	case DMA_DEV_TO_MEM:
 		chcr = RCAR_DMACHCR_DM_INC | RCAR_DMACHCR_SM_FIXED
 		     | RCAR_DMACHCR_RS_DMARS;
-		xfer_size = chan->src_xfer_size;
+		xfer_size = chan->src.xfer_size;
 		break;
 
 	case DMA_MEM_TO_DEV:
 		chcr = RCAR_DMACHCR_DM_FIXED | RCAR_DMACHCR_SM_INC
 		     | RCAR_DMACHCR_RS_DMARS;
-		xfer_size = chan->dst_xfer_size;
+		xfer_size = chan->dst.xfer_size;
 		break;
 
 	case DMA_MEM_TO_MEM:
@@ -1038,7 +1044,7 @@ rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	}
 
 	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src_slave_addr : rchan->dst_slave_addr;
+		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
 	return rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
 				      dir, flags, false);
 }
@@ -1093,7 +1099,7 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
 	}
 
 	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src_slave_addr : rchan->dst_slave_addr;
+		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
 	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
 				      dir, flags, true);
 
@@ -1110,10 +1116,10 @@ static int rcar_dmac_device_config(struct dma_chan *chan,
 	 * We could lock this, but you shouldn't be configuring the
 	 * channel, while using it...
 	 */
-	rchan->src_slave_addr = cfg->src_addr;
-	rchan->dst_slave_addr = cfg->dst_addr;
-	rchan->src_xfer_size = cfg->src_addr_width;
-	rchan->dst_xfer_size = cfg->dst_addr_width;
+	rchan->src.slave_addr = cfg->src_addr;
+	rchan->dst.slave_addr = cfg->dst_addr;
+	rchan->src.xfer_size = cfg->src_addr_width;
+	rchan->dst.xfer_size = cfg->dst_addr_width;
 
 	return 0;
 }
-- 
2.8.2

[PATCHv7 6/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Niklas Söderlund]

Enable slave transfers to a device behind a IPMMU by mapping the slave
addresses using the dma-mapping API.

Signed-off-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/dma/sh/rcar-dmac.c | 82 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 74 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index b0c3bb2..8592598 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -128,6 +128,18 @@ struct rcar_dmac_chan_slave {
 };
 
 /*
+ * struct rcar_dmac_chan_map - Map of slave device phys to dma address
+ * @addr: slave dma address
+ * @dir: direction of mapping
+ * @slave: slave configuration that is mapped
+ */
+struct rcar_dmac_chan_map {
+	dma_addr_t addr;
+	enum dma_data_direction dir;
+	struct rcar_dmac_chan_slave slave;
+};
+
+/*
  * struct rcar_dmac_chan - R-Car Gen2 DMA Controller Channel
  * @chan: base DMA channel object
  * @iomem: channel I/O memory base
@@ -152,6 +164,7 @@ struct rcar_dmac_chan {
 
 	struct rcar_dmac_chan_slave src;
 	struct rcar_dmac_chan_slave dst;
+	struct rcar_dmac_chan_map map;
 	int mid_rid;
 
 	spinlock_t lock;
@@ -1027,13 +1040,65 @@ rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest,
 				      DMA_MEM_TO_MEM, flags, false);
 }
 
+static int rcar_dmac_map_slave_addr(struct dma_chan *chan,
+				    enum dma_transfer_direction dir)
+{
+	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
+	struct rcar_dmac_chan_map *map = &rchan->map;
+	phys_addr_t dev_addr;
+	size_t dev_size;
+	enum dma_data_direction dev_dir;
+
+	if (dir == DMA_DEV_TO_MEM) {
+		dev_addr = rchan->src.slave_addr;
+		dev_size = rchan->src.xfer_size;
+		dev_dir = DMA_TO_DEVICE;
+	} else {
+		dev_addr = rchan->dst.slave_addr;
+		dev_size = rchan->dst.xfer_size;
+		dev_dir = DMA_FROM_DEVICE;
+	}
+
+	/* Reuse current map if possible. */
+	if (dev_addr == map->slave.slave_addr &&
+	    dev_size == map->slave.xfer_size &&
+	    dev_dir == map->dir)
+		return 0;
+
+	/* Remove old mapping if present. */
+	if (map->slave.xfer_size)
+		dma_unmap_resource(chan->device->dev, map->addr,
+				   map->slave.xfer_size, map->dir, NULL);
+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, NULL);
+
+	if (dma_mapping_error(chan->device->dev, map->addr)) {
+		dev_err(chan->device->dev,
+			"chan%u: failed to map %zx@%pap", rchan->index,
+			dev_size, &dev_addr);
+		return -EIO;
+	}
+
+	dev_dbg(chan->device->dev, "chan%u: map %zx@%pap to %pad dir: %s\n",
+		rchan->index, dev_size, &dev_addr, &map->addr,
+		dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" : "DMA_FROM_DEVICE");
+
+	map->slave.slave_addr = dev_addr;
+	map->slave.xfer_size = dev_size;
+	map->dir = dev_dir;
+
+	return 0;
+}
+
 static struct dma_async_tx_descriptor *
 rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 			unsigned int sg_len, enum dma_transfer_direction dir,
 			unsigned long flags, void *context)
 {
 	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
-	dma_addr_t dev_addr;
 
 	/* Someone calling slave DMA on a generic channel? */
 	if (rchan->mid_rid < 0 || !sg_len) {
@@ -1043,9 +1108,10 @@ rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		return NULL;
 	}
 
-	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
-	return rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
+	if (rcar_dmac_map_slave_addr(chan, dir))
+		return NULL;
+
+	return rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, rchan->map.addr,
 				      dir, flags, false);
 }
 
@@ -1059,7 +1125,6 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
 	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
 	struct dma_async_tx_descriptor *desc;
 	struct scatterlist *sgl;
-	dma_addr_t dev_addr;
 	unsigned int sg_len;
 	unsigned int i;
 
@@ -1071,6 +1136,9 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
 		return NULL;
 	}
 
+	if (rcar_dmac_map_slave_addr(chan, dir))
+		return NULL;
+
 	sg_len = buf_len / period_len;
 	if (sg_len > RCAR_DMAC_MAX_SG_LEN) {
 		dev_err(chan->device->dev,
@@ -1098,9 +1166,7 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
 		sg_dma_len(&sgl[i]) = period_len;
 	}
 
-	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
-	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
+	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, rchan->map.addr,
 				      dir, flags, true);
 
 	kfree(sgl);
-- 
2.8.2

[PATCHv7 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Vinod Koul]

On Wed, Jun 01, 2016 at 05:22:23PM +0200, Niklas S?derlund wrote:
> Hi,
> 
> [In this v7 series I have tried to address the questions raised by Christoph 
> Hellwig and I hope it can awnser your concernes regarding dma-debug.]
> 
> This series tries to solve the problem with DMA with device registers
> (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> recent patch '9575632 (dmaengine: make slave address physical)'
> clarifies that DMA slave address provided by clients is the physical
> address. This puts the task of mapping the DMA slave address from a
> phys_addr_t to a dma_addr_t on the DMA engine.
> 
> Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> the same and no special care is needed. However if you have a IOMMU you
> need to map the DMA slave phys_addr_t to a dma_addr_t using something
> like this.
> 
> This series is based on top of v4.7-rc1.

The dmanegine bits looks okay to me. Btw how is the merge planned for this?
Do you wnat this to be merged thru dmaengine tree or something else?

-- 
~Vinod

[PATCHv7 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Niklas Söderlund]

Hi Vinod,

On 2016-06-01 23:36:11 +0530, Vinod Koul wrote:
> On Wed, Jun 01, 2016 at 05:22:23PM +0200, Niklas S?derlund wrote:
> > Hi,
> > 
> > [In this v7 series I have tried to address the questions raised by Christoph 
> > Hellwig and I hope it can awnser your concernes regarding dma-debug.]
> > 
> > This series tries to solve the problem with DMA with device registers
> > (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> > recent patch '9575632 (dmaengine: make slave address physical)'
> > clarifies that DMA slave address provided by clients is the physical
> > address. This puts the task of mapping the DMA slave address from a
> > phys_addr_t to a dma_addr_t on the DMA engine.
> > 
> > Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> > the same and no special care is needed. However if you have a IOMMU you
> > need to map the DMA slave phys_addr_t to a dma_addr_t using something
> > like this.
> > 
> > This series is based on top of v4.7-rc1.
> 
> The dmanegine bits looks okay to me. Btw how is the merge planned for this?
> Do you wnat this to be merged thru dmaengine tree or something else?

Yes, since the arm specific patch are depending on other parts of the 
series I was hoping to be able to get Russells Ack on it and then try to 
get it all in through the dmaengine tree.

If you see a better way I'm happy to do it that way, let me know what 
you think. I hold off v8 that adresses the issues Russell brought up a 
few days untill I know what you think is best.

-- 
Regards,
Niklas S?derlund

[PATCHv7 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

[Vinod Koul]

On Thu, Jun 02, 2016 at 02:58:07PM +0200, Niklas S?derlund wrote:
> Hi Vinod,
> 
> On 2016-06-01 23:36:11 +0530, Vinod Koul wrote:
> > On Wed, Jun 01, 2016 at 05:22:23PM +0200, Niklas S?derlund wrote:
> > > Hi,
> > > 
> > > [In this v7 series I have tried to address the questions raised by Christoph 
> > > Hellwig and I hope it can awnser your concernes regarding dma-debug.]
> > > 
> > > This series tries to solve the problem with DMA with device registers
> > > (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A
> > > recent patch '9575632 (dmaengine: make slave address physical)'
> > > clarifies that DMA slave address provided by clients is the physical
> > > address. This puts the task of mapping the DMA slave address from a
> > > phys_addr_t to a dma_addr_t on the DMA engine.
> > > 
> > > Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are
> > > the same and no special care is needed. However if you have a IOMMU you
> > > need to map the DMA slave phys_addr_t to a dma_addr_t using something
> > > like this.
> > > 
> > > This series is based on top of v4.7-rc1.
> > 
> > The dmanegine bits looks okay to me. Btw how is the merge planned for this?
> > Do you wnat this to be merged thru dmaengine tree or something else?
> 
> Yes, since the arm specific patch are depending on other parts of the 
> series I was hoping to be able to get Russells Ack on it and then try to 
> get it all in through the dmaengine tree.

Sounds good to me..

> If you see a better way I'm happy to do it that way, let me know what 
> you think. I hold off v8 that adresses the issues Russell brought up a 
> few days untill I know what you think is best.

-- 
~Vinod