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

Hi,

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.8-rc1. And I'm hoping to be able to collect a
Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
able to take the whole series through the dmaengine tree. If this is not the
best route I'm more then happy to do it another way.

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 v8
- Rebased on v4.8-rc1

* Changes since v7
- Use size_t instead of int for length in arm_iommu_map_resource() and
  arm_iommu_unmap_resource().
- Fix bug in arm_iommu_map_resource() where wrong variable where passed to
  __alloc_iova(). Thanks to Russell King for pointing out both errors.

* 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
  (?) bug 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.9.2

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 c44f8ee..e3bd27f 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h

@@ -95,6 +95,12 @@ struct dma_map_ops {
 			 struct scatterlist *sg, int nents,
 			 enum dma_data_direction dir,
 			 unsigned long attrs);
+	dma_addr_t (*map_resource)(struct device *dev, phys_addr_t phys_addr,
+			       size_t size, enum dma_data_direction dir,
+			       unsigned long attrs);
+	void (*unmap_resource)(struct device *dev, dma_addr_t dma_handle,
+			   size_t size, enum dma_data_direction dir,
+			   unsigned long attrs);
 	void (*sync_single_for_cpu)(struct device *dev,
 				    dma_addr_t dma_handle, size_t size,
 				    enum dma_data_direction dir);

-- 
2.9.2

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 fcfa193..2ba086b 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" };

@@ -399,6 +401,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;
 }
 

@@ -1495,6 +1500,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.9.2

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 1d26eeb6..6b20128 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, unsigned long attrs)
+
+void
+dma_unmap_resource(struct device *dev, dma_addr_t addr, size_t size,
+		   enum dma_data_direction dir, unsigned long 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 e3bd27f..9a07882 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h

@@ -264,6 +264,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,
+					  unsigned long 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,
+				      unsigned long 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.9.2

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 c6834c0..746eb29 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c

@@ -2014,6 +2014,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, unsigned long 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;
+	unsigned int offset = phys_addr & ~PAGE_MASK;
+	size_t len = PAGE_ALIGN(size + offset);
+
+	dma_addr = __alloc_iova(mapping, 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, len);
+	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,
+		unsigned long attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t iova = dma_handle & PAGE_MASK;
+	unsigned int offset = dma_handle & ~PAGE_MASK;
+	size_t 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)
 {

@@ -2057,6 +2114,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 = {

@@ -2070,6 +2130,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.9.2

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 0dd9538..cf983a9 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:

@@ -1040,7 +1046,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);
 }

@@ -1095,7 +1101,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);
 

@@ -1112,10 +1118,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.9.2

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 cf983a9..22a5e40 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;

@@ -1029,13 +1042,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, 0);
+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+
+	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) {

@@ -1045,9 +1110,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);
 }
 

@@ -1061,7 +1127,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;
 

@@ -1073,6 +1138,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,

@@ -1100,9 +1168,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.9.2

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

Hi,

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.8-rc1. And I'm hoping to be able to collect a
Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
able to take the whole series through the dmaengine tree. If this is not the
best route I'm more then happy to do it another way.

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.

As I said in last one, the dmaengine parts look fine to me. But to go thru
dmaengine tree I would need ACK on non dmaengine patches.


-- 
~Vinod

Hi Russell,

If you have the time can you please have a look at this patch? This 
series have been out for some time now and Vinod is willing to take it 
through the dmaengine tree but a ACK is needed on this patch from you 
first.

On 2016-08-10 13:22:17 +0200, Niklas S?derlund wrote:

-- 
Regards,
Niklas S?derlund

Hi Niklas,

Thank you for the patch.

On Wednesday 10 Aug 2016 13:22:15 Niklas S?derlund wrote:

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>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Hi Niklas,

Thank you for the patch.

On Wednesday 10 Aug 2016 13:22:16 Niklas S?derlund wrote:

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>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

dir,

+				      unsigned long 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)

-- 
Regards,

Laurent Pinchart

Hi Niklas,

Thank you for the patch.

On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:

Shouldn't this be DMA_FROM_DEVICE, and DMA_TO_DEVICE below ?

+	} 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, 0);

Unless I'm mistaken the resource will not be unmapped when freeing channel 
resources, will it ?

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+
+	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");

-- 
Regards,

Laurent Pinchart

Hello Niklas and Russell,

On Tuesday 23 Aug 2016 17:31:36 Niklas S?derlund wrote:

Hi Russell,

If you have the time can you please have a look at this patch? This
series have been out for some time now and Vinod is willing to take it
through the dmaengine tree but a ACK is needed on this patch from you
first.

I've reviewed and acked all the patches touching the DMA mapping API (1/6 to 
4/6). Russell, if you can find a bit of time to review this one it would be 
very appreciated.

On 2016-08-10 13:22:17 +0200, Niklas S?derlund wrote:

quoted

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 c6834c0..746eb29 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c

@@ -2014,6 +2014,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, unsigned long 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;
+	unsigned int offset = phys_addr & ~PAGE_MASK;
+	size_t len = PAGE_ALIGN(size + offset);
+
+	dma_addr = __alloc_iova(mapping, 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, len);
+	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,
+		unsigned long attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t iova = dma_handle & PAGE_MASK;
+	unsigned int offset = dma_handle & ~PAGE_MASK;
+	size_t 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)
 
 {

@@ -2057,6 +2114,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 = {

@@ -2070,6 +2130,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,

 };
 
 /**

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On 05/09/16 10:52, Laurent Pinchart wrote:

Hi Niklas,

Thank you for the patch.

On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:

quoted

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 cf983a9..22a5e40 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;

@@ -1029,13 +1042,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;

Shouldn't this be DMA_FROM_DEVICE, and DMA_TO_DEVICE below ?

No, this is in fact correct (double-checking against the equivalent I
wrote for the ARM PL330 based on this - I should resurrect those patches...)

The "DEV" in the dmaengine direction is the peripheral FIFO, whereas the
"DEVICE" in the DMA API direction is the DMA engine itself, thus for the
DEV_TO_MEM transaction, the mapping of the "DEV" end needs to allow data
transfer "from" the address being mapped "to" the DMA engine.

Yes, it's hideously confusing.

Robin.

quoted

+	} 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, 0);

Unless I'm mistaken the resource will not be unmapped when freeing channel 
resources, will it ?

quoted

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+
+	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");

quoted

+
+	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) {

@@ -1045,9 +1110,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);
 }

@@ -1061,7 +1127,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;

@@ -1073,6 +1138,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,

@@ -1100,9 +1168,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);

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

quoted

Hi,

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.8-rc1. And I'm hoping to be able to collect a
Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
able to take the whole series through the dmaengine tree. If this is not the
best route I'm more then happy to do it another way.

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.

As I said in last one, the dmaengine parts look fine to me. But to go thru
dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now.
If anyone has any objects, please speak up now...

Also ACKs welcome...

-- 
~Vinod

On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

quoted

Hi,

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.8-rc1. And I'm hoping to be able to collect a
Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
able to take the whole series through the dmaengine tree. If this is not the
best route I'm more then happy to do it another way.

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.

As I said in last one, the dmaengine parts look fine to me. But to go thru
dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now.
If anyone has any objects, please speak up now...

Also ACKs welcome...

I had not looked at the series earlier, but this version looks entirely
reasonable to me, so

Acked-by: Arnd Bergmann <arnd@arndb.de>


One concern I have is that we might get an awkward situation if we
ever encounter one DMA engine hardware that is used in different
systems that all have an IOMMU, but on some of them the connection
between the DMA master and the slave FIFO bypasses the IOMMU
while on others the IOMMU is required. I don't have any idea for
how this could be handled in a generic way, so my best answer
here is to hope we never get there, and if we do, handle it
using some local hack in the driver.

	Arnd

Hi Arnd,

On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

quoted

Hi,

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.8-rc1. And I'm hoping to be able to
collect a Ack from Russell King on patch 4/6 that adds the ARM
specific part and then be able to take the whole series through the
dmaengine tree. If this is not the best route I'm more then happy to
do it another way.

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.

As I said in last one, the dmaengine parts look fine to me. But to go
thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now.
If anyone has any objects, please speak up now...

Also ACKs welcome...

I had not looked at the series earlier, but this version looks entirely
reasonable to me, so

Acked-by: Arnd Bergmann <arnd@arndb.de>


One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems that all
have an IOMMU, but on some of them the connection between the DMA master and
the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go 
through the IOMMU while others do not ? We indeed have no solution today for 
such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA 
engines that have different channels served by different IOMMUs. I recall 
discussing this in the past with you, and the solution you proposed was to add 
a channel index to struct dma_attrs seems good to me. To support the case 
where some channels don't go through an IOMMU we would only need support for 
null entries in the IOMMUs list associated with a device (for instance in the 
DT case null entries in the iommus property).

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski [off-list ref]
Date:   Wed Aug 3 13:46:00 2016 -0700

    dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but 
I'm not very happy with that patch as I can see how a future proposal to 
handle the channel number through the DMA attributes will get rejected on the 
grounds of bits starvation then :-(

I don't have any idea for how this could be handled in a generic way, so my
best answer here is to hope we never get there, and if we do, handle it
using some local hack in the driver.

-- 
Regards,

Laurent Pinchart

On 16/09/16 10:48, Laurent Pinchart wrote:

Hi Arnd,

On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

quoted

On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

quoted

Hi,

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.8-rc1. And I'm hoping to be able to
collect a Ack from Russell King on patch 4/6 that adds the ARM
specific part and then be able to take the whole series through the
dmaengine tree. If this is not the best route I'm more then happy to
do it another way.

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.

As I said in last one, the dmaengine parts look fine to me. But to go
thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now.
If anyone has any objects, please speak up now...

Also ACKs welcome...

I had not looked at the series earlier, but this version looks entirely
reasonable to me, so

Acked-by: Arnd Bergmann <arnd@arndb.de>


One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems that all
have an IOMMU, but on some of them the connection between the DMA master and
the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go 
through the IOMMU while others do not ? We indeed have no solution today for 
such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA 
engines that have different channels served by different IOMMUs. I recall 
discussing this in the past with you, and the solution you proposed was to add 
a channel index to struct dma_attrs seems good to me. To support the case 
where some channels don't go through an IOMMU we would only need support for 
null entries in the IOMMUs list associated with a device (for instance in the 
DT case null entries in the iommus property).

I think at that point we just create the channels as child devices of
the main dmaengine device so they each get their own DMA ops, and can do
whatever. The Qualcomm HIDMA driver already does that for a very similar
reason (so that the IOMMU can map individual channels into different
guest VMs).

Robin.

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski [off-list ref]
Date:   Wed Aug 3 13:46:00 2016 -0700

    dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but 
I'm not very happy with that patch as I can see how a future proposal to 
handle the channel number through the DMA attributes will get rejected on the 
grounds of bits starvation then :-(

quoted

I don't have any idea for how this could be handled in a generic way, so my
best answer here is to hope we never get there, and if we do, handle it
using some local hack in the driver.

On Friday, September 16, 2016 12:48:23 PM CEST Laurent Pinchart wrote:

On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

quoted

On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

I had not looked at the series earlier, but this version looks entirely
reasonable to me, so

Acked-by: Arnd Bergmann <arnd@arndb.de>


One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems that all
have an IOMMU, but on some of them the connection between the DMA master and
the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go 
through the IOMMU while others do not ? We indeed have no solution today for 
such a situation.

I wasn't thinking quite that far, though that is also a theoretical
problem. However, the simple solution would be to have a bit in the DMA
specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine
either goes through an IOMMU or doesn't (depending on the integration
into the SoC), so we'd have to find out through some DT property
or compatible string in the DMA enginen driver.

The problem is a bit broader than that, we'll also have an issue with DMA 
engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know exists?

I recall 
discussing this in the past with you, and the solution you proposed was to add 
a channel index to struct dma_attrs seems good to me. To support the case 
where some channels don't go through an IOMMU we would only need support for 
null entries in the IOMMUs list associated with a device (for instance in the 
DT case null entries in the iommus property).

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski [off-list ref]
Date:   Wed Aug 3 13:46:00 2016 -0700

    dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but 
I'm not very happy with that patch as I can see how a future proposal to 
handle the channel number through the DMA attributes will get rejected on the 
grounds of bits starvation then :-(

Agreed, that can become interesting.

	Arnd

Hi Rubin,

On Friday 16 Sep 2016 11:36:29 Robin Murphy wrote:

On 16/09/16 10:48, Laurent Pinchart wrote:

quoted

On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

quoted

On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

quoted

Hi,

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.8-rc1. And I'm hoping to be able to
collect a Ack from Russell King on patch 4/6 that adds the ARM
specific part and then be able to take the whole series through the
dmaengine tree. If this is not the best route I'm more then happy to
do it another way.

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.

As I said in last one, the dmaengine parts look fine to me. But to go
thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one
now. If anyone has any objects, please speak up now...

Also ACKs welcome...

I had not looked at the series earlier, but this version looks entirely
reasonable to me, so

Acked-by: Arnd Bergmann <arnd@arndb.de>


One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems that
all have an IOMMU, but on some of them the connection between the DMA
master and the slave FIFO bypasses the IOMMU while on others the IOMMU
is required.

Do you mean systems where some of the channels of a specific DMA engine go
through the IOMMU while others do not ? We indeed have no solution today
for such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA
engines that have different channels served by different IOMMUs. I recall
discussing this in the past with you, and the solution you proposed was to
add a channel index to struct dma_attrs seems good to me. To support the
case where some channels don't go through an IOMMU we would only need
support for null entries in the IOMMUs list associated with a device (for
instance in the DT case null entries in the iommus property).

I think at that point we just create the channels as child devices of
the main dmaengine device so they each get their own DMA ops, and can do
whatever. The Qualcomm HIDMA driver already does that for a very similar
reason (so that the IOMMU can map individual channels into different
guest VMs).

That's another option, but it seems more like a workaround to me, instead of a 
proper solution to fix the more global problem of multiple memory paths within 
a single device. I have other hardware devices that can act as bus masters 
through different paths (for instance a display-related device that fetches 
data and commands through different paths). Luckily so far all those paths are 
served by the same IOMMU, but there's no guarantee this will remain true in 
the future. Furthermore, even today, the IOMMU connected to that device has 
the ability to selectively enable and disable its ports. I have to keep them 
all enabled due to the lack of channel information in the DMA mapping and 
IOMMU APIs, leading to increased power consumption.

quoted

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski [off-list ref]
Date:   Wed Aug 3 13:46:00 2016 -0700

    dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number,
but I'm not very happy with that patch as I can see how a future proposal
to handle the channel number through the DMA attributes will get rejected
on the grounds of bits starvation then :-(

quoted

I don't have any idea for how this could be handled in a generic way, so
my best answer here is to hope we never get there, and if we do, handle
it using some local hack in the driver.

-- 
Regards,

Laurent Pinchart

Hi Arnd,

On Friday 16 Sep 2016 14:02:35 Arnd Bergmann wrote:

On Friday, September 16, 2016 12:48:23 PM CEST Laurent Pinchart wrote:

quoted

On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

quoted

On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

I had not looked at the series earlier, but this version looks entirely
reasonable to me, so

Acked-by: Arnd Bergmann <arnd@arndb.de>


One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems that
all have an IOMMU, but on some of them the connection between the DMA
master and the slave FIFO bypasses the IOMMU while on others the IOMMU
is required.

Do you mean systems where some of the channels of a specific DMA engine go
through the IOMMU while others do not ? We indeed have no solution today
for such a situation.

I wasn't thinking quite that far, though that is also a theoretical
problem. However, the simple solution would be to have a bit in the DMA
specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine
either goes through an IOMMU or doesn't (depending on the integration
into the SoC), so we'd have to find out through some DT property
or compatible string in the DMA enginen driver.

Don't we already get that information from the iommus DT property ? If the DMA 
engine goes through an IOMMU the property will be set, otherwise it will not.

quoted

The problem is a bit broader than that, we'll also have an issue with DMA
engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know exists?

That's theoretical. The problem I'm facing today is a DMA engine whose 
channels are served by different ports of the same IOMMU. This works in a 
suboptimal way because I have to keep all the IOMMU ports enabled regardless 
of whether they're used or not, as the DMA engine and IOMMU APIs don't carry 
channel information.

quoted

I recall discussing this in the past with you, and the solution you
proposed was to add a channel index to struct dma_attrs seems good to me.
To support the case where some channels don't go through an IOMMU we would
only need support for null entries in the IOMMUs list associated with a
device (for instance in the DT case null entries in the iommus property).

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski [off-list ref]
Date:   Wed Aug 3 13:46:00 2016 -0700

    dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number,
but I'm not very happy with that patch as I can see how a future proposal
to handle the channel number through the DMA attributes will get rejected
on the grounds of bits starvation then :-(

Agreed, that can become interesting.

Does the above-mentioned patch really fix a performance, memory consumption or 
other issue ?

-- 
Regards,

Laurent Pinchart

On Friday, September 16, 2016 3:09:29 PM CEST Laurent Pinchart wrote:

quoted

I wasn't thinking quite that far, though that is also a theoretical
problem. However, the simple solution would be to have a bit in the DMA
specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine
either goes through an IOMMU or doesn't (depending on the integration
into the SoC), so we'd have to find out through some DT property
or compatible string in the DMA enginen driver.

Don't we already get that information from the iommus DT property ? If the DMA 
engine goes through an IOMMU the property will be set, otherwise it will not.

It depends. A dmaengine typically at least has two DMA masters,
possibly more. It's likely that some dmaengine implementations are
connected to RAM through an IOMMU, but have direct access to an
I/O bus for the slave FIFOs.

quoted

quoted

The problem is a bit broader than that, we'll also have an issue with DMA
engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know exists?

That's theoretical. The problem I'm facing today is a DMA engine whose 
channels are served by different ports of the same IOMMU. This works in a 
suboptimal way because I have to keep all the IOMMU ports enabled regardless 
of whether they're used or not, as the DMA engine and IOMMU APIs don't carry 
channel information.

Ok

	Arnd

On 16/09/16 13:05, Laurent Pinchart wrote:
[...]

quoted

quoted

quoted

One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems that
all have an IOMMU, but on some of them the connection between the DMA
master and the slave FIFO bypasses the IOMMU while on others the IOMMU
is required.

Do you mean systems where some of the channels of a specific DMA engine go
through the IOMMU while others do not ? We indeed have no solution today
for such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA
engines that have different channels served by different IOMMUs. I recall
discussing this in the past with you, and the solution you proposed was to
add a channel index to struct dma_attrs seems good to me. To support the
case where some channels don't go through an IOMMU we would only need
support for null entries in the IOMMUs list associated with a device (for
instance in the DT case null entries in the iommus property).

I think at that point we just create the channels as child devices of
the main dmaengine device so they each get their own DMA ops, and can do
whatever. The Qualcomm HIDMA driver already does that for a very similar
reason (so that the IOMMU can map individual channels into different
guest VMs).

That's another option, but it seems more like a workaround to me, instead of a 
proper solution to fix the more global problem of multiple memory paths within 
a single device. I have other hardware devices that can act as bus masters 
through different paths (for instance a display-related device that fetches 
data and commands through different paths). Luckily so far all those paths are 
served by the same IOMMU, but there's no guarantee this will remain true in 
the future. Furthermore, even today, the IOMMU connected to that device has 
the ability to selectively enable and disable its ports. I have to keep them 
all enabled due to the lack of channel information in the DMA mapping and 
IOMMU APIs, leading to increased power consumption.

Indeed, I think both the Exynos and Rockchip IOMMU drivers already do
cater for a device mastering though multiple discrete IOMMUs, not being
the fancy multi-port multi-context ones like yours and mine.

I guess what we could really do with is a decent abstraction of
multi-master peripherals at the device level; a "threads within the same
process" sort of granularity, as it were. I'd envisage it more along the
lines of how we handle NUMA, i.e. dma_map_page_attrs(...) becomes a
wrapper for dma_map_page_attrs_multi(..., CHANNEL_ALL), and trickier
users can call the latter with the a more specific channel(s) argument
(maybe it's a bitmask rather than an index). Meanwhile,
dev->archdata.dma_ops may point to a device-specific array of
dma_map_ops, which the DMA API backend iterates over if necessary.

Strangely, that doesn't actually sound too horrible.

Robin.

quoted

quoted

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski [off-list ref]
Date:   Wed Aug 3 13:46:00 2016 -0700

    dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number,
but I'm not very happy with that patch as I can see how a future proposal
to handle the channel number through the DMA attributes will get rejected
on the grounds of bits starvation then :-(

quoted

I don't have any idea for how this could be handled in a generic way, so
my best answer here is to hope we never get there, and if we do, handle
it using some local hack in the driver.

Hi Arnd,

On Friday 16 Sep 2016 14:22:31 Arnd Bergmann wrote:

On Friday, September 16, 2016 3:09:29 PM CEST Laurent Pinchart wrote:

quoted

quoted

I wasn't thinking quite that far, though that is also a theoretical
problem. However, the simple solution would be to have a bit in the DMA
specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine
either goes through an IOMMU or doesn't (depending on the integration
into the SoC), so we'd have to find out through some DT property
or compatible string in the DMA enginen driver.

Don't we already get that information from the iommus DT property ? If the
DMA engine goes through an IOMMU the property will be set, otherwise it
will not.

It depends. A dmaengine typically at least has two DMA masters,
possibly more. It's likely that some dmaengine implementations are
connected to RAM through an IOMMU, but have direct access to an
I/O bus for the slave FIFOs.

Sure, but I expect the DMA engine DT node to list all the relevant IOMMU(s) 
(if any) in the iommus property in a way that allows the DMA engine driver to 
know what IOMMU port is used for what purpose. It will then be up to the DMA 
engine driver to select the right port identifier to pass to the DMA mapping 
API.

I'm not sure how this would work with Robin's proposal of creating one device 
per channel though, as there would still be a single node in DT for the DMA 
engine device. Furthermore, a single channel might indeed have multiple DMA 
masters, not all of them being served by an IOMMU. We would thus still need 
memory port identifiers in the DMA mapping API.

quoted

quoted

quoted

The problem is a bit broader than that, we'll also have an issue with
DMA engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know
exists?

That's theoretical. The problem I'm facing today is a DMA engine whose
channels are served by different ports of the same IOMMU. This works in a
suboptimal way because I have to keep all the IOMMU ports enabled
regardless of whether they're used or not, as the DMA engine and IOMMU
APIs don't carry channel information.

Ok

-- 
Regards,

Laurent Pinchart

Hi Robin,

On Friday 16 Sep 2016 13:49:21 Robin Murphy wrote:

On 16/09/16 13:05, Laurent Pinchart wrote:
[...]

quoted

quoted

quoted

quoted

One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems
that all have an IOMMU, but on some of them the connection between the
DMA master and the slave FIFO bypasses the IOMMU while on others the
IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine
go through the IOMMU while others do not ? We indeed have no solution
today for such a situation.

The problem is a bit broader than that, we'll also have an issue with
DMA engines that have different channels served by different IOMMUs. I
recall discussing this in the past with you, and the solution you
proposed was to add a channel index to struct dma_attrs seems good to
me. To support the case where some channels don't go through an IOMMU we
would only need support for null entries in the IOMMUs list associated
with a device (for instance in the DT case null entries in the iommus
property).

I think at that point we just create the channels as child devices of
the main dmaengine device so they each get their own DMA ops, and can do
whatever. The Qualcomm HIDMA driver already does that for a very similar
reason (so that the IOMMU can map individual channels into different
guest VMs).

That's another option, but it seems more like a workaround to me, instead
of a proper solution to fix the more global problem of multiple memory
paths within a single device. I have other hardware devices that can act
as bus masters through different paths (for instance a display-related
device that fetches data and commands through different paths). Luckily
so far all those paths are served by the same IOMMU, but there's no
guarantee this will remain true in the future. Furthermore, even today,
the IOMMU connected to that device has the ability to selectively enable
and disable its ports. I have to keep them all enabled due to the lack of
channel information in the DMA mapping and IOMMU APIs, leading to
increased power consumption.

Indeed, I think both the Exynos and Rockchip IOMMU drivers already do
cater for a device mastering though multiple discrete IOMMUs, not being
the fancy multi-port multi-context ones like yours and mine.

I guess what we could really do with is a decent abstraction of
multi-master peripherals at the device level; a "threads within the same
process" sort of granularity, as it were. I'd envisage it more along the
lines of how we handle NUMA, i.e. dma_map_page_attrs(...) becomes a
wrapper for dma_map_page_attrs_multi(..., CHANNEL_ALL), and trickier
users can call the latter with the a more specific channel(s) argument
(maybe it's a bitmask rather than an index).

That's pretty much what I've discussed with Arnd in the past, except that we 
were planning to add the channel to struct dma_attrs. Hence my disappointment 
seeing the structure go away.

Meanwhile, dev->archdata.dma_ops may point to a device-specific array of
dma_map_ops, which the DMA API backend iterates over if necessary.

Strangely, that doesn't actually sound too horrible.

-- 
Regards,

Laurent Pinchart

Hi Vinod,

On 2016-09-15 21:56:51 +0530, Vinod Koul wrote:

On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:

quoted

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

quoted

Hi,

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.8-rc1. And I'm hoping to be able to collect a
Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
able to take the whole series through the dmaengine tree. If this is not the
best route I'm more then happy to do it another way.

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.

As I said in last one, the dmaengine parts look fine to me. But to go thru
dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now.
If anyone has any objects, please speak up now...

I'm just curios, do you plan to merge this series with Arnds Ack? If not 
is there anything I can do to help move the series in the right 
direction?

Also ACKs welcome...

-- 
Regards,
Niklas S?derlund

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

Hi,

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.8-rc1. And I'm hoping to be able to collect a
Ack from Russell King on patch 4/6 that adds the ARM specific part and then be
able to take the whole series through the dmaengine tree. If this is not the
best route I'm more then happy to do it another way.

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.

Applied, thanks

-- 
~Vinod

Hi Niklas,

On Monday 05 Sep 2016 12:52:44 Laurent Pinchart wrote:

On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:

quoted

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 cf983a9..22a5e40 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c

[snip]

quoted

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

Shouldn't this be DMA_FROM_DEVICE, and DMA_TO_DEVICE below ?

This comment can be ignored (thank you Robin for the explanation), but ...

quoted

+	} 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, 0);

Unless I'm mistaken the resource will not be unmapped when freeing channel
resources, will it ?

I believe this one still needs to be addressed.

quoted

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+
+	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,

quoted

+		dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" :

"DMA_FROM_DEVICE");

quoted

+
+	map->slave.slave_addr = dev_addr;
+	map->slave.xfer_size = dev_size;
+	map->dir = dev_dir;
+
+	return 0;
+}

[snip]

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On 2017-01-02 01:08:04 +0200, Laurent Pinchart wrote:

Hi Niklas,

On Monday 05 Sep 2016 12:52:44 Laurent Pinchart wrote:

quoted

On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:

quoted

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 cf983a9..22a5e40 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c

[snip]

quoted

quoted

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

Shouldn't this be DMA_FROM_DEVICE, and DMA_TO_DEVICE below ?

This comment can be ignored (thank you Robin for the explanation), but ...

quoted

quoted

+	} 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, 0);

Unless I'm mistaken the resource will not be unmapped when freeing channel
resources, will it ?

I believe this one still needs to be addressed.

I believe you are correct, I will look in to it and hopefully submit a 
patch to address it. Thanks for brining it up.

quoted

quoted

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+
+	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,

quoted

+		dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" :

"DMA_FROM_DEVICE");

quoted

+
+	map->slave.slave_addr = dev_addr;
+	map->slave.xfer_size = dev_size;
+	map->dir = dev_dir;
+
+	return 0;
+}

[snip]

-- 
Regards,

Laurent Pinchart

-- 
Regards,
Niklas S?derlund