[PATCH v5 0/9] dmaengine: rcar-dmac: add iommu support for slave transfers

[PATCH v5 0/9] dmaengine: rcar-dmac: add iommu support for slave transfers

[Niklas Söderlund]

Hi,

This series add iommu support to rcar-dmac. It's tested on Koelsch with
CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I
verified operation by interacting with /dev/mmcblk1 and the serial
console which both are devices behind the iommu.

The series depends patch '[PATCH] dmaengine: use phys_addr_t for slave
configuration'.

* 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 (8):
  dma-mapping: add {map,unmap}_resource to dma_map_ops
  dma-mapping: add dma_{map,unmap}_resource
  arm: dma-mapping: add {map,unmap}_resource for iommu ops
  dmaengine: rcar-dmac: slave address are physical
  dmaengine: rcar-dmac: group slave configuration
  dmaengine: rcar-dmac: add iommu support for slave transfers
  ARM: dts: r8a7790: add iommus to dmac0 and dmac1
  ARM: dts: r8a7791: add iommus to dmac0 and dmac1

Robin Murphy (1):
  iommu: Add MMIO mapping type

 arch/arm/boot/dts/r8a7790.dtsi |  30 +++++++++++
 arch/arm/boot/dts/r8a7791.dtsi |  30 +++++++++++
 arch/arm/mm/dma-mapping.c      |  63 ++++++++++++++++++++++
 drivers/dma/sh/rcar-dmac.c     | 116 +++++++++++++++++++++++++++++++++--------
 drivers/iommu/io-pgtable-arm.c |   9 +++-
 include/linux/dma-mapping.h    |  38 ++++++++++++++
 include/linux/iommu.h          |   1 +
 7 files changed, 263 insertions(+), 24 deletions(-)

--
2.7.2

[PATCH v5 1/9] iommu: Add MMIO mapping type

[Niklas Söderlund]

From: Robin Murphy <robin.murphy@arm.com>

On some platforms, MMIO regions might need slightly different treatment
compared to mapping regular memory; add the notion of MMIO mappings to
the IOMMU API's memory type flags, so that callers can let the IOMMU
drivers know to do the right thing.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/iommu/io-pgtable-arm.c | 9 +++++++--
 include/linux/iommu.h          | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 381ca5a..9c19989 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -355,7 +355,10 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
 			pte |= ARM_LPAE_PTE_AP_RDONLY;
 
-		if (prot & IOMMU_CACHE)
+		if (prot & IOMMU_MMIO)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+		else if (prot & IOMMU_CACHE)
 			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	} else {
@@ -364,7 +367,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 			pte |= ARM_LPAE_PTE_HAP_READ;
 		if (prot & IOMMU_WRITE)
 			pte |= ARM_LPAE_PTE_HAP_WRITE;
-		if (prot & IOMMU_CACHE)
+		if (prot & IOMMU_MMIO)
+			pte |= ARM_LPAE_PTE_MEMATTR_DEV;
+		else if (prot & IOMMU_CACHE)
 			pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
 		else
 			pte |= ARM_LPAE_PTE_MEMATTR_NC;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a5c539f..34b6432 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -30,6 +30,7 @@
 #define IOMMU_WRITE	(1 << 1)
 #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
 #define IOMMU_NOEXEC	(1 << 3)
+#define IOMMU_MMIO	(1 << 4) /* e.g. things like MSI doorbells */
 
 struct iommu_ops;
 struct iommu_group;
-- 
2.7.2

[PATCH v5 2/9] 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 75857cd..e3aba4e 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.7.2

[PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Niklas Söderlund]

Map/Unmap a device 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>
---
 include/linux/dma-mapping.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e3aba4e..cc305d1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -216,6 +216,38 @@ 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);
+
+	BUG_ON(!valid_dma_direction(dir));
+
+	/* Don't allow RAM to be mapped */
+	BUG_ON(pfn_valid(pfn));
+
+	if (ops->map_resource)
+		return ops->map_resource(dev, phys_addr, size, dir, attrs);
+
+	return phys_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);
+
+}
+
 static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
 					   size_t size,
 					   enum dma_data_direction dir)
-- 
2.7.2

[PATCH v5 4/9] 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 0eca381..ae2b175 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1814,6 +1814,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)
 {
@@ -1858,6 +1915,9 @@ struct dma_map_ops iommu_ops = {
 	.sync_sg_for_cpu	= arm_iommu_sync_sg_for_cpu,
 	.sync_sg_for_device	= arm_iommu_sync_sg_for_device,
 
+	.map_resource		= arm_iommu_map_resource,
+	.unmap_resource		= arm_iommu_unmap_resource,
+
 	.set_dma_mask		= arm_dma_set_mask,
 };
 
@@ -1873,6 +1933,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,
+
 	.set_dma_mask	= arm_dma_set_mask,
 };
 
-- 
2.7.2

[PATCH v5 5/9] dmaengine: rcar-dmac: slave address are physical

[Niklas Söderlund]

Slave addresses coming from a client is physical not dma. Store the
address using the correct data type. 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>
---
 drivers/dma/sh/rcar-dmac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 7820d07..01cf82f 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -144,8 +144,8 @@ struct rcar_dmac_chan {
 
 	unsigned int src_xfer_size;
 	unsigned int dst_xfer_size;
-	dma_addr_t src_slave_addr;
-	dma_addr_t dst_slave_addr;
+	phys_addr_t src_slave_addr;
+	phys_addr_t dst_slave_addr;
 	int mid_rid;
 
 	spinlock_t lock;
-- 
2.7.2

[PATCH v5 6/9] 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>
---
 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 01cf82f..b3911fe 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;
-	phys_addr_t src_slave_addr;
-	phys_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.7.2

[PATCH v5 7/9] 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 b3911fe..56816e3 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.7.2

[PATCH v5 8/9] ARM: dts: r8a7790: add iommus to dmac0 and dmac1

[Niklas Söderlund]

A unconfirmed hardware bug prevents channel 0 and 15 to be used by the
DMAC together with the IPMMU. The DMAC driver will disable the channels
reducing the effective number of channels to 14 per DMAC.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm/boot/dts/r8a7790.dtsi | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 26772f7..c600daa 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -294,6 +294,21 @@
 		power-domains = <&cpg_clocks>;
 		#dma-cells = <1>;
 		dma-channels = <15>;
+		iommus = <&ipmmu_ds 0>,
+			 <&ipmmu_ds 1>,
+			 <&ipmmu_ds 2>,
+			 <&ipmmu_ds 3>,
+			 <&ipmmu_ds 4>,
+			 <&ipmmu_ds 5>,
+			 <&ipmmu_ds 6>,
+			 <&ipmmu_ds 7>,
+			 <&ipmmu_ds 8>,
+			 <&ipmmu_ds 9>,
+			 <&ipmmu_ds 10>,
+			 <&ipmmu_ds 11>,
+			 <&ipmmu_ds 12>,
+			 <&ipmmu_ds 13>,
+			 <&ipmmu_ds 14>;
 	};
 
 	dmac1: dma-controller@e6720000 {
@@ -325,6 +340,21 @@
 		power-domains = <&cpg_clocks>;
 		#dma-cells = <1>;
 		dma-channels = <15>;
+		iommus = <&ipmmu_ds 15>,
+			 <&ipmmu_ds 16>,
+			 <&ipmmu_ds 17>,
+			 <&ipmmu_ds 18>,
+			 <&ipmmu_ds 19>,
+			 <&ipmmu_ds 20>,
+			 <&ipmmu_ds 21>,
+			 <&ipmmu_ds 22>,
+			 <&ipmmu_ds 23>,
+			 <&ipmmu_ds 24>,
+			 <&ipmmu_ds 25>,
+			 <&ipmmu_ds 26>,
+			 <&ipmmu_ds 27>,
+			 <&ipmmu_ds 28>,
+			 <&ipmmu_ds 29>;
 	};
 
 	audma0: dma-controller@ec700000 {
-- 
2.7.2

[PATCH v5 9/9] ARM: dts: r8a7791: add iommus to dmac0 and dmac1

[Niklas Söderlund]

A unconfirmed hardware bug prevents channel 0 and 15 to be used by the
DMAC together with the IPMMU. The DMAC driver will disable the channels
reducing the effective number of channels to 14 per DMAC.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm/boot/dts/r8a7791.dtsi | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index c6bee4a..341c460 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -283,6 +283,21 @@
 		power-domains = <&cpg_clocks>;
 		#dma-cells = <1>;
 		dma-channels = <15>;
+		iommus = <&ipmmu_ds 0>,
+			 <&ipmmu_ds 1>,
+			 <&ipmmu_ds 2>,
+			 <&ipmmu_ds 3>,
+			 <&ipmmu_ds 4>,
+			 <&ipmmu_ds 5>,
+			 <&ipmmu_ds 6>,
+			 <&ipmmu_ds 7>,
+			 <&ipmmu_ds 8>,
+			 <&ipmmu_ds 9>,
+			 <&ipmmu_ds 10>,
+			 <&ipmmu_ds 11>,
+			 <&ipmmu_ds 12>,
+			 <&ipmmu_ds 13>,
+			 <&ipmmu_ds 14>;
 	};
 
 	dmac1: dma-controller@e6720000 {
@@ -314,6 +329,21 @@
 		power-domains = <&cpg_clocks>;
 		#dma-cells = <1>;
 		dma-channels = <15>;
+		iommus = <&ipmmu_ds 15>,
+			 <&ipmmu_ds 16>,
+			 <&ipmmu_ds 17>,
+			 <&ipmmu_ds 18>,
+			 <&ipmmu_ds 19>,
+			 <&ipmmu_ds 20>,
+			 <&ipmmu_ds 21>,
+			 <&ipmmu_ds 22>,
+			 <&ipmmu_ds 23>,
+			 <&ipmmu_ds 24>,
+			 <&ipmmu_ds 25>,
+			 <&ipmmu_ds 26>,
+			 <&ipmmu_ds 27>,
+			 <&ipmmu_ds 28>,
+			 <&ipmmu_ds 29>;
 	};
 
 	audma0: dma-controller@ec700000 {
-- 
2.7.2

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Christoph Hellwig]

Please add some documentation on where/how this should be used.  It's
not a very obvious interface.

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Niklas S??derlund]

Hi Christoph,

On 2016-03-07 23:38:47 -0800, Christoph Hellwig wrote:
> Please add some documentation on where/how this should be used.  It's
> not a very obvious interface.

Good idea, I have added the following to Documentation/DMA-API.txt and 
folded it in to this patch. Do you feel it's adequate and do you know 
anywhere else I should add documentation?

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 45ef3f2..248556a 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -277,14 +277,29 @@ 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)
+
+Maps a MMIO region so it can be accessed by the device and returns the
+DMA address of the memory. API should only be used to map device MMIO,
+mapping of RAM is not permitted.
+
+void
+dma_unmap_resource(struct device *dev, dma_addr_t addr, size_t size,
+                  enum dma_data_direction dir, struct dma_attrs *attrs)
+
+Unmaps the IOMMU region previously mapped. All the parameters passed in
+must be identical to those passed in (and returned) by the mapping API.
+
 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).

-- 
Regards,
Niklas Söderlund

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Vinod Koul]

On Thu, Mar 10, 2016 at 05:05:23PM +0100, Niklas S??derlund wrote:
> Hi Christoph,
> 
> On 2016-03-07 23:38:47 -0800, Christoph Hellwig wrote:
> > Please add some documentation on where/how this should be used.  It's
> > not a very obvious interface.
> 
> Good idea, I have added the following to Documentation/DMA-API.txt and 
> folded it in to this patch. Do you feel it's adequate and do you know 
> anywhere else I should add documentation?
> 
> diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
> index 45ef3f2..248556a 100644
> --- a/Documentation/DMA-API.txt
> +++ b/Documentation/DMA-API.txt
> @@ -277,14 +277,29 @@ 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)
> +
> +Maps a MMIO region so it can be accessed by the device and returns the
> +DMA address of the memory. API should only be used to map device MMIO,
> +mapping of RAM is not permitted.
> +
> +void
> +dma_unmap_resource(struct device *dev, dma_addr_t addr, size_t size,
> +                  enum dma_data_direction dir, struct dma_attrs *attrs)

Using function prototypes in documentation may not be a great idea as you
are explaining the role of this function and not arguments.

> +
> +Unmaps the IOMMU region previously mapped. All the parameters passed in
> +must be identical to those passed in (and returned) by the mapping API.
> +
>  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).
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
~Vinod

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Dan Williams]

On Thu, Mar 10, 2016 at 8:05 AM, Niklas S??derlund
<niklas.soderlund@ragnatech.se> wrote:
> Hi Christoph,
>
> On 2016-03-07 23:38:47 -0800, Christoph Hellwig wrote:
>> Please add some documentation on where/how this should be used.  It's
>> not a very obvious interface.
>
> Good idea, I have added the following to Documentation/DMA-API.txt and
> folded it in to this patch. Do you feel it's adequate and do you know
> anywhere else I should add documentation?
>
> diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
> index 45ef3f2..248556a 100644
> --- a/Documentation/DMA-API.txt
> +++ b/Documentation/DMA-API.txt
> @@ -277,14 +277,29 @@ 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)
> +
> +Maps a MMIO region so it can be accessed by the device and returns the
> +DMA address of the memory. API should only be used to map device MMIO,
> +mapping of RAM is not permitted.
> +

I think it is confusing to use the dma_ prefix for this peer-to-peer
mmio functionality.  dma_addr_t is a device's view of host memory.
Something like bus_addr_t bus_map_resource().  Doesn't this routine
also need the source device in addition to the target device?  The
resource address is from the perspective of the host cpu, it may be a
different address space in the view of two devices relative to each
other.

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Christoph Hellwig]

On Thu, Mar 10, 2016 at 10:47:10PM -0800, Dan Williams wrote:
> I think it is confusing to use the dma_ prefix for this peer-to-peer
> mmio functionality.  dma_addr_t is a device's view of host memory.
> Something like bus_addr_t bus_map_resource().  Doesn't this routine
> also need the source device in addition to the target device?  The
> resource address is from the perspective of the host cpu, it may be a
> different address space in the view of two devices relative to each
> other.

Is it supposed to be per-mmio?  It's in dma-mapping ops, and has dma
in the name, so I suspected it's for some form of peer dma.  But given
that our dma APIs reuqire a struct page backing I have no idea how this
even supposed to work, and this little documentation blurb still doesn't
clear that up.

So for now I'd like to NAK this patch until the use case can be
explained clearly, and actually works.

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Niklas Söderlund]

Hi all,

Thanks for your comments.

On 2016-03-11 03:15:22 -0800, Christoph Hellwig wrote:
> On Thu, Mar 10, 2016 at 10:47:10PM -0800, Dan Williams wrote:
> > I think it is confusing to use the dma_ prefix for this peer-to-peer
> > mmio functionality.  dma_addr_t is a device's view of host memory.
> > Something like bus_addr_t bus_map_resource().  Doesn't this routine
> > also need the source device in addition to the target device?  The
> > resource address is from the perspective of the host cpu, it may be a
> > different address space in the view of two devices relative to each
> > other.
> 
> Is it supposed to be per-mmio?  It's in dma-mapping ops, and has dma
> in the name, so I suspected it's for some form of peer dma.  But given
> that our dma APIs reuqire a struct page backing I have no idea how this
> even supposed to work, and this little documentation blurb still doesn't
> clear that up.
> 
> So for now I'd like to NAK this patch until the use case can be
> explained clearly, and actually works.

I can explain the use case and maybe we can figure out if this approach 
is the correct one to solve it.

The problem is that I have devices behind an IOMMU which I would like to 
use with DMA. Vinod recently moved forward with his and Linus Walleij
patch '[PATCH] dmaengine: use phys_addr_t for slave configuration' which 
clarifies that the DMA slave address provided by a client 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. Is it not very similar to dma_map_single() where one maps 
processor virtual memory (instead if MMIO) so that it can be used with 
DMA slaves?

-- 
Regards,
Niklas Söderlund

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Robin Murphy]

Hi Dan,

On 11/03/16 06:47, Dan Williams wrote:
> On Thu, Mar 10, 2016 at 8:05 AM, Niklas S??derlund
> <niklas.soderlund@ragnatech.se> wrote:
>> Hi Christoph,
>>
>> On 2016-03-07 23:38:47 -0800, Christoph Hellwig wrote:
>>> Please add some documentation on where/how this should be used.  It's
>>> not a very obvious interface.
>>
>> Good idea, I have added the following to Documentation/DMA-API.txt and
>> folded it in to this patch. Do you feel it's adequate and do you know
>> anywhere else I should add documentation?
>>
>> diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
>> index 45ef3f2..248556a 100644
>> --- a/Documentation/DMA-API.txt
>> +++ b/Documentation/DMA-API.txt
>> @@ -277,14 +277,29 @@ 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)
>> +
>> +Maps a MMIO region so it can be accessed by the device and returns the
>> +DMA address of the memory. API should only be used to map device MMIO,
>> +mapping of RAM is not permitted.
>> +
>
> I think it is confusing to use the dma_ prefix for this peer-to-peer
> mmio functionality.  dma_addr_t is a device's view of host memory.
> Something like bus_addr_t bus_map_resource().  Doesn't this routine
> also need the source device in addition to the target device?  The
> resource address is from the perspective of the host cpu, it may be a
> different address space in the view of two devices relative to each
> other.

Hmm, the trouble with that is that when the DMA master is behind an 
IOMMU, the address space as seen by the device is dynamic and whatever 
we decide it to be, so there is no distinction between a "DMA" address 
and a "bus" address.

In practice the dmaengine API has clearly worked for however long with 
slave MMIO addresses being a dma_addr_t, and it doesn't look like anyone 
objected to the change to phys_addr_t in -next either. If nothing is 
using bus_addr_t anyway, what's the right thing to do? Looking up 
through higher abstraction layers, we have the likes of struct 
snd_dmaengine_dai_dma_data also expecting the slave address to be a 
dma_addr_t, leading to things like the direct casting in 
bcm2835_i2s_probe() for the non-IOMMU dma != phys != bus case that could 
also be cleaned up with this proposed interface.

Robin.

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Dan Williams]

On Fri, Mar 11, 2016 at 5:46 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> Hi Dan,
>
>
> On 11/03/16 06:47, Dan Williams wrote:
>>
>> On Thu, Mar 10, 2016 at 8:05 AM, Niklas S??derlund
>> <niklas.soderlund@ragnatech.se> wrote:
>>>
>>> Hi Christoph,
>>>
>>> On 2016-03-07 23:38:47 -0800, Christoph Hellwig wrote:
>>>>
>>>> Please add some documentation on where/how this should be used.  It's
>>>> not a very obvious interface.
>>>
>>>
>>> Good idea, I have added the following to Documentation/DMA-API.txt and
>>> folded it in to this patch. Do you feel it's adequate and do you know
>>> anywhere else I should add documentation?
>>>
>>> diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
>>> index 45ef3f2..248556a 100644
>>> --- a/Documentation/DMA-API.txt
>>> +++ b/Documentation/DMA-API.txt
>>> @@ -277,14 +277,29 @@ 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)
>>> +
>>> +Maps a MMIO region so it can be accessed by the device and returns the
>>> +DMA address of the memory. API should only be used to map device MMIO,
>>> +mapping of RAM is not permitted.
>>> +
>>
>>
>> I think it is confusing to use the dma_ prefix for this peer-to-peer
>> mmio functionality.  dma_addr_t is a device's view of host memory.
>> Something like bus_addr_t bus_map_resource().  Doesn't this routine
>> also need the source device in addition to the target device?  The
>> resource address is from the perspective of the host cpu, it may be a
>> different address space in the view of two devices relative to each
>> other.
>
>
> Hmm, the trouble with that is that when the DMA master is behind an IOMMU,
> the address space as seen by the device is dynamic and whatever we decide it
> to be, so there is no distinction between a "DMA" address and a "bus"
> address.
>
> In practice the dmaengine API has clearly worked for however long with slave
> MMIO addresses being a dma_addr_t, and it doesn't look like anyone objected
> to the change to phys_addr_t in -next either. If nothing is using bus_addr_t
> anyway, what's the right thing to do? Looking up through higher abstraction
> layers, we have the likes of struct snd_dmaengine_dai_dma_data also
> expecting the slave address to be a dma_addr_t, leading to things like the
> direct casting in bcm2835_i2s_probe() for the non-IOMMU dma != phys != bus
> case that could also be cleaned up with this proposed interface.
>

So the "bus_addr_t" reaction was prompted by the recent activity of
RDMA developers looking to re-use the devm_memremap_pages() api.  That
enabling is looking at how to setup peer-to-peer PCI-E cycles for an
RDMA device to deliver data to another local device without taking a
round trip through host memory.

I understand the history of the dmaengine-slave implementation, but it
seems we're getting to point where we need a less overloaded
identifier than "dma" for the case of devices talking to each other.

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Christoph Hellwig]

On Fri, Mar 11, 2016 at 01:58:46PM +0100, Niklas S?derlund wrote:
> 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. Is it not very similar to dma_map_single() where one maps 
> processor virtual memory (instead if MMIO) so that it can be used with 
> DMA slaves?

It's similar, but I don't think this actually works as a general case
as there are quite a few places that expect to be able to have a
struct page for a physical address.  We'd at least need a very careful
audit for that case.

Re: [PATCH v5 1/9] iommu: Add MMIO mapping type

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 08 March 2016 03:42:46 Niklas Söderlund wrote:
> From: Robin Murphy <robin.murphy@arm.com>
> 
> On some platforms, MMIO regions might need slightly different treatment
> compared to mapping regular memory; add the notion of MMIO mappings to
> the IOMMU API's memory type flags, so that callers can let the IOMMU
> drivers know to do the right thing.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

> ---
>  drivers/iommu/io-pgtable-arm.c | 9 +++++++--
>  include/linux/iommu.h          | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 381ca5a..9c19989 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -355,7 +355,10 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct
> arm_lpae_io_pgtable *data, if (!(prot & IOMMU_WRITE) && (prot &
> IOMMU_READ))
>  			pte |= ARM_LPAE_PTE_AP_RDONLY;
> 
> -		if (prot & IOMMU_CACHE)
> +		if (prot & IOMMU_MMIO)
> +			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
> +				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
> +		else if (prot & IOMMU_CACHE)
>  			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
>  				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
>  	} else {
> @@ -364,7 +367,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct
> arm_lpae_io_pgtable *data, pte |= ARM_LPAE_PTE_HAP_READ;
>  		if (prot & IOMMU_WRITE)
>  			pte |= ARM_LPAE_PTE_HAP_WRITE;
> -		if (prot & IOMMU_CACHE)
> +		if (prot & IOMMU_MMIO)
> +			pte |= ARM_LPAE_PTE_MEMATTR_DEV;
> +		else if (prot & IOMMU_CACHE)
>  			pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
>  		else
>  			pte |= ARM_LPAE_PTE_MEMATTR_NC;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a5c539f..34b6432 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -30,6 +30,7 @@
>  #define IOMMU_WRITE	(1 << 1)
>  #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
>  #define IOMMU_NOEXEC	(1 << 3)
> +#define IOMMU_MMIO	(1 << 4) /* e.g. things like MSI doorbells */
> 
>  struct iommu_ops;
>  struct iommu_group;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 6/9] dmaengine: rcar-dmac: group slave configuration

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 08 March 2016 03:42:51 Niklas Söderlund wrote:
> 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 01cf82f..b3911fe 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;
> -	phys_addr_t src_slave_addr;
> -	phys_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;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 5/9] dmaengine: rcar-dmac: slave address are physical

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 08 March 2016 03:42:50 Niklas Söderlund wrote:
> Slave addresses coming from a client is physical not dma. Store the
> address using the correct data type. 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>
> ---
>  drivers/dma/sh/rcar-dmac.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 7820d07..01cf82f 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -144,8 +144,8 @@ struct rcar_dmac_chan {
> 
>  	unsigned int src_xfer_size;
>  	unsigned int dst_xfer_size;
> -	dma_addr_t src_slave_addr;
> -	dma_addr_t dst_slave_addr;
> +	phys_addr_t src_slave_addr;
> +	phys_addr_t dst_slave_addr;

This moves the cast from phys_addr_t to dma_addr_t from the driver's DMA 
engine operations to other places. I'm not sure there's much value in doing 
so. I'd squash this patch with 7/9, the result will be easier to review.

>  	int mid_rid;
> 
>  	spinlock_t lock;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Laurent Pinchart]

Hello,

On Tuesday 15 March 2016 01:22:54 Christoph Hellwig wrote:
> On Fri, Mar 11, 2016 at 01:58:46PM +0100, Niklas S?derlund wrote:
> > 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. Is it not very similar to dma_map_single() where one maps
> > processor virtual memory (instead if MMIO) so that it can be used with
> > DMA slaves?
> 
> It's similar, but I don't think this actually works as a general case
> as there are quite a few places that expect to be able to have a
> struct page for a physical address.  We'd at least need a very careful
> audit for that case.

The good news is that, given that no code uses this new API at the moment, 
there isn't much to audit. The patch series implements the resource mapping 
for arch/arm only, and makes use of it in the rcar-dmac driver only. Would you 
like anything audited else than the arch/arm dma mapping implementation, the 
rcar-dmac driver and the code that then deals with the dma addresses (I'm 
thinking about the IOMMU subsystem and the ipmmu-vmsa driver in particular) ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Christoph Hellwig]

On Thu, Mar 17, 2016 at 01:33:51PM +0200, Laurent Pinchart wrote:
> The good news is that, given that no code uses this new API at the moment, 
> there isn't much to audit. The patch series implements the resource mapping 
> for arch/arm only, and makes use of it in the rcar-dmac driver only. Would you 
> like anything audited else than the arch/arm dma mapping implementation, the 
> rcar-dmac driver and the code that then deals with the dma addresses (I'm 
> thinking about the IOMMU subsystem and the ipmmu-vmsa driver in particular) ?

Yes, it would be good to do an audit of all the ARM dma_ops as well
as generic code like drivers/base/dma-*.c, lib/dma-debug.c and
include/linux/dma-*.h

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Niklas Söderlund]

Hi Christoph,

On 2016-03-21 08:26:01 -0700, Christoph Hellwig wrote:
> On Thu, Mar 17, 2016 at 01:33:51PM +0200, Laurent Pinchart wrote:
> > The good news is that, given that no code uses this new API at the moment, 
> > there isn't much to audit. The patch series implements the resource mapping 
> > for arch/arm only, and makes use of it in the rcar-dmac driver only. Would you 
> > like anything audited else than the arch/arm dma mapping implementation, the 
> > rcar-dmac driver and the code that then deals with the dma addresses (I'm 
> > thinking about the IOMMU subsystem and the ipmmu-vmsa driver in particular) ?
> 
> Yes, it would be good to do an audit of all the ARM dma_ops as well
> as generic code like drivers/base/dma-*.c, lib/dma-debug.c and
> include/linux/dma-*.h

I have now done an audit to the best of my abilities, thanks to Laurent 
for pointing me in the right direction. And from what I can tell we are 
good.

* 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 transfere 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, memmory that is mapped is device memmory
  - 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

-- 
Regards,
Niklas Söderlund

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Niklas Söderlund]

Hi Christoph,

Have you had time to look at the audit? Is there anything else I can do 
make progress on this?

On 2016-04-13 15:29:16 +0200, Niklas Söderlund wrote:
> Hi Christoph,
> 
> On 2016-03-21 08:26:01 -0700, Christoph Hellwig wrote:
> > On Thu, Mar 17, 2016 at 01:33:51PM +0200, Laurent Pinchart wrote:
> > > The good news is that, given that no code uses this new API at the moment, 
> > > there isn't much to audit. The patch series implements the resource mapping 
> > > for arch/arm only, and makes use of it in the rcar-dmac driver only. Would you 
> > > like anything audited else than the arch/arm dma mapping implementation, the 
> > > rcar-dmac driver and the code that then deals with the dma addresses (I'm 
> > > thinking about the IOMMU subsystem and the ipmmu-vmsa driver in particular) ?
> > 
> > Yes, it would be good to do an audit of all the ARM dma_ops as well
> > as generic code like drivers/base/dma-*.c, lib/dma-debug.c and
> > include/linux/dma-*.h
> 
> I have now done an audit to the best of my abilities, thanks to Laurent 
> for pointing me in the right direction. And from what I can tell we are 
> good.
> 
> * 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 transfere 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, memmory that is mapped is device memmory
>   - 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

-- 
Regards,
Niklas Söderlund

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Christoph Hellwig]

On Wed, Apr 13, 2016 at 03:29:17PM +0200, Niklas S?derlund wrote:
> > Yes, it would be good to do an audit of all the ARM dma_ops as well
> > as generic code like drivers/base/dma-*.c, lib/dma-debug.c and
> > include/linux/dma-*.h

What about things like the phys_addr() helper in lib/dma-debug.c?  That
was in fact what prompted my question for an audit, and it seems
to not even feature on your list.  What patterns / symbols did you look
for?

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Niklas Söderlund]

Hi Christoph,

On 2016-04-21 06:49:42 -0700, Christoph Hellwig wrote:
> On Wed, Apr 13, 2016 at 03:29:17PM +0200, Niklas S?derlund wrote:
> > > Yes, it would be good to do an audit of all the ARM dma_ops as well
> > > as generic code like drivers/base/dma-*.c, lib/dma-debug.c and
> > > include/linux/dma-*.h
> 
> What about things like the phys_addr() helper in lib/dma-debug.c?  That
> was in fact what prompted my question for an audit, and it seems
> to not even feature on your list.  What patterns / symbols did you look
> for?


I have followed the call path from the usage in 
drivers/dma/sh/rcar-dmac.c and made sure the dma_addr_t is not used in a 
bad way.

I also looked at the dma-mapping API and ruled out that the only 
function that make sens to use with a dma_addr_t from dma_map_resource() 
are dma_unmap_resource() and dma_mapping_error(). With that I can't see 
how a dma_addr_t would end up in lib/dma-debug.c. But I might be missing 
something?

In the big picture do you feel the approach I have is the correct way to 
solve my problem? Provided we can work out this issues ofc?

-- 
Regards,
Niklas Söderlund

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Christoph Hellwig]

On Mon, Apr 25, 2016 at 04:26:19PM +0200, Niklas S?derlund wrote:
> I have followed the call path from the usage in 
> drivers/dma/sh/rcar-dmac.c and made sure the dma_addr_t is not used in a 
> bad way.

The dma-debug routines are called from the generic code in
include/linux/dma-mapping.h, and from my reading of the other patches
in your series you are calling it for these as well.

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Niklas Söderlund]

Hi Christoph,

On 2016-04-25 12:10:04 -0700, Christoph Hellwig wrote:
> On Mon, Apr 25, 2016 at 04:26:19PM +0200, Niklas S?derlund wrote:
> > I have followed the call path from the usage in 
> > drivers/dma/sh/rcar-dmac.c and made sure the dma_addr_t is not used in a 
> > bad way.
> 
> The dma-debug routines are called from the generic code in
> include/linux/dma-mapping.h, and from my reading of the other patches
> in your series you are calling it for these as well.

You are correct I have not consider that dma_mapping_error() call in to 
lib/dma-debug.c. I will see if I can make the dma_mapping_error() safe 
to use with a dma_addr_t obtained from dma_map_resource() and post a new 
series.

Thanks for pointing this out!

-- 
Regards,
Niklas Söderlund