Message-ID: <AANLkTik1R-ZPpG5QibWuT1OhUPZr3i_Hr0LMW7D7s6zm@mail.gmail.com>
Date:	Thu, 23 Sep 2010 14:58:06 -0700
From:	Dan Williams <dan.j.williams@...el.com>
To:	"Ira W. Snyder" <iws@...o.caltech.edu>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver

On Thu, Sep 23, 2010 at 1:12 PM, Ira W. Snyder <iws@...o.caltech.edu> wrote:
> On Thu, Sep 23, 2010 at 11:31:11AM -0700, Dan Williams wrote:
>> On Tue, Sep 7, 2010 at 1:45 PM, Ira W. Snyder <iws@...o.caltech.edu> wrote:
>> > The DMA_SLAVE support functions all existed as static inlines in the
>> > driver specific header arch/powerpc/include/asm/fsldma.h. Move the body
>> > of the functions to the driver itself, and EXPORT_SYMBOL_GPL() them.
>> >
>> > At the same time, add the missing linux/list.h header.
>> >
>> > Signed-off-by: Ira W. Snyder <iws@...o.caltech.edu>
>> > ---
>> >
>> > Hello AKPM, Dan,
>>
>> Hi, I'm back from paternity leave.
>>
>> >
>> > At AKPM's request, I moved these static inline'd functions into the driver
>> > code itself, and then EXPORT_SYMBOL_GPL()'d them. I also added a gfp
>> > parameter to the fsl_dma_slave_alloc() function. And included the
>> > linux/list.h header.
>> >
>> > If you'd like three separate patches, I can do that. The changes were so
>> > trivial that I lumped all of them into a single patch.
>> >
>> > Thanks,
>> > Ira
>> >
>> >  arch/powerpc/include/asm/fsldma.h |   70 +++------------------------------
>> >  drivers/dma/fsldma.c              |   77 +++++++++++++++++++++++++++++++++++++
>>
>> Remind me why we need this header file and these routines again?  git
>> grep says nothing in-tree is using these?
>>
>> I also do not want to proliferate driver specific interfaces.  It
>> defeats the purpose of having a common dmaengine structure.  Along
>> these lines Linus W. added the new device_control() method (first
>> added in commit c3635c78, later tweaked in 05827630).  Could that be
>> massaged for your needs?
>>
>
> These routines are used by a (currently out of tree) driver that I
> maintain. Please see the LKML and linuxppc-dev ML post titled:
> [PATCH RFCv2 0/5] CARMA Board Support
>
> The FPGA programming driver uses these interfaces. Past a few initial
> comments (which I promptly fixed!), it seems that nobody wants to look
> at this driver. I don't know how to move it towards mainline other than
> spam people, which I would prefer to avoid doing.
>
> The new device_control() method doesn't have enough information for this
> purpose. I also don't know of any other DMA controllers that have the
> feature I'm using (described below).
>
> The fsldma controller has the option of being controlled by an external
> device, via some pins. This is leveraged by our system controller to
> properly program some big data processing FPGAs.
>
> Basically, the fsldma controller must be told:
> 1) descriptors (src, dst, len tuples)
> 2) use the external-start feature (external control)
> 3) go
>
> Now #3's "go" doesn't actually start the transfer. It just starts
> listening on the external pins for the correct sequence for each
> transaction.
>
> Also, the descriptors must be set up such that they form a single chain,
> without interruptions between segments. Back at the beginning of the
> year, I did a lot of work on the fsldma driver. It *should* be able to
> chain DMA transfers appropriately for this mode using the existing
> device_prep_dma_memcpy() routine.
>
> This means that it would be possible to move some of the functionality
> of fsldma's device_prep_slave_sg() into core DMAEngine code, and
> implement something like dma_memcpy_sg_to_sg(). This would take two
> scatterlists and call device_prep_slave_sg() enough times to setup a DMA
> operation that will move all data appropriately.
>
> This is exactly what fsldma's device_prep_slave_sg() actually does. It
> was non-trivial to get the triply nested loops correct.
>
> As an example from my driver, I have this scenario:
> 1) a scatterlist of kernel memory pages (this is my data payload)
> 2) a fixed set of non-incrementing physical addresses I need to send that data to
>
> Other than the fsldma device_prep_slave_sg(), this is difficult to do.
>
> I hope that helps to explain the problem.

Yes, thanks now I recall, and I remember saying something about how
this passes the "Voyager test".  That said it seems odd to carry
symbol exports for something like this.  Instead of exporting can we
exploit the fact that the fpga driver already knows it has a
fsldma_device pointer and do something like:

struct fsldma_device *fdev = container_of(dma, typeof(fdev), common);

fdev->slave_alloc();

This keeps it contained to the fsldma driver.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists