fsldma: add support to 36-bit physical address

Signed-off-by: Li Yang <leoli@freescale.com>
---
 drivers/dma/fsldma.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

On Sep 21, 2010, at 5:57 AM, Li Yang wrote:

> Signed-off-by: Li Yang <leoli@freescale.com>
> ---

We really should have a sentence about how or why this works to address 36-bit addressing.

> drivers/dma/fsldma.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index cea08be..9163552 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1,7 +1,7 @@
> /*
>  * Freescale MPC85xx, MPC83xx DMA Engine support
>  *
> - * Copyright (C) 2007 Freescale Semiconductor, Inc. All rights reserved.
> + * Copyright (C) 2007-2009 Freescale Semiconductor, Inc. All rights reserved.
>  *
>  * Author:
>  *   Zhang Wei <wei.zhang@freescale.com>, Jul 2007
> @@ -1338,6 +1338,7 @@ static int __devinit fsldma_of_probe(struct platform_device *op,
> 	fdev->common.device_control = fsl_dma_device_control;
> 	fdev->common.dev = &op->dev;
> 
> +	dma_set_mask(&op->dev, DMA_BIT_MASK(36));
> 	dev_set_drvdata(&op->dev, fdev);
> 
> 	/*
> -- 
> 1.6.6-rc1.GIT
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

On Tue, Sep 21, 2010 at 7:55 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
>
> On Sep 21, 2010, at 5:57 AM, Li Yang wrote:
>
>> Signed-off-by: Li Yang <leoli@freescale.com>
>> ---
>
> We really should have a sentence about how or why this works to address 36-bit addressing.

For example, I would like to know which memory is going to be
allocated above 4GB.  I don't know much about the kernel's async
library, but my understanding is that fsldma does not allocate any of
the memory buffers that it copies data to/from.  The only memory that
fsldma allocates is for the DMA descriptors, which are very small and
probably don't take up more than a couple pages.

On Tue, 21 Sep 2010 16:24:10 -0500
Timur Tabi <timur.tabi@gmail.com> wrote:

> On Tue, Sep 21, 2010 at 7:55 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> > On Sep 21, 2010, at 5:57 AM, Li Yang wrote:
> >
> >> Signed-off-by: Li Yang <leoli@freescale.com>
> >> ---
> >
> > We really should have a sentence about how or why this works to address 36-bit addressing.
> 
> For example, I would like to know which memory is going to be
> allocated above 4GB.  I don't know much about the kernel's async
> library, but my understanding is that fsldma does not allocate any of
> the memory buffers that it copies data to/from.

This doesn't control allocation (it probably should with
dma_alloc_coherent, though I don't see it in the code), it controls
whether swiotlb will create a bounce buffer -- defeating the point of
using DMA to accelerate a memcpy.

-Scott

On Tue, Sep 21, 2010 at 4:34 PM, Scott Wood <scottwood@freescale.com> wrote:

> This doesn't control allocation (it probably should with
> dma_alloc_coherent, though I don't see it in the code), it controls
> whether swiotlb will create a bounce buffer -- defeating the point of
> using DMA to accelerate a memcpy.

But it would do that only for the 'dev' used in the dma_set_mask()
call.  That dev is only used here:

	chan->desc_pool = dma_pool_create("fsl_dma_engine_desc_pool",
					  chan->dev,
					  sizeof(struct fsl_desc_sw),
					  __alignof__(struct fsl_desc_sw), 0);

Since we don't DMA the descriptors themselves, I just don't see how
this patch does anything.

On Tue, 21 Sep 2010 16:43:12 -0500
Timur Tabi <timur.tabi@gmail.com> wrote:

> On Tue, Sep 21, 2010 at 4:34 PM, Scott Wood <scottwood@freescale.com> wrote:
> 
> > This doesn't control allocation (it probably should with
> > dma_alloc_coherent, though I don't see it in the code), it controls
> > whether swiotlb will create a bounce buffer -- defeating the point of
> > using DMA to accelerate a memcpy.
> 
> But it would do that only for the 'dev' used in the dma_set_mask()
> call.  That dev is only used here:
> 
> 	chan->desc_pool = dma_pool_create("fsl_dma_engine_desc_pool",
> 					  chan->dev,
> 					  sizeof(struct fsl_desc_sw),
> 					  __alignof__(struct fsl_desc_sw), 0);
> 
> Since we don't DMA the descriptors themselves, I just don't see how
> this patch does anything.

Look in dmaengine.c, there are calls to dma_map_single() and
dma_map_page(), using what I assume is that same device pointer --
unless there's confusion between the channel and the controller.

-Scott

On Tue, Sep 21, 2010 at 4:49 PM, Scott Wood <scottwood@freescale.com> wrote:

> Look in dmaengine.c, there are calls to dma_map_single() and
> dma_map_page(), using what I assume is that same device pointer --
> unless there's confusion between the channel and the controller.

You're right.  I missed this line in the driver:

	fdev->common.dev = &op->dev;

Also, the driver does something stupid.  Sometimes "chan->dev" refers
to dma_chan.chan, and sometimes it refers to fsldma_chan.chan.  I
could have sworn I saw a patch that fixes that, though.

On Sep 21, 2010, at 2:49 PM, Scott Wood wrote:

>
> On Tue, 21 Sep 2010 16:43:12 -0500
> Timur Tabi <timur.tabi@gmail.com> wrote:
>
>> Since we don't DMA the descriptors themselves, I just don't see how
>> this patch does anything.
>
> Look in dmaengine.c, there are calls to dma_map_single() and
> dma_map_page(), using what I assume is that same device pointer --
> unless there's confusion between the channel and the controller.

The DMA descriptors are accessed using DMA by the
controller itself.  The APIs need to ensure proper coherency
between the CPU and the DMA controller for the
descriptor access.  The underlying implementation of the
API will depend upon the hardware capabilities that
ensure this coherency.

	-- Dan

On Tue, Sep 21, 2010 at 5:05 PM, Dan Malek <ppc6dev@digitaldans.com> wrote:

> The DMA descriptors are accessed using DMA by the
> controller itself.

Yes and no.  Technically, it is DMA, but it's not something that
SWIOTLB could ever know about.  We just pass the physical address to
the DMA controller, and it does a memory read to obtain the data.
That's not the kind of DMA that SWIOTLB would deal with, I think.

> The APIs need to ensure proper coherency
> between the CPU and the DMA controller for the
> descriptor access.  The underlying implementation of the
> API will depend upon the hardware capabilities that
> ensure this coherency.

I think that's already covered.  The dma_set_mask() call is supposed
to only affect the dma_map_single() calls that dmaengine makes, as
Scott pointed out.

My question is, should dmaengine be using the same 'dev' that fsldma
uses to allocate the DMA descriptors?  I wonder if the 'dev' should be
allocated internally by dmaengine, or provided by the client drivers.

On Tue, 21 Sep 2010 17:08:54 -0500
Timur Tabi <timur.tabi@gmail.com> wrote:

> On Tue, Sep 21, 2010 at 5:05 PM, Dan Malek <ppc6dev@digitaldans.com> wrote:
> 
> > The DMA descriptors are accessed using DMA by the
> > controller itself.
> 
> Yes and no.  Technically, it is DMA, but it's not something that
> SWIOTLB could ever know about.  We just pass the physical address to
> the DMA controller, and it does a memory read to obtain the data.
> That's not the kind of DMA that SWIOTLB would deal with, I think.

SWIOTLB should see that the address is directly DMAable and do nothing,
but you don't want to skip the DMA API altogether.  You still need to
flush caches if DMA is noncoherent, etc.

> My question is, should dmaengine be using the same 'dev' that fsldma
> uses to allocate the DMA descriptors?  I wonder if the 'dev' should be
> allocated internally by dmaengine, or provided by the client drivers.

It needs to be the actual device that is performing the DMA -- the
platform may need to do things such as IOMMU manipulation where
knowing the device matters.

-Scott

On Tue, Sep 21, 2010 at 5:17 PM, Scott Wood <scottwood@freescale.com> wrote:

> It needs to be the actual device that is performing the DMA -- the
> platform may need to do things such as IOMMU manipulation where
> knowing the device matters.

Ok, this all makes sense.  So it appears that the patch is valid, at
least in theory.  I would like to see some testing of it, but I
realize that may be too difficult.  There's no easy way to force an
allocation above 4GB.

On Sep 21, 2010, at 5:34 PM, Timur Tabi wrote:

> On Tue, Sep 21, 2010 at 5:17 PM, Scott Wood <scottwood@freescale.com> wrote:
> 
>> It needs to be the actual device that is performing the DMA -- the
>> platform may need to do things such as IOMMU manipulation where
>> knowing the device matters.
> 
> Ok, this all makes sense.  So it appears that the patch is valid, at
> least in theory.  I would like to see some testing of it, but I
> realize that may be too difficult.  There's no easy way to force an
> allocation above 4GB.

I think the patch is pretty safe w/o testing.  However I agree we need a better solution to testing 36-bit addressing.

- k

On Tue, Sep 21, 2010 at 10:41 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>
> On Sep 21, 2010, at 5:34 PM, Timur Tabi wrote:
>
>> On Tue, Sep 21, 2010 at 5:17 PM, Scott Wood <scottwood@freescale.com> wrote:
>>
>>> It needs to be the actual device that is performing the DMA -- the
>>> platform may need to do things such as IOMMU manipulation where
>>> knowing the device matters.
>>
>> Ok, this all makes sense.  So it appears that the patch is valid, at
>> least in theory.  I would like to see some testing of it, but I
>> realize that may be too difficult.  There's no easy way to force an
>> allocation above 4GB.
>
> I think the patch is pretty safe w/o testing.  However I agree we need a better solution to testing 36-bit addressing.

I'll take that as an acked-by, but I'll wait for the next version of
the patch with the completed changelog before acting on it.

--
Dan