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

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

[Niklas Söderlund]

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

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

[Niklas Söderlund]

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

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

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 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

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

[Niklas Söderlund]

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

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

diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index fe8cb61..c7d844f 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -56,6 +56,13 @@ extern void debug_dma_alloc_coherent(struct device *dev, size_t size,
 extern void debug_dma_free_coherent(struct device *dev, size_t size,
 				    void *virt, dma_addr_t addr);
 
+extern void debug_dma_map_resource(struct device *dev, phys_addr_t addr,
+				   size_t size, int direction,
+				   dma_addr_t dma_addr);
+
+extern void debug_dma_unmap_resource(struct device *dev, dma_addr_t dma_addr,
+				     size_t size, int direction);
+
 extern void debug_dma_sync_single_for_cpu(struct device *dev,
 					  dma_addr_t dma_handle, size_t size,
 					  int direction);
@@ -141,6 +148,18 @@ static inline void debug_dma_free_coherent(struct device *dev, size_t size,
 {
 }
 
+static inline void debug_dma_map_resource(struct device *dev, phys_addr_t addr,
+					  size_t size, int direction,
+					  dma_addr_t dma_addr)
+{
+}
+
+static inline void debug_dma_unmap_resource(struct device *dev,
+					    dma_addr_t dma_addr, size_t size,
+					    int direction)
+{
+}
+
 static inline void debug_dma_sync_single_for_cpu(struct device *dev,
 						 dma_addr_t dma_handle,
 						 size_t size, int direction)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 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

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

[Laurent Pinchart]

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>

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

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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

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

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 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

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

[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>

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

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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

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

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 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

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

[Niklas Söderlund]

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:
> 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
> 

-- 
Regards,
Niklas S?derlund

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

[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:
> > 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

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

[Niklas Söderlund]

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

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

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 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

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

[Niklas Söderlund]

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

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

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 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

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:
> 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 ?

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

-- 
Regards,

Laurent Pinchart

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

[Robin Murphy]

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:
>> 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.

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

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

[Laurent Pinchart]

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:
> > 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]

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

> > +	} 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.

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

[snip]

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

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:
> > On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:
> > > 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]
> 
> > > +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 ...
> 
> > > +	} 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.

> 
> > > +	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;
> > > +}
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas S?derlund

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

[Vinod Koul]

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

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

[Vinod Koul]

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:
> > 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

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

[Arnd Bergmann]

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:
> > 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.
> 
> 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

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

[Laurent Pinchart]

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:
> > 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:
> >>> 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 <k.kozlowski@samsung.com>
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

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

[Robin Murphy]

On 16/09/16 10:48, Laurent Pinchart wrote:
> 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:
>>> 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:
>>>>> 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 <k.kozlowski@samsung.com>
> 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.
> 

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

[Laurent Pinchart]

Hi Rubin,

On Friday 16 Sep 2016 11:36:29 Robin Murphy wrote:
> On 16/09/16 10:48, Laurent Pinchart wrote:
> > On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> >> 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:
> >>>> 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.
> >>> 
> >>> 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.

> > Now I see that struct dma_attrs has been replaced by unsigned long in
> > 
> > commit 00085f1efa387a8ce100e3734920f7639c80caa3
> > Author: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > 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

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

[Robin Murphy]

On 16/09/16 13:05, Laurent Pinchart wrote:
[...]
>>>> 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.

> 
>>> Now I see that struct dma_attrs has been replaced by unsigned long in
>>>
>>> commit 00085f1efa387a8ce100e3734920f7639c80caa3
>>> Author: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>> 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.
> 

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

[Laurent Pinchart]

Hi Robin,

On Friday 16 Sep 2016 13:49:21 Robin Murphy wrote:
> On 16/09/16 13:05, Laurent Pinchart wrote:
> [...]
> 
> >>>> 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

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

[Arnd Bergmann]

On Friday, September 16, 2016 12:48:23 PM CEST Laurent Pinchart wrote:
> On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> > 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:
> > >> 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 <k.kozlowski@samsung.com>
> 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

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

[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:
> > On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> >> 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:
> >>>> 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.

> > 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.

> > 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 <k.kozlowski@samsung.com>
> > 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

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

[Arnd Bergmann]

On Friday, September 16, 2016 3:09:29 PM CEST Laurent Pinchart wrote:
> > 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.

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

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

[Laurent Pinchart]

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:
> >> 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.

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

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

[Niklas Söderlund]

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:
> > 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.
> 
> 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

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

[Vinod Koul]

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