Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command

[Linus Walleij <linus.walleij@linaro.org>]

On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
>>> PL330 has fixed channels to peripherals.
>>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>>> Client drivers shouldn't bother.
>>
>> That's utter crap, and isn't what the DMA engine API is about.
>>
>> The above looks correctly implemented.  Slave DMA engine users are
>> supposed to supply the device DMA register address via this
>> DMA_SLAVE_CONFIG call.  Doing this via platform data for the DMA
>> device is braindead.
>
> Rather than have 32 client drivers expect fifo_address from the
> platform and then
> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
> get 32 addresses
> from the platform.

My idea (when I implemented it) is that the device driver knows the
physical and virtual base and thus the address where DMA data is
destined. It knows all other registers, it remaps them, noone else should
be bothered with containing the knowledge of adress ranges for the
specific hardware block.

Passing this through platform data requires the machine to keep track
not only of the base adress of the peripheral (as is generally contained
in the machine or device tree) but also the offset of specific registers
in that memory region, which is too much.

Usually this means the offsets are defined in files like <mach/perfoo.h>
which means they can't be pushed down into drivers/foo/foo.c and
creates unnecessary bindings and de-encapsulation of the driver.
We want to get rid of such stuff. (I think?)

Therefore I introduced this to confine the different registers into
the driver itself and ask the DMA engine to transfer to a specific
address.

> Esp when the DMAC driver already expect similar h/w
> parameter -- *direction*.
>
> I don't understand why is 'fifo_address' a property much different
> than 'direction' of the
> channel ?

struct dma_slave_config {
        enum dma_data_direction direction;
        dma_addr_t src_addr;
        dma_addr_t dst_addr;
        enum dma_slave_buswidth src_addr_width;
        enum dma_slave_buswidth dst_addr_width;
        u32 src_maxburst;
        u32 dst_maxburst;
};

We do support changing direction as well as src and dst address
(i.e. FIFO endpoint) at runtime.

Check drivers/mmc/host/mmci.c for an example of how we switch
a single channel from TX to RX in runtime using this one property.

However it has been noted by Russell that the direction member
is unnecessary since the device_prep_slave_sg() function in the
dmaengine vtable already takes a direction argument like this:

        struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
                struct dma_chan *chan, struct scatterlist *sgl,
                unsigned int sg_len, enum dma_data_direction direction,
                unsigned long flags);

Therefore the direction setting needs to go from either the config
struct or the device_prep_slave_sg() call, as right now the channel
is being told about the direction twice which is not elegant and
confusing.

So you even have two ways of changing direction at runtime...

Yours,
Linus Walleij