Re: [PATCH 1/2] dmaengine: rcar-dmac: add iommu support for slave transfers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

(Again with Linus' e-mail address fixed, sorry for the noise)

On Thursday 14 January 2016 01:13:20 Laurent Pinchart wrote:
> Hi Vinod,
> 
> (CC'ing Linus as he's mentioned)
> 
> On Wednesday 13 January 2016 14:55:50 Niklas Söderlund wrote:
> > * Vinod Koul <vinod.koul@intel.com> [2016-01-13 19:06:01 +0530]:
> >> On Mon, Jan 11, 2016 at 03:17:46AM +0100, Niklas Söderlund wrote:
> >>> Enable slave transfers to devices behind IPMMU:s 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 | 64 +++++++++++++++++++++++++++++++++++---
> >>>  1 file changed, 60 insertions(+), 4 deletions(-)
> >>> 
> >>> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> >>> index 7820d07..da94809 100644
> >>> --- a/drivers/dma/sh/rcar-dmac.c
> >>> +++ b/drivers/dma/sh/rcar-dmac.c
> >>> @@ -13,6 +13,7 @@
> >>>  #include <linux/dma-mapping.h>
> >>>  #include <linux/dmaengine.h>
> >>>  #include <linux/interrupt.h>
> >>> +#include <linux/iommu.h>
> >>>  #include <linux/list.h>
> >>>  #include <linux/module.h>
> >>>  #include <linux/mutex.h>
> >>> @@ -1101,6 +1102,24 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,
> >>> dma_addr_t buf_addr,
> >>>  	return desc;
> >>>  }
> >>> 
> >>> +static dma_addr_t __rcar_dmac_dma_map(struct dma_chan *chan,
> >>> phys_addr_t addr,
> >>> +		size_t size, enum dma_data_direction dir)
> >>> +{
> >>> +	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
> >>> +	struct page *page = phys_to_page(addr);
> >>> +	size_t offset = addr - page_to_phys(page);
> >>> +	dma_addr_t map = dma_map_page(chan->device->dev, page, offset, size,
> >>> +			dir);
> >> 
> >> Hmmmm, dmaengine APIs for slave cases expect that client has already
> >> ammped and provided an address which the dmaengine understands. So doing
> >> this in driver here does not sound good to me
> > 
> > It was my understanding that clients do not do this mapping and in fact
> > are expected not to. Is this not what Linus Walleij is trying to address
> > in '[PATCH] dmaengine: use phys_addr_t for slave configuration'?
> 
> There's a problem somewhere and we need to fix it. Clients currently pass
> physical addresses and the DMA engine API expects a DMA address. There's
> only two ways to fix that, either modify the API to expect a phys_addr_t,
> or modify the clients to provide a dma_addr_t.
> 
> The struct device used to map buffer through the DMA mapping API needs to be
> the DMA engine struct device, not the client struct device. As the client
> is not expected to have access to the DMA engine device I would argue that
> DMA engines should perform the mapping and the API should take a
> phys_addr_t.
> 
> Vinod, unless you have reasons to do it otherwise, can we get your ack on
> this approach and start hammering at the code ? The problem has remained
> known and unfixed for too long, we need to move on.
> 
> >>> On Fri, Apr 26, 2013 at 11:06 AM, Linus Walleij wrote:
> >>>> The documentation already says these are physical addresses, and
> >>>> we have concluded that any translation into the DMA address space
> >>>> needs to reside in the dmaengine driver, so change the type of
> >>>> the passed arguments

-- 
Regards,

Laurent Pinchart