lkml.org 
[lkml]   [2012]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()
On Wed, Mar 07, 2012 at 04:44:12PM +0100, Guennadi Liakhovetski wrote:
> So, the question remains: which way should we go? If we don't come up with
> a generic solution, I'd be inclined to just do something as silly as
>
> arch/arm/mach-shmobile/board-*.c
>
> static const struct device *group1_dma_dev[] = {
> &dma0.device,
> &dma1.device,
> };
>
> static const struct device *group2_dma_dev[] = {
> &dma2.device,
> &dma3.device,
> };
>
> static struct my_platform_data = {
> .dma_dev_list = group1_dma_dev,
> .dma_dev_list_num = ARRAY_SIZE(group1_dma_dev),
> };
>
> drivers/.../sh_*.c
>
> static bool filter(struct dma_chan *chan, void *arg)
> {
> struct device *dev = chan->dev->device.parent;
> struct my_platform_data *pdata = arg;
>
> for (i = 0; i < pdata->dma_dev_list_num; i++)
> if (pdata->dma_dev_list[i] == dev)
> return true;
>
> return false;
> }
>
> even though I find it silly to have to do this on every platform and in
> every driver.

Why are you thinking that the filter function implementation has to be
provided by the peripheral driver? That's just wrong and broken.

Think about it - how does the peripheral driver know what kind of dma
channel its filter function has been passed - to give an example, if
you built into your kernel support for the PL08x DMA engine, and lets
say you had PL08x DMA engine hardware, how would your filter function
decide whether it was one of your per-device channels or whether it
was a PL08x DMA engine channel?

And in the case of a PL08x DMA engine channel (which for slaves are
all virtual channels depending on the request signal desired), how
would your peripheral driver know which channel would be the right
one?

This is why DMA engine drivers such as PL08x and SA11x0 export their
own filter functions. The filter function will only match against a
channel owned by that exact driver - essentially this:

bool xxx_filter_id(struct dma_chan *chan, void *chan_id)
{
/* Reject channels for devices not bound to this driver */
if (chan->device->dev->driver != &xxx_driver)
return false;

... xxx device specific channel matching ...

return false;
}

This ensures that when the filter function needs to dereference the
driver private parts of struct dma_chan, it can be sure that the channel
really is being driven by this driver, and the underlying structure
behind struct dma_chan is well known at that point.

Anything which isn't driven by this driver is (as far as the filter
function is concerned) an unknown.

Once you know that the channel is owned by the right driver you can then
take whatever action is necessary to check - with type safety - whether
the channel is correct or not. You get type safety through two facts:

1. As described above, the filter function will reject any channel not
driven by this driver.
2. You need to pass the drivers filter function and data together into
the same API. Because both are together you have locality of
reference.

And... moreover, matching struct devices just would not fit for any DMA
engine I've worked with - that being PL08x, SA11x0 and OMAP. Neither
does, as I've said, using the dma_slave_config struct.

Let me put it another way:

1. using struct device would be wrong for these DMA engines because a
single DMA engine _device_ has multiple physical DMA channels, and
not only do these physical channels share registers and interrupts,
but they also share DMA requests.

2. the dma_slave_config struct can't be used to select a DMA engine or
a DMA channel because it does not contain information about whether
the DMA engine can access the peripheral, whether there's a link for
the device request signals to the DMA engine channel.

I will give you that channel matching in DMA engine stuff is far from
perfect, but what I'm seeing are attempts to optimize the channel
allocation for a different set of DMA devices at the expense of
penalising those already there.

We could pass a struct device corresponding with the desired DMA engine
and DMA request number in, but that still won't fit the PL08x driver,
where there's a platform specific hardware mux in front of a subset of
the PL08x's request signals.

So, what this is all telling me is that the _right_ interface is the one
we currently have, that being the filter function and an opaque set of
data to be interpreted by the supplied filter function. If you need to
match by a list of struct device, then the interface allows it. If you
need to match by string against a channel driven by a specific DMA engine
driver, the interface allows it. If you need to do something more
complicated, like one of N drivers, the interface allows it.

Hell, even with the PL08x and SA11x0 filter functions, lets say for
arguments sake that I had hardware with both these in, I could select
a channel like this:

bool pl08x_or_sa11x0_filter(struct dma_chan *chan, void *chan_id)
{
if (pl08x_filter_id(chan, chan_id))
return true;

return sa11x0_filter_id(chan, chan_id);
}

where both treat 'chan_id' as a string.


\
 
 \ /
  Last update: 2012-03-07 17:31    [W:0.130 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site